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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

> +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.

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. 

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

... here.

> -            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.

> +                    /*
> +                     * 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.

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

Jan