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

[PATCH V3] 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 which meant the register
accesses never made it to vmce_amd_* functions.

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

While at it, remove some clutter in the vmce_amd* functions. Retained
current policy of returning zero for reads and ignoring writes.

Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
---
 xen/arch/x86/cpu/mcheck/amd_f10.c |   41 ++++++-------------------------------
 xen/arch/x86/cpu/mcheck/vmce.c    |    4 ++--
 2 files changed, 8 insertions(+), 37 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/amd_f10.c b/xen/arch/x86/cpu/mcheck/amd_f10.c
index 61319dc..03797ab 100644
--- a/xen/arch/x86/cpu/mcheck/amd_f10.c
+++ b/xen/arch/x86/cpu/mcheck/amd_f10.c
@@ -105,43 +105,14 @@ enum mcheck_type amd_f10_mcheck_init(struct cpuinfo_x86 *c)
 /* amd specific MCA MSR */
 int vmce_amd_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
 {
-	switch (msr) {
-	case MSR_F10_MC4_MISC1: /* DRAM error type */
-		v->arch.vmce.bank[1].mci_misc = val; 
-		mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
-		break;
-	case MSR_F10_MC4_MISC2: /* Link error type */
-	case MSR_F10_MC4_MISC3: /* L3 cache error type */
-		/* ignore write: we do not emulate link and l3 cache errors
-		 * to the guest.
-		 */
-		mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
-		break;
-	default:
-		return 0;
-	}
-
-	return 1;
+    /* Do nothing as we don't emulate this MC bank currently */
+    mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
+    return 1;
 }
 
 int vmce_amd_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
 {
-	switch (msr) {
-	case MSR_F10_MC4_MISC1: /* DRAM error type */
-		*val = v->arch.vmce.bank[1].mci_misc;
-		mce_printk(MCE_VERBOSE, "MCE: rd msr %#"PRIx64"\n", *val);
-		break;
-	case MSR_F10_MC4_MISC2: /* Link error type */
-	case MSR_F10_MC4_MISC3: /* L3 cache error type */
-		/* we do not emulate link and l3 cache
-		 * errors to the guest.
-		 */
-		*val = 0;
-		mce_printk(MCE_VERBOSE, "MCE: rd msr %#"PRIx64"\n", *val);
-		break;
-	default:
-		return 0;
-	}
-
-	return 1;
+    /* Assign '0' as we don't emulate this MC bank currently */
+    *val = 0;
+    return 1;
 }
diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
index f6c35db..be9bb5e 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 | 3) )
     {
     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 | 3) )
     {
     case MSR_IA32_MC0_CTL:
         /*
-- 
1.7.9.5

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

[Liu, Jinsong]

Aravind Gopalakrishnan wrote:
> 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 which meant the register
> accesses never made it to vmce_amd_* functions.
> 
> Corrected 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.
> 
> While at it, remove some clutter in the vmce_amd* functions. Retained
> current policy of returning zero for reads and ignoring writes.
> 
> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> ---
>  xen/arch/x86/cpu/mcheck/amd_f10.c |   41
>  ++++++------------------------------- xen/arch/x86/cpu/mcheck/vmce.c
>  |    4 ++-- 2 files changed, 8 insertions(+), 37 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/amd_f10.c
> b/xen/arch/x86/cpu/mcheck/amd_f10.c 
> index 61319dc..03797ab 100644
> --- a/xen/arch/x86/cpu/mcheck/amd_f10.c
> +++ b/xen/arch/x86/cpu/mcheck/amd_f10.c
> @@ -105,43 +105,14 @@ enum mcheck_type amd_f10_mcheck_init(struct
>  cpuinfo_x86 *c) /* amd specific MCA MSR */
>  int vmce_amd_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>  {
> -	switch (msr) {
> -	case MSR_F10_MC4_MISC1: /* DRAM error type */
> -		v->arch.vmce.bank[1].mci_misc = val;
> -		mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
> -		break;
> -	case MSR_F10_MC4_MISC2: /* Link error type */
> -	case MSR_F10_MC4_MISC3: /* L3 cache error type */
> -		/* ignore write: we do not emulate link and l3 cache errors
> -		 * to the guest.
> -		 */
> -		mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
> -		break;
> -	default:
> -		return 0;
> -	}
> -
> -	return 1;
> +    /* Do nothing as we don't emulate this MC bank currently */
> +    mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
> +    return 1;
>  }
> 
>  int vmce_amd_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>  {
> -	switch (msr) {
> -	case MSR_F10_MC4_MISC1: /* DRAM error type */
> -		*val = v->arch.vmce.bank[1].mci_misc;
> -		mce_printk(MCE_VERBOSE, "MCE: rd msr %#"PRIx64"\n", *val);
> -		break;
> -	case MSR_F10_MC4_MISC2: /* Link error type */
> -	case MSR_F10_MC4_MISC3: /* L3 cache error type */
> -		/* we do not emulate link and l3 cache
> -		 * errors to the guest.
> -		 */
> -		*val = 0;
> -		mce_printk(MCE_VERBOSE, "MCE: rd msr %#"PRIx64"\n", *val);
> -		break;
> -	default:
> -		return 0;
> -	}
> -
> -	return 1;
> +    /* Assign '0' as we don't emulate this MC bank currently */
> +    *val = 0;
> +    return 1;
>  }
> diff --git a/xen/arch/x86/cpu/mcheck/vmce.c
> b/xen/arch/x86/cpu/mcheck/vmce.c 
> index f6c35db..be9bb5e 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 | 3) )

Please add comments for '-MSR_IA32_MC0_CTL' where 1 mask works for all cases, so that later reader know the history:

Intel: MCi_CTL/STATUS/ADDR/MISC    0x400~0x403
        MCi_CTL2                                   0x280 ...

AMD: MCi_CTL/STATUS/ADDR/MISC    0x400~0x403
         MCi_MISCj                                  0xC0000_040x

Thanks,
Jinsong

>      {
>      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 | 3) )
>      {
>      case MSR_IA32_MC0_CTL:
>          /*