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
