[PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
The UEFI specification requires for ExitBootServices() that "the boot services watchdog timer is disabled". We already disable the software watchdog. We should additionally disable the hardware watchdogs.
Reported-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- lib/efi_loader/efi_boottime.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 1823990d9b..e33821c93a 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -19,6 +19,7 @@ #include <u-boot/crc.h> #include <usb.h> #include <watchdog.h> +#include <wdt.h> #include <asm/global_data.h> #include <asm/setjmp.h> #include <linux/libfdt_env.h> @@ -2166,6 +2167,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle, /* Fix up caches for EFI payloads if necessary */ efi_exit_caches();
+ /* Disable watchdogs */ + efi_set_watchdog(0); + if IS_ENABLED(CONFIG_WDT) + wdt_stop_all(); + /* This stops all lingering devices */ bootm_disable_interrupts();
@@ -2181,9 +2187,6 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle, /* Recalculate CRC32 */ efi_update_table_header_crc32(&systab.hdr);
- /* Give the payload some time to boot */ - efi_set_watchdog(0); - WATCHDOG_RESET(); out: if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) { if (ret != EFI_SUCCESS)
The UEFI specification requires for ExitBootServices() that "the boot services watchdog timer is disabled". We already disable the software watchdog. We should additionally disable the hardware watchdogs.
Reported-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- lib/efi_loader/efi_boottime.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 1823990d9b..e33821c93a 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -19,6 +19,7 @@ #include <u-boot/crc.h> #include <usb.h> #include <watchdog.h> +#include <wdt.h> #include <asm/global_data.h> #include <asm/setjmp.h> #include <linux/libfdt_env.h> @@ -2166,6 +2167,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle, /* Fix up caches for EFI payloads if necessary */ efi_exit_caches();
+ /* Disable watchdogs */ + efi_set_watchdog(0); + if IS_ENABLED(CONFIG_WDT) + wdt_stop_all(); + /* This stops all lingering devices */ bootm_disable_interrupts();
@@ -2181,9 +2187,6 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle, /* Recalculate CRC32 */ efi_update_table_header_crc32(&systab.hdr);
- /* Give the payload some time to boot */ - efi_set_watchdog(0); - WATCHDOG_RESET(); out: if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) { if (ret != EFI_SUCCESS)
The UEFI specification requires for ExitBootServices() that "the boot services watchdog timer is disabled". We already disable the software watchdog. We should additionally disable the hardware watchdogs.
What about watchdogs that cannot be stopped? IIRC the IMX SoCs are like that.
-michael
From: Michael Walle michael@walle.cc Date: Tue, 9 Nov 2021 15:20:17 +0100
The UEFI specification requires for ExitBootServices() that "the boot services watchdog timer is disabled". We already disable the software watchdog. We should additionally disable the hardware watchdogs.
What about watchdogs that cannot be stopped? IIRC the IMX SoCs are like that.
You have to hope that your OS takes control of the watchdog quickly enough for the machine not to reset in between. Strictly speaking such a platform can not be fully compliant with the UEFI standard. In practice this doesn't really matter as the OS has to do this quickly enough if you're using a non-UEFI bootpath anyway.
Maybe somebody who cares enough can get the UEFI standard amended to handle this scenario. Maybe an interface can be added to the standard to provide more control over the watchdog such that the timeout can be set to a larger value before ExitBootServices() gets called. And add a way to keep the watchdog enabled on SoCs where it can be disabled. Last time this issue came up, someone pointed out that a watchdog that can be turned off isn't a proper watchdog. And indeed, turning the watchdog off when ExitBootServices() gets called means there is a time window where the watchdog isn't running and where the OS could hang forever.
Cheers,
Mark
Am 2021-11-09 15:46, schrieb Mark Kettenis:
From: Michael Walle michael@walle.cc Date: Tue, 9 Nov 2021 15:20:17 +0100
The UEFI specification requires for ExitBootServices() that "the boot services watchdog timer is disabled". We already disable the software watchdog. We should additionally disable the hardware watchdogs.
What about watchdogs that cannot be stopped? IIRC the IMX SoCs are like that.
You have to hope that your OS takes control of the watchdog quickly enough for the machine not to reset in between. Strictly speaking such a platform can not be fully compliant with the UEFI standard. In practice this doesn't really matter as the OS has to do this quickly enough if you're using a non-UEFI bootpath anyway.
Maybe somebody who cares enough can get the UEFI standard amended to handle this scenario. Maybe an interface can be added to the standard to provide more control over the watchdog such that the timeout can be set to a larger value before ExitBootServices() gets called. And add a way to keep the watchdog enabled on SoCs where it can be disabled. Last time this issue came up, someone pointed out that a watchdog that can be turned off isn't a proper watchdog. And indeed, turning the watchdog off when ExitBootServices() gets called means there is a time window where the watchdog isn't running and where the OS could hang forever.
Yeah there was already a disussion [1] about this very specific topic. I just noticed there was another one this week.
Anyway, I was just wondering that is just _tries_ to disable it. Or if you want to put it another way: the error is just ignored and the user will then wonder why the board will do a reset (or not if he's lucky).
-michael
[1] https://lore.kernel.org/u-boot/20200923164527.26894-1-michael@walle.cc/
On 11/9/21 15:54, Michael Walle wrote:
Am 2021-11-09 15:46, schrieb Mark Kettenis:
From: Michael Walle michael@walle.cc Date: Tue, 9 Nov 2021 15:20:17 +0100
The UEFI specification requires for ExitBootServices() that "the boot services watchdog timer is disabled". We already disable the software watchdog. We should additionally disable the hardware watchdogs.
What about watchdogs that cannot be stopped? IIRC the IMX SoCs are like that.
You have to hope that your OS takes control of the watchdog quickly enough for the machine not to reset in between. Strictly speaking such a platform can not be fully compliant with the UEFI standard. In practice this doesn't really matter as the OS has to do this quickly enough if you're using a non-UEFI bootpath anyway.
Maybe somebody who cares enough can get the UEFI standard amended to handle this scenario. Maybe an interface can be added to the standard to provide more control over the watchdog such that the timeout can be set to a larger value before ExitBootServices() gets called. And add a way to keep the watchdog enabled on SoCs where it can be disabled. Last time this issue came up, someone pointed out that a watchdog that can be turned off isn't a proper watchdog. And indeed, turning the watchdog off when ExitBootServices() gets called means there is a time window where the watchdog isn't running and where the OS could hang forever.
Yeah there was already a disussion [1] about this very specific topic. I just noticed there was another one this week.
Anyway, I was just wondering that is just _tries_ to disable it. Or if you want to put it another way: the error is just ignored and the user will then wonder why the board will do a reset (or not if he's lucky).
Stopping the boot process here would not make sense.
Writing out messages from U-Boot while an EFI binary is running is possible but may mess up the user's screen if the EFI application has some graphical output. I prefer to keep this silent.
Best regards
Heinrich
-michael
[1] https://lore.kernel.org/u-boot/20200923164527.26894-1-michael@walle.cc/
On Tue, Nov 09, 2021 at 11:19:01AM +0100, Heinrich Schuchardt wrote:
The UEFI specification requires for ExitBootServices() that "the boot services watchdog timer is disabled". We already disable the software watchdog. We should additionally disable the hardware watchdogs.
Reported-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
Let me start by saying thank you for bringing this up with UEFI folks as well. To be clear, for right now I would much rather see U-Boot continue to be non-complaint with UEFI in this regard and assume that a running watchdog will be able to be handled by the running OS (which tends to be the case, but not always as sunxi has just shown) than to attempt to be complaint with the spec as it stands now as I am hopeful that we can get this case handled in a way that matches long standing industry practice.
On 11/9/21 18:55, Tom Rini wrote:
On Tue, Nov 09, 2021 at 11:19:01AM +0100, Heinrich Schuchardt wrote:
The UEFI specification requires for ExitBootServices() that "the boot services watchdog timer is disabled". We already disable the software watchdog. We should additionally disable the hardware watchdogs.
Reported-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
Let me start by saying thank you for bringing this up with UEFI folks as well. To be clear, for right now I would much rather see U-Boot continue to be non-complaint with UEFI in this regard and assume that a running watchdog will be able to be handled by the running OS (which tends to be the case, but not always as sunxi has just shown) than to attempt to be complaint with the spec as it stands now as I am hopeful that we can get this case handled in a way that matches long standing industry practice.
We have either merge this patch or
[1/1] watchdog: don't autostart watchdog on Sunxi boards https://patchwork.ozlabs.org/project/uboot/patch/20211105183431.117221-1-hei...
or we will be breaking boot processes that have been running up to now.
As Sunxi watchdogs were only enabled by a recent patch disabling autostart should not cause any harm.
If you want to be able to deviate from the UEFI spec, this should be customizable and disabled by default.
Best regards
Heinrich
On Tue, Nov 09, 2021 at 07:15:10PM +0100, Heinrich Schuchardt wrote:
On 11/9/21 18:55, Tom Rini wrote:
On Tue, Nov 09, 2021 at 11:19:01AM +0100, Heinrich Schuchardt wrote:
The UEFI specification requires for ExitBootServices() that "the boot services watchdog timer is disabled". We already disable the software watchdog. We should additionally disable the hardware watchdogs.
Reported-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
Let me start by saying thank you for bringing this up with UEFI folks as well. To be clear, for right now I would much rather see U-Boot continue to be non-complaint with UEFI in this regard and assume that a running watchdog will be able to be handled by the running OS (which tends to be the case, but not always as sunxi has just shown) than to attempt to be complaint with the spec as it stands now as I am hopeful that we can get this case handled in a way that matches long standing industry practice.
We have either merge this patch or
[1/1] watchdog: don't autostart watchdog on Sunxi boards https://patchwork.ozlabs.org/project/uboot/patch/20211105183431.117221-1-hei...
or we will be breaking boot processes that have been running up to now.
As Sunxi watchdogs were only enabled by a recent patch disabling autostart should not cause any harm.
Yes, I was expecting a PR from Andre in my inbox this morning with your patch, as he had said he would apply it. I'm just assuming now I got a bit ahead of myself with expecting it so quickly.
On Tue, 9 Nov 2021 13:18:28 -0500 Tom Rini trini@konsulko.com wrote:
On Tue, Nov 09, 2021 at 07:15:10PM +0100, Heinrich Schuchardt wrote:
On 11/9/21 18:55, Tom Rini wrote:
On Tue, Nov 09, 2021 at 11:19:01AM +0100, Heinrich Schuchardt wrote:
The UEFI specification requires for ExitBootServices() that "the boot services watchdog timer is disabled". We already disable the software watchdog. We should additionally disable the hardware watchdogs.
Reported-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
Let me start by saying thank you for bringing this up with UEFI folks as well. To be clear, for right now I would much rather see U-Boot continue to be non-complaint with UEFI in this regard and assume that a running watchdog will be able to be handled by the running OS (which tends to be the case, but not always as sunxi has just shown) than to attempt to be complaint with the spec as it stands now as I am hopeful that we can get this case handled in a way that matches long standing industry practice.
We have either merge this patch or
[1/1] watchdog: don't autostart watchdog on Sunxi boards https://patchwork.ozlabs.org/project/uboot/patch/20211105183431.117221-1-hei...
or we will be breaking boot processes that have been running up to now.
As Sunxi watchdogs were only enabled by a recent patch disabling autostart should not cause any harm.
Yes, I was expecting a PR from Andre in my inbox this morning with your
This was my plan as well, until I opened up the UEFI spec ;-) Also I didn't see any problems so far in my - admittedly limited - casual testing (because I boot quickly and have the WDT compiled in).
But I see it now when disabling the watchdog in the kernel, so will take the "don't autostart" patch anyway.
If people have an embedded deployment where they rely on the watchdog, they can surely enable it before building U-Boot.
Cheers, Andre
patch, as he had said he would apply it. I'm just assuming now I got a bit ahead of myself with expecting it so quickly.
On Tue, Nov 9, 2021 at 5:55 PM Tom Rini trini@konsulko.com wrote:
On Tue, Nov 09, 2021 at 11:19:01AM +0100, Heinrich Schuchardt wrote:
The UEFI specification requires for ExitBootServices() that "the boot services watchdog timer is disabled". We already disable the software watchdog. We should additionally disable the hardware watchdogs.
Reported-by: Andre Przywara andre.przywara@arm.com Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
Let me start by saying thank you for bringing this up with UEFI folks as well. To be clear, for right now I would much rather see U-Boot continue to be non-complaint with UEFI in this regard and assume that a running watchdog will be able to be handled by the running OS (which tends to be the case, but not always as sunxi has just shown) than to attempt to be complaint with the spec as it stands now as I am hopeful that we can get this case handled in a way that matches long standing industry practice.
I’m also not keen to see the watchdogs disabled, even if it does make platforms technically non-compliant with UEFI. Disabling the watchdog does not make sense for throw devices using U-Boot get used.
I’ll put this on the agenda for the next EBBR biweekly
g.
-- Tom
participants (7)
-
Andre Przywara -
Grant Likely -
Heinrich Schuchardt -
Heinrich Schuchardt -
Mark Kettenis -
Michael Walle -
Tom Rini