| Message ID | YfP0osb45uJldtM9@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm64: move efi_reboot to restart handler | expand |
Hi! On 28/01/2022 14:50, Krzysztof Adamski wrote: > On EFI enabled arm64 systems, efi_reboot was called before > do_kernel_restart, completely omitting the reset_handlers functionality. > By registering efi_reboot as part of the chain with slightly elevated > priority, we make it run before the default handler but still allow > plugging in other handlers. > Thanks to that, things like gpio_restart, restart handlers in > watchdog_core, mmc or mtds are working on those platforms. > > The priority 130 is one higher than PSCI, to overrule that but still > allow to easily register higher prio handlers, if needed. > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> > --- > > Changes in v2: > - Register the handler in EFI code, instead of arm64 setup.c > - Remove the contdition from the handler - it should be run in all > cases when it is registered > - Bump the priority to 130 to make it completly obious this should be > run before PSCI (which has priority of 129) > > arch/arm64/kernel/process.c | 7 ------- > drivers/firmware/efi/arm-runtime.c | 21 +++++++++++++++++++++ > 2 files changed, 21 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 5369e649fa79..b86ef77bb0c8 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -130,13 +130,6 @@ void machine_restart(char *cmd) > local_irq_disable(); > smp_send_stop(); > > - /* > - * UpdateCapsule() depends on the system being reset via > - * ResetSystem(). > - */ > - if (efi_enabled(EFI_RUNTIME_SERVICES)) > - efi_reboot(reboot_mode, NULL); > - > /* Now call the architecture specific reboot code. */ > do_kernel_restart(cmd); > > diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c > index 3359ae2adf24..b9a2cdbe80b4 100644 > --- a/drivers/firmware/efi/arm-runtime.c > +++ b/drivers/firmware/efi/arm-runtime.c > @@ -80,6 +80,24 @@ static bool __init efi_virtmap_init(void) > return true; > } > > +static int efi_restart(struct notifier_block *nb, unsigned long action, > + void *data) > +{ > + /* > + * UpdateCapsule() depends on the system being reset via > + * ResetSystem(). > + */ > + efi_reboot(reboot_mode, NULL); > + > + return NOTIFY_DONE; > +} > + > +static struct notifier_block efi_restart_nb = { > + .notifier_call = efi_restart, > + /* We want this to take priority over PSCI which has priority of 129. */ > + .priority = 130, > +}; > + > /* > * Enable the UEFI Runtime Services if all prerequisites are in place, i.e., > * non-early mapping of the UEFI system table and virtual mappings for all > @@ -148,6 +166,9 @@ static int __init arm_enable_runtime_services(void) > efi_native_runtime_setup(); > set_bit(EFI_RUNTIME_SERVICES, &efi.flags); > > + if (IS_ENABLED(CONFIG_ARM64)) > + register_restart_handler(&efi_restart_nb); > + > return 0; > } > early_initcall(arm_enable_runtime_services);
On 1/28/22 05:50, Krzysztof Adamski wrote: > On EFI enabled arm64 systems, efi_reboot was called before > do_kernel_restart, completely omitting the reset_handlers functionality. > By registering efi_reboot as part of the chain with slightly elevated > priority, we make it run before the default handler but still allow > plugging in other handlers. > Thanks to that, things like gpio_restart, restart handlers in > watchdog_core, mmc or mtds are working on those platforms. > > The priority 130 is one higher than PSCI, to overrule that but still > allow to easily register higher prio handlers, if needed. > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > > Changes in v2: > - Register the handler in EFI code, instead of arm64 setup.c > - Remove the contdition from the handler - it should be run in all > cases when it is registered > - Bump the priority to 130 to make it completly obious this should be > run before PSCI (which has priority of 129) > > arch/arm64/kernel/process.c | 7 ------- > drivers/firmware/efi/arm-runtime.c | 21 +++++++++++++++++++++ > 2 files changed, 21 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 5369e649fa79..b86ef77bb0c8 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -130,13 +130,6 @@ void machine_restart(char *cmd) > local_irq_disable(); > smp_send_stop(); > > - /* > - * UpdateCapsule() depends on the system being reset via > - * ResetSystem(). > - */ > - if (efi_enabled(EFI_RUNTIME_SERVICES)) > - efi_reboot(reboot_mode, NULL); > - > /* Now call the architecture specific reboot code. */ > do_kernel_restart(cmd); > > diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c > index 3359ae2adf24..b9a2cdbe80b4 100644 > --- a/drivers/firmware/efi/arm-runtime.c > +++ b/drivers/firmware/efi/arm-runtime.c > @@ -80,6 +80,24 @@ static bool __init efi_virtmap_init(void) > return true; > } > > +static int efi_restart(struct notifier_block *nb, unsigned long action, > + void *data) > +{ > + /* > + * UpdateCapsule() depends on the system being reset via > + * ResetSystem(). > + */ > + efi_reboot(reboot_mode, NULL); > + > + return NOTIFY_DONE; > +} > + > +static struct notifier_block efi_restart_nb = { > + .notifier_call = efi_restart, > + /* We want this to take priority over PSCI which has priority of 129. */ > + .priority = 130, > +}; > + > /* > * Enable the UEFI Runtime Services if all prerequisites are in place, i.e., > * non-early mapping of the UEFI system table and virtual mappings for all > @@ -148,6 +166,9 @@ static int __init arm_enable_runtime_services(void) > efi_native_runtime_setup(); > set_bit(EFI_RUNTIME_SERVICES, &efi.flags); > > + if (IS_ENABLED(CONFIG_ARM64)) > + register_restart_handler(&efi_restart_nb); > + > return 0; > } > early_initcall(arm_enable_runtime_services);
On Fri, Jan 28, 2022 at 02:50:26PM +0100, Krzysztof Adamski wrote: > On EFI enabled arm64 systems, efi_reboot was called before > do_kernel_restart, completely omitting the reset_handlers functionality. As I pointed out before, per the EFI spec we *need* to do this before any other restart mechanism so that anything which EFI ties to the restart actually occurs as expected -- e.g. UpdateCapsule(), as the comment says. AFAICT, either: * The other restart handlers have lower priority, in which case they'll be called after this anyway, and in such cases this patch is not necessary. * At least one other restart handler has higher priority, and the EFI restart isn't actually used, and so any functionaltiy tied to the EFI restart will not work on that machine. > By registering efi_reboot as part of the chain with slightly elevated > priority, we make it run before the default handler but still allow > plugging in other handlers. > Thanks to that, things like gpio_restart, restart handlers in > watchdog_core, mmc or mtds are working on those platforms. On which platforms is it necessary that these are used in preference to the EFI restart? Can you please give a specific example? If there's a specific platform that needs this, then we should call that out and explain why the EFI restart isn't actually required on that (or if it is, and functionality is broken, why that's acceptable). Otherwise this patch is making this logic more complicated *and* making it possible to have problems which we avoid by construction today, without any actual benefit. Thanks, Mark. > The priority 130 is one higher than PSCI, to overrule that but still > allow to easily register higher prio handlers, if needed. > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > --- > > Changes in v2: > - Register the handler in EFI code, instead of arm64 setup.c > - Remove the contdition from the handler - it should be run in all > cases when it is registered > - Bump the priority to 130 to make it completly obious this should be > run before PSCI (which has priority of 129) > > arch/arm64/kernel/process.c | 7 ------- > drivers/firmware/efi/arm-runtime.c | 21 +++++++++++++++++++++ > 2 files changed, 21 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 5369e649fa79..b86ef77bb0c8 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -130,13 +130,6 @@ void machine_restart(char *cmd) > local_irq_disable(); > smp_send_stop(); > > - /* > - * UpdateCapsule() depends on the system being reset via > - * ResetSystem(). > - */ > - if (efi_enabled(EFI_RUNTIME_SERVICES)) > - efi_reboot(reboot_mode, NULL); > - > /* Now call the architecture specific reboot code. */ > do_kernel_restart(cmd); > > diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c > index 3359ae2adf24..b9a2cdbe80b4 100644 > --- a/drivers/firmware/efi/arm-runtime.c > +++ b/drivers/firmware/efi/arm-runtime.c > @@ -80,6 +80,24 @@ static bool __init efi_virtmap_init(void) > return true; > } > > +static int efi_restart(struct notifier_block *nb, unsigned long action, > + void *data) > +{ > + /* > + * UpdateCapsule() depends on the system being reset via > + * ResetSystem(). > + */ > + efi_reboot(reboot_mode, NULL); > + > + return NOTIFY_DONE; > +} > + > +static struct notifier_block efi_restart_nb = { > + .notifier_call = efi_restart, > + /* We want this to take priority over PSCI which has priority of 129. */ > + .priority = 130, > +}; > + > /* > * Enable the UEFI Runtime Services if all prerequisites are in place, i.e., > * non-early mapping of the UEFI system table and virtual mappings for all > @@ -148,6 +166,9 @@ static int __init arm_enable_runtime_services(void) > efi_native_runtime_setup(); > set_bit(EFI_RUNTIME_SERVICES, &efi.flags); > > + if (IS_ENABLED(CONFIG_ARM64)) > + register_restart_handler(&efi_restart_nb); > + > return 0; > } > early_initcall(arm_enable_runtime_services); > -- > 2.34.1 >
> + /* We want this to take priority over PSCI which has priority of 129. */ > + .priority = 130, Is it an idea to add a #define for the PSCI priority somewhere and use here the define + 1? Hardcoded priorities look a bit fragile to me.
Hi Mark, Thank you for your feedback. I don't know UEFI specification that much, it is quite complicated so maybe I am misunderstnading something. Let's clarify. Dnia Fri, Jan 28, 2022 at 04:01:41PM +0000, Mark Rutland napisał(a): >On Fri, Jan 28, 2022 at 02:50:26PM +0100, Krzysztof Adamski wrote: >> On EFI enabled arm64 systems, efi_reboot was called before >> do_kernel_restart, completely omitting the reset_handlers functionality. > >As I pointed out before, per the EFI spec we *need* to do this before any other >restart mechanism so that anything which EFI ties to the restart actually >occurs as expected -- e.g. UpdateCapsule(), as the comment says. >AFAICT, either: > >* The other restart handlers have lower priority, in which case they'll be > called after this anyway, and in such cases this patch is not necessary. But efi_reboot calls ResetSystem(), which according to the spec: "Resets all processors and devices and reboots the system." So nothing should be able to run *after* this call. Thus, none of the reset handlers will ever run on a system where efi_reboot() is used (with current, unmodified kernel code) so this case is invalid. > >* At least one other restart handler has higher priority, and the EFI restart > isn't actually used, and so any functionaltiy tied to the EFI restart will > not work on that machine. If we use the restart handlers only to reset the system, this is indeed true. But technically, restart handlers support the scenario where the handler does some action that does not do reset of the whole system and passes the control further down the chain, eventually reaching a handler that will reset the whole system. This can be done on non-uefi systems without problems but it doesn't work on UEFI bases arm64 systems and this is a problem for us. In other words, I would like to be able to run a restart handler on EFI based ARM64 systems, just like I can on other systems, just for its "side effects", not to do the actual reboot. Current code disables this possibility on an ARM64 EFI system. > >> By registering efi_reboot as part of the chain with slightly elevated >> priority, we make it run before the default handler but still allow >> plugging in other handlers. >> Thanks to that, things like gpio_restart, restart handlers in >> watchdog_core, mmc or mtds are working on those platforms. > >On which platforms is it necessary that these are used in preference to the EFI >restart? Can you please give a specific example? >If there's a specific platform that needs this, then we should call that out >and explain why the EFI restart isn't actually required on that (or if it is, >and functionality is broken, why that's acceptable). Again, I'm not saying EFI restart is not required. I would just like to have the flexibility I have on other architectures, and run some code just before restart. My understanding is that pwrseq_emmc.c driver does exactly that, for example, but there are also other possibilities: - using gpio-restart to notify external components about reset of the CPU - starting an external watchdog that makes sure we are not stuck in the reset procedure, etc. >Otherwise this patch is making this logic more complicated *and* making it >possible to have problems which we avoid by construction today, without any >actual benefit. The benefit for me is this added flexibility and unification between architectures. On other systems, like ARM32 or non-EFI ARM64 I can insert a custom reset handler to do some actions just before restart and on EFI based ARM64, I can't. You could argue that restart handlers were not created for that but they suit this purpose ideally and it wouldn't make much sense (in my opinion) to add yet another notifier chain that would run before reset notifiers, for code that is not supposed to reset the whole system and this is exacly what I would have to do if efi_reboot() is forced to be called before all handlers. Best regards, Krzysztof Adamski
Dnia Fri, Jan 28, 2022 at 08:29:06PM +0100, Wolfram Sang napisał(a): > >> + /* We want this to take priority over PSCI which has priority of 129. */ >> + .priority = 130, > >Is it an idea to add a #define for the PSCI priority somewhere and use >here the define + 1? Hardcoded priorities look a bit fragile to me. > You are right. Our priority policy for restart handlers isn't really good and it is hard to choose right priority just about every time a handler is added. That being said, after reading Mark's argumentation, I now thing that just being before PSCI is not enough. I think that a much higher priority should be used as it is not normal situation that you would like to run something before efi_reboot(). I would really still like to move the efi_reboot() to the restart_handler even if you consider running some code before efi_reboot() crazy. Since 255 is max priorty but using it basically does not make sense (as this would let to the exact same situation we have now). I would use something like 250, or even 254, just to indicate that we know that most certainly nothing should be run before efi_reboot(), but still allow those silly people like me, to do what they want in their system, without the need to run the custom kernel. I think we could even add a proper comment, so it woudld become something like: /** * If you are running UEFI based system, you most certainly should let * efi_reboot() do a reset for you. If you think you know better, we * leave you a window of opportunity here by not using maximal priorty * here. */ .priority = 250, What is the downside of doing that? That we will run through atomic notfier chain instead of calling efi_reboot directly? Sure this is slightly more complicated but it works on all our platforms and is battle proven and we don't worry about that there. And the upside is that we give people possibility to use their beloved mechanism if they really like to. Because flexibility is a good thing. What do you think? Krzysztof
Hi Krzysztof, > I would use something like 250, or even 254, just to indicate that we > know that most certainly nothing should be run before efi_reboot(), but > still allow those silly people like me, to do what they want in their > system, without the need to run the custom kernel. I think we could even > add a proper comment, so it woudld become something like: > > /** > * If you are running UEFI based system, you most certainly should let > * efi_reboot() do a reset for you. If you think you know better, we > * leave you a window of opportunity here by not using maximal priorty > * here. > */ > .priority = 250, For your patchset, this seems good enough for me because it is decoupled from PSCI now. I still think a set of defines should be collected in linux/reboot.h so they can be used in reboot handlers. This is a different patch series, though. > What is the downside of doing that? That we will run through atomic > notfier chain instead of calling efi_reboot directly? Sure this is > slightly more complicated but it works on all our platforms and is > battle proven and we don't worry about that there. And the upside is > that we give people possibility to use their beloved mechanism if they > really like to. Because flexibility is a good thing. I agree with the upside having more value than the downside. Happy hacking, Wolfram
On Fri, Jan 28, 2022 at 11:05:32PM +0100, Krzysztof Adamski wrote: > Hi Mark, > > Thank you for your feedback. I don't know UEFI specification that much, > it is quite complicated so maybe I am misunderstnading something. Let's > clarify. > > Dnia Fri, Jan 28, 2022 at 04:01:41PM +0000, Mark Rutland napisał(a): > > On Fri, Jan 28, 2022 at 02:50:26PM +0100, Krzysztof Adamski wrote: > > > On EFI enabled arm64 systems, efi_reboot was called before > > > do_kernel_restart, completely omitting the reset_handlers functionality. > > > > As I pointed out before, per the EFI spec we *need* to do this before any other > > restart mechanism so that anything which EFI ties to the restart actually > > occurs as expected -- e.g. UpdateCapsule(), as the comment says. > > AFAICT, either: > > > > * The other restart handlers have lower priority, in which case they'll be > > called after this anyway, and in such cases this patch is not necessary. > > But efi_reboot calls ResetSystem(), which according to the spec: > "Resets all processors and devices and reboots the system." > > So nothing should be able to run *after* this call. Thus, none of the > reset handlers will ever run on a system where efi_reboot() is used > (with current, unmodified kernel code) so this case is invalid. Yes; that was the point I was making: *in this case* it doesn't matter at all, and therefore *in this case* the existing code is fine. > > * At least one other restart handler has higher priority, and the EFI restart > > isn't actually used, and so any functionaltiy tied to the EFI restart will > > not work on that machine. > > If we use the restart handlers only to reset the system, this is indeed > true. But technically, restart handlers support the scenario where the > handler does some action that does not do reset of the whole system and > passes the control further down the chain, eventually reaching a handler > that will reset the whole system. > This can be done on non-uefi systems without problems but it doesn't > work on UEFI bases arm64 systems and this is a problem for us. > > In other words, I would like to be able to run a restart handler on EFI > based ARM64 systems, just like I can on other systems, just for its > "side effects", not to do the actual reboot. Current code disables this > possibility on an ARM64 EFI system. It sounds like two things are being conflated here: 1) A *notification* that a restart will subsequently occur. 2) A *request* to initiate a restart. IIUC (1) is supposed to be handled by the existing reboot notifier mechanism (see the reboot_notifier_list) which *is* invoked prior to the EFI reboot today. IMO, using restart handlers as notifiers is an abuse of the interface, and that's the fundamental problem. What am I missing? > > > By registering efi_reboot as part of the chain with slightly elevated > > > priority, we make it run before the default handler but still allow > > > plugging in other handlers. > > > Thanks to that, things like gpio_restart, restart handlers in > > > watchdog_core, mmc or mtds are working on those platforms. > > > > On which platforms is it necessary that these are used in preference to the EFI > > restart? Can you please give a specific example? > > > If there's a specific platform that needs this, then we should call that out > > and explain why the EFI restart isn't actually required on that (or if it is, > > and functionality is broken, why that's acceptable). > > Again, I'm not saying EFI restart is not required. I would just like to > have the flexibility I have on other architectures, and run some code > just before restart. My understanding is that pwrseq_emmc.c driver does > exactly that, for example, but there are also other possibilities: > - using gpio-restart to notify external components about reset of the > CPU > - starting an external watchdog that makes sure we are not stuck in the > reset procedure, etc. As above, IIUC those notification cases are supposed to be handled with reboot notifiers, which are already supported in generic code across all architectures, and should run before the EFI restart. If there's a case where they're not, that's either an oversight or an edge case that needs more consideration. > > Otherwise this patch is making this logic more complicated *and* making it > > possible to have problems which we avoid by construction today, without any > > actual benefit. > > The benefit for me is this added flexibility and unification between > architectures. On other systems, like ARM32 or non-EFI ARM64 I can > insert a custom reset handler to do some actions just before restart and > on EFI based ARM64, I can't. > > You could argue that restart handlers were not created for that but they > suit this purpose ideally and it wouldn't make much sense (in my > opinion) to add yet another notifier chain that would run before reset > notifiers, for code that is not supposed to reset the whole system and > this is exacly what I would have to do if efi_reboot() is forced to be > called before all handlers. As above, I think that's just using the wrong interface, and the reboot notifier mechanism *already* exists, so I'm really confused here. Have I misunderstood what you're trying to achieve? Is there some problem with the reboot notifier mechanism that I am unaware of? e.g. do we bypass them in some case where you think they're needed? Are you simply unaware of reboot notifiers? Thanks, Mark.
Dnia Tue, Feb 01, 2022 at 01:58:29PM +0000, Mark Rutland napisał(a): >> If we use the restart handlers only to reset the system, this is indeed >> true. But technically, restart handlers support the scenario where the >> handler does some action that does not do reset of the whole system and >> passes the control further down the chain, eventually reaching a handler >> that will reset the whole system. >> This can be done on non-uefi systems without problems but it doesn't >> work on UEFI bases arm64 systems and this is a problem for us. >> >> In other words, I would like to be able to run a restart handler on EFI >> based ARM64 systems, just like I can on other systems, just for its >> "side effects", not to do the actual reboot. Current code disables this >> possibility on an ARM64 EFI system. > >It sounds like two things are being conflated here: > >1) A *notification* that a restart will subsequently occur. >2) A *request* to initiate a restart. > >IIUC (1) is supposed to be handled by the existing reboot notifier mechanism >(see the reboot_notifier_list) which *is* invoked prior to the EFI reboot >today. > >IMO, using restart handlers as notifiers is an abuse of the interface, and >that's the fundamental problem. > >What am I missing? You are completly right. It is possible that I would like to be able to *abuse* the restart handlers as notifier. You are right that we have a reboot_notifier but it is not good enough for my usecase - it is only called, well, on reboot. It is not called in case of emergency_restart() so in case of a panic, this won't happen. It also is called much earlier than restart handlers which also makes a difference in some cases. So I see no other choice than to abuse the restart_handler mechanism for that. So, ideally, for that usecase, we would need a restart notifier that is called immidietely before restart handlers, is basically done in the same way as reset_handlers mechanism, but would simply be called differently and would use separate chain. But does it make sense to have that? Apart from this simple usecase that I have focused so far, I also have another one that I didn't mention before. As you know, even efi_reboot() has several types of resets it can trigger (warm, cold), depending on how "deep" the reset should be. In some cases, however, we have a need to escalate the reset/reboot even deeper, into full board reset which is performed by an external component - this cannot be done from EFI firmware so we have to do this from Linux, before efi_reboot is called. This has to be done also in case of a emergency_restart(). And obviosly we do not call efi_reboot() in this case, as the whole board goes into reset, including the CPU. This reset is, however, conditional - it does not replace the efi_reboot() which is still used in most cases. We use restart_handlers also for that, but this doesn't work on ARM64 with EFI. >> > Otherwise this patch is making this logic more complicated *and* making it >> > possible to have problems which we avoid by construction today, without any >> > actual benefit. >> >> The benefit for me is this added flexibility and unification between >> architectures. On other systems, like ARM32 or non-EFI ARM64 I can >> insert a custom reset handler to do some actions just before restart and >> on EFI based ARM64, I can't. >> >> You could argue that restart handlers were not created for that but they >> suit this purpose ideally and it wouldn't make much sense (in my >> opinion) to add yet another notifier chain that would run before reset >> notifiers, for code that is not supposed to reset the whole system and >> this is exacly what I would have to do if efi_reboot() is forced to be >> called before all handlers. > >As above, I think that's just using the wrong interface, and the reboot >notifier mechanism *already* exists, so I'm really confused here. > >Have I misunderstood what you're trying to achieve? > >Is there some problem with the reboot notifier mechanism that I am unaware of? >e.g. do we bypass them in some case where you think they're needed? > >Are you simply unaware of reboot notifiers? As explained above, I am aware of them but they have their limitation. They were designed for slightly different usecase. The are designed around blocking_notfier and are supposed to be called only in still safe environment during graceful reboot, quite early in the reboot process. If you need to do something just before reset and/or in case of a panic, you are out of luck. Of course reset_handlers have their limitations too. For one, they are called in the context of atomic_notifier and we can't assume the system is in good condition when it is called but trying to flip a GPIO line or do a write to MMIO register is a sane thing to do here. Krzysztof
On Wed, 2 Feb 2022 at 13:41, Krzysztof Adamski <krzysztof.adamski@nokia.com> wrote: > > Dnia Tue, Feb 01, 2022 at 01:58:29PM +0000, Mark Rutland napisał(a): > >> If we use the restart handlers only to reset the system, this is indeed > >> true. But technically, restart handlers support the scenario where the > >> handler does some action that does not do reset of the whole system and > >> passes the control further down the chain, eventually reaching a handler > >> that will reset the whole system. > >> This can be done on non-uefi systems without problems but it doesn't > >> work on UEFI bases arm64 systems and this is a problem for us. > >> > >> In other words, I would like to be able to run a restart handler on EFI > >> based ARM64 systems, just like I can on other systems, just for its > >> "side effects", not to do the actual reboot. Current code disables this > >> possibility on an ARM64 EFI system. > > > >It sounds like two things are being conflated here: > > > >1) A *notification* that a restart will subsequently occur. > >2) A *request* to initiate a restart. > > > >IIUC (1) is supposed to be handled by the existing reboot notifier mechanism > >(see the reboot_notifier_list) which *is* invoked prior to the EFI reboot > >today. > > > >IMO, using restart handlers as notifiers is an abuse of the interface, and > >that's the fundamental problem. > > > >What am I missing? > > You are completly right. It is possible that I would like to be able to > *abuse* the restart handlers as notifier. You are right that we have a > reboot_notifier but it is not good enough for my usecase - it is only > called, well, on reboot. It is not called in case of emergency_restart() > so in case of a panic, this won't happen. It also is called much earlier > than restart handlers which also makes a difference in some cases. So I > see no other choice than to abuse the restart_handler mechanism for that. > Why would such a platform implement ResetSystem() in the first place if it cannot be used? So the right solution here is for the firmware to publish a EFI_RT_PROPERTIES_TABLE that describes ResetSystem() as unsupported, and Linux will happily disregard it and try something else. Btw please cc linux-efi@vger.kernel.org and myself on future EFI issues. I found this thread by accident.
Dnia Wed, Feb 02, 2022 at 03:01:37PM +0100, Ard Biesheuvel napisał(a): >On Wed, 2 Feb 2022 at 13:41, Krzysztof Adamski ><krzysztof.adamski@nokia.com> wrote: >> >> Dnia Tue, Feb 01, 2022 at 01:58:29PM +0000, Mark Rutland napisał(a): >> >> If we use the restart handlers only to reset the system, this is indeed >> >> true. But technically, restart handlers support the scenario where the >> >> handler does some action that does not do reset of the whole system and >> >> passes the control further down the chain, eventually reaching a handler >> >> that will reset the whole system. >> >> This can be done on non-uefi systems without problems but it doesn't >> >> work on UEFI bases arm64 systems and this is a problem for us. >> >> >> >> In other words, I would like to be able to run a restart handler on EFI >> >> based ARM64 systems, just like I can on other systems, just for its >> >> "side effects", not to do the actual reboot. Current code disables this >> >> possibility on an ARM64 EFI system. >> > >> >It sounds like two things are being conflated here: >> > >> >1) A *notification* that a restart will subsequently occur. >> >2) A *request* to initiate a restart. >> > >> >IIUC (1) is supposed to be handled by the existing reboot notifier mechanism >> >(see the reboot_notifier_list) which *is* invoked prior to the EFI reboot >> >today. >> > >> >IMO, using restart handlers as notifiers is an abuse of the interface, and >> >that's the fundamental problem. >> > >> >What am I missing? >> >> You are completly right. It is possible that I would like to be able to >> *abuse* the restart handlers as notifier. You are right that we have a >> reboot_notifier but it is not good enough for my usecase - it is only >> called, well, on reboot. It is not called in case of emergency_restart() >> so in case of a panic, this won't happen. It also is called much earlier >> than restart handlers which also makes a difference in some cases. So I >> see no other choice than to abuse the restart_handler mechanism for that. >> > >Why would such a platform implement ResetSystem() in the first place >if it cannot be used? > >So the right solution here is for the firmware to publish a >EFI_RT_PROPERTIES_TABLE that describes ResetSystem() as unsupported, >and Linux will happily disregard it and try something else. The firmware is generic but the problem is specific to one usecase of such a platform. In other words, the firmware is written for the SoC and it does not know about other parts on the board, outside of SoC. It doesn't make much sense to provide the support for such custom device in the firmware. Please also note that the ResetSystem is supported. As I said, the normal, typical reset is done via EFI, via ResetSystem. Ony if you have a special case, where not only SoC, but also some other components in the system have to be reset, then you want to change the reset type. We also have one case of abusing the restart_handler as restart notifier in the kernel in the form of drivers/mmc/core/pwrseq_emmc.c. I would just like to be able to do this also on EFI based ARM64 systems. Is that crazy? :) >Btw please cc linux-efi@vger.kernel.org and myself on future EFI >issues. I found this thread by accident. Sure, sorry for that. Krzysztof
Dnia Wed, Feb 02, 2022 at 05:47:33PM +0100, Krzysztof Adamski napisał(a): >Dnia Wed, Feb 02, 2022 at 03:01:37PM +0100, Ard Biesheuvel napisał(a): >>On Wed, 2 Feb 2022 at 13:41, Krzysztof Adamski >><krzysztof.adamski@nokia.com> wrote: >>> >>>Dnia Tue, Feb 01, 2022 at 01:58:29PM +0000, Mark Rutland napisał(a): >>>>> If we use the restart handlers only to reset the system, this is indeed >>>>> true. But technically, restart handlers support the scenario where the >>>>> handler does some action that does not do reset of the whole system and >>>>> passes the control further down the chain, eventually reaching a handler >>>>> that will reset the whole system. >>>>> This can be done on non-uefi systems without problems but it doesn't >>>>> work on UEFI bases arm64 systems and this is a problem for us. >>>>> >>>>> In other words, I would like to be able to run a restart handler on EFI >>>>> based ARM64 systems, just like I can on other systems, just for its >>>>> "side effects", not to do the actual reboot. Current code disables this >>>>> possibility on an ARM64 EFI system. >>>> >>>>It sounds like two things are being conflated here: >>>> >>>>1) A *notification* that a restart will subsequently occur. >>>>2) A *request* to initiate a restart. >>>> >>>>IIUC (1) is supposed to be handled by the existing reboot notifier mechanism >>>>(see the reboot_notifier_list) which *is* invoked prior to the EFI reboot >>>>today. >>>> >>>>IMO, using restart handlers as notifiers is an abuse of the interface, and >>>>that's the fundamental problem. >>>> >>>>What am I missing? >>> >>>You are completly right. It is possible that I would like to be able to >>>*abuse* the restart handlers as notifier. You are right that we have a >>>reboot_notifier but it is not good enough for my usecase - it is only >>>called, well, on reboot. It is not called in case of emergency_restart() >>>so in case of a panic, this won't happen. It also is called much earlier >>>than restart handlers which also makes a difference in some cases. So I >>>see no other choice than to abuse the restart_handler mechanism for that. >>> >> >>Why would such a platform implement ResetSystem() in the first place >>if it cannot be used? >> >>So the right solution here is for the firmware to publish a >>EFI_RT_PROPERTIES_TABLE that describes ResetSystem() as unsupported, >>and Linux will happily disregard it and try something else. > >The firmware is generic but the problem is specific to one usecase of >such a platform. In other words, the firmware is written for the SoC and >it does not know about other parts on the board, outside of SoC. It >doesn't make much sense to provide the support for such custom device in >the firmware. > >Please also note that the ResetSystem is supported. As I said, the >normal, typical reset is done via EFI, via ResetSystem. Ony if you have >a special case, where not only SoC, but also some other components in >the system have to be reset, then you want to change the reset type. > >We also have one case of abusing the restart_handler as restart notifier >in the kernel in the form of drivers/mmc/core/pwrseq_emmc.c. I would >just like to be able to do this also on EFI based ARM64 systems. Is that >crazy? :) Sorry, I forgot to mention another argument - doing that in the firmware is SoC specific while doing that in the kernel is platform agnostic. We use the same code on different platforms, architectures, etc. Now, I don't understand why it is sane to have this functionality on other archs, when we use PSCI or whatever else reset mechanism, but it isn't if we use EFI. For the notification purpose, we acknowledge it might be beneficial to notify some code about the reboot (this is why we have the reboot_notifier functionality) but having such a notifier on reset, instead of reboot (i.e. at the end of the whole process and at all possible cases where we reset our machine), seems strange. I don't get it. Krzysztof
Hello Ard, Mark, On 02/02/2022 15:01, Ard Biesheuvel wrote: >>>> In other words, I would like to be able to run a restart handler on EFI >>>> based ARM64 systems, just like I can on other systems, just for its >>>> "side effects", not to do the actual reboot. Current code disables this >>>> possibility on an ARM64 EFI system. >>> >>> It sounds like two things are being conflated here: >>> >>> 1) A *notification* that a restart will subsequently occur. >>> 2) A *request* to initiate a restart. >>> >>> IIUC (1) is supposed to be handled by the existing reboot notifier mechanism >>> (see the reboot_notifier_list) which *is* invoked prior to the EFI reboot >>> today. >>> >>> IMO, using restart handlers as notifiers is an abuse of the interface, and >>> that's the fundamental problem. >>> >>> What am I missing? >> >> You are completly right. It is possible that I would like to be able to >> *abuse* the restart handlers as notifier. You are right that we have a >> reboot_notifier but it is not good enough for my usecase - it is only >> called, well, on reboot. It is not called in case of emergency_restart() >> so in case of a panic, this won't happen. It also is called much earlier >> than restart handlers which also makes a difference in some cases. So I >> see no other choice than to abuse the restart_handler mechanism for that. >> > > Why would such a platform implement ResetSystem() in the first place > if it cannot be used? > > So the right solution here is for the firmware to publish a > EFI_RT_PROPERTIES_TABLE that describes ResetSystem() as unsupported, > and Linux will happily disregard it and try something else. Let me outline the use case once again: we have cases where different SoCs are integrated into more complicated FPGA based systems where FPGA controls the reset sequence of the whole system. Those SoCs are of different ARCHs and only one of them currently has EFI. So the implementation is largely platform-independent and is implemented in Linux. Do you suggest that in EFI case we cannot implement a conditional reset implementation (native EFI reset vs system-wide FPGA-controlled) in Linux any longer (because register_restart_handler() doesn't work for EFI based ARM64 system) and need to reimplement very special solution in EFI?
Hello Mark, Ard, On 01/02/2022 14:58, Mark Rutland wrote: >> You could argue that restart handlers were not created for that but they >> suit this purpose ideally and it wouldn't make much sense (in my >> opinion) to add yet another notifier chain that would run before reset >> notifiers, for code that is not supposed to reset the whole system and >> this is exacly what I would have to do if efi_reboot() is forced to be >> called before all handlers. > > As above, I think that's just using the wrong interface, and the reboot > notifier mechanism *already* exists, so I'm really confused here. > > Have I misunderstood what you're trying to achieve? > > Is there some problem with the reboot notifier mechanism that I am unaware of? > e.g. do we bypass them in some case where you think they're needed? > > Are you simply unaware of reboot notifiers? Could you please check the simple case of pwrseq_emmc.c? While that's currently the only example of this kind upstream I can imagine further use-cases, especially in storage area like above. Would you suggest it's illegal usage of register_restart_handler()? Do we need to fix pwrseq_emmc.c by introducing new atomic notifier chain which will be called before restart handlers, so that it works on emergency_restart()?
On 2/15/22 00:44, Alexander Sverdlin wrote: > Hello Mark, Ard, > > On 01/02/2022 14:58, Mark Rutland wrote: >>> You could argue that restart handlers were not created for that but they >>> suit this purpose ideally and it wouldn't make much sense (in my >>> opinion) to add yet another notifier chain that would run before reset >>> notifiers, for code that is not supposed to reset the whole system and >>> this is exacly what I would have to do if efi_reboot() is forced to be >>> called before all handlers. >> >> As above, I think that's just using the wrong interface, and the reboot >> notifier mechanism *already* exists, so I'm really confused here. >> >> Have I misunderstood what you're trying to achieve? >> >> Is there some problem with the reboot notifier mechanism that I am unaware of? >> e.g. do we bypass them in some case where you think they're needed? >> >> Are you simply unaware of reboot notifiers? > > Could you please check the simple case of pwrseq_emmc.c? > > While that's currently the only example of this kind upstream I can imagine > further use-cases, especially in storage area like above. > > Would you suggest it's illegal usage of register_restart_handler()? > Do we need to fix pwrseq_emmc.c by introducing new atomic notifier chain > which will be called before restart handlers, so that it works on > emergency_restart()? > Abuse isn't just about using an API for something it isn't originally intended for, abuse is also to intentionally _not_ use an API, as it is currently done by the EFI restart code for arm64. Also, keep in mind that the same argument (our restart handler _must_ be executed under all circumstances and does therefore not use the restart API) is likely going to be used again in the future. All it takes is for some other standard (or chip vendor, for that matter) to declare their restart handler mandatory if present. That means there will be other non-users of the restart API in the future, and you simply can not rely on it being used. That was clearly not the intention of the restart API - quite the opposite - but it is what it is. Given that, I'd suggest to cut your losses and try to find another solution for your problem. That may be to introduce yet another API, one that is called before the EFI restart handling but that is always called, unlike reboot notifiers, or simply stick with out-of-tree code. Guenter
Dnia Tue, Feb 15, 2022 at 06:30:26AM -0800, Guenter Roeck napisał(a): >On 2/15/22 00:44, Alexander Sverdlin wrote: >>Hello Mark, Ard, >> >>On 01/02/2022 14:58, Mark Rutland wrote: >>>>You could argue that restart handlers were not created for that but they >>>>suit this purpose ideally and it wouldn't make much sense (in my >>>>opinion) to add yet another notifier chain that would run before reset >>>>notifiers, for code that is not supposed to reset the whole system and >>>>this is exacly what I would have to do if efi_reboot() is forced to be >>>>called before all handlers. >>> >>>As above, I think that's just using the wrong interface, and the reboot >>>notifier mechanism *already* exists, so I'm really confused here. >>> >>>Have I misunderstood what you're trying to achieve? >>> >>>Is there some problem with the reboot notifier mechanism that I am unaware of? >>>e.g. do we bypass them in some case where you think they're needed? >>> >>>Are you simply unaware of reboot notifiers? >> >>Could you please check the simple case of pwrseq_emmc.c? >> >>While that's currently the only example of this kind upstream I can imagine >>further use-cases, especially in storage area like above. >> >>Would you suggest it's illegal usage of register_restart_handler()? >>Do we need to fix pwrseq_emmc.c by introducing new atomic notifier chain >>which will be called before restart handlers, so that it works on >>emergency_restart()? >> > >Abuse isn't just about using an API for something it isn't originally intended >for, abuse is also to intentionally _not_ use an API, as it is currently done >by the EFI restart code for arm64. Also, keep in mind that the same argument >(our restart handler _must_ be executed under all circumstances and does therefore >not use the restart API) is likely going to be used again in the future. All >it takes is for some other standard (or chip vendor, for that matter) to declare >their restart handler mandatory if present. Wait, but it is up to us to decide if we want to follow such standard or not. If we want to have code that is more flexible, nobody can refuse us to do so, right? None of the standards says that we can't support restart handlers in case of EFI on ARM64, it was decided by kernel developers, not some vendors. We can change that. >Given that, I'd suggest to cut your losses and try to find another >solution for your problem. That may be to introduce yet another API, >one that is called before the EFI restart handling but that is always >called, unlike reboot notifiers, or simply stick with out-of-tree code. Sure I could create yet another API like you suggest but in practice it would be a copy of existing API as i would have to work exactly the same - would be called at the same time in the same situations. The only thing that would be different would be separate chain. But if we want to prevent registering some custom code to be run before efi_reboot(), that new API would have to be rejected as well, for the same reason. So what is the point? Krzysztof
On 2/15/22 07:01, Krzysztof Adamski wrote: > Dnia Tue, Feb 15, 2022 at 06:30:26AM -0800, Guenter Roeck napisał(a): >> On 2/15/22 00:44, Alexander Sverdlin wrote: >>> Hello Mark, Ard, >>> >>> On 01/02/2022 14:58, Mark Rutland wrote: >>>>> You could argue that restart handlers were not created for that but they >>>>> suit this purpose ideally and it wouldn't make much sense (in my >>>>> opinion) to add yet another notifier chain that would run before reset >>>>> notifiers, for code that is not supposed to reset the whole system and >>>>> this is exacly what I would have to do if efi_reboot() is forced to be >>>>> called before all handlers. >>>> >>>> As above, I think that's just using the wrong interface, and the reboot >>>> notifier mechanism *already* exists, so I'm really confused here. >>>> >>>> Have I misunderstood what you're trying to achieve? >>>> >>>> Is there some problem with the reboot notifier mechanism that I am unaware of? >>>> e.g. do we bypass them in some case where you think they're needed? >>>> >>>> Are you simply unaware of reboot notifiers? >>> >>> Could you please check the simple case of pwrseq_emmc.c? >>> >>> While that's currently the only example of this kind upstream I can imagine >>> further use-cases, especially in storage area like above. >>> >>> Would you suggest it's illegal usage of register_restart_handler()? >>> Do we need to fix pwrseq_emmc.c by introducing new atomic notifier chain >>> which will be called before restart handlers, so that it works on >>> emergency_restart()? >>> >> >> Abuse isn't just about using an API for something it isn't originally intended >> for, abuse is also to intentionally _not_ use an API, as it is currently done >> by the EFI restart code for arm64. Also, keep in mind that the same argument >> (our restart handler _must_ be executed under all circumstances and does therefore >> not use the restart API) is likely going to be used again in the future. All >> it takes is for some other standard (or chip vendor, for that matter) to declare >> their restart handler mandatory if present. > > Wait, but it is up to us to decide if we want to follow such standard or > not. If we want to have code that is more flexible, nobody can refuse us > to do so, right? None of the standards says that we can't support > restart handlers in case of EFI on ARM64, it was decided by kernel > developers, not some vendors. We can change that. > Of course it was decided by kernel developers. Point is that they use the EFI standard as argument for bypassing the API. What I am saying is that others can (and likely will, since the flood doors have been opened) do the same in the future, using the same line of arguments. >> Given that, I'd suggest to cut your losses and try to find another >> solution for your problem. That may be to introduce yet another API, >> one that is called before the EFI restart handling but that is always >> called, unlike reboot notifiers, or simply stick with out-of-tree code. > > Sure I could create yet another API like you suggest but in practice it > would be a copy of existing API as i would have to work exactly the > same - would be called at the same time in the same situations. The only > thing that would be different would be separate chain. Correct. > But if we want to prevent registering some custom code to be run before > efi_reboot(), that new API would have to be rejected as well, for the > same reason. So what is the point? > Ah, yes, you are right. The emmc example does reset the emmc, after all, which one could use as argument that it "violates" the EFI mandate. Sorry, I guess you'll be stuck with out-of-tree code (and, realistically, so is everyone using emmc in an arm64 based system with an EFI restart handler which does not implement emmc reset). Actually, turns out that the emmc restart handling code is not reliable anyway, since for example x86 doesn't use/support the restart handler call chain, and neither do several other architectures. Interestingly, in this context, x86 isn't as inflexible as arm64 and does support other means to reset the system even in the presence of EFI (and actually seems to prefer ACPI reset over EFI reset unless I misunderstand the code). Other options for you might be to disable EFI restart handling in your system (assuming that is possible), or to implement the necessary code as part of the EFI restart handler, ie outside Linux, again if that is possible. Guenter
On Tue, 15 Feb 2022 at 17:57, Guenter Roeck <linux@roeck-us.net> wrote: > > On 2/15/22 07:01, Krzysztof Adamski wrote: > > Dnia Tue, Feb 15, 2022 at 06:30:26AM -0800, Guenter Roeck napisał(a): > >> On 2/15/22 00:44, Alexander Sverdlin wrote: > >>> Hello Mark, Ard, > >>> > >>> On 01/02/2022 14:58, Mark Rutland wrote: > >>>>> You could argue that restart handlers were not created for that but they > >>>>> suit this purpose ideally and it wouldn't make much sense (in my > >>>>> opinion) to add yet another notifier chain that would run before reset > >>>>> notifiers, for code that is not supposed to reset the whole system and > >>>>> this is exacly what I would have to do if efi_reboot() is forced to be > >>>>> called before all handlers. > >>>> > >>>> As above, I think that's just using the wrong interface, and the reboot > >>>> notifier mechanism *already* exists, so I'm really confused here. > >>>> > >>>> Have I misunderstood what you're trying to achieve? > >>>> > >>>> Is there some problem with the reboot notifier mechanism that I am unaware of? > >>>> e.g. do we bypass them in some case where you think they're needed? > >>>> > >>>> Are you simply unaware of reboot notifiers? > >>> > >>> Could you please check the simple case of pwrseq_emmc.c? > >>> > >>> While that's currently the only example of this kind upstream I can imagine > >>> further use-cases, especially in storage area like above. > >>> > >>> Would you suggest it's illegal usage of register_restart_handler()? > >>> Do we need to fix pwrseq_emmc.c by introducing new atomic notifier chain > >>> which will be called before restart handlers, so that it works on > >>> emergency_restart()? > >>> > >> > >> Abuse isn't just about using an API for something it isn't originally intended > >> for, abuse is also to intentionally _not_ use an API, as it is currently done > >> by the EFI restart code for arm64. Also, keep in mind that the same argument > >> (our restart handler _must_ be executed under all circumstances and does therefore > >> not use the restart API) is likely going to be used again in the future. All > >> it takes is for some other standard (or chip vendor, for that matter) to declare > >> their restart handler mandatory if present. > > > > Wait, but it is up to us to decide if we want to follow such standard or > > not. If we want to have code that is more flexible, nobody can refuse us > > to do so, right? None of the standards says that we can't support > > restart handlers in case of EFI on ARM64, it was decided by kernel > > developers, not some vendors. We can change that. > > > > Of course it was decided by kernel developers. Point is that they use > the EFI standard as argument for bypassing the API. What I am saying is > that others can (and likely will, since the flood doors have been opened) > do the same in the future, using the same line of arguments. > I don't think anyone was doing what you describe here. My primary point was that platforms should not implement and expose EFI reset if that implementation fails to deal with the platform's peripherals adequately. > >> Given that, I'd suggest to cut your losses and try to find another > >> solution for your problem. That may be to introduce yet another API, > >> one that is called before the EFI restart handling but that is always > >> called, unlike reboot notifiers, or simply stick with out-of-tree code. > > > > Sure I could create yet another API like you suggest but in practice it > > would be a copy of existing API as i would have to work exactly the > > same - would be called at the same time in the same situations. The only > > thing that would be different would be separate chain. > > Correct. > > > But if we want to prevent registering some custom code to be run before > efi_reboot(), that new API would have to be rejected as well, for the > > same reason. So what is the point? > > > > Ah, yes, you are right. The emmc example does reset the emmc, after all, > which one could use as argument that it "violates" the EFI mandate. > Sorry, I guess you'll be stuck with out-of-tree code (and, realistically, > so is everyone using emmc in an arm64 based system with an EFI restart > handler which does not implement emmc reset). Actually, turns out that > the emmc restart handling code is not reliable anyway, since for example > x86 doesn't use/support the restart handler call chain, and neither > do several other architectures. > Rich firmware like EFI and ACPI implies that the firmware knows how to manage the hardware. > Interestingly, in this context, x86 isn't as inflexible as arm64 and does > support other means to reset the system even in the presence of EFI > (and actually seems to prefer ACPI reset over EFI reset unless I > misunderstand the code). > No, correct. This is primarily for historical reasons and for parity with Windows/x86 which does the same (which means that on many cheap Wintel laptops, ACPI reset is the only thing that works) > Other options for you might be to disable EFI restart handling in your > system (assuming that is possible), or to implement the necessary code > as part of the EFI restart handler, ie outside Linux, again if that is > possible. > Either implement EFI reset properly, or not at all. Adding code to the OS that forces it to reason about whether or not EFI reset can be called safely simply means that the EFI implementation is broken and should probably be avoided entirely.
Dnia Tue, Feb 15, 2022 at 06:03:30PM +0100, Ard Biesheuvel napisał(a): >> > But if we want to prevent registering some custom code to be run before > efi_reboot(), that new API would have to be rejected as well, for the >> > same reason. So what is the point? >> > >> >> Ah, yes, you are right. The emmc example does reset the emmc, after all, >> which one could use as argument that it "violates" the EFI mandate. >> Sorry, I guess you'll be stuck with out-of-tree code (and, realistically, >> so is everyone using emmc in an arm64 based system with an EFI restart >> handler which does not implement emmc reset). Actually, turns out that >> the emmc restart handling code is not reliable anyway, since for example >> x86 doesn't use/support the restart handler call chain, and neither >> do several other architectures. >> > >Rich firmware like EFI and ACPI implies that the firmware knows how to >manage the hardware. > In an ideal world, probably yes. When I move there, I will probably have less concerns :) > >> Other options for you might be to disable EFI restart handling in your >> system (assuming that is possible), or to implement the necessary code >> as part of the EFI restart handler, ie outside Linux, again if that is >> possible. >> > >Either implement EFI reset properly, or not at all. Adding code to the >OS that forces it to reason about whether or not EFI reset can be >called safely simply means that the EFI implementation is broken and >should probably be avoided entirely. That is black-and-white thinking. There are shades of gray, too :) Imagine this sitation - you have a SoC and the vendor delivers this SoC together with support and code for that SoC. You use it to build some platform around that SoC but that platform is much more complicated than the basic one. It is custom and specific to your needs. The vendor provided firmware, together with the EFI implementation can handle reseting the SoC without problems but you also have some additional parts in your platform you would like to handle. Why, in such a case, I should not use the existing EFI implementation and just build on top of that? Avoiding using it entirely does not seem reasonable to me. It might also not be possible as you might not know all the details to reimplement it. On PC/Server world, that approach might be feasible - the vendor knows the usecase for their product well enough to support all the cases. But ARM64 is being often used for other, so called "embedded", usecases. People working in "embedded" often have unique needs and I do understand that mainline kernel cannot support all of them. But when supporting them is easy and does not require some ugly hacks to be added, why not? For years we were implementing ways to overcome limitations in hardware and vendor software implementations. restart_handler mechanism is quite pretty example of that, one which isn't really hacky, does not introduce maintainabity problems if widely used. Why not support it on ARM64 as well? Only because in ideal world it wouldn't be needed? :) I understand it should not be needed in EFI world and there are approaches that could be used instead of that one, but they all seem like taking a sledgehammer to crack a nut. Krzysztof
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 5369e649fa79..b86ef77bb0c8 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -130,13 +130,6 @@ void machine_restart(char *cmd) local_irq_disable(); smp_send_stop(); - /* - * UpdateCapsule() depends on the system being reset via - * ResetSystem(). - */ - if (efi_enabled(EFI_RUNTIME_SERVICES)) - efi_reboot(reboot_mode, NULL); - /* Now call the architecture specific reboot code. */ do_kernel_restart(cmd); diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c index 3359ae2adf24..b9a2cdbe80b4 100644 --- a/drivers/firmware/efi/arm-runtime.c +++ b/drivers/firmware/efi/arm-runtime.c @@ -80,6 +80,24 @@ static bool __init efi_virtmap_init(void) return true; } +static int efi_restart(struct notifier_block *nb, unsigned long action, + void *data) +{ + /* + * UpdateCapsule() depends on the system being reset via + * ResetSystem(). + */ + efi_reboot(reboot_mode, NULL); + + return NOTIFY_DONE; +} + +static struct notifier_block efi_restart_nb = { + .notifier_call = efi_restart, + /* We want this to take priority over PSCI which has priority of 129. */ + .priority = 130, +}; + /* * Enable the UEFI Runtime Services if all prerequisites are in place, i.e., * non-early mapping of the UEFI system table and virtual mappings for all @@ -148,6 +166,9 @@ static int __init arm_enable_runtime_services(void) efi_native_runtime_setup(); set_bit(EFI_RUNTIME_SERVICES, &efi.flags); + if (IS_ENABLED(CONFIG_ARM64)) + register_restart_handler(&efi_restart_nb); + return 0; } early_initcall(arm_enable_runtime_services);
On EFI enabled arm64 systems, efi_reboot was called before do_kernel_restart, completely omitting the reset_handlers functionality. By registering efi_reboot as part of the chain with slightly elevated priority, we make it run before the default handler but still allow plugging in other handlers. Thanks to that, things like gpio_restart, restart handlers in watchdog_core, mmc or mtds are working on those platforms. The priority 130 is one higher than PSCI, to overrule that but still allow to easily register higher prio handlers, if needed. Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> --- Changes in v2: - Register the handler in EFI code, instead of arm64 setup.c - Remove the contdition from the handler - it should be run in all cases when it is registered - Bump the priority to 130 to make it completly obious this should be run before PSCI (which has priority of 129) arch/arm64/kernel/process.c | 7 ------- drivers/firmware/efi/arm-runtime.c | 21 +++++++++++++++++++++ 2 files changed, 21 insertions(+), 7 deletions(-)