Re: [PATCH 1/5] x86/cpu-policy: Infrastructure for the AMD SVM and SEV leaves
On Tue, Apr 30, 2024 at 2:25 PM Andrew Cooper wrote: > > On 30/04/2024 1:45 pm, Jan Beulich wrote: > > On 29.04.2024 17:16, Andrew Cooper wrote: > >> Allocate two new feature leaves, and extend cpu_policy with the non-feature > >> fields too. > >> > >> The CPUID dependency between the SVM bit on the whole SVM leaf is > >> intentionally deferred, to avoid transiently breaking nested virt. > > In reply to this I meant to ask that you at least add those dependencies in > > commented-out form, such that from looking at gen-cpuid.py it becomes clear > > they're intentionally omitted. But you don't add feature identifiers either, > > making dependencies impossible to express. Maybe this sentence was really > > meant for another of the patches? (Then my request would actually apply > > there.) > > This is necessary because c/s 4f8b0e94d7ca is buggy. Notice how it puts > an edit to the policy object in the middle of a block of logic editing > the featureset, which ends with writing the featureset back over the > policy object. > > And it's not the first outstanding problem from what is a very small > number of nested-virt patches so far... I specifically raised this on the x86 maintainers call, and you said to go ahead with it. -George
Re: [PATCH 1/5] x86/cpu-policy: Infrastructure for the AMD SVM and SEV leaves
On 30.04.2024 15:25, Andrew Cooper wrote: > On 30/04/2024 1:45 pm, Jan Beulich wrote: >> On 29.04.2024 17:16, Andrew Cooper wrote: >>> Allocate two new feature leaves, and extend cpu_policy with the non-feature >>> fields too. >>> >>> The CPUID dependency between the SVM bit on the whole SVM leaf is >>> intentionally deferred, to avoid transiently breaking nested virt. >> In reply to this I meant to ask that you at least add those dependencies in >> commented-out form, such that from looking at gen-cpuid.py it becomes clear >> they're intentionally omitted. But you don't add feature identifiers either, >> making dependencies impossible to express. Maybe this sentence was really >> meant for another of the patches? (Then my request would actually apply >> there.) > > This is necessary because c/s 4f8b0e94d7ca is buggy. Notice how it puts > an edit to the policy object in the middle of a block of logic editing > the featureset, which ends with writing the featureset back over the > policy object. When seeing the description of that next patch replacing that code, I first thought you're right about that being buggy (i.e. not achieving the intended effect). But imo it isn't really buggy, as x86_cpu_featureset_to_policy() doesn't overwrite that leaf in the policy prior to the adjustment made there by this very patch. Nevertheless it also wasn't intended to be that way, I agree (and I should have noticed while reviewing the earlier change). This means, however, that there _is_ breakage now between this and the next patch, as now said leaf is indeed overwritten after its custom setting in calculate_hvm_max_policy(). So maybe you want to defer the x86_cpu_featureset_to_policy() adjustment until patch 2. Jan
Re: [PATCH 1/5] x86/cpu-policy: Infrastructure for the AMD SVM and SEV leaves
On 30/04/2024 1:45 pm, Jan Beulich wrote: > On 29.04.2024 17:16, Andrew Cooper wrote: >> Allocate two new feature leaves, and extend cpu_policy with the non-feature >> fields too. >> >> The CPUID dependency between the SVM bit on the whole SVM leaf is >> intentionally deferred, to avoid transiently breaking nested virt. > In reply to this I meant to ask that you at least add those dependencies in > commented-out form, such that from looking at gen-cpuid.py it becomes clear > they're intentionally omitted. But you don't add feature identifiers either, > making dependencies impossible to express. Maybe this sentence was really > meant for another of the patches? (Then my request would actually apply > there.) This is necessary because c/s 4f8b0e94d7ca is buggy. Notice how it puts an edit to the policy object in the middle of a block of logic editing the featureset, which ends with writing the featureset back over the policy object. And it's not the first outstanding problem from what is a very small number of nested-virt patches so far... >> @@ -296,7 +298,16 @@ struct cpu_policy >> uint32_t /* d */:32; >> >> uint64_t :64, :64; /* Leaf 0x8009. */ >> -uint64_t :64, :64; /* Leaf 0x800a - SVM rev and features. */ >> + >> +/* Leaf 0x800a - SVM rev and features. */ >> +uint8_t svm_rev, :8, :8, :8; >> +uint32_t /* b */ :32; >> +uint32_t nr_asids; > According to the doc I'm looking at it is %ebx which holds the number of > ASIDs and %ecx is reserved. With that adjusted That's fun... The PPR I used for this has it wrong. A sample of others match the APM, so I'll raise a bug with AMD against this PPR. > Reviewed-by: Jan Beulich Thanks. ~Andrew
Re: [PATCH 1/5] x86/cpu-policy: Infrastructure for the AMD SVM and SEV leaves
On 29.04.2024 17:16, Andrew Cooper wrote: > Allocate two new feature leaves, and extend cpu_policy with the non-feature > fields too. > > The CPUID dependency between the SVM bit on the whole SVM leaf is > intentionally deferred, to avoid transiently breaking nested virt. In reply to this I meant to ask that you at least add those dependencies in commented-out form, such that from looking at gen-cpuid.py it becomes clear they're intentionally omitted. But you don't add feature identifiers either, making dependencies impossible to express. Maybe this sentence was really meant for another of the patches? (Then my request would actually apply there.) > @@ -296,7 +298,16 @@ struct cpu_policy > uint32_t /* d */:32; > > uint64_t :64, :64; /* Leaf 0x8009. */ > -uint64_t :64, :64; /* Leaf 0x800a - SVM rev and features. */ > + > +/* Leaf 0x800a - SVM rev and features. */ > +uint8_t svm_rev, :8, :8, :8; > +uint32_t /* b */ :32; > +uint32_t nr_asids; According to the doc I'm looking at it is %ebx which holds the number of ASIDs and %ecx is reserved. With that adjusted Reviewed-by: Jan Beulich Jan
[PATCH 1/5] x86/cpu-policy: Infrastructure for the AMD SVM and SEV leaves
Allocate two new feature leaves, and extend cpu_policy with the non-feature
fields too.
The CPUID dependency between the SVM bit on the whole SVM leaf is
intentionally deferred, to avoid transiently breaking nested virt.
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/libs/light/libxl_cpuid.c | 2 ++
tools/misc/xen-cpuid.c | 10 +
xen/arch/x86/cpu/common.c | 4
xen/include/public/arch-x86/cpufeatureset.h | 4
xen/include/xen/lib/x86/cpu-policy.h| 24 +++--
xen/lib/x86/cpuid.c | 4
6 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index ce4f3c7095ba..c7a8b76f541d 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -342,6 +342,8 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list
*policy, const char* str)
CPUID_ENTRY(0x0007, 1, CPUID_REG_EDX),
MSR_ENTRY(0x10a, CPUID_REG_EAX),
MSR_ENTRY(0x10a, CPUID_REG_EDX),
+CPUID_ENTRY(0x800a, NA, CPUID_REG_EDX),
+CPUID_ENTRY(0x801f, NA, CPUID_REG_EAX),
#undef MSR_ENTRY
#undef CPUID_ENTRY
};
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 8893547bebce..ab09410a05d6 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -264,6 +264,14 @@ static const char *const str_m10Ah[32] =
{
};
+static const char *const str_eAd[32] =
+{
+};
+
+static const char *const str_e1Fa[32] =
+{
+};
+
static const struct {
const char *name;
const char *abbr;
@@ -288,6 +296,8 @@ static const struct {
{ "CPUID 0x0007:1.edx", "7d1", str_7d1 },
{ "MSR_ARCH_CAPS.lo", "m10Al", str_m10Al },
{ "MSR_ARCH_CAPS.hi", "m10Ah", str_m10Ah },
+{ "CPUID 0x800a.edx", "eAd", str_eAd },
+{ "CPUID 0x801f.eax", "e1Fa", str_e1Fa },
};
#define COL_ALIGN "24"
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 28d7f34c4dbe..25b11e6472b8 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -477,6 +477,10 @@ static void generic_identify(struct cpuinfo_x86 *c)
c->x86_capability[FEATURESET_e7d] = cpuid_edx(0x8007);
if (c->extended_cpuid_level >= 0x8008)
c->x86_capability[FEATURESET_e8b] = cpuid_ebx(0x8008);
+ if (c->extended_cpuid_level >= 0x800a)
+ c->x86_capability[FEATURESET_eAd] = cpuid_edx(0x800a);
+ if (c->extended_cpuid_level >= 0x801f)
+ c->x86_capability[FEATURESET_e1Fa] = cpuid_eax(0x801f);
if (c->extended_cpuid_level >= 0x8021)
c->x86_capability[FEATURESET_e21a] = cpuid_eax(0x8021);
diff --git a/xen/include/public/arch-x86/cpufeatureset.h
b/xen/include/public/arch-x86/cpufeatureset.h
index 53f13dec31f7..0f869214811e 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -357,6 +357,10 @@ 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 */
+
+/* AMD-defined CPU features, CPUID level 0x801f.eax, word 19 */
+
#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 d5e447e9dc06..936e00e4da73 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -22,6 +22,8 @@
#define FEATURESET_7d1 15 /* 0x0007:1.edx*/
#define FEATURESET_m10Al 16 /* 0x010a.eax */
#define FEATURESET_m10Ah 17 /* 0x010a.edx */
+#define FEATURESET_eAd 18 /* 0x800a.edx */
+#define FEATURESET_e1Fa 19 /* 0x801f.eax */
struct cpuid_leaf
{
@@ -296,7 +298,16 @@ struct cpu_policy
uint32_t /* d */:32;
uint64_t :64, :64; /* Leaf 0x8009. */
-uint64_t :64, :64; /* Leaf 0x800a - SVM rev and features. */
+
+/* Leaf 0x800a - SVM rev and features. */
+uint8_t svm_rev, :8, :8, :8;
+uint32_t /* b */ :32;
+uint32_t nr_asids;
+union {
+uint32_t eAd;
+struct { DECL_BITFIELD(eAd); };
+};
+
uint64_t :64, :64; /* Leaf 0x800b. */
uint64_t :64, :64; /* Leaf 0x800c. */
uint64_t :64, :64; /* Leaf 0x800d. */
@@ -317,7 +328,16 @@ struct cpu_policy
uint64_t :64, :64; /* Leaf 0x801c. */
uint64_t :64, :64; /* Leaf
