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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.06.15 at 19:17, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 06/15/2015 11:50 AM, Jan Beulich wrote:
>>>>> On 10.06.15 at 17:04, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> +    {
>>> +        unsigned int num_enabled = 0;
>>> +        struct xen_pmu_amd_ctxt *guest_ctxt = 
>>> &vpmu->xenpmu_data->pmu.c.amd;
>>> +
>>> +        ASSERT(!is_hvm_vcpu(v));
>>> +
>>> +        ctxt = vpmu->context;
>>> +        ctrl_regs = vpmu_reg_pointer(ctxt, ctrls);
>>> +
>>> +        memcpy(&ctxt->regs[0], &guest_ctxt->regs[0], regs_sz);
>> So that's the live structure, not any staging area iiuc. What state
>> is the guest going to be in when validation fails (and you can't
>> restore the original state)? And what guarantees that nothing
>> elsewhere in the hypervisor uses the data _before_ the
>> validation below succeeds?
> 
> 
> We don't load this data into hardware until we have validated it. On 
> failed validation guest will receive hypercall error --- it's up to the 
> guest to decide what to do.
> 
> The hypervisor will not use this data as it will still be flagged as 
> PMU_CACHED, i.e. invalid/stale. (That's why I say in the comment below 
> that re-initializing it is really not necessary)

Okay, thanks.

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

Jan


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