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

In which case those newly added handlers are not likely to inject a
#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.

Thanks, Roger.