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

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

On 06/23/2015 04:26 AM, Jan Beulich wrote:
>>>> On 22.06.15 at 18:10, <boris.ostrovsky@oracle.com> wrote:
>> On 06/22/2015 11:10 AM, Jan Beulich wrote:
>>>> +    switch ( op )
>>>> +    {
>>>> +    case XENPMU_mode_set:
>>>> +    {
>>>> +        if ( (pmu_params.val & ~(XENPMU_MODE_SELF | XENPMU_MODE_HV)) ||
>>>> +             (hweight64(pmu_params.val) > 1) )
>>>> +            return -EINVAL;
>>>> +
>>>> +        /* 32-bit dom0 can only sample itself. */
>>>> +        if ( is_pv_32bit_vcpu(current) && (pmu_params.val & XENPMU_MODE_HV) )
>>>> +            return -EINVAL;
>>>> +
>>>> +        spin_lock(&vpmu_lock);
>>>> +
>>>> +        /*
>>>> +         * We can always safely switch between XENPMU_MODE_SELF and
>>>> +         * XENPMU_MODE_HV while other VPMUs are active.
>>>> +         */
>>>> +        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;
>>>> +        else
>>>> +        {
>>> I think this would better be
>>>
>>>           if ( (vpmu_count == 0) ||
>>>                ((vpmu_mode ^ pmu_params.val) ==
>>>                 (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
>>>               vpmu_mode = pmu_params.val;
>>>           else if ( vpmu_mode != pmu_params.val )
>>>           {
>>>
>>> But there's no need to re-submit just because of this (it could be
>>> changed upon commit if you agree).
>> This will generate an error (with a message to the log) even if we keep
>> the mode unchanged, something that I wanted to avoid. (Same thing for
>> XENPMU_feature_set, which is what Kevin mentioned last time).
> I don't see this: The only difference to the code you have is -
> afaics - that my variant avoids the pointless write to vpmu_mode.

Oh, right, I was only looking at the change that you made to 'if', not 
to the 'esle'. Yes, that's good.

-boris