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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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