Re: [PATCH 2/5] x86/cpu-policy: Add SVM features already used by Xen

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.04.2024 17:16, Andrew Cooper wrote:
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -358,6 +358,20 @@ 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 PageTables */
> +XEN_CPUFEATURE(V_LBR,              18*32+ 1) /*h  Virtualised LBR */
> +XEN_CPUFEATURE(SVM_LOCK,           18*32+ 2) /*   SVM locking MSR */
> +XEN_CPUFEATURE(NRIPS,              18*32+ 3) /*h  Next-RIP saved on VMExit */
> +XEN_CPUFEATURE(V_TSC_RATE,         18*32+ 4) /*   Virtualised TSC Ratio */
> +XEN_CPUFEATURE(VMCB_CLEANBITS,     18*32+ 5) /*!  VMCB Clean-bits */

Wouldn't this better be marked !h nevertheless?

As to the name - does it need to be this long? VMCB_CLEAN would be in line
with the PM. But yeah, while CLEANBITS would be clear in the context of this
leaf, there might be whatever other "cleanbits" elsewhere, so some qualifier
is wanted, I guess. As to the V_ prefixes you use for several of the
features: Isn't there a risk of this being ambiguous towards VT-x? Maybe
they should all be SVM_*, even if ...

> +XEN_CPUFEATURE(FLUSH_BY_ASID,      18*32+ 6) /*   TLB Flush by ASID */
> +XEN_CPUFEATURE(DECODE_ASSIST,      18*32+ 7) /*h  Decode assists */
> +XEN_CPUFEATURE(PAUSE_FILTER,       18*32+10) /*h  Pause filter */
> +XEN_CPUFEATURE(PAUSE_THRESH,       18*32+12) /*   Pause filter threshold */
> +XEN_CPUFEATURE(V_LOADSAVE,         18*32+15) /*   Virtualised VMLOAD/SAVE */
> +XEN_CPUFEATURE(V_GIF,              18*32+16) /*   Virtualised GIF */

... these two at least are clearly SVM terminology and hence already
unambiguous.

> +XEN_CPUFEATURE(NPT_SSS,            18*32+19) /*   NPT Supervisor Shadow 
> Stacks */
> +XEN_CPUFEATURE(V_SPEC_CTRL,        18*32+20) /*   Virtualised MSR_SPEC_CTRL 
> */

Whereas this, when used somewhere in isolation, would not make clear
whether AMD's or Intel's is meant.

> --- a/xen/tools/gen-cpuid.py
> +++ b/xen/tools/gen-cpuid.py
> @@ -280,6 +280,9 @@ def crunch_numbers(state):
>          # standard 3DNow in the earlier K6 processors.
>          _3DNOW: [_3DNOWEXT],
>  
> +        # The SVM bit enumerates the whole SVM leave.
> +        SVM: list(range(NPT, NPT + 32)),

Seeing this and taking it together with the somewhat confusing (to me) part
of the description of patch 1: What is it then that you try to avoid there,
when adding the dependencies here is okay, while doing so there would be
entirely impossible (short of there being identifiers)?

Jan