1. Patchew
  2. Xen
  3. [PATCH 0/6] AMD Nested Virt Preparation
  4. View patch

[PATCH 2/6] svm: Improve type of cpu_has_svm_feature

George Dunlap posted 6 patches 7 months ago
There is a newer version of this series
[PATCH 2/6] svm: Improve type of cpu_has_svm_feature
Posted by George Dunlap 7 months ago 
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
Re: [PATCH 2/6] svm: Improve type of cpu_has_svm_feature
Posted by Jan Beulich 6 months, 3 weeks ago 
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
Re: [PATCH 2/6] svm: Improve type of cpu_has_svm_feature
Posted by George Dunlap 6 months, 2 weeks ago 
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
Re: [PATCH 2/6] svm: Improve type of cpu_has_svm_feature
Posted by Jan Beulich 6 months, 3 weeks ago 
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
Re: [PATCH 2/6] svm: Improve type of cpu_has_svm_feature
Posted by George Dunlap 6 months, 2 weeks ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.