On Fri, Dec 01, 2017 at 10:11:50PM +1100, Michael Ellerman wrote: > David Gibson <da...@gibson.dropbear.id.au> writes: > > > On Wed, Nov 29, 2017 at 02:23:43PM +1000, Nicholas Piggin wrote: > >> On Wed, 29 Nov 2017 15:06:52 +1100 > >> David Gibson <da...@gibson.dropbear.id.au> wrote: > >> > >> > a3b2cb30 "powerpc: Do not call ppc_md.panic in fadump panic notifier" > >> > purports to fix a problem when the kernel panics with fadump not > >> > registered, but it breaks something else instead. I _think_ it was > >> > working on the incorrect assumption that ppc_md.panic was (or should > >> > be) only used with fadump, but I'm not really sure. > >> > > >> > Panic works with kdump enabled, and (I think) with fadump enabled). > >> > However, with neither of these enabled, we always go to the generic > >> > panic logic. > >> > >> Yeah thanks, I can't remember what assumption I was working on tbh. > >> > >> > That's incorrect for PAPR guests - they should call ibm,os-term via > >> > RTAS. Under qemu this leads to a "GUEST_PANICKED" event notification > >> > which higher-level management pays attention to. Since a3b2cb30 we > >> > now reboot instead of reporting that. > >> > > >> > I believe it will also break panic for PS3 machines, but since that > >> > platform basically no longer exists, we probably don't care. > >> > >> I (hope) it should just go down to the normal panic path and not do > >> much worse than it already does -- although it won't print out that > >> message. > >> > >> > I'm not entirely sure how to fix this. I _think_ what we want is to > >> > call ppc_md.panic from a late panic notifier, the way this patch does > >> > for fadump_panic_event() if fadump is registered. > >> > >> The problem I had there is that some of the printk and console stuff > >> wasn't getting flushed out, so I was getting a blank screen. This was > >> probably in conjunction with panicing from NMI context that we're now > >> starting to introduce. > >> > >> So it's a bit annoying. There's other ugliness we have for being unable > >> to control panic code well enough from arch code > >> (arch/powerpc/platforms/powernv/opal.c) > >> > >> I guess a really minimal fix is to put an #ifdef powerpc down the bottom > >> there (/me *cries*). > > > > Um.. right. I'm not really sure from that how to go forward from > > here. We want to fix this for RHEL7.5, which doesn't give us a lot of > > time. > > > > Adding the #ifdef at the bottom of the generic panic code is gross, > > but there's already a bunch of that, so maybe adequate until a better > > solution can be found? > > I think you mean put an #ifdef at the bottom of panic(). If so that > won't work. Our default panic_timeout is 180 so we never get to the > bottom of panic(), we call emergency_restart(). > > You *could* put an #ifdef powerpc before that, but that's even more > gross because it's in a different place to the sparc/s390 #ifdefs. > > I notice we don't implement machine_emergency_restart(), it just > becomes machine_restart(NULL). > > So it seems like that's the place we should be hooking, > machine_emergency_restart(). That's what x86 does.
Only sort of. machine_emergency_restart() is in basically the right
place, but AFAICT it is supposed to restart the machine, which os-term
(by design) does not. x86 uses this for a restart, when using pvpanic
which provides similar functionality to os-term, that gets called
before reaching emergency_reset().
> But panic() is not the only caller of emergency_restart(), so it's not
> an entirely straight forward conversion.
>
> So I think for 4.15 and 4.14 I'm inclined to revert. Then we can do a
> bigger rework for 4.16.
I've sent a revert for now.
> Nick that shouldn't break your original aim too badly I think? ie. worst
> case is we panic() but don't see the output if we came from NMI?
>
> cheers
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
