linux-kernel - Re: [PATCH v3 4/7] x86/cpu, kvm: Move CPUID 0x80000021 EAX feature bits propagation to kvm_set_cpu

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]

Date:   Wed, 30 Nov 2022 00:13:09 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Kim Phillips <kim.phillips@....com>
Cc:     x86@...nel.org, Babu Moger <Babu.Moger@....com>,
        Borislav Petkov <bp@...en8.de>, Borislav Petkov <bp@...e.de>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
        Joao Martins <joao.m.martins@...cle.com>,
        Jonathan Corbet <corbet@....net>,
        Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        David Woodhouse <dwmw@...zon.co.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Juergen Gross <jgross@...e.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Tony Luck <tony.luck@...el.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Alexey Kardashevskiy <aik@....com>, kvm@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/7] x86/cpu, kvm: Move CPUID 0x80000021 EAX feature
 bits propagation to kvm_set_cpu_caps

() after function names, i.e. kvm_set_cpu_caps().

On Tue, Nov 29, 2022, Kim Phillips wrote:
> Since they're now all scattered, group CPUID 0x80000021 EAX feature bits

Nit, scattering feature bits isn't required to use KVM's reverse CPUID magic,
e.g. see commit 047c72299061 ("KVM: x86: Update KVM-only leaf handling to allow
for 100% KVM-only leafs") that's sitting in kvm/queue.

The real justification for this patch is that open coding numbers is error prone
and is very frowned upon in KVM.

> propagation to kvm_set_cpu_caps instead of open-coding them in

kvm_set_cpu_caps()

> __do_cpuid_func().
> 
> Signed-off-by: Kim Phillips <kim.phillips@....com>
> ---
>  arch/x86/kvm/cpuid.c         | 35 ++++++++++++++++++++---------------
>  arch/x86/kvm/reverse_cpuid.h | 22 ++++++++++++++++------
>  2 files changed, 36 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index c92c49a0b35b..8e37760cea1b 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -730,6 +730,25 @@ void kvm_set_cpu_caps(void)
>  		0 /* SME */ | F(SEV) | 0 /* VM_PAGE_FLUSH */ | F(SEV_ES) |
>  		F(SME_COHERENT));
>  
> +	/*
> +	 * Pass down these bits:
> +	 *    EAX      0      NNDBP, Processor ignores nested data breakpoints
> +	 *    EAX      2      LAS, LFENCE always serializing
> +	 *    EAX      6      NSCB, Null selector clear base
> +	 *    EAX      8      Automatic IBRS

Automatic IBRS isn't advertised as of this patch.  Just drop the comment, it's
guaranteed to become stale at some point, and one of the main reasons for the
flag magic is so that the code is self-documenting, i.e. so that we don't need
comments like this.

> +	 *
> +	 * Other defined bits are for MSRs that KVM does not expose:
> +	 *   EAX      3      SPCL, SMM page configuration lock
> +	 *   EAX      13     PCMSR, Prefetch control MSR
> +	 */
> +	kvm_cpu_cap_init_scattered(CPUID_8000_0021_EAX,
> +				   SF(NO_NESTED_DATA_BP) | SF(LFENCE_RDTSC) |
> +				   SF(NULL_SEL_CLR_BASE));

Please follow the established style, e.g.

	kvm_cpu_cap_init_scattered(CPUID_8000_0021_EAX,                         
		SF(NO_NESTED_DATA_BP) | SF(LFENCE_RDTSC) | SF(NULL_SEL_CLR_BASE)
	);

> +	if (cpu_feature_enabled(X86_FEATURE_LFENCE_RDTSC))

I highly doubt it matters, but using cpu_feature_enabled() instead of static_cpu_has()
is an unrelated change.  At the very least, it should be mentioned in the changelog.

> +		kvm_cpu_cap_set(X86_FEATURE_LFENCE_RDTSC);
> +	if (!static_cpu_has_bug(X86_BUG_NULL_SEG))
> +		kvm_cpu_cap_set(X86_FEATURE_NULL_SEL_CLR_BASE);
> +
>  	kvm_cpu_cap_mask(CPUID_C000_0001_EDX,
>  		F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) |
>  		F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) |