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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Boris Ostrovsky [mailto:boris.ostrovsky@xxxxxxxxxx]
> Sent: Thursday, June 11, 2015 10:54 PM
> 
> 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).

I got this explanation. Thanks.

> 
> 
> >> +
> >> +    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)
> 

My point is whether you want to allow setting same bit twice
when active PMUs exist. From above code it's disallowed
w/ check on vpmu_count. However in earlier code handling
vpmu_mode setting, you actually allow setting same mode
twice:

+        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;

So I thought you may keep the same policy to allow setting
vpmu_features with same bit twice too.

Thanks
Kevin


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