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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 02/20/2015 08:59 AM, Jan Beulich wrote:
> > > > On 16.02.15 at 23:26, <boris.ostrovsky@xxxxxxxxxx> 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

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