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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.12.2022 05:33, Demi Marie Obenour wrote:
> --- 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 would be the at least 4th instance of these constants in our tree
(see xen/arch/x86/include/asm/mtrr.h, xen/include/public/domctl.h, and
xen/include/public/hvm/dm_op.h). Since they're part of the public
interface, I'm inclined to suggest to use those. If that faces
opposition, the next best approach would apparently be to move mtrr.h's
into x86-defns.h (including the largely identical MTRR values), thus
then also allowing hvmloader sources (which currently open-code the
MTRR values in cacheattr.c).

>  /*
>   * 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");

I'm afraid this won't compile on all gcc versions we support; see
xen/include/xen/lib.h's conditional use thereof for BUILD_BUG_ON().
You really want to use BUILD_BUG_ON() here instead of partly open-
coding it. (Yes, it requires to be put where a statement can be
put, not just a declaration, but the check doesn't strictly need
to live in a header file, so that's reasonably easy to accommodate
for.)

Jan