Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
> 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()
> From: Michael Walle > 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()
Am 2021-11-09 15:46, schrieb Mark Kettenis: From: Michael Walle 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-mich...@walle.cc/
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
On 11/9/21 15:54, Michael Walle wrote: Am 2021-11-09 15:46, schrieb Mark Kettenis: From: Michael Walle 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-mich...@walle.cc/
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
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 > Signed-off-by: Heinrich Schuchardt 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 signature.asc Description: PGP signature
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
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 Signed-off-by: Heinrich Schuchardt 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.schucha...@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()
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 > > > Signed-off-by: Heinrich Schuchardt > > > > 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.schucha...@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 signature.asc Description: PGP signature
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
On Tue, 9 Nov 2021 13:18:28 -0500 Tom Rini 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 > > > > Signed-off-by: Heinrich Schuchardt > > > > > > 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.schucha...@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()
On Tue, Nov 9, 2021 at 5:55 PM 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 > > Signed-off-by: Heinrich Schuchardt > > 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()
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
> Signed-off-by: Heinrich Schuchardt
> ---
> 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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
signature.asc
Description: PGP signature
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
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
> > Signed-off-by: Heinrich Schuchardt
> > ---
> > 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
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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
signature.asc
Description: PGP signature
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
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
> > > Signed-off-by: Heinrich Schuchardt
> > > ---
> > > 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
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -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()
Hi,
On Tue, 31 Jan 2023 at 05:04, 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
> > > > Signed-off-by: Heinrich Schuchardt
> > > > ---
> > > > 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
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -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()
On 1/31/23 15:16, Simon Glass wrote: Hi, On Tue, 31 Jan 2023 at 05:04, 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: 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()
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
> > > > Signed-off-by: Heinrich Schuchardt
> > > > ---
> > > > 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
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -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
signature.asc
Description: PGP signature
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
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
> Signed-off-by: Heinrich Schuchardt
> ---
> 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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()
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
Signed-off-by: Heinrich Schuchardt
---
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
#include
#include
+#include
#include
#include
#include
@@ -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()
> Date: Wed, 1 Feb 2023 09:32:54 +0100
> From: 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
> > Signed-off-by: Heinrich Schuchardt
> > ---
> > 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
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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
paylo
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
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
> >
> > 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
> > > Signed-off-by: Heinrich Schuchardt
> > > ---
> > > 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
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -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 o
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
Hello Heinrich and all,
On Wed, 1 Feb 2023 at 10:00, Heinrich Schuchardt
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
> >> Signed-off-by: Heinrich Schuchardt
> >> ---
> >> 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
> >> #include
> >> #include
> >> +#include
> >> #include
> >> #include
> >> #include
> >> @@ -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.
> >
> > Unre
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
Hi,
On Thu, 2 Feb 2023 at 01:17, Etienne Carriere
wrote:
>
> Hello Heinrich and all,
>
> On Wed, 1 Feb 2023 at 10:00, Heinrich Schuchardt
> 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
> > >> Signed-off-by: Heinrich Schuchardt
> > >>
> > >> ---
> > >> 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
> > >> #include
> > >> #include
> > >> +#include
> > >> #include
> > >> #include
> > >> #include
> > >> @@ -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
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
On Thu, Feb 02, 2023 at 10:12:07AM -0700, Simon Glass wrote:
> Hi,
>
> On Thu, 2 Feb 2023 at 01:17, Etienne Carriere
> wrote:
> >
> > Hello Heinrich and all,
> >
> > On Wed, 1 Feb 2023 at 10:00, Heinrich Schuchardt
> > 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
> > > >> Signed-off-by: Heinrich Schuchardt
> > > >>
> > > >> ---
> > > >> 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
> > > >> #include
> > > >> #include
> > > >> +#include
> > > >> #include
> > > >> #include
> > > >> #include
> > > >> @@ -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
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
Hi Tom,
On Thu, 2 Feb 2023 at 10:22, Tom Rini 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
> > wrote:
> > >
> > > Hello Heinrich and all,
> > >
> > > On Wed, 1 Feb 2023 at 10:00, Heinrich Schuchardt
> > > 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
> > > > >> Signed-off-by: Heinrich Schuchardt
> > > > >>
> > > > >> ---
> > > > >> 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
> > > > >> #include
> > > > >> #include
> > > > >> +#include
> > > > >> #include
> > > > >> #include
> > > > >> #include
> > > > >> @@ -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
> > > > >>> timer
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
On 03/02/2023 03.15, Simon Glass wrote: > Hi Tom, > > On Thu, 2 Feb 2023 at 10:22, Tom Rini 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()
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 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
> > > wrote:
> > > >
> > > > Hello Heinrich and all,
> > > >
> > > > On Wed, 1 Feb 2023 at 10:00, Heinrich Schuchardt
> > > > 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
> > > > > >> Signed-off-by: Heinrich Schuchardt
> > > > > >>
> > > > > >> ---
> > > > > >> 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
> > > > > >> #include
> > > > > >> #include
> > > > > >> +#include
> > > > > >> #include
> > > > > >> #include
> > > > > >> #include
> > > > > >> @@ -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
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
Hi,
On Fri, 3 Feb 2023 at 08:51, Tom Rini 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 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
> > > > wrote:
> > > > >
> > > > > Hello Heinrich and all,
> > > > >
> > > > > On Wed, 1 Feb 2023 at 10:00, Heinrich Schuchardt
> > > > > 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
> > > > > > >> Signed-off-by: Heinrich Schuchardt
> > > > > > >>
> > > > > > >> ---
> > > > > > >> 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
> > > > > > >> #include
> > > > > > >> #include
> > > > > > >> +#include
> > > > > > >> #include
> > > > > > >> #include
> > > > > > >> #include
> > > > > > >> @@ -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 i
Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices()
>>> 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()
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()
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()
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()
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
