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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 20.02.15 at 17:04, <boris.ostrovsky@oracle.com> wrote:
> 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.

The question wasn't on the wrmsr() - writing zero here
unconditionally is fine - but on the actions context_save() takes.

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

But non-current doesn't mean not-running. I.e. what if the vCPU
you deal with here is active on another pCPU?

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

Grep through the sources, and if you don't find other examples
doing so (I think you will), I'm fine with you dropping the opt_prefix
(in a separate patch).

Jan