Message-ID: <16658367af25852e4bb6abb0caf7c3bc58538db0.camel@redhat.com>
Date: Thu, 04 Jul 2024 22:43:49 -0400
From: Maxim Levitsky <mlevitsk@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini
 <pbonzini@...hat.com>,  Vitaly Kuznetsov <vkuznets@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, Hou Wenlong
 <houwenlong.hwl@...group.com>, Kechen Lu <kechenl@...dia.com>, Oliver Upton
 <oliver.upton@...ux.dev>, Binbin Wu <binbin.wu@...ux.intel.com>, Yang
 Weijiang <weijiang.yang@...el.com>, Robert Hoo <robert.hoo.linux@...il.com>
Subject: Re: [PATCH v2 48/49] KVM: x86: Add a macro for features that are
 synthesized into boot_cpu_data

On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote:
> Add yet another CPUID macro, this time for features that the host kernel
> synthesizes into boot_cpu_data, i.e. that the kernel force sets even in
> situations where the feature isn't reported by CPUID.  Thanks to the
> macro shenanigans of kvm_cpu_cap_init(), such features can now be handled
> in the core CPUID framework, i.e. don't need to be handled out-of-band and
> thus without as many guardrails.
> 
> Adding a dedicated macro also helps document what's going on, e.g. the
> calls to kvm_cpu_cap_check_and_set() are very confusing unless the reader
> knows exactly how kvm_cpu_cap_init() generates kvm_cpu_caps (and even
> then, it's far from obvious).
> 
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/cpuid.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 0130e0677387..0e64a6332052 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -106,6 +106,17 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
>  	F(name);						\
>  })
>  
> +/*
> + * Synthesized Feature - For features that are synthesized into boot_cpu_data,
> + * i.e. may not be present in the raw CPUID, but can still be advertised to
> + * userspace.  Primarily used for mitigation related feature flags.
> + */
> +#define SYN_F(name)						\
> +({								\
> +	kvm_cpu_cap_synthesized |= F(name);			\
> +	F(name);						\
> +})
> +
>  /*
>   * Aliased Features - For features in 0x8000_0001.EDX that are duplicates of
>   * identical 0x1.EDX features, and thus are aliased from 0x1 to 0x8000_0001.
> @@ -727,13 +738,15 @@ do {									\
>  	const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32);	\
>  	const u32 __maybe_unused kvm_cpu_cap_init_in_progress = leaf;	\
>  	u32 kvm_cpu_cap_emulated = 0;					\
> +	u32 kvm_cpu_cap_synthesized = 0;				\
>  									\
>  	if (leaf < NCAPINTS)						\
>  		kvm_cpu_caps[leaf] &= (mask);				\
>  	else								\
>  		kvm_cpu_caps[leaf] = (mask);				\
>  									\
> -	kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid);			\
> +	kvm_cpu_caps[leaf] &= (raw_cpuid_get(cpuid) |			\
> +			       kvm_cpu_cap_synthesized);		\
>  	kvm_cpu_caps[leaf] |= kvm_cpu_cap_emulated;			\
>  } while (0)
>  
> @@ -913,13 +926,10 @@ void kvm_set_cpu_caps(void)
>  	kvm_cpu_cap_init(CPUID_8000_0021_EAX,
>  		F(NO_NESTED_DATA_BP) | F(LFENCE_RDTSC) | 0 /* SmmPgCfgLock */ |
>  		F(NULL_SEL_CLR_BASE) | F(AUTOIBRS) | 0 /* PrefetchCtlMsr */ |
> -		F(WRMSR_XX_BASE_NS)
> +		F(WRMSR_XX_BASE_NS) | SYN_F(SBPB) | SYN_F(IBPB_BRTYPE) |
> +		SYN_F(SRSO_NO)
>  	);
>  
> -	kvm_cpu_cap_check_and_set(X86_FEATURE_SBPB);
> -	kvm_cpu_cap_check_and_set(X86_FEATURE_IBPB_BRTYPE);
> -	kvm_cpu_cap_check_and_set(X86_FEATURE_SRSO_NO);
> -
>  	kvm_cpu_cap_init(CPUID_8000_0022_EAX,
>  		F(PERFMON_V2)
>  	);


Hi,

Now that you added the final F_* macro, let's list all of them:


#define F(name)							\

/* Scattered Flag - For features that are scattered by cpufeatures.h. */
#define SF(name)						\



/* Features that KVM supports only on 64-bit kernels. */
#define X86_64_F(name)						\

/*
 * Raw Feature - For features that KVM supports based purely on raw host CPUID,
 * i.e. that KVM virtualizes even if the host kernel doesn't use the feature.
 * Simply force set the feature in KVM's capabilities, raw CPUID support will
 * be factored in by __kvm_cpu_cap_mask().
 */
#define RAW_F(name)						\

/*
 * Emulated Feature - For features that KVM emulates in software irrespective
 * of host CPU/kernel support.
 */
#define EMUL_F(name)						\

/*
 * Synthesized Feature - For features that are synthesized into boot_cpu_data,
 * i.e. may not be present in the raw CPUID, but can still be advertised to
 * userspace.  Primarily used for mitigation related feature flags.
 */
#define SYN_F(name)						\

/*
 * Aliased Features - For features in 0x8000_0001.EDX that are duplicates of
 * identical 0x1.EDX features, and thus are aliased from 0x1 to 0x8000_0001.
 */
#define AF(name)								\

/*
 * VMM Features - For features that KVM "supports" in some capacity, i.e. that
 * KVM may query, but that are never advertised to userspace.  E.g. KVM allows
 * userspace to enumerate MONITOR+MWAIT support to the guest, but the MWAIT
 * feature flag is never advertised to userspace because MONITOR+MWAIT aren't
 * virtualized by hardware, can't be faithfully emulated in software (KVM
 * emulates them as NOPs), and allowing the guest to execute them natively
 * requires enabling a per-VM capability.
 */
#define VMM_F(name)								\


Honestly, I already somewhat lost in what each of those macros means even when reading
the comments, which might indicate that a future reader might also have a 
hard time understanding those.

I now support even more the case of setting each feature bit in a separate statement
as I explained in an earlier patch.

What do you think?


Best regards,
	Maxim Levitsky

Powered by blists - more mailing lists