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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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).

> 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<---

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.

> 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?

~Andrew