Hi Tom,
On Thu, 2 Feb 2023 at 10:22, Tom Rini <tr...@konsulko.com> wrote:
>
> On Thu, Feb 02, 2023 at 10:12:07AM -0700, Simon Glass wrote:
> > Hi,
> >
> > On Thu, 2 Feb 2023 at 01:17, Etienne Carriere
> > <etienne.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.
>
> Honestly, not really? Some good number of SoCs will start the watchdog
> in ROM and these are also the ones that don't allow you to turn it off.
I hope not, that sounds really risky. How would you debug such a platform?
Regards,
Simon
