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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.02.14 at 05:25, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> 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

> 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@xxxxxxx>
>> ---
>>  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() */
>> +    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:



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