Hi,
On Thu, 2 Feb 2023 at 01:17, Etienne Carriere
<etienne.carri...@linaro.org> wrote:
>
> Hello Heinrich and all,
>
> On Wed, 1 Feb 2023 at 10:00, Heinrich Schuchardt
> <heinrich.schucha...@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.przyw...@arm.com>
> > >>>>>> Signed-off-by: Heinrich Schuchardt
> > >>>>>> <heinrich.schucha...@canonical.com>
> > >>>>>> ---
> > >>>>>> lib/efi_loader/efi_boottime.c | 10 ++++++----
> > >>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/lib/efi_loader/efi_boottime.c
> > >>>>>> b/lib/efi_loader/efi_boottime.c
> > >>>>>> index ba28989f36..71215af9d2 100644
> > >>>>>> --- a/lib/efi_loader/efi_boottime.c
> > >>>>>> +++ b/lib/efi_loader/efi_boottime.c
> > >>>>>> @@ -19,6 +19,7 @@
> > >>>>>> #include <u-boot/crc.h>
> > >>>>>> #include <usb.h>
> > >>>>>> #include <watchdog.h>
> > >>>>>> +#include <wdt.h>
> > >>>>>> #include <asm/global_data.h>
> > >>>>>> #include <asm/setjmp.h>
> > >>>>>> #include <linux/libfdt_env.h>
> > >>>>>> @@ -2171,6 +2172,11 @@ static efi_status_t EFIAPI
> > >>>>>> efi_exit_boot_services(efi_handle_t image_handle,
> > >>>>>> list_del(&evt->link);
> > >>>>>> }
> > >>>>>>
> > >>>>>> + /* Disable watchdogs */
> > >>>>>> + efi_set_watchdog(0);
> > >>>>>> + if IS_ENABLED(CONFIG_WDT)
> > >>>>>> + wdt_stop_all();
> > >>>>>> +
> > >>>>>> if (!efi_st_keep_devices) {
> > >>>>>> bootm_disable_interrupts();
> > >>>>>> if (IS_ENABLED(CONFIG_USB_DEVICE))
> > >>>>>> @@ -2196,10 +2202,6 @@ static efi_status_t EFIAPI
> > >>>>>> efi_exit_boot_services(efi_handle_t image_handle,
> > >>>>>>
> > >>>>>> /* Recalculate CRC32 */
> > >>>>>> efi_update_table_header_crc32(&systab.hdr);
> > >>>>>> -
> > >>>>>> - /* Give the payload some time to boot */
> > >>>>>> - efi_set_watchdog(0);
> > >>>>>> - schedule();
> > >>>>>> out:
> > >>>>>> if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> > >>>>>> if (ret != EFI_SUCCESS)
> > >>>>>
> > >>>>> I thought we had rejected going down this path since the UEFI spec is
> > >>>>> unhelpfully wrong if it insists this?
> > >>>>
> > >>>> Because, to be clear, stopping hardware watchdogs is not to be done.
> > >>>> The
> > >>>> one in-tree caller of wdt_stop_all is very questionable. You cannot
> > >>>> seriously stop a watchdog until someone else can hopefully resume it as
> > >>>> that violates the function of a hardware watchdog. A pure software
> > >>>> watchdog is one thing, and a hardware watchdog is another. I feel like
> > >>>> the most likely answer here is that someone needs to, still, push back
> > >>>> to the UEFI specification to get hardware watchdogs better understood
> > >>>> and handled, as it must never be stopped once started and if you cannot
> > >>>> reach the next stage in time, that's an engineering issue to resolve.
> > >>>> My
> > >>>> first guess is that ExitBootServices should service the watchdog one
> > >>>> last time to ensure the largest window of time for the OS to take over
> > >>>> servicing of the watchdog.
> > >>>>
> > >>>
> > >>> There's two scenarios I can think of
> > >>> 1. After U-Boot is done it can disable the hardware watchdog.
> > >>> The kernel will go through the EFI-stub -> kernel proper -> watchdog
> > >>> gets re-initialized. In that case you are *hoping* that device
> > >>> won't
> > >>> hang in the efi-stub or until the wd is up again.
> > >>> 2. EFI makes sure the hardware wd gets configured with the highest
> > >>> allowed
> > >>> value. The efi-stub doesn't have any driver to refresh the wd, so
> > >>> we
> > >>> will again rely on the wd driver coming up and refreshing the
> > >>> timers.
> > >>
> > >> You cannot stop the hardware watchdog, period. I think in the previous
> > >> thread about this it was noted that some hardware watchdogs cannot be
> > >> disabled, it's not function that the watchdog supports. Someone needs to
> > >> go and talk with the UEFI specification people and get this addressed.
> > >> There is no sane path for "disable the hardware watchdog".
> > >>
> > >
> > > Indeed.
> > >
> > > But I think one reasonable thing to do would be to say "ok, the payload
> > > is now ready to assume responsibility, so on the U-Boot side we stop
> > > _petting_ the watchdog(s)" (i.e. nowadays that would mean deregistering
> > > them from the cyclic framework), even if the payload still performs
> > > calls into U-Boot where we would otherwise use the opportunity to feed
> > > the watchdog. And of course it's reasonable in that case to do one last
> > > ping. Because it's also a recipe for disaster if, say, both the payload
> > > and U-Boot toggles the same gpio or frobs the same SOC registers.
> > >
> > > Unrelated, but does anybody know who "the UEFI specification people" are
> > > and how to reach out?
> > >
> > > Rasmus
> > >
> >
> > 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
