The panic notifiers infrastructure is a bit limited in the scope of
the callbacks - basically every kind of functionality is dropped
in a list that runs in the same point during the kernel panic path.
This is not really on par with the complexities and particularities
of architecture / hypervisors' needs, and a refactor is ongoing.
As part of this refactor, it was observed that powerpc has 2 notifiers,
with mixed goals: one is just a KASLR offset dumper, whereas the other
aims to hard-disable IRQs (necessary on panic path), warn firmware of
the panic event (fadump) and run low-level platform-specific machinery
that might stop kernel execution and never come back.
Clearly, the 2nd notifier has opposed goals: disable IRQs / fadump
should run earlier while low-level platform actions should
run late since it might not even return. Hence, this patch decouples
the notifiers splitting them in three:
- First one is responsible for hard-disable IRQs and fadump,
should run early;
- The kernel KASLR offset dumper is really an informative notifier,
harmless and may run at any moment in the panic path;
- The last notifier should run last, since it aims to perform
low-level actions for specific platforms, and might never return.
It is also only registered for 2 platforms, pseries and ps3.
The patch better documents the notifiers and clears the code too,
also removing a useless header.
Currently no functionality change should be observed, but after
the planned panic refactor we should expect more panic reliability
with this patch.
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Hari Bathini <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Nicholas Piggin <[email protected]>
Cc: Paul Mackerras <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
We'd like to thanks specially the MiniCloud infrastructure [0] maintainers,
that allow us to test PowerPC code in a very complete, functional and FREE
environment (there's no need even for adding a credit card, like many "free"
clouds require ¬¬ ).
[0] https://openpower.ic.unicamp.br/minicloud
arch/powerpc/kernel/setup-common.c | 74 ++++++++++++++++++++++--------
1 file changed, 54 insertions(+), 20 deletions(-)
diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 518ae5aa9410..52f96b209a96 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -23,7 +23,6 @@
#include <linux/console.h>
#include <linux/screen_info.h>
#include <linux/root_dev.h>
-#include <linux/notifier.h>
#include <linux/cpu.h>
#include <linux/unistd.h>
#include <linux/serial.h>
@@ -680,8 +679,25 @@ int check_legacy_ioport(unsigned long base_port)
}
EXPORT_SYMBOL(check_legacy_ioport);
-static int ppc_panic_event(struct notifier_block *this,
- unsigned long event, void *ptr)
+/*
+ * Panic notifiers setup
+ *
+ * We have 3 notifiers for powerpc, each one from a different "nature":
+ *
+ * - ppc_panic_fadump_handler() is a hypervisor notifier, which hard-disables
+ * IRQs and deal with the Firmware-Assisted dump, when it is configured;
+ * should run early in the panic path.
+ *
+ * - dump_kernel_offset() is an informative notifier, just showing the KASLR
+ * offset if we have RANDOMIZE_BASE set.
+ *
+ * - ppc_panic_platform_handler() is a low-level handler that's registered
+ * only if the platform wishes to perform final actions in the panic path,
+ * hence it should run late and might not even return. Currently, only
+ * pseries and ps3 platforms register callbacks.
+ */
+static int ppc_panic_fadump_handler(struct notifier_block *this,
+ unsigned long event, void *ptr)
{
/*
* panic does a local_irq_disable, but we really
@@ -691,45 +707,63 @@ static int ppc_panic_event(struct notifier_block *this,
/*
* If firmware-assisted dump has been registered then trigger
- * firmware-assisted dump and let firmware handle everything else.
+ * its callback and let the firmware handles everything else.
*/
crash_fadump(NULL, ptr);
- if (ppc_md.panic)
- ppc_md.panic(ptr); /* May not return */
+
return NOTIFY_DONE;
}
-static struct notifier_block ppc_panic_block = {
- .notifier_call = ppc_panic_event,
- .priority = INT_MIN /* may not return; must be done last */
-};
-
-/*
- * Dump out kernel offset information on panic.
- */
static int dump_kernel_offset(struct notifier_block *self, unsigned long v,
void *p)
{
pr_emerg("Kernel Offset: 0x%lx from 0x%lx\n",
kaslr_offset(), KERNELBASE);
- return 0;
+ return NOTIFY_DONE;
}
+static int ppc_panic_platform_handler(struct notifier_block *this,
+ unsigned long event, void *ptr)
+{
+ /*
+ * This handler is only registered if we have a panic callback
+ * on ppc_md, hence NULL check is not needed.
+ * Also, it may not return, so it runs really late on panic path.
+ */
+ ppc_md.panic(ptr);
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block ppc_fadump_block = {
+ .notifier_call = ppc_panic_fadump_handler,
+ .priority = INT_MAX, /* run early, to notify the firmware ASAP */
+};
+
static struct notifier_block kernel_offset_notifier = {
- .notifier_call = dump_kernel_offset
+ .notifier_call = dump_kernel_offset,
+};
+
+static struct notifier_block ppc_panic_block = {
+ .notifier_call = ppc_panic_platform_handler,
+ .priority = INT_MIN, /* may not return; must be done last */
};
void __init setup_panic(void)
{
+ /* Hard-disables IRQs + deal with FW-assisted dump (fadump) */
+ atomic_notifier_chain_register(&panic_notifier_list,
+ &ppc_fadump_block);
+
if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) && kaslr_offset() > 0)
atomic_notifier_chain_register(&panic_notifier_list,
&kernel_offset_notifier);
- /* PPC64 always does a hard irq disable in its panic handler */
- if (!IS_ENABLED(CONFIG_PPC64) && !ppc_md.panic)
- return;
- atomic_notifier_chain_register(&panic_notifier_list, &ppc_panic_block);
+ /* Low-level platform-specific routines that should run on panic */
+ if (ppc_md.panic)
+ atomic_notifier_chain_register(&panic_notifier_list,
+ &ppc_panic_block);
}
#ifdef CONFIG_CHECK_CACHE_COHERENCY
--
2.36.0
On 28/04/22 4:19 am, Guilherme G. Piccoli wrote:
> The panic notifiers infrastructure is a bit limited in the scope of
> the callbacks - basically every kind of functionality is dropped
> in a list that runs in the same point during the kernel panic path.
> This is not really on par with the complexities and particularities
> of architecture / hypervisors' needs, and a refactor is ongoing.
>
> As part of this refactor, it was observed that powerpc has 2 notifiers,
> with mixed goals: one is just a KASLR offset dumper, whereas the other
> aims to hard-disable IRQs (necessary on panic path), warn firmware of
> the panic event (fadump) and run low-level platform-specific machinery
> that might stop kernel execution and never come back.
>
> Clearly, the 2nd notifier has opposed goals: disable IRQs / fadump
> should run earlier while low-level platform actions should
> run late since it might not even return. Hence, this patch decouples
> the notifiers splitting them in three:
>
> - First one is responsible for hard-disable IRQs and fadump,
> should run early;
>
> - The kernel KASLR offset dumper is really an informative notifier,
> harmless and may run at any moment in the panic path;
>
> - The last notifier should run last, since it aims to perform
> low-level actions for specific platforms, and might never return.
> It is also only registered for 2 platforms, pseries and ps3.
>
> The patch better documents the notifiers and clears the code too,
> also removing a useless header.
>
> Currently no functionality change should be observed, but after
> the planned panic refactor we should expect more panic reliability
> with this patch.
>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Hari Bathini <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Nicholas Piggin <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
The change looks good. I have tested it on an LPAR (ppc64).
Reviewed-by: Hari Bathini <[email protected]>
> ---
>
> We'd like to thanks specially the MiniCloud infrastructure [0] maintainers,
> that allow us to test PowerPC code in a very complete, functional and FREE
> environment (there's no need even for adding a credit card, like many "free"
> clouds require ¬¬ ).
>
> [0] https://openpower.ic.unicamp.br/minicloud
>
> arch/powerpc/kernel/setup-common.c | 74 ++++++++++++++++++++++--------
> 1 file changed, 54 insertions(+), 20 deletions(-)
>
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index 518ae5aa9410..52f96b209a96 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -23,7 +23,6 @@
> #include <linux/console.h>
> #include <linux/screen_info.h>
> #include <linux/root_dev.h>
> -#include <linux/notifier.h>
> #include <linux/cpu.h>
> #include <linux/unistd.h>
> #include <linux/serial.h>
> @@ -680,8 +679,25 @@ int check_legacy_ioport(unsigned long base_port)
> }
> EXPORT_SYMBOL(check_legacy_ioport);
>
> -static int ppc_panic_event(struct notifier_block *this,
> - unsigned long event, void *ptr)
> +/*
> + * Panic notifiers setup
> + *
> + * We have 3 notifiers for powerpc, each one from a different "nature":
> + *
> + * - ppc_panic_fadump_handler() is a hypervisor notifier, which hard-disables
> + * IRQs and deal with the Firmware-Assisted dump, when it is configured;
> + * should run early in the panic path.
> + *
> + * - dump_kernel_offset() is an informative notifier, just showing the KASLR
> + * offset if we have RANDOMIZE_BASE set.
> + *
> + * - ppc_panic_platform_handler() is a low-level handler that's registered
> + * only if the platform wishes to perform final actions in the panic path,
> + * hence it should run late and might not even return. Currently, only
> + * pseries and ps3 platforms register callbacks.
> + */
> +static int ppc_panic_fadump_handler(struct notifier_block *this,
> + unsigned long event, void *ptr)
> {
> /*
> * panic does a local_irq_disable, but we really
> @@ -691,45 +707,63 @@ static int ppc_panic_event(struct notifier_block *this,
>
> /*
> * If firmware-assisted dump has been registered then trigger
> - * firmware-assisted dump and let firmware handle everything else.
> + * its callback and let the firmware handles everything else.
> */
> crash_fadump(NULL, ptr);
> - if (ppc_md.panic)
> - ppc_md.panic(ptr); /* May not return */
> +
> return NOTIFY_DONE;
> }
>
> -static struct notifier_block ppc_panic_block = {
> - .notifier_call = ppc_panic_event,
> - .priority = INT_MIN /* may not return; must be done last */
> -};
> -
> -/*
> - * Dump out kernel offset information on panic.
> - */
> static int dump_kernel_offset(struct notifier_block *self, unsigned long v,
> void *p)
> {
> pr_emerg("Kernel Offset: 0x%lx from 0x%lx\n",
> kaslr_offset(), KERNELBASE);
>
> - return 0;
> + return NOTIFY_DONE;
> }
>
> +static int ppc_panic_platform_handler(struct notifier_block *this,
> + unsigned long event, void *ptr)
> +{
> + /*
> + * This handler is only registered if we have a panic callback
> + * on ppc_md, hence NULL check is not needed.
> + * Also, it may not return, so it runs really late on panic path.
> + */
> + ppc_md.panic(ptr);
> +
> + return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block ppc_fadump_block = {
> + .notifier_call = ppc_panic_fadump_handler,
> + .priority = INT_MAX, /* run early, to notify the firmware ASAP */
> +};
> +
> static struct notifier_block kernel_offset_notifier = {
> - .notifier_call = dump_kernel_offset
> + .notifier_call = dump_kernel_offset,
> +};
> +
> +static struct notifier_block ppc_panic_block = {
> + .notifier_call = ppc_panic_platform_handler,
> + .priority = INT_MIN, /* may not return; must be done last */
> };
>
> void __init setup_panic(void)
> {
> + /* Hard-disables IRQs + deal with FW-assisted dump (fadump) */
> + atomic_notifier_chain_register(&panic_notifier_list,
> + &ppc_fadump_block);
> +
> if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) && kaslr_offset() > 0)
> atomic_notifier_chain_register(&panic_notifier_list,
> &kernel_offset_notifier);
>
> - /* PPC64 always does a hard irq disable in its panic handler */
> - if (!IS_ENABLED(CONFIG_PPC64) && !ppc_md.panic)
> - return;
> - atomic_notifier_chain_register(&panic_notifier_list, &ppc_panic_block);
> + /* Low-level platform-specific routines that should run on panic */
> + if (ppc_md.panic)
> + atomic_notifier_chain_register(&panic_notifier_list,
> + &ppc_panic_block);
> }
>
> #ifdef CONFIG_CHECK_CACHE_COHERENCY
On 05/05/2022 15:55, Hari Bathini wrote:
> [...]
>
> The change looks good. I have tested it on an LPAR (ppc64).
>
> Reviewed-by: Hari Bathini <[email protected]>
Thanks a bunch Hari, much appreciated!
On 05/05/2022 15:55, Hari Bathini wrote:
> [...]
> The change looks good. I have tested it on an LPAR (ppc64).
>
> Reviewed-by: Hari Bathini <[email protected]>
>
Hi Michael. do you think it's possible to add this one to powerpc/next
(or something like that), or do you prefer a V2 with his tag?
Thanks,
Guilherme
On 10/05/2022 10:53, Michael Ellerman wrote:
> "Guilherme G. Piccoli" <[email protected]> writes:
>> On 05/05/2022 15:55, Hari Bathini wrote:
>>> [...]
>>> The change looks good. I have tested it on an LPAR (ppc64).
>>>
>>> Reviewed-by: Hari Bathini <[email protected]>
>>>
>>
>> Hi Michael. do you think it's possible to add this one to powerpc/next
>> (or something like that), or do you prefer a V2 with his tag?
>
> Ah sorry, I assumed it was going in as part of the whole series. I guess
> I misread the cover letter.
>
> So you want me to take this patch on its own via the powerpc tree?
>
> cheers
Hi Michael, thanks for the prompt response!
You didn't misread, that was the plan heh
But some maintainers start to take patches and merge in their trees, and
in the end, it seems to make sense - almost half of this series are
fixes or clean-ups, that are not really necessary to get merged altogether.
So, if you can take this one, I'd appreciate - it'll make V2 a bit
smaller =)
Cheers,
Guilherme
"Guilherme G. Piccoli" <[email protected]> writes:
> On 05/05/2022 15:55, Hari Bathini wrote:
>> [...]
>> The change looks good. I have tested it on an LPAR (ppc64).
>>
>> Reviewed-by: Hari Bathini <[email protected]>
>>
>
> Hi Michael. do you think it's possible to add this one to powerpc/next
> (or something like that), or do you prefer a V2 with his tag?
Ah sorry, I assumed it was going in as part of the whole series. I guess
I misread the cover letter.
So you want me to take this patch on its own via the powerpc tree?
cheers