From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 2/4] VMX/vPMU: fix DebugCtl MSR handling Date: Tue, 12 Aug 2014 10:44:06 +0100 Message-ID: <53E9E1E6.8090402@citrix.com> References: <53E9F5A0020000780002B70C@mail.emea.novell.com> <53E9F73E020000780002B726@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============9049329383782249903==" Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XH8iL-0001Ij-Kl for xen-devel@lists.xenproject.org; Tue, 12 Aug 2014 09:49:57 +0000 In-Reply-To: <53E9F73E020000780002B726@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , xen-devel Cc: Kevin Tian , Eddie Dong , Jun Nakajima List-Id: xen-devel@lists.xenproject.org --===============9049329383782249903== Content-Type: multipart/alternative; boundary="------------060505040105060106060902" --------------060505040105060106060902 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 12/08/14 10:15, Jan Beulich wrote: > - writes with one of the vPMU-supported flags and some unsupported one > set got accepted, leading to a VM entry failure > - writes with one of the vPMU-supported flags set but the Debug Store > feature unavailable produced a #GP (other than other writes to this > MSR with unsupported bits set) > > Signed-off-by: Jan Beulich > > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -1789,7 +1789,7 @@ static int svm_msr_write_intercept(unsig > case MSR_AMD_FAM15H_EVNTSEL3: > case MSR_AMD_FAM15H_EVNTSEL4: > case MSR_AMD_FAM15H_EVNTSEL5: > - vpmu_do_wrmsr(msr, msr_content); > + vpmu_do_wrmsr(msr, msr_content, 0); > break; > > case MSR_IA32_MCx_MISC(4): /* Threshold register */ > --- a/xen/arch/x86/hvm/svm/vpmu.c > +++ b/xen/arch/x86/hvm/svm/vpmu.c > @@ -278,11 +278,14 @@ static void context_update(unsigned int > } > } > > -static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content) > +static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, > + uint64_t supported) > { > struct vcpu *v = current; > struct vpmu_struct *vpmu = vcpu_vpmu(v); > > + ASSERT(!supported); > + > /* For all counters, enable guest only mode for HVM guest */ > if ( (get_pmu_reg_type(msr) == MSR_TYPE_CTRL) && > !(is_guest_mode(msr_content)) ) > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -2249,7 +2249,7 @@ static int vmx_msr_write_intercept(unsig > if ( msr_content & ~supported ) > { > /* Perhaps some other bits are supported in vpmu. */ > - if ( !vpmu_do_wrmsr(msr, msr_content) ) > + if ( !vpmu_do_wrmsr(msr, msr_content, supported) ) > break; > } > if ( msr_content & IA32_DEBUGCTLMSR_LBR ) > @@ -2278,7 +2278,7 @@ static int vmx_msr_write_intercept(unsig > goto gp_fault; > break; > default: > - if ( vpmu_do_wrmsr(msr, msr_content) ) > + if ( vpmu_do_wrmsr(msr, msr_content, 0) ) > return X86EMUL_OKAY; > if ( passive_domain_do_wrmsr(msr, msr_content) ) > return X86EMUL_OKAY; > --- a/xen/arch/x86/hvm/vmx/vpmu_core2.c > +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c > @@ -454,7 +454,8 @@ static int core2_vpmu_msr_common_check(u > return 1; > } > > -static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content) > +static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, > + uint64_t supported) > { > u64 global_ctrl, non_global_ctrl; > char pmu_enable = 0; > @@ -469,24 +470,26 @@ static int core2_vpmu_do_wrmsr(unsigned > /* Special handling for BTS */ > if ( msr == MSR_IA32_DEBUGCTLMSR ) > { > - uint64_t supported = IA32_DEBUGCTLMSR_TR | IA32_DEBUGCTLMSR_BTS | > - IA32_DEBUGCTLMSR_BTINT; > + supported |= IA32_DEBUGCTLMSR_TR | IA32_DEBUGCTLMSR_BTS | > + IA32_DEBUGCTLMSR_BTINT; > > if ( cpu_has(¤t_cpu_data, X86_FEATURE_DSCPL) ) > supported |= IA32_DEBUGCTLMSR_BTS_OFF_OS | > IA32_DEBUGCTLMSR_BTS_OFF_USR; > - if ( msr_content & supported ) > - { > - if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) ) > - return 1; > - gdprintk(XENLOG_WARNING, "Debug Store is not supported on this cpu\n"); > - hvm_inject_hw_exception(TRAP_gp_fault, 0); > - return 0; > - } > + if ( !(msr_content & ~supported) && > + vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) ) > + return 1; > + if ( (msr_content & supported) && > + !vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) ) > + printk(XENLOG_G_WARNING > + "%pv: Debug Store unsupported on this CPU\n", > + current); > } > return 0; > } > > + ASSERT(!supported); > + > core2_vpmu_cxt = vpmu->context; > switch ( msr ) > { > --- a/xen/arch/x86/hvm/vpmu.c > +++ b/xen/arch/x86/hvm/vpmu.c > @@ -64,12 +64,12 @@ static void __init parse_vpmu_param(char > } > } > > -int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content) > +int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, uint64_t supported) > { > struct vpmu_struct *vpmu = vcpu_vpmu(current); > > if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr ) > - return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content); > + return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content, supported); > return 0; > } > > --- a/xen/include/asm-x86/hvm/vpmu.h > +++ b/xen/include/asm-x86/hvm/vpmu.h > @@ -45,7 +45,8 @@ > > /* Arch specific operations shared by all vpmus */ > struct arch_vpmu_ops { > - int (*do_wrmsr)(unsigned int msr, uint64_t msr_content); > + int (*do_wrmsr)(unsigned int msr, uint64_t msr_content, > + uint64_t supported); > int (*do_rdmsr)(unsigned int msr, uint64_t *msr_content); > int (*do_interrupt)(struct cpu_user_regs *regs); > void (*do_cpuid)(unsigned int input, > @@ -86,7 +87,7 @@ struct vpmu_struct { > #define vpmu_is_set(_vpmu, _x) ((_vpmu)->flags & (_x)) > #define vpmu_clear(_vpmu) ((_vpmu)->flags = 0) > > -int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content); > +int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, uint64_t supported); It might, for clarity sake, be preferable to name the new parameter "supported_bits". At the moment, "supported" could equally well refer to the MSR itself. ~Andrew > int vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content); > int vpmu_do_interrupt(struct cpu_user_regs *regs); > void vpmu_do_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------060505040105060106060902 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 12/08/14 10:15, Jan Beulich wrote:
- writes with one of the vPMU-supported flags and some unsupported one
  set got accepted, leading to a VM entry failure
- writes with one of the vPMU-supported flags set but the Debug Store
  feature unavailable produced a #GP (other than other writes to this
  MSR with unsupported bits set)

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1789,7 +1789,7 @@ static int svm_msr_write_intercept(unsig
     case MSR_AMD_FAM15H_EVNTSEL3:
     case MSR_AMD_FAM15H_EVNTSEL4:
     case MSR_AMD_FAM15H_EVNTSEL5:
-        vpmu_do_wrmsr(msr, msr_content);
+        vpmu_do_wrmsr(msr, msr_content, 0);
         break;
 
     case MSR_IA32_MCx_MISC(4): /* Threshold register */
--- a/xen/arch/x86/hvm/svm/vpmu.c
+++ b/xen/arch/x86/hvm/svm/vpmu.c
@@ -278,11 +278,14 @@ static void context_update(unsigned int 
     }
 }
 
-static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
+static int amd_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
+                             uint64_t supported)
 {
     struct vcpu *v = current;
     struct vpmu_struct *vpmu = vcpu_vpmu(v);
 
+    ASSERT(!supported);
+
     /* For all counters, enable guest only mode for HVM guest */
     if ( (get_pmu_reg_type(msr) == MSR_TYPE_CTRL) &&
         !(is_guest_mode(msr_content)) )
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2249,7 +2249,7 @@ static int vmx_msr_write_intercept(unsig
         if ( msr_content & ~supported )
         {
             /* Perhaps some other bits are supported in vpmu. */
-            if ( !vpmu_do_wrmsr(msr, msr_content) )
+            if ( !vpmu_do_wrmsr(msr, msr_content, supported) )
                 break;
         }
         if ( msr_content & IA32_DEBUGCTLMSR_LBR )
@@ -2278,7 +2278,7 @@ static int vmx_msr_write_intercept(unsig
             goto gp_fault;
         break;
     default:
-        if ( vpmu_do_wrmsr(msr, msr_content) )
+        if ( vpmu_do_wrmsr(msr, msr_content, 0) )
             return X86EMUL_OKAY;
         if ( passive_domain_do_wrmsr(msr, msr_content) )
             return X86EMUL_OKAY;
--- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
+++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
@@ -454,7 +454,8 @@ static int core2_vpmu_msr_common_check(u
     return 1;
 }
 
-static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
+static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
+                               uint64_t supported)
 {
     u64 global_ctrl, non_global_ctrl;
     char pmu_enable = 0;
@@ -469,24 +470,26 @@ static int core2_vpmu_do_wrmsr(unsigned 
         /* Special handling for BTS */
         if ( msr == MSR_IA32_DEBUGCTLMSR )
         {
-            uint64_t supported = IA32_DEBUGCTLMSR_TR | IA32_DEBUGCTLMSR_BTS |
-                                 IA32_DEBUGCTLMSR_BTINT;
+            supported |= IA32_DEBUGCTLMSR_TR | IA32_DEBUGCTLMSR_BTS |
+                         IA32_DEBUGCTLMSR_BTINT;
 
             if ( cpu_has(&current_cpu_data, X86_FEATURE_DSCPL) )
                 supported |= IA32_DEBUGCTLMSR_BTS_OFF_OS |
                              IA32_DEBUGCTLMSR_BTS_OFF_USR;
-            if ( msr_content & supported )
-            {
-                if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
-                    return 1;
-                gdprintk(XENLOG_WARNING, "Debug Store is not supported on this cpu\n");
-                hvm_inject_hw_exception(TRAP_gp_fault, 0);
-                return 0;
-            }
+            if ( !(msr_content & ~supported) &&
+                 vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
+                return 1;
+            if ( (msr_content & supported) &&
+                 !vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
+                printk(XENLOG_G_WARNING
+                       "%pv: Debug Store unsupported on this CPU\n",
+                       current);
         }
         return 0;
     }
 
+    ASSERT(!supported);
+
     core2_vpmu_cxt = vpmu->context;
     switch ( msr )
     {
--- a/xen/arch/x86/hvm/vpmu.c
+++ b/xen/arch/x86/hvm/vpmu.c
@@ -64,12 +64,12 @@ static void __init parse_vpmu_param(char
     }
 }
 
-int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
+int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, uint64_t supported)
 {
     struct vpmu_struct *vpmu = vcpu_vpmu(current);
 
     if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr )
-        return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content);
+        return vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content, supported);
     return 0;
 }
 
--- a/xen/include/asm-x86/hvm/vpmu.h
+++ b/xen/include/asm-x86/hvm/vpmu.h
@@ -45,7 +45,8 @@
 
 /* Arch specific operations shared by all vpmus */
 struct arch_vpmu_ops {
-    int (*do_wrmsr)(unsigned int msr, uint64_t msr_content);
+    int (*do_wrmsr)(unsigned int msr, uint64_t msr_content,
+                    uint64_t supported);
     int (*do_rdmsr)(unsigned int msr, uint64_t *msr_content);
     int (*do_interrupt)(struct cpu_user_regs *regs);
     void (*do_cpuid)(unsigned int input,
@@ -86,7 +87,7 @@ struct vpmu_struct {
 #define vpmu_is_set(_vpmu, _x) ((_vpmu)->flags & (_x))
 #define vpmu_clear(_vpmu)      ((_vpmu)->flags = 0)
 
-int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content);
+int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, uint64_t supported);

It might, for clarity sake, be preferable to name the new parameter "supported_bits".  At the moment, "supported" could equally well refer to the MSR itself.

~Andrew

 int vpmu_do_rdmsr(unsigned int msr, uint64_t *msr_content);
 int vpmu_do_interrupt(struct cpu_user_regs *regs);
 void vpmu_do_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

--------------060505040105060106060902-- --===============9049329383782249903== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============9049329383782249903==--