Re: [PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC

[Sean Christopherson]

On Mon, May 24, 2021, Paolo Bonzini wrote:
> On 18/05/21 22:39, Sean Christopherson wrote:
> > > +/* enable / disable AVIC */
> > > +static int avic;
> > > +module_param(avic, int, 0444);
> > We should opportunistically make avic a "bool".
> > 
> 
> And also:
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 11714c22c9f1..48cb498ff070 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -185,9 +185,12 @@ module_param(vls, int, 0444);
>  static int vgif = true;
>  module_param(vgif, int, 0444);
> -/* enable / disable AVIC */
> -static int avic;
> -module_param(avic, 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;
> +module_param(avic, bool, 0444);
>  bool __read_mostly dump_invalid_vmcb;
>  module_param(dump_invalid_vmcb, bool, 0644);
> @@ -1013,11 +1016,7 @@ static __init int svm_hardware_setup(void)
>  			nrips = false;
>  	}
> -	if (!npt_enabled || !boot_cpu_has(X86_FEATURE_AVIC))
> -		avic = false;
> -
> -	/* 'enable_apicv' is common between VMX/SVM but the defaults differ */
> -	enable_apicv = avic;
> +	enable_apicv = avic && npt_enabled && boot_cpu_has(X86_FEATURE_AVIC);
>  	if (enable_apicv) {
>  		pr_info("AVIC enabled\n");
> 
> The "if" can come back when AVIC is enabled by default.

But "avic" is connected to the module param, even if it's off by default its
effective value should be reflected in sysfs.  E.g. the user may incorrectly
think AVIC is in use if they set avic=1 but the CPU doesn't support AVIC.
Forcing the user to check /proc/cpuinfo or look for "AVIC enabled" in dmesg is
kludgy at best.