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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.

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?

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.

Thanks, Roger.