Provide for up to 16/32 on (32/64-bit) extended MCE MSRs, and use
actually existing extended MSRs on 64-bits that were inaccessible so
far.
Signed-off-by: Jan Beulich <jbeulich@novell.com>
--- 2009-03-27.orig/xen/arch/x86/cpu/mcheck/mce_intel.c 2009-03-27
16:43:26.000000000 +0100
+++ 2009-03-27/xen/arch/x86/cpu/mcheck/mce_intel.c 2009-03-27 12:06:52.000000000
+0100
@@ -117,6 +117,16 @@ static void intel_init_thermal(struct cp
}
#endif /* CONFIG_X86_MCE_THERMAL */
+static inline void intel_get_extended_msr(struct mcinfo_extended *ext, u32 msr)
+{
+ 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;
+ 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)
{
@@ -129,30 +139,29 @@ intel_get_extended_msrs(struct mc_info *
memset(&mc_ext, 0, sizeof(struct mcinfo_extended));
mc_ext.common.type = MC_TYPE_EXTENDED;
mc_ext.common.size = sizeof(mc_ext);
- mc_ext.mc_msrs = 10;
- mc_ext.mc_msr[0].reg = MSR_IA32_MCG_EAX;
- rdmsrl(MSR_IA32_MCG_EAX, mc_ext.mc_msr[0].value);
- mc_ext.mc_msr[1].reg = MSR_IA32_MCG_EBX;
- rdmsrl(MSR_IA32_MCG_EBX, mc_ext.mc_msr[1].value);
- mc_ext.mc_msr[2].reg = MSR_IA32_MCG_ECX;
- rdmsrl(MSR_IA32_MCG_ECX, mc_ext.mc_msr[2].value);
-
- mc_ext.mc_msr[3].reg = MSR_IA32_MCG_EDX;
- rdmsrl(MSR_IA32_MCG_EDX, mc_ext.mc_msr[3].value);
- mc_ext.mc_msr[4].reg = MSR_IA32_MCG_ESI;
- rdmsrl(MSR_IA32_MCG_ESI, mc_ext.mc_msr[4].value);
- mc_ext.mc_msr[5].reg = MSR_IA32_MCG_EDI;
- rdmsrl(MSR_IA32_MCG_EDI, mc_ext.mc_msr[5].value);
-
- mc_ext.mc_msr[6].reg = MSR_IA32_MCG_EBP;
- rdmsrl(MSR_IA32_MCG_EBP, mc_ext.mc_msr[6].value);
- mc_ext.mc_msr[7].reg = MSR_IA32_MCG_ESP;
- rdmsrl(MSR_IA32_MCG_ESP, mc_ext.mc_msr[7].value);
- mc_ext.mc_msr[8].reg = MSR_IA32_MCG_EFLAGS;
- rdmsrl(MSR_IA32_MCG_EFLAGS, mc_ext.mc_msr[8].value);
- mc_ext.mc_msr[9].reg = MSR_IA32_MCG_EIP;
- rdmsrl(MSR_IA32_MCG_EIP, mc_ext.mc_msr[9].value);
+ 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);
+
+#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);
+#endif
x86_mcinfo_add(mci, &mc_ext);
--- 2009-03-27.orig/xen/include/asm-x86/msr-index.h 2009-03-27
16:43:26.000000000 +0100
+++ 2009-03-27/xen/include/asm-x86/msr-index.h 2009-03-27 13:18:04.000000000
+0100
@@ -326,7 +326,15 @@
#define MSR_IA32_MCG_ESP 0x00000187
#define MSR_IA32_MCG_EFLAGS 0x00000188
#define MSR_IA32_MCG_EIP 0x00000189
-#define MSR_IA32_MCG_RESERVED 0x0000018a
+#define MSR_IA32_MCG_MISC 0x0000018a
+#define MSR_IA32_MCG_R8 0x00000190
+#define MSR_IA32_MCG_R9 0x00000191
+#define MSR_IA32_MCG_R10 0x00000192
+#define MSR_IA32_MCG_R11 0x00000193
+#define MSR_IA32_MCG_R12 0x00000194
+#define MSR_IA32_MCG_R13 0x00000195
+#define MSR_IA32_MCG_R14 0x00000196
+#define MSR_IA32_MCG_R15 0x00000197
/* Pentium IV performance counter MSRs */
#define MSR_P4_BPU_PERFCTR0 0x00000300
--- 2009-03-27.orig/xen/include/public/arch-x86/xen-mca.h 2009-03-27
16:47:30.000000000 +0100
+++ 2009-03-27/xen/include/public/arch-x86/xen-mca.h 2009-03-27
16:48:35.000000000 +0100
@@ -166,11 +166,11 @@ struct mcinfo_extended {
uint32_t mc_msrs; /* Number of msr with valid values. */
/*
- * Currently Intel extended MSR (32/64) including all gp registers
- * and E(R)DI, E(R)BP, E(R)SP, E(R)FLAGS, E(R)IP, E(R)MISC, only 10
- * of them might be useful. So expend this array to 10.
- */
- struct mcinfo_msr mc_msr[10];
+ * Currently Intel extended MSR (32/64) include all gp registers
+ * and E(R)FLAGS, E(R)IP, E(R)MISC, up to 11/19 of them might be
+ * useful at present. So expand this array to 16/32 to leave room.
+ */
+ struct mcinfo_msr mc_msr[sizeof(void *) * 4];
};
/* Recovery Action flags. Giving recovery result information to DOM0 */
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
This patch looks very good to me. Just one comment at the bottom. Christoph On Friday 27 March 2009 17:34:07 Jan Beulich wrote:> Provide for up to 16/32 on (32/64-bit) extended MCE MSRs, and use > actually existing extended MSRs on 64-bits that were inaccessible so > far. > > Signed-off-by: Jan Beulich <jbeulich@novell.com> > > --- 2009-03-27.orig/xen/arch/x86/cpu/mcheck/mce_intel.c 2009-03-27 > 16:43:26.000000000 +0100 +++ > 2009-03-27/xen/arch/x86/cpu/mcheck/mce_intel.c 2009-03-27 > 12:06:52.000000000 +0100 @@ -117,6 +117,16 @@ static void > intel_init_thermal(struct cp > } > #endif /* CONFIG_X86_MCE_THERMAL */ > > +static inline void intel_get_extended_msr(struct mcinfo_extended *ext, u32 > msr) +{ > + 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; > + 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) { > @@ -129,30 +139,29 @@ intel_get_extended_msrs(struct mc_info * > memset(&mc_ext, 0, sizeof(struct mcinfo_extended)); > mc_ext.common.type = MC_TYPE_EXTENDED; > mc_ext.common.size = sizeof(mc_ext); > - mc_ext.mc_msrs = 10; > > - mc_ext.mc_msr[0].reg = MSR_IA32_MCG_EAX; > - rdmsrl(MSR_IA32_MCG_EAX, mc_ext.mc_msr[0].value); > - mc_ext.mc_msr[1].reg = MSR_IA32_MCG_EBX; > - rdmsrl(MSR_IA32_MCG_EBX, mc_ext.mc_msr[1].value); > - mc_ext.mc_msr[2].reg = MSR_IA32_MCG_ECX; > - rdmsrl(MSR_IA32_MCG_ECX, mc_ext.mc_msr[2].value); > - > - mc_ext.mc_msr[3].reg = MSR_IA32_MCG_EDX; > - rdmsrl(MSR_IA32_MCG_EDX, mc_ext.mc_msr[3].value); > - mc_ext.mc_msr[4].reg = MSR_IA32_MCG_ESI; > - rdmsrl(MSR_IA32_MCG_ESI, mc_ext.mc_msr[4].value); > - mc_ext.mc_msr[5].reg = MSR_IA32_MCG_EDI; > - rdmsrl(MSR_IA32_MCG_EDI, mc_ext.mc_msr[5].value); > - > - mc_ext.mc_msr[6].reg = MSR_IA32_MCG_EBP; > - rdmsrl(MSR_IA32_MCG_EBP, mc_ext.mc_msr[6].value); > - mc_ext.mc_msr[7].reg = MSR_IA32_MCG_ESP; > - rdmsrl(MSR_IA32_MCG_ESP, mc_ext.mc_msr[7].value); > - mc_ext.mc_msr[8].reg = MSR_IA32_MCG_EFLAGS; > - rdmsrl(MSR_IA32_MCG_EFLAGS, mc_ext.mc_msr[8].value); > - mc_ext.mc_msr[9].reg = MSR_IA32_MCG_EIP; > - rdmsrl(MSR_IA32_MCG_EIP, mc_ext.mc_msr[9].value); > + 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); > + > +#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); > +#endif > > x86_mcinfo_add(mci, &mc_ext); > > --- 2009-03-27.orig/xen/include/asm-x86/msr-index.h 2009-03-27 > 16:43:26.000000000 +0100 +++ > 2009-03-27/xen/include/asm-x86/msr-index.h 2009-03-27 13:18:04.000000000 > +0100 @@ -326,7 +326,15 @@ > #define MSR_IA32_MCG_ESP 0x00000187 > #define MSR_IA32_MCG_EFLAGS 0x00000188 > #define MSR_IA32_MCG_EIP 0x00000189 > -#define MSR_IA32_MCG_RESERVED 0x0000018a > +#define MSR_IA32_MCG_MISC 0x0000018a > +#define MSR_IA32_MCG_R8 0x00000190 > +#define MSR_IA32_MCG_R9 0x00000191 > +#define MSR_IA32_MCG_R10 0x00000192 > +#define MSR_IA32_MCG_R11 0x00000193 > +#define MSR_IA32_MCG_R12 0x00000194 > +#define MSR_IA32_MCG_R13 0x00000195 > +#define MSR_IA32_MCG_R14 0x00000196 > +#define MSR_IA32_MCG_R15 0x00000197 > > /* Pentium IV performance counter MSRs */ > #define MSR_P4_BPU_PERFCTR0 0x00000300 > --- 2009-03-27.orig/xen/include/public/arch-x86/xen-mca.h 2009-03-27 > 16:47:30.000000000 +0100 +++ > 2009-03-27/xen/include/public/arch-x86/xen-mca.h 2009-03-27 > 16:48:35.000000000 +0100 @@ -166,11 +166,11 @@ struct mcinfo_extended { > > uint32_t mc_msrs; /* Number of msr with valid values. */ > /* > - * Currently Intel extended MSR (32/64) including all gp registers > - * and E(R)DI, E(R)BP, E(R)SP, E(R)FLAGS, E(R)IP, E(R)MISC, only 10 > - * of them might be useful. So expend this array to 10. > - */ > - struct mcinfo_msr mc_msr[10]; > + * Currently Intel extended MSR (32/64) include all gp registers > + * and E(R)FLAGS, E(R)IP, E(R)MISC, up to 11/19 of them might be > + * useful at present. So expand this array to 16/32 to leave room. > + */ > + struct mcinfo_msr mc_msr[sizeof(void *) * 4];Please make this a fixed sized array. There are users like Oracle who run a 32bit PAE Dom0 on a 64bit Xen ...> }; > > /* Recovery Action flags. Giving recovery result information to DOM0 */-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> Christoph Egger <Christoph.Egger@amd.com> 27.03.09 17:49 >>> >On Friday 27 March 2009 17:34:07 Jan Beulich wrote: >> - * Currently Intel extended MSR (32/64) including all gp registers >> - * and E(R)DI, E(R)BP, E(R)SP, E(R)FLAGS, E(R)IP, E(R)MISC, only 10 >> - * of them might be useful. So expend this array to 10. >> - */ >> - struct mcinfo_msr mc_msr[10]; >> + * Currently Intel extended MSR (32/64) include all gp registers >> + * and E(R)FLAGS, E(R)IP, E(R)MISC, up to 11/19 of them might be >> + * useful at present. So expand this array to 16/32 to leave room. >> + */ >> + struct mcinfo_msr mc_msr[sizeof(void *) * 4]; > >Please make this a fixed sized array. There are users like Oracle who run >a 32bit PAE Dom0 on a 64bit Xen ...And you expect a 32-bit kernel to be able to make sense of the MSRs corresponding to 64-bit-only registers? But you remind me that I failed to handle the difference in size of that array for 32-on-64 - I really need to check why the structure layout checking logic didn''t catch the difference in size. Oh, right, sizeof(void *) needs special treatment (and I really don#t want to sue sizeof(long) here due to the implied dependency on the OS ABI). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Friday 27 March 2009 18:08:00 Jan Beulich wrote:> >>> Christoph Egger <Christoph.Egger@amd.com> 27.03.09 17:49 >>> > > > >On Friday 27 March 2009 17:34:07 Jan Beulich wrote: > >> - * Currently Intel extended MSR (32/64) including all gp registers > >> - * and E(R)DI, E(R)BP, E(R)SP, E(R)FLAGS, E(R)IP, E(R)MISC, only 10 > >> - * of them might be useful. So expend this array to 10. > >> - */ > >> - struct mcinfo_msr mc_msr[10]; > >> + * Currently Intel extended MSR (32/64) include all gp registers > >> + * and E(R)FLAGS, E(R)IP, E(R)MISC, up to 11/19 of them might be > >> + * useful at present. So expand this array to 16/32 to leave room. > >> + */ > >> + struct mcinfo_msr mc_msr[sizeof(void *) * 4]; > > > >Please make this a fixed sized array. There are users like Oracle who run > >a 32bit PAE Dom0 on a 64bit Xen ... > > And you expect a 32-bit kernel to be able to make sense of the MSRs > corresponding to 64-bit-only registers? > > But you remind me that I failed to handle the difference in size of that > array for 32-on-64 - I really need to check why the structure layout > checking logic didn''t catch the difference in size. Oh, right, sizeof(void > *) needs special treatment (and I really don#t want to sue sizeof(long) > here due to the implied dependency on the OS ABI).Having the same size for both 32bit and 64bit makes the Dom0 code simpler to use it. Taking care about the different sizes would enforce either hackish code or two different implementations. That''s not acceptable IMO. Christoph -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich wrote:> > And you expect a 32-bit kernel to be able to make sense of the MSRs > corresponding to 64-bit-only registers?The other fixes that you did for the MC hypercalls all only apply to the case of a 32bit dom0 on 64bit Xen too, so if we care about that case, we should care about this case too, I guess. I was aware of some of the alignment issues, but basically forgot about them, since we do 64bit dom0 on 64bit Xen only. - Frank _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> Christoph Egger <Christoph.Egger@amd.com> 27.03.09 18:21 >>> >On Friday 27 March 2009 18:08:00 Jan Beulich wrote: >> >Please make this a fixed sized array. There are users like Oracle who run >> >a 32bit PAE Dom0 on a 64bit Xen ... >> >> And you expect a 32-bit kernel to be able to make sense of the MSRs >> corresponding to 64-bit-only registers? >> >> But you remind me that I failed to handle the difference in size of that >> array for 32-on-64 - I really need to check why the structure layout >> checking logic didn''t catch the difference in size. Oh, right, sizeof(void >> *) needs special treatment (and I really don#t want to sue sizeof(long) >> here due to the implied dependency on the OS ABI). > >Having the same size for both 32bit and 64bit makes the Dom0 code >simpler to use it. Taking care about the different sizes would >enforce either hackish code or two different implementations. >That''s not acceptable IMO.After some more thinking about it I actually like the way the patch currently is. This is because the formal structure layout now properly represents the differences between the two bit widths, while it specifically does *not* require different code paths in the guest: The way the structure gets used, regardless of Dom0''s bitness all registers will always be put in the structure (since there''s no is_pv_32on64_???() check in that code), and Dom0 will also not have any issues reading them (unless it applies very pedantic checks on the structure contents) since the structure is to be parsed using its mcinfo_common header, which correctly states the size of the structure. If anything, I''d be inclined to either make the structure variable length (but that would imply all compilers possibly used to compile producing and consuming code understand this construct) or add a comment to the effect of the above explanation. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel