Re: [Qemu-devel][PATCH v5 1/4] x86/cpu: Add CET CPUID/XSAVES flags and data structures

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 5/10/2020 9:42 AM, Yang Weijiang wrote:
> CET feature SHSTK and IBT are enumerated via CPUID(EAX=0x7,0):ECX[bit 7]
> and EDX[bit 20] respectively. Two CET bits (bit 11 and 12) are defined in
> MSR_IA32_XSS to support XSAVES/XRSTORS. CPUID(EAX=0xd, 1):ECX[bit 11] and
> ECX[bit 12] correspond to CET states in user and supervisor mode respectively.
> 
> Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com>
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>   target/i386/cpu.h | 35 +++++++++++++++++++++++++++++++++++
>   1 file changed, 35 insertions(+)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index e818fc712a..ed03cd1760 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -489,6 +489,9 @@ typedef enum X86Seg {
>   #define XSTATE_ZMM_Hi256_BIT            6
>   #define XSTATE_Hi16_ZMM_BIT             7
>   #define XSTATE_PKRU_BIT                 9
> +#define XSTATE_RESERVED_BIT             10

I think this is unnecessary. bit 8 and so many other undefined bits are 
reserved too.

> +#define XSTATE_CET_U_BIT                11
> +#define XSTATE_CET_S_BIT                12
>   
>   #define XSTATE_FP_MASK                  (1ULL << XSTATE_FP_BIT)
>   #define XSTATE_SSE_MASK                 (1ULL << XSTATE_SSE_BIT)
> @@ -499,6 +502,19 @@ typedef enum X86Seg {
>   #define XSTATE_ZMM_Hi256_MASK           (1ULL << XSTATE_ZMM_Hi256_BIT)
>   #define XSTATE_Hi16_ZMM_MASK            (1ULL << XSTATE_Hi16_ZMM_BIT)
>   #define XSTATE_PKRU_MASK                (1ULL << XSTATE_PKRU_BIT)
> +#define XSTATE_RESERVED_MASK            (1ULL << XSTATE_RESERVED_BIT)

Ditto.

> +#define XSTATE_CET_U_MASK               (1ULL << XSTATE_CET_U_BIT)
> +#define XSTATE_CET_S_MASK               (1ULL << XSTATE_CET_S_BIT)
> +
> +/* CPUID feature bits available in XCR0 */
> +#define CPUID_XSTATE_USER_MASK  (XSTATE_FP_MASK | XSTATE_SSE_MASK | \
> +                                 XSTATE_YMM_MASK | XSTATE_BNDREGS_MASK | \
> +                                 XSTATE_BNDCSR_MASK | XSTATE_OPMASK_MASK | \
> +                                 XSTATE_ZMM_Hi256_MASK | \
> +                                 XSTATE_Hi16_ZMM_MASK | XSTATE_PKRU_MASK)
> +
> +/* CPUID feature bits available in XSS */
> +#define CPUID_XSTATE_KERNEL_MASK    (XSTATE_CET_U_MASK)

How about we name it XSTATE_XCR0_MASK and XSTATE_XSS_MASK.

They are not CPUID feature bit, at least the CPUID_ prefix needs to be 
removed.

>   
>   /* CPUID feature words */
>   typedef enum FeatureWord {
> @@ -536,6 +552,8 @@ typedef enum FeatureWord {
>       FEAT_VMX_EPT_VPID_CAPS,
>       FEAT_VMX_BASIC,
>       FEAT_VMX_VMFUNC,
> +    FEAT_XSAVES_LO,     /* CPUID[EAX=0xd,ECX=1].ECX */
> +    FEAT_XSAVES_HI,     /* CPUID[EAX=0xd,ECX=1].EDX */

I don't think *XSAVES* is a good name, because XSAVES and manipulate the 
features masked by XCR0 | MSR_IA32_XSS. But this CPUID leaf only 
enumerated XSTATE features for XSS.

How about name it FEAT_XSS_LO/HI? or FEAT_XSAVES_XSS_HO/HI

>       FEATURE_WORDS,
>   } FeatureWord;
>   
> @@ -743,6 +761,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
>   #define CPUID_7_0_ECX_WAITPKG           (1U << 5)
>   /* Additional AVX-512 Vector Byte Manipulation Instruction */
>   #define CPUID_7_0_ECX_AVX512_VBMI2      (1U << 6)
> +/* CET SHSTK feature */
> +#define CPUID_7_0_ECX_CET_SHSTK         (1U << 7)
>   /* Galois Field New Instructions */
>   #define CPUID_7_0_ECX_GFNI              (1U << 8)
>   /* Vector AES Instructions */
> @@ -770,6 +790,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
>   #define CPUID_7_0_EDX_AVX512_4VNNIW     (1U << 2)
>   /* AVX512 Multiply Accumulation Single Precision */
>   #define CPUID_7_0_EDX_AVX512_4FMAPS     (1U << 3)
> +/* CET IBT feature */
> +#define CPUID_7_0_EDX_CET_IBT           (1U << 20)
>   /* Speculation Control */
>   #define CPUID_7_0_EDX_SPEC_CTRL         (1U << 26)
>   /* Single Thread Indirect Branch Predictors */
> @@ -1260,6 +1282,19 @@ typedef struct XSavePKRU {
>       uint32_t padding;
>   } XSavePKRU;
>   
> +/* Ext. save area 11: User mode CET state */
> +typedef struct XSavesCETU {
> +    uint64_t u_cet;
> +    uint64_t user_ssp;
> +} XSavesCETU;
> +
> +/* Ext. save area 12: Supervisor mode CET state */
> +typedef struct XSavesCETS {
> +    uint64_t kernel_ssp;
> +    uint64_t pl1_ssp;
> +    uint64_t pl2_ssp;
> +} XSavesCETS;
> +
>   typedef struct X86XSaveArea {
>       X86LegacyXSaveArea legacy;
>       X86XSaveHeader header;
>