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

Re: [Xen-devel] [PATCH v9 13/20] x86/VPMU: When handling MSR accesses, leave fault injection to callers



On 08/12/2014 08:45 AM, Jan Beulich wrote:
On 08.08.14 at 18:55, <boris.ostrovsky@xxxxxxxxxx> wrote:
--- a/xen/arch/x86/hvm/vmx/vpmu_core2.c
+++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c
@@ -458,13 +458,13 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t 
msr_content)
              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 ( !(msr_content & supported ) ||
+                 !vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
              {
-                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;
+                gdprintk(XENLOG_WARNING,
+                         "Debug Store is not supported on this cpu\n");
+                return 1;
The code here (and in particular the #GP injection) was wrong anyway;
see the small series I sent earlier today. Please base your on top of that.

@@ -476,18 +476,17 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t 
msr_content)
      {
      case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
          core2_vpmu_cxt->global_status &= ~msr_content;
-        return 1;
+        return 0;
      case MSR_CORE_PERF_GLOBAL_STATUS:
          gdprintk(XENLOG_INFO, "Can not write readonly MSR: "
                   "MSR_PERF_GLOBAL_STATUS(0x38E)!\n");
-        hvm_inject_hw_exception(TRAP_gp_fault, 0);
          return 1;
Suspicious: Most return values get flipped, but this one (and at least
one more below) doesn't. Any such inconsistencies that are being
corrected (assuming this is intentional) as you go should be spelled
out in the description. And then the question of course is whether
it's really necessary to flip the meaning of this and some similar SVM
function's return values anyway.

The return value of 1 of vpmu_do_msr() will now indicate that the routine encountered a fault as opposed to indicating whether the MSR access was to a VPMU register.

I will make a comment to that effect.


@@ -541,45 +539,43 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t 
msr_content)
          }
      }
- if ( (global_ctrl & *enabled_cntrs) || (core2_vpmu_cxt->ds_area != 0) )
-        vpmu_set(vpmu, VPMU_RUNNING);
-    else
-        vpmu_reset(vpmu, VPMU_RUNNING);
The moving off this code is - again without at least a brief comment -
also not obviously correct; at least to me it looks like this slightly
alters behavior (perhaps towards the better, but anyway).

Right. Because we don't want to change VPMU state if a fault is encountered (which is possible with current code).

Come think of it, this is still not enough --- we may leave registers updated when fault happens. I should fix that too.

-boris


@@ -607,19 +603,14 @@ static int core2_vpmu_do_rdmsr(unsigned int msr, uint64_t 
*msr_content)
              rdmsrl(msr, *msr_content);
          }
      }
-    else
+    else if ( msr == MSR_IA32_MISC_ENABLE )
      {
          /* Extension for BTS */
-        if ( msr == MSR_IA32_MISC_ENABLE )
-        {
-            if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
-                *msr_content &= ~MSR_IA32_MISC_ENABLE_BTS_UNAVAIL;
-        }
-        else
-            return 0;
Again a case where the return value stays the same even if in
general it appears to get flipped in this function.

+        if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) )
+            *msr_content &= ~MSR_IA32_MISC_ENABLE_BTS_UNAVAIL;
      }
- return 1;
+    return 0;
  }
Jan



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


 


Rackspace

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