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

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

Christoph Egger wrote:
> On 09/25/12 12:48, Jan Beulich wrote:
> 
>>>>> On 25.09.12 at 11:06, Christoph Egger <Christoph.Egger@xxxxxxx>
>>>>> 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@xxxxxxxxx>
>>>> Suggested_by: Christoph Egger <Christoph.Egger@xxxxxxx>
>>> 
>>> 
>>> Acked-by: Christoph Egger <Christoph.Egger@xxxxxxx>
>> 
>> 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

I fold patch 6 to patch 2, will send out later.

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