On Tue, Mar 09, 2021 at 07:13:26PM +0000, Andrew Cooper wrote:
> On 09/03/2021 10:56, Roger Pau Monne wrote:
> > Introduce an option to allow selecting a behavior similar to the pre
> > Xen 4.15 one for accesses to MSRs not explicitly handled. Since commit
> > 84e848fd7a162f669 and 322ec7c89f6640e accesses to MSRs not explicitly
> > handled by Xen result in the injection of a #GP to the guest. This
> > is a behavior change since previously a #GP was only injected if
> > accessing the MSR on the real hardware would also trigger a #GP, or if
> > the attempted to be set bits wouldn't match the hardware values (for
> > PV).
> >
> > This seems to be problematic for some guests, so introduce an option
> > to fallback to this kind of legacy behavior without leaking the
> > underlying MSR values to the guest.
> >
> > When the option is set, for both PV and HVM don't inject a #GP to the
> > guest on MSR read if reading the underlying MSR doesn't result in a
> > #GP, do the same for writes and simply discard the value to be written
> > on that case.
> >
> > Note that for guests restored or migrated from previous Xen versions
> > the option is enabled by default, in order to keep a compatible
> > MSR behavior. Such compatibility is done at the libxl layer, to avoid
> > higher-level toolstacks from having to know the details about this flag.
> >
> > Signed-off-by: Roger Pau Monné <roger....@citrix.com>
>
> Thankyou for doing this. By and large, Reviewed-by: Andrew Cooper
> <andrew.coop...@citrix.com>, subject to some pandoc syntax fixes below.
Thanks.
> However, I think it is worth saying explicitly that the reasons behind
> the changes in 84e848fd7a162f669 and 322ec7c89f6640e (not leaking
> hardware MSRs, and not breaking #GP-probing) are still legitimate, and
> this influences the change in behaviour between msr_relaxed and 4.14
> (i.e. read-as-zero rather than leaking).
Right, I tried to convey this fact by using "compatible" behavior
instead of "legacy" one, as the behavior provided by msr_relaxed is
not exactly the same as what you would get on 4.14.
I've added the following at the end of the first paragraph:
"The reasons for not leaking hardware MSR values and injecting a
#GP are fully valid, so the solution proposed here should be
considered a temporary workaround until all the required MSRs are
properly handled."
> > diff --git a/docs/misc/xen-command-line.pandoc
> > b/docs/misc/xen-command-line.pandoc
> > index 4737c92bfe..6cf61a5c57 100644
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -740,7 +740,7 @@ Specify the bit width of the DMA heap.
> >
> > ### dom0
> > = List of [ pv | pvh, shadow=<bool>, verbose=<bool>,
> > - cpuid-faulting=<bool> ]
> > + cpuid-faulting=<bool>, msr-relaxed=<bool> ]
> >
> > Applicability: x86
> >
> > @@ -789,6 +789,21 @@ Controls for how dom0 is constructed on x86 systems.
> > restore the pre-4.13 behaviour. If specifying `no-cpuid-faulting`
> > fixes
> > an issue in dom0, please report a bug.
> >
> > +* msr-relaxed: Select whether to use a relaxed behavior for accesses to
> > MSRs
> > + not explicitly handled by Xen instead of injecting a #GP to dom0.
> > + Such access mode will force Xen to replicate the behavior from the
> > hardware
> > + it's currently running on in order to decide whether a #GP is injected
> > to
> > + dom0 for MSR reads. Note that dom0 is never allowed to read the value
> > of
> > + unhandled MSRs, this option only changes whether a #GP might be
> > injected
> > + or not. For writes a #GP won't be injected as long as reading the
> > + underlying MSR doesn't result in a #GP.
>
> I don't find this overly clear to follow, and it also misses stating the
> default explicitly. How about:
>
> ---8<---
> The `msr-relaxed` boolean is an interim option, and defaults to false.
>
> In Xen 4.15, the default behaviour for unhandled MSRs has been changed,
> to avoid leaking host data into guests, and to avoid breaking guest
> logic which uses \#GP probing to identify the availability of MSRs.
>
> However, this new stricter behaviour has the possibility to break
> guests, and a more 4.14-like behaviour can be selected by specifying
> `dom0=msr-relaxed`.
>
> If using this option is necessary to fix an issue, please report a bug.
> ---8<---
OK, this seems to be less verbose that what I previously had, but I'm
fine with it. I assume this should also be changed in xl.cfg then?
> Other pending Sphinx work is drawing together the "how to contact us"
> information, so the various "please report a bug"s through this document
> will turn into links.
>
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 23bbb6e8c1..d217c223b0 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -749,6 +749,7 @@ static struct domain *__init create_dom0(const module_t
> > *image,
> > .max_grant_frames = -1,
> > .max_maptrack_frames = -1,
> > .max_vcpus = dom0_max_vcpus(),
> > + .arch.domain_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED :
> > 0,
>
> Can I request
>
> .arch = {
> .domain_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
> },
>
> please, to reduce the patch complexity of further additions inside .arch.
Sure.
> > diff --git a/xen/include/public/arch-x86/xen.h
> > b/xen/include/public/arch-x86/xen.h
> > index 629cb2ba40..f9d0e33b94 100644
> > --- a/xen/include/public/arch-x86/xen.h
> > +++ b/xen/include/public/arch-x86/xen.h
> > @@ -304,6 +304,14 @@ struct xen_arch_domainconfig {
> > XEN_X86_EMU_PIT |
> > XEN_X86_EMU_USE_PIRQ |\
> > XEN_X86_EMU_VPCI)
> > uint32_t emulation_flags;
> > +
> > +/*
> > + * Select whether to use a relaxed behavior for accesses to MSRs not
> > explicitly
> > + * handled by Xen instead of injecting a #GP to the guest. Note this option
> > + * doesn't allow the guest to read or write to the underlying MSR.
> > + */
> > +#define XEN_X86_MSR_RELAXED (1u << 0)
> > + uint32_t domain_flags;
>
> The domain prefix is somewhat redundant, given the name of the structure
> or the hypercall it is used for. OTOH, 'flags' on its own probably
> isn't ok. Thoughts on misc_flags?
I'm fine with it, will change unless Jan objects to the name.
Thanks, Roger.
