Re: [PATCH v5 09/10] x86/mm: Reject invalid cacheability in PV guests by default
On 22.12.2022 10:55, Demi Marie Obenour wrote: > On Thu, Dec 22, 2022 at 10:48:02AM +0100, Jan Beulich wrote: >> 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. That's likely a mistake then, at least for work on PV guest OSes. In particular PV memory management code is sprinkled with gdprintk(), in an attempt to help those people understand why some operation has failed. Jan
Re: [PATCH v5 09/10] x86/mm: Reject invalid cacheability in PV guests by default
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
signature.asc
Description: PGP signature
Re: [PATCH v5 09/10] x86/mm: Reject invalid cacheability in PV guests by default
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
