Re: [PATCH 2/5] x86/cpu-policy: Add SVM features already used by Xen
On Wed, May 1, 2024 at 11:39 AM Andrew Cooper wrote:
>
> On 01/05/2024 11:00 am, George Dunlap wrote:
> > On Mon, Apr 29, 2024 at 4:16 PM Andrew Cooper
> > wrote:
> >> These will replace svm_feature_flags and the SVM_FEATURE_* constants over
> >> the
> >> next few changes. Take the opportunity to rationalise some names.
> >>
> >> Drop the opencoded "inherit from host" logic in calculate_hvm_max_policy()
> >> and
> >> use 'h'/'!' annotations. The logic needs to operate on fs, not the policy
> >> object, given its position within the function.
> >>
> >> Drop some trailing whitespace introduced when this block of code was last
> >> moved.
> >>
> >> Signed-off-by: Andrew Cooper
> >> ---
> >> CC: Jan Beulich
> >> CC: Roger Pau Monné
> >> CC: Stefano Stabellini
> >> CC: Xenia Ragiadakou
> >> CC: Sergiy Kibrik
> >> CC: George Dunlap
> >> CC: Andrei Semenov
> >> CC: Vaishali Thakkar
> >> ---
> >> tools/misc/xen-cpuid.c | 11 +++
> >> xen/arch/x86/cpu-policy.c | 17 +
> >> xen/include/public/arch-x86/cpufeatureset.h | 14 ++
> >> xen/tools/gen-cpuid.py | 3 +++
> >> 4 files changed, 33 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
> >> index ab09410a05d6..0d01b0e797f1 100644
> >> --- a/tools/misc/xen-cpuid.c
> >> +++ b/tools/misc/xen-cpuid.c
> >> @@ -266,6 +266,17 @@ static const char *const str_m10Ah[32] =
> >>
> >> static const char *const str_eAd[32] =
> >> {
> >> +[ 0] = "npt", [ 1] = "v-lbr",
> >> +[ 2] = "svm-lock",[ 3] = "nrips",
> >> +[ 4] = "v-tsc-rate", [ 5] = "vmcb-cleanbits",
> >> +[ 6] = "flush-by-asid", [ 7] = "decode-assist",
> >> +
> >> +[10] = "pause-filter",
> >> +[12] = "pause-thresh",
> >> +/* 14 */ [15] = "v-loadsave",
> >> +[16] = "v-gif",
> >> +/* 18 */ [19] = "npt-sss",
> >> +[20] = "v-spec-ctrl",
> >> };
> >>
> >> static const char *const str_e1Fa[32] =
> >> diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
> >> index 4b6d96276399..da4401047e89 100644
> >> --- a/xen/arch/x86/cpu-policy.c
> >> +++ b/xen/arch/x86/cpu-policy.c
> >> @@ -9,7 +9,6 @@
> >> #include
> >> #include
> >> #include
> >> -#include
> >> #include
> >> #include
> >> #include
> >> @@ -748,22 +747,16 @@ static void __init calculate_hvm_max_policy(void)
> >> if ( !cpu_has_vmx )
> >> __clear_bit(X86_FEATURE_PKS, fs);
> >>
> >> -/*
> >> +/*
> >> * Make adjustments to possible (nested) virtualization features
> >> exposed
> >> * to the guest
> >> */
> >> -if ( p->extd.svm )
> >> +if ( test_bit(X86_FEATURE_SVM, fs) )
> >> {
> >> -/* Clamp to implemented features which require hardware support.
> >> */
> >> -p->extd.raw[0xa].d &= ((1u << SVM_FEATURE_NPT) |
> >> - (1u << SVM_FEATURE_LBRV) |
> >> - (1u << SVM_FEATURE_NRIPS) |
> >> - (1u << SVM_FEATURE_PAUSEFILTER) |
> >> - (1u << SVM_FEATURE_DECODEASSISTS));
> >> -/* Enable features which are always emulated. */
> >> -p->extd.raw[0xa].d |= (1u << SVM_FEATURE_VMCBCLEAN);
> >> +/* Xen always emulates cleanbits. */
> >> +__set_bit(X86_FEATURE_VMCB_CLEANBITS, fs);
> >> }
> > What about this line, at the end of recalculate_cpuid_policy()?
> >
> > if ( !p->extd.svm )
> > p->extd.raw[0xa] = EMPTY_LEAF;
> >
> > Is there a reason to continue to operate directly on the policy here,
> > or would it be better to do this up in the featureset section?
>
> That's still needed.
>
> Otherwise in a !SVM VM you still see svm_rev and nr_asids in a leaf that
> should be all zeroes.
...Uh, yes of course we still need to clear the non-existent CPUID
bits. I was asking if we should change *how* we should clear them.
In the hunk I responded to, when enabling VMCBCLEAN, we switch from
setting bits in the policy, to setting bits in the featureset. I was
asking whether it would make sense to do something like
if !test_bit(X86_FEATURE_SVM, fs)
fs[FEATURESET_eAd] = 0;
before the x86_cpu_featureset_to_policy() instead.
-George
Re: [PATCH 2/5] x86/cpu-policy: Add SVM features already used by Xen
On 01/05/2024 11:00 am, George Dunlap wrote:
> On Mon, Apr 29, 2024 at 4:16 PM Andrew Cooper
> wrote:
>> These will replace svm_feature_flags and the SVM_FEATURE_* constants over the
>> next few changes. Take the opportunity to rationalise some names.
>>
>> Drop the opencoded "inherit from host" logic in calculate_hvm_max_policy()
>> and
>> use 'h'/'!' annotations. The logic needs to operate on fs, not the policy
>> object, given its position within the function.
>>
>> Drop some trailing whitespace introduced when this block of code was last
>> moved.
>>
>> Signed-off-by: Andrew Cooper
>> ---
>> CC: Jan Beulich
>> CC: Roger Pau Monné
>> CC: Stefano Stabellini
>> CC: Xenia Ragiadakou
>> CC: Sergiy Kibrik
>> CC: George Dunlap
>> CC: Andrei Semenov
>> CC: Vaishali Thakkar
>> ---
>> tools/misc/xen-cpuid.c | 11 +++
>> xen/arch/x86/cpu-policy.c | 17 +
>> xen/include/public/arch-x86/cpufeatureset.h | 14 ++
>> xen/tools/gen-cpuid.py | 3 +++
>> 4 files changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
>> index ab09410a05d6..0d01b0e797f1 100644
>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -266,6 +266,17 @@ static const char *const str_m10Ah[32] =
>>
>> static const char *const str_eAd[32] =
>> {
>> +[ 0] = "npt", [ 1] = "v-lbr",
>> +[ 2] = "svm-lock",[ 3] = "nrips",
>> +[ 4] = "v-tsc-rate", [ 5] = "vmcb-cleanbits",
>> +[ 6] = "flush-by-asid", [ 7] = "decode-assist",
>> +
>> +[10] = "pause-filter",
>> +[12] = "pause-thresh",
>> +/* 14 */ [15] = "v-loadsave",
>> +[16] = "v-gif",
>> +/* 18 */ [19] = "npt-sss",
>> +[20] = "v-spec-ctrl",
>> };
>>
>> static const char *const str_e1Fa[32] =
>> diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
>> index 4b6d96276399..da4401047e89 100644
>> --- a/xen/arch/x86/cpu-policy.c
>> +++ b/xen/arch/x86/cpu-policy.c
>> @@ -9,7 +9,6 @@
>> #include
>> #include
>> #include
>> -#include
>> #include
>> #include
>> #include
>> @@ -748,22 +747,16 @@ static void __init calculate_hvm_max_policy(void)
>> if ( !cpu_has_vmx )
>> __clear_bit(X86_FEATURE_PKS, fs);
>>
>> -/*
>> +/*
>> * Make adjustments to possible (nested) virtualization features exposed
>> * to the guest
>> */
>> -if ( p->extd.svm )
>> +if ( test_bit(X86_FEATURE_SVM, fs) )
>> {
>> -/* Clamp to implemented features which require hardware support. */
>> -p->extd.raw[0xa].d &= ((1u << SVM_FEATURE_NPT) |
>> - (1u << SVM_FEATURE_LBRV) |
>> - (1u << SVM_FEATURE_NRIPS) |
>> - (1u << SVM_FEATURE_PAUSEFILTER) |
>> - (1u << SVM_FEATURE_DECODEASSISTS));
>> -/* Enable features which are always emulated. */
>> -p->extd.raw[0xa].d |= (1u << SVM_FEATURE_VMCBCLEAN);
>> +/* Xen always emulates cleanbits. */
>> +__set_bit(X86_FEATURE_VMCB_CLEANBITS, fs);
>> }
> What about this line, at the end of recalculate_cpuid_policy()?
>
> if ( !p->extd.svm )
> p->extd.raw[0xa] = EMPTY_LEAF;
>
> Is there a reason to continue to operate directly on the policy here,
> or would it be better to do this up in the featureset section?
That's still needed.
Otherwise in a !SVM VM you still see svm_rev and nr_asids in a leaf that
should be all zeroes.
~Andrew
Re: [PATCH 2/5] x86/cpu-policy: Add SVM features already used by Xen
On Mon, Apr 29, 2024 at 4:16 PM Andrew Cooper wrote:
>
> These will replace svm_feature_flags and the SVM_FEATURE_* constants over the
> next few changes. Take the opportunity to rationalise some names.
>
> Drop the opencoded "inherit from host" logic in calculate_hvm_max_policy() and
> use 'h'/'!' annotations. The logic needs to operate on fs, not the policy
> object, given its position within the function.
>
> Drop some trailing whitespace introduced when this block of code was last
> moved.
>
> Signed-off-by: Andrew Cooper
> ---
> CC: Jan Beulich
> CC: Roger Pau Monné
> CC: Stefano Stabellini
> CC: Xenia Ragiadakou
> CC: Sergiy Kibrik
> CC: George Dunlap
> CC: Andrei Semenov
> CC: Vaishali Thakkar
> ---
> tools/misc/xen-cpuid.c | 11 +++
> xen/arch/x86/cpu-policy.c | 17 +
> xen/include/public/arch-x86/cpufeatureset.h | 14 ++
> xen/tools/gen-cpuid.py | 3 +++
> 4 files changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
> index ab09410a05d6..0d01b0e797f1 100644
> --- a/tools/misc/xen-cpuid.c
> +++ b/tools/misc/xen-cpuid.c
> @@ -266,6 +266,17 @@ static const char *const str_m10Ah[32] =
>
> static const char *const str_eAd[32] =
> {
> +[ 0] = "npt", [ 1] = "v-lbr",
> +[ 2] = "svm-lock",[ 3] = "nrips",
> +[ 4] = "v-tsc-rate", [ 5] = "vmcb-cleanbits",
> +[ 6] = "flush-by-asid", [ 7] = "decode-assist",
> +
> +[10] = "pause-filter",
> +[12] = "pause-thresh",
> +/* 14 */ [15] = "v-loadsave",
> +[16] = "v-gif",
> +/* 18 */ [19] = "npt-sss",
> +[20] = "v-spec-ctrl",
> };
>
> static const char *const str_e1Fa[32] =
> diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
> index 4b6d96276399..da4401047e89 100644
> --- a/xen/arch/x86/cpu-policy.c
> +++ b/xen/arch/x86/cpu-policy.c
> @@ -9,7 +9,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -748,22 +747,16 @@ static void __init calculate_hvm_max_policy(void)
> if ( !cpu_has_vmx )
> __clear_bit(X86_FEATURE_PKS, fs);
>
> -/*
> +/*
> * Make adjustments to possible (nested) virtualization features exposed
> * to the guest
> */
> -if ( p->extd.svm )
> +if ( test_bit(X86_FEATURE_SVM, fs) )
> {
> -/* Clamp to implemented features which require hardware support. */
> -p->extd.raw[0xa].d &= ((1u << SVM_FEATURE_NPT) |
> - (1u << SVM_FEATURE_LBRV) |
> - (1u << SVM_FEATURE_NRIPS) |
> - (1u << SVM_FEATURE_PAUSEFILTER) |
> - (1u << SVM_FEATURE_DECODEASSISTS));
> -/* Enable features which are always emulated. */
> -p->extd.raw[0xa].d |= (1u << SVM_FEATURE_VMCBCLEAN);
> +/* Xen always emulates cleanbits. */
> +__set_bit(X86_FEATURE_VMCB_CLEANBITS, fs);
> }
What about this line, at the end of recalculate_cpuid_policy()?
if ( !p->extd.svm )
p->extd.raw[0xa] = EMPTY_LEAF;
Is there a reason to continue to operate directly on the policy here,
or would it be better to do this up in the featureset section?
-George
Re: [PATCH 2/5] x86/cpu-policy: Add SVM features already used by Xen
On 29.04.2024 17:16, Andrew Cooper wrote: > --- a/xen/include/public/arch-x86/cpufeatureset.h > +++ b/xen/include/public/arch-x86/cpufeatureset.h > @@ -358,6 +358,20 @@ XEN_CPUFEATURE(RFDS_CLEAR, 16*32+28) /*!A > Register File(s) cleared by VE > /* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.edx, word 17 */ > > /* AMD-defined CPU features, CPUID level 0x800a.edx, word 18 */ > +XEN_CPUFEATURE(NPT,18*32+ 0) /*h Nested PageTables */ > +XEN_CPUFEATURE(V_LBR, 18*32+ 1) /*h Virtualised LBR */ > +XEN_CPUFEATURE(SVM_LOCK, 18*32+ 2) /* SVM locking MSR */ > +XEN_CPUFEATURE(NRIPS, 18*32+ 3) /*h Next-RIP saved on VMExit */ > +XEN_CPUFEATURE(V_TSC_RATE, 18*32+ 4) /* Virtualised TSC Ratio */ > +XEN_CPUFEATURE(VMCB_CLEANBITS, 18*32+ 5) /*! VMCB Clean-bits */ Wouldn't this better be marked !h nevertheless? As to the name - does it need to be this long? VMCB_CLEAN would be in line with the PM. But yeah, while CLEANBITS would be clear in the context of this leaf, there might be whatever other "cleanbits" elsewhere, so some qualifier is wanted, I guess. As to the V_ prefixes you use for several of the features: Isn't there a risk of this being ambiguous towards VT-x? Maybe they should all be SVM_*, even if ... > +XEN_CPUFEATURE(FLUSH_BY_ASID, 18*32+ 6) /* TLB Flush by ASID */ > +XEN_CPUFEATURE(DECODE_ASSIST, 18*32+ 7) /*h Decode assists */ > +XEN_CPUFEATURE(PAUSE_FILTER, 18*32+10) /*h Pause filter */ > +XEN_CPUFEATURE(PAUSE_THRESH, 18*32+12) /* Pause filter threshold */ > +XEN_CPUFEATURE(V_LOADSAVE, 18*32+15) /* Virtualised VMLOAD/SAVE */ > +XEN_CPUFEATURE(V_GIF, 18*32+16) /* Virtualised GIF */ ... these two at least are clearly SVM terminology and hence already unambiguous. > +XEN_CPUFEATURE(NPT_SSS,18*32+19) /* NPT Supervisor Shadow > Stacks */ > +XEN_CPUFEATURE(V_SPEC_CTRL,18*32+20) /* Virtualised MSR_SPEC_CTRL > */ Whereas this, when used somewhere in isolation, would not make clear whether AMD's or Intel's is meant. > --- a/xen/tools/gen-cpuid.py > +++ b/xen/tools/gen-cpuid.py > @@ -280,6 +280,9 @@ def crunch_numbers(state): > # standard 3DNow in the earlier K6 processors. > _3DNOW: [_3DNOWEXT], > > +# The SVM bit enumerates the whole SVM leave. > +SVM: list(range(NPT, NPT + 32)), Seeing this and taking it together with the somewhat confusing (to me) part of the description of patch 1: What is it then that you try to avoid there, when adding the dependencies here is okay, while doing so there would be entirely impossible (short of there being identifiers)? Jan
Re: [PATCH 2/5] x86/cpu-policy: Add SVM features already used by Xen
On 29/04/2024 4:16 pm, Andrew Cooper wrote: > diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py > index bf3f9ec01e6e..f07b1f4cf905 100755 > --- a/xen/tools/gen-cpuid.py > +++ b/xen/tools/gen-cpuid.py > @@ -280,6 +280,9 @@ def crunch_numbers(state): > # standard 3DNow in the earlier K6 processors. > _3DNOW: [_3DNOWEXT], > > +# The SVM bit enumerates the whole SVM leave. > +SVM: list(range(NPT, NPT + 32)), This should say leaf. Fixed locally. ~Andrew
[PATCH 2/5] x86/cpu-policy: Add SVM features already used by Xen
These will replace svm_feature_flags and the SVM_FEATURE_* constants over the
next few changes. Take the opportunity to rationalise some names.
Drop the opencoded "inherit from host" logic in calculate_hvm_max_policy() and
use 'h'/'!' annotations. The logic needs to operate on fs, not the policy
object, given its position within the function.
Drop some trailing whitespace introduced when this block of code was last
moved.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Roger Pau Monné
CC: Stefano Stabellini
CC: Xenia Ragiadakou
CC: Sergiy Kibrik
CC: George Dunlap
CC: Andrei Semenov
CC: Vaishali Thakkar
---
tools/misc/xen-cpuid.c | 11 +++
xen/arch/x86/cpu-policy.c | 17 +
xen/include/public/arch-x86/cpufeatureset.h | 14 ++
xen/tools/gen-cpuid.py | 3 +++
4 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index ab09410a05d6..0d01b0e797f1 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -266,6 +266,17 @@ static const char *const str_m10Ah[32] =
static const char *const str_eAd[32] =
{
+[ 0] = "npt", [ 1] = "v-lbr",
+[ 2] = "svm-lock",[ 3] = "nrips",
+[ 4] = "v-tsc-rate", [ 5] = "vmcb-cleanbits",
+[ 6] = "flush-by-asid", [ 7] = "decode-assist",
+
+[10] = "pause-filter",
+[12] = "pause-thresh",
+/* 14 */ [15] = "v-loadsave",
+[16] = "v-gif",
+/* 18 */ [19] = "npt-sss",
+[20] = "v-spec-ctrl",
};
static const char *const str_e1Fa[32] =
diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index 4b6d96276399..da4401047e89 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -9,7 +9,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -748,22 +747,16 @@ static void __init calculate_hvm_max_policy(void)
if ( !cpu_has_vmx )
__clear_bit(X86_FEATURE_PKS, fs);
-/*
+/*
* Make adjustments to possible (nested) virtualization features exposed
* to the guest
*/
-if ( p->extd.svm )
+if ( test_bit(X86_FEATURE_SVM, fs) )
{
-/* Clamp to implemented features which require hardware support. */
-p->extd.raw[0xa].d &= ((1u << SVM_FEATURE_NPT) |
- (1u << SVM_FEATURE_LBRV) |
- (1u << SVM_FEATURE_NRIPS) |
- (1u << SVM_FEATURE_PAUSEFILTER) |
- (1u << SVM_FEATURE_DECODEASSISTS));
-/* Enable features which are always emulated. */
-p->extd.raw[0xa].d |= (1u << SVM_FEATURE_VMCBCLEAN);
+/* Xen always emulates cleanbits. */
+__set_bit(X86_FEATURE_VMCB_CLEANBITS, fs);
}
-
+
guest_common_max_feature_adjustments(fs);
guest_common_feature_adjustments(fs);
diff --git a/xen/include/public/arch-x86/cpufeatureset.h
b/xen/include/public/arch-x86/cpufeatureset.h
index 0f869214811e..80d252a38c2d 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -358,6 +358,20 @@ XEN_CPUFEATURE(RFDS_CLEAR, 16*32+28) /*!A Register
File(s) cleared by VE
/* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.edx, word 17 */
/* AMD-defined CPU features, CPUID level 0x800a.edx, word 18 */
+XEN_CPUFEATURE(NPT,18*32+ 0) /*h Nested PageTables */
+XEN_CPUFEATURE(V_LBR, 18*32+ 1) /*h Virtualised LBR */
+XEN_CPUFEATURE(SVM_LOCK, 18*32+ 2) /* SVM locking MSR */
+XEN_CPUFEATURE(NRIPS, 18*32+ 3) /*h Next-RIP saved on VMExit */
+XEN_CPUFEATURE(V_TSC_RATE, 18*32+ 4) /* Virtualised TSC Ratio */
+XEN_CPUFEATURE(VMCB_CLEANBITS, 18*32+ 5) /*! VMCB Clean-bits */
+XEN_CPUFEATURE(FLUSH_BY_ASID, 18*32+ 6) /* TLB Flush by ASID */
+XEN_CPUFEATURE(DECODE_ASSIST, 18*32+ 7) /*h Decode assists */
+XEN_CPUFEATURE(PAUSE_FILTER, 18*32+10) /*h Pause filter */
+XEN_CPUFEATURE(PAUSE_THRESH, 18*32+12) /* Pause filter threshold */
+XEN_CPUFEATURE(V_LOADSAVE, 18*32+15) /* Virtualised VMLOAD/SAVE */
+XEN_CPUFEATURE(V_GIF, 18*32+16) /* Virtualised GIF */
+XEN_CPUFEATURE(NPT_SSS,18*32+19) /* NPT Supervisor Shadow Stacks
*/
+XEN_CPUFEATURE(V_SPEC_CTRL,18*32+20) /* Virtualised MSR_SPEC_CTRL */
/* AMD-defined CPU features, CPUID level 0x801f.eax, word 19 */
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index bf3f9ec01e6e..f07b1f4cf905 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -280,6 +280,9 @@ def crunch_numbers(state):
# standard 3DNow in the earlier K6 processors.
_3DNOW: [_3DNOWEXT],
+# The SVM bit enumerates the whole SVM leave.
+SVM: list(range(NPT, NPT + 32)),
+
#
