comments for c/s 25919 Xen mce

comments for c/s 25919 Xen mce

[Liu, Jinsong]

Christoph, Keir, Jan

I have a draft look at c/s 25919. It moves some mce_intel.c logic to mce.c (and remove old mce.c logic). By draft reviewing the patch I think it need more work to do, and currently it in fact would hung at AMD platform (I have no AMD platform to test), i.e, MACHINE_CHECK_SOFTIRQ --> mce_delayed_action() --> mce_action() --> ASSERT(handler_num);

For AMD mce it may need add (if any misunderstanding please point to me)
1). add default handler which used at softirq context
2). add AMD vmce inject logic
3). more test

Thoughts?

Thanks,
Jinsong

Re: comments for c/s 25919 Xen mce

[Christoph Egger]

On 09/21/12 08:44, Liu, Jinsong wrote:

> Christoph, Keir, Jan
> 
> I have a draft look at c/s 25919. It moves some mce_intel.c logic to mce.c (and remove old mce.c logic). By draft reviewing the patch I think it need more work to do, and currently it in fact would hung at AMD platform (I have no AMD platform to test), i.e, MACHINE_CHECK_SOFTIRQ --> mce_delayed_action() --> mce_action() --> ASSERT(handler_num);
> 
> For AMD mce it may need add (if any misunderstanding please point to me)
> 1). add default handler which used at softirq context


This is mcee_softirq().

> 2). add AMD vmce inject logic


Yes, patch is sent.
See http://lists.xen.org/archives/html/xen-devel/2012-09/msg01413.html

> 3). more test


There are more patches in my queue.

Christoph

> 
> Thoughts?
> 
> Thanks,
> Jinsong



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

Re: comments for c/s 25919 Xen mce

[Liu, Jinsong]

Christoph Egger wrote:
> On 09/21/12 08:44, Liu, Jinsong wrote:
> 
>> Christoph, Keir, Jan
>> 
>> I have a draft look at c/s 25919. It moves some mce_intel.c logic to
>> mce.c (and remove old mce.c logic). By draft reviewing the patch I
>> think it need more work to do, and currently it in fact would hung
>> at AMD platform (I have no AMD platform to test), i.e,
>> MACHINE_CHECK_SOFTIRQ --> mce_delayed_action() --> mce_action() -->
>> ASSERT(handler_num);     
>> 
>> For AMD mce it may need add (if any misunderstanding please point to
>> me) 1). add default handler which used at softirq context
> 
> 
> This is mcee_softirq().
> 
>> 2). add AMD vmce inject logic
> 
> 
> Yes, patch is sent.
> See http://lists.xen.org/archives/html/xen-devel/2012-09/msg01413.html
> 
>> 3). more test
> 
> 
> There are more patches in my queue.
> 
> Christoph
> 

So, Christoph, considering you have a patches queue and would change more mce/vmce, how about wait a moment and then based your patches on my vmce patches? My vmce patches have been posted for quite some time, and have already been reviewed-rebased-tested several rounds. I think they are basically stable now, currently only need tools maintainers to have review patch4/5 (live migration tools part).

... I know I have no right asking you to do so, but we need cooperate for mce/vmce, and if you agree I would be very much appreciated. As for your mce/vmce patches, I'd like to help test at Intel's platform to make sure it works fine.

Thanks,
Jinsong

Re: comments for c/s 25919 Xen mce

[Christoph Egger]

On 09/24/12 12:24, Liu, Jinsong wrote:

> Christoph Egger wrote:
>> On 09/21/12 08:44, Liu, Jinsong wrote:
>>
>>> Christoph, Keir, Jan
>>>
>>> I have a draft look at c/s 25919. It moves some mce_intel.c logic to
>>> mce.c (and remove old mce.c logic). By draft reviewing the patch I
>>> think it need more work to do, and currently it in fact would hung
>>> at AMD platform (I have no AMD platform to test), i.e,
>>> MACHINE_CHECK_SOFTIRQ --> mce_delayed_action() --> mce_action() -->
>>> ASSERT(handler_num);     
>>>
>>> For AMD mce it may need add (if any misunderstanding please point to
>>> me) 1). add default handler which used at softirq context
>>
>>
>> This is mcee_softirq().
>>
>>> 2). add AMD vmce inject logic
>>
>>
>> Yes, patch is sent.
>> See http://lists.xen.org/archives/html/xen-devel/2012-09/msg01413.html
>>
>>> 3). more test
>>
>>
>> There are more patches in my queue.
>>
>> Christoph
>>
> 
> So, Christoph, considering you have a patches queue and would

> change more mce/vmce, how about wait a moment and then based your
> patches on my vmce patches? My vmce patches have been posted for
> quite some time, and have already been reviewed-rebased-tested
> several rounds. I think they are basically stable now, currently only
> need tools maintainers to have review patch4/5 (live migration tools
> part).

> 
> ... I know I have no right asking you to do so, but we need cooperate

> for mce/vmce, and if you agree I would be very much appreciated. As
> for your mce/vmce patches, I'd like to help test at Intel's platform
> to make sure it works fine.

I don't mind in which order the patches go upstream -
yours first, mine first or somehow interleaved..

It is just important to me that you take my comments (to patch 2/5)
into account to minimize the required rebase-retest effort.

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

Re: comments for c/s 25919 Xen mce

[Liu, Jinsong]

Christoph Egger wrote:
> On 09/24/12 12:24, Liu, Jinsong wrote:
> 
>> Christoph Egger wrote:
>>> On 09/21/12 08:44, Liu, Jinsong wrote:
>>> 
>>>> Christoph, Keir, Jan
>>>> 
>>>> I have a draft look at c/s 25919. It moves some mce_intel.c logic
>>>> to mce.c (and remove old mce.c logic). By draft reviewing the
>>>> patch I think it need more work to do, and currently it in fact
>>>> would hung at AMD platform (I have no AMD platform to test), i.e,
>>>> MACHINE_CHECK_SOFTIRQ --> mce_delayed_action() --> mce_action()
>>>> --> ASSERT(handler_num); 
>>>> 
>>>> For AMD mce it may need add (if any misunderstanding please point
>>>> to me) 1). add default handler which used at softirq context
>>> 
>>> 
>>> This is mcee_softirq().
>>> 
>>>> 2). add AMD vmce inject logic
>>> 
>>> 
>>> Yes, patch is sent.
>>> See
>>> http://lists.xen.org/archives/html/xen-devel/2012-09/msg01413.html 
>>> 
>>>> 3). more test
>>> 
>>> 
>>> There are more patches in my queue.
>>> 
>>> Christoph
>>> 
>> 
>> So, Christoph, considering you have a patches queue and would
> 
>> change more mce/vmce, how about wait a moment and then based your
>> patches on my vmce patches? My vmce patches have been posted for
>> quite some time, and have already been reviewed-rebased-tested
>> several rounds. I think they are basically stable now, currently only
>> need tools maintainers to have review patch4/5 (live migration tools
>> part).
> 
>> 
>> ... I know I have no right asking you to do so, but we need cooperate
> 
>> for mce/vmce, and if you agree I would be very much appreciated. As
>> for your mce/vmce patches, I'd like to help test at Intel's platform
>> to make sure it works fine.
> 
> I don't mind in which order the patches go upstream -
> yours first, mine first or somehow interleaved..
> 
> It is just important to me that you take my comments (to patch 2/5)
> into account to minimize the required rebase-retest effort.
> 
> Christoph

Thanks Christoph! I have add a patch injecting vmce for AMD per your comments, will sent out later.

Best Regards,
Jinsong