Message-ID: <ZrFFua_7kWKBESbe@google.com>
Date: Mon, 5 Aug 2024 14:35:53 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: mlevitsk@...hat.com
Cc: Paolo Bonzini <pbonzini@...hat.com>, Vitaly Kuznetsov <vkuznets@...hat.com>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Hou Wenlong <houwenlong.hwl@...group.com>, 
	Kechen Lu <kechenl@...dia.com>, Oliver Upton <oliver.upton@...ux.dev>, 
	Binbin Wu <binbin.wu@...ux.intel.com>, Yang Weijiang <weijiang.yang@...el.com>, 
	Robert Hoo <robert.hoo.linux@...il.com>, Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH v2 24/49] KVM: x86: #undef SPEC_CTRL_SSBD in cpuid.c to
 avoid macro collisions

+Boris

On Mon, Aug 05, 2024, mlevitsk@...hat.com wrote:
> У пт, 2024-07-26 у 16:34 -0700, Sean Christopherson пише:
> > > On Wed, Jul 24, 2024, Maxim Levitsky wrote:
> > > > > On Mon, 2024-07-08 at 14:29 -0700, Sean Christopherson wrote:
> > > > > > > On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> > > > > > > > > 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.
> > > 
> > > For the MSR names themselves, yes, I agree 100%.  But for the bits and mask, I
> > > disagree.  It's simply too verbose, especially given that in the vast majority
> > > of cases simply looking at the surrounding code will provide enough context to
> > > glean an understanding of what's going on.
> 
> I am not that sure about this, especially if someone by mistake uses a flag
> that belong to one MSR, in some unrelated place. Verbose code is rarely a bad thing.
> 
> 
> > >   E.g. even for SPEC_CTRL_SSBD, where
> > > there's an absurd amount of magic and layering, looking at the #define makes
> > > it fairly obvious that it belongs to MSR_IA32_SPEC_CTRL.
> > > 
> > > And for us x86 folks, who obviously look at this code far more often than non-x86
> > > folks, I find it valuable to know that a bit/mask is exactly that, and _not_ an
> > > MSR index.  E.g. VMX_BASIC_TRUE_CTLS is a good example, where renaming that to
> > > MSR_VMX_BASIC_TRUE_CTLS would make it look too much like MSR_IA32_VMX_TRUE_ENTRY_CTLS
> > > and all the other "true" VMX MSRs.
> > > 
> > > > > 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,
> > > 
> > > Ya, those are my feelings exactly.  And in this case, since we already have an
> > > AMD variant, I think it's actually a net positive to add an INTEL variant so that
> > > it's clear that Intel and AMD ended up defining separate CPUID to enumerate the
> > > same basic info.
> > > 
> > > > > 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.
> > > 
> > > If we go that route, then we also need to rename nearly ever bit/mask definition
> > > in msr-index.h, otherwise SPEC_CTRL_* will be the odd ones out.  And as above, I
> > > don't think this is the right direction.
> 
> Honestly not really. If you look carefully at the file, many bits are already defined
> in the way I suggest, for example:
> 
> MSR_PLATFORM_INFO_CPUID_FAULT_BIT
> MSR_IA32_POWER_CTL_BIT_EE
> MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT
> MSR_AMD64_DE_CFG_LFENCE_SERIALIZE_BIT

Heh, I know there are some bits that have an "MSR" prefix, hence "nearly every".

> This file has all kind of names for both msrs and flags. There is not much
> order, so renaming the bit definitions of IA32_SPEC_CTRL won't increase the
> level of disorder in this file IMHO.

It depends on what direction msr-index.h is headed.  If the long-term preference
is to have bits/masks namespaced with only their associated MSR name, i.e. no
explicit MSR_, then renaming the bits is counter-productive.

I added Boris, who I believe was the most opinionated about the MSR bit names,
i.e. who can most likely give us the closest thing to an authoritative answer as
to the preferred style.

Boris, we're debating about the best way to solve a weird collision between:

  #define SPEC_CTRL_SSBD

and

  #define X86_FEATURE_SPEC_CTRL_SSBD

KVM wants to use its CPUID macros to essentially do:

  #define F(name) (X86_FEATURE_##name)

as a shorthand for X86_FEATURE_SPEC_CTRL_SSBD, but that can cause build failures
depending on how KVM's macros are layered.  E.g. SPEC_CTRL_SSBD can get resolved
to its value prior to token concatentation and result in KVM effectively generating
X86_FEATURE_BIT(SPEC_CTRL_SSBD_SHIFT).

One of the proposed solutions is to rename all of the SPEC_CTRL_* bit definitions
to add a MSR_ prefix, e.g. to generate MSR_SPEC_CTRL_SSBD and avoid the conflict.
My recollection from the IA32_FEATURE_CONTROL rework a few years back is that you
wanted to prioritize shorter names over having everything namespaced with MSR_,
i.e. that this approach is a non-starter.

Powered by blists - more mailing lists