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

[Maxim Levitsky]

On Mon, 2025-02-03 at 22:33 +0530, Naveen N Rao (AMD) wrote:
> apicv_update_lock is primarily meant for protecting updates to the apicv
> state, and is not necessary for guarding updates to synic_auto_eoi_used.
> Convert synic_auto_eoi_used to an atomic and use
> kvm_set_or_clear_apicv_inhibit() helper to simplify the logic.
> 
> Signed-off-by: Naveen N Rao (AMD) <naveen@xxxxxxxxxx>
> ---
>  arch/x86/include/asm/kvm_host.h |  7 ++-----
>  arch/x86/kvm/hyperv.c           | 17 +++++------------
>  2 files changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 5193c3dfbce1..fb93563714c2 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1150,11 +1150,8 @@ struct kvm_hv {
>  	/* How many vCPUs have VP index != vCPU index */
>  	atomic_t num_mismatched_vp_indexes;
>  
> -	/*
> -	 * How many SynICs use 'AutoEOI' feature
> -	 * (protected by arch.apicv_update_lock)
> -	 */
> -	unsigned int synic_auto_eoi_used;
> +	/* How many SynICs use 'AutoEOI' feature */
> +	atomic_t synic_auto_eoi_used;
>  
>  	struct kvm_hv_syndbg hv_syndbg;
>  
> 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.

IMHO, knowing that this code is mostly a precaution and that modern windows doesn't use AutoEOI
(at least when AutoEOI deprecation bit is set), instead of counting, we can unconditionally
inhibit the APICv when the guest attempts to use AutoEOI once.
But as usual I won't be surprised that this breaks *some* old and/or odd windows versions.

Best regards,
	Maxim Levitsky





>  }
>  
>  static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,