On 31/01/2023 16.07, Tom Rini wrote: > On Tue, Jan 31, 2023 at 02:03:10PM +0200, Ilias Apalodimas 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.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. > > You cannot stop the hardware watchdog, period. I think in the previous > thread about this it was noted that some hardware watchdogs cannot be > disabled, it's not function that the watchdog supports. Someone needs to > go and talk with the UEFI specification people and get this addressed. > There is no sane path for "disable the hardware watchdog". >
Indeed. But I think one reasonable thing to do would be to say "ok, the payload is now ready to assume responsibility, so on the U-Boot side we stop _petting_ the watchdog(s)" (i.e. nowadays that would mean deregistering them from the cyclic framework), even if the payload still performs calls into U-Boot where we would otherwise use the opportunity to feed the watchdog. And of course it's reasonable in that case to do one last ping. Because it's also a recipe for disaster if, say, both the payload and U-Boot toggles the same gpio or frobs the same SOC registers. Unrelated, but does anybody know who "the UEFI specification people" are and how to reach out? Rasmus