[PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()

[Simon Glass]

Hi,

On Tue, 31 Jan 2023 at 05:04, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> 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.przywara at arm.com>
> > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at 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

How does this work if U-Boot runs grub instead of Linux? Does grub
update the watchdog?

Regards,
Simon