Re: [PATCH] x86/cpufeatures: Add new cpuid features in SPR to featureset

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/08/2024 4:52 pm, Matthew Barnes wrote:
> Upon running `xen-cpuid -d` on a host machine with Sapphire Rapids

I presume you mean -v ?

> within Dom0, there exist unrecognised features.
>
> This patch adds these features as macros to the CPU featureset,
> disabled by default.
>
> Signed-off-by: Matthew Barnes <matthew.barnes@xxxxxxxxx>
> ---
>  xen/include/public/arch-x86/cpufeatureset.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/xen/include/public/arch-x86/cpufeatureset.h 
> b/xen/include/public/arch-x86/cpufeatureset.h
> index abab78fa86db..db633bd5c128 100644
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -121,6 +121,7 @@ XEN_CPUFEATURE(SMX,           1*32+ 6) /*   Safer Mode 
> Extensions */
>  XEN_CPUFEATURE(EIST,          1*32+ 7) /*   Enhanced SpeedStep */
>  XEN_CPUFEATURE(TM2,           1*32+ 8) /*   Thermal Monitor 2 */
>  XEN_CPUFEATURE(SSSE3,         1*32+ 9) /*A  Supplemental Streaming SIMD 
> Extensions-3 */
> +XEN_CPUFEATURE(SDGB,          1*32+11) /*   Silicon Debugging */
>  XEN_CPUFEATURE(FMA,           1*32+12) /*A  Fused Multiply Add */
>  XEN_CPUFEATURE(CX16,          1*32+13) /*A  CMPXCHG16B */
>  XEN_CPUFEATURE(XTPR,          1*32+14) /*   Send Task Priority Messages */
> @@ -181,6 +182,7 @@ XEN_CPUFEATURE(XSAVEOPT,      4*32+ 0) /*A  XSAVEOPT 
> instruction */
>  XEN_CPUFEATURE(XSAVEC,        4*32+ 1) /*A  XSAVEC/XRSTORC instructions */
>  XEN_CPUFEATURE(XGETBV1,       4*32+ 2) /*A  XGETBV with %ecx=1 */
>  XEN_CPUFEATURE(XSAVES,        4*32+ 3) /*S  XSAVES/XRSTORS instructions */
> +XEN_CPUFEATURE(XFD,           4*32+ 4) /*   Extended Feature Disable */

/* MSR_XFD{,_ERR} (eXtended Feature Disable) */

>  
>  /* Intel-defined CPU features, CPUID level 0x00000007:0.ebx, word 5 */
>  XEN_CPUFEATURE(FSGSBASE,      5*32+ 0) /*A  {RD,WR}{FS,GS}BASE instructions 
> */
> @@ -221,6 +223,7 @@ XEN_CPUFEATURE(AVX512_VBMI,   6*32+ 1) /*A  AVX-512 
> Vector Byte Manipulation Ins
>  XEN_CPUFEATURE(UMIP,          6*32+ 2) /*S  User Mode Instruction Prevention 
> */
>  XEN_CPUFEATURE(PKU,           6*32+ 3) /*H  Protection Keys for Userspace */
>  XEN_CPUFEATURE(OSPKE,         6*32+ 4) /*!  OS Protection Keys Enable */
> +XEN_CPUFEATURE(WAITPKG,       6*32+ 5) /*   User-level monitoring support */

UMONITOR/UMWAIT/TPAUSE

It's more than just monitoring support.

>  XEN_CPUFEATURE(AVX512_VBMI2,  6*32+ 6) /*A  Additional AVX-512 Vector Byte 
> Manipulation Instrs */
>  XEN_CPUFEATURE(CET_SS,        6*32+ 7) /*   CET - Shadow Stacks */
>  XEN_CPUFEATURE(GFNI,          6*32+ 8) /*A  Galois Field Instrs */
> @@ -228,13 +231,16 @@ XEN_CPUFEATURE(VAES,          6*32+ 9) /*A  Vector AES 
> Instrs */
>  XEN_CPUFEATURE(VPCLMULQDQ,    6*32+10) /*A  Vector Carry-less Multiplication 
> Instrs */
>  XEN_CPUFEATURE(AVX512_VNNI,   6*32+11) /*A  Vector Neural Network Instrs */
>  XEN_CPUFEATURE(AVX512_BITALG, 6*32+12) /*A  Support for VPOPCNT[B,W] and 
> VPSHUFBITQMB */
> +XEN_CPUFEATURE(TME_EN,        6*32+13) /*   TME MSR support */

Just TME as a name.  We don't tend to have suffixes like
enabled/supported because they're just line-noise.

As a comment, go for Total Memory Encryption.  It's more than just an MSR.

>  XEN_CPUFEATURE(AVX512_VPOPCNTDQ, 6*32+14) /*A  POPCNT for vectors of DW/QW */
> +XEN_CPUFEATURE(LA57,          6*32+16) /*   57-bit linear addressing and 
> five-level paging */

"5-level paging (57-bit linear addresses)" is more concise.

>  XEN_CPUFEATURE(RDPID,         6*32+22) /*A  RDPID instruction */
>  XEN_CPUFEATURE(BLD,           6*32+24) /*   BusLock Detect (#DB trap) 
> support */
>  XEN_CPUFEATURE(CLDEMOTE,      6*32+25) /*A  CLDEMOTE instruction */
>  XEN_CPUFEATURE(MOVDIRI,       6*32+27) /*a  MOVDIRI instruction */
>  XEN_CPUFEATURE(MOVDIR64B,     6*32+28) /*a  MOVDIR64B instruction */
>  XEN_CPUFEATURE(ENQCMD,        6*32+29) /*   ENQCMD{,S} instructions */
> +XEN_CPUFEATURE(SGX_LC,        6*32+30) /*   SGX Launch Configuration */

This is an irritating case where the SDM disagrees with all the
whitepapers on the name.  Everywhere else calls it Control, not
Configuration (and technically, "Flexible" at that).

I doubt the difference matters in practice.

>  XEN_CPUFEATURE(PKS,           6*32+31) /*H  Protection Key for Supervisor */
>  
>  /* AMD-defined CPU features, CPUID level 0x80000007.edx, word 7 */
> @@ -264,6 +270,7 @@ XEN_CPUFEATURE(BTC_NO,        8*32+29) /*A  Hardware not 
> vulnerable to Branch Ty
>  XEN_CPUFEATURE(IBPB_RET,      8*32+30) /*A  IBPB clears RSB/RAS too. */
>  
>  /* Intel-defined CPU features, CPUID level 0x00000007:0.edx, word 9 */
> +XEN_CPUFEATURE(SGX_KEYS,      9*32+ 1) /*   Attestation Services for Intel 
> SGX */

Again, "SGX Attestation Service" is more concise.

>  XEN_CPUFEATURE(AVX512_4VNNIW, 9*32+ 2) /*   Xeon Phi AVX512 Neural Network 
> Instructions */
>  XEN_CPUFEATURE(AVX512_4FMAPS, 9*32+ 3) /*   Xeon Phi AVX512 Multiply 
> Accumulation Single Precision */
>  XEN_CPUFEATURE(FSRM,          9*32+ 4) /*A  Fast Short REP MOVS */
> @@ -276,10 +283,13 @@ XEN_CPUFEATURE(TSX_FORCE_ABORT, 9*32+13) /* 
> MSR_TSX_FORCE_ABORT.RTM_ABORT */
>  XEN_CPUFEATURE(SERIALIZE,     9*32+14) /*A  SERIALIZE insn */
>  XEN_CPUFEATURE(HYBRID,        9*32+15) /*   Heterogeneous platform */
>  XEN_CPUFEATURE(TSXLDTRK,      9*32+16) /*a  TSX load tracking suspend/resume 
> insns */
> +XEN_CPUFEATURE(PCONFIG,       9*32+18) /*   Platform configuration support */

"PCONFIG insn".  That's a lot more meaningful to someone glancing at
this file.  "platform configuration" is a functionally infinite set of
things.

>  XEN_CPUFEATURE(ARCH_LBR,      9*32+19) /*   Architectural Last Branch Record 
> */
>  XEN_CPUFEATURE(CET_IBT,       9*32+20) /*   CET - Indirect Branch Tracking */
> +XEN_CPUFEATURE(AMX_BF16,      9*32+22) /*   Tile computational operations on 
> bfloat16 numbers */
>  XEN_CPUFEATURE(AVX512_FP16,   9*32+23) /*A  AVX512 FP16 instructions */
>  XEN_CPUFEATURE(AMX_TILE,      9*32+24) /*   AMX Tile architecture */
> +XEN_CPUFEATURE(AMX_INT8,      9*32+25) /*   Tile computational operations on 
> 8-bit integers */
>  XEN_CPUFEATURE(IBRSB,         9*32+26) /*A  IBRS and IBPB support (used by 
> Intel) */
>  XEN_CPUFEATURE(STIBP,         9*32+27) /*A  STIBP */
>  XEN_CPUFEATURE(L1D_FLUSH,     9*32+28) /*S  MSR_FLUSH_CMD and L1D flush. */
> @@ -363,6 +373,8 @@ XEN_CPUFEATURE(GDS_CTRL,           16*32+25) /*   
> MCU_OPT_CTRL.GDS_MIT_{DIS,LOCK
>  XEN_CPUFEATURE(GDS_NO,             16*32+26) /*A  No Gather Data Sampling */
>  XEN_CPUFEATURE(RFDS_NO,            16*32+27) /*A  No Register File Data 
> Sampling */
>  XEN_CPUFEATURE(RFDS_CLEAR,         16*32+28) /*!A| Register File(s) cleared 
> by VERW */
> +XEN_CPUFEATURE(IGN_UMONITOR_SUPPORT, 16*32+29) /*  UMONITOR Ignore support */
> +XEN_CPUFEATURE(MON_UMON_MITG_SUPPORT, 16*32+30) /*  (U)MONITOR Mitigation 
> support */

These two want splitting out into separate patch, which does more than
just this.

Read the whitepaper:
https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/monitor-umonitor-performance-guidance.html

and we want all 5 new enumerations, even the new CPUID bit (because it
will start existing on GNR/CWF I believe).  Also, ARCH_CAPS is special
because it's duplicated here and in msr-index.h

Finally, somewhere in the Intel bringup code, if we see
ARCH_CAPS.IGN_UMON then we want to force MCU_OPT.IGN_UMON (because we
don't support UMONITOR) and clear MCU_OPT.MON_MITG.

The ucode-load defaults are safe for Xen (this is why we didn't XSA it
on the embargo date), but there's a corner case if something earlier in
the boot chain decided to activate the less-safe mode.

~Andrew