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

["Liu, Jinsong" <jinsong.liu@intel.com>]

Jan Beulich wrote:
>>>> On 18.02.14 at 05:25, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
>> For Intel it didn't disturb Intel's vmce logic so it's OK for me.
>> 
>> For AMD, c000_040x is bank4 (MC4_MISCj), while vmce current only
>> support bank0/1. Even considering in the future AMD may add
>> MC0/1_MISCj, it doesn't need emulation (say, read return 0 and write
>> ignore). So how about simply filter out AMD MCx_MISCj at
>> mce_vendor_bank_msr()? 
> 
> mce_vendor_bank_msr() already has
> 
>     case X86_VENDOR_AMD:
>         switch (msr) {
>         case MSR_F10_MC4_MISC1:
>         case MSR_F10_MC4_MISC2:
>         case MSR_F10_MC4_MISC3:
>             return 1;
>         }
> 
> Jan

OK.

> 
>> 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 |   14 +++++++++++-- 2 files
>>> changed, 18 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..84843fc 100644
>>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>>> @@ -107,7 +107,8 @@ static int bank_mce_rdmsr(const struct vcpu *v,
>>> uint32_t msr, uint64_t *val) 
>>> 
>>>      *val = 0;
>>> 
>>> -    switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>>> +    /* Allow only first 3 MC banks into switch() */

I don't think this comments is good here. Remove it is better.

Thanks,
Jinsong


>>> +    switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
>>>      {
>>>      case MSR_IA32_MC0_CTL:
>>>          /* stick all 1's to MCi_CTL */
>>> @@ -148,6 +149,10 @@ static int bank_mce_rdmsr(const struct vcpu *v,
>>>              uint32_t msr, uint64_t *val) ret = vmce_intel_rdmsr(v,
>>>              msr, val); break;
>>>          case X86_VENDOR_AMD:
>>> +            /*
>>> +             * Extended block of AMD thresholding registers fall
>>> into default. +             * Handle reads here.
>>> +             */
>>>              ret = vmce_amd_rdmsr(v, msr, val);
>>>              break;
>>>          default:
>>> @@ -210,7 +215,8 @@ 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) )
>>> +    /* Allow only first 3 MC banks into switch() */
>>> +    switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
>>>      {
>>>      case MSR_IA32_MC0_CTL:
>>>          /*
>>> @@ -246,6 +252,10 @@ static int bank_mce_wrmsr(struct vcpu *v,
>>>              uint32_t msr, uint64_t val) ret = vmce_intel_wrmsr(v,
>>>              msr, val); break;
>>>          case X86_VENDOR_AMD:
>>> +            /*
>>> +             * Extended block of AMD thresholding registers fall
>>> into default. +             * Handle writes here.
>>> +             */
>>>              ret = vmce_amd_wrmsr(v, msr, val);
>>>              break;
>>>          default: