Re: [PATCH 7/8] x86/mm: make code robust to future PAT changes

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

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

On Tue, Dec 06, 2022 at 12:06:24PM +0000, Andrew Cooper wrote:
> On 06/12/2022 04:33, 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.
> >
> > Additionally, Xen has two unused entries in the PAT.  Currently these
> > are UC, but this will change if the hardware ever supports additional
> > memory types.  To avoid future problems, this adds a check in debug
> > builds that injects #GP into a guest that tries to use one of these
> > entries, along with returning -EINVAL from the hypercall.  Future
> > versions of Xen will refuse to use these entries even in release builds.
> >
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > ---
> >  xen/arch/x86/mm.c | 58 +++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 54 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 5d05399c3a841bf03991a3bed63df9a815c1e891..517fccee699b2a673ba537e47933aefc80017aa5 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -849,6 +849,45 @@ static int cf_check print_mmio_emul_range(
> >  }
> >  #endif
> >  
> > +static void __init __maybe_unused build_assertions(void)
> 
> This wants to be at the very bottom of the file.  (And also in the
> previous patch to remove the _Static_assert())
> 
> > +{
> > +    /* A bunch of static assertions to check that the XEN_MSR_PAT is valid
> > +     * and consistent with the _PAGE_* macros */
> > +#define PAT_VALUE(v) (0xFF & (XEN_MSR_PAT >> (8 * (v))))
> > +#define BAD_VALUE(v) ((v) < 0 || (v) > 7 ||                                    \
> > +                      (v) == MSR_PAT_RESERVED_1 || (v) == MSR_PAT_RESERVED_2)
> > +#define BAD_PAT_VALUE(v) BUILD_BUG_ON(BAD_VALUE(PAT_VALUE(v)))
> > +    BAD_PAT_VALUE(0);
> > +    BAD_PAT_VALUE(1);
> > +    BAD_PAT_VALUE(2);
> > +    BAD_PAT_VALUE(3);
> > +    BAD_PAT_VALUE(4);
> > +    BAD_PAT_VALUE(5);
> > +    BAD_PAT_VALUE(6);
> > +    BAD_PAT_VALUE(7);
> > +#undef BAD_PAT_VALUE
> > +#undef BAD_VALUE
> 
> Given that you've reworked the PAT declaration to be of the form (MT <<
> shift), I'm not sure how much value this check is.

One of my goals with this patch set is that it should be possible to
choose *any* value for XEN_MSR_PAT and for the PAT-related _PAGE_*, with
all bad values caught at compile-time.  This would allow making it a
Kconfig option.

> > +#define PAT_SHIFT(page_value) (((page_value) & _PAGE_PAT) >> 5 |               \
> > +                               ((page_value) & (_PAGE_PCD | _PAGE_PWT)) >> 3)
> 
> pte_flags_to_cacheattr()

That’s a function, not a macro (and for good reason), so it can’t be
used in BUILD_BUG_ON().

> > +#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_VALUE(PAT_SHIFT(_PAGE_##page_value)) !=                   \
> > +                 (MSR_PAT_##page_value));                                      \
> > +} while (0)
> > +    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 PAT_SHIFT
> > +#undef PAT_VALUE
> > +}
> > +
> >  /*
> >   * get_page_from_l1e returns:
> >   *   0  => success (page not present also counts as such)
> > @@ -961,13 +1000,24 @@ get_page_from_l1e(
> >  
> >          switch ( l1f & PAGE_CACHE_ATTRS )
> >          {
> > -        case _PAGE_WB:
> > +        default:
> > +#ifndef NDEBUG
> > +            printk(XENLOG_G_WARNING
> > +                   "d%d: Guest tried to use bad cachability attribute %u for MFN %lx\n",
> > +                   l1e_owner->domain_id, l1f & PAGE_CACHE_ATTRS, mfn);
> 
> %pd.  You absolutely want to convert the PTE bits to a PAT value before
> priniting (decimal on a PTE value is useless), and %PRI_mfn.

I’ll have to look at the rest of the Xen tree to see how to use this.

> > +            pv_inject_hw_exception(TRAP_gp_fault, 0);
> 
> As I said on IRC, we do want this, but I'm not certain if we can get
> away with just enabling it in debug builds.  _PAGE_GNTTAB was ok because
> it has always been like that, but there's a non-trivial chance that
> there are existing dom0 kernels which violate this constraint.

I chose this approach because it is very simple to implement.  Anything
more complex ought to be in a separate patch, IMO.

> > +            return -EINVAL;
> > +#endif
> >          case _PAGE_WT:
> >          case _PAGE_WP:
> > -            flip |= (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC;
> > +        case _PAGE_WB:
> > +            /* Force this to be uncachable */
> > +            return flip | ( (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC );
> > +        case _PAGE_WC:
> > +        case _PAGE_UC:
> > +        case _PAGE_UCM:
> > +            return flip;
> >          }
> > -
> > -        return flip;
> 
> This wants reworking over Jan's suggestion in patch 1, and modifying to
> reduce churn.  (Keep _PAGE_WB in the same order WRT _PAGE_WT, the
> uncached memory types should simply break, and default should be at the
> end.)

I put the default in the middle to keep the preprocessor conditionals
simple and avoid duplication.  I will have the default be treated as
cachable memory.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

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