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

2023-01-05 Thread Andrew Cooper
On 05/01/2023 2:01 pm, Jan Beulich wrote:
> On 22.12.2022 23:31, Demi Marie Obenour wrote:
>> --- 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;
>>  }
>>  
>> +
>> +/*
> Nit: Please avoid introducing double blank lines.
>
>> + * 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.
> This comment is too specific for a function of ...
>
>> + */
>>  static void __init __maybe_unused build_assertions(void)
> ... this name, in this file.

IMO, you really don't need to comment build_assertions().  It's a
pattern we use elsewhere, and the BUILD_BUG_ON()'s are individually
commented.

I'd just drop this hunk entirely.

~Andrew


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

2023-01-05 Thread Jan Beulich
On 22.12.2022 23:31, Demi Marie Obenour wrote:
> --- 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;
>  }
>  
> +
> +/*

Nit: Please avoid introducing double blank lines.

> + * 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.

This comment is too specific for a function of ...

> + */
>  static void __init __maybe_unused build_assertions(void)

... this name, in this file.

> @@ -6361,6 +6366,71 @@ 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
> + * PV guests assume it.
> + */
> +BUILD_BUG_ON(_PAGE_WB);
> +
> +#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) < 0 || (v) > 7 ||   
>   \

See my v5 comments.

Jan



[PATCH v6 3/5] x86/mm: make code robust to future PAT changes

2022-12-22 Thread Demi Marie Obenour
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.

Signed-off-by: Demi Marie Obenour 
---
Changes since v5:
- Remove unhelpful comment.
- Move a BUILD_BUG_ON.
- Use fewer hardcoded constants in PTE_FLAGS_TO_CACHEATTR.
- Fix coding style.

Changes since v4:
- Add lots of comments explaining what the various BUILD_BUG_ON()s mean.

Changes since v3:
- Refactor some macros
- Avoid including a string literal in BUILD_BUG_ON
---
 xen/arch/x86/mm.c | 70 +++
 1 file changed, 70 insertions(+)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 
3558ca215b02a517d55d75329d645ae5905424e4..65ba0f58ed8c26ac0343528303851739981c03bd
 100644
--- 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,71 @@ 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
+ * PV guests assume it.
+ */
+BUILD_BUG_ON(_PAGE_WB);
+
+#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) < 0 || (v) > 7 || 
\
+ (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);
+
+#undef CHECK_PAT_ENTRY
+#undef CHECK_PAT_ENTRY_VALUE
+
+/* Macro version of pte_flags_to_cacheattr(), for use in BUILD_BUG_ON()s */
+#define PTE_FLAGS_TO_CACHEATTR(pte_value)  
\
+pte_value) & _PAGE_PAT) >> 5) |
\
+ (((pte_value) & (_PAGE_PCD | _PAGE_PWT)) >> 3))
+
+/* 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)); 
\
+} 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.
+ */
+CHECK_PAGE_VALUE(WT);
+CHECK_PAGE_VALUE(WB);
+CHECK_PAGE_VALUE(WC);
+CHECK_PAGE_VALUE(UC);
+CHECK_PAGE_VALUE(UCM);
+CHECK_PAGE_VALUE(WP);
+
+#undef CHECK_PAGE_VALUE
+#undef PAGE_FLAGS_TO_CACHEATTR
+#undef PAT_ENTRY
 }
 
 /*
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab