Re: [Xen-devel] [PATCH v4 07/17] x86/VPMU: Add public xenpmu.h

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.01.14 at 20:08, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> Add xenpmu.h header file,

To me, naming a public Xen header (other than the core one) xen*.h
is redundant. There's no information lost if you just called it pmu.h.

Also I think you ought to use plural here.

> --- /dev/null
> +++ b/xen/include/public/arch-x86/xenpmu.h
> @@ -0,0 +1,66 @@
> +#ifndef __XEN_PUBLIC_ARCH_X86_PMU_H__
> +#define __XEN_PUBLIC_ARCH_X86_PMU_H__
> +
> +/* x86-specific PMU definitions */
> +
> +#include "xen.h"

Why?

> +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;
> +    uint64_t fixed_counters;  /* Offset to fixed counter MSRs */
> +    uint64_t arch_counters;   /* Offset to architectural counter MSRs */

I think these last two could easily be uint32_t.

> +/* Shared between hypervisor and PV domain */
> +struct xen_pmu_data {
> +    uint32_t domain_id;
> +    uint32_t vcpu_id;
> +    uint32_t pcpu_id;
> +    uint32_t pmu_flags;
> +
> +    xen_arch_pmu_t pmu;
> +};

So if this got included by an architecture independent source file
on ARM, how would this build? You at least need a stub definition
there for xen_arch_pmu_t afaict (if already give the impression -
further up - that you're supporting ARM compilation of this header).

Jan


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