Re: [PATCH v18 05/16] x86/VPMU: Interface for setting PMU mode and flags

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

On 02/20/2015 08:59 AM, Jan Beulich wrote:
>>>> On 16.02.15 at 23:26, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/hvm/svm/vpmu.c
>> +++ b/xen/arch/x86/hvm/svm/vpmu.c
>> @@ -253,6 +253,26 @@ static int amd_vpmu_save(struct vpmu_struct *vpmu)
>>       return 1;
>>   }
>>   
>> +static void amd_vpmu_unload(struct vpmu_struct *vpmu)
>> +{
>> +    struct vcpu *v;
>> +
>> +    if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED | VPMU_FROZEN) )
>> +    {
>> +        unsigned int i;
>> +
>> +        for ( i = 0; i < num_counters; i++ )
>> +            wrmsrl(ctrls[i], 0);
>> +        context_save(vpmu);
> This assumes vpmu_vcpu(vpmu) == current, yet it looks like you're
> also calling it under different circumstances now.

This doesn't make this assumption. PMU MSRs may be loaded with values 
that belong to a VCPU that is not currently running if, for example, 
current VCPU is not using PMU. And flags test guarantees that we won't 
stomp on someone else's registers.

I could make a test here and write zeroes to the MSRs only if vpmu is 
current but I don't like leaving these MSRs with what is essentially 
garbage values. This routine is executed very rarely so overhead is 
negligible.



>
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -1183,11 +1183,10 @@ int vmx_read_guest_msr(u32 msr, u64 *val)
>>       return -ESRCH;
>>   }
>>   
>> -int vmx_write_guest_msr(u32 msr, u64 val)
>> +int vmx_write_guest_msr_vcpu(struct vcpu *v, u32 msr, u64 val)
>>   {
>> -    struct vcpu *curr = current;
>> -    unsigned int i, msr_count = curr->arch.hvm_vmx.msr_count;
>> -    struct vmx_msr_entry *msr_area = curr->arch.hvm_vmx.msr_area;
>> +    unsigned int i, msr_count = v->arch.hvm_vmx.msr_count;
>> +    struct vmx_msr_entry *msr_area = v->arch.hvm_vmx.msr_area;
>>   
>>       for ( i = 0; i < msr_count; i++ )
>>       {
> Same here - while the code itself is only accessing memory, it
> remains unclear whether correct behavior results when the subject
> vCPU is actually executing.

Not sure what you mean here. The changes are specifically for updating 
MSR values (cached in VMCS) of possibly non-current VCPU.

>
> In both cases, if there are restrictions on the conditions under
> which unload can validly be done, I think you should ASSERT()
> those conditions (at once making them explicit).
>
>> +long do_xenpmu_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
>> +{
>> +    int ret;
>> +    struct xen_pmu_params pmu_params;
>> +
>> +    if ( vpmu_disabled )
>> +        return -EINVAL;
> EOPNOTSUPP perhaps?
>
> And I agree with Dietmar that keeping opt_vpmu_enabled instead
> of introducing vpmu_disabled would seem more natural, the more
> that the default is disabled.

I can switch polarity back to enabled since both of you think it makes 
more sense but I don't think I should keep "opt_" prefix since this flag 
can be modified independently of what boot option says (e.g. when 
watchdog is on).

>
>> +/* Context switch */
>> +static inline void vpmu_switch_from(struct vpmu_struct *prev,
>> +                                    struct vpmu_struct *next)
>> +{
>> +    if ( vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV) )
>> +        vpmu_save(prev);
>> +}
>> +
>> +static inline void vpmu_switch_to(struct vpmu_struct *prev,
>> +                                  struct vpmu_struct *next)
>> +{
>> +    if ( vpmu_mode & (XENPMU_MODE_SELF | XENPMU_MODE_HV) )
>> +        vpmu_load(next);
>> +    else if ( vpmu_is_set(next, VPMU_CONTEXT_LOADED | VPMU_RUNNING) )
>> +        vpmu_unload(next);
> I don't recall there having been an unload path here, and I don't
> see this being explained anywhere either. Wouldn't that more
> naturally be done in the switch-from function?

I had this for a couple of revisions now. And I had a very specific 
reason for (1) having it and (2) having it here (it had something to do 
with the fact that 'next' may still be LOADED after we've changed 
vpmu_mode to, say, OFF) . But I can't remember what it was now. If I 
remember why I'll add a comment, otherwise I'll drop this.

>
> Apart from that it's also not clear why both prev and next get
> passed to both of these functions. Iirc a later patch may make
> use of that, but then that later patch should add the second
> function parameter.

Right, I was thinking of using both parameters at some point and left 
them here. Only one is needed.

-boris