Re: [PATCH v7 11/19] x86/VPMU: Interface for setting PMU mode and flags

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 06/06/14 18:40, Boris Ostrovsky wrote:
> Add runtime interface for setting PMU mode and flags. Three main modes are
> provided:
> * PMU off
> * PMU on: Guests can access PMU MSRs and receive PMU interrupts. dom0
>   profiles itself and the hypervisor.
> * dom0-only PMU: dom0 collects samples for both itself and guests.
>
> For feature flags only Intel's BTS is currently supported.
>
> Mode and flags are set via HYPERVISOR_xenpmu_op hypercall.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Reviewed-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
> Tested-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
> ---
>
> @@ -275,3 +283,100 @@ void vpmu_dump(struct vcpu *v)
>          vpmu->arch_vpmu_ops->arch_vpmu_dump(v);
>  }
>  
> +/* Unload VPMU contexts */
> +static void vpmu_unload_all(void)
> +{
> +    struct domain *d;
> +    struct vcpu *v;
> +    struct vpmu_struct *vpmu;
> +
> +    for_each_domain( d )
> +    {
> +        for_each_vcpu ( d, v )
> +        {
> +            if ( v != current )
> +                vcpu_pause(v);
> +            vpmu = vcpu_vpmu(v);
> +
> +            if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
> +            {
> +                if ( v != current )
> +                    vcpu_unpause(v);
> +                continue;
> +            }
> +
> +            vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
> +            on_selected_cpus(cpumask_of(vpmu->last_pcpu),
> +                             vpmu_save_force, (void *)v, 1);
> +            vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
> +
> +            if ( v != current )
> +                vcpu_unpause(v);
> +        }
> +    }
> +}

This is a very long function without any preemption.

To reduce its impact greatly, would it be better to have a global mode
controlling vcpu context switching?  You could enable
"VCPU_GLOBAL_DISABLING", then IPI all cpus at the same time to save
their vpmu context.  Once the IPI is complete, all vpmu context is clean.

This avoids a double loop and all the vcpu_pause/unpause()s.

> +
> +
> +long do_xenpmu_op(int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
> +{
> +    int ret = -EINVAL;
> +    xen_pmu_params_t pmu_params;

This is probably going to want to be XSM'd up for permissions?

>
> diff --git a/xen/include/public/pmu.h b/xen/include/public/pmu.h
> index d237c25..d41049f 100644
> --- a/xen/include/public/pmu.h
> +++ b/xen/include/public/pmu.h
> @@ -13,6 +13,54 @@
>  #define XENPMU_VER_MAJ    0
>  #define XENPMU_VER_MIN    1
>  
> +/*
> + * ` enum neg_errnoval
> + * ` HYPERVISOR_xenpmu_op(enum xenpmu_op cmd, struct xenpmu_params *args);
> + *
> + * @cmd  == XENPMU_* (PMU operation)
> + * @args == struct xenpmu_params
> + */
> +/* ` enum xenpmu_op { */
> +#define XENPMU_mode_get        0 /* Also used for getting PMU version */
> +#define XENPMU_mode_set        1
> +#define XENPMU_feature_get     2
> +#define XENPMU_feature_set     3
> +/* ` } */
> +
> +/* Parameters structure for HYPERVISOR_xenpmu_op call */
> +struct xen_pmu_params {
> +    /* IN/OUT parameters */
> +    union {
> +        struct version {
> +            uint8_t maj;
> +            uint8_t min;
> +        } version;
> +        uint64_t pad;

introducing "struct version" into the namespace is very antisocial.

> +    } v;
> +    union {
> +        uint64_t val;
> +        XEN_GUEST_HANDLE(void) valp;
> +    } d;
> +
> +    /* IN parameters */
> +    uint64_t vcpu;
> +};

Bits are cheep these days.

struct xen_pmu_params {
    struct { uint32_t maj, min; } version;
    XEN_GUEST_HANDLE_64(void) valp;
    uint64_t vcpu;
};

Comes with far less padding/unions and is still 32/64bit safe.