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

[Sean Christopherson]

() 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@xxxxxxx>
> ---
>  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) |