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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.12.2022 05: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

I realize we already have a case of injecting of #GP from a hypercall
handling path, but I'm afraid this isn't really sane. _If_ #GP was
possible at all for any hypercall, it should imo be raised on the
syscall instruction (or the int $0x82 for compat guests), not on the
instruction following it. Plus, as a major difference, in the
existing case there's no other means to return an error indication.

> entries, along with returning -EINVAL from the hypercall.  Future
> versions of Xen will refuse to use these entries even in release builds.

Mind me asking where you're taking "will" from? (I might view "may" as
appropriate here.)

> @@ -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);
> +            pv_inject_hw_exception(TRAP_gp_fault, 0);
> +            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;
>      }

May I ask that you leave the basic structure here as is, merely adding
the default block you care about and the three case labels which you
need to add to compensate? Also please use %pd in new code logging
domain IDs. I'm also not convinced l1e_owner is the most appropriate
domain to actually blame here, at least with the wording you've chosen
to use.

Jan