On Thu, Nov 30, 2023 at 12:28 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > On Mon, Oct 23, 2023, Sean Christopherson wrote: > > On Mon, Oct 23, 2023, Jim Mattson wrote: > > > The compiler will probably do better than linear search. > > > > It shouldn't matter, KVM relies on the compiler to resolve the translation at > > compile time, e.g. the result is fed into reverse_cpuid_check(). > > > > I.e. we should pick whatever is least ugly. > > What if we add a macro to generate each case statement? It's arguably a wee bit > more readable, and also eliminates the possibility of returning the wrong feature > due to copy+paste errors, e.g. nothing would break at compile time if we goofed > and did: > > case X86_FEATURE_SGX1: > return KVM_X86_FEATURE_SGX1; > case X86_FEATURE_SGX2: > return KVM_X86_FEATURE_SGX1; > > If you've no objection, I'll push this: :barf: Um, okay. > -- > Author: Jim Mattson <jmattson@xxxxxxxxxx> > Date: Mon Oct 23 17:16:36 2023 -0700 > > KVM: x86: Use a switch statement and macros in __feature_translate() > > Use a switch statement with macro-generated case statements to handle > translating feature flags in order to reduce the probability of runtime > errors due to copy+paste goofs, to make compile-time errors easier to > debug, and to make the code more readable. > > E.g. the compiler won't directly generate an error for duplicate if > statements > > if (x86_feature == X86_FEATURE_SGX1) > return KVM_X86_FEATURE_SGX1; > else if (x86_feature == X86_FEATURE_SGX2) > return KVM_X86_FEATURE_SGX1; > > and so instead reverse_cpuid_check() will fail due to the untranslated > entry pointing at a Linux-defined leaf, which provides practically no > hint as to what is broken > > arch/x86/kvm/reverse_cpuid.h:108:2: error: call to __compiletime_assert_450 declared with 'error' attribute: > BUILD_BUG_ON failed: x86_leaf == CPUID_LNX_4 > BUILD_BUG_ON(x86_leaf == CPUID_LNX_4); > ^ > whereas duplicate case statements very explicitly point at the offending > code: > > arch/x86/kvm/reverse_cpuid.h:125:2: error: duplicate case value '361' > KVM_X86_TRANSLATE_FEATURE(SGX2); > ^ > arch/x86/kvm/reverse_cpuid.h:124:2: error: duplicate case value '360' > KVM_X86_TRANSLATE_FEATURE(SGX1); > ^ > > And without macros, the opposite type of copy+paste goof doesn't generate > any error at compile-time, e.g. this yields no complaints: > > case X86_FEATURE_SGX1: > return KVM_X86_FEATURE_SGX1; > case X86_FEATURE_SGX2: > return KVM_X86_FEATURE_SGX1; > > Note, __feature_translate() is forcibly inlined and the feature is known > at compile-time, so the code generation between an if-elif sequence and a > switch statement should be identical. > > Signed-off-by: Jim Mattson <jmattson@xxxxxxxxxx> > Link: https://lore.kernel.org/r/20231024001636.890236-2-jmattson@xxxxxxxxxx > [sean: use a macro, rewrite changelog] > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx> > > diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h > index 17007016d8b5..aadefcaa9561 100644 > --- a/arch/x86/kvm/reverse_cpuid.h > +++ b/arch/x86/kvm/reverse_cpuid.h > @@ -116,20 +116,19 @@ static __always_inline void reverse_cpuid_check(unsigned int x86_leaf) > */ > static __always_inline u32 __feature_translate(int x86_feature) > { > - if (x86_feature == X86_FEATURE_SGX1) > - return KVM_X86_FEATURE_SGX1; > - else if (x86_feature == X86_FEATURE_SGX2) > - return KVM_X86_FEATURE_SGX2; > - else if (x86_feature == X86_FEATURE_SGX_EDECCSSA) > - return KVM_X86_FEATURE_SGX_EDECCSSA; > - else if (x86_feature == X86_FEATURE_CONSTANT_TSC) > - return KVM_X86_FEATURE_CONSTANT_TSC; > - else if (x86_feature == X86_FEATURE_PERFMON_V2) > - return KVM_X86_FEATURE_PERFMON_V2; > - else if (x86_feature == X86_FEATURE_RRSBA_CTRL) > - return KVM_X86_FEATURE_RRSBA_CTRL; > +#define KVM_X86_TRANSLATE_FEATURE(f) \ > + case X86_FEATURE_##f: return KVM_X86_FEATURE_##f > > - return x86_feature; > + switch (x86_feature) { > + KVM_X86_TRANSLATE_FEATURE(SGX1); > + KVM_X86_TRANSLATE_FEATURE(SGX2); > + KVM_X86_TRANSLATE_FEATURE(SGX_EDECCSSA); > + KVM_X86_TRANSLATE_FEATURE(CONSTANT_TSC); > + KVM_X86_TRANSLATE_FEATURE(PERFMON_V2); > + KVM_X86_TRANSLATE_FEATURE(RRSBA_CTRL); > + default: > + return x86_feature; > + } > } > > static __always_inline u32 __feature_leaf(int x86_feature) >
