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

[Tom Rini <trini@konsulko.com>]

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

On Thu, Feb 02, 2023 at 10:12:07AM -0700, Simon Glass wrote:
> Hi,
> 
> On Thu, 2 Feb 2023 at 01:17, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Hello Heinrich and all,
> >
> > On Wed, 1 Feb 2023 at 10:00, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> > >
> > >
> > >
> > > On 2/1/23 09:32, Rasmus Villemoes wrote:
> > > > On 31/01/2023 16.07, Tom Rini wrote:
> > > >> On Tue, Jan 31, 2023 at 02:03:10PM +0200, Ilias Apalodimas wrote:
> > > >>> Hi all,
> > > >>>
> > > >>> On Mon, Jan 30, 2023 at 01:30:49PM -0500, Tom Rini wrote:
> > > >>>> On Mon, Jan 30, 2023 at 01:13:55PM -0500, Tom Rini wrote:
> > > >>>>> On Sat, Jan 28, 2023 at 09:57:45AM +0100, Heinrich Schuchardt wrote:
> > > >>>>>
> > > >>>>>> The UEFI specification requires for ExitBootServices() that "the boot
> > > >>>>>> services watchdog timer is disabled". We already disable the software
> > > >>>>>> watchdog. We should additionally disable the hardware watchdogs.
> > > >>>>>>
> > > >>>>>> Reported-by: Andre Przywara <andre.przywara@arm.com>
> > > >>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > >>>>>> ---
> > > >>>>>>   lib/efi_loader/efi_boottime.c | 10 ++++++----
> > > >>>>>>   1 file changed, 6 insertions(+), 4 deletions(-)
> > > >>>>>>
> > > >>>>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > > >>>>>> index ba28989f36..71215af9d2 100644
> > > >>>>>> --- a/lib/efi_loader/efi_boottime.c
> > > >>>>>> +++ b/lib/efi_loader/efi_boottime.c
> > > >>>>>> @@ -19,6 +19,7 @@
> > > >>>>>>   #include <u-boot/crc.h>
> > > >>>>>>   #include <usb.h>
> > > >>>>>>   #include <watchdog.h>
> > > >>>>>> +#include <wdt.h>
> > > >>>>>>   #include <asm/global_data.h>
> > > >>>>>>   #include <asm/setjmp.h>
> > > >>>>>>   #include <linux/libfdt_env.h>
> > > >>>>>> @@ -2171,6 +2172,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
> > > >>>>>>                          list_del(&evt->link);
> > > >>>>>>          }
> > > >>>>>>
> > > >>>>>> +        /* Disable watchdogs */
> > > >>>>>> +        efi_set_watchdog(0);
> > > >>>>>> +        if IS_ENABLED(CONFIG_WDT)
> > > >>>>>> +                wdt_stop_all();
> > > >>>>>> +
> > > >>>>>>          if (!efi_st_keep_devices) {
> > > >>>>>>                  bootm_disable_interrupts();
> > > >>>>>>                  if (IS_ENABLED(CONFIG_USB_DEVICE))
> > > >>>>>> @@ -2196,10 +2202,6 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
> > > >>>>>>
> > > >>>>>>          /* Recalculate CRC32 */
> > > >>>>>>          efi_update_table_header_crc32(&systab.hdr);
> > > >>>>>> -
> > > >>>>>> -        /* Give the payload some time to boot */
> > > >>>>>> -        efi_set_watchdog(0);
> > > >>>>>> -        schedule();
> > > >>>>>>   out:
> > > >>>>>>          if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> > > >>>>>>                  if (ret != EFI_SUCCESS)
> > > >>>>>
> > > >>>>> I thought we had rejected going down this path since the UEFI spec is
> > > >>>>> unhelpfully wrong if it insists this?
> > > >>>>
> > > >>>> Because, to be clear, stopping hardware watchdogs is not to be done. The
> > > >>>> one in-tree caller of wdt_stop_all is very questionable. You cannot
> > > >>>> seriously stop a watchdog until someone else can hopefully resume it as
> > > >>>> that violates the function of a hardware watchdog. A pure software
> > > >>>> watchdog is one thing, and a hardware watchdog is another. I feel like
> > > >>>> the most likely answer here is that someone needs to, still, push back
> > > >>>> to the UEFI specification to get hardware watchdogs better understood
> > > >>>> and handled, as it must never be stopped once started and if you cannot
> > > >>>> reach the next stage in time, that's an engineering issue to resolve. My
> > > >>>> first guess is that ExitBootServices should service the watchdog one
> > > >>>> last time to ensure the largest window of time for the OS to take over
> > > >>>> servicing of the watchdog.
> > > >>>>
> > > >>>
> > > >>> There's two scenarios I can think of
> > > >>> 1. After U-Boot is done it can disable the hardware watchdog.
> > > >>>     The kernel will go through the EFI-stub -> kernel proper -> watchdog
> > > >>>     gets re-initialized.  In that case you are *hoping* that device won't
> > > >>>     hang in the efi-stub or until the wd is up again.
> > > >>> 2. EFI makes sure the hardware wd gets configured with the highest allowed
> > > >>>     value.  The efi-stub doesn't have any driver to refresh the wd, so we
> > > >>>     will again rely on the wd driver coming up and refreshing the timers.
> > > >>
> > > >> You cannot stop the hardware watchdog, period. I think in the previous
> > > >> thread about this it was noted that some hardware watchdogs cannot be
> > > >> disabled, it's not function that the watchdog supports. Someone needs to
> > > >> go and talk with the UEFI specification people and get this addressed.
> > > >> There is no sane path for "disable the hardware watchdog".
> > > >>
> > > >
> > > > Indeed.
> > > >
> > > > But I think one reasonable thing to do would be to say "ok, the payload
> > > > is now ready to assume responsibility, so on the U-Boot side we stop
> > > > _petting_ the watchdog(s)" (i.e. nowadays that would mean deregistering
> > > > them from the cyclic framework), even if the payload still performs
> > > > calls into U-Boot where we would otherwise use the opportunity to feed
> > > > the watchdog. And of course it's reasonable in that case to do one last
> > > > ping. Because it's also a recipe for disaster if, say, both the payload
> > > > and U-Boot toggles the same gpio or frobs the same SOC registers.
> > > >
> > > > Unrelated, but does anybody know who "the UEFI specification people" are
> > > > and how to reach out?
> > > >
> > > > Rasmus
> > > >
> > >
> > > After ExitBootServices() the memory occupied by U-Boot will be reused by
> > > the operating system. Don't expect any U-Boot interrupt vector code to
> > > exist after this point.
> > >
> > > If the hardware watchdog is not configured to immediately reset the CPU
> > > but create an interrupt instead, anything may happen.
> > >
> > > @Tom
> > > Are all hardware watchdogs used in U-Boot configured to immediately
> > > reset the CPU?
> >
> > I guess not all but there are some. Likely related to some chip
> > specific fuse(s), once burnt a watchdog is initially kicked at reset
> > and can't be stopped.
> > We're indeed facing issues with hardware watchdogs timeout
> > capabilities when booting EFI bootloaders or even kernels that may
> > take time to download and install their watchdog driver as a kmod.
> > Extending timeout capabilities looks like the only viable way to
> > address the later case.
> >
> > BR,
> > Etienne
> >
> > >
> > > The UEFI Forum's site is https://uefi.org/. Bugs are reported via
> > > https://bugzilla.tianocore.org/. For changing the spec you will have to
> > > create a change request in their 'Mantis' system.
> 
> Just to point out that this sort of thing is easier with the VBE
> approach, since the OS can tell U-Boot whether it supports a watchdog
> or not.

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

-- 
Tom

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