Re: [PATCH v3 for-4.15] x86/msr: introduce an option for compatible MSR behavior selection

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.pau@xxxxxxxxxx>
> 
> Thankyou for doing this.  By and large, Reviewed-by: Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>, 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.