Messages in this thread | | | Subject | Re: [PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC | From | Paolo Bonzini <> | Date | Mon, 24 May 2021 19:02:18 +0200 |
| |
On 24/05/21 18:52, Sean Christopherson wrote: > 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.
Indeed -- I even tested the above, but only before realizing that module_param_named would change the default. So for now this needs to be "enable_apicv = avic = ...", and later it can become just
enable_apicv &= npt_enabled && boot_cpu_has(X86_FEATURE_AVIC);
Paolo
| |