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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 06/11/2015 11:04 AM, Jan Beulich wrote:
>>>> On 11.06.15 at 16:54, <boris.ostrovsky@oracle.com> wrote:
>> On 06/11/2015 04:17 AM, Tian, Kevin wrote:
>>>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>>>>        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