| Message ID | 20240429151625.977884-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: AMD CPUID handling improvements | expand |
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 0x80000009. */ > - uint64_t :64, :64; /* Leaf 0x8000000a - SVM rev and features. */ > + > + /* Leaf 0x8000000a - 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 <jbeulich@suse.com> Jan
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 0x80000009. */ >> - uint64_t :64, :64; /* Leaf 0x8000000a - SVM rev and features. */ >> + >> + /* Leaf 0x8000000a - 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 <jbeulich@suse.com> Thanks. ~Andrew
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
On Tue, Apr 30, 2024 at 2:25 PM Andrew Cooper <andrew.cooper3@citrix.com> 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
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(0x00000007, 1, CPUID_REG_EDX), MSR_ENTRY(0x10a, CPUID_REG_EAX), MSR_ENTRY(0x10a, CPUID_REG_EDX), + CPUID_ENTRY(0x8000000a, NA, CPUID_REG_EDX), + CPUID_ENTRY(0x8000001f, 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 0x00000007:1.edx", "7d1", str_7d1 }, { "MSR_ARCH_CAPS.lo", "m10Al", str_m10Al }, { "MSR_ARCH_CAPS.hi", "m10Ah", str_m10Ah }, + { "CPUID 0x8000000a.edx", "eAd", str_eAd }, + { "CPUID 0x8000001f.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(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 >= 0x8000001f) + c->x86_capability[FEATURESET_e1Fa] = cpuid_eax(0x8000001f); 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 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 0x8000000a.edx, word 18 */ + +/* AMD-defined CPU features, CPUID level 0x8000001f.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 /* 0x00000007:1.edx */ #define FEATURESET_m10Al 16 /* 0x0000010a.eax */ #define FEATURESET_m10Ah 17 /* 0x0000010a.edx */ +#define FEATURESET_eAd 18 /* 0x8000000a.edx */ +#define FEATURESET_e1Fa 19 /* 0x8000001f.eax */ struct cpuid_leaf { @@ -296,7 +298,16 @@ 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. */ + 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 0x8000000b. */ uint64_t :64, :64; /* Leaf 0x8000000c. */ uint64_t :64, :64; /* Leaf 0x8000000d. */ @@ -317,7 +328,16 @@ struct cpu_policy uint64_t :64, :64; /* Leaf 0x8000001c. */ uint64_t :64, :64; /* Leaf 0x8000001d - Cache properties. */ uint64_t :64, :64; /* Leaf 0x8000001e - Extd APIC/Core/Node IDs. */ - uint64_t :64, :64; /* Leaf 0x8000001f - AMD Secure Encryption. */ + + /* Leaf 0x8000001f - AMD Secure Encryption. */ + union { + uint32_t e1Fa; + struct { DECL_BITFIELD(e1Fa); }; + }; + uint32_t cbit:6, paddr_reduce:6, nr_vmpls:4, :16; + uint32_t nr_enc_guests; + uint32_t sev_min_asid; + uint64_t :64, :64; /* Leaf 0x80000020 - Platform QoS. */ /* Leaf 0x80000021 - Extended Feature 2 */ diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c index eb7698dc7325..14deb01a6d0b 100644 --- a/xen/lib/x86/cpuid.c +++ b/xen/lib/x86/cpuid.c @@ -81,6 +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; + fs[FEATURESET_e1Fa] = p->extd.e1Fa; } void x86_cpu_featureset_to_policy( @@ -104,6 +106,8 @@ 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]; + p->extd.e1Fa = fs[FEATURESET_e1Fa]; } void x86_cpu_policy_recalc_synth(struct cpu_policy *p)
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 <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Xenia Ragiadakou <xenia.ragiadakou@amd.com> CC: Sergiy Kibrik <Sergiy_Kibrik@epam.com> CC: George Dunlap <george.dunlap@citrix.com> CC: Andrei Semenov <andrei.semenov@vates.fr> CC: Vaishali Thakkar <vaishali.thakkar@vates.tech> --- 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(-)