Re: [Xen-devel] [PATCH] MCHECK, AMD: Fix code to allow calls to vmce_amd_rdmsr and vmce_amd_wrmsr

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.11.13 at 21:02, Aravind Gopalakrishnan 
>>> <aravind.gopalakrishnan@xxxxxxx> wrote:
> The existing switch statement: switch ( msr & (MSR_IA32_MC0_CTL | 0x3))
> causes MSR_F10_MC4_MISC1 ... MSR_F10_MC4_MISC3 to be wrongly routed.
> The old values were:
> Value                           After applying msr & (MSR_IA32_MC0_CTL | 0x3)
> -------------------------------- --------------------------------------------
> MSR_F10_MC4_MISC1 = 0xc0000408  0x400 : Falls to case MC0_CTL
> MSR_F10_MC4_MISC2 = 0xc0000409  0x401 : Falls to case MC0_STATUS
> MSR_F10_MC4_MISC3 = 0xc000040A  0x402 : Falls to case MC0_ADDR
> 
> The patch corrects the switch statement to allow vmce_amd_* functions to 
> handle
> guest's rdmsr and wrmsr calls for MSR_F10_MC4_MISC1 ... MSR_F10_MC4_MISC3

But it should do so properly.

> While at it, correct the above mentioned MSR values in msr-index.h
> The values should be -
> MSR_F10_MC4_MISC1 (DRAM error type) = 0x00000413
> MSR_F10_MC4_MISC2 (Link error type) = 0xc0000408
> MSR_F10_MC4_MISC3 (L3 cache error)  = 0xc0000409
> 
> Refer F10 BKDG F3x1[78, 70, 68, 60]. Also, MSRC0000040A does not exist from
> Fam15 onwards. So let's use 0x413 for DRAM errors.

I don't think I follow what you're trying to say here.

> --- a/xen/arch/x86/cpu/mcheck/vmce.c
> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
> @@ -107,7 +107,7 @@ static int bank_mce_rdmsr(const struct vcpu *v, uint32_t 
> msr, uint64_t *val)
>  
>      *val = 0;
>  
> -    switch ( msr & (MSR_IA32_MC0_CTL | 3) )
> +    switch ( msr ) 

You can't drop the masking altogether here - that way you're
preventing banks other than bank 0 to be handled here.

> @@ -210,7 +210,7 @@ static int bank_mce_wrmsr(struct vcpu *v, uint32_t msr, 
> uint64_t val)
>      int ret = 1;
>      unsigned int bank = (msr - MSR_IA32_MC0_CTL) / 4;
>  
> -    switch ( msr & (MSR_IA32_MC0_CTL | 3) )
> +    switch ( msr ) 

Same here.

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1460,8 +1460,9 @@ static int svm_msr_read_intercept(unsigned int msr, 
> uint64_t *msr_content)
>          *msr_content = v->arch.hvm_svm.guest_sysenter_eip;
>          break;
>  
> -    case MSR_IA32_MCx_MISC(4): /* Threshold register */

This deletion isn't being explained at all in the description.

> @@ -1659,8 +1660,9 @@ static int svm_msr_write_intercept(unsigned int msr, 
> uint64_t msr_content)
>          vpmu_do_wrmsr(msr, msr_content);
>          break;
>  
> -    case MSR_IA32_MCx_MISC(4): /* Threshold register */

Again, same thing here.

> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -218,9 +218,9 @@
>  #define AMD64_NB_CFG_CF8_EXT_ENABLE_BIT      46
>  
>  /* AMD Family10h machine check MSRs */
> -#define MSR_F10_MC4_MISC1            0xc0000408
> -#define MSR_F10_MC4_MISC2            0xc0000409
> -#define MSR_F10_MC4_MISC3            0xc000040A
> +#define MSR_F10_MC4_MISC1            0x00000413
> +#define MSR_F10_MC4_MISC2            0xc0000408
> +#define MSR_F10_MC4_MISC3            0xc0000409

Fam10 BIOS and Kernel Developer's Guide disagrees with this.
Fam15 model 0x also doesn't list C0000413 (but indeed marks
C000040A [as well as subsequent ones] as reserved). Fam15
model 1x even lists everything from C0000409 onwards as
reserved.

So I'm afraid you need to start over.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel