Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Thu, 30 Nov 2023 12:28:07 -0800
In-Reply-To: <ZTcO8M3T9DGYrN2M@google.com>
Mime-Version: 1.0
References: <20231024001636.890236-1-jmattson@google.com> <20231024001636.890236-2-jmattson@google.com>
 <ZTcO8M3T9DGYrN2M@google.com>
Message-ID: <ZWjwV7rQ9i2NCf5A@google.com>
Subject: Re: [PATCH 2/2] KVM: x86: Use a switch statement in __feature_translate()
From:   Sean Christopherson <seanjc@google.com>
To:     Jim Mattson <jmattson@google.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        "'Paolo Bonzini '" <pbonzini@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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:

--
Author: Jim Mattson <jmattson@google.com>
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@google.com>
    Link: https://lore.kernel.org/r/20231024001636.890236-2-jmattson@google.com
    [sean: use a macro, rewrite changelog]
    Signed-off-by: Sean Christopherson <seanjc@google.com>

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)