Re: [PATCH v2 for-4.15] x86/msr: introduce an option for HVM relaxed rdmsr behavior

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

On Fri, Mar 05, 2021 at 12:06:19AM +0000, Andrew Cooper wrote:
> On 04/03/2021 14:47, Roger Pau Monne wrote:
> > Introduce an option to allow selecting a less strict behaviour for
> > rdmsr accesses targeting a MSR not explicitly handled by Xen. Since
> > commit 84e848fd7a162f669 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 will also trigger a #GP.
> >
> > This commit attempts to offer a fallback option similar to the
> > previous behavior. Note however that the value of the underlying MSR
> > is never leaked to the guest, as the newly introduced option only
> > changes whether a #GP is injected or not.
> >
> > Long term the plan is to properly handle all the MSRs, so the option
> > introduced here should be considered a temporary resort for OSes that
> > don't work properly with the new MSR policy. Any OS that requires this
> > option to be enabled should be reported to
> > xen-devel@xxxxxxxxxxxxxxxxxxxx.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Changes since v1:
> >  - Only apply the option to HVM guests.
> >  - Only apply the special handling to MSR reads.
> >  - Sanitize the newly introduced flags field.
> >  - Print a warning message when the option is used.
> > ---
> > Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> > ---
> > Boris, could you please test with Solaris to see if this fixes the
> > issue?
> >
> > I wonder whether we need to to enable this option by default for
> > guests being migrated from previous Xen versions? Maybe that's not
> > required as the option is helpful mostly for early boot I would
> > assume, afterwards an OS should already have the #GP handler setup
> > when accessing MSRs.
> 
> We know when building a domain whether it is a migrate or not, but don't
> recall any version information existing at an appropriate point in the
> migration stream to do this easily.
> 
> We can buffer the stream forward and peek at the libxc domain header,
> which does have the source hypervisor version, but that is going to be
> very invasive to implement.

I need to look at this more closely to have an opinion. Instead of
figuring out if the source version is older maybe we could add
something to the 4.15 stream in a suitable position to detect whether
the source is new enough?

> >
> > From a release PoV the biggest risk would be breaking some of the
> > existing MSR functionality. I think that's a necessary risk in order
> > to offer such fallback option, or else we might discover after the
> > release that guests that worked on Xen 4.14 don't work anymore in Xen
> > 4.15.
> 
> Much as I'd prefer not to have this, I agree with the sentiment that we
> should have an "emergency undo" which people can use, and carry it for
> at least a short while.
> 
> However, to be useful for the purpose of unbreaking VMs, it must change
> both the read and write behaviour, because both are potential
> compatibility concerns (without reintroducing the information leak).

I think I was confused here and assumed the previous behavior would
check the written value to match the current underlying value before
injecting a #GP. That's not the case.

I can expand this patch to include the write side, I just thought
having the rad side only would be enough to cover for the unhandled
MSRs accesses.

> 
> > ---
> >  docs/man/xl.cfg.5.pod.in          | 17 +++++++++++++++++
> >  tools/include/libxl.h             |  8 ++++++++
> >  tools/libs/light/libxl_types.idl  |  2 ++
> >  tools/libs/light/libxl_x86.c      |  4 ++++
> >  tools/xl/xl_parse.c               |  7 +++++++
> >  xen/arch/x86/domain.c             | 10 ++++++++++
> >  xen/arch/x86/hvm/svm/svm.c        |  6 ++++++
> >  xen/arch/x86/hvm/vmx/vmx.c        |  7 +++++++
> >  xen/include/asm-x86/hvm/domain.h  |  3 +++
> >  xen/include/public/arch-x86/xen.h |  8 ++++++++
> 
> This needs changes to the Ocaml bindings as well.  I guess I'll add that
> to the todo list.

Hm, would be better if someone else did those, as I don't know
anything about Ocaml. I can try however in the next version, maybe
it's trivial to add.

Thanks, Roger.