Re: [Xen-devel] [PATCH 2/2] Xen/vMCE: bugfix to remove problematic is_vmce_ready check

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

Christoph Egger wrote:
> On 09.05.13 19:05, Liu, Jinsong wrote:
>> Christoph Egger wrote:
>>> On 06.05.13 18:00, Liu, Jinsong wrote:
>>>> Christoph Egger wrote:
>>>>> On 06.05.13 17:14, Liu, Jinsong wrote:
>>>>>> Egger, Christoph wrote:
>>>>>>> On 06.05.13 17:00, Liu, Jinsong wrote:
>>>>>>>> Christoph Egger wrote:
>>>>>>>>> On 06.05.13 11:50, Liu, Jinsong wrote:
>>>>>>>>>> Christoph Egger wrote:
>>>>>>>>>>> On 06.05.13 11:24, Liu, Jinsong wrote:
>>>>>>>>>>>> Jan Beulich wrote:
>>>>>>>>>>>>>>>> On 06.05.13 at 10:54, Christoph Egger
>>>>>>>>>>>>>>>> <chegger@xxxxxxxxx> wrote:
>>>>>>>>>>>>>> On 03.05.13 17:51, Liu, Jinsong wrote:
>>>>>>>>>>>>>>> Jan Beulich wrote:
>>>>>>>>>>>>>>>>>>> On 03.05.13 at 16:16, "Liu, Jinsong"
>>>>>>>>>>>>>>>>>>> <jinsong.liu@xxxxxxxxx> wrote:
>>>>>>>>>>>>>>>>> Jan Beulich wrote:
>>>>>>>>>>>>>>>>>>>>> On 03.05.13 at 10:41, "Liu, Jinsong"
>>>>>>>>>>>>>>>>>>>>> <jinsong.liu@xxxxxxxxx> wrote:
>>>>>>>>>>>>>>>>>>> Jan Beulich wrote:
>>>>>>>>>>>>>>>>>>>>>>> On 27.04.13 at 10:38, "Liu, Jinsong"
>>>>>>>>>>>>>>>>>>>>>>> <jinsong.liu@xxxxxxxxx> wrote:
>>>>>>>>>>>>>>>>>>>>> From 9098666db640183f894b9aec09599dd32dddb7fa Mon
>>>>>>>>>>>>>>>>>>>>> Sep 17 00:00:00 2001 From: Liu Jinsong
>>>>>>>>>>>>>>>>>>>>> <jinsong.liu@xxxxxxxxx> Date: Sat, 27 Apr 2013
>>>>>>>>>>>>>>>>>>>>> 22:37:35 +0800
>>>>>>>>>>>>>>>>>>>>> Subject: [PATCH 2/2] Xen/vMCE: bugfix to remove
>>>>>>>>>>>>>>>>>>>>> problematic is_vmce_ready check
>>>>>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>>>>> is_vmce_ready() is problematic:
>>>>>>>>>>>>>>>>>>>>> * For dom0, it checks if virq bind to dom0 mcelog
>>>>>>>>>>>>>>>>>>>>> driver. If not, it results dom0 crash. However,
>>>>>>>>>>>>>>>>>>>>> it's problematic and overkilled since mcelog as a
>>>>>>>>>>>>>>>>>>>>> dom0 feature could be enabled/disabled per dom0
>>>>>>>>>>>>>>>>>>>>> option: (XEN) MCE: This error page is ownded by
>>>>>>>>>>>>>>>>>>>>> DOM 0 (XEN) DOM0 not ready for vMCE (XEN)
>>>>>>>>>>>>>>>>>>>>> domain_crash called from mcaction.c:133 (XEN)
>>>>>>>>>>>>>>>>>>>>> Domain 0 reported crashed by domain 32767 on
>>>>>>>>>>>>>>>>>>>>> cpu#31: (XEN) Domain 0 crashed: rebooting machine
>>>>>>>>>>>>>>>>>>>>> in 5 seconds. (XEN) Resetting with ACPI MEMORY or
>>>>>>>>>>>>>>>>>>>>> I/O RESET_REG. 
>>>>>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>>>>> * For dom0, if really need check, it should check
>>>>>>>>>>>>>>>>>>>>> whether vMCE injection for dom0 ready (say,
>>>>>>>>>>>>>>>>>>>>> exception trap bounce check, which has been done
>>>>>>>>>>>>>>>>>>>>> at inject_vmce()), not check dom0 mcelog ready
>>>>>>>>>>>>>>>>>>>>> (which has been done at mce_softirq() before send
>>>>>>>>>>>>>>>>>>>>> global virq to dom0).
>>>>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>>>> Following the argumentation above, I wonder which
>>>>>>>>>>>>>>>>>>>> of the other "goto vmce_failed" are really
>>>>>>>>>>>>>>>>>>>> appropriate, i.e. whether the patch shouldn't be
>>>>>>>>>>>>>>>>>>>> extended (at least for the Dom0 case).
>>>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>>> You mean other 'goto vmce_failed' are also not
>>>>>>>>>>>>>>>>>>> appropriate (I'm not quite clear your point)?
>>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>> Yes.
>>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>>> Would you please point out which point you think not
>>>>>>>>>>>>>>>>>>> appropriate?
>>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>> I question whether it is correct/necessary to crash
>>>>>>>>>>>>>>>>>> the domain in any of those failure cases. Perhaps
>>>>>>>>>>>>>>>>>> when we fail to unmap the page it is, but failure of
>>>>>>>>>>>>>>>>>> fill_vmsr_data() and inject_vmce() don't appear to be
>>>>>>>>>>>>>>>>>> valid reasons once the is_vmce_ready() path is being
>>>>>>>>>>>>>>>>>> dropped.
>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>> For fill_vmsr_data(), it failed only when
>>>>>>>>>>>>>>>>> MCG_STATUS_MCIP bit still set when next vMCE# occur,
>>>>>>>>>>>>>>>>> means the 2nd vMCE# occur when the 1st vMCE# not
>>>>>>>>>>>>>>>>> handled yet. Per SDM it should shutdown.
>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>> For inject_vmce(), it failed when
>>>>>>>>>>>>>>>>> 1). vcpu is still mce_pending, or
>>>>>>>>>>>>>>>>> 2). pv not register trap callback
>>>>>>>>>>>>>>>>> Maybe it's some overkilled for dom0 (for other guest,
>>>>>>>>>>>>>>>>> it's ok to kill them), but any graceful way to quit?
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>> Just exit and do nothing (except perhaps log a rate
>>>>>>>>>>>>>>>> limited message)? 
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> One concern of quiet exit is, the error will be totally
>>>>>>>>>>>>>>> ignored by guest --> it
>>>>>>>>>>>>>> didn't get preperly handled, and may recursively occur to
>>>>>>>>>>>>>> make worse error --> it's better to kill guest under such
>>>>>>>>>>>>>> case.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>> or, considering it rarely happens, how about keep
>>>>>>>>>>>>>>>>> current way (kill guest no matter dom0 or not)?
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>> Possibly - I was merely asking why this one condition
>>>>>>>>>>>>>>>> was found to be too strict, while the others are being
>>>>>>>>>>>>>>>> left as is. 
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>> Jan
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> Ah, the reason of removing is_vmce_ready check is, it's
>>>>>>>>>>>>>>> problematic (check mcelog driver, not vmce tap
>>>>>>>>>>>>>>> callback), and overkilled (since defaultly dom0 will
>>>>>>>>>>>>>>> not start mcelog driver, under which case system will
>>>>>>>>>>>>>>> crash whenever vmce inject to dom0) 
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> --> So patch 2/2 is not too strict for dom0.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Please keep in mind the mcelog userland/kernel interface
>>>>>>>>>>>>>> is not designed with xen in mind. mcelog cannot report
>>>>>>>>>>>>>> which guest is impacted for example, although xen
>>>>>>>>>>>>>> reports that to dom0. I object 'fixing' the hypervisor
>>>>>>>>>>>>>> to come over with mcelog drawbacks. I prefer fixing Dom0
>>>>>>>>>>>>>> instead. 
>>>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> Sure, xen mcelog driver in linux is implemented by me :-)
>>>>>>>>>>>> This patch does not intend to 'fix' hypervisor but just
>>>>>>>>>>>> avoid overkilled system (when xen mcelog driver in dom0
>>>>>>>>>>>> not loaded as default).
>>>>>>>>>>> 
>>>>>>>>>>> I assume dom0 w/o xen mcelog driver active means dom0 is not
>>>>>>>>>>> capable to deal with machine check errors. Is this correct?
>>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> No, w/o xen mcelog driver active, only user daemon 'mcelog'
>>>>>>>>>> was affected. Dom0 is still capable of handling vmce as long
>>>>>>>>>> as it registered trap callback (which is checked at
>>>>>>>>>> hypervisor inject_vmce()).
>>>>>>>>> 
>>>>>>>>> Oh, I thought the xen mcelog *driver* registers the trap
>>>>>>>>> callback and in a Dom0 it additionally registers the logging
>>>>>>>>> callback. What registers the logging callback and what
>>>>>>>>> registers the trap callback? 
>>>>>>>>> 
>>>>>>>>> Christoph
>>>>>>>>> 
>>>>>>>> 
>>>>> [...]
>>>>>>> 
>>>>>>> Ok, that's how I understand the code, too. But you say above
>>>>>>> that w/o this driver Dom0 is still capable to handle vmce. That
>>>>>>> puzzle's me. 
>>>>>>> 
>>>>>>> Christoph
>>>>>> 
>>>>>> Ah, what I meant is, w/o mcelog dirver, kernel still successfully
>>>>>> register its mce handler as trap callback to hypervisor, hence
>>>>>> the injection of vmce from hypervisor to dom0 is OK, and surely
>>>>>> got properly handled at dom0. mcelog driver is just for error
>>>>>> logging, getting error info from hypervisor and provide
>>>>>> interface for user daemon tools 'mcelog'.
>>>>> 
>>>>> Ah, so the trap handler is *always* installed in a Linux Dom0?
>>>> 
>>>> Hypervisor didn't have such assumption, it's agnostic to guest.
>>>> 
>>>>> 
>>>>> NetBSD Dom0/DomU still has no machine check support at all, so in
>>>>> that case Dom0/DomU is not vmce ready. That means the vmce ready
>>>>> check is needed. 
>>>>> 
>>>>> Christoph
>>>> 
>>>> Hypervisor indeed check vmce ready or not at inject_vmce().
>>>> Point of patch 2/2 is, is_vmce_ready itself is problematic.
>>> 
>>> ok. If I understand the problem right, xen sends a machine check
>>> error for logging purpose via virq to dom0 regardless whether
>>> it binds the virq or not. In the latter case dom0 crashes. Is this
>>> correct?
>> 
>> No, in latter case dom0 is not affected, only fails to receive
>> error log (dom0 itself doesn't care error log info, it's just for
>> user tools 'mcelog'). 
> 
> Yes, that's the expected behaviour. What does your patch try
> to fix then?
> 
> Christoph
> 

Please refer to the description of patch 2/2, especially

    * For dom0, if really need check, it should check whether vMCE
    injection for dom0 ready (say, exception trap bounce check, which
    has been done at inject_vmce()), not check dom0 mcelog ready (which
    has been done at mce_softirq() before send global virq to dom0).

Which means before hypervisor send error log via virq to dom0, current code has 
checked whether mcelog ready at dom0 or not --> that's the right place for your 
concern, and it has indeed done check.


Thanks,
Jinsong

>> 
>>> 
>>> In this case xen should just log a rate limited message
>>> (which a sysadmin should be able to understand)
>>> and do nothing else. I think, this is what Jan already suggested.
>>> 
>> 
>> No, that's another story --> Jan concern why other 'goto
>> vmce_failed' are not overkilled. As for is_vmce_ready itself, patch
>> 2/2 is necessary. 
>> 
>> Thanks,
>> Jinsong


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