Re: [Xen-devel] [PATCH] 2/3: MCA/MCE correctable error handling

["Jan Beulich" <jbeulich@xxxxxxxxxx>]

>+} __attribute__((packed));

I think it was a general agreement that it is not a good idea (non-portable to
non-GNU compilers) to put things like this in public headers. I think by 
properly
ordering things you can get away without this (and almost without padding).

>+struct mcinfo_global {
>...
>+    uint16_t mc_socketid;
>+    uint16_t mc_coreid;
>+    uint16_t mc_vcpu_id;

Unless mc_vcpu_id is intended for that purpose, this neglects hyperthreading (I
know, AMD doesn't use this, but the interface should be vendor neutral).

If mc_vcpu_id is meant for that purpose, its naming is ambiguous.

If mc_vcpu_id is meant as a vcpuid, then ordering things os that mc_domid and
mc_vcpu_id are contiguous would seem more natural (making eventual
examination in raw memory less confusing).

>+/* sizeof(struct mcinfo_global) + 6 * sizeof(struct mcinfo_bank) == 200.
>+ * This is enough space to store mc information of up to six banks.
>+ */
>+#define MCINFO_MAXSIZE (204 - sizeof(uint32_t))

Why don't you use the sizeof()-s from the description? If this is really needed
for some reason, then having 200 in the description and 204 in the macro is
at least confusing...

>     /* Frame containing list of mfns containing list of mfns containing p2m. 
> */
>     xen_pfn_t     pfn_to_mfn_frame_list_list;
>     unsigned long nmi_reason;
>+    struct arch_mc_info mc_info; /* machine check information */
>     uint64_t pad[32];
> };

Are you sure it is appropriate to add a member here without reducing the
padding member?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel