Re: [PATCH 1/3] KVM: x86: hyper-v: Convert synic_auto_eoi_used to an atomic

[Naveen N Rao]

Hi Maxim,

On Mon, Feb 03, 2025 at 08:30:13PM -0500, Maxim Levitsky wrote:
> On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote:
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index 6a6dd5a84f22..7a4554ea1d16 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -131,25 +131,18 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> >  	if (auto_eoi_old == auto_eoi_new)
> >  		return;
> >  
> > -	if (!enable_apicv)
> > -		return;
> > -
> > -	down_write(&vcpu->kvm->arch.apicv_update_lock);
> > -
> >  	if (auto_eoi_new)
> > -		hv->synic_auto_eoi_used++;
> > +		atomic_inc(&hv->synic_auto_eoi_used);
> >  	else
> > -		hv->synic_auto_eoi_used--;
> > +		atomic_dec(&hv->synic_auto_eoi_used);
> >  
> >  	/*
> >  	 * Inhibit APICv if any vCPU is using SynIC's AutoEOI, which relies on
> >  	 * the hypervisor to manually inject IRQs.
> >  	 */
> > -	__kvm_set_or_clear_apicv_inhibit(vcpu->kvm,
> > -					 APICV_INHIBIT_REASON_HYPERV,
> > -					 !!hv->synic_auto_eoi_used);
> > -
> > -	up_write(&vcpu->kvm->arch.apicv_update_lock);
> > +	kvm_set_or_clear_apicv_inhibit(vcpu->kvm,
> > +				       APICV_INHIBIT_REASON_HYPERV,
> > +				       !!atomic_read(&hv->synic_auto_eoi_used));
> 
> Hi,
> 
> This introduces a race, because there is a race window between
> the moment we read hv->synic_auto_eoi_used, and decide to set/clear the inhibit.
> 
> After we read hv->synic_auto_eoi_used, but before we call the kvm_set_or_clear_apicv_inhibit,
> other core might also run synic_update_vector and change hv->synic_auto_eoi_used, 
> finish setting the inhibit in kvm_set_or_clear_apicv_inhibit,
> and only then we will call kvm_set_or_clear_apicv_inhibit with the stale value of hv->synic_auto_eoi_used and clear it.

Ah, indeed. Thanks for the explanation.

I wonder if we can switch to using kvm_hv->hv_lock in place of 
apicv_update_lock. That lock is already used to guard updates to 
partition-wide MSRs in kvm_hv_set_msr_common(). So, that might be ok 
too?


Thanks,
Naveen