* [PATCH] fix c/s 18938
@ 2009-03-27 16:34 Jan Beulich
2009-03-27 16:49 ` Christoph Egger
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2009-03-27 16:34 UTC (permalink / raw)
To: xen-devel
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 */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fix c/s 18938
2009-03-27 16:34 [PATCH] fix c/s 18938 Jan Beulich
@ 2009-03-27 16:49 ` Christoph Egger
2009-03-27 17:08 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Christoph Egger @ 2009-03-27 16:49 UTC (permalink / raw)
To: 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fix c/s 18938
2009-03-27 16:49 ` Christoph Egger
@ 2009-03-27 17:08 ` Jan Beulich
2009-03-27 17:21 ` Christoph Egger
2009-03-27 17:22 ` Frank van der Linden
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2009-03-27 17:08 UTC (permalink / raw)
To: Christoph Egger, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix c/s 18938
2009-03-27 17:08 ` Jan Beulich
@ 2009-03-27 17:21 ` Christoph Egger
2009-03-30 7:14 ` Jan Beulich
2009-03-27 17:22 ` Frank van der Linden
1 sibling, 1 reply; 6+ messages in thread
From: Christoph Egger @ 2009-03-27 17:21 UTC (permalink / raw)
To: Jan Beulich; +Cc: 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix c/s 18938
2009-03-27 17:21 ` Christoph Egger
@ 2009-03-30 7:14 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2009-03-30 7:14 UTC (permalink / raw)
To: Christoph Egger; +Cc: 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix c/s 18938
2009-03-27 17:08 ` Jan Beulich
2009-03-27 17:21 ` Christoph Egger
@ 2009-03-27 17:22 ` Frank van der Linden
1 sibling, 0 replies; 6+ messages in thread
From: Frank van der Linden @ 2009-03-27 17:22 UTC (permalink / raw)
To: Jan Beulich; +Cc: Christoph Egger, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-03-30 7:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-03-27 16:34 [PATCH] fix c/s 18938 Jan Beulich
2009-03-27 16:49 ` Christoph Egger
2009-03-27 17:08 ` Jan Beulich
2009-03-27 17:21 ` Christoph Egger
2009-03-30 7:14 ` Jan Beulich
2009-03-27 17:22 ` Frank van der Linden
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.