Re: [PATCH v24 12/15] x86/VPMU: Handle PMU interrupts for PV(H) guests

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

On 06/16/2015 03:45 AM, Jan Beulich wrote:
>>>> On 15.06.15 at 19:17, <boris.ostrovsky@oracle.com> wrote:
>>>>
>>>> +        for ( i = 0; i < num_counters; i++ )
>>>> +        {
>>>> +            if ( (ctrl_regs[i] & CTRL_RSVD_MASK) != ctrl_rsvd[i] )
>>>> +            {
>>>> +                /*
>>>> +                 * Not necessary to re-init context since we should never load
>>>> +                 * it until guest provides valid values. But just to be safe.
>>>> +                 */
>>>> +                amd_vpmu_init_regs(ctxt);
>>>> +                return -EINVAL;
>>>> +            }
>>>> +
>>>> +            if ( is_pmu_enabled(ctrl_regs[i]) )
>>>> +                num_enabled++;
>>>> +        }
>>>> +
>>>> +        if ( num_enabled )
>>> Looks like a boolean flag would do - the exact count doesn't seem to
>>> be of interest here or in later patches?
>> So the reason why I use a counter here is because keeping track of
>> VPMU_RUNNING state is currently broken on AMD, I noticed it when I was
>> updating this patch. amd_vpmu_do_wrmsr() will reset VPMU_RUNNING if the
>> MSR write is disabling current counter, even though there may still be
>> other counters running. This may be related to HVM brokenness of AMD
>> counters that I mentioned a while ago where the guest, when running with
>> multiple counters, sometimes gets unexpected NMIs. (This goes back all
>> the way to to 4.1.)
>>
>> I don't want to fix this in this series but I will likely need to count
>> number of active counters when I do (just like I do for Intel).
>>
>> I can use a boolean now though since I am not dealing with this problem
>> here.
> If another rev is needed, I'd prefer if you did so. But if we can have
> this version go in (provided we get all the necessary acks), I wouldn't
> insist on you doing another round just because of this.

I think there are a couple of (fairly cosmetic) changes that need to be 
done so there will be another rev.

OTOH I just tried a quick-and-dirty fix for this problem and it doesn't 
resolve it so presumably there is more to this. It's not particularly 
invasive but I think it would be rather pointless to put it in as it 
still doesn't allow multiple counters on AMD and I suspect the final fix 
will be touching the same code again.

-boris