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

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

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