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

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

[Heinrich Schuchardt]

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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 1823990d9b..e33821c93a 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>
@@ -2166,6 +2167,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
 	/* Fix up caches for EFI payloads if necessary */
 	efi_exit_caches();
 
+	/* Disable watchdogs */
+	efi_set_watchdog(0);
+	if IS_ENABLED(CONFIG_WDT)
+		wdt_stop_all();
+
 	/* This stops all lingering devices */
 	bootm_disable_interrupts();
 
@@ -2181,9 +2187,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);
-	WATCHDOG_RESET();
 out:
 	if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
 		if (ret != EFI_SUCCESS)
-- 
2.32.0

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

[Heinrich Schuchardt]

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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 1823990d9b..e33821c93a 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>
@@ -2166,6 +2167,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
 	/* Fix up caches for EFI payloads if necessary */
 	efi_exit_caches();
 
+	/* Disable watchdogs */
+	efi_set_watchdog(0);
+	if IS_ENABLED(CONFIG_WDT)
+		wdt_stop_all();
+
 	/* This stops all lingering devices */
 	bootm_disable_interrupts();
 
@@ -2181,9 +2187,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);
-	WATCHDOG_RESET();
 out:
 	if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
 		if (ret != EFI_SUCCESS)
-- 
2.32.0

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

[Michael Walle]

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

What about watchdogs that cannot be stopped? IIRC the IMX SoCs are
like that.

-michael

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

[Mark Kettenis]

> From: Michael Walle <michael@walle.cc>
> Date: Tue,  9 Nov 2021 15:20:17 +0100
> 
> > 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.
> 
> What about watchdogs that cannot be stopped? IIRC the IMX SoCs are
> like that.

You have to hope that your OS takes control of the watchdog quickly
enough for the machine not to reset in between.  Strictly speaking
such a platform can not be fully compliant with the UEFI standard.  In
practice this doesn't really matter as the OS has to do this quickly
enough if you're using a non-UEFI bootpath anyway.

Maybe somebody who cares enough can get the UEFI standard amended to
handle this scenario.  Maybe an interface can be added to the standard
to provide more control over the watchdog such that the timeout can be
set to a larger value before ExitBootServices() gets called.  And add
a way to keep the watchdog enabled on SoCs where it can be disabled.
Last time this issue came up, someone pointed out that a watchdog that
can be turned off isn't a proper watchdog.  And indeed, turning the
watchdog off when ExitBootServices() gets called means there is a time
window where the watchdog isn't running and where the OS could hang
forever.

Cheers,

Mark

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

[Michael Walle]

Am 2021-11-09 15:46, schrieb Mark Kettenis:
>> From: Michael Walle <michael@walle.cc>
>> Date: Tue,  9 Nov 2021 15:20:17 +0100
>> 
>> > 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.
>> 
>> What about watchdogs that cannot be stopped? IIRC the IMX SoCs are
>> like that.
> 
> You have to hope that your OS takes control of the watchdog quickly
> enough for the machine not to reset in between.  Strictly speaking
> such a platform can not be fully compliant with the UEFI standard.  In
> practice this doesn't really matter as the OS has to do this quickly
> enough if you're using a non-UEFI bootpath anyway.
> 
> Maybe somebody who cares enough can get the UEFI standard amended to
> handle this scenario.  Maybe an interface can be added to the standard
> to provide more control over the watchdog such that the timeout can be
> set to a larger value before ExitBootServices() gets called.  And add
> a way to keep the watchdog enabled on SoCs where it can be disabled.
> Last time this issue came up, someone pointed out that a watchdog that
> can be turned off isn't a proper watchdog.  And indeed, turning the
> watchdog off when ExitBootServices() gets called means there is a time
> window where the watchdog isn't running and where the OS could hang
> forever.

Yeah there was already a disussion [1] about this very specific topic.
I just noticed there was another one this week.

Anyway, I was just wondering that is just _tries_ to disable it. Or
if you want to put it another way: the error is just ignored and the
user will then wonder why the board will do a reset (or not if
he's lucky).

-michael

[1] 
https://lore.kernel.org/u-boot/20200923164527.26894-1-michael@walle.cc/

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

[Heinrich Schuchardt]

On 11/9/21 15:54, Michael Walle wrote:
> Am 2021-11-09 15:46, schrieb Mark Kettenis:
>>> From: Michael Walle <michael@walle.cc>
>>> Date: Tue,  9 Nov 2021 15:20:17 +0100
>>>
>>> > 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.
>>>
>>> What about watchdogs that cannot be stopped? IIRC the IMX SoCs are
>>> like that.
>>
>> You have to hope that your OS takes control of the watchdog quickly
>> enough for the machine not to reset in between.  Strictly speaking
>> such a platform can not be fully compliant with the UEFI standard.  In
>> practice this doesn't really matter as the OS has to do this quickly
>> enough if you're using a non-UEFI bootpath anyway.
>>
>> Maybe somebody who cares enough can get the UEFI standard amended to
>> handle this scenario.  Maybe an interface can be added to the standard
>> to provide more control over the watchdog such that the timeout can be
>> set to a larger value before ExitBootServices() gets called.  And add
>> a way to keep the watchdog enabled on SoCs where it can be disabled.
>> Last time this issue came up, someone pointed out that a watchdog that
>> can be turned off isn't a proper watchdog.  And indeed, turning the
>> watchdog off when ExitBootServices() gets called means there is a time
>> window where the watchdog isn't running and where the OS could hang
>> forever.
>
> Yeah there was already a disussion [1] about this very specific topic.
> I just noticed there was another one this week.
>
> Anyway, I was just wondering that is just _tries_ to disable it. Or
> if you want to put it another way: the error is just ignored and the
> user will then wonder why the board will do a reset (or not if
> he's lucky).

Stopping the boot process here would not make sense.

Writing out messages from U-Boot while an EFI binary is running is
possible but may mess up the user's screen if the EFI application has
some graphical output. I prefer to keep this silent.

Best regards

Heinrich

>
> -michael
>
> [1] https://lore.kernel.org/u-boot/20200923164527.26894-1-michael@walle.cc/

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

[Tom Rini]

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

On Tue, Nov 09, 2021 at 11:19:01AM +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>

Let me start by saying thank you for bringing this up with UEFI folks as
well.  To be clear, for right now I would much rather see U-Boot
continue to be non-complaint with UEFI in this regard and assume that a
running watchdog will be able to be handled by the running OS (which
tends to be the case, but not always as sunxi has just shown) than to
attempt to be complaint with the spec as it stands now as I am hopeful
that we can get this case handled in a way that matches long standing
industry practice.

-- 
Tom

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

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

[Heinrich Schuchardt]

On 11/9/21 18:55, Tom Rini wrote:
> On Tue, Nov 09, 2021 at 11:19:01AM +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>
> 
> Let me start by saying thank you for bringing this up with UEFI folks as
> well.  To be clear, for right now I would much rather see U-Boot
> continue to be non-complaint with UEFI in this regard and assume that a
> running watchdog will be able to be handled by the running OS (which
> tends to be the case, but not always as sunxi has just shown) than to
> attempt to be complaint with the spec as it stands now as I am hopeful
> that we can get this case handled in a way that matches long standing
> industry practice.
> 

We have either merge this patch or

[1/1] watchdog: don't autostart watchdog on Sunxi boards
https://patchwork.ozlabs.org/project/uboot/patch/20211105183431.117221-1-heinrich.schuchardt@canonical.com/

or we will be breaking boot processes that have been running up to now.

As Sunxi watchdogs were only enabled by a recent patch disabling 
autostart should not cause any harm.

If you want to be able to deviate from the UEFI spec, this should be 
customizable and disabled by default.

Best regards

Heinrich

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

[Tom Rini]

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

On Tue, Nov 09, 2021 at 07:15:10PM +0100, Heinrich Schuchardt wrote:
> On 11/9/21 18:55, Tom Rini wrote:
> > On Tue, Nov 09, 2021 at 11:19:01AM +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>
> > 
> > Let me start by saying thank you for bringing this up with UEFI folks as
> > well.  To be clear, for right now I would much rather see U-Boot
> > continue to be non-complaint with UEFI in this regard and assume that a
> > running watchdog will be able to be handled by the running OS (which
> > tends to be the case, but not always as sunxi has just shown) than to
> > attempt to be complaint with the spec as it stands now as I am hopeful
> > that we can get this case handled in a way that matches long standing
> > industry practice.
> > 
> 
> We have either merge this patch or
> 
> [1/1] watchdog: don't autostart watchdog on Sunxi boards
> https://patchwork.ozlabs.org/project/uboot/patch/20211105183431.117221-1-heinrich.schuchardt@canonical.com/
> 
> or we will be breaking boot processes that have been running up to now.
> 
> As Sunxi watchdogs were only enabled by a recent patch disabling autostart
> should not cause any harm.

Yes, I was expecting a PR from Andre in my inbox this morning with your
patch, as he had said he would apply it.  I'm just assuming now I got a
bit ahead of myself with expecting it so quickly.

-- 
Tom

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

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

[Andre Przywara]

On Tue, 9 Nov 2021 13:18:28 -0500
Tom Rini <trini@konsulko.com> wrote:

> On Tue, Nov 09, 2021 at 07:15:10PM +0100, Heinrich Schuchardt wrote:
> > On 11/9/21 18:55, Tom Rini wrote:  
> > > On Tue, Nov 09, 2021 at 11:19:01AM +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>  
> > > 
> > > Let me start by saying thank you for bringing this up with UEFI folks as
> > > well.  To be clear, for right now I would much rather see U-Boot
> > > continue to be non-complaint with UEFI in this regard and assume that a
> > > running watchdog will be able to be handled by the running OS (which
> > > tends to be the case, but not always as sunxi has just shown) than to
> > > attempt to be complaint with the spec as it stands now as I am hopeful
> > > that we can get this case handled in a way that matches long standing
> > > industry practice.
> > >   
> > 
> > We have either merge this patch or
> > 
> > [1/1] watchdog: don't autostart watchdog on Sunxi boards
> > https://patchwork.ozlabs.org/project/uboot/patch/20211105183431.117221-1-heinrich.schuchardt@canonical.com/
> > 
> > or we will be breaking boot processes that have been running up to now.
> > 
> > As Sunxi watchdogs were only enabled by a recent patch disabling autostart
> > should not cause any harm.  
> 
> Yes, I was expecting a PR from Andre in my inbox this morning with your

This was my plan as well, until I opened up the UEFI spec ;-)
Also I didn't see any problems so far in my - admittedly limited -
casual testing (because I boot quickly and have the WDT compiled in).

But I see it now when disabling the watchdog in the kernel, so will
take the "don't autostart" patch anyway.

If people have an embedded deployment where they rely on the watchdog,
they can surely enable it before building U-Boot.

Cheers,
Andre

> patch, as he had said he would apply it.  I'm just assuming now I got a
> bit ahead of myself with expecting it so quickly.
> 

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

[Grant Likely]

On Tue, Nov 9, 2021 at 5:55 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Nov 09, 2021 at 11:19:01AM +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>
>
> Let me start by saying thank you for bringing this up with UEFI folks as
> well.  To be clear, for right now I would much rather see U-Boot
> continue to be non-complaint with UEFI in this regard and assume that a
> running watchdog will be able to be handled by the running OS (which
> tends to be the case, but not always as sunxi has just shown) than to
> attempt to be complaint with the spec as it stands now as I am hopeful
> that we can get this case handled in a way that matches long standing
> industry practice.

I’m also not keen to see the watchdogs disabled, even if it does make
platforms technically non-compliant with UEFI. Disabling the watchdog
does not make sense for throw devices using U-Boot get used.

I’ll put this on the agenda for the next EBBR biweekly

g.

>
> --
> Tom

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

[Michael Walle]

>>>> Basically I want the following:
>>>> 
>>>> (1) board boots with watchdog enabled
>>>> (2) u-boot services watchdog
>>>> (3a) booting embedded linux with booti (watchdog enabled) or
>>>> (3b) booting generic OS with bootefi (watchdog disabled)
>>>> 
>>>> The missing case is booting an embedded linux with bootefi, which
>>>> would be nice to have. But I don't really see it as a use-case for
>>>> our board.
>>>> 
>>> For SUNXI boards disabling CONFIG_WATCHDOG_AUTOSTART solved the
>>> problem with the very short maximum expiration time of the watchdog.
>> 
>> I can't follow you here. What "very short maximum expiration time"?
>> With CONFIG_WATCHDOG_AUTOSTART disabled, the watchdog won't be kicked
>> by u-boot, right? wdt->running will never be set to true and
>> wdt_cyclic() will be a noop.
> 
> The sunxi boards failed to boot with CONFIG_WATCHDOG_AUTOSTART because
> 16s is too short for Linux to install a watchdog driver. With
> CONFIG_WATCHDOG_AUTOSTART=n the watchdog is not running and the boards
> boot.

But how does that help in my case?

-michael

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

[Heinrich Schuchardt]

On 2/7/23 16:29, Michael Walle wrote:
>>>>>> Honestly, not really? Some good number of SoCs will start the 
>>>>>> watchdog
>>>>>> in ROM and these are also the ones that don't allow you to turn it 
>>>>>> off.
>>>>>
>>>>> I hope not, that sounds really risky. How would you debug such a 
>>>>> platform?
>>>>
>>>> _Every single_ custom piece of industrial (as opposed to 
>>>> consumer-grade)
>>>> hardware I've worked on as a consultant has had an external,
>>>> always-running, gpio-petted watchdog. It's simply just something that
>>>> the hardware designers include, and in some cases that's even due to
>>>> certification requirements. So an always-running, cannot-be-turned-off,
>>>> watchdog is a real thing, in real hardware, and if specs don't account
>>>> for that, well, the spec is just paper, and we can ignore it.
>>>
>>> I agree. But on the other hand, you cannot assume or force the OS to
>>> have a watchdog driver in the general case - which is as I understand
>>> it - one goal of EFI.
>>>
>>> Obviously, there are watchdogs that can be disabled and some which
>>> cannot. I don't want to argue about the advantages and disadvantages.
>>>
>>> For watchdogs which cannot be turned off, we can't really do anything
>>> anyway after the handoff to the OS - except increasing its timeout if
>>> thats possible.
>>>
>>> For watchdogs that can be disabled (and are enabled in u-boot of 
>>> course),
>>> there seems to be two use-cases:
>>>   (1) embedded EFI boot, that is you know exactly what you are 
>>> booting, i.e.
>>>       self compiled kernel with a watchdog driver
>>>   (2) booting a general OS via EFI, think of a debian boot CD for 
>>> example.
>>>
>>> I agree, that for (1) the watchdog shouldn't be disabled. For (2) you
>>> cannot assume the booting OS has a driver for the watchdog, let it be an
>>> older version of a distribution which just haven't the SoC watchdog 
>>> driver
>>> enabled or maybe because there is no driver for it at all (yet).
>>>
>>> Is there a way, to have the watchdog disabled for case (2) while also
>>> having the possibity to use bootm/booti/bootz and keep the watchdog
>>> enabled? Basically I want the following:
>>>
>>> (1) board boots with watchdog enabled
>>> (2) u-boot services watchdog
>>> (3a) booting embedded linux with booti (watchdog enabled) or
>>> (3b) booting generic OS with bootefi (watchdog disabled)
>>>
>>> The missing case is booting an embedded linux with bootefi, which
>>> would be nice to have. But I don't really see it as a use-case for
>>> our board.
>>>
>>> -michael
>>
>> For SUNXI boards disabling CONFIG_WATCHDOG_AUTOSTART solved the
>> problem with the very short maximum expiration time of the watchdog.
> 
> I can't follow you here. What "very short maximum expiration time"?
> With CONFIG_WATCHDOG_AUTOSTART disabled, the watchdog won't be kicked
> by u-boot, right? wdt->running will never be set to true and
> wdt_cyclic() will be a noop.
> 
> -michael
The sunxi boards failed to boot with CONFIG_WATCHDOG_AUTOSTART because 
16s is too short for Linux to install a watchdog driver. With 
CONFIG_WATCHDOG_AUTOSTART=n the watchdog is not running and the boards boot.

Best regards

Heinrich

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

[Michael Walle]

>>>>> Honestly, not really? Some good number of SoCs will start the 
>>>>> watchdog
>>>>> in ROM and these are also the ones that don't allow you to turn it 
>>>>> off.
>>>> 
>>>> I hope not, that sounds really risky. How would you debug such a 
>>>> platform?
>>> 
>>> _Every single_ custom piece of industrial (as opposed to 
>>> consumer-grade)
>>> hardware I've worked on as a consultant has had an external,
>>> always-running, gpio-petted watchdog. It's simply just something that
>>> the hardware designers include, and in some cases that's even due to
>>> certification requirements. So an always-running, 
>>> cannot-be-turned-off,
>>> watchdog is a real thing, in real hardware, and if specs don't 
>>> account
>>> for that, well, the spec is just paper, and we can ignore it.
>> 
>> I agree. But on the other hand, you cannot assume or force the OS to
>> have a watchdog driver in the general case - which is as I understand
>> it - one goal of EFI.
>> 
>> Obviously, there are watchdogs that can be disabled and some which
>> cannot. I don't want to argue about the advantages and disadvantages.
>> 
>> For watchdogs which cannot be turned off, we can't really do anything
>> anyway after the handoff to the OS - except increasing its timeout if
>> thats possible.
>> 
>> For watchdogs that can be disabled (and are enabled in u-boot of 
>> course),
>> there seems to be two use-cases:
>>   (1) embedded EFI boot, that is you know exactly what you are 
>> booting, i.e.
>>       self compiled kernel with a watchdog driver
>>   (2) booting a general OS via EFI, think of a debian boot CD for 
>> example.
>> 
>> I agree, that for (1) the watchdog shouldn't be disabled. For (2) you
>> cannot assume the booting OS has a driver for the watchdog, let it be 
>> an
>> older version of a distribution which just haven't the SoC watchdog 
>> driver
>> enabled or maybe because there is no driver for it at all (yet).
>> 
>> Is there a way, to have the watchdog disabled for case (2) while also
>> having the possibity to use bootm/booti/bootz and keep the watchdog
>> enabled? Basically I want the following:
>> 
>> (1) board boots with watchdog enabled
>> (2) u-boot services watchdog
>> (3a) booting embedded linux with booti (watchdog enabled) or
>> (3b) booting generic OS with bootefi (watchdog disabled)
>> 
>> The missing case is booting an embedded linux with bootefi, which
>> would be nice to have. But I don't really see it as a use-case for
>> our board.
>> 
>> -michael
> 
> For SUNXI boards disabling CONFIG_WATCHDOG_AUTOSTART solved the
> problem with the very short maximum expiration time of the watchdog.

I can't follow you here. What "very short maximum expiration time"?
With CONFIG_WATCHDOG_AUTOSTART disabled, the watchdog won't be kicked
by u-boot, right? wdt->running will never be set to true and
wdt_cyclic() will be a noop.

-michael

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

[Heinrich Schuchardt]

On 2/7/23 15:59, Michael Walle wrote:
>>>> Honestly, not really? Some good number of SoCs will start the watchdog
>>>> in ROM and these are also the ones that don't allow you to turn it off.
>>>
>>> I hope not, that sounds really risky. How would you debug such a platform?
>>
>> _Every single_ custom piece of industrial (as opposed to consumer-grade)
>> hardware I've worked on as a consultant has had an external,
>> always-running, gpio-petted watchdog. It's simply just something that
>> the hardware designers include, and in some cases that's even due to
>> certification requirements. So an always-running, cannot-be-turned-off,
>> watchdog is a real thing, in real hardware, and if specs don't account
>> for that, well, the spec is just paper, and we can ignore it.
> 
> I agree. But on the other hand, you cannot assume or force the OS to
> have a watchdog driver in the general case - which is as I understand
> it - one goal of EFI.
> 
> Obviously, there are watchdogs that can be disabled and some which
> cannot. I don't want to argue about the advantages and disadvantages.
> 
> For watchdogs which cannot be turned off, we can't really do anything
> anyway after the handoff to the OS - except increasing its timeout if
> thats possible.
> 
> For watchdogs that can be disabled (and are enabled in u-boot of course),
> there seems to be two use-cases:
>   (1) embedded EFI boot, that is you know exactly what you are booting, i.e.
>       self compiled kernel with a watchdog driver
>   (2) booting a general OS via EFI, think of a debian boot CD for example.
> 
> I agree, that for (1) the watchdog shouldn't be disabled. For (2) you
> cannot assume the booting OS has a driver for the watchdog, let it be an
> older version of a distribution which just haven't the SoC watchdog driver
> enabled or maybe because there is no driver for it at all (yet).
> 
> Is there a way, to have the watchdog disabled for case (2) while also
> having the possibity to use bootm/booti/bootz and keep the watchdog
> enabled? Basically I want the following:
> 
> (1) board boots with watchdog enabled
> (2) u-boot services watchdog
> (3a) booting embedded linux with booti (watchdog enabled) or
> (3b) booting generic OS with bootefi (watchdog disabled)
> 
> The missing case is booting an embedded linux with bootefi, which
> would be nice to have. But I don't really see it as a use-case for
> our board.
> 
> -michael

For SUNXI boards disabling CONFIG_WATCHDOG_AUTOSTART solved the problem 
with the very short maximum expiration time of the watchdog.

Best regards

Heinrich

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

[Michael Walle]

>>> Honestly, not really? Some good number of SoCs will start the watchdog
>>> in ROM and these are also the ones that don't allow you to turn it off.
>> 
>> I hope not, that sounds really risky. How would you debug such a platform?
>
> _Every single_ custom piece of industrial (as opposed to consumer-grade)
> hardware I've worked on as a consultant has had an external,
> always-running, gpio-petted watchdog. It's simply just something that
> the hardware designers include, and in some cases that's even due to
> certification requirements. So an always-running, cannot-be-turned-off,
> watchdog is a real thing, in real hardware, and if specs don't account
> for that, well, the spec is just paper, and we can ignore it.

I agree. But on the other hand, you cannot assume or force the OS to
have a watchdog driver in the general case - which is as I understand
it - one goal of EFI.

Obviously, there are watchdogs that can be disabled and some which
cannot. I don't want to argue about the advantages and disadvantages.

For watchdogs which cannot be turned off, we can't really do anything
anyway after the handoff to the OS - except increasing its timeout if
thats possible.

For watchdogs that can be disabled (and are enabled in u-boot of course),
there seems to be two use-cases:
 (1) embedded EFI boot, that is you know exactly what you are booting, i.e.
     self compiled kernel with a watchdog driver
 (2) booting a general OS via EFI, think of a debian boot CD for example.

I agree, that for (1) the watchdog shouldn't be disabled. For (2) you
cannot assume the booting OS has a driver for the watchdog, let it be an
older version of a distribution which just haven't the SoC watchdog driver
enabled or maybe because there is no driver for it at all (yet).

Is there a way, to have the watchdog disabled for case (2) while also
having the possibity to use bootm/booti/bootz and keep the watchdog
enabled? Basically I want the following:

(1) board boots with watchdog enabled
(2) u-boot services watchdog
(3a) booting embedded linux with booti (watchdog enabled) or
(3b) booting generic OS with bootefi (watchdog disabled)

The missing case is booting an embedded linux with bootefi, which
would be nice to have. But I don't really see it as a use-case for
our board.

-michael

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

[Simon Glass]

Hi,

On Fri, 3 Feb 2023 at 08:51, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Feb 02, 2023 at 07:15:35PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 2 Feb 2023 at 10:22, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Feb 02, 2023 at 10:12:07AM -0700, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Thu, 2 Feb 2023 at 01:17, Etienne Carriere
> > > > <etienne.carriere@linaro.org> wrote:
> > > > >
> > > > > Hello Heinrich and all,
> > > > >
> > > > > On Wed, 1 Feb 2023 at 10:00, Heinrich Schuchardt
> > > > > <heinrich.schuchardt@canonical.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > 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@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.
> > > > > > >>
> > > > > > >> 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?
> > > > >
> > > > > I guess not all but there are some. Likely related to some chip
> > > > > specific fuse(s), once burnt a watchdog is initially kicked at reset
> > > > > and can't be stopped.
> > > > > We're indeed facing issues with hardware watchdogs timeout
> > > > > capabilities when booting EFI bootloaders or even kernels that may
> > > > > take time to download and install their watchdog driver as a kmod.
> > > > > Extending timeout capabilities looks like the only viable way to
> > > > > address the later case.
> > > > >
> > > > > BR,
> > > > > Etienne
> > > > >
> > > > > >
> > > > > > 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.
> > > >
> > > > Just to point out that this sort of thing is easier with the VBE
> > > > approach, since the OS can tell U-Boot whether it supports a watchdog
> > > > or not.
> > >
> > > Honestly, not really? Some good number of SoCs will start the watchdog
> > > in ROM and these are also the ones that don't allow you to turn it off.
> >
> > I hope not, that sounds really risky. How would you debug such a platform?
>
> Same way we have for the last 20+ years? This isn't a new thing by far.

I mean, when you pause U-Boot with your JTAG debugger, how does it
avoid resetting and losing your progress? On-chip watchdogs handle
this, but if it is something that is started externally to the chip
and generates a reset signal for the system, then I don't see how it
can work.

Regards,
Simon

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

[Tom Rini]

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

On Thu, Feb 02, 2023 at 07:15:35PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 2 Feb 2023 at 10:22, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Feb 02, 2023 at 10:12:07AM -0700, Simon Glass wrote:
> > > Hi,
> > >
> > > On Thu, 2 Feb 2023 at 01:17, Etienne Carriere
> > > <etienne.carriere@linaro.org> wrote:
> > > >
> > > > Hello Heinrich and all,
> > > >
> > > > On Wed, 1 Feb 2023 at 10:00, Heinrich Schuchardt
> > > > <heinrich.schuchardt@canonical.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > 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@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.
> > > > > >>
> > > > > >> 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?
> > > >
> > > > I guess not all but there are some. Likely related to some chip
> > > > specific fuse(s), once burnt a watchdog is initially kicked at reset
> > > > and can't be stopped.
> > > > We're indeed facing issues with hardware watchdogs timeout
> > > > capabilities when booting EFI bootloaders or even kernels that may
> > > > take time to download and install their watchdog driver as a kmod.
> > > > Extending timeout capabilities looks like the only viable way to
> > > > address the later case.
> > > >
> > > > BR,
> > > > Etienne
> > > >
> > > > >
> > > > > 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.
> > >
> > > Just to point out that this sort of thing is easier with the VBE
> > > approach, since the OS can tell U-Boot whether it supports a watchdog
> > > or not.
> >
> > Honestly, not really? Some good number of SoCs will start the watchdog
> > in ROM and these are also the ones that don't allow you to turn it off.
> 
> I hope not, that sounds really risky. How would you debug such a platform?

Same way we have for the last 20+ years? This isn't a new thing by far.

-- 
Tom

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

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

[Rasmus Villemoes]

On 03/02/2023 03.15, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 2 Feb 2023 at 10:22, Tom Rini <trini@konsulko.com> wrote:
>>

>> Honestly, not really? Some good number of SoCs will start the watchdog
>> in ROM and these are also the ones that don't allow you to turn it off.
> 
> I hope not, that sounds really risky. How would you debug such a platform?

_Every single_ custom piece of industrial (as opposed to consumer-grade)
hardware I've worked on as a consultant has had an external,
always-running, gpio-petted watchdog. It's simply just something that
the hardware designers include, and in some cases that's even due to
certification requirements. So an always-running, cannot-be-turned-off,
watchdog is a real thing, in real hardware, and if specs don't account
for that, well, the spec is just paper, and we can ignore it.

As for debugging and bringup, I've seen various solutions (depending on
the actual watchdog chip). Usually there's some way to place a jumper
that will either feed the watchdog from some, say, 32kHz output from an
RTC or elsewhere, or place a jumper to pull up/pull down some
enable/disable pin to the watchdog chip. IOW, when you have physical
access to the PCB lying on your desk, you can disable the watchdog, but
there's no way to do that in the field or in production.

Rasmus

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

[Simon Glass]

Hi Tom,

On Thu, 2 Feb 2023 at 10:22, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Feb 02, 2023 at 10:12:07AM -0700, Simon Glass wrote:
> > Hi,
> >
> > On Thu, 2 Feb 2023 at 01:17, Etienne Carriere
> > <etienne.carriere@linaro.org> wrote:
> > >
> > > Hello Heinrich and all,
> > >
> > > On Wed, 1 Feb 2023 at 10:00, Heinrich Schuchardt
> > > <heinrich.schuchardt@canonical.com> wrote:
> > > >
> > > >
> > > >
> > > > 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@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.
> > > > >>
> > > > >> 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?
> > >
> > > I guess not all but there are some. Likely related to some chip
> > > specific fuse(s), once burnt a watchdog is initially kicked at reset
> > > and can't be stopped.
> > > We're indeed facing issues with hardware watchdogs timeout
> > > capabilities when booting EFI bootloaders or even kernels that may
> > > take time to download and install their watchdog driver as a kmod.
> > > Extending timeout capabilities looks like the only viable way to
> > > address the later case.
> > >
> > > BR,
> > > Etienne
> > >
> > > >
> > > > 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.
> >
> > Just to point out that this sort of thing is easier with the VBE
> > approach, since the OS can tell U-Boot whether it supports a watchdog
> > or not.
>
> Honestly, not really? Some good number of SoCs will start the watchdog
> in ROM and these are also the ones that don't allow you to turn it off.

I hope not, that sounds really risky. How would you debug such a platform?

Regards,
Simon

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

[Tom Rini]

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

On Thu, Feb 02, 2023 at 10:12:07AM -0700, Simon Glass wrote:
> Hi,
> 
> On Thu, 2 Feb 2023 at 01:17, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Hello Heinrich and all,
> >
> > On Wed, 1 Feb 2023 at 10:00, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> > >
> > >
> > >
> > > 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@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.
> > > >>
> > > >> 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?
> >
> > I guess not all but there are some. Likely related to some chip
> > specific fuse(s), once burnt a watchdog is initially kicked at reset
> > and can't be stopped.
> > We're indeed facing issues with hardware watchdogs timeout
> > capabilities when booting EFI bootloaders or even kernels that may
> > take time to download and install their watchdog driver as a kmod.
> > Extending timeout capabilities looks like the only viable way to
> > address the later case.
> >
> > BR,
> > Etienne
> >
> > >
> > > 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.
> 
> Just to point out that this sort of thing is easier with the VBE
> approach, since the OS can tell U-Boot whether it supports a watchdog
> or not.

Honestly, not really? Some good number of SoCs will start the watchdog
in ROM and these are also the ones that don't allow you to turn it off.

-- 
Tom

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

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

[Simon Glass]

Hi,

On Thu, 2 Feb 2023 at 01:17, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello Heinrich and all,
>
> On Wed, 1 Feb 2023 at 10:00, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
> >
> >
> >
> > 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@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.
> > >>
> > >> 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?
>
> I guess not all but there are some. Likely related to some chip
> specific fuse(s), once burnt a watchdog is initially kicked at reset
> and can't be stopped.
> We're indeed facing issues with hardware watchdogs timeout
> capabilities when booting EFI bootloaders or even kernels that may
> take time to download and install their watchdog driver as a kmod.
> Extending timeout capabilities looks like the only viable way to
> address the later case.
>
> BR,
> Etienne
>
> >
> > 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.

Just to point out that this sort of thing is easier with the VBE
approach, since the OS can tell U-Boot whether it supports a watchdog
or not.

Regards,
Simon

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

[Etienne Carriere]

Hello Heinrich and all,

On Wed, 1 Feb 2023 at 10:00, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> 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@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.
> >>
> >> 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?

I guess not all but there are some. Likely related to some chip
specific fuse(s), once burnt a watchdog is initially kicked at reset
and can't be stopped.
We're indeed facing issues with hardware watchdogs timeout
capabilities when booting EFI bootloaders or even kernels that may
take time to download and install their watchdog driver as a kmod.
Extending timeout capabilities looks like the only viable way to
address the later case.

BR,
Etienne

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

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

[Tom Rini]

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

On Wed, Feb 01, 2023 at 01:49:58PM +0100, Mark Kettenis wrote:
> > Date: Wed, 1 Feb 2023 09:32:54 +0100
> > From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > 
> > 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@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.
> > > 
> > > 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.
> 
> Well, for EFI the point where the handover happens is when the payload
> calls ExitBootServices().  After that point the payload "owns" the
> hardware exposed to it in the device tree.  And the only way for the
> payload to call into U-Boot at that point is through EFI runtime
> services, which are not supposed to touch the hardware that is now
> under control of the payload.  With the exception of the ResetSystem()
> EFI runtime service of course.  But that call will never return and
> therefore souldn't lead to any conflicts.  So nothing will pet the
> watchdog anymore.
> 
> The discussion is about what exactly should happen to the watchdog
> when ExitBootServices() gets called.  The UEFI specification says that
> the (unspecified) watchdog should be disabled at that point.  Which is
> something that makes no sense to many people with experience in
> embedded systems and may even be technically impossible on some
> hardware.  So U-Boot doesn't follow the spec in this regard.
> 
> What U-Boot currently does actually works just fine in most cases.
> You just have to make sure that the payload takes over the petting of
> the watchdog in a timely fashion after calling ExitBootServices().
> But that is not really different from what happens if you boot a Linux
> kernel using one of the "legacy" boot methods that U-Boot provides
> (e.g. using the "bootm" command).
> 
> The "problem" here really is that every 6 months somebody fails to
> include the watchdog driver in their Linux kernel, reads the UEFI
> spec, decides U-Boot is violating the spec and comes up with a diff to
> "fix" U-Boot.  And then we have the same discussion again.  The
> "solution" to that probablem is to get the UEFI spec changes to allow
> for U-Boot's behaviour.  But nobody here cares enough about that to
> actually makes that happen.

Yes, this.  And I am hoping that at this point I can motivate one of the
many people that do already have contacts with, and accounts in the
various UEFI forums and are otherwise familiar with the processes to get
this addressed. There are groups that want UEFI to be a serious choice
in embedded usage and fixing the spec to understand how hardware
watchdogs are used in the embedded space for very good reason will
benefit them. I'm quite happy to have U-Boot continue to violate this
specific part of the specification until then.

-- 
Tom

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

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

[Mark Kettenis]

> Date: Wed, 1 Feb 2023 09:32:54 +0100
> From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> 
> 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@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.
> > 
> > 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.

Well, for EFI the point where the handover happens is when the payload
calls ExitBootServices().  After that point the payload "owns" the
hardware exposed to it in the device tree.  And the only way for the
payload to call into U-Boot at that point is through EFI runtime
services, which are not supposed to touch the hardware that is now
under control of the payload.  With the exception of the ResetSystem()
EFI runtime service of course.  But that call will never return and
therefore souldn't lead to any conflicts.  So nothing will pet the
watchdog anymore.

The discussion is about what exactly should happen to the watchdog
when ExitBootServices() gets called.  The UEFI specification says that
the (unspecified) watchdog should be disabled at that point.  Which is
something that makes no sense to many people with experience in
embedded systems and may even be technically impossible on some
hardware.  So U-Boot doesn't follow the spec in this regard.

What U-Boot currently does actually works just fine in most cases.
You just have to make sure that the payload takes over the petting of
the watchdog in a timely fashion after calling ExitBootServices().
But that is not really different from what happens if you boot a Linux
kernel using one of the "legacy" boot methods that U-Boot provides
(e.g. using the "bootm" command).

The "problem" here really is that every 6 months somebody fails to
include the watchdog driver in their Linux kernel, reads the UEFI
spec, decides U-Boot is violating the spec and comes up with a diff to
"fix" U-Boot.  And then we have the same discussion again.  The
"solution" to that probablem is to get the UEFI spec changes to allow
for U-Boot's behaviour.  But nobody here cares enough about that to
actually makes that happen.

Re: [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@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.
>>
>> 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

Re: [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@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.
> 
> 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

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

[Tom Rini]

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

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

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

-- 
Tom

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

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

[Heinrich Schuchardt]

On 1/31/23 15:16, Simon Glass wrote:
> Hi,
> 
> On Tue, 31 Jan 2023 at 05:04, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> 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:
<snip />
>>>>
>>>> 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
> 
> How does this work if U-Boot runs grub instead of Linux? Does grub
> update the watchdog?
> 

U-Boot will reset the watchdog when the UEFI API is invoked to read the 
console or to access the network or block devices. Just grep for 
"efi_timer_check()".

Best regards

Heinrich

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

[Simon Glass]

Hi,

On Tue, 31 Jan 2023 at 05:04, Ilias Apalodimas
<ilias.apalodimas@linaro.org> 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@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

How does this work if U-Boot runs grub instead of Linux? Does grub
update the watchdog?

Regards,
Simon

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

[Ilias Apalodimas]

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

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

[Tom Rini]

[-- 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 --]

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

[Tom Rini]

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

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?

-- 
Tom

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

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

[Heinrich Schuchardt]

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)
-- 
2.38.1