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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 06/11/2015 11:04 AM, Jan Beulich wrote:
> > > > On 11.06.15 at 16:54, <boris.ostrovsky@xxxxxxxxxx> wrote:
> > 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.
> Or you're running on a Cyrix CPU (unless there's code earlier on
> preventing that switch() from being reached).
Can't happen: we are getting into vpmu_initialise() from
    * VMX or SVM code --- so it's AMD or Intel.
    * pvpmu_init() but it is gated by 'vpmu_mode == XENPMU_MODE_OFF'. 
And the only way vpmu_mode can be anything but XENPMU_MODE_OFF is if 
vpmu_init() set it in the first place. And that routine does check for 
boot CPU's vendor.
As a side question, will Xen boot on Cyrix (or are they VIA)?

-boris


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