| Message ID | 20240206012051.3564035-3-george.dunlap@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | AMD Nested Virt Preparation | expand |
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 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 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
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
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)
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(-)