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

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

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?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature