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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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.

> +/* 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?

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.

> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -109,6 +109,7 @@
>  ?    pmu_cntr_pair                   arch-x86/pmu.h
>  ?    pmu_intel_ctxt                  arch-x86/pmu.h
>  ?    pmu_regs                        arch-x86/pmu.h
> +?    pmu_params                      pmu.h

Apparently I overlooked this in earlier patches and revision: The
entries in this file are (mostly) alphabetically sorted (by filename,
then by structure name). Please don't make the situation worse
than it already is.

Jan


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