The "effective type" of the cpu_has_svm_feature macro is effectively
an unsigned log with one bit set (or not); at least one place someone
felt compelled to do a !! to make sure that they got a boolean out of
it.
Ideally the whole of this would be folded into the cpufeature.h
infrastructure. But for now, duplicate the more type-safe static
inlines in that file, and remove the !!.
Signed-off-by: George Dunlap <george.dunlap@cloud.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
xen/arch/x86/hvm/svm/svm.c | 2 +-
xen/arch/x86/include/asm/hvm/svm/svm.h | 5 ++++-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 5741287355..40bc1ffbc6 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2580,7 +2580,7 @@ const struct hvm_function_table * __init start_svm(void)
if ( !printed )
printk(" - none\n");
- svm_function_table.hap_supported = !!cpu_has_svm_npt;
+ svm_function_table.hap_supported = cpu_has_svm_npt;
svm_function_table.caps.hap_superpage_2mb = true;
svm_function_table.caps.hap_superpage_1gb = cpu_has_page1gb;
diff --git a/xen/arch/x86/include/asm/hvm/svm/svm.h b/xen/arch/x86/include/asm/hvm/svm/svm.h
index 687d35be40..9e9a148845 100644
--- a/xen/arch/x86/include/asm/hvm/svm/svm.h
+++ b/xen/arch/x86/include/asm/hvm/svm/svm.h
@@ -38,7 +38,10 @@ extern u32 svm_feature_flags;
#define SVM_FEATURE_SSS 19 /* NPT Supervisor Shadow Stacks */
#define SVM_FEATURE_SPEC_CTRL 20 /* MSR_SPEC_CTRL virtualisation */
-#define cpu_has_svm_feature(f) (svm_feature_flags & (1u << (f)))
+static inline bool cpu_has_svm_feature(unsigned int feat)
+{
+ return svm_feature_flags & (1u << (feat));
+}
#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)
--
2.25.1
On 06.02.2024 02:20, George Dunlap wrote: > --- a/xen/arch/x86/include/asm/hvm/svm/svm.h > +++ b/xen/arch/x86/include/asm/hvm/svm/svm.h > @@ -38,7 +38,10 @@ extern u32 svm_feature_flags; > #define SVM_FEATURE_SSS 19 /* NPT Supervisor Shadow Stacks */ > #define SVM_FEATURE_SPEC_CTRL 20 /* MSR_SPEC_CTRL virtualisation */ > > -#define cpu_has_svm_feature(f) (svm_feature_flags & (1u << (f))) > +static inline bool cpu_has_svm_feature(unsigned int feat) > +{ > + return svm_feature_flags & (1u << (feat)); > +} > #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) Having seen patch 4 now, I have to raise the question here as well: Why do we need a separate variable (svm_feature_flags) when we could use the host policy (provided it isn't abused; see comments on patch 4)? Jan
On Mon, Feb 19, 2024 at 11:24 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 06.02.2024 02:20, George Dunlap wrote: > > --- a/xen/arch/x86/include/asm/hvm/svm/svm.h > > +++ b/xen/arch/x86/include/asm/hvm/svm/svm.h > > @@ -38,7 +38,10 @@ extern u32 svm_feature_flags; > > #define SVM_FEATURE_SSS 19 /* NPT Supervisor Shadow Stacks */ > > #define SVM_FEATURE_SPEC_CTRL 20 /* MSR_SPEC_CTRL virtualisation */ > > > > -#define cpu_has_svm_feature(f) (svm_feature_flags & (1u << (f))) > > +static inline bool cpu_has_svm_feature(unsigned int feat) > > +{ > > + return svm_feature_flags & (1u << (feat)); > > +} > > #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) > > Having seen patch 4 now, I have to raise the question here as well: Why > do we need a separate variable (svm_feature_flags) when we could use > the host policy (provided it isn't abused; see comments on patch 4)? We certainly don't need an extra variable; in fact, moving all of these into the host cpuid policy thing would make it easier, for example, to test some of the guest creation restrictions: One could use the Xen command-line parameters to disable some of the bits, then try to create a nested SVM guest and verify that it fails as expected. I would like to do that eventually, but this patch is already done and improves the code, so I just kept it. Let me know if you'd like me to simply drop this patch instead. -George
On 06.02.2024 02:20, George Dunlap wrote: > The "effective type" of the cpu_has_svm_feature macro is effectively > an unsigned log with one bit set (or not); at least one place someone > felt compelled to do a !! to make sure that they got a boolean out of > it. > > Ideally the whole of this would be folded into the cpufeature.h > infrastructure. But for now, duplicate the more type-safe static > inlines in that file, and remove the !!. > > Signed-off-by: George Dunlap <george.dunlap@cloud.com> Acked-by: Jan Beulich <jbeulich@suse.com> albeit preferably with ... > --- a/xen/arch/x86/include/asm/hvm/svm/svm.h > +++ b/xen/arch/x86/include/asm/hvm/svm/svm.h > @@ -38,7 +38,10 @@ extern u32 svm_feature_flags; > #define SVM_FEATURE_SSS 19 /* NPT Supervisor Shadow Stacks */ > #define SVM_FEATURE_SPEC_CTRL 20 /* MSR_SPEC_CTRL virtualisation */ > > -#define cpu_has_svm_feature(f) (svm_feature_flags & (1u << (f))) > +static inline bool cpu_has_svm_feature(unsigned int feat) > +{ > + return svm_feature_flags & (1u << (feat)); ... the inner pair of parentheses dropped. Jan
On Mon, Feb 19, 2024 at 10:03 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 06.02.2024 02:20, George Dunlap wrote: > > The "effective type" of the cpu_has_svm_feature macro is effectively > > an unsigned log with one bit set (or not); at least one place someone > > felt compelled to do a !! to make sure that they got a boolean out of > > it. > > > > Ideally the whole of this would be folded into the cpufeature.h > > infrastructure. But for now, duplicate the more type-safe static > > inlines in that file, and remove the !!. > > > > Signed-off-by: George Dunlap <george.dunlap@cloud.com> > > Acked-by: Jan Beulich <jbeulich@suse.com> > albeit preferably with ... > > > --- a/xen/arch/x86/include/asm/hvm/svm/svm.h > > +++ b/xen/arch/x86/include/asm/hvm/svm/svm.h > > @@ -38,7 +38,10 @@ extern u32 svm_feature_flags; > > #define SVM_FEATURE_SSS 19 /* NPT Supervisor Shadow Stacks */ > > #define SVM_FEATURE_SPEC_CTRL 20 /* MSR_SPEC_CTRL virtualisation */ > > > > -#define cpu_has_svm_feature(f) (svm_feature_flags & (1u << (f))) > > +static inline bool cpu_has_svm_feature(unsigned int feat) > > +{ > > + return svm_feature_flags & (1u << (feat)); > > ... the inner pair of parentheses dropped. Done, thanks. -George
© 2016 - 2024 Red Hat, Inc.