Re: [Xen-devel] [PATCH 2/4] VMX/vPMU: fix DebugCtl MSR handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>

--- 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

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