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

Re: [Xen-devel] [PATCH v24 04/15] x86/VPMU: Interface for setting PMU mode and flags



On 06/11/2015 04:17 AM, Tian, Kevin wrote:
From: Boris Ostrovsky [mailto:boris.ostrovsky@xxxxxxxxxx]

      switch ( vendor )
      {
      case X86_VENDOR_AMD:
-        ret = svm_vpmu_initialise(v, opt_vpmu_enabled);
+        ret = svm_vpmu_initialise(v);
          break;

      case X86_VENDOR_INTEL:
-        ret = vmx_vpmu_initialise(v, opt_vpmu_enabled);
+        ret = vmx_vpmu_initialise(v);
          break;

      default:
-        if ( opt_vpmu_enabled )
+        if ( vpmu_mode != XENPMU_MODE_OFF )
          {
              printk(XENLOG_G_WARNING "VPMU: Unknown CPU vendor %d. "
                     "Disabling VPMU\n", vendor);
              opt_vpmu_enabled = 0;
+            vpmu_mode = XENPMU_MODE_OFF;
          }
-        return;
+        return; /* Don't bother restoring vpmu_count, VPMU is off forever */

why not restoring vpmu_count here? There could be a race condition
regarding to the mode control path:

+        if ( (vpmu_count == 0) || (vpmu_mode == pmu_params.val) ||
+             ((vpmu_mode ^ pmu_params.val) ==
+              (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
+            vpmu_mode = pmu_params.val;

It's possible "vpmu_mode=pmu_params.val" happens later than
"vpmu_mode = XENPMU_MODE_OFF"...

It might not be a big problem since opt_vpmu_enabled is 0 then, but
then there's pointless to reset vpmu_mode further if the behavior
is not guaranteed.


It is somewhat pointless to reset it, but mostly because if we ever get into the default case above we have much bigger problems than VPMU: the only way that I can see when this can happen (vendor not being AMD or Intel) is that current_cpu_data.x86_vendor got overwritten by something else, which means memory corruption. (I in fact wondered whether I should just stick a BUG() here).

BTW, even if I decremented vpmu_count above and took vpmu_lock to avoid
the race this would still not be enough to avoid VPMU-related inconsistencies: we would really need to make sure that no VCPU in the system (i.e. for all guests) is in the middle of a VPMU operation. And that would be somewhat non-trivial (short of pausing all guests, but then we'd still need to deal with dom0).

That's basically the reason for the "Don't bother" comment. Getting this completely right is way too much effort for no benefit (AFAICS).


+
+    case XENPMU_feature_set:
+        if ( pmu_params.val & ~XENPMU_FEATURE_INTEL_BTS )
+            return -EINVAL;
+
+        spin_lock(&vpmu_lock);
+
+        if ( vpmu_count == 0 )
+            vpmu_features = pmu_params.val;
+        else
+        {
+            printk(XENLOG_WARNING "VPMU: Cannot change features while"
+                                  " active VPMUs exist\n");
+            ret = -EBUSY;
+        }

what about setting same features as existing in vpmu_features?
we should do same check as done in mode setting.


Not sure I follow you. There is only one feature currently that we support --- BTS. And trying to set any other feature will result in -EINVAL. What is wrong with trying to set the same bit twice? (except for being pointless)

-boris


_______________________________________________
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®.