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

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

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
>>> 
>> 
>> They are 2 paths (refer latest kernl):
>> 1. for xen mcelog driver, dom0 registers irq handler at driver init
>> via         drivers/xen/mcelog.c 
>>             --> bind_virq_for_mce()
>>                 --> bind_virq_to_irqhandler()
>>         which got VIRQ_MCA via eventchannel binding and handle
>> mcelog error logging accordingly; 
>> 
>> 2. for vmce injection, dom0 registers trap callback (which re-use
>>         kernel machine check handler) via normal kernel traps init:
>> arch/x86/kernel/traps.c 
>>             --> set_intr_gate_ist()
>>                 --> write_idt_entry()
>>                     --> xen_write_idt_entry()
>>                         --> cvt_gate_to_trap()  &
>>         HYPERVISOR_set_trap_table() hypervisor entry.S use the
>> registerred trap callback to construct bounce back stack for
>> TRAP_machine_check, bounce back to dom0 mce handler.  
> 
> 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'.

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