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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 2774 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]