Re: [PATCH] x86/cpuid-policy: Add AMD SVM CPUID leaf to featureset

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxx>
> ---
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Roger Pau Monne <roger.pau@xxxxxxxxx>
> ---
>  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