Re: [PATCH v5 08/10] x86/mm: make code robust to future PAT changes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.12.2022 02:07, Demi Marie Obenour wrote:
> It may be desirable to change Xen's PAT for various reasons.  This
> requires changes to several _PAGE_* macros as well.  Add static
> assertions to check that XEN_MSR_PAT is consistent with the _PAGE_*
> macros, and that _PAGE_WB is 0 as required by Linux.

In line with the code comment, perhaps add (not just)"?

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -6352,6 +6352,11 @@ unsigned long get_upper_mfn_bound(void)
>      return min(max_mfn, 1UL << (paddr_bits - PAGE_SHIFT)) - 1;
>  }
>  
> +
> +/*
> + * A bunch of static assertions to check that the XEN_MSR_PAT is valid
> + * and consistent with the _PAGE_* macros, and that _PAGE_WB is zero.
> + */
>  static void __init __maybe_unused build_assertions(void)
>  {
>      /*
> @@ -6361,6 +6366,72 @@ static void __init __maybe_unused 
> build_assertions(void)
>       * using different PATs will not work.
>       */
>      BUILD_BUG_ON(XEN_MSR_PAT != 0x050100070406ULL);
> +
> +    /*
> +     * _PAGE_WB must be zero for several reasons, not least because Linux
> +     * assumes it.
> +     */
> +    BUILD_BUG_ON(_PAGE_WB);

Strictly speaking this is a requirement only for PV guests. We may not
want to go as far as putting "#ifdef CONFIG_PV" around it, but at least
the code comment (and then also the part of the description referring
to this) will imo want to say so.

> +    /* A macro to convert from cache attributes to actual cacheability */
> +#define PAT_ENTRY(v) (0xFF & (XEN_MSR_PAT >> (8 * (v))))

I don't think the comment is appropriate here. All you do is extract a
slot from the hard-coded PAT value we use.

> +    /* Validate at compile-time that v is a valid value for a PAT entry */
> +#define CHECK_PAT_ENTRY_VALUE(v)                                             
>   \
> +    BUILD_BUG_ON((v) < 0 || (v) > 7 ||                                       
>   \

PAT_ENTRY() won't produce negative values, so I don't think "(v) < 0" is
really needed here. And the "(v) > 7" will imo want replacing by
"(v) >= X86_NUM_MT".

> +                 (v) == X86_MT_RSVD_2 || (v) == X86_MT_RSVD_3)
> +
> +    /* Validate at compile-time that PAT entry v is valid */
> +#define CHECK_PAT_ENTRY(v) do {                                              
>   \
> +    BUILD_BUG_ON((v) < 0 || (v) > 7);                                        
>   \

I think this would better be part of PAT_ENTRY(), as the validity of the
shift there depends on it. If this check is needed in the first place,
seeing that the macro is #undef-ed right after use below, and hence the
checks only cover the eight invocations of the macro right here.

> +    CHECK_PAT_ENTRY_VALUE(PAT_ENTRY(v));                                     
>   \
> +} while (0);

Nit (style): Missing blanks around 0 and perhaps better nowadays to use
"false" in such constructs. (Because of other comments this may go away
here anyway, but there's another similar instance below).

> +    /*
> +     * If one of these trips, the corresponding entry in XEN_MSR_PAT is 
> invalid.
> +     * This would cause Xen to crash (with #GP) at startup.
> +     */
> +    CHECK_PAT_ENTRY(0);
> +    CHECK_PAT_ENTRY(1);
> +    CHECK_PAT_ENTRY(2);
> +    CHECK_PAT_ENTRY(3);
> +    CHECK_PAT_ENTRY(4);
> +    CHECK_PAT_ENTRY(5);
> +    CHECK_PAT_ENTRY(6);
> +    CHECK_PAT_ENTRY(7);
> +
> +#undef CHECK_PAT_ENTRY
> +#undef CHECK_PAT_ENTRY_VALUE
> +
> +    /* Macro version of page_flags_to_cacheattr(), for use in 
> BUILD_BUG_ON()s */

DYM pte_flags_to_cacheattr()? At which point the macro name wants to
match and its parameter may also better be named "pte_value"?

> +#define PAGE_FLAGS_TO_CACHEATTR(page_value)                                  
>   \
> +    ((((page_value) >> 5) & 4) | (((page_value) >> 3) & 3))

Hmm, yet more uses of magic literal numbers.

> +    /* Check that a PAT-related _PAGE_* macro is correct */
> +#define CHECK_PAGE_VALUE(page_value) do {                                    
>   \
> +    /* Check that the _PAGE_* macros only use bits from PAGE_CACHE_ATTRS */  
>   \
> +    BUILD_BUG_ON(((_PAGE_##page_value) & PAGE_CACHE_ATTRS) !=                
>   \
> +                  (_PAGE_##page_value));                                     
>   \

Nit (style): One too many blanks used for indentation.

> +    /* Check that the _PAGE_* are consistent with XEN_MSR_PAT */             
>   \
> +    BUILD_BUG_ON(PAT_ENTRY(PAGE_FLAGS_TO_CACHEATTR(_PAGE_##page_value)) !=   
>   \
> +                 (X86_MT_##page_value));                                     
>   \

Nit (style): Nowadays we tend to consider ## a binary operator just like
e.g. +, and hence it wants to be surrounded by blanks.

> +} while (0)
> +
> +    /*
> +     * If one of these trips, the corresponding _PAGE_* macro is inconsistent
> +     * with XEN_MSR_PAT.  This would cause Xen to use incorrect cacheability
> +     * flags, with results that are undefined and probably harmful.

Why "undefined"? They may be wrong / broken, but the result is still well-
defined afaict.

Jan