Re: [PATCH v5 09/10] x86/mm: Reject invalid cacheability in PV guests by default

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Dec 22, 2022 at 10:48:02AM +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
> > @@ -145,6 +145,8 @@
> >  
> >  #ifdef CONFIG_PV
> >  #include "pv/mm.h"
> > +bool allow_invalid_cacheability;
> 
> static and __ro_after_init please (the former not the least with Misra
> in mind).

Will fix in v6.

> > +boolean_param("allow_invalid_cacheability", allow_invalid_cacheability);
> >  #endif
> 
> Any new command line option will need to come with an entry in
> docs/misc/xen-command-line.pandoc. Furthermore we're trying to avoid
> underscores in command line option names, when dashes serve the
> purpose quite fine.

Will fix in v6.

> Also please add a blank line at least between #include and your
> addition. Personally I would find it more natural if such a single-use
> control was defined next to the place it is used, i.e. 

Will fix in v6.

> > @@ -1343,7 +1345,33 @@ static int promote_l1_table(struct page_info *page)
> >          }
> >          else
> >          {
> 
> ... here.

Will move in v6.

> > -            switch ( ret = get_page_from_l1e(pl1e[i], d, d) )
> > +            l1_pgentry_t l1e = pl1e[i];
> > +
> > +            if ( !allow_invalid_cacheability )
> > +            {
> > +                switch ( l1e.l1 & PAGE_CACHE_ATTRS )
> > +                {
> > +                case _PAGE_WB:
> > +                case _PAGE_UC:
> > +                case _PAGE_UCM:
> > +                case _PAGE_WC:
> > +                case _PAGE_WT:
> > +                case _PAGE_WP:
> > +                    break;
> > +                default:
> 
> Nit (style): Blank line please between non-fall-through case blocks.

Will fix in v6.

> > +                    /*
> > +                     * If we get here, a PV guest tried to use one of the
> > +                     * reserved values in Xen's PAT.  This indicates a bug 
> > in
> > +                     * the guest, so inject #GP to cause the guest to log a
> > +                     * stack trace.
> > +                     */
> 
> And likely make it die. Which, yes, ...
> 
> > +                    pv_inject_hw_exception(TRAP_gp_fault, 0);
> > +                    ret = -EINVAL;
> 
> ... also may or may not be the result of returning failure here (if the
> guest "checks" for errors by using a BUG()-like construct), but that's
> then more of its own fault than not coping with a non-architectural #GP.

Is there really any architectural behavior here?

> Also wasn't there talk of having this behavior in debug hypervisors
> only? Without that restriction I'm yet less happy with the change ...

My understanding was that Andrew preferred the behavior to be turned on
in release hypervisors too.  I am inclined to agree with Andrew, if for
no other reason than that those working on guest OSs do not generally
use debug hypervisors if they are not also working on Xen itself.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature