| Message ID | 20240416064606.3470052-1-Sergiy_Kibrik@epam.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 16/04/2024 7:46 am, Sergiy Kibrik wrote: > From: Xenia Ragiadakou <burzalodowa@gmail.com> > > To be able to use cpu_has_{svm/vmx}_* macros in common code without enclosing > them inside #ifdef guards when the respective virtualization technology is > not enabled, define corresponding helper routines as false when not applicable. > > No functional change intended. > > Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> > --- > xen/arch/x86/include/asm/hvm/svm/svm.h | 8 ++++++++ > xen/arch/x86/include/asm/hvm/vmx/vmcs.h | 7 +++++++ > 2 files changed, 15 insertions(+) > > diff --git a/xen/arch/x86/include/asm/hvm/svm/svm.h b/xen/arch/x86/include/asm/hvm/svm/svm.h > index 4eeeb25da9..7e8cdb4a27 100644 > --- a/xen/arch/x86/include/asm/hvm/svm/svm.h > +++ b/xen/arch/x86/include/asm/hvm/svm/svm.h > @@ -38,10 +38,18 @@ extern u32 svm_feature_flags; > #define SVM_FEATURE_SSS 19 /* NPT Supervisor Shadow Stacks */ > #define SVM_FEATURE_SPEC_CTRL 20 /* MSR_SPEC_CTRL virtualisation */ > > +#ifdef CONFIG_SVM > static inline bool cpu_has_svm_feature(unsigned int feat) > { > return svm_feature_flags & (1u << feat); > } > +#else > +static inline bool cpu_has_svm_feature(unsigned int feat) > +{ > + return false; > +} > +#endif > + > #define cpu_has_svm_npt cpu_has_svm_feature(SVM_FEATURE_NPT) > #define cpu_has_svm_lbrv cpu_has_svm_feature(SVM_FEATURE_LBRV) > #define cpu_has_svm_svml cpu_has_svm_feature(SVM_FEATURE_SVML) > diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h > index fd197e2603..2d927d3100 100644 > --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h > +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h > @@ -287,10 +287,17 @@ extern uint64_t vmx_tertiary_exec_control; > #define VMX_VPID_INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL 0x80000000000ULL > extern u64 vmx_ept_vpid_cap; > > +#ifdef CONFIG_VMX > static inline bool vmx_ctrl_has_feature(uint64_t control, unsigned long feature) > { > return control & feature; > } > +#else > +static inline bool vmx_ctrl_has_feature(uint64_t control, unsigned long feature) > +{ > + return false; > +} > +#endif > > #define VMX_MISC_ACTIVITY_MASK 0x000001c0 > #define VMX_MISC_PROC_TRACE 0x00004000 I'm afraid this is going in an unhelpful direction. We want to move both of these files to be local to arch/x86/hvm/{vmx,svm}/. cpu_has_svm_* isn't actually used outside of svm/; only the plain SVM_FEATURE_* constants are, and that's only because they're not expressed as plain cpu features yet. cpu_has_vmx_* has a few more users, but most are unlikely to remain in this form. One critical set of changes to fix vulnerabilities in nested-virt is to make almost of of these decisions based on per-domain state, not host state. The aspects which are host state should be in regular cpu features. I already volunteered to sort out the SEV feature leaf properly, and I was going to do the SVM leaf while I was at it. If you can wait a few days, I might be able to make half of this problem disappear. ~Andrew
On 16.04.2024 08:46, Sergiy Kibrik wrote: > --- a/xen/arch/x86/include/asm/hvm/svm/svm.h > +++ b/xen/arch/x86/include/asm/hvm/svm/svm.h > @@ -38,10 +38,18 @@ extern u32 svm_feature_flags; > #define SVM_FEATURE_SSS 19 /* NPT Supervisor Shadow Stacks */ > #define SVM_FEATURE_SPEC_CTRL 20 /* MSR_SPEC_CTRL virtualisation */ > > +#ifdef CONFIG_SVM > static inline bool cpu_has_svm_feature(unsigned int feat) > { > return svm_feature_flags & (1u << feat); > } > +#else > +static inline bool cpu_has_svm_feature(unsigned int feat) > +{ > + return false; > +} > +#endif Already static inline bool cpu_has_svm_feature(unsigned int feat) { #ifdef CONFIG_SVM return svm_feature_flags & (1u << feat); #else return false; #endif } would be less redundancy. But why not simply static inline bool cpu_has_svm_feature(unsigned int feat) { return is_ENABLED(CONFIG_SVM) && (svm_feature_flags & (1u << feat)); } ? Jan
16.04.24 16:26, Andrew Cooper: > I'm afraid this is going in an unhelpful direction. We want to move > both of these files to be local to arch/x86/hvm/{vmx,svm}/. > > cpu_has_svm_* isn't actually used outside of svm/; only the plain > SVM_FEATURE_* constants are, and that's only because they're not > expressed as plain cpu features yet. > > cpu_has_vmx_* has a few more users, but most are unlikely to remain in > this form. One critical set of changes to fix vulnerabilities in > nested-virt is to make almost of of these decisions based on per-domain > state, not host state. The aspects which are host state should be in > regular cpu features. > > I already volunteered to sort out the SEV feature leaf properly, and I > was going to do the SVM leaf while I was at it. If you can wait a few > days, I might be able to make half of this problem disappear. I guess it can wait, surely if a better solution is to be crafted at the end. Stefano, what's your opinion on that? -Sergiy
On Thu, 18 Apr 2024, Sergiy Kibrik wrote: > 16.04.24 16:26, Andrew Cooper: > > I'm afraid this is going in an unhelpful direction. We want to move > > both of these files to be local to arch/x86/hvm/{vmx,svm}/. > > > > cpu_has_svm_* isn't actually used outside of svm/; only the plain > > SVM_FEATURE_* constants are, and that's only because they're not > > expressed as plain cpu features yet. > > > > cpu_has_vmx_* has a few more users, but most are unlikely to remain in > > this form. One critical set of changes to fix vulnerabilities in > > nested-virt is to make almost of of these decisions based on per-domain > > state, not host state. The aspects which are host state should be in > > regular cpu features. > > > > I already volunteered to sort out the SEV feature leaf properly, and I > > was going to do the SVM leaf while I was at it. If you can wait a few > > days, I might be able to make half of this problem disappear. > > I guess it can wait, surely if a better solution is to be crafted at the end. > > Stefano, what's your opinion on that? I think Andrew's suggested direction is cleaner. We can certainly wait a few days for Andrew to make progress. We can also follow Andrew's suggestion in the next version of the series ourselves.
diff --git a/xen/arch/x86/include/asm/hvm/svm/svm.h b/xen/arch/x86/include/asm/hvm/svm/svm.h index 4eeeb25da9..7e8cdb4a27 100644 --- a/xen/arch/x86/include/asm/hvm/svm/svm.h +++ b/xen/arch/x86/include/asm/hvm/svm/svm.h @@ -38,10 +38,18 @@ extern u32 svm_feature_flags; #define SVM_FEATURE_SSS 19 /* NPT Supervisor Shadow Stacks */ #define SVM_FEATURE_SPEC_CTRL 20 /* MSR_SPEC_CTRL virtualisation */ +#ifdef CONFIG_SVM static inline bool cpu_has_svm_feature(unsigned int feat) { return svm_feature_flags & (1u << feat); } +#else +static inline bool cpu_has_svm_feature(unsigned int feat) +{ + return false; +} +#endif + #define cpu_has_svm_npt cpu_has_svm_feature(SVM_FEATURE_NPT) #define cpu_has_svm_lbrv cpu_has_svm_feature(SVM_FEATURE_LBRV) #define cpu_has_svm_svml cpu_has_svm_feature(SVM_FEATURE_SVML) diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h index fd197e2603..2d927d3100 100644 --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h @@ -287,10 +287,17 @@ extern uint64_t vmx_tertiary_exec_control; #define VMX_VPID_INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL 0x80000000000ULL extern u64 vmx_ept_vpid_cap; +#ifdef CONFIG_VMX static inline bool vmx_ctrl_has_feature(uint64_t control, unsigned long feature) { return control & feature; } +#else +static inline bool vmx_ctrl_has_feature(uint64_t control, unsigned long feature) +{ + return false; +} +#endif #define VMX_MISC_ACTIVITY_MASK 0x000001c0 #define VMX_MISC_PROC_TRACE 0x00004000