Re: [PATCH v5 08/10] x86/mm: make code robust to future PAT changes
On 22.12.2022 11:00, Demi Marie Obenour wrote: > On Thu, Dec 22, 2022 at 10:54:48AM +0100, Jan Beulich wrote: >> On 22.12.2022 10:50, Demi Marie Obenour wrote: >>> On Thu, Dec 22, 2022 at 10:35:08AM +0100, Jan Beulich wrote: On 20.12.2022 02:07, Demi Marie Obenour wrote: > @@ -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. >>> >>> Does Xen itself depend on this? >> >> With the wording in the description I was going from the assumption that >> you have audited code and found that we properly use _PAGE_* constants >> everywhere. > > There could be other hard-coded uses of magic numbers I haven’t found, > and _PAGE_WB is currently zero so I would be quite surpised if no code > in Xen omits it. Linux also has a BUILD_BUG_ON() to check the same > thing. Fair enough - please adjust description and comment text accordingly then. Jan
Re: [PATCH v5 08/10] x86/mm: make code robust to future PAT changes
On Thu, Dec 22, 2022 at 10:54:48AM +0100, Jan Beulich wrote:
> On 22.12.2022 10:50, Demi Marie Obenour wrote:
> > On Thu, Dec 22, 2022 at 10:35:08AM +0100, Jan Beulich wrote:
> >> On 20.12.2022 02:07, 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;
> >>> }
> >>>
> >>> +
> >>> +/*
> >>> + * 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.
> >
> > Does Xen itself depend on this?
>
> With the wording in the description I was going from the assumption that
> you have audited code and found that we properly use _PAGE_* constants
> everywhere.
There could be other hard-coded uses of magic numbers I haven’t found,
and _PAGE_WB is currently zero so I would be quite surpised if no code
in Xen omits it. Linux also has a BUILD_BUG_ON() to check the same
thing.
> >>> +} 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.
> >
> > “undefined” is meant as “one has violated a core assumption that
> > higher-level stuff depends on, so things can go arbitrarily wrong,
> > including e.g. corrupting memory or data”. Is this accurate? Should I
> > drop the dependent clause, or do you have a suggestion for something
> > better?
>
> s/undefined/unknown/ ?
Will fix in v6.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
signature.asc
Description: PGP signature
Re: [PATCH v5 08/10] x86/mm: make code robust to future PAT changes
On 22.12.2022 10:50, Demi Marie Obenour wrote:
> On Thu, Dec 22, 2022 at 10:35:08AM +0100, Jan Beulich wrote:
>> On 20.12.2022 02:07, 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;
>>> }
>>>
>>> +
>>> +/*
>>> + * 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.
>
> Does Xen itself depend on this?
With the wording in the description I was going from the assumption that
you have audited code and found that we properly use _PAGE_* constants
everywhere.
>>> +} 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.
>
> “undefined” is meant as “one has violated a core assumption that
> higher-level stuff depends on, so things can go arbitrarily wrong,
> including e.g. corrupting memory or data”. Is this accurate? Should I
> drop the dependent clause, or do you have a suggestion for something
> better?
s/undefined/unknown/ ?
Jan
Re: [PATCH v5 08/10] x86/mm: make code robust to future PAT changes
On Thu, Dec 22, 2022 at 10:35:08AM +0100, Jan Beulich wrote:
> 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)"?
Will reword in v6.
> > --- 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.
Does Xen itself depend on this?
> > +/* 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.
Will drop in v6.
> > +/* 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".
Will fix in v6.
> > + (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).
Will fix in v6.
> > +/*
> > + * 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"?
Indeed so.
> > +#define PAGE_FLAGS_TO_CACHEATTR(page_value)
> > \
> > +page_value) >> 5) & 4) | (((page_value) >> 3) & 3))
>
> Hmm, yet more uses of magic literal numbers.
I wanted to keep the definition as close to that of
pte_flags_to_cacheattr() as possible.
> > +/* 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.
Will fix in v6.
> > +/* Check that the _PAGE_* are consistent with XEN_MSR_PAT */
> > \
> > +BUILD_BUG_ON(PAT_ENTRY(PAGE_FLAGS_TO_CACHEATTR(_PAGE_##page_value)) !=
> > \
> > +
Re: [PATCH v5 08/10] x86/mm: make code robust to future PAT changes
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, bu
[PATCH v5 08/10] x86/mm: make code robust to future PAT changes
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 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 | 71 +++
1 file changed, 71 insertions(+)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index
b40a575b61418ea1137299e68b64f7efd9efeced..a72556668633ee57b77c9a57d3a13dd5a12d9bbf
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,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);
+
+/* A macro to convert from cache attributes to actual cacheability */
+#define PAT_ENTRY(v) (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) do {
\
+BUILD_BUG_ON((v) < 0 || (v) > 7);
\
+CHECK_PAT_ENTRY_VALUE(PAT_ENTRY(v));
\
+} while (0);
+
+/*
+ * 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
*/
+#define PAGE_FLAGS_TO_CACHEATTR(page_value)
\
+page_value) >> 5) & 4) | (((page_value) >> 3) & 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(PAGE_FLAGS_TO_CACHEATTR(_PAGE_##page_value)) !=
\
+ (X86_MT_##page_value));
\
+} 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.
+ */
+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
