Re: [PATCH v7 3/4] x86/mm: make code robust to future PAT changes

[Jan Beulich <jbeulich@suse.com>]

On 07.01.2023 23:07, Demi Marie Obenour wrote:
> @@ -6412,6 +6414,100 @@ 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.  Linux PV guests assume that _PAGE_WB will be
> +     * zero, and indeed Linux has a BUILD_BUG_ON validating that their version
> +     * of _PAGE_WB *is* zero.  Furthermore, since _PAGE_WB is zero, it is quite
> +     * likely to be omitted from various parts of Xen, and indeed L1 PTE
> +     * validation code checks that ((l1f & PAGE_CACHE_ATTRs) == 0), not
> +     * ((l1f & PAGE_CACHE_ATTRs) == _PAGE_WB).
> +     */
> +    BUILD_BUG_ON(_PAGE_WB);
> +
> +    /* _PAGE_RSVD_1 must be less than _PAGE_RSVD_2 */
> +    BUILD_BUG_ON(_PAGE_RSVD_1 >= _PAGE_RSVD_2);
> +
> +#define PAT_ENTRY(v)                                                           \
> +    (BUILD_BUG_ON_ZERO(((v) < 0) || ((v) > 7)) +                               \
> +     (0xFF & (XEN_MSR_PAT >> (8 * (v)))))
> +
> +    /* Validate at compile-time that v is a valid value for a PAT entry */
> +#define CHECK_PAT_ENTRY_VALUE(v)                                               \
> +    BUILD_BUG_ON((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) CHECK_PAT_ENTRY_VALUE(PAT_ENTRY(v))
> +
> +    /*
> +     * 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);
> +
> +    /* Macro version of pte_flags_to_cacheattr(), for use in BUILD_BUG_ON()s */
> +#define PTE_FLAGS_TO_CACHEATTR(pte_value)                                      \
> +    /* Check that the _PAGE_* macros only use bits from PAGE_CACHE_ATTRS */    \
> +    (BUILD_BUG_ON_ZERO(((pte_value) & PAGE_CACHE_ATTRS) != (pte_value)) |      \

Slightly cheaper as BUILD_BUG_ON_ZERO((pte_value) & ~PAGE_CACHE_ATTRS)?

> +     (((pte_value) & _PAGE_PAT) >> 5) |                                        \
> +     (((pte_value) & (_PAGE_PCD | _PAGE_PWT)) >> 3))
> +
> +    CHECK_PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(_PAGE_RSVD_1));
> +    CHECK_PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(_PAGE_RSVD_2));

What do these two check that the 8 instances above don't already check?

> +#define PAT_ENTRY_FROM_FLAGS(x) PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(x))
> +
> +    /* Validate at compile time that X does not duplicate a smaller PAT entry */
> +#define CHECK_DUPLICATE_ENTRY(x, y)                                            \
> +    BUILD_BUG_ON((x) >= (y) &&                                                 \
> +                 (PAT_ENTRY_FROM_FLAGS(x) == PAT_ENTRY_FROM_FLAGS(y)))

Imo nothing says that the reserved entries come last. I'm therefore not
convinced of the usefulness of the two uses of this macro.

> +    /* 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));                                      \
> +    /* Check that the _PAGE_* are consistent with XEN_MSR_PAT */               \
> +    BUILD_BUG_ON(PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(_PAGE_ ## page_value)) !=    \
> +                 (X86_MT_ ## page_value));                                     \
> +    case _PAGE_ ## page_value:; /* ensure no duplicate values */               \

Wouldn't this better come first in the macro? The semicolon looks unnecessary
in any event.

> +    /*                                                                         \
> +     * Check that the _PAGE_* entries do not duplicate a smaller reserved      \
> +     * entry.                                                                  \
> +     */                                                                        \
> +    CHECK_DUPLICATE_ENTRY(_PAGE_ ## page_value, _PAGE_RSVD_1);                 \
> +    CHECK_DUPLICATE_ENTRY(_PAGE_ ## page_value, _PAGE_RSVD_2);                 \
> +    CHECK_PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(_PAGE_ ## page_value));             \
> +} while ( false )
> +
> +    /*
> +     * 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 unknown and possibly harmful.
> +     */
> +    switch (0) {

Nit: Style.

> +    CHECK_PAGE_VALUE(WT);
> +    CHECK_PAGE_VALUE(WB);
> +    CHECK_PAGE_VALUE(WC);
> +    CHECK_PAGE_VALUE(UC);
> +    CHECK_PAGE_VALUE(UCM);
> +    CHECK_PAGE_VALUE(WP);

All of these are lacking "break" and hence are liable to trigger static checker
warnings.

> +    case _PAGE_RSVD_1:
> +    case _PAGE_RSVD_2:
> +        break;
> +    }
> +#undef CHECK_PAT_ENTRY
> +#undef CHECK_PAT_ENTRY_VALUE
> +#undef CHECK_PAGE_VALUE
> +#undef PAGE_FLAGS_TO_CACHEATTR

PTE_FLAGS_TO_CACHEATTR?

> +#undef PAT_ENTRY

You also #define more than these 5 macros now (but as per above e.g.
CHECK_DUPLICATE_ENTRY() may go away again).

Jan