KVM: x86: X86_FEATURE bit() cleanup
cover KVM: x86: X86_FEATURE bit() cleanup
Small series to add build-time protections on reverse CPUID lookup (and other usages of bit()), and to rename the misleading-generic bit() helper to something that better conveys its purpose.
Sean Christopherson (2):
KVM: x86: Add build-time assertion on usage of bit()
KVM: x86: Refactor and rename bit() to feature_bit() macro
arch/x86/kvm/cpuid.c | 2 +- arch/x86/kvm/cpuid.h | 4 ++-- arch/x86/kvm/emulate.c | 8 +++----- arch/x86/kvm/svm.c | 4 ++-- arch/x86/kvm/vmx/vmx.c | 42 +++++++++++++++++++++--------------------- arch/x86/kvm/x86.h | 24 ++++++++++++++++++++++-- 6 files changed, 51 insertions(+), 33 deletions(-)
Signature · 1 line
2.24.0
PATCH 1/2 KVM: x86: Add build-time assertion on usage of bit()
Add build-time checks to ensure KVM isn't trying to do a reverse CPUID lookup on Linux-defined feature bits, along with comments to explain the gory details of X86_FEATUREs and bit().
---
Note, the premature newline in the first line of the second comment is intentional to reduce churn in the next patch.
arch/x86/kvm/x86.h | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
arch/x86/kvm/x86.h +21 -2
@@ -144,9 +144,28 @@ static inline bool is_pae_paging(struct kvm_vcpu *vcpu)return !is_long_mode (vcpu) &&is_pae (vcpu) &&is_paging (vcpu); } -static inline u32 bit (int bitno) +/* +* Retrieve the bit mask from an X86_FEATURE_* definition. Features contain +* the hardware defined bit number (stored in bits 4:0) and a software defined +* "word" (stored in bits 31:5). The word is used to index into arrays of +* bit masks that hold the per-cpu feature capabilities, e.g. this_cpu_has(). +*/ +static __always_inline u32 bit(int feature) { -return 1 << (bitno &31 ); +/* +* bit() is intended to be used only for hardware-defined +* words, i.e. words whose bits directly correspond to a CPUID leaf. +* Retrieving the bit mask from a Linux-defined word is nonsensical +* as the bit number/mask is an arbitrary software-defined value and +* can't be used by KVM to query/control guest capabilities. +*/ +BUILD_BUG_ON ((feature >>5 ) ==CPUID_LNX_1 ); +BUILD_BUG_ON ((feature >>5 ) ==CPUID_LNX_2 ); +BUILD_BUG_ON ((feature >>5 ) ==CPUID_LNX_3 ); +BUILD_BUG_ON ((feature >>5 ) ==CPUID_LNX_4 ); +BUILD_BUG_ON ((feature >>5 ) >CPUID_7_EDX ); + +return 1 << (feature &31 ); }static inline u8 vcpu_virt_addr_bits (struct kvm_vcpu *vcpu)
Signature · 1 line
2.24.0
Re: [PATCH 1/2] KVM: x86: Add build-time assertion on usage of bit()
Quoted 34 lines · Sean Christopherson
Add build-time checks to ensure KVM isn't trying to do a reverse CPUID lookup on Linux-defined feature bits, along with comments to explain the gory details of
X86_FEATUREsandbit().Signed-off-by Sean Christopherson <[email protected]>---
Note, the premature newline in the first line of the second comment is intentional to reduce churn in the next patch.
arch/x86/kvm/x86.h | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)arch/x86/kvm/x86.h +9
-2@@ -144,9 +144,28 @@ static inline bool is_pae_paging(struct kvm_vcpu *vcpu)return !is_long_mode (vcpu) &&is_pae (vcpu) &&is_paging (vcpu); } -static inline u32 bit (int bitno) +/* +* Retrieve the bit mask from an X86_FEATURE_* definition. Features contain +* the hardware defined bit number (stored in bits 4:0) and a software defined +* "word" (stored in bits 31:5). The word is used to index into arrays of +* bit masks that hold the per-cpu feature capabilities, e.g. this_cpu_has(). +*/ +static __always_inline u32 bit(int feature) { -return 1 << (bitno &31 ); +/* +* bit() is intended to be used only for hardware-defined
arch/x86/kvm/x86.h static inline bool is_pae_paging(struct kvm_vcpu *vcpu) · line 166
Quoted 10 lines
arch/x86/kvm/x86.h +10
-0+* words, i.e. words whose bits directly correspond to a CPUID leaf. +* Retrieving the bit mask from a Linux-defined word is nonsensical +* as the bit number/mask is an arbitrary software-defined value and +* can't be used by KVM to query/control guest capabilities. +*/ +BUILD_BUG_ON ((feature >>5 ) ==CPUID_LNX_1 ); +BUILD_BUG_ON ((feature >>5 ) ==CPUID_LNX_2 ); +BUILD_BUG_ON ((feature >>5 ) ==CPUID_LNX_3 ); +BUILD_BUG_ON ((feature >>5 ) ==CPUID_LNX_4 ); +BUILD_BUG_ON ((feature >>5 ) >CPUID_7_EDX );
What is magical about CPUID_7_EDX?
arch/x86/kvm/x86.h static inline bool is_pae_paging(struct kvm_vcpu *vcpu) · line 168
Quoted 2 lines
arch/x86/kvm/x86.h +2
-0+ +return 1 << (feature &31 );
Why not BIT(feature & 31)?
Quoted 6 lines
arch/x86/kvm/x86.h +0
-1}static inline u8 vcpu_virt_addr_bits (struct kvm_vcpu *vcpu) --2.24.0
Re: [PATCH 1/2] KVM: x86: Add build-time assertion on usage of bit()
Quoted 40 lines · Jim Mattson
Quoted 38 lines · Sean Christopherson
Add build-time checks to ensure KVM isn't trying to do a reverse CPUID lookup on Linux-defined feature bits, along with comments to explain the gory details of
X86_FEATUREsandbit().Signed-off-by Sean Christopherson <[email protected]>---
Note, the premature newline in the first line of the second comment is intentional to reduce churn in the next patch.
arch/x86/kvm/x86.h | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)arch/x86/kvm/x86.h +13
-2@@ -144,9 +144,28 @@ static inline bool is_pae_paging(struct kvm_vcpu *vcpu)return !is_long_mode (vcpu) &&is_pae (vcpu) &&is_paging (vcpu); } -static inline u32 bit (int bitno) +/* +* Retrieve the bit mask from an X86_FEATURE_* definition. Features contain +* the hardware defined bit number (stored in bits 4:0) and a software defined +* "word" (stored in bits 31:5). The word is used to index into arrays of +* bit masks that hold the per-cpu feature capabilities, e.g. this_cpu_has(). +*/ +static __always_inline u32 bit(int feature) { -return 1 << (bitno &31 ); +/* +* bit() is intended to be used only for hardware-defined +* words, i.e. words whose bits directly correspond to a CPUID leaf. +* Retrieving the bit mask from a Linux-defined word is nonsensical +* as the bit number/mask is an arbitrary software-defined value and +* can't be used by KVM to query/control guest capabilities.
Quoted 8 lines
arch/x86/kvm/x86.h static inline bool is_pae_paging(struct kvm_vcpu *vcpu) · line 166
Quoted 6 lines
arch/x86/kvm/x86.h +6
-0+*/ +BUILD_BUG_ON ((feature >>5 ) ==CPUID_LNX_1 ); +BUILD_BUG_ON ((feature >>5 ) ==CPUID_LNX_2 ); +BUILD_BUG_ON ((feature >>5 ) ==CPUID_LNX_3 ); +BUILD_BUG_ON ((feature >>5 ) ==CPUID_LNX_4 ); +BUILD_BUG_ON ((feature >>5 ) >CPUID_7_EDX );What is magical about
CPUID_7_EDX?
It's currently the last cpufeatures word. My thought was to force this to be updated in order to do reverse lookup on the next new word. I didn't want to use NCAPINTS because that gets updated when a new word is added to cpufeatures, i.e. wouldn't catch the case where the next new word is a Linux-defined word, which is extremely unlikely but theoretically possible.
Quoted 4 lines
arch/x86/kvm/x86.h static inline bool is_pae_paging(struct kvm_vcpu *vcpu) · line 168
Quoted 2 lines
arch/x86/kvm/x86.h +2
-0+ +return 1 << (feature &31 );Why not
BIT(feature & 31)?
That's a very good question.
Quoted 6 lines
Quoted 6 lines
arch/x86/kvm/x86.h +0
-1}static inline u8 vcpu_virt_addr_bits (struct kvm_vcpu *vcpu) --2.24.0
PATCH 2/2 KVM: x86: Refactor and rename bit() to feature_bit() macro
In reply to Sean ChristophersonRename bit() to __feature_bit() to give it a more descriptive name, and add a macro, feature_bit(), to stuff the X68_FEATURE_ prefix to keep line lengths manageable for code that hardcodes the bit to be retrieved.
No functional change intended.
--- arch/x86/kvm/cpuid.c | 2 +- arch/x86/kvm/cpuid.h | 4 ++-- arch/x86/kvm/emulate.c | 8 +++----- arch/x86/kvm/svm.c | 4 ++-- arch/x86/kvm/vmx/vmx.c | 42 +++++++++++++++++++++--------------------- arch/x86/kvm/x86.h | 5 +++-- 6 files changed, 32 insertions(+), 33 deletions(-)
arch/x86/kvm/cpuid.c +1 -1
@@ -62,7 +62,7 @@ u64 kvm_supported_xcr0(void)return xcr0; } -#define F (x) bit(X86_FEATURE_##x) +#define F feature_bitint kvm_update_cpuid (struct kvm_vcpu *vcpu) {
arch/x86/kvm/cpuid.h +2 -2
@@ -101,7 +101,7 @@ static __always_inline bool guest_cpuid_has(struct kvm_vcpu *vcpu, unsigned x86_if (!reg)return false; -return *reg &bit (x86_feature); +return *reg &__feature_bit (x86_feature); } static __always_inline void guest_cpuid_clear(struct kvm_vcpu *vcpu, unsigned x86_feature)
@@ -110,7 +110,7 @@ static __always_inline void guest_cpuid_clear(struct kvm_vcpu *vcpu, unsigned x8 reg =guest_cpuid_get_register (vcpu, x86_feature);if (reg) - *reg &= ~bit (x86_feature); + *reg &= ~__feature_bit (x86_feature); }static inline bool guest_cpuid_is_amd (struct kvm_vcpu *vcpu)
arch/x86/kvm/emulate.c +3 -5
@@ -2353,7 +2353,7 @@ static int emulator_has_longmode(struct x86_emulate_ctxt *ctxt) eax =0x80000001 ; ecx =0 ; ctxt->ops->get_cpuid (ctxt, &eax, &ebx, &ecx, &edx, false); -return edx &bit (X86_FEATURE_LM ); +return edx &feature_bit (LM );#else return false;#endif
@@ -3618,8 +3618,6 @@ static int em_mov(struct x86_emulate_ctxt *ctxt)return X86EMUL_CONTINUE ; } -#define FFL (x) bit(X86_FEATURE_##x) -static int em_movbe (struct x86_emulate_ctxt *ctxt) {u32 ebx, ecx, edx, eax =1 ;
@@ -3629,7 +3627,7 @@ static int em_movbe(struct x86_emulate_ctxt *ctxt)* Check MOVBE is set in the guest-visible CPUID leaf. */ ctxt->ops->get_cpuid (ctxt, &eax, &ebx, &ecx, &edx, false); -if (!(ecx &FFL (MOVBE ))) +if (!(ecx &feature_bit (MOVBE )))return emulate_ud (ctxt);switch (ctxt->op_bytes) {
@@ -4030,7 +4028,7 @@ static int check_fxsr(struct x86_emulate_ctxt *ctxt)u32 eax =1 , ebx, ecx =0 , edx; ctxt->ops->get_cpuid (ctxt, &eax, &ebx, &ecx, &edx, false); -if (!(edx &FFL (FXSR ))) +if (!(edx &feature_bit (FXSR )))return emulate_ud (ctxt);if (ctxt->ops->get_cr (ctxt,0 ) & (X86_CR0_TS |X86_CR0_EM ))
arch/x86/kvm/svm.c +2 -2
@@ -5924,14 +5924,14 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu)guest_cpuid_clear (vcpu,X86_FEATURE_X2APIC ); } -#define F (x) bit(X86_FEATURE_##x) +#define F feature_bitstatic void svm_set_supported_cpuid (u32 func,struct kvm_cpuid_entry2 *entry) {switch (func) {case 0x1 :if (avic) - entry->ecx &= ~bit (X86_FEATURE_X2APIC ); + entry->ecx &= ~F (X2APIC );break ;case 0x80000001 :if (nested)
arch/x86/kvm/vmx/vmx.c +21 -21
56 lines not on this page · raw message
arch/x86/kvm/x86.h +3 -2
@@ -150,10 +150,10 @@ static inline bool is_pae_paging(struct kvm_vcpu *vcpu)* "word" (stored in bits 31:5). The word is used to index into arrays of * bit masks that hold the per-cpu feature capabilities, e.g. this_cpu_has(). */ -static __always_inline u32 bit(int feature) +static __always_inline u32 __feature_bit(u32 feature) {/* -* bit() is intended to be used only for hardware-defined +* __feature_bit() is intended to be used only for hardware-defined * words, i.e. words whose bits directly correspond to a CPUID leaf. * Retrieving the bit mask from a Linux-defined word is nonsensical * as the bit number/mask is an arbitrary software-defined value and
@@ -167,6 +167,7 @@ static __always_inline u32 bit(int feature)return 1 << (feature &31 ); } +#define feature_bit (name) __feature_bit(X86_FEATURE_##name)static inline u8 vcpu_virt_addr_bits (struct kvm_vcpu *vcpu) {
Signature · 1 line
2.24.0
Re: [PATCH 2/2] KVM: x86: Refactor and rename bit() to feature_bit() macro
Quoted 8 lines · Sean Christopherson
Rename
bit()to__feature_bit()to give it a more descriptive name, and add a macro,feature_bit(), to stuff theX68_FEATURE_prefix to keep line lengths manageable for code that hardcodes the bit to be retrieved.No functional change intended.
Signed-off-by Sean Christopherson <[email protected]>
Quoted 3 lines · Sean Christopherson
Small series to add build-time protections on reverse CPUID lookup (and other usages of
bit()), and to rename the misleading-genericbit()helper to something that better conveys its purpose.
Paolo, please don't merge this even though Jim's feedback was minor, I have a revamped version that I'll send out next week. Thanks!