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
