Re: [PATCH 6] X86/MCE: update vMCE injection for AMD

[Christoph Egger <Christoph.Egger@amd.com>]

On 09/25/12 12:48, Jan Beulich wrote:

>>>> On 25.09.12 at 11:06, Christoph Egger <Christoph.Egger@amd.com> wrote:
>> On 09/25/12 07:00, Liu, Jinsong wrote:
>>
>>> X86/MCE: update vMCE injection for AMD
>>>
>>> For Intel MCE, it broadcasts vMCE to all vcpus. For AMD MCE, it injects
>>> vMCE only to vcpu0. This patch update inject_vmce for AMD.
>>>
>>> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
>>> Suggested_by: Christoph Egger <Christoph.Egger@amd.com>
>>
>>
>> Acked-by: Christoph Egger <Christoph.Egger@amd.com>
> 
> Are you sure (see below)?

Yes, see below.

>>> diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/mce.h
>>> --- a/xen/arch/x86/cpu/mcheck/mce.h	Thu Sep 20 00:03:25 2012 +0800
>>> +++ b/xen/arch/x86/cpu/mcheck/mce.h	Tue Sep 25 19:52:20 2012 +0800
>>> @@ -168,7 +168,7 @@
>>>  
>>>  int fill_vmsr_data(struct mcinfo_bank *mc_bank, struct domain *d,
>>>      uint64_t gstatus);
>>> -int inject_vmce(struct domain *d);
>>> +int inject_vmce(struct domain *d, bool_t vmce_broadcast);
>>>  
>>>  static inline int mce_vendor_bank_msr(const struct vcpu *v, uint32_t msr)
>>>  {
>>> diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/mce_intel.c
>>> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c	Thu Sep 20 00:03:25 2012 +0800
>>> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c	Tue Sep 25 19:52:20 2012 +0800
>>> @@ -365,7 +365,7 @@
>>>                  }
>>>  
>>>                  /* We will inject vMCE to DOMU*/
>>> -                if ( inject_vmce(d) < 0 )
>>> +                if ( inject_vmce(d, 1) < 0 )
>>>                  {
>>>                      mce_printk(MCE_QUIET, "inject vMCE to DOM%d"
>>>                        " failed\n", d->domain_id);
>>> diff -r a6d12a1bc758 xen/arch/x86/cpu/mcheck/vmce.c
>>> --- a/xen/arch/x86/cpu/mcheck/vmce.c	Thu Sep 20 00:03:25 2012 +0800
>>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c	Tue Sep 25 19:52:20 2012 +0800
>>> @@ -344,11 +344,14 @@
>>>  HVM_REGISTER_SAVE_RESTORE(VMCE_VCPU, vmce_save_vcpu_ctxt,
>>>                            vmce_load_vcpu_ctxt, 1, HVMSR_PER_VCPU);
>>>  
>>> -int inject_vmce(struct domain *d)
>>> +/* 
>>> + * for Intel MCE, broadcast vMCE to all vcpus
>>> + * for AMD MCE, only inject vMCE to vcpu0
>>> + */
>>> +int inject_vmce(struct domain *d, bool_t vmce_broadcast)
>>>  {
>>>      struct vcpu *v;
>>>  
>>> -    /* inject vMCE to all vcpus */
>>>      for_each_vcpu(d, v)
>>>      {
>>>          if ( !test_and_set_bool(v->mce_pending) &&
>>> @@ -365,6 +368,9 @@
>>>                         d->domain_id, v->vcpu_id);
>>>              return -1;
>>>          }
>>> +
>>> +        if ( !vmce_broadcast )
>>> +            break;
> 
> That'll allow (non-broadcast) injection to vCPU 0 only - is that
> really the right thing to do?


On AMD side memory errors are found by the northbridge and the first
cpu-core reports this. As long as we do not have NUMA support for the
guest this is fine.

> I.e. shouldn't the caller rather be given flexibility to specify
> which vCPU this is to go to (with a negative value meaning broadcast)?

This is a good idea. Go for it.


> And I'm intending to fold this into patch 2 anyway before
> committing.


Yes, please.

Christoph

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632