| Message ID | 20191001151633.1659-1-boris.ostrovsky@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/xen: Return from panic notifier | expand |
On 01.10.19 17:16, Boris Ostrovsky wrote: > Currently execution of panic() continues until Xen's panic notifier > (xen_panic_event()) is called at which point we make a hypercall that > never returns. > > This means that any notifier that is supposed to be called later as > well as significant part of panic() code (such as pstore writes from > kmsg_dump()) is never executed. > > There is no reason for xen_panic_event() to be this last point in > execution since panic()'s emergency_restart() will call into > xen_emergency_restart() from where we can perform our hypercall. > > Reported-by: James Dingwall <james@dingwall.me.uk> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
On 01.10.2019 17:16, Boris Ostrovsky wrote: > Currently execution of panic() continues until Xen's panic notifier > (xen_panic_event()) is called at which point we make a hypercall that > never returns. > > This means that any notifier that is supposed to be called later as > well as significant part of panic() code (such as pstore writes from > kmsg_dump()) is never executed. Back at the time when this was introduced into the XenoLinux tree, I think this behavior was intentional for at least DomU-s. I wonder whether you wouldn't want your change to further distinguish Dom0 and DomU behavior. > There is no reason for xen_panic_event() to be this last point in > execution since panic()'s emergency_restart() will call into > xen_emergency_restart() from where we can perform our hypercall. Did you consider, as an alternative, to lower the notifier's priority? Jan
On 10/2/19 3:40 AM, Jan Beulich wrote: > On 01.10.2019 17:16, Boris Ostrovsky wrote: >> Currently execution of panic() continues until Xen's panic notifier >> (xen_panic_event()) is called at which point we make a hypercall that >> never returns. >> >> This means that any notifier that is supposed to be called later as >> well as significant part of panic() code (such as pstore writes from >> kmsg_dump()) is never executed. > Back at the time when this was introduced into the XenoLinux tree, > I think this behavior was intentional for at least DomU-s. I wonder > whether you wouldn't want your change to further distinguish Dom0 > and DomU behavior. Do you remember what the reason for that was? I think having ability to call kmsg_dump() on a panic is a useful thing to have for domUs as well. Besides, there may be other functionality added post-notifiers in panic() in the future. Or another notifier may be registered later with the same lowest priority. Is there a downside in allowing domUs to fall through panic() all the way to emergency_restart()? > >> There is no reason for xen_panic_event() to be this last point in >> execution since panic()'s emergency_restart() will call into >> xen_emergency_restart() from where we can perform our hypercall. > Did you consider, as an alternative, to lower the notifier's > priority? I didn't but that wouldn't help with the original problem that James reported --- we'd still not get to kmsg_dump() call. -boris
On 02.10.2019 15:24, Boris Ostrovsky wrote: > On 10/2/19 3:40 AM, Jan Beulich wrote: >> On 01.10.2019 17:16, Boris Ostrovsky wrote: >>> Currently execution of panic() continues until Xen's panic notifier >>> (xen_panic_event()) is called at which point we make a hypercall that >>> never returns. >>> >>> This means that any notifier that is supposed to be called later as >>> well as significant part of panic() code (such as pstore writes from >>> kmsg_dump()) is never executed. >> Back at the time when this was introduced into the XenoLinux tree, >> I think this behavior was intentional for at least DomU-s. I wonder >> whether you wouldn't want your change to further distinguish Dom0 >> and DomU behavior. > > Do you remember what the reason for that was? I can only guess that the thinking probably was that e.g. external dumping (by the tool stack) would be more reliable (including but not limited to this meaning less change of state from when the original crash reason was detected) than having the domain dump itself. > I think having ability to call kmsg_dump() on a panic is a useful thing > to have for domUs as well. Besides, there may be other functionality > added post-notifiers in panic() in the future. Or another notifier may > be registered later with the same lowest priority. > > Is there a downside in allowing domUs to fall through panic() all the > way to emergency_restart()? See above. >>> There is no reason for xen_panic_event() to be this last point in >>> execution since panic()'s emergency_restart() will call into >>> xen_emergency_restart() from where we can perform our hypercall. >> Did you consider, as an alternative, to lower the notifier's >> priority? > > I didn't but that wouldn't help with the original problem that James > reported --- we'd still not get to kmsg_dump() call. True. I guess more control over the behavior needs to be given to the admin, as either approach has its up- and downsides Jan
On 10/2/19 9:42 AM, Jan Beulich wrote: > > I can only guess that the thinking probably was that e.g. external > dumping (by the tool stack) would be more reliable (including but > not limited to this meaning less change of state from when the > original crash reason was detected) than having the domain dump > itself. We could register an external dumper (controlled by a boot option perhaps, off by default) that will call directly into hypervisor with SHUTDOWN_crash. That will guarantee that we will complete the notifier chain without relying on priorities. (Of course this still won't address a possible new feature in panic() that might be called post-dumping) If you think it's worth doing this can be easily added. -boris > True. I guess more control over the behavior needs to be given to > the admin, as either approach has its up- and downsides >
On 02.10.2019 16:14, Boris Ostrovsky wrote: > On 10/2/19 9:42 AM, Jan Beulich wrote: >> >> I can only guess that the thinking probably was that e.g. external >> dumping (by the tool stack) would be more reliable (including but >> not limited to this meaning less change of state from when the >> original crash reason was detected) than having the domain dump >> itself. > > > We could register an external dumper (controlled by a boot option > perhaps, off by default) that will call directly into hypervisor with > SHUTDOWN_crash. That will guarantee that we will complete the notifier > chain without relying on priorities. (Of course this still won't address > a possible new feature in panic() that might be called post-dumping) > > If you think it's worth doing this can be easily added. Well, I think this is the better option than potentially pingponging between the two extremes, because one wants it the way it currently is and another wants it the way this patch changes things to. Jan
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c index 750f46ad018a..fd4f58cf51dc 100644 --- a/arch/x86/xen/enlighten.c +++ b/arch/x86/xen/enlighten.c @@ -269,16 +269,27 @@ void xen_reboot(int reason) BUG(); } +static int reboot_reason = SHUTDOWN_reboot; void xen_emergency_restart(void) { - xen_reboot(SHUTDOWN_reboot); + xen_reboot(reboot_reason); } static int xen_panic_event(struct notifier_block *this, unsigned long event, void *ptr) { - if (!kexec_crash_loaded()) - xen_reboot(SHUTDOWN_crash); + if (!kexec_crash_loaded()) { + reboot_reason = SHUTDOWN_crash; + + /* + * If panic_timeout==0 then we are supposed to wait forever. + * However, to preserve original dom0 behavior we have to drop + * into hypervisor. (domU behavior is controlled by its + * config file) + */ + if (panic_timeout == 0) + panic_timeout = -1; + } return NOTIFY_DONE; }
Currently execution of panic() continues until Xen's panic notifier (xen_panic_event()) is called at which point we make a hypercall that never returns. This means that any notifier that is supposed to be called later as well as significant part of panic() code (such as pstore writes from kmsg_dump()) is never executed. There is no reason for xen_panic_event() to be this last point in execution since panic()'s emergency_restart() will call into xen_emergency_restart() from where we can perform our hypercall. Reported-by: James Dingwall <james@dingwall.me.uk> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- arch/x86/xen/enlighten.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)