From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Christoph Egger" Subject: Re: [PATCH] 2/3: MCA/MCE correctable error handling Date: Wed, 22 Aug 2007 10:47:30 +0200 Message-ID: <200708221047.31033.Christoph.Egger@amd.com> References: <200708211531.38706.Christoph.Egger@amd.com> <46CB26A9.76E4.0078.0@novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <46CB26A9.76E4.0078.0@novell.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: xen-devel@lists.xensource.com Cc: Gavin.Maltby@sun.com, Keir Fraser , Jan Beulich List-Id: xen-devel@lists.xenproject.org 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-portab= le > 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= =20 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) =3D=3D= 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) =3D=3D 204. That is where is number co= mes from. > > /* 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];" ? =2D-=20 AMD Saxony, Dresden, Germany Operating System Research Center Legal Information: AMD Saxony Limited Liability Company & Co. KG Sitz (Gesch=E4ftsanschrift): Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland Registergericht Dresden: HRA 4896 vertretungsberechtigter Komplement=E4r: AMD Saxony LLC (Sitz Wilmington, Delaware, USA) Gesch=E4ftsf=FChrer der AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy