On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote:
> Now that kvm_cpu_cap_init() is a macro with its own scope, add EMUL_F() to
> OR-in features that KVM emulates in software, i.e. that don't depend on
> the feature being available in hardware. The contained scope
> of kvm_cpu_cap_init() allows using a local variable to track the set of
> emulated leaves, which in addition to avoiding confusing and/or
> unnecessary variables, helps prevent misuse of EMUL_F().
>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> arch/x86/kvm/cpuid.c | 36 +++++++++++++++++++++---------------
> 1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 1064e4d68718..33e3e77de1b7 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -94,6 +94,16 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
> F(name); \
> })
>
> +/*
> + * Emulated Feature - For features that KVM emulates in software irrespective
> + * of host CPU/kernel support.
> + */
> +#define EMUL_F(name) \
> +({ \
> + kvm_cpu_cap_emulated |= F(name); \
> + F(name); \
> +})
To me it feels more and more that this patch series doesn't go into the right direction.
How about we just abandon the whole concept of masks and instead just have a list of statements
Pretty much the opposite of the patch series I confess:
#define CAP_PASSTHOUGH 0x01
#define CAP_EMULATED 0x02
#define CAP_AMD_ALIASED 0x04 // for AMD aliased features
#define CAP_SCATTERED 0x08
#define CAP_X86_64 0x10 // supported only on 64 bit hypervisors
...
/* CPUID_1_ECX*/
/* TMA is not passed though because: xyz*/
kvm_cpu_cap_init(TMA, 0);
kvm_cpu_cap_init(SSSE3, CAP_PASSTHOUGH);
/* CNXT_ID is not passed though because: xyz*/
kvm_cpu_cap_init(CNXT_ID, 0);
kvm_cpu_cap_init(RESERVED, 0);
kvm_cpu_cap_init(FMA, CAP_PASSTHOUGH);
...
/* KVM always emulates TSC_ADJUST */
kvm_cpu_cap_init(TSC_ADJUST, CAP_EMULATED | CAP_SCATTERED);
...
/* CPUID_D_1_EAX*/
/* XFD is disabled on 32 bit systems because: xyz*/
kvm_cpu_cap_init(XFD, CAP_PASSTHOUGH | CAP_X86_64)
'kvm_cpu_cap_init' can be a macro if needed to have the compile checks.
There are several advantages to this:
- more readability, plus if needed each statement can be amended with a comment.
- No weird hacks in 'F*' macros, which additionally eventually evaluate into a bit, which is confusing.
In fact no need to even have them at all.
- No need to verify that bitmask belongs to a feature word.
- Merge friendly - each capability has its own line.
Disadvantages:
- Longer list - IMHO not a problem, since it is very easy to read / search
and can have as much comments as needed.
For example this is how the kernel lists the CPUID features and this list IMHO
is very manageable.
- Slower - kvm_set_cpu_caps is called exactly once per KVM module load, thus
performance is the last thing I would care about in this function.
Another note about this patch: It is somewhat confusing that EMUL_F just forces a feature in kvm caps,
regardless of CPU support, because KVM also has KVM_GET_EMULATED_CPUID and it has a different meaning.
Users can easily confuse the EMUL_F for something that sets a feature bit in the KVM_GET_EMULATED_CPUID.
Best regards,
Maxim Levitsky
> +
> /*
> * 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.
> @@ -649,6 +659,7 @@ do { \
> 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; \
> \
> if (leaf < NCAPINTS) \
> kvm_cpu_caps[leaf] &= (mask); \
> @@ -656,6 +667,7 @@ do { \
> kvm_cpu_caps[leaf] = (mask); \
> \
> kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid); \
> + kvm_cpu_caps[leaf] |= kvm_cpu_cap_emulated; \
> } while (0)
>
> /*
> @@ -684,12 +696,10 @@ void kvm_set_cpu_caps(void)
> 0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ |
> F(FMA) | F(CX16) | 0 /* xTPR Update */ | F(PDCM) |
> F(PCID) | 0 /* Reserved, DCA */ | F(XMM4_1) |
> - F(XMM4_2) | F(X2APIC) | F(MOVBE) | F(POPCNT) |
> + F(XMM4_2) | EMUL_F(X2APIC) | F(MOVBE) | F(POPCNT) |
> 0 /* Reserved*/ | F(AES) | F(XSAVE) | 0 /* OSXSAVE */ | F(AVX) |
> F(F16C) | F(RDRAND)
> );
> - /* KVM emulates x2apic in software irrespective of host support. */
> - kvm_cpu_cap_set(X86_FEATURE_X2APIC);
>
> kvm_cpu_cap_init(CPUID_1_EDX,
> F(FPU) | F(VME) | F(DE) | F(PSE) |
> @@ -703,13 +713,13 @@ void kvm_set_cpu_caps(void)
> );
>
> kvm_cpu_cap_init(CPUID_7_0_EBX,
> - F(FSGSBASE) | F(SGX) | F(BMI1) | F(HLE) | F(AVX2) |
> - F(FDP_EXCPTN_ONLY) | F(SMEP) | F(BMI2) | F(ERMS) | F(INVPCID) |
> - F(RTM) | F(ZERO_FCS_FDS) | 0 /*MPX*/ | F(AVX512F) |
> - F(AVX512DQ) | F(RDSEED) | F(ADX) | F(SMAP) | F(AVX512IFMA) |
> - F(CLFLUSHOPT) | F(CLWB) | 0 /*INTEL_PT*/ | F(AVX512PF) |
> - F(AVX512ER) | F(AVX512CD) | F(SHA_NI) | F(AVX512BW) |
> - F(AVX512VL));
> + F(FSGSBASE) | EMUL_F(TSC_ADJUST) | F(SGX) | F(BMI1) | F(HLE) |
> + F(AVX2) | F(FDP_EXCPTN_ONLY) | F(SMEP) | F(BMI2) | F(ERMS) |
> + F(INVPCID) | F(RTM) | F(ZERO_FCS_FDS) | 0 /*MPX*/ |
> + F(AVX512F) | F(AVX512DQ) | F(RDSEED) | F(ADX) | F(SMAP) |
> + F(AVX512IFMA) | F(CLFLUSHOPT) | F(CLWB) | 0 /*INTEL_PT*/ |
> + F(AVX512PF) | F(AVX512ER) | F(AVX512CD) | F(SHA_NI) |
> + F(AVX512BW) | F(AVX512VL));
>
> kvm_cpu_cap_init(CPUID_7_ECX,
> F(AVX512VBMI) | RAW_F(LA57) | F(PKU) | 0 /*OSPKE*/ | F(RDPID) |
> @@ -728,16 +738,12 @@ void kvm_set_cpu_caps(void)
>
> kvm_cpu_cap_init(CPUID_7_EDX,
> F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) |
> - F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(INTEL_STIBP) |
> + F(SPEC_CTRL_SSBD) | EMUL_F(ARCH_CAPABILITIES) | F(INTEL_STIBP) |
> F(MD_CLEAR) | F(AVX512_VP2INTERSECT) | F(FSRM) |
> F(SERIALIZE) | F(TSXLDTRK) | F(AVX512_FP16) |
> F(AMX_TILE) | F(AMX_INT8) | F(AMX_BF16) | F(FLUSH_L1D)
> );
>
> - /* TSC_ADJUST and ARCH_CAPABILITIES are emulated in software. */
> - kvm_cpu_cap_set(X86_FEATURE_TSC_ADJUST);
> - kvm_cpu_cap_set(X86_FEATURE_ARCH_CAPABILITIES);
> -
> if (boot_cpu_has(X86_FEATURE_IBPB) && boot_cpu_has(X86_FEATURE_IBRS))
> kvm_cpu_cap_set(X86_FEATURE_SPEC_CTRL);
> if (boot_cpu_has(X86_FEATURE_STIBP))
