[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 9 Mar 2021 11:17:31 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=ysQDz0JtYjuWpyf2eObItyAVOvfmBWKEK5gpJuJOCkc=; b=Sk0+xOFPvlWkw+O1rGMk1lUwkhhVAm0u6jXH4ifUuVM1owcgK2ri9MEJvpQ75eY3CllOkOPAFWXCECgRlJKbf9/fl/MIvl/GCd/y3ZhS1a73M5q/PQm6HRysOYEane/k9eM2JJWYNN6p2F5LtWc7yM87zXuJbaowWV/cRH6heMS9VODTORcj7mUD1NMcTQqwMQ9SvIPU5lo3l6Q+24Rzhb3QBUJwOrrMREGUeh+fm0bA6YkeQbPTRjDeY0b1+t4+oHRFLRmfBkGakd9Ro7nJPTxSgLURMbm4YGL51oXyuIHT2IZKEvSPbKiPtCBSeHtIF3kBvopO81lE6uo3KsFpSg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Kq5mwoTcKbyz4AGnYFJCoj28HUeksPtPHkpWUnoHTvSEDcPZwwbnuAqb3dv9U4N5o8DUw+F0/QIFN5TAOXnbsZOgLLpc+1RI+t4R/+MroTuM4w/dygPYKHJ3TRt3SXdQYIYJPvfos0kcMDDfVm/xEVuS+nUu1rR4BDiLn7hwtQ21OltYgqenl0E5al5EjJ+ITNW1puvgjZM/W5JC697REBkiP1iFyOEHivH3Oua4cOAekcft4sKeyBK22ydk8DHhb74AJsTcoQbVBWd5eKZ+EtmbrFdcWgaN5k4zHF+FXHk6Avab8Nldbq5CTvPStsUeaH4em5HHLb8dYLZHlEMbfw==
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>
  • Delivery-date: Tue, 09 Mar 2021 10:18:19 +0000
  • Ironport-sdr: LlU6w+mFcUlqOArpTWbH+wwiBhr0UBqk+EIpJxvn7HoSOb+NJQisxKei1wspMsy2BIC6p9k5uU bLFIZXPgGdJ3rMFLdibUs3iIeZQ8Vdh06qfOPQ4AUs3gU2SBg5nlrPH7/BV6arckcLNV3sO1Ew AirtPfebFTgv1kxPcfmoKUDkEAV7uGMDm6ZwlwvT6iUN9/X4GM22w/9kjZgEK3f+bC3+0Ec9Kx nFeP4o0LSePkntgQE9XOoxLJSRZvRO5v/Gmi6HqpCiwte8gEA5L3KMmiBqfhJNGkr6i4hHF6yV tPc=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.