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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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@arm.com>
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@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