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: xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: Re: [Xen-devel] [PATCH] 2/3: MCA/MCE correctable error handling
From: "Christoph Egger" <Christoph.Egger@xxxxxxx>
Date: Wed, 22 Aug 2007 10:47:30 +0200
Cc: Gavin.Maltby@xxxxxxx, Keir Fraser <keir@xxxxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxxxx>
Delivery-date: Wed, 22 Aug 2007 01:48:33 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <46CB26A9.76E4.0078.0@xxxxxxxxxx>
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> <46CB26A9.76E4.0078.0@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: KMail/1.9.6
On Tuesday 21 August 2007 17:53:45 Jan Beulich wrote:
> >+} __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).

Oh, you're right. I should use #pragma pack(1)  instead.
Is this fine with you?

> >+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).

mc_coreid is the physical core that reported the machine check information.
mc_socketid is the physical socket the physical core is in. This is useful
to find all other affected physical cores, when an error in the L3-Cache is 
reported that is shared over all cores in the chip.

mc_vcpu_id is the id of the active vcpu for the domain, that reported the
machine check information. Inside Xen, this is current->vcpu_id.
mc_vcpu_id is needed to deal properly with the upcoming NUMA support
my collegue is working on.

> >+/* 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...

MCINFO_MAXSIZE is the size for the content of the mi_data array.
MCINFO_MAXSIZE + sizeof(mi_nentries) == 204. That is where is number comes

> >     /* 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?

You want me to replace "uint64_t pad[32];" with "uint32_t pad[13];" ?

AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

Xen-devel mailing list