Re: [PATCH v2 1/2][4.15] x86/PV: conditionally avoid raising #GP for early guest MSR reads

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

On Tue, Mar 09, 2021 at 03:50:59PM +0100, Jan Beulich wrote:
> On 09.03.2021 14:37, Roger Pau Monné wrote:
> > On Tue, Mar 09, 2021 at 12:16:49PM +0100, Jan Beulich wrote:
> >> On 09.03.2021 11:17, Roger Pau Monné wrote:
> >>> On Mon, Mar 08, 2021 at 02:49:19PM +0100, Jan Beulich wrote:
> >>>> On 08.03.2021 13:11, Roger Pau Monné wrote:
> >>>>> On Mon, Mar 08, 2021 at 10:33:12AM +0100, Jan Beulich wrote:
> >>>>>> On 08.03.2021 09:56, Roger Pau Monné wrote:
> >>>>>>> On Fri, Mar 05, 2021 at 10:50:34AM +0100, Jan Beulich wrote:
> >>>>>>>> --- a/xen/arch/x86/pv/emul-priv-op.c
> >>>>>>>> +++ b/xen/arch/x86/pv/emul-priv-op.c
> >>>>>>>> @@ -874,7 +874,7 @@ static int read_msr(unsigned int reg, ui
> >>>>>>>>      struct vcpu *curr = current;
> >>>>>>>>      const struct domain *currd = curr->domain;
> >>>>>>>>      const struct cpuid_policy *cp = currd->arch.cpuid;
> >>>>>>>> -    bool vpmu_msr = false;
> >>>>>>>> +    bool vpmu_msr = false, warn = false;
> >>>>>>>>      int ret;
> >>>>>>>>  
> >>>>>>>>      if ( (ret = guest_rdmsr(curr, reg, val)) != 
> >>>>>>>> X86EMUL_UNHANDLEABLE )
> >>>>>>>> @@ -882,7 +882,7 @@ static int read_msr(unsigned int reg, ui
> >>>>>>>>          if ( ret == X86EMUL_EXCEPTION )
> >>>>>>>>              x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
> >>>>>>>>  
> >>>>>>>> -        return ret;
> >>>>>>>> +        goto done;
> >>>>>>>>      }
> >>>>>>>>  
> >>>>>>>>      switch ( reg )
> >>>>>>>> @@ -986,7 +986,7 @@ static int read_msr(unsigned int reg, ui
> >>>>>>>>          }
> >>>>>>>>          /* fall through */
> >>>>>>>>      default:
> >>>>>>>> -        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", 
> >>>>>>>> reg);
> >>>>>>>> +        warn = true;
> >>>>>>>>          break;
> >>>>>>>>  
> >>>>>>>>      normal:
> >>>>>>>> @@ -995,7 +995,19 @@ static int read_msr(unsigned int reg, ui
> >>>>>>>>          return X86EMUL_OKAY;
> >>>>>>>>      }
> >>>>>>>>  
> >>>>>>>> -    return X86EMUL_UNHANDLEABLE;
> >>>>>>>> + done:
> >>>>>>>
> >>>>>>> Won't this handling be better placed in the 'default' switch case
> >>>>>>> above?
> >>>>>>
> >>>>>> No - see the "goto done" added near the top of the function.
> >>>>>
> >>>>> Yes, I'm not sure of that. If guest_rdmsr returns anything different
> >>>>> than X86EMUL_UNHANDLEABLE it means it has handled the MSR in some way,
> >>>>> and hence we shouldn't check whether the #GP handler is set or not.
> >>>>>
> >>>>> This is not the behavior of older Xen versions, so I'm unsure whether
> >>>>> we should introduce a policy that's even less strict than the previous
> >>>>> one in regard to whether a #GP is injected or not.
> >>>>>
> >>>>> I know injecting a #GP when the handler is not set is not helpful for
> >>>>> the guest, but we should limit the workaround to kind of restoring the
> >>>>> previous behavior for making buggy guests happy, not expanding it
> >>>>> anymore.
> >>>>
> >>>> Yet then we risk breaking guests with any subsequent addition to
> >>>> guest_rdmsr() which, under whatever extra conditions, wants to
> >>>> raise #GP.
> >>>
> >>> But it's always been like that AFAICT? Additions to guest_{rd/wr}msr
> >>> preventing taking the default path in the {read/write}_msr PV
> >>> handlers.
> >>
> >> Yes, but the impact so far and the impact going forward are different.
> > 
> > OK, I assume this is because we plan to handle more MSRs in
> > guest_{rd/wr}msr?
> 
> Yes.
> 
> > In which case those newly added handlers are not likely to inject a
> > #GP?
> 
> Kind of the opposite - because it's not impossible that some
> addition there may want to raise #GP.
> 
> >>> If #GP signaled by guest_{rd/wr}msr are no longer injected when the guest
> >>> #GP handler is not set we might as well drop the rdmsr_safe check and
> >>> just don't try to inject any #GP at all from MSR accesses unless the
> >>> handler is setup?
> >>
> >> Well, that's what I had initially. You asked me to change to what I
> >> have now.
> >>
> >>> I feel this is likely going too far. I agree we should attempt to
> >>> restore something compatible with the previous behavior for unhandled
> >>> MSRs, but also not injecting the #GPs signaled by guest_{rd/wr}msr
> >>> seems to go beyond that.
> >>
> >> I understand this is a downside. Yet as said - the downside of _not_
> >> doing so is that every further raising of #GP will risk breaking a
> >> random guest kernel variant.
> > 
> > Right. So given this awkward position Xen is in, we should maybe make
> > the lack of #GP injection as a result of an MSR access when no handler
> > is set formally part of the ABI and written down somewhere?
> > 
> > It's not ideal, but at the end of day PV is 'our' own architecture,
> > and given that this workaround will be enabled by default, and that we
> > won't be able to turn it off we should have it written down as part of
> > the ABI.
> > 
> > If you agree with this I'm fine with not injecting a #GP at all unless
> > the handler is set for PV, like you proposed in your first patch. IMO
> > it's not ideal, but it's better if it's a consistent behavior and
> > clearly written down in the public headers (likely next to the
> > hypercall used to setup the #GP handler).
> > 
> > I know this can be seen as broken behavior from an x86 perspective,
> > but again PV is already different from x86.
> 
> I'm certainly not opposed to spelling this out somewhere; iirc you
> said the other day that you couldn't spot a good place. I can't think
> of a good place either.

After looking some more, I think placing such comment next to
HYPERVISOR_set_trap_table (in arch-x86/xen.h) would be fine.

> Furthermore before we spell out anything we
> (which specifically includes Andrew) need to settle on the precise
> behavior we want. I did suggest earlier that I could see us tighten
> the condition, and there are many possible variations. For example we
> could record whether a #GP handler was ever installed, so we wouldn't
> return back to the relaxed behavior in case a guest zapped its handler
> again. But for behavior like this the immediate question is going to
> be what effect migration (or saving/restoring) of the guest ought to
> have.

Replying to the save/restore part: this is covered by my patch. Any
restore (or incoming live migration) from a source that doesn't have
msr_relaxed support will get that option enabled by default, so that
guests migrated from previous Xen versions don't see a change in MSR
access behavior. That applies to both PV and HVM guests (unless I have
messed things up in my patch).

Thanks, Roger.