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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.01.14 at 20:24, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> --- /dev/null
> +++ b/xen/include/public/arch-x86/xenpmu.h
> @@ -0,0 +1,74 @@
> +#ifndef __XEN_PUBLIC_ARCH_X86_PMU_H__
> +#define __XEN_PUBLIC_ARCH_X86_PMU_H__
> +
> +/* x86-specific PMU definitions */
> +#include "xen.h"

The comment looks like it is describing the #include, which it
clearly isn't. Please have a blank line between them.

> +
> +/* Start of PMU register bank */
> +#define vpmu_reg_pointer(ctxt, offset) ((void *)((uintptr_t)ctxt + \
> +                                                 (uintptr_t)ctxt->offset))

This doesn't seem to belong in a public header.

> +
> +/* AMD PMU registers and structures */
> +struct amd_vpmu_context {
> +    uint64_t counters;      /* Offset to counter MSRs */
> +    uint64_t ctrls;         /* Offset to control MSRs */
> +    uint8_t msr_bitmap_set; /* Used by HVM only */
> +};

sizeof() of this structure will differ between 32- and 64-bit guests -
are you intending to do the necessary translation even though it
seems rather easy to avoid having to do so?

> +
> +/* Intel PMU registers and structures */
> +struct arch_cntr_pair {
> +    uint64_t counter;
> +    uint64_t control;
> +};
> +struct core2_vpmu_context {

Blank line missing between the two structures.

> +    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 */
> +};
> +
> +/* ANSI-C does not support anonymous unions */
> +#if !defined(__GNUC__) || defined(__STRICT_ANSI__)
> +#define __ANON_UNION_NAME(x) x
> +#else
> +#define __ANON_UNION_NAME(x)
> +#endif

Why? And if really needed, why here?

> +
> +#define XENPMU_MAX_CTXT_SZ        (sizeof(struct amd_vpmu_context) > \
> +                                    sizeof(struct core2_vpmu_context) ? \
> +                                     sizeof(struct amd_vpmu_context) : \
> +                                     sizeof(struct core2_vpmu_context))
> +#define XENPMU_CTXT_PAD_SZ        (((XENPMU_MAX_CTXT_SZ + 64) & ~63) + 128)
> +struct arch_xenpmu {
> +    union {
> +        struct cpu_user_regs regs;
> +        uint8_t pad1[256];
> +    } __ANON_UNION_NAME(r);
> +    union {
> +        uint32_t lapic_lvtpc;
> +        uint64_t pad2;
> +    } __ANON_UNION_NAME(l);
> +    union {
> +        struct amd_vpmu_context amd;
> +        struct core2_vpmu_context intel;
> +        uint8_t pad3[XENPMU_CTXT_PAD_SZ];
> +    } __ANON_UNION_NAME(c);

I don't think there's be a severe problem if you simply always
had names on these unions.

> +};
> +typedef struct arch_xenpmu arch_xenpmu_t;

Overall you should also prefix all types added to global scope with
"xen". I know this wasn't done consistently for older headers, but
we shouldn't be extending this name space cluttering.

Jan


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