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

[Heinrich Schuchardt]

On 2/1/23 09:32, Rasmus Villemoes wrote:
> 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
> 

After ExitBootServices() the memory occupied by U-Boot will be reused by 
the operating system. Don't expect any U-Boot interrupt vector code to 
exist after this point.

If the hardware watchdog is not configured to immediately reset the CPU 
but create an interrupt instead, anything may happen.

@Tom
Are all hardware watchdogs used in U-Boot configured to immediately 
reset the CPU?

The UEFI Forum's site is https://uefi.org/. Bugs are reported via 
https://bugzilla.tianocore.org/. For changing the spec you will have to 
create a change request in their 'Mantis' system.

Best regards

Heinrich