Re: [PATCH v5 08/10] x86/mm: make code robust to future PAT changes

[Demi Marie Obenour <demi@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 2690 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]