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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.03.2021 15: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.

While the title says this is limited to HVM guests, I have to admit
that I fail to see why this is, and hence I would have hoped for
some clarification in the description. In particular I don't think
my "guest in early boot" workaround, of which I posted v2 earlier
today, can be assumed to be enough in the longer run. Recall that
it relaxes behavior only when the guest didn't install a handler
for #GP yet - this means it wouldn't help with any unguarded RDMSR
the guest might issue later, with a handler already installed.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -760,6 +760,13 @@ int arch_domain_create(struct domain *d,
>                 d->domain_id);
>      }
>  
> +    if ( config->arch.domain_flags & ~XEN_X86_RDMSR_RELAXED )
> +    {
> +        printk(XENLOG_G_ERR "d%d: Invalid arch domain flags: %#x\n",
> +               d->domain_id, config->arch.domain_flags);
> +        return -EINVAL;
> +    }

This would look to better go into arch_sanitise_domain_config().
And if the flag remains HVM-only, that aspect should then also be
checked (i.e. the flag being set would then also need rejecting
for PV guests).

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1795,6 +1795,7 @@ static int svm_msr_read_intercept(unsigned int msr, 
> uint64_t *msr_content)
>      const struct domain *d = v->domain;
>      struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>      const struct nestedsvm *nsvm = &vcpu_nestedsvm(v);
> +    uint64_t tmp;
>  
>      switch ( msr )
>      {
> @@ -1965,6 +1966,11 @@ static int svm_msr_read_intercept(unsigned int msr, 
> uint64_t *msr_content)
>          break;
>  
>      default:
> +        if ( d->arch.hvm.rdmsr_relaxed && !rdmsr_safe(msr, tmp) )
> +        {
> +            *msr_content = 0;
> +            break;
> +        }

You don't really need "tmp" here, do you? You could as well read
into *msr_content, as you're zapping the value afterwards anyway.

> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -122,6 +122,9 @@ struct hvm_domain {
>  
>      bool_t                 is_s3_suspended;
>  
> +    /* Don't unconditionally inject #GP for unhandled MSRs reads. */
> +    bool rdmsr_relaxed;

If, again, this is to remain HVM-only, then you insertion wants
to honor the blank padding other field decls use. I'd also like
to ask for your insertion to be moved up a few lines, to after
"is_in_uc_mode". I have a patch queued already to also move
"is_s3_suspended" into that hole; it's from November last year,
so it looks like I simply forgot to post it.

Jan