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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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