Hi all, On Mon, Jan 30, 2023 at 01:30:49PM -0500, Tom Rini wrote: > On Mon, Jan 30, 2023 at 01:13:55PM -0500, Tom Rini wrote: > > On Sat, Jan 28, 2023 at 09:57:45AM +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.przyw...@arm.com> > > > Signed-off-by: Heinrich Schuchardt <heinrich.schucha...@canonical.com> > > > --- > > > lib/efi_loader/efi_boottime.c | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > > > index ba28989f36..71215af9d2 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> > > > @@ -2171,6 +2172,11 @@ static efi_status_t EFIAPI > > > efi_exit_boot_services(efi_handle_t image_handle, > > > list_del(&evt->link); > > > } > > > > > > + /* Disable watchdogs */ > > > + efi_set_watchdog(0); > > > + if IS_ENABLED(CONFIG_WDT) > > > + wdt_stop_all(); > > > + > > > if (!efi_st_keep_devices) { > > > bootm_disable_interrupts(); > > > if (IS_ENABLED(CONFIG_USB_DEVICE)) > > > @@ -2196,10 +2202,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); > > > - schedule(); > > > out: > > > if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) { > > > if (ret != EFI_SUCCESS) > > > > I thought we had rejected going down this path since the UEFI spec is > > unhelpfully wrong if it insists this? > > Because, to be clear, stopping hardware watchdogs is not to be done. The > one in-tree caller of wdt_stop_all is very questionable. You cannot > seriously stop a watchdog until someone else can hopefully resume it as > that violates the function of a hardware watchdog. A pure software > watchdog is one thing, and a hardware watchdog is another. I feel like > the most likely answer here is that someone needs to, still, push back > to the UEFI specification to get hardware watchdogs better understood > and handled, as it must never be stopped once started and if you cannot > reach the next stage in time, that's an engineering issue to resolve. My > first guess is that ExitBootServices should service the watchdog one > last time to ensure the largest window of time for the OS to take over > servicing of the watchdog. >
There's two scenarios I can think of 1. After U-Boot is done it can disable the hardware watchdog. The kernel will go through the EFI-stub -> kernel proper -> watchdog gets re-initialized. In that case you are *hoping* that device won't hang in the efi-stub or until the wd is up again. 2. EFI makes sure the hardware wd gets configured with the highest allowed value. The efi-stub doesn't have any driver to refresh the wd, so we will again rely on the wd driver coming up and refreshing the timers. None of those is perfect, but I prefer the latter Regards /Ilias > -- > Tom