[PATCH 0/2 V2] Fix AMD threshold register definitions and activate vmce_amd_* functions

[PATCH 0/2 V2] Fix AMD threshold register definitions and activate vmce_amd_* functions

[Aravind Gopalakrishnan]

Patch 1: Deals with correcting AMD threshold register definitions.
Patch 2: Fixes mask in vmce.c to allow vmce_amd_* functions to handle access to
         AMD thresholding registers.

Changes from V1:
    - Correct time skew on the V1 patch.

Aravind Gopalakrishnan (2):
  hvm,svm: Update AMD Thresholding MSR definitions
  mcheck,vmce: Allow vmce_amd_* functions to handle AMD thresolding
    MSRs

 xen/arch/x86/cpu/mcheck/vmce.c  |    4 ++--
 xen/arch/x86/hvm/svm/svm.c      |   10 ++++++----
 xen/include/asm-x86/msr-index.h |    6 +++---
 3 files changed, 11 insertions(+), 9 deletions(-)

-- 
1.7.9.5

[PATCH 1/2 V2] hvm, svm: Update AMD Thresholding MSR definitions

[Aravind Gopalakrishnan]

MSR 0xC000040A is marked as reserved from Fam15 onwards and
MSR 0x413 is marked alias of MSR0xC000040A on Fam10 BKDG.
So remove the unnecessary definition of the reserved MSR and
use MSR_IA32_MCx_MISC() to define MSR 0x413.

Also, according to BKDG, MSR 0x413 is the first of the thresholding
registers; MSR 0xC0000408 and MSR 0xC0000409 are second and third
respectively. So rework the #define's accordingly.

Fam15 Model 00h-0fh  BKDG reference:
http://support.amd.com/TechDocs/42301_15h_Mod_00h-0Fh_BKDG.pdf

Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/arch/x86/hvm/svm/svm.c      |   10 ++++++----
 xen/include/asm-x86/msr-index.h |    6 +++---
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 406d394..07c2684 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1461,8 +1461,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 */
-    case MSR_F10_MC4_MISC1 ... MSR_F10_MC4_MISC3:
+    case MSR_F10_MC4_MISC1: /* Threshold registers */
+    case MSR_F10_MC4_MISC2:
+    case MSR_F10_MC4_MISC3:
         /*
          * MCA/MCE: We report that the threshold register is unavailable
          * for OS use (locked by the BIOS).
@@ -1660,8 +1661,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 */
-    case MSR_F10_MC4_MISC1 ... MSR_F10_MC4_MISC3:
+    case MSR_F10_MC4_MISC1: /* Threshold registers */
+    case MSR_F10_MC4_MISC2:
+    case MSR_F10_MC4_MISC3:
         /*
          * MCA/MCE: Threshold register is reported to be locked, so we ignore
          * all write accesses. This behaviour matches real HW, so guests should
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index fc9fbc6..e5ffbf2 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -219,9 +219,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		MSR_IA32_MCx_MISC(4)
+#define MSR_F10_MC4_MISC2		0xc0000408
+#define MSR_F10_MC4_MISC3		0xc0000409
 
 /* AMD Family10h Bus Unit MSRs */
 #define MSR_F10_BU_CFG 		0xc0011023
-- 
1.7.9.5

Re: [PATCH 1/2 V2] hvm, svm: Update AMD Thresholding MSR definitions

[Jan Beulich]

>>> On 27.01.14 at 23:44, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:
> MSR 0xC000040A is marked as reserved from Fam15 onwards and
> MSR 0x413 is marked alias of MSR0xC000040A on Fam10 BKDG.
> So remove the unnecessary definition of the reserved MSR and
> use MSR_IA32_MCx_MISC() to define MSR 0x413.

My Fam10 BKDG version doesn't say anything like this. Instead it
says that the low 32 bits of all 4 registers are identical (i.e. all are
aliasing 0x413), whereas the high 32 bits are different among all
the four registers (with 0xc000040a having them all zero).

> Also, according to BKDG, MSR 0x413 is the first of the thresholding
> registers; MSR 0xC0000408 and MSR 0xC0000409 are second and third
> respectively. So rework the #define's accordingly.
> 
> Fam15 Model 00h-0fh  BKDG reference:
> http://support.amd.com/TechDocs/42301_15h_Mod_00h-0Fh_BKDG.pdf 

Higher model numbers appear to also have 0xc0000409 reserved...

Jan

> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c      |   10 ++++++----
>  xen/include/asm-x86/msr-index.h |    6 +++---
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 406d394..07c2684 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1461,8 +1461,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 */
> -    case MSR_F10_MC4_MISC1 ... MSR_F10_MC4_MISC3:
> +    case MSR_F10_MC4_MISC1: /* Threshold registers */
> +    case MSR_F10_MC4_MISC2:
> +    case MSR_F10_MC4_MISC3:
>          /*
>           * MCA/MCE: We report that the threshold register is unavailable
>           * for OS use (locked by the BIOS).
> @@ -1660,8 +1661,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 */
> -    case MSR_F10_MC4_MISC1 ... MSR_F10_MC4_MISC3:
> +    case MSR_F10_MC4_MISC1: /* Threshold registers */
> +    case MSR_F10_MC4_MISC2:
> +    case MSR_F10_MC4_MISC3:
>          /*
>           * MCA/MCE: Threshold register is reported to be locked, so we 
> ignore
>           * all write accesses. This behaviour matches real HW, so guests 
> should
> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
> index fc9fbc6..e5ffbf2 100644
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -219,9 +219,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		MSR_IA32_MCx_MISC(4)
> +#define MSR_F10_MC4_MISC2		0xc0000408
> +#define MSR_F10_MC4_MISC3		0xc0000409
>  
>  /* AMD Family10h Bus Unit MSRs */
>  #define MSR_F10_BU_CFG 		0xc0011023
> -- 
> 1.7.9.5
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH 1/2 V2] hvm, svm: Update AMD Thresholding MSR definitions

[Aravind Gopalakrishnan]

On 1/28/2014 5:17 AM, Jan Beulich wrote:
>>>> On 27.01.14 at 23:44, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:
>> MSR 0xC000040A is marked as reserved from Fam15 onwards and
>> MSR 0x413 is marked alias of MSR0xC000040A on Fam10 BKDG.
>> So remove the unnecessary definition of the reserved MSR and
>> use MSR_IA32_MCx_MISC() to define MSR 0x413.
> My Fam10 BKDG version doesn't say anything like this. Instead it
> says that the low 32 bits of all 4 registers are identical (i.e. all are
> aliasing 0x413), whereas the high 32 bits are different among all
> the four registers (with 0xc000040a having them all zero).

Thanks for pointing this out; looks like MSR 0xc000040a is zeroed out 
completely:
(F3x178 (MSRC000_040A): RAZ.)(page 339 on 
http://support.amd.com/TechDocs/31116.pdf)
I have reworded commit message which (hopefully) conveys this better..

>> Also, according to BKDG, MSR 0x413 is the first of the thresholding
>> registers; MSR 0xC0000408 and MSR 0xC0000409 are second and third
>> respectively. So rework the #define's accordingly.
>>
>> Fam15 Model 00h-0fh  BKDG reference:
>> http://support.amd.com/TechDocs/42301_15h_Mod_00h-0Fh_BKDG.pdf
> Higher model numbers appear to also have 0xc0000409 reserved...
>
>

Yes, thanks again for the pointer..

I have now reworked the code to care for the existence of extended block 
of MC4_MISC registers..
(Note: 0xc0000409 is reserved in newer model of F15h, while both 
0xc0000408 and 0xc0000409 are reserved in F16h)
a) If the registers exist in HW, then we continue to enforce current 
policy of not emulating as we do not expose MC4 to guest
b) If they don't, then #GP fault to guest.

-Aravind.

[PATCH 2/2 V2] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs

[Aravind Gopalakrishnan]

vmce_amd_[rd|wr]msr functions can handle accesses to AMD thresholding
registers. But due to this statement here:
switch ( msr & (MSR_IA32_MC0_CTL | 3) )
we are wrongly masking off top two bits and bit 4 which meant the
register accesses never made it to vmce_amd_* functions.

We correct this problem by modifying the mask in this patch to allow
AMD thresholding registers to fall to 'default' case which in turn
allows vmce_amd_* functions to handle access to the registers.

Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/arch/x86/cpu/mcheck/vmce.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
index f6c35db..cb4fd12 100644
--- 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 & (MSR_IA32_MC0_CTL | (0x3 << 30) | 0x13))
     {
     case MSR_IA32_MC0_CTL:
         /* stick all 1's to MCi_CTL */
@@ -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 & (MSR_IA32_MC0_CTL | (0x3 << 30) | 0x13))
     {
     case MSR_IA32_MC0_CTL:
         /*
-- 
1.7.9.5

Re: [PATCH 2/2 V2] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs

[Jan Beulich]

>>> On 27.01.14 at 23:44, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:
> --- 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 & (MSR_IA32_MC0_CTL | (0x3 << 30) | 0x13))

As one of the other reviewers already said - 0xC0000000 would
be better recognizable here.

As to the 3 -> 0x13 change - I don't think this is conceptually
correct. While at present we emulate only 2 banks, this had
been different in the past and may become different again.
Hence introducing a dis-contiguity after bank 3 is undesirable.

Jan

Re: [PATCH 2/2 V2] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs

[Aravind Gopalakrishnan]

On 1/28/2014 5:24 AM, Jan Beulich wrote:
>>>> On 27.01.14 at 23:44, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:
>> --- 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 & (MSR_IA32_MC0_CTL | (0x3 << 30) | 0x13))
> As one of the other reviewers already said - 0xC0000000 would
> be better recognizable here.
>
> As to the 3 -> 0x13 change - I don't think this is conceptually
> correct. While at present we emulate only 2 banks, this had
> been different in the past and may become different again.
> Hence introducing a dis-contiguity after bank 3 is undesirable.
>
>

IMHO, including the '0x13' is necessary. The reason is that 0x413, 
0xc0000408 and 0xc0000409
together form the set of MC4 thresholding registers. Not including 0x13 
in the mask would mean
that accesses to 0x413 alone would not be handled. (which would be 
confusing if someone new
were to look into the mce codebase)

Also, (in response to Boris's comments) - AFAICT, this should not affect 
Intel codepath..
Intel's vmce_* functions only care about MSR_IA32_MC0_CTL2 = 0x00000280 
(if bank0) or 0x00000281 (if bank 1)
and, having 0x13 in the mask does not affect the ability of 
bank_mce_[rd|wr]msr functions to call into vmce_intel*
functions..
(I haven't tested this, so if someone could test and let me know, that'd 
be great.)

I have addressed Christoph's and Boris.O's earlier comments too in the 
next version of this patch.


Thanks,
-Aravind.

Re: [PATCH 2/2 V2] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs

[Jan Beulich]

>>> On 05.02.14 at 21:41, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
wrote:
> On 1/28/2014 5:24 AM, Jan Beulich wrote:
>>>>> On 27.01.14 at 23:44, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> 
> wrote:
>>> --- 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 & (MSR_IA32_MC0_CTL | (0x3 << 30) | 0x13))
>> As one of the other reviewers already said - 0xC0000000 would
>> be better recognizable here.
>>
>> As to the 3 -> 0x13 change - I don't think this is conceptually
>> correct. While at present we emulate only 2 banks, this had
>> been different in the past and may become different again.
>> Hence introducing a dis-contiguity after bank 3 is undesirable.
> 
> IMHO, including the '0x13' is necessary. The reason is that 0x413, 
> 0xc0000408 and 0xc0000409
> together form the set of MC4 thresholding registers. Not including 0x13 
> in the mask would mean
> that accesses to 0x413 alone would not be handled. (which would be 
> confusing if someone new
> were to look into the mce codebase)

No - bit 4 is part of what forms the bank number. Hence it must
be masked out in the switch() expression.

Jan

Re: [PATCH 2/2 V2] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs

[Egger, Christoph]

On 06.02.14 10:07, Jan Beulich wrote:
>>>> On 05.02.14 at 21:41, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> wrote:
>> On 1/28/2014 5:24 AM, Jan Beulich wrote:
>>>>>> On 27.01.14 at 23:44, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> 
>> wrote:
>>>> --- 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 & (MSR_IA32_MC0_CTL | (0x3 << 30) | 0x13))
>>> As one of the other reviewers already said - 0xC0000000 would
>>> be better recognizable here.
>>>
>>> As to the 3 -> 0x13 change - I don't think this is conceptually
>>> correct. While at present we emulate only 2 banks, this had
>>> been different in the past and may become different again.
>>> Hence introducing a dis-contiguity after bank 3 is undesirable.
>>
>> IMHO, including the '0x13' is necessary. The reason is that 0x413, 
>> 0xc0000408 and 0xc0000409
>> together form the set of MC4 thresholding registers. Not including 0x13 
>> in the mask would mean
>> that accesses to 0x413 alone would not be handled. (which would be 
>> confusing if someone new
>> were to look into the mce codebase)
> 
> No - bit 4 is part of what forms the bank number. Hence it must
> be masked out in the switch() expression.

I prefer to see a comment in the code that makes this clear.

Christoph

Re: [PATCH 2/2 V2] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs

[Aravind Gopalakrishnan]

On Wed, Feb 12, 2014 at 03:36:26AM -0600, Egger, Christoph wrote:
> On 06.02.14 10:07, Jan Beulich wrote:
> >>>> On 05.02.14 at 21:41, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> > wrote:
> >> On 1/28/2014 5:24 AM, Jan Beulich wrote:
> >>>>>> On 27.01.14 at 23:44, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> 
> >> wrote:
> > 
> > No - bit 4 is part of what forms the bank number. Hence it must
> > be masked out in the switch() expression.
> 
> I prefer to see a comment in the code that makes this clear.
> 

Agreed, I have added very brief comments in the code.
Do let me know if it needs to be more verbose..

Thanks,
-Aravind.