| Message ID | 20240517173926.965351-25-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: CPUID overhaul, fixes, and caching | expand |
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@google.com> > --- > 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. Best regards, Maxim Levitsky
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@google.com> > > --- > > 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. 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.
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@google.com> > > > --- > > > 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. >
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.
У пт, 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 > >
+Boris On Mon, Aug 05, 2024, mlevitsk@redhat.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.
On Mon, 2024-08-05 at 14:35 -0700, Sean Christopherson wrote: > +Boris > > On Mon, Aug 05, 2024, mlevitsk@redhat.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. > Hi, Any update on this? Best regards, Maxim Levitsky
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));
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@google.com> --- arch/x86/kvm/cpuid.c | 6 ++++++ 1 file changed, 6 insertions(+)