[RFC] [PATCH 0/2] Some clean-up to MCA handling

[RFC] [PATCH 0/2] Some clean-up to MCA handling

[Jiang, Yunhong]

These two patches includes some clean-up/changes to MCA handling.

The mce_extended.patch change the method to get the extended MCA information. This change is somwhat straightforward and is easy to understand. But please notice it changes some interface in include/public/arch-x86/xen-mca.h.

The mce_ops.patch add mca_ops to MCE/polling/CMCI handler, to combine the handling process and then wrap mce(intel and AMD)/polling/CMCI with it. I'm sorry that it also include some Intel specific changes as listed in intem 'g' in corresponding patch description. I tried to maintain it as different patch but later found it bring too much extra effort, so I qfold them together.

Christoph/Frank, can you please have a look on the mce_ops method, especially the working flow of mcheck_mca_handler and the mca_ops interface definition?

This patchset depends on the two patches I sent out yesterday, but those patches are very straightforward and thus I didn't emphasize as RFC.

Thanks
Yunhong Jiang

Re: [RFC] [PATCH 0/2] Some clean-up to MCA handling

[Christoph Egger]

On Monday 19 April 2010 10:58:44 Jiang, Yunhong wrote:
> These two patches includes some clean-up/changes to MCA handling.
>
> The mce_extended.patch change the method to get the extended MCA
> information. This change is somwhat straightforward and is easy to
> understand. But please notice it changes some interface in
> include/public/arch-x86/xen-mca.h.

This breaks backward-compatibility. You can't change the API/ABI just for the 
sake of "easier" internal handling.

Please explain:
- what exactly is wrong with the interface as it is in upstream ?
- what *requries* an API/ABI change ?

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

RE: [RFC] [PATCH 0/2] Some clean-up to MCA handling

[Jiang, Yunhong]

>-----Original Message-----
>From: Christoph Egger [mailto:Christoph.Egger@amd.com]
>Sent: Monday, April 19, 2010 6:07 PM
>To: Jiang, Yunhong
>Cc: Frank.Vanderlinden@Sun.COM; Keir Fraser; xen-devel@lists.xensource.com
>Subject: Re: [RFC] [PATCH 0/2] Some clean-up to MCA handling
>
>On Monday 19 April 2010 10:58:44 Jiang, Yunhong wrote:
>> These two patches includes some clean-up/changes to MCA handling.
>>
>> The mce_extended.patch change the method to get the extended MCA
>> information. This change is somwhat straightforward and is easy to
>> understand. But please notice it changes some interface in
>> include/public/arch-x86/xen-mca.h.
>
>This breaks backward-compatibility. You can't change the API/ABI just for the
>sake of "easier" internal handling.
>
>Please explain:
>- what exactly is wrong with the interface as it is in upstream ?
>- what *requries* an API/ABI change ?

It is not "easier" internalhandling. In fact, it makes no difference to internal handling at all. The reason is:
1) In amd_f10.c, it will only occupy 4 mc_msr, while in Intel platform, it may occupy 32 mc_msr, that is sure to cost extra space. The mc_info buffer is quite limited and can't be expanded in run time, so reduce the size is quite important.
2) sizeof(void *) is different in 64 hypervisor and32 bit dom0. I'm not sure if it is tested in compatibility mode, which might be confused.

In fact, since we have mc_msrs included in mcinfo_extended already, the caller can get the size of the buffer quite easy.

Of course, if you *really* don't care the waste of size in AMD platform, it's ok for me. After all, in intel platform, either there is no extended information, or it will occupy all of them, so it really does not matter to me. But the (void*) issue should be resolved, I suspect.

How about your option to the other patch?

Thanks
--jyh

>
>Christoph
>
>
>--
>---to satisfy European Law for business letters:
>Advanced Micro Devices GmbH
>Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
>Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
>Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
>Registergericht Muenchen, HRB Nr. 43632

Re: [RFC] [PATCH 0/2] Some clean-up to MCA handling

[Christoph Egger]

On Monday 19 April 2010 17:32:17 Jiang, Yunhong wrote:
> >-----Original Message-----
> >From: Christoph Egger [mailto:Christoph.Egger@amd.com]
> >Sent: Monday, April 19, 2010 6:07 PM
> >To: Jiang, Yunhong
> >Cc: Frank.Vanderlinden@Sun.COM; Keir Fraser; xen-devel@lists.xensource.com
> >Subject: Re: [RFC] [PATCH 0/2] Some clean-up to MCA handling
> >
> >On Monday 19 April 2010 10:58:44 Jiang, Yunhong wrote:
> >> These two patches includes some clean-up/changes to MCA handling.
> >>
> >> The mce_extended.patch change the method to get the extended MCA
> >> information. This change is somwhat straightforward and is easy to
> >> understand. But please notice it changes some interface in
> >> include/public/arch-x86/xen-mca.h.
> >
> >This breaks backward-compatibility. You can't change the API/ABI just for
> > the sake of "easier" internal handling.
> >
> >Please explain:
> >- what exactly is wrong with the interface as it is in upstream ?
> >- what *requries* an API/ABI change ?
>
> It is not "easier" internalhandling. In fact, it makes no difference to
> internal handling at all. The reason is: 1) In amd_f10.c, it will only
> occupy 4 mc_msr,

well, 4 in the generic handler plus 3 MSRs via mcinfo_extended which have
been introduced in family10h.

> while in Intel platform, it may occupy 32 mc_msr, that is 
> sure to cost extra space. The mc_info buffer is quite limited and can't be
> expanded in run time, so reduce the size is quite important.
> 2) sizeof(void *) is different in 64 hypervisor and 32 bit dom0. I'm not
> sure if it is tested in compatibility mode, which might be confused.
>
> In fact, since we have mc_msrs included in mcinfo_extended already, the
> caller can get the size of the buffer quite easy.
>
> Of course, if you *really* don't care the waste of size in AMD platform,
> it's ok for me. After all, in intel platform, either there is no extended
> information, or it will occupy all of them, so it really does not matter to
> me. But the (void*) issue should be resolved, I suspect.

Is it possible to change the internal infrastructure to deal with multiple
mc_info's ? The user (Dom0) will keep to see just one because the switch
happens underneath.

What does not fit into one mc_info will be put into the next.

In xen you will need some operations:
lowlevel: allocate, free, switch, read, write
highlevel: get and put

The mce code itself should just use the highlevel operations and also just
see one mc_info. The highlevel operations see as many mc_info as needed and
use the lowlevel operations which work on mc_info directly.

Does that make sense to you ?

> How about your option to the other patch?

Still need to have a look at it.

> Thanks
> --jyh
>
> >Christoph
> >
> >
> >--
> >---to satisfy European Law for business letters:
> >Advanced Micro Devices GmbH
> >Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
> >Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
> >Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
> >Registergericht Muenchen, HRB Nr. 43632



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

RE: [RFC] [PATCH 0/2] Some clean-up to MCA handling

[Jiang, Yunhong]

[-- Attachment #1: Type: text/plain, Size: 3146 bytes --]


>> It is not "easier" internalhandling. In fact, it makes no difference to
>> internal handling at all. The reason is: 1) In amd_f10.c, it will only
>> occupy 4 mc_msr,
>
>well, 4 in the generic handler plus 3 MSRs via mcinfo_extended which have
>been introduced in family10h.

So it means 16 -3 extra mc_msrs be wasted at least, for each mcinfo_extended :-)

>
>> while in Intel platform, it may occupy 32 mc_msr, that is
>> sure to cost extra space. The mc_info buffer is quite limited and can't be
>> expanded in run time, so reduce the size is quite important.
>> 2) sizeof(void *) is different in 64 hypervisor and 32 bit dom0. I'm not
>> sure if it is tested in compatibility mode, which might be confused.
>>
>> In fact, since we have mc_msrs included in mcinfo_extended already, the
>> caller can get the size of the buffer quite easy.
>>
>> Of course, if you *really* don't care the waste of size in AMD platform,
>> it's ok for me. After all, in intel platform, either there is no extended
>> information, or it will occupy all of them, so it really does not matter to
>> me. But the (void*) issue should be resolved, I suspect.
>
>Is it possible to change the internal infrastructure to deal with multiple
>mc_info's ? The user (Dom0) will keep to see just one because the switch
>happens underneath.

I think the size limitation happens on two side. Firstly, there is only 10 mc_info reserved in urgent queue, and 20 in non-urgent queue, and that queue size is not adjusted dynamical; Secondly, each mc_info contains 768 uint64_t.

>
>What does not fit into one mc_info will be put into the next.
>
>In xen you will need some operations:
>lowlevel: allocate, free, switch, read, write
>highlevel: get and put
>
>The mce code itself should just use the highlevel operations and also just
>see one mc_info. The highlevel operations see as many mc_info as needed and
>use the lowlevel operations which work on mc_info directly.
>
>Does that make sense to you ?

I suspect this will cause much more complex. Also will this cause trouble to ABI also, since the mc_info is defined in public?

As I have no data for MCE/CMCI trigger model in run time, maybe we can postpone this changes, unless some one raise this issue? 
Attached is the new patch, which does not change the interface anymore.

--jyh

>
>> How about your option to the other patch?
>
>Still need to have a look at it.
>
>> Thanks
>> --jyh
>>
>> >Christoph
>> >
>> >
>> >--
>> >---to satisfy European Law for business letters:
>> >Advanced Micro Devices GmbH
>> >Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
>> >Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
>> >Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
>> >Registergericht Muenchen, HRB Nr. 43632
>
>
>
>--
>---to satisfy European Law for business letters:
>Advanced Micro Devices GmbH
>Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
>Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
>Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
>Registergericht Muenchen, HRB Nr. 43632


[-- Attachment #2: mce_intel_gext.patch --]
[-- Type: application/octet-stream, Size: 8579 bytes --]

# HG changeset patch
# User Yunhong Jiang <yunhong.jiang@intel.com>
# Date 1271750467 -28800
# Node ID a8e9c9040f1a176ff1e8e64884ce5e679dea6827
# Parent  f1313dd68da4077ccd4f5530a7bc79d649bdffab
Change the method to get the extended MCA information.

Several changes to get the extended MCA information:
a) Use the x86_mcinfo_reserve in mcinfo_extended to reserve buffer from
   mc_info, instead of using the stack
b) For intel's extended MSR, we don't need write them one
   by one as the MSR are continous
c) We don't need enum mca_extinfo, since we can consider
   the extended MSR as either per bank, or global. Currently
   we add a hook in global data collection, and didn't call
   register intel_get_extended_msrs as callback. Later that
   hook can be replaced by cleaner way

    Signed-off-by: Jiang, Yunhong <yunhong.jiang@inte.com>

diff -r f1313dd68da4 -r a8e9c9040f1a xen/arch/x86/cpu/mcheck/amd_f10.c
--- a/xen/arch/x86/cpu/mcheck/amd_f10.c	Mon Apr 19 11:47:59 2010 +0100
+++ b/xen/arch/x86/cpu/mcheck/amd_f10.c	Tue Apr 20 16:01:07 2010 +0800
@@ -49,37 +49,43 @@
 #include "x86_mca.h"
 
 
-static enum mca_extinfo
+static struct mcinfo_extended *
 amd_f10_handler(struct mc_info *mi, uint16_t bank, uint64_t status)
 {
-	struct mcinfo_extended mc_ext;
+	struct mcinfo_extended *mc_ext;
 
 	/* Family 0x10 introduced additional MSR that belong to the
 	 * northbridge bank (4). */
 	if (mi == NULL || bank != 4)
-		return MCA_EXTINFO_IGNORED;
+		return NULL;
 
 	if (!(status & MCi_STATUS_VAL))
-		return MCA_EXTINFO_IGNORED;
+		return NULL;
 
 	if (!(status & MCi_STATUS_MISCV))
-		return MCA_EXTINFO_IGNORED;
+		return NULL;
 
-	memset(&mc_ext, 0, sizeof(mc_ext));
-	mc_ext.common.type = MC_TYPE_EXTENDED;
-	mc_ext.common.size = sizeof(mc_ext);
-	mc_ext.mc_msrs = 3;
+	mc_ext = x86_mcinfo_reserve(mi, sizeof(struct mcinfo_extended));
+	if (!mc_ext)
+	{
+		mi->flags |= MCINFO_FLAGS_UNCOMPLETE;
+		return NULL;
+	}
 
-	mc_ext.mc_msr[0].reg = MSR_F10_MC4_MISC1;
-	mc_ext.mc_msr[1].reg = MSR_F10_MC4_MISC2;
-	mc_ext.mc_msr[2].reg = MSR_F10_MC4_MISC3;
+	memset(mc_ext, 0, sizeof(mc_ext));
+	mc_ext->common.type = MC_TYPE_EXTENDED;
+	mc_ext->common.size = sizeof(mc_ext);
+	mc_ext->mc_msrs = 3;
 
-	mca_rdmsrl(MSR_F10_MC4_MISC1, mc_ext.mc_msr[0].value);
-	mca_rdmsrl(MSR_F10_MC4_MISC2, mc_ext.mc_msr[1].value);
-	mca_rdmsrl(MSR_F10_MC4_MISC3, mc_ext.mc_msr[2].value);
-	
-	x86_mcinfo_add(mi, &mc_ext);
-	return MCA_EXTINFO_LOCAL;
+	mc_ext->mc_msr[0].reg = MSR_F10_MC4_MISC1;
+	mc_ext->mc_msr[1].reg = MSR_F10_MC4_MISC2;
+	mc_ext->mc_msr[2].reg = MSR_F10_MC4_MISC3;
+
+	mca_rdmsrl(MSR_F10_MC4_MISC1, mc_ext->mc_msr[0].value);
+	mca_rdmsrl(MSR_F10_MC4_MISC2, mc_ext->mc_msr[1].value);
+	mca_rdmsrl(MSR_F10_MC4_MISC3, mc_ext->mc_msr[2].value);
+
+	return mc_ext;
 }
 
 /* AMD Family10 machine check */
diff -r f1313dd68da4 -r a8e9c9040f1a xen/arch/x86/cpu/mcheck/mce.c
--- a/xen/arch/x86/cpu/mcheck/mce.c	Mon Apr 19 11:47:59 2010 +0100
+++ b/xen/arch/x86/cpu/mcheck/mce.c	Tue Apr 20 16:01:07 2010 +0800
@@ -229,7 +229,6 @@ mctelem_cookie_t mcheck_mca_logout(enum 
 	mctelem_class_t which = MC_URGENT;	/* XXXgcc */
 	int errcnt = 0;
 	int i;
-	enum mca_extinfo cbret = MCA_EXTINFO_IGNORED;
 
 	mca_rdmsrl(MSR_IA32_MCG_STATUS, gstatus);
 	switch (who) {
@@ -291,6 +290,15 @@ mctelem_cookie_t mcheck_mca_logout(enum 
 				/* mc_info should at least hold up the global information */
 				ASSERT(mig);
 				mca_init_global(mc_flags, mig);
+				/* A hook here to get global extended msrs */
+				{
+					struct mcinfo_extended *intel_get_extended_msrs(
+					 struct mcinfo_global *mig, struct mc_info *mi);
+
+					 if (boot_cpu_data.x86_vendor ==
+					     X86_VENDOR_INTEL)
+						 intel_get_extended_msrs(mig, mci);
+				}
 			}
 		}
 
@@ -309,9 +317,8 @@ mctelem_cookie_t mcheck_mca_logout(enum 
 
 		mib = mca_init_bank(who, mci, i);
 
-		if (mc_callback_bank_extended && cbret != MCA_EXTINFO_GLOBAL) {
-			cbret = mc_callback_bank_extended(mci, i, status);
-		}
+		if (mc_callback_bank_extended)
+			mc_callback_bank_extended(mci, i, status);
 
 		/* By default, need_clear = 1 */
 		if (who != MCA_MCE_SCAN && need_clear)
diff -r f1313dd68da4 -r a8e9c9040f1a xen/arch/x86/cpu/mcheck/mce.h
--- a/xen/arch/x86/cpu/mcheck/mce.h	Mon Apr 19 11:47:59 2010 +0100
+++ b/xen/arch/x86/cpu/mcheck/mce.h	Tue Apr 20 16:01:07 2010 +0800
@@ -110,12 +110,6 @@ enum mca_source {
 	MCA_MCE_SCAN
 };
 
-enum mca_extinfo {
-	MCA_EXTINFO_LOCAL,
-	MCA_EXTINFO_GLOBAL,
-	MCA_EXTINFO_IGNORED
-};
-
 struct mca_summary {
 	uint32_t	errcnt;	/* number of banks with valid errors */
 	int		ripv;	/* meaningful on #MC */
@@ -157,7 +151,7 @@ typedef int (*mce_need_clearbank_t)(enum
 typedef int (*mce_need_clearbank_t)(enum mca_source who, u64 status);
 extern void mce_need_clearbank_register(mce_need_clearbank_t);
 
-typedef enum mca_extinfo (*x86_mce_callback_t)
+typedef struct mcinfo_extended *(*x86_mce_callback_t)
     (struct mc_info *, uint16_t, uint64_t);
 extern void x86_mce_callback_register(x86_mce_callback_t);
 
diff -r f1313dd68da4 -r a8e9c9040f1a xen/arch/x86/cpu/mcheck/mce_intel.c
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c	Mon Apr 19 11:47:59 2010 +0100
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c	Tue Apr 20 16:01:07 2010 +0800
@@ -154,52 +154,48 @@ static inline void intel_get_extended_ms
     if ( ext->mc_msrs < ARRAY_SIZE(ext->mc_msr)
          && msr < MSR_IA32_MCG_EAX + nr_intel_ext_msrs ) {
         ext->mc_msr[ext->mc_msrs].reg = msr;
-        mca_rdmsrl(msr, ext->mc_msr[ext->mc_msrs].value);
+        rdmsrl(msr, ext->mc_msr[ext->mc_msrs].value);
         ++ext->mc_msrs;
     }
 }
 
-static enum mca_extinfo
-intel_get_extended_msrs(struct mc_info *mci, uint16_t bank, uint64_t status)
-{
-    struct mcinfo_extended mc_ext;
-
-    if (mci == NULL || nr_intel_ext_msrs == 0 || !(status & MCG_STATUS_EIPV))
-        return MCA_EXTINFO_IGNORED;
+
+struct mcinfo_extended *
+intel_get_extended_msrs(struct mcinfo_global *mig, struct mc_info *mi)
+{
+    struct mcinfo_extended *mc_ext;
+    int i;
+
+    /*
+     * According to spec, processor _support_ 64 bit will always
+     * have MSR beyond IA32_MCG_MISC
+     */
+    if (!mi|| !mig || nr_intel_ext_msrs == 0 ||
+            !(mig->mc_gstatus & MCG_STATUS_EIPV))
+        return NULL;
+
+    mc_ext = x86_mcinfo_reserve(mi, sizeof(struct mcinfo_extended));
+    if (!mc_ext)
+    {
+        mi->flags |= MCINFO_FLAGS_UNCOMPLETE;
+        return NULL;
+    }
 
     /* this function will called when CAP(9).MCG_EXT_P = 1 */
     memset(&mc_ext, 0, sizeof(struct mcinfo_extended));
-    mc_ext.common.type = MC_TYPE_EXTENDED;
-    mc_ext.common.size = sizeof(mc_ext);
-
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_EAX);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_EBX);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_ECX);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_EDX);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_ESI);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_EDI);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_EBP);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_ESP);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_EFLAGS);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_EIP);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_MISC);
+    mc_ext->common.type = MC_TYPE_EXTENDED;
+    mc_ext->common.size = sizeof(struct mcinfo_extended);
+
+    for (i = MSR_IA32_MCG_EAX; i <= MSR_IA32_MCG_MISC; i++)
+        intel_get_extended_msr(mc_ext, i);
 
 #ifdef __x86_64__
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R8);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R9);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R10);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R11);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R12);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R13);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R14);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R15);
+    for (i = MSR_IA32_MCG_R8; i <= MSR_IA32_MCG_R15; i++)
+        intel_get_extended_msr(mc_ext, i);
 #endif
 
-    x86_mcinfo_add(mci, &mc_ext);
-
-    return MCA_EXTINFO_GLOBAL;
-}
-
+    return mc_ext;
+}
 
 static void intel_UCR_handler(struct mcinfo_bank *bank,
              struct mcinfo_global *global,
@@ -959,7 +955,6 @@ enum mcheck_type intel_mcheck_init(struc
 
     /* machine check is available */
     x86_mce_vector_register(intel_machine_check);
-    x86_mce_callback_register(intel_get_extended_msrs);
     mce_recoverable_register(intel_recoverable_scan);
     mce_need_clearbank_register(intel_need_clearbank_scan);
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [RFC] [PATCH 0/2] Some clean-up to MCA handling

[Christoph Egger]

On Tuesday 20 April 2010 10:06:12 Jiang, Yunhong wrote:
> >> It is not "easier" internalhandling. In fact, it makes no difference to
> >> internal handling at all. The reason is: 1) In amd_f10.c, it will only
> >> occupy 4 mc_msr,
> >
> >well, 4 in the generic handler plus 3 MSRs via mcinfo_extended which have
> >been introduced in family10h.
>
> So it means 16 -3 extra mc_msrs be wasted at least, for each
> mcinfo_extended :-)

I consider this as 'reserved space for the future' :-)

>
> >> while in Intel platform, it may occupy 32 mc_msr, that is
> >> sure to cost extra space. The mc_info buffer is quite limited and can't
> >> be expanded in run time, so reduce the size is quite important.
> >> 2) sizeof(void *) is different in 64 hypervisor and 32 bit dom0. I'm not
> >> sure if it is tested in compatibility mode, which might be confused.
> >>
> >> In fact, since we have mc_msrs included in mcinfo_extended already, the
> >> caller can get the size of the buffer quite easy.
> >>
> >> Of course, if you *really* don't care the waste of size in AMD platform,
> >> it's ok for me. After all, in intel platform, either there is no
> >> extended information, or it will occupy all of them, so it really does
> >> not matter to me. But the (void*) issue should be resolved, I suspect.
> >
> >Is it possible to change the internal infrastructure to deal with multiple
> >mc_info's ? The user (Dom0) will keep to see just one because the switch
> >happens underneath.
>
> I think the size limitation happens on two side. Firstly, there is only 10
> mc_info reserved in urgent queue, and 20 in non-urgent queue, and that
> queue size is not adjusted dynamical; Secondly, each mc_info contains 768
> uint64_t.

The API allows the user (Dom0) to fetch a mc_info again and again. So what
didn't fit into the first can be placed into the next.
The reason to have it static is that the MCE implementation in both xen and in 
Dom0 can be lockless (xmalloc/xfree use locks).

>
> >What does not fit into one mc_info will be put into the next.
> >
> >In xen you will need some operations:
> >lowlevel: allocate, free, switch, read, write
> >highlevel: get and put
> >
> >The mce code itself should just use the highlevel operations and also just
> >see one mc_info. The highlevel operations see as many mc_info as needed
> > and use the lowlevel operations which work on mc_info directly.
> >
> >Does that make sense to you ?
>
> I suspect this will cause much more complex. Also will this cause trouble
> to ABI also, since the mc_info is defined in public?

Yes, that is to make mc_info look dynamic from the internal side.

> As I have no data for MCE/CMCI trigger model in run time, maybe we can
> postpone this changes, unless some one raise this issue?

You can ask back within your company to get a known broken but reasonable 
usable hardware (i.e. CPU L2 cache produces certain errors or DIMM plugged
in slot 3 produces certain errors).
Then you know what error types you can expect on such a machine and how
to deal with.

> Attached is the new patch, which does not change the interface anymore.

This patch is good.

>
> --jyh
>
> >> How about your option to the other patch?
> >
> >Still need to have a look at it.
> >
> >> Thanks
> >> --jyh
> >>
> >> >Christoph
> >> >
> >> >
> >> >--
> >> >---to satisfy European Law for business letters:
> >> >Advanced Micro Devices GmbH
> >> >Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
> >> >Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
> >> >Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
> >> >Registergericht Muenchen, HRB Nr. 43632
> >
> >--
> >---to satisfy European Law for business letters:
> >Advanced Micro Devices GmbH
> >Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
> >Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
> >Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
> >Registergericht Muenchen, HRB Nr. 43632



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

RE: [RFC] [PATCH 0/2] Some clean-up to MCA handling

[Jiang, Yunhong]

>> As I have no data for MCE/CMCI trigger model in run time, maybe we can
>> postpone this changes, unless some one raise this issue?
>
>You can ask back within your company to get a known broken but reasonable
>usable hardware (i.e. CPU L2 cache produces certain errors or DIMM plugged
>in slot 3 produces certain errors).
>Then you know what error types you can expect on such a machine and how
>to deal with.

we can trigger #MCE in our platform, and we can find some DIMM with one bit broken if needed, but the run-time static data is something different IMO.

>
>> Attached is the new patch, which does not change the interface anymore.
>
>This patch is good.

Thanks for review and comments. I will re-submit it after comments on the other patch.
BTW, do you know if Frank still working on MCA?

--jyh

>
>>
>> --jyh
>>
>> >> How about your option to the other patch?
>> >
>> >Still need to have a look at it.
>> >
>> >> Thanks
>> >> --jyh
>> >>
>> >> >Christoph
>> >> >
>> >> >
>> >> >--
>> >> >---to satisfy European Law for business letters:
>> >> >Advanced Micro Devices GmbH
>> >> >Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
>> >> >Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
>> >> >Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
>> >> >Registergericht Muenchen, HRB Nr. 43632
>> >
>> >--
>> >---to satisfy European Law for business letters:
>> >Advanced Micro Devices GmbH
>> >Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
>> >Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
>> >Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
>> >Registergericht Muenchen, HRB Nr. 43632
>
>
>
>--
>---to satisfy European Law for business letters:
>Advanced Micro Devices GmbH
>Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
>Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
>Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
>Registergericht Muenchen, HRB Nr. 43632

RE: [RFC] [PATCH 0/2] Some clean-up to MCA handling

[Jiang, Yunhong]

[-- Attachment #1: Type: text/plain, Size: 3894 bytes --]

Keir, can you please check-in this patch?

This patch has been acked by Egger before (" This patch is good" at http://lists.xensource.com/archives/html/xen-devel/2010-04/msg00990.html) . Originaly I want to re-submit after comments with the other patch, but since I decide to split the other patch , to make it more easy to review, can you please check-in this firslty?

Thanks
--jyh


-----Original Message-----
From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Jiang, Yunhong
Sent: Tuesday, April 20, 2010 4:06 PM
To: Christoph Egger; Frank Van Der Linden
Cc: xen-devel@lists.xensource.com; Keir Fraser
Subject: [Xen-devel] RE: [RFC] [PATCH 0/2] Some clean-up to MCA handling


>> It is not "easier" internalhandling. In fact, it makes no difference to
>> internal handling at all. The reason is: 1) In amd_f10.c, it will only
>> occupy 4 mc_msr,
>
>well, 4 in the generic handler plus 3 MSRs via mcinfo_extended which have
>been introduced in family10h.

So it means 16 -3 extra mc_msrs be wasted at least, for each mcinfo_extended :-)

>
>> while in Intel platform, it may occupy 32 mc_msr, that is
>> sure to cost extra space. The mc_info buffer is quite limited and can't be
>> expanded in run time, so reduce the size is quite important.
>> 2) sizeof(void *) is different in 64 hypervisor and 32 bit dom0. I'm not
>> sure if it is tested in compatibility mode, which might be confused.
>>
>> In fact, since we have mc_msrs included in mcinfo_extended already, the
>> caller can get the size of the buffer quite easy.
>>
>> Of course, if you *really* don't care the waste of size in AMD platform,
>> it's ok for me. After all, in intel platform, either there is no extended
>> information, or it will occupy all of them, so it really does not matter to
>> me. But the (void*) issue should be resolved, I suspect.
>
>Is it possible to change the internal infrastructure to deal with multiple
>mc_info's ? The user (Dom0) will keep to see just one because the switch
>happens underneath.

I think the size limitation happens on two side. Firstly, there is only 10 mc_info reserved in urgent queue, and 20 in non-urgent queue, and that queue size is not adjusted dynamical; Secondly, each mc_info contains 768 uint64_t.

>
>What does not fit into one mc_info will be put into the next.
>
>In xen you will need some operations:
>lowlevel: allocate, free, switch, read, write
>highlevel: get and put
>
>The mce code itself should just use the highlevel operations and also just
>see one mc_info. The highlevel operations see as many mc_info as needed and
>use the lowlevel operations which work on mc_info directly.
>
>Does that make sense to you ?

I suspect this will cause much more complex. Also will this cause trouble to ABI also, since the mc_info is defined in public?

As I have no data for MCE/CMCI trigger model in run time, maybe we can postpone this changes, unless some one raise this issue? 
Attached is the new patch, which does not change the interface anymore.

--jyh

>
>> How about your option to the other patch?
>
>Still need to have a look at it.
>
>> Thanks
>> --jyh
>>
>> >Christoph
>> >
>> >
>> >--
>> >---to satisfy European Law for business letters:
>> >Advanced Micro Devices GmbH
>> >Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
>> >Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
>> >Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
>> >Registergericht Muenchen, HRB Nr. 43632
>
>
>
>--
>---to satisfy European Law for business letters:
>Advanced Micro Devices GmbH
>Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
>Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
>Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
>Registergericht Muenchen, HRB Nr. 43632


[-- Attachment #2: mce_intel_gext.patch --]
[-- Type: application/octet-stream, Size: 8579 bytes --]

# HG changeset patch
# User Yunhong Jiang <yunhong.jiang@intel.com>
# Date 1271750467 -28800
# Node ID a8e9c9040f1a176ff1e8e64884ce5e679dea6827
# Parent  f1313dd68da4077ccd4f5530a7bc79d649bdffab
Change the method to get the extended MCA information.

Several changes to get the extended MCA information:
a) Use the x86_mcinfo_reserve in mcinfo_extended to reserve buffer from
   mc_info, instead of using the stack
b) For intel's extended MSR, we don't need write them one
   by one as the MSR are continous
c) We don't need enum mca_extinfo, since we can consider
   the extended MSR as either per bank, or global. Currently
   we add a hook in global data collection, and didn't call
   register intel_get_extended_msrs as callback. Later that
   hook can be replaced by cleaner way

    Signed-off-by: Jiang, Yunhong <yunhong.jiang@inte.com>

diff -r f1313dd68da4 -r a8e9c9040f1a xen/arch/x86/cpu/mcheck/amd_f10.c
--- a/xen/arch/x86/cpu/mcheck/amd_f10.c	Mon Apr 19 11:47:59 2010 +0100
+++ b/xen/arch/x86/cpu/mcheck/amd_f10.c	Tue Apr 20 16:01:07 2010 +0800
@@ -49,37 +49,43 @@
 #include "x86_mca.h"
 
 
-static enum mca_extinfo
+static struct mcinfo_extended *
 amd_f10_handler(struct mc_info *mi, uint16_t bank, uint64_t status)
 {
-	struct mcinfo_extended mc_ext;
+	struct mcinfo_extended *mc_ext;
 
 	/* Family 0x10 introduced additional MSR that belong to the
 	 * northbridge bank (4). */
 	if (mi == NULL || bank != 4)
-		return MCA_EXTINFO_IGNORED;
+		return NULL;
 
 	if (!(status & MCi_STATUS_VAL))
-		return MCA_EXTINFO_IGNORED;
+		return NULL;
 
 	if (!(status & MCi_STATUS_MISCV))
-		return MCA_EXTINFO_IGNORED;
+		return NULL;
 
-	memset(&mc_ext, 0, sizeof(mc_ext));
-	mc_ext.common.type = MC_TYPE_EXTENDED;
-	mc_ext.common.size = sizeof(mc_ext);
-	mc_ext.mc_msrs = 3;
+	mc_ext = x86_mcinfo_reserve(mi, sizeof(struct mcinfo_extended));
+	if (!mc_ext)
+	{
+		mi->flags |= MCINFO_FLAGS_UNCOMPLETE;
+		return NULL;
+	}
 
-	mc_ext.mc_msr[0].reg = MSR_F10_MC4_MISC1;
-	mc_ext.mc_msr[1].reg = MSR_F10_MC4_MISC2;
-	mc_ext.mc_msr[2].reg = MSR_F10_MC4_MISC3;
+	memset(mc_ext, 0, sizeof(mc_ext));
+	mc_ext->common.type = MC_TYPE_EXTENDED;
+	mc_ext->common.size = sizeof(mc_ext);
+	mc_ext->mc_msrs = 3;
 
-	mca_rdmsrl(MSR_F10_MC4_MISC1, mc_ext.mc_msr[0].value);
-	mca_rdmsrl(MSR_F10_MC4_MISC2, mc_ext.mc_msr[1].value);
-	mca_rdmsrl(MSR_F10_MC4_MISC3, mc_ext.mc_msr[2].value);
-	
-	x86_mcinfo_add(mi, &mc_ext);
-	return MCA_EXTINFO_LOCAL;
+	mc_ext->mc_msr[0].reg = MSR_F10_MC4_MISC1;
+	mc_ext->mc_msr[1].reg = MSR_F10_MC4_MISC2;
+	mc_ext->mc_msr[2].reg = MSR_F10_MC4_MISC3;
+
+	mca_rdmsrl(MSR_F10_MC4_MISC1, mc_ext->mc_msr[0].value);
+	mca_rdmsrl(MSR_F10_MC4_MISC2, mc_ext->mc_msr[1].value);
+	mca_rdmsrl(MSR_F10_MC4_MISC3, mc_ext->mc_msr[2].value);
+
+	return mc_ext;
 }
 
 /* AMD Family10 machine check */
diff -r f1313dd68da4 -r a8e9c9040f1a xen/arch/x86/cpu/mcheck/mce.c
--- a/xen/arch/x86/cpu/mcheck/mce.c	Mon Apr 19 11:47:59 2010 +0100
+++ b/xen/arch/x86/cpu/mcheck/mce.c	Tue Apr 20 16:01:07 2010 +0800
@@ -229,7 +229,6 @@ mctelem_cookie_t mcheck_mca_logout(enum 
 	mctelem_class_t which = MC_URGENT;	/* XXXgcc */
 	int errcnt = 0;
 	int i;
-	enum mca_extinfo cbret = MCA_EXTINFO_IGNORED;
 
 	mca_rdmsrl(MSR_IA32_MCG_STATUS, gstatus);
 	switch (who) {
@@ -291,6 +290,15 @@ mctelem_cookie_t mcheck_mca_logout(enum 
 				/* mc_info should at least hold up the global information */
 				ASSERT(mig);
 				mca_init_global(mc_flags, mig);
+				/* A hook here to get global extended msrs */
+				{
+					struct mcinfo_extended *intel_get_extended_msrs(
+					 struct mcinfo_global *mig, struct mc_info *mi);
+
+					 if (boot_cpu_data.x86_vendor ==
+					     X86_VENDOR_INTEL)
+						 intel_get_extended_msrs(mig, mci);
+				}
 			}
 		}
 
@@ -309,9 +317,8 @@ mctelem_cookie_t mcheck_mca_logout(enum 
 
 		mib = mca_init_bank(who, mci, i);
 
-		if (mc_callback_bank_extended && cbret != MCA_EXTINFO_GLOBAL) {
-			cbret = mc_callback_bank_extended(mci, i, status);
-		}
+		if (mc_callback_bank_extended)
+			mc_callback_bank_extended(mci, i, status);
 
 		/* By default, need_clear = 1 */
 		if (who != MCA_MCE_SCAN && need_clear)
diff -r f1313dd68da4 -r a8e9c9040f1a xen/arch/x86/cpu/mcheck/mce.h
--- a/xen/arch/x86/cpu/mcheck/mce.h	Mon Apr 19 11:47:59 2010 +0100
+++ b/xen/arch/x86/cpu/mcheck/mce.h	Tue Apr 20 16:01:07 2010 +0800
@@ -110,12 +110,6 @@ enum mca_source {
 	MCA_MCE_SCAN
 };
 
-enum mca_extinfo {
-	MCA_EXTINFO_LOCAL,
-	MCA_EXTINFO_GLOBAL,
-	MCA_EXTINFO_IGNORED
-};
-
 struct mca_summary {
 	uint32_t	errcnt;	/* number of banks with valid errors */
 	int		ripv;	/* meaningful on #MC */
@@ -157,7 +151,7 @@ typedef int (*mce_need_clearbank_t)(enum
 typedef int (*mce_need_clearbank_t)(enum mca_source who, u64 status);
 extern void mce_need_clearbank_register(mce_need_clearbank_t);
 
-typedef enum mca_extinfo (*x86_mce_callback_t)
+typedef struct mcinfo_extended *(*x86_mce_callback_t)
     (struct mc_info *, uint16_t, uint64_t);
 extern void x86_mce_callback_register(x86_mce_callback_t);
 
diff -r f1313dd68da4 -r a8e9c9040f1a xen/arch/x86/cpu/mcheck/mce_intel.c
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c	Mon Apr 19 11:47:59 2010 +0100
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c	Tue Apr 20 16:01:07 2010 +0800
@@ -154,52 +154,48 @@ static inline void intel_get_extended_ms
     if ( ext->mc_msrs < ARRAY_SIZE(ext->mc_msr)
          && msr < MSR_IA32_MCG_EAX + nr_intel_ext_msrs ) {
         ext->mc_msr[ext->mc_msrs].reg = msr;
-        mca_rdmsrl(msr, ext->mc_msr[ext->mc_msrs].value);
+        rdmsrl(msr, ext->mc_msr[ext->mc_msrs].value);
         ++ext->mc_msrs;
     }
 }
 
-static enum mca_extinfo
-intel_get_extended_msrs(struct mc_info *mci, uint16_t bank, uint64_t status)
-{
-    struct mcinfo_extended mc_ext;
-
-    if (mci == NULL || nr_intel_ext_msrs == 0 || !(status & MCG_STATUS_EIPV))
-        return MCA_EXTINFO_IGNORED;
+
+struct mcinfo_extended *
+intel_get_extended_msrs(struct mcinfo_global *mig, struct mc_info *mi)
+{
+    struct mcinfo_extended *mc_ext;
+    int i;
+
+    /*
+     * According to spec, processor _support_ 64 bit will always
+     * have MSR beyond IA32_MCG_MISC
+     */
+    if (!mi|| !mig || nr_intel_ext_msrs == 0 ||
+            !(mig->mc_gstatus & MCG_STATUS_EIPV))
+        return NULL;
+
+    mc_ext = x86_mcinfo_reserve(mi, sizeof(struct mcinfo_extended));
+    if (!mc_ext)
+    {
+        mi->flags |= MCINFO_FLAGS_UNCOMPLETE;
+        return NULL;
+    }
 
     /* this function will called when CAP(9).MCG_EXT_P = 1 */
     memset(&mc_ext, 0, sizeof(struct mcinfo_extended));
-    mc_ext.common.type = MC_TYPE_EXTENDED;
-    mc_ext.common.size = sizeof(mc_ext);
-
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_EAX);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_EBX);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_ECX);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_EDX);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_ESI);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_EDI);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_EBP);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_ESP);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_EFLAGS);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_EIP);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_MISC);
+    mc_ext->common.type = MC_TYPE_EXTENDED;
+    mc_ext->common.size = sizeof(struct mcinfo_extended);
+
+    for (i = MSR_IA32_MCG_EAX; i <= MSR_IA32_MCG_MISC; i++)
+        intel_get_extended_msr(mc_ext, i);
 
 #ifdef __x86_64__
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R8);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R9);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R10);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R11);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R12);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R13);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R14);
-    intel_get_extended_msr(&mc_ext, MSR_IA32_MCG_R15);
+    for (i = MSR_IA32_MCG_R8; i <= MSR_IA32_MCG_R15; i++)
+        intel_get_extended_msr(mc_ext, i);
 #endif
 
-    x86_mcinfo_add(mci, &mc_ext);
-
-    return MCA_EXTINFO_GLOBAL;
-}
-
+    return mc_ext;
+}
 
 static void intel_UCR_handler(struct mcinfo_bank *bank,
              struct mcinfo_global *global,
@@ -959,7 +955,6 @@ enum mcheck_type intel_mcheck_init(struc
 
     /* machine check is available */
     x86_mce_vector_register(intel_machine_check);
-    x86_mce_callback_register(intel_get_extended_msrs);
     mce_recoverable_register(intel_recoverable_scan);
     mce_need_clearbank_register(intel_need_clearbank_scan);
 

[-- Attachment #3: ATT00001..txt --]
[-- Type: text/plain, Size: 142 bytes --]

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

[-- Attachment #4: Type: text/plain, Size: 138 bytes --]

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