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 12:36:39PM +0100, Jan Beulich wrote:
> On 09.03.2021 11: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>
> 
> I'm generally okay with this approach, but wouldn't want to give it
> my R-b until it's at least clear it's not entirely unacceptable to
> anyone else (Andrew in particular). Couple of remarks:

Thanks.

> > Changes since v2:
> >  - Apply the option to both HVM and PV guest.
> >  - Handle both reads and writes.
> 
> I take it that it's intentional that you didn't allow separate read
> and write control?

Yes, I don't have a strong opinion, but I think just having a single
option is better: guests requiring the read side bodge are also likely
to require the same adjustment on the write side. It's also better
from a user perspective, as it's likely people would enable them in
tandem anyway.

> 
> > --- 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 )
> >      {
> 
> While to some degree a matter of taste, I think such variables needed
> only inside a switch() and not needing an initializer would better
> live in the respective switch()'s scope.

I can indeed define them inside the switch statement.

> > --- a/xen/arch/x86/pv/emul-priv-op.c
> > +++ b/xen/arch/x86/pv/emul-priv-op.c
> > @@ -875,6 +875,7 @@ static int read_msr(unsigned int reg, uint64_t *val,
> >      const struct domain *currd = curr->domain;
> >      const struct cpuid_policy *cp = currd->arch.cpuid;
> >      bool vpmu_msr = false;
> > +    uint64_t tmp;
> >      int ret;
> >  
> >      if ( (ret = guest_rdmsr(curr, reg, val)) != X86EMUL_UNHANDLEABLE )
> > @@ -986,6 +987,12 @@ static int read_msr(unsigned int reg, uint64_t *val,
> >          }
> >          /* fall through */
> >      default:
> > +        if ( currd->arch.msr_relaxed && !rdmsr_safe(reg, tmp) )
> > +        {
> > +            *val = 0;
> > +            return X86EMUL_OKAY;
> > +        }
> > +
> >          gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", reg);
> >          break;
> >  
> > @@ -1148,6 +1155,9 @@ static int write_msr(unsigned int reg, uint64_t val,
> >          }
> >          /* fall through */
> >      default:
> > +        if ( currd->arch.msr_relaxed && !rdmsr_safe(reg, val) )
> > +            return X86EMUL_OKAY;
> > +
> >          gdprintk(XENLOG_WARNING,
> >                   "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
> >                   reg, val);
> 
> So what are your thoughts wrt my change to this file? Drop it
> altogether and require people to use this new option? Or do you
> see both coexist?

I wouldn't be opposed to have both changes co-exist, as long as the PV
one is made part of the PV ABI, that is have it properly described in
the public headers as part of the PV behavior. I've replied with some
details along those lines in your patch.

> In the latter case, since you had suggested
> that I drop the write side of my change - does your changing of
> the write path indicate you've changed your mind?

Yes, I think we need to provide an option to allow users to revert
back to an MSR behavior as close as possible to the previous one for
compatibility reasons, and that should include the write side even if
we don't know any users requiring it right now.

We would be in a bad position if that use-case gets discovered after
the release, so it's IMO best to provide an option that covers both
read and write side straight away.

Thanks, Roger.