Re: [PATCH v7 09/19] x86/VPMU: Add public xenpmu.h

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

On 06/06/14 18:40, Boris Ostrovsky wrote:
> Add pmu.h header files, move various macros and structures that will be
> shared between hypervisor and PV guests to it.
>
> Move MSR banks out of architectural PMU structures to allow for larger sizes
> in the future. The banks are allocated immediately after the context and
> PMU structures store offsets to them.
>
> 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>
> ---
>  xen/arch/x86/hvm/svm/vpmu.c              |  81 +++++++++++----------
>  xen/arch/x86/hvm/vmx/vpmu_core2.c        | 117 +++++++++++++++++--------------
>  xen/arch/x86/hvm/vpmu.c                  |   5 ++
>  xen/arch/x86/oprofile/op_model_ppro.c    |   6 +-
>  xen/include/asm-x86/hvm/vmx/vpmu_core2.h |  32 ---------
>  xen/include/asm-x86/hvm/vpmu.h           |  16 ++---
>  xen/include/public/arch-arm.h            |   3 +
>  xen/include/public/arch-x86/pmu.h        |  58 +++++++++++++++
>  xen/include/public/pmu.h                 |  38 ++++++++++
>  9 files changed, 224 insertions(+), 132 deletions(-)
>  delete mode 100644 xen/include/asm-x86/hvm/vmx/vpmu_core2.h
>  create mode 100644 xen/include/public/arch-x86/pmu.h
>  create mode 100644 xen/include/public/pmu.h
>
> diff --git a/xen/arch/x86/hvm/svm/vpmu.c b/xen/arch/x86/hvm/svm/vpmu.c
> @@ -382,7 +382,9 @@ static int amd_vpmu_initialise(struct vcpu *v)
>  	 }
>      }
>  
> -    ctxt = xzalloc(struct amd_vpmu_context);
> +    ctxt = xzalloc_bytes(sizeof(struct xen_pmu_amd_ctxt) + 
> +			 sizeof(uint64_t) * AMD_MAX_COUNTERS + 
> +			 sizeof(uint64_t) * AMD_MAX_COUNTERS);

Two lots of counters?  If so, then * 2 would be better.

>      if ( !ctxt )
>      {
>          gdprintk(XENLOG_WARNING, "Insufficient memory for PMU, "
> @@ -391,7 +393,11 @@ static int amd_vpmu_initialise(struct vcpu *v)
>          return -ENOMEM;
>      }
>  
> +    ctxt->counters = sizeof(struct xen_pmu_amd_ctxt);
> +    ctxt->ctrls = ctxt->counters + sizeof(uint64_t) * AMD_MAX_COUNTERS;
> +
>      vpmu->context = ctxt;
> +    vpmu->priv_context = NULL;
>      vpmu_set(vpmu, VPMU_CONTEXT_ALLOCATED);
>      return 0;
>  }
> @@ -403,8 +409,7 @@ static void amd_vpmu_destroy(struct vcpu *v)
>      if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
>          return;
>  
> -    if ( has_hvm_container_domain(v->domain) &&
> -         ((struct amd_vpmu_context *)vpmu->context)->msr_bitmap_set )
> +    if ( has_hvm_container_domain(v->domain) && is_msr_bitmap_on(vpmu) )
>          amd_vpmu_unset_msr_bitmap(v);
>  
>      xfree(vpmu->context);
> @@ -421,7 +426,9 @@ static void amd_vpmu_destroy(struct vcpu *v)
>  static void amd_vpmu_dump(const struct vcpu *v)
>  {
>      const struct vpmu_struct *vpmu = vcpu_vpmu(v);
> -    const struct amd_vpmu_context *ctxt = vpmu->context;
> +    const struct xen_pmu_amd_ctxt *ctxt = vpmu->context;
> +    uint64_t *counter_regs = vpmu_reg_pointer(ctxt, counters);
> +    uint64_t *ctrl_regs = vpmu_reg_pointer(ctxt, ctrls);

const for both of these.  The pointer arithmatic in vpmu_reg_pointer()
hides what would otherwise should be a compiler error.

> diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
> index 4ddac7d..56b48be 100644
> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -31,6 +31,7 @@
>  #include <asm/hvm/svm/svm.h>
>  #include <asm/hvm/svm/vmcb.h>
>  #include <asm/apic.h>
> +#include <public/pmu.h>
>  
>  /*
>   * "vpmu" :     vpmu generally enabled
> @@ -228,6 +229,10 @@ void vpmu_initialise(struct vcpu *v)
>      struct vpmu_struct *vpmu = vcpu_vpmu(v);
>      uint8_t vendor = current_cpu_data.x86_vendor;
>  
> +    BUILD_BUG_ON((sizeof(struct xen_pmu_intel_ctxt) > XENPMU_CTXT_PAD_SZ) ||
> +                 (sizeof(struct xen_pmu_amd_ctxt) > XENPMU_CTXT_PAD_SZ));
> +

Can this be two BUILD_BUG_ON()s so the compiler tells you which
structure is too big?

~Andrew