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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 01/13/2014 08:39 AM, Jan Beulich wrote:
> > --- /dev/null
> > +++ b/xen/include/public/arch-x86/xenpmu.h
> > @@ -0,0 +1,74 @@
....

> > +
> > +/* 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?
'msr_bitmap_set' field is actually never used by PV and it's the last 
one in the structure which is why I didn't bother to make it bigger. But 
you are right, I should fix this to avoid problems in the future.
> > +
> > +/* 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?
I'll drop this. I thought anonymous unions looked better but now that I 
look at it again I think the ifdefs are rather ugly too.
> > +
> > +#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.
You mean something like arch_xenpmu ==> xen_arch_pmu ?

-boris


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