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. 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.
