Re: [PATCH v22 11/14] x86/VPMU: Handle PMU interrupts for PV(H) guests

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

On 05/28/2015 11:05 AM, Aravind Gopalakrishnan wrote:
> On 5/26/2015 1:09 PM, Boris Ostrovsky wrote:
>> On 05/26/2015 12:24 PM, Jan Beulich wrote:
>>>>>> On 21.05.15 at 19:57, <boris.ostrovsky@oracle.com> wrote:
>>>> @@ -188,27 +189,52 @@ static inline void context_load(struct vcpu *v)
>>>>       }
>>>>   }
>>>>   -static void amd_vpmu_load(struct vcpu *v)
>>>> +static int amd_vpmu_load(struct vcpu *v, bool_t from_guest)
>>>>   {
>>>>       struct vpmu_struct *vpmu = vcpu_vpmu(v);
>>>> -    struct xen_pmu_amd_ctxt *ctxt = vpmu->context;
>>>> -    uint64_t *ctrl_regs = vpmu_reg_pointer(ctxt, ctrls);
>>>> +    struct xen_pmu_amd_ctxt *ctxt;
>>>> +    uint64_t *ctrl_regs;
>>>> +    unsigned int i;
>>>>         vpmu_reset(vpmu, VPMU_FROZEN);
>>>>   -    if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
>>>> +    if ( !from_guest && vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
>>>>       {
>>>> -        unsigned int i;
>>>> +        ctxt = vpmu->context;
>>>> +        ctrl_regs = vpmu_reg_pointer(ctxt, ctrls);
>>>>             for ( i = 0; i < num_counters; i++ )
>>>>               wrmsrl(ctrls[i], ctrl_regs[i]);
>>>>   -        return;
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    if ( from_guest )
>>>> +    {
>>>> +        ASSERT(!is_hvm_vcpu(v));
>>>> +
>>>> +        ctxt = &vpmu->xenpmu_data->pmu.c.amd;
>>>> +        ctrl_regs = vpmu_reg_pointer(ctxt, ctrls);
>>>> +        for ( i = 0; i < num_counters; i++ )
>>>> +        {
>>>> +            if ( is_pmu_enabled(ctrl_regs[i]) )
>>>> +            {
>>>> +                vpmu_set(vpmu, VPMU_RUNNING);
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +
>>>> +        if ( i == num_counters )
>>>> +            vpmu_reset(vpmu, VPMU_RUNNING);
>>>> +
>>>> +        memcpy(vpmu->context, &vpmu->xenpmu_data->pmu.c.amd, 
>>>> ctxt_sz);
>>>>       }
>>>>         vpmu_set(vpmu, VPMU_CONTEXT_LOADED);
>>>>         context_load(v);
>>>> +
>>>> +    return 0;
>>>>   }
>>> So no verification needed at all on the AMD side? If so,
>>
>>
>> So I went back to BKDGs and it looks like some models of family 15 
>> redefined one of the bits from Reserved to MBZ so I think I'll need 
>> to verify that bit now.
>>
>> It's rather strange that this bit (MSRC001_0200[19]) is reserved for 
>> models 00h-0Fh and 30-3Fh but is MBZ for 10h-1Fh. It is also reserved 
>> for families 10h and 16h. I don't have access to the MBZ models so I 
>> can't test whether it is indeed MBZ or a typo in the spec (I can 
>> certainly write it with 1 on family 10h and 15h/model2).
>
>
> So I asked about it internally and it seems it is indeed a BKDG error. 
> The bit is 'Reserved'.
> I also tried writing 1 to it on Fam15h Model10h and it works fine.

Excellent, thanks Aravind!

As Jan pointed out though for Reserved fields we still may need to 
preserve bits if we are to follow BKDG to the letter (although writing 
them doesn't seem to have any effect, at least on processors that I tried).

And in fact we don't check those bits currently neither so I think I'll 
add a separate patch to verify them in amd_vpmu_do_wrmsr().

-boris