Re: [PATCH 6/8] x86: Derive XEN_MSR_PAT from its individual entries

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 06/12/2022 04:33, Demi Marie Obenour wrote:
> This avoids it being a magic constant that is difficult for humans to
> decode.  Use a _Static_assert to check that the old and new values are
> identical.
>
> Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> ---
>  xen/arch/x86/include/asm/processor.h | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/include/asm/processor.h 
> b/xen/arch/x86/include/asm/processor.h
> index 
> 8e2816fae9b97bd4e153a30cc3802971fe0355af..64b75e444947c64e2e5eba457deec92a873d7a63
>  100644
> --- a/xen/arch/x86/include/asm/processor.h
> +++ b/xen/arch/x86/include/asm/processor.h
> @@ -92,13 +92,33 @@
>                            X86_EFLAGS_NT|X86_EFLAGS_DF|X86_EFLAGS_IF|    \
>                            X86_EFLAGS_TF)
>  
> +/* Individual entries in IA32_CR_PAT */
> +#define MSR_PAT_UC  _AC(0x00, ULL)
> +#define MSR_PAT_WC  _AC(0x01, ULL)
> +#define MSR_PAT_RESERVED_1  _AC(0x02, ULL)
> +#define MSR_PAT_RESERVED_2  _AC(0x03, ULL)
> +#define MSR_PAT_WT  _AC(0x04, ULL)
> +#define MSR_PAT_WP  _AC(0x05, ULL)
> +#define MSR_PAT_WB  _AC(0x06, ULL)
> +#define MSR_PAT_UCM _AC(0x07, ULL)

This isn't really correct.  Constants for MSRs typically live in
msr-index.h, but these are architectural x86 memory types.

These ought be

#define X86_MT_$X ...  (skipping the two reserved values)

in x86-defns.h, and the PAT_TYPE_*, MTRR_TYPE_* and EPT_EMT_* constants
want removing.

There are two minor restrictions (EPT can't have UCM, MTRR can't have
WC), but they are all operating in terms of architectural memory type
values, and the code ought to reflect this.

> +
>  /*
>   * Host IA32_CR_PAT value to cover all memory types.  This is not the default
>   * MSR_PAT value, and is an ABI with PV guests.
>   */
> -#define XEN_MSR_PAT _AC(0x050100070406, ULL)
> +#define XEN_MSR_PAT (MSR_PAT_WB  << 0x00 | \
> +                     MSR_PAT_WT  << 0x08 | \
> +                     MSR_PAT_UCM << 0x10 | \
> +                     MSR_PAT_UC  << 0x18 | \
> +                     MSR_PAT_WC  << 0x20 | \
> +                     MSR_PAT_WP  << 0x28 | \
> +                     MSR_PAT_UC  << 0x30 | \
> +                     MSR_PAT_UC  << 0x38 | \
> +                     0)
>  
>  #ifndef __ASSEMBLY__
> +_Static_assert(XEN_MSR_PAT == 0x050100070406ULL,
> +               "wrong XEN_MSR_PAT breaks PV guests");

This wants to be in the build_assertions() that you introduce in the
next patch, and a BUILD_BUG_ON().  We still support compilers which
don't know _Static_assert().

~Andrew