Re: [PATCH v2 24/49] KVM: x86: #undef SPEC_CTRL_SSBD in cpuid.c to avoid macro collisions

[Maxim Levitsky]

On Mon, 2024-07-08 at 14:29 -0700, Sean Christopherson wrote:
> On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> > On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote:
> > > Undefine SPEC_CTRL_SSBD, which is #defined by msr-index.h to represent the
> > > enable flag in MSR_IA32_SPEC_CTRL, to avoid issues with the macro being
> > > unpacked into its raw value when passed to KVM's F() macro.  This will
> > > allow using multiple layers of macros in F() and friends, e.g. to harden
> > > against incorrect usage of F().
> > > 
> > > No functional change intended (cpuid.c doesn't consume SPEC_CTRL_SSBD).
> > > 
> > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > ---
> > >  arch/x86/kvm/cpuid.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > index 8efffd48cdf1..a16d6e070c11 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -639,6 +639,12 @@ static __always_inline void kvm_cpu_cap_init(u32 leaf, u32 mask)
> > >  	kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid);
> > >  }
> > >  
> > > +/*
> > > + * Undefine the MSR bit macro to avoid token concatenation issues when
> > > + * processing X86_FEATURE_SPEC_CTRL_SSBD.
> > > + */
> > > +#undef SPEC_CTRL_SSBD
> > > +
> > >  void kvm_set_cpu_caps(void)
> > >  {
> > >  	memset(kvm_cpu_caps, 0, sizeof(kvm_cpu_caps));
> > 
> > Hi,
> > 
> > Maybe we should instead rename the SPEC_CTRL_SSBD to
> > 'MSR_IA32_SPEC_CTRL_SSBD' and together with it, other fields of this msr.  It
> > seems that at least some msrs in this file do this.
> 
> Yeah, the #undef hack is quite ugly.  But I didn't (and still don't) want to
> introduce all the renaming churn in the middle of this already too-big series,
> especially since it would require touching quite a bit of code outside of KVM.



> 
> I'm also not sure that's the right thing to do; I kinda feel like KVM is the one
> that's being silly here.

I don't think that KVM is silly here. I think that hardware definitions like
MSRs, register names, register bit fields, etc, *must* come with a unique prefix,
it's not an issue of breaking some deeply nested macro, but rather an issue of readability.

SPEC_CTRL_SSBD for example won't mean much to someone who only knows ARM, while
MSR_SPEC_CTRL_SSBD, or even better IA32_MSR_SPEC_CTRL_SSBD, lets you instantly know
that this is a MSR, and anyone with even a bit of x86 knowledge should at least have
heard about what a MSR is.

In regard to X86_FEATURE_INTEL_SSBD, I don't oppose this idea, because we have
X86_FEATURE_AMD_SSBD, but in general I do oppose the idea of adding 'INTEL' prefix,
because it sets a not that good precedent, because most of the features on x86
are first done by Intel, but then are also implemented by AMD, and thus an intel-only
feature name can stick after it becomes a general x86 feature.

IN case of X86_FEATURE_INTEL_SSBD, we already have sadly different CPUID bits for
each vendor (although I wonder if AMD also sets the X86_FEATURE_INTEL_SSBD).

I vote to rename 'SPEC_CTRL_SSBD', it can be done as a standalone patch, and can
be accepted right now, even before this patch series is accepted.

Best regards,
	Maxim Levitsky


> 
> Aha!  Rather than rename the MSR bits, what if we rename the X86_FEATURE flag,
> e.g. to X86_FEATURE_INTEL_SPEC_CTRL_SSBD, X86_FEATURE_MSR_SPEC_CTRL_SSBD, or maybe
> even just X86_FEATURE_INTEL_SSBD.  Much less churn, and it would add even more
> clarity as to why there's also X86_FEATURE_SSBD and X86_FEATURE_AMD_SSBD.
> 
> I'll post a standalone patch to make that change, and maybe see if I can take it
> through the KVM tree.
>