| Message ID | 20240417132237.2946862-1-george.dunlap@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/cpuid-policy: Add AMD SVM CPUID leaf to featureset | expand |
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
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
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)
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(-)