Re: [Xen-devel] [PATCH v6 08/19] x86/VPMU: Add public xenpmu.h

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.14 at 17:53, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -83,12 +84,10 @@ static const u32 AMD_F15H_CTRLS[] = {
>      MSR_AMD_FAM15H_EVNTSEL5
>  };
>  
> -/* storage for context switching */
> -struct amd_vpmu_context {
> -    u64 counters[MAX_NUM_COUNTERS];
> -    u64 ctrls[MAX_NUM_COUNTERS];
> -    bool_t msr_bitmap_set;
> -};
> +/* Use private context as a flag for MSR bitmap */

Stop missing.

> +#define msr_bitmap_on(vpmu)    {vpmu->priv_context = (void *)-1;}
> +#define msr_bitmap_off(vpmu)   {vpmu->priv_context = NULL;}

Blanks inside the braces please. Also the constant above would better
be -1L.

> +#define is_msr_bitmap_on(vpmu) (vpmu->priv_context != NULL)

All three macros fail to properly parenthesize their parameter.

> @@ -370,12 +371,20 @@ static int core2_vpmu_alloc_resource(struct vcpu *v)
>          goto out_err;
>      vmx_write_guest_msr(MSR_CORE_PERF_GLOBAL_CTRL, 0);
>  
> -    core2_vpmu_cxt = xzalloc_bytes(sizeof(struct core2_vpmu_context) +
> -                    (arch_pmc_cnt-1)*sizeof(struct arch_msr_pair));
> -    if ( !core2_vpmu_cxt )
> +    core2_vpmu_cxt = xzalloc_bytes(sizeof(struct xen_pmu_intel_ctxt) +
> +                                   sizeof(uint64_t) * fixed_pmc_cnt +
> +                                   sizeof(struct xen_pmu_cntr_pair) *
> +                                   arch_pmc_cnt);
> +    p = xzalloc_bytes(sizeof(uint64_t));
> +    if ( !core2_vpmu_cxt || !p )
>          goto out_err;
>  
> +    core2_vpmu_cxt->fixed_counters = sizeof(struct xen_pmu_intel_ctxt);
> +    core2_vpmu_cxt->arch_counters = core2_vpmu_cxt->fixed_counters +
> +                                    sizeof(uint64_t) * fixed_pmc_cnt;
> +
>      vpmu->context = (void *)core2_vpmu_cxt;
> +    vpmu->priv_context = (void *)p;

Pointless cast.

> @@ -447,10 +460,11 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, 
> uint64_t msr_content)
>      }
>  
>      core2_vpmu_cxt = vpmu->context;
> +    enabled_cntrs = (uint64_t *)vpmu->priv_context;

Again.

> --- /dev/null
> +++ b/xen/include/public/arch-x86/pmu.h
> @@ -0,0 +1,62 @@
> +#ifndef __XEN_PUBLIC_ARCH_X86_PMU_H__
> +#define __XEN_PUBLIC_ARCH_X86_PMU_H__
> +
> +/* x86-specific PMU definitions */
> +
> +/* AMD PMU registers and structures */
> +struct xen_pmu_amd_ctxt {
> +    uint32_t counters;       /* Offset to counter MSRs */
> +    uint32_t ctrls;          /* Offset to control MSRs */
> +};
> +
> +/* Intel PMU registers and structures */
> +struct xen_pmu_cntr_pair {
> +    uint64_t counter;
> +    uint64_t control;
> +};
> +
> +struct xen_pmu_intel_ctxt {
> +    uint64_t global_ctrl;
> +    uint64_t global_ovf_ctrl;
> +    uint64_t global_status;
> +    uint64_t fixed_ctrl;
> +    uint64_t ds_area;
> +    uint64_t pebs_enable;
> +    uint64_t debugctl;
> +    uint32_t fixed_counters;  /* Offset to fixed counter MSRs */
> +    uint32_t arch_counters;   /* Offset to architectural counter MSRs */
> +};
> +
> +#define XENPMU_MAX_CTXT_SZ        (sizeof(struct xen_pmu_amd_ctxt) > \
> +                                    sizeof(struct xen_pmu_intel_ctxt) ? \
> +                                     sizeof(struct xen_pmu_amd_ctxt) : \
> +                                     sizeof(struct xen_pmu_intel_ctxt))
> +#define XENPMU_CTXT_PAD_SZ        (((XENPMU_MAX_CTXT_SZ + 64) & ~63) + 128)

Is this really usefully derived from XENPMU_MAX_CTXT_SZ? I.e. is it
okay for this value to change when one of the structures grows big
enough? I would have thought that the padding below is to fix the
size of the structure once and for all (and if that's right, I suppose
a respective BUILD_BUG_ON() would be quite desirable).

> +struct xen_arch_pmu {
> +    union {
> +        struct cpu_user_regs regs;
> +        uint8_t pad1[256];
> +    } r;
> +    union {
> +        uint32_t lapic_lvtpc;
> +        uint64_t pad2;
> +    } l;
> +    union {
> +        struct xen_pmu_amd_ctxt amd;
> +        struct xen_pmu_intel_ctxt intel;
> +        uint8_t pad3[XENPMU_CTXT_PAD_SZ];

No need for the number suffixes on the pad fields now that the
unions each have a name.

> --- /dev/null
> +++ b/xen/include/public/pmu.h
> @@ -0,0 +1,38 @@
> +#ifndef __XEN_PUBLIC_PMU_H__
> +#define __XEN_PUBLIC_PMU_H__
> +
> +#include "xen.h"
> +#if defined(__i386__) || defined(__x86_64__)
> +#include "arch-x86/pmu.h"
> +#elif defined (__arm__) || defined (__aarch64__)
> +#include "arch-arm.h"
> +#else
> +#error "Unsupported architecture"
> +#endif
> +
> +#define XENPMU_VER_MAJ    0
> +#define XENPMU_VER_MIN    0

Do you really want to start at 0.0?

Jan

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