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

[Aravind Gopalakrioshnan <aravind.gopalakrishnan@amd.com>]

  
-    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
>
>