Re: [PATCH v10 03/26] x86/fpu/xstate: Introduce CET MSR XSAVES supervisor states

Sean Christopherson
2020-07-23T16:10:43Z

On Wed, Apr 29, 2020 at 03:07:09PM -0700, Yu-cheng Yu wrote:
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 12c9684d59ba..47f603729543 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -885,4 +885,22 @@
> #define MSR_VM_IGNNE 0xc0010115
> #define MSR_VM_HSAVE_PA 0xc0010117
>
> +/* Control-flow Enforcement Technology MSRs */
> +#define MSR_IA32_U_CET 0x6a0 /* user mode cet setting */
> +#define MSR_IA32_S_CET 0x6a2 /* kernel mode cet setting */
> +#define MSR_IA32_PL0_SSP 0x6a4 /* kernel shstk pointer */
> +#define MSR_IA32_PL1_SSP 0x6a5 /* ring-1 shstk pointer */
> +#define MSR_IA32_PL2_SSP 0x6a6 /* ring-2 shstk pointer */
> +#define MSR_IA32_PL3_SSP 0x6a7 /* user shstk pointer */
> +#define MSR_IA32_INT_SSP_TAB 0x6a8 /* exception shstk table */
> +
> +/* MSR_IA32_U_CET and MSR_IA32_S_CET bits */
> +#define MSR_IA32_CET_SHSTK_EN 0x0000000000000001ULL

Can we drop the MSR_IA32 prefix for the individual bits? Mostly to yield
shorter line lengths, but also because it's more or less redundant info,
and in some ways unhelpful as it's hard to quickly differentiate between
"this is an MSR index" and "this is a bit/mask for an MSR".

My vote would also be to use BIT() or BIT_ULL(). The SDM defines the flags
by their (decimal) bit number. Manually converting the bits to masks makes
it difficult to check for correctness.

E.g.

#define CET_SHSTK_EN BIT(0)
#define CET_WRSS_EN BIT(1)
#define CET_ENDBR_EN BIT(2)
#define CET_LEG_IW_EN BIT(3)
#define CET_NO_TRACK_EN BIT(4)
#define CET_WAIT_ENDBR BIT(5)

> +#define MSR_IA32_CET_WRSS_EN 0x0000000000000002ULL
> +#define MSR_IA32_CET_ENDBR_EN 0x0000000000000004ULL
> +#define MSR_IA32_CET_LEG_IW_EN 0x0000000000000008ULL
> +#define MSR_IA32_CET_NO_TRACK_EN 0x0000000000000010ULL
> +#define MSR_IA32_CET_WAIT_ENDBR 0x00000000000000800UL
> +#define MSR_IA32_CET_BITMAP_MASK 0xfffffffffffff000ULL

This particular define, the so called BITMAP_MASK, is no longer used in the
IBT series. IMO it'd be better off dropping this mask as it's not clear
from the name that this is really nothing more than a mask for a virtual
address, e.g. at first glance (for someone without CET knowledge) it looks
like bits 63:12 hold a bitmap as opposed to holding a pointer to a bitmap.