Re: [Xen-devel] [PATCH v12 for-xen-4.5 09/20] x86/VPMU: Add public xenpmu.h

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.09.14 at 21:28, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -385,7 +389,8 @@ static int amd_vpmu_initialise(struct vcpu *v)
>        }
>      }
>  
> -    ctxt = xzalloc(struct amd_vpmu_context);
> +    ctxt = xzalloc_bytes(sizeof(struct xen_pmu_amd_ctxt) +
> +                         2 * sizeof(uint64_t) * AMD_MAX_COUNTERS);
>      if ( !ctxt )
>      {
>          gdprintk(XENLOG_WARNING, "Insufficient memory for PMU, "
> @@ -394,7 +399,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;

Is using the compile time count really necessary? I.e. is the runtime
limit (which hopefully is going to be lower) not possible here? If not,
why is doing so on the VMX side possible?

> @@ -228,6 +229,11 @@ 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);
> +    BUILD_BUG_ON(sizeof(struct xen_pmu_amd_ctxt) > XENPMU_CTXT_PAD_SZ);
> +    BUILD_BUG_ON(sizeof(struct xen_pmu_regs) > XENPMU_REGS_PAD_SZ);
> +    BUILD_BUG_ON(sizeof(struct compat_pmu_regs) > XENPMU_REGS_PAD_SZ);

I'm having trouble finding where struct compat_pmu_regs gets defined
(largely since you're not adding anything to xen/include/xlat.h).

>  #define vpmu_vcpu(vpmu)   (container_of((vpmu), struct vcpu, \
>                                            arch.hvm_vcpu.vpmu))
> -#define vpmu_domain(vpmu) (vpmu_vcpu(vpmu)->domain)

Is this really useful to delete, i.e. are absolutely sure that no future
use will ever arise?

> --- a/xen/include/public/arch-x86/xen-x86_64.h
> +++ b/xen/include/public/arch-x86/xen-x86_64.h
> @@ -174,6 +174,14 @@ struct cpu_user_regs {
>  typedef struct cpu_user_regs cpu_user_regs_t;
>  DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t);
>  
> +struct xen_pmu_regs {
> +    __DECL_REG(ip);
> +    __DECL_REG(sp);

Do you really need __DECL_REG() here? I.e. can't these two fields
be just xen_ulong_t e[is]p and the structure definition then be
shared with 32-bit code (and hence moved altogether into pmu.h)?

Jan


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