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

[Christoph Egger <chegger@xxxxxxxxx>]

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?

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.

Christoph


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