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

[Rasmus Villemoes]

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.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.
> 
> 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