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

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

On 02/20/2015 11:23 AM, Jan Beulich wrote:
>>>> 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.

Same reasoning for context_save() (which is a bunch of RDMSRs) --- if 
flags indicate that the context is LOADED|FROZEN then contents of those 
MSRs belong to this vpmu, regardless of whether it is current.

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

I see. With VPMU (which is the only place that calls this routine) this 
will never happen but the interface itself allows it. I'll add an ASSERT 
or some other test to make sure.


-boris