Message-ID: <ffa76b1b62c5cd2001f5f313009376e131bc2817.camel@redhat.com>
Date: Mon, 05 Aug 2024 14:11:42 +0300
From: mlevitsk@...hat.com
To: Sean Christopherson <seanjc@...gle.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>
Subject: Re: [PATCH v2 24/49] KVM: x86: #undef SPEC_CTRL_SSBD in cpuid.c to
 avoid macro collisions

У пт, 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


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.


Best regards,
	Maxim Levitsky



> > 

Powered by blists - more mailing lists