| Message ID | a8920eef3254cbf470a0d35a887dbaf0e4907a6d.1671497984.git.demi@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make PAT handling less brittle | expand |
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
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)) != \ > > + (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. Will fix in v6. > > +} 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?
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
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.
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
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 } /*
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 <demi@invisiblethingslab.com> --- 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(+)