This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


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

To: "Christoph Egger" <Christoph.Egger@xxxxxxx>
Subject: Re: [Xen-devel] [PATCH] 2/3: MCA/MCE correctable error handling
From: "Jan Beulich" <jbeulich@xxxxxxxxxx>
Date: Tue, 21 Aug 2007 16:53:45 +0100
Cc: Gavin.Maltby@xxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxx, Keir Fraser <keir@xxxxxxxxxxxxx>
Delivery-date: Tue, 21 Aug 2007 08:53:45 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <200708211531.38706.Christoph.Egger@xxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <200708211531.38706.Christoph.Egger@xxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
>+} __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 
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?


Xen-devel mailing list