Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
MIME-Version: 1.0
References: <20231024001636.890236-1-jmattson@google.com> <20231024001636.890236-2-jmattson@google.com>
 <ZTcO8M3T9DGYrN2M@google.com> <ZWjwV7rQ9i2NCf5A@google.com>
In-Reply-To: <ZWjwV7rQ9i2NCf5A@google.com>
From:   Jim Mattson <jmattson@google.com>
Date:   Thu, 30 Nov 2023 17:39:25 -0800
Message-ID: <CALMp9eSwtVZQ_aeQ0itT7O5fDNznA2vQ0zT32VkyZS2-Q6eYZQ@mail.gmail.com>
Subject: Re: [PATCH 2/2] KVM: x86: Use a switch statement in __feature_translate()
To:     Sean Christopherson <seanjc@google.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Nov 30, 2023 at 12:28=E2=80=AFPM Sean Christopherson <seanjc@google=
.com> 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 translat=
ion 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@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 runti=
me
>     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 =3D=3D X86_FEATURE_SGX1)
>                     return KVM_X86_FEATURE_SGX1;
>             else if (x86_feature =3D=3D X86_FEATURE_SGX2)
>                     return KVM_X86_FEATURE_SGX1;
>
>     and so instead reverse_cpuid_check() will fail due to the untranslate=
d
>     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_as=
sert_450 declared with 'error' attribute:
>                                           BUILD_BUG_ON failed: x86_leaf =
=3D=3D CPUID_LNX_4
>               BUILD_BUG_ON(x86_leaf =3D=3D CPUID_LNX_4);
>               ^
>     whereas duplicate case statements very explicitly point at the offend=
ing
>     code:
>
>       arch/x86/kvm/reverse_cpuid.h:125:2: error: duplicate case value '36=
1'
>               KVM_X86_TRANSLATE_FEATURE(SGX2);
>               ^
>       arch/x86/kvm/reverse_cpuid.h:124:2: error: duplicate case value '36=
0'
>               KVM_X86_TRANSLATE_FEATURE(SGX1);
>               ^
>
>     And without macros, the opposite type of copy+paste goof doesn't gene=
rate
>     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 kn=
own
>     at compile-time, so the code generation between an if-elif sequence a=
nd a
>     switch statement should be identical.
>
>     Signed-off-by: Jim Mattson <jmattson@google.com>
>     Link: https://lore.kernel.org/r/20231024001636.890236-2-jmattson@goog=
le.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(uns=
igned int x86_leaf)
>   */
>  static __always_inline u32 __feature_translate(int x86_feature)
>  {
> -       if (x86_feature =3D=3D X86_FEATURE_SGX1)
> -               return KVM_X86_FEATURE_SGX1;
> -       else if (x86_feature =3D=3D X86_FEATURE_SGX2)
> -               return KVM_X86_FEATURE_SGX2;
> -       else if (x86_feature =3D=3D X86_FEATURE_SGX_EDECCSSA)
> -               return KVM_X86_FEATURE_SGX_EDECCSSA;
> -       else if (x86_feature =3D=3D X86_FEATURE_CONSTANT_TSC)
> -               return KVM_X86_FEATURE_CONSTANT_TSC;
> -       else if (x86_feature =3D=3D X86_FEATURE_PERFMON_V2)
> -               return KVM_X86_FEATURE_PERFMON_V2;
> -       else if (x86_feature =3D=3D 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)
>