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

[Aravind Gopalakrioshnan <aravind.gopalakrishnan@xxxxxxx>]

-    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.
When I drop the mask, I am allowing all MCA MSR's here.. It is when I 
apply the mask
that I am allowing only MC0 bank MSR's alone. Here is an example: (All 
values in hexadecimal form)
(MSR_IA32_MC0_CTL | 3) = 0x403; Therefore -


Msr val = 400
val & 0x403 = 400

Msr val = 401
val & 0x403 = 401

Msr val = 402
val & 0x403 = 402

Msr val = 403
val & 0x403 = 403

Msr val = 404
val & 0x403 = 400

Msr val = 405
val & 0x403 = 401

Msr val = 406
val & 0x403 = 402

Msr val = 407
val & 0x403 = 403

Msr val = 413
val & 0x403 = 403

Msr val = c0000408
val & 0x403 = 400

Msr val = c0000409
val & 0x403 = 401

We need to route the MC4 MSR's (0x413 for DRAM errors, 0xc0000408 for 
Link errors, 0xc0000409 for L3 errors)
to be handled by vmce_amd_wrmsr and vmce_amd_rdmsr. Since by removing 
the mask altogether, I am also
allowing MSR's 0x404, 0x405, 0x406 and 0x407 (which is harmless still as 
they fall to 'default' in vmce_amd_rdmsr or
vmce_amd_wrmsr functions);


> > @@ -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.
 I will explain this better in a V2 patch...
> > --- 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.
It is MSR 0x413 and not 0xc0000413. MSR 0x413 *is* listed as DRAM
thresholding register.

And you are right about Link Thresholding (MSRC0000408) and
L3 Thresholding(MSRC0000409). One way to resolve this can be to add
quirks in 'mce_amd_quirks' structure and check for it before we emulate
Link/L3 thresholding MSR's to the guest.

Thoughts?

Thanks,
-Aravind.
> So I'm afraid you need to start over.
>
> Jan


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