Re: [RFC PATCH v2 3/5] KVM: SVM: Move all AVIC setup to avic_hardware_setup()

[Sean Christopherson <seanjc@google.com>]

On Thu, Sep 04, 2025, Naveen N Rao (AMD) wrote:
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index cb4f81be0024..d5854e0bc799 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -162,7 +162,7 @@ module_param(tsc_scaling, int, 0444);
>   * enable / disable AVIC.  Because the defaults differ for APICv
>   * support between VMX and SVM we cannot use module_param_named.
>   */
> -static bool avic;
> +bool avic;

I'm all for consolidating module params and globals, but they should be grouped
by "area", i.e. the AVIC variables/params should grouped be in avic.c, not in svm.c.

>  module_param(avic, bool, 0444);
>  module_param(enable_ipiv, bool, 0444);
>  
> @@ -5406,7 +5406,7 @@ static __init int svm_hardware_setup(void)
>  			goto err;
>  	}
>  
> -	enable_apicv = avic = avic && avic_hardware_setup();
> +	avic_hardware_setup();


I would prefer to keep a mix of the old and new: move "avic" into avic.c, but
have avic_hardware_setup() return true/false and use it to set enable_apicv.

enable_apicv is tied to kvm.ko, and so I find it helpful to have it be set by
svm_hardware_setup(), e.g. so that when reading code in common x86, it's easier
to find where/when/how a global variable is configured by vendor code.  More
importantly, I don't want to create a hidden dependency between avic_hardware_setup()
setting enable_apicv and this code reading enable_apicv.

Alternatively, we could have avic_hardware_setup() clear the svm_x86_ops hooks,
but I wouldn't want to do that without having avic_hardware_setup() also _set_
the hooks, and I think doing that would be a big net negative since it would
make it harder to see the vcpu_(un)blocking = avic_vcpu_(un)blocking connection
for the soon-to-be-common case of AVIC being enabled.