linux-kernel - Re: [PATCH v2 3/5] KVM: x86: Use common 'enable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YKvZ6vI2vFVmkCeb@google.com>
Date:   Mon, 24 May 2021 16:52:58 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>, kvm@...r.kernel.org,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Maxim Levitsky <mlevitsk@...hat.com>,
        Kechen Lu <kechenl@...dia.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for
 both APICv and AVIC

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.