Re: [PATCH] fix c/s 18938

[Christoph Egger <Christoph.Egger@amd.com>]

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