* [PATCH] x86/cpuid-policy: Add AMD SVM CPUID leaf to featureset
@ 2024-04-17 13:22 George Dunlap
2024-04-22 15:09 ` Jan Beulich
0 siblings, 1 reply; 3+ messages in thread
From: George Dunlap @ 2024-04-17 13:22 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Andrew Cooper, Jan Beulich, Roger Pau Monne
Currently, the CPUID leaf for SVM features (extd 0xa.edx) is manually
twiddled:
- hvm_max_policy takes host_policy and clamps it to supported
features (with some features unilaterally enabled because they're
always emulated
- hvm_default_policy is copied from there
- When recalculate_policy() is called for a guest, if SVM is clear,
then the entire leaf is zeroed out.
Move to a mode where the extended features are off by default, and
enabled when nested_virt is enabled.
In cpufeatureset.h, define a new featureset word for the AMD SVM
features, and declare all of the bits defined in
x86/include/asm/hvm/svm/svm.h. Mark the ones we currently pass
through to the "max policy" as HAP-only and optional.
In cpu-policy.h, define FEATURESET_ead, and convert the un-named space
in struct_cpu_policy into the appropriate union. FIXME: Do this in a
prerequisite patch, and change all references to p->extd.raw[0xa].
Update x86_cpu_X_to_Y and Y_to_X to copy this into and out of the
appropriate leaf.
Populate this during boot in generic_identify().
Add the new featureset definition into libxl_cpuid.c.
Update the code in calculate_hvm_max_policy() to do nothing with the
"normal" CPUID bits, and use the feature bit to unconditionally enable
VMCBCLEAN. FIXME Move this to a follow-up patch.
In recalculate_cpuid_policy(), enable max_fs when nested_hvm() is
true.
Signed-off-by: George Dunlap <george.dunlap@cloud.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Roger Pau Monne <roger.pau@cloud.com>
---
tools/libs/light/libxl_cpuid.c | 1 +
xen/arch/x86/cpu-policy.c | 19 +++++++++----------
xen/arch/x86/cpu/common.c | 2 ++
xen/include/public/arch-x86/cpufeatureset.h | 16 ++++++++++++++++
xen/include/xen/lib/x86/cpu-policy.h | 10 +++++++++-
xen/lib/x86/cpuid.c | 4 +++-
6 files changed, 40 insertions(+), 12 deletions(-)
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index ce4f3c7095..2c5749c3a0 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -342,6 +342,7 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *policy, const char* str)
CPUID_ENTRY(0x00000007, 1, CPUID_REG_EDX),
MSR_ENTRY(0x10a, CPUID_REG_EAX),
MSR_ENTRY(0x10a, CPUID_REG_EDX),
+ CPUID_ENTRY(0x8000000a, NA, CPUID_REG_EDX),
#undef MSR_ENTRY
#undef CPUID_ENTRY
};
diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index 4b6d962763..4a5d1b916b 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -754,14 +754,8 @@ static void __init calculate_hvm_max_policy(void)
*/
if ( p->extd.svm )
{
- /* 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);
+ __set_bit(X86_FEATURE_VMCBCLEAN, fs);
}
guest_common_max_feature_adjustments(fs);
@@ -909,6 +903,14 @@ void recalculate_cpuid_policy(struct domain *d)
__clear_bit(X86_FEATURE_VMX, max_fs);
__clear_bit(X86_FEATURE_SVM, max_fs);
}
+ else
+ {
+ /*
+ * Enable SVM features. This will be empty on VMX
+ * hosts.
+ */
+ fs[FEATURESET_ead] = max_fs[FEATURESET_ead];
+ }
}
/*
@@ -975,9 +977,6 @@ void recalculate_cpuid_policy(struct domain *d)
((vpmu_mode & XENPMU_MODE_ALL) && !is_hardware_domain(d)) )
p->basic.raw[0xa] = EMPTY_LEAF;
- if ( !p->extd.svm )
- p->extd.raw[0xa] = EMPTY_LEAF;
-
if ( !p->extd.page1gb )
p->extd.raw[0x19] = EMPTY_LEAF;
}
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 28d7f34c4d..5093379a43 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -477,6 +477,8 @@ static void generic_identify(struct cpuinfo_x86 *c)
c->x86_capability[FEATURESET_e7d] = cpuid_edx(0x80000007);
if (c->extended_cpuid_level >= 0x80000008)
c->x86_capability[FEATURESET_e8b] = cpuid_ebx(0x80000008);
+ if (c->extended_cpuid_level >= 0x8000000a)
+ c->x86_capability[FEATURESET_ead] = cpuid_edx(0x8000000a);
if (c->extended_cpuid_level >= 0x80000021)
c->x86_capability[FEATURESET_e21a] = cpuid_eax(0x80000021);
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 53f13dec31..c5c712cca3 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -357,6 +357,22 @@ 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 0x8000000a.edx, word 18 */
+XEN_CPUFEATURE(NPT, 18*32+ 0) /*h Nested page table support */
+XEN_CPUFEATURE(LBRV, 18*32+ 1) /*h LBR virtualization support */
+XEN_CPUFEATURE(SVML, 18*32+ 2) /* SVM locking MSR support */
+XEN_CPUFEATURE(NRIPS, 18*32+ 3) /*h Next RIP save on VMEXIT support */
+XEN_CPUFEATURE(TSCRATEMSR, 18*32+ 4) /* TSC ratio MSR support */
+XEN_CPUFEATURE(VMCBCLEAN, 18*32+ 5) /*h VMCB clean bits support */
+XEN_CPUFEATURE(FLUSHBYASID, 18*32+ 6) /* TLB flush by ASID support */
+XEN_CPUFEATURE(DECODEASSISTS, 18*32+ 7) /*h Decode assists support */
+XEN_CPUFEATURE(PAUSEFILTER, 18*32+10) /*h Pause intercept filter support */
+XEN_CPUFEATURE(PAUSETHRESH, 18*32+12) /* Pause intercept filter threshold */
+XEN_CPUFEATURE(VLOADSAVE, 18*32+15) /* virtual vmload/vmsave */
+XEN_CPUFEATURE(VGIF, 18*32+16) /* Virtual GIF */
+XEN_CPUFEATURE(SSS, 18*32+19) /* NPT Supervisor Shadow Stacks */
+XEN_CPUFEATURE(SPEC_CTRL, 18*32+20) /* MSR_SPEC_CTRL virtualisation */
+
#endif /* XEN_CPUFEATURE */
/* Clean up from a default include. Close the enum (for C). */
diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index d5e447e9dc..4e5c05c56d 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -22,6 +22,7 @@
#define FEATURESET_7d1 15 /* 0x00000007:1.edx */
#define FEATURESET_m10Al 16 /* 0x0000010a.eax */
#define FEATURESET_m10Ah 17 /* 0x0000010a.edx */
+#define FEATURESET_ead 18 /* 0x8000000a.edx */
struct cpuid_leaf
{
@@ -296,7 +297,14 @@ struct cpu_policy
uint32_t /* d */:32;
uint64_t :64, :64; /* Leaf 0x80000009. */
- uint64_t :64, :64; /* Leaf 0x8000000a - SVM rev and features. */
+
+ /* Leaf 0x8000000a - SVM rev and features. */
+ uint64_t /* a, b */:64, /* c */:32;
+ union {
+ uint32_t ead;
+ struct { DECL_BITFIELD(ead); };
+ };
+
uint64_t :64, :64; /* Leaf 0x8000000b. */
uint64_t :64, :64; /* Leaf 0x8000000c. */
uint64_t :64, :64; /* Leaf 0x8000000d. */
diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c
index eb7698dc73..d68f442d4e 100644
--- a/xen/lib/x86/cpuid.c
+++ b/xen/lib/x86/cpuid.c
@@ -81,7 +81,8 @@ void x86_cpu_policy_to_featureset(
fs[FEATURESET_7d1] = p->feat._7d1;
fs[FEATURESET_m10Al] = p->arch_caps.lo;
fs[FEATURESET_m10Ah] = p->arch_caps.hi;
-}
+ fs[FEATURESET_ead] = p->extd.ead;
+ }
void x86_cpu_featureset_to_policy(
const uint32_t fs[FEATURESET_NR_ENTRIES], struct cpu_policy *p)
@@ -104,6 +105,7 @@ void x86_cpu_featureset_to_policy(
p->feat._7d1 = fs[FEATURESET_7d1];
p->arch_caps.lo = fs[FEATURESET_m10Al];
p->arch_caps.hi = fs[FEATURESET_m10Ah];
+ p->extd.ead = fs[FEATURESET_ead];
}
void x86_cpu_policy_recalc_synth(struct cpu_policy *p)
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] x86/cpuid-policy: Add AMD SVM CPUID leaf to featureset
2024-04-17 13:22 [PATCH] x86/cpuid-policy: Add AMD SVM CPUID leaf to featureset George Dunlap
@ 2024-04-22 15:09 ` Jan Beulich
2024-04-22 15:14 ` Jan Beulich
0 siblings, 1 reply; 3+ messages in thread
From: Jan Beulich @ 2024-04-22 15:09 UTC (permalink / raw)
To: George Dunlap; +Cc: Andrew Cooper, Roger Pau Monne, xen-devel
On 17.04.2024 15:22, George Dunlap wrote:
> Currently, the CPUID leaf for SVM features (extd 0xa.edx) is manually
> twiddled:
>
> - hvm_max_policy takes host_policy and clamps it to supported
> features (with some features unilaterally enabled because they're
> always emulated
>
> - hvm_default_policy is copied from there
>
> - When recalculate_policy() is called for a guest, if SVM is clear,
> then the entire leaf is zeroed out.
>
> Move to a mode where the extended features are off by default, and
> enabled when nested_virt is enabled.
>
> In cpufeatureset.h, define a new featureset word for the AMD SVM
> features, and declare all of the bits defined in
> x86/include/asm/hvm/svm/svm.h. Mark the ones we currently pass
> through to the "max policy" as HAP-only and optional.
>
> In cpu-policy.h, define FEATURESET_ead, and convert the un-named space
> in struct_cpu_policy into the appropriate union. FIXME: Do this in a
> prerequisite patch, and change all references to p->extd.raw[0xa].
Just wondering: Did you mean to submit with this FIXME?
> Update x86_cpu_X_to_Y and Y_to_X to copy this into and out of the
> appropriate leaf.
>
> Populate this during boot in generic_identify().
>
> Add the new featureset definition into libxl_cpuid.c.
>
> Update the code in calculate_hvm_max_policy() to do nothing with the
> "normal" CPUID bits, and use the feature bit to unconditionally enable
> VMCBCLEAN. FIXME Move this to a follow-up patch.
>
> In recalculate_cpuid_policy(), enable max_fs when nested_hvm() is
> true.
>
> Signed-off-by: George Dunlap <george.dunlap@cloud.com>
> ---
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Roger Pau Monne <roger.pau@cloud.com>
> ---
> tools/libs/light/libxl_cpuid.c | 1 +
> xen/arch/x86/cpu-policy.c | 19 +++++++++----------
> xen/arch/x86/cpu/common.c | 2 ++
> xen/include/public/arch-x86/cpufeatureset.h | 16 ++++++++++++++++
> xen/include/xen/lib/x86/cpu-policy.h | 10 +++++++++-
> xen/lib/x86/cpuid.c | 4 +++-
> 6 files changed, 40 insertions(+), 12 deletions(-)
tools/misc/xen-cpuid.c also wants adjusting, I think.
I further think the dependencies (on the SVM feature at the very least)
also want recording in xen/tools/gen-cpuid.py.
> @@ -909,6 +903,14 @@ void recalculate_cpuid_policy(struct domain *d)
> __clear_bit(X86_FEATURE_VMX, max_fs);
> __clear_bit(X86_FEATURE_SVM, max_fs);
> }
> + else
> + {
> + /*
> + * Enable SVM features. This will be empty on VMX
> + * hosts.
> + */
> + fs[FEATURESET_ead] = max_fs[FEATURESET_ead];
> + }
> }
I'm afraid I don't understand this part: Why would you forcefully enable
everything, no matter what the tool stack set? Considering the if() part
above, wouldn't you want to mark the features non-optional, relying on
them being cleared (via dependencies) when SVM is clear?
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -477,6 +477,8 @@ static void generic_identify(struct cpuinfo_x86 *c)
> c->x86_capability[FEATURESET_e7d] = cpuid_edx(0x80000007);
> if (c->extended_cpuid_level >= 0x80000008)
> c->x86_capability[FEATURESET_e8b] = cpuid_ebx(0x80000008);
> + if (c->extended_cpuid_level >= 0x8000000a)
> + c->x86_capability[FEATURESET_ead] = cpuid_edx(0x8000000a);
> if (c->extended_cpuid_level >= 0x80000021)
> c->x86_capability[FEATURESET_e21a] = cpuid_eax(0x80000021);
Aiui this is needed right in this change because of calculate_host_policy()
deriving from boot_cpu_data.x86_capability. What I'd have expected in
addition (going forward: instead) is an adjustment to
x86_cpu_policy_fill_native().
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -357,6 +357,22 @@ 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 0x8000000a.edx, word 18 */
> +XEN_CPUFEATURE(NPT, 18*32+ 0) /*h Nested page table support */
> +XEN_CPUFEATURE(LBRV, 18*32+ 1) /*h LBR virtualization support */
> +XEN_CPUFEATURE(SVML, 18*32+ 2) /* SVM locking MSR support */
> +XEN_CPUFEATURE(NRIPS, 18*32+ 3) /*h Next RIP save on VMEXIT support */
> +XEN_CPUFEATURE(TSCRATEMSR, 18*32+ 4) /* TSC ratio MSR support */
> +XEN_CPUFEATURE(VMCBCLEAN, 18*32+ 5) /*h VMCB clean bits support */
> +XEN_CPUFEATURE(FLUSHBYASID, 18*32+ 6) /* TLB flush by ASID support */
> +XEN_CPUFEATURE(DECODEASSISTS, 18*32+ 7) /*h Decode assists support */
> +XEN_CPUFEATURE(PAUSEFILTER, 18*32+10) /*h Pause intercept filter support */
> +XEN_CPUFEATURE(PAUSETHRESH, 18*32+12) /* Pause intercept filter threshold */
> +XEN_CPUFEATURE(VLOADSAVE, 18*32+15) /* virtual vmload/vmsave */
> +XEN_CPUFEATURE(VGIF, 18*32+16) /* Virtual GIF */
> +XEN_CPUFEATURE(SSS, 18*32+19) /* NPT Supervisor Shadow Stacks */
> +XEN_CPUFEATURE(SPEC_CTRL, 18*32+20) /* MSR_SPEC_CTRL virtualisation */
This can't be just SPEC_CTRL without causing confusion. I guess it wants to
be VIRT_SPEC_CTRL (probably confusing, too), AMD_VIRT_SPEC_CTRL,
AMD_SPEC_CTRL_VIRT, or some such.
> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -22,6 +22,7 @@
> #define FEATURESET_7d1 15 /* 0x00000007:1.edx */
> #define FEATURESET_m10Al 16 /* 0x0000010a.eax */
> #define FEATURESET_m10Ah 17 /* 0x0000010a.edx */
> +#define FEATURESET_ead 18 /* 0x8000000a.edx */
Maybe better eAd here and elsewhere, to visually separate the constituent
pieces of the name? I wonder whether Andrew had any plans naming-wise here.
Jan
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] x86/cpuid-policy: Add AMD SVM CPUID leaf to featureset
2024-04-22 15:09 ` Jan Beulich
@ 2024-04-22 15:14 ` Jan Beulich
0 siblings, 0 replies; 3+ messages in thread
From: Jan Beulich @ 2024-04-22 15:14 UTC (permalink / raw)
To: George Dunlap; +Cc: Andrew Cooper, Roger Pau Monne, xen-devel
On 22.04.2024 17:09, Jan Beulich wrote:
> On 17.04.2024 15:22, George Dunlap wrote:
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -477,6 +477,8 @@ static void generic_identify(struct cpuinfo_x86 *c)
>> c->x86_capability[FEATURESET_e7d] = cpuid_edx(0x80000007);
>> if (c->extended_cpuid_level >= 0x80000008)
>> c->x86_capability[FEATURESET_e8b] = cpuid_ebx(0x80000008);
>> + if (c->extended_cpuid_level >= 0x8000000a)
>> + c->x86_capability[FEATURESET_ead] = cpuid_edx(0x8000000a);
>> if (c->extended_cpuid_level >= 0x80000021)
>> c->x86_capability[FEATURESET_e21a] = cpuid_eax(0x80000021);
>
> Aiui this is needed right in this change because of calculate_host_policy()
> deriving from boot_cpu_data.x86_capability. What I'd have expected in
> addition (going forward: instead) is an adjustment to
> x86_cpu_policy_fill_native().
I'm sorry, but no, there should be no need to adjust that function.
Jan
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-22 15:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-17 13:22 [PATCH] x86/cpuid-policy: Add AMD SVM CPUID leaf to featureset George Dunlap
2024-04-22 15:09 ` Jan Beulich
2024-04-22 15:14 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.