Re: [PATCH v3 01/12] x86/mm: Avoid hard-coding PAT in get_page_from_l1e()

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Dec 15, 2022 at 09:46:41AM +0100, Jan Beulich wrote:
> On 15.12.2022 00:11, Demi Marie Obenour wrote:
> > get_page_from_l1e() relied on Xen's choice of PAT, which is brittle in
> > the face of future PAT changes.  Use the proper _PAGE_* constants
> > instead.  Also, treat the two unused cases as if they are cacheable, as
> > future changes may make them cacheable.
> 
> This still does not cover ...
> 
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -959,14 +959,19 @@ get_page_from_l1e(
> >              flip = _PAGE_RW;
> >          }
> >  
> > +        /* Force cacheable memtypes to UC */
> >          switch ( l1f & PAGE_CACHE_ATTRS )
> >          {
> > -        case 0: /* WB */
> > -            flip |= _PAGE_PWT | _PAGE_PCD;
> > +        case _PAGE_UC:
> > +        case _PAGE_UCM:
> > +        case _PAGE_WC:
> > +            /* not cached */
> >              break;
> > -        case _PAGE_PWT: /* WT */
> > -        case _PAGE_PWT | _PAGE_PAT: /* WP */
> > -            flip |= _PAGE_PCD | (l1f & _PAGE_PAT);
> > +        case _PAGE_WB:
> > +        case _PAGE_WT:
> > +        case _PAGE_WP:
> > +        default:
> > +            flip |= (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC;
> >              break;
> 
> ... the three cases here assuming certain properties wrt the flipping of
> _PAGE_UC. As said before - going from one kind of assumption to another
> _may_ be a good thing to do, but needs justifying as actually being an
> improvement. Alternatively such assumptions could be checked by suitable
> BUILD_BUG_ON(), which then at the same serve as documentation thereof.
> 
> Jan

I think I understand your point now: this still assumes that the two
unused types are not UCM or WC, but this is not documented anywhere.  I
will move this to after the patch that introduces the X86_MT_* flags,
which will allow me to switch on (XEN_MSR_PAT >> pte_flags_to_cacheattr(l1f))
instead without having to change the code again in a subsequent patch.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature