From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DBE82C61DA4 for ; Thu, 2 Feb 2023 17:12:36 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 9E59E85D75; Thu, 2 Feb 2023 18:12:26 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="gL2Db3tr"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 4FD8C85D6F; Thu, 2 Feb 2023 18:12:23 +0100 (CET) Received: from mail-ej1-x631.google.com (mail-ej1-x631.google.com [IPv6:2a00:1450:4864:20::631]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id E5FEA85D1B for ; Thu, 2 Feb 2023 18:12:19 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-ej1-x631.google.com with SMTP id bk15so7870385ejb.9 for ; Thu, 02 Feb 2023 09:12:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=2Brzevo/alERG4dkIogXcXTgo24L3dMxhBmIHm1EQaQ=; b=gL2Db3trLnKFOv+988DNteeGxM+y2G9hKW41G//nBR6mVWUYVULBcdksmX+IGPBmFk oWo6lPGQXie5GHes5Jvblf0+VGNQfifljE4Eq4YN1Hznfs9xcbo2ha2N2P55JoCTQHWn uIef3u0GuYyK+rfOqaBcuEavEZ3GxaQlzjFhk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=2Brzevo/alERG4dkIogXcXTgo24L3dMxhBmIHm1EQaQ=; b=PucKxGxPUpvVz1V9WeQrt5Rr3/VFRH0R4rdKC9Bh9R4OEMXIQe2Gg5VHF7ytQ37W1l 0AOlVcx+3oSL2IBrw1CYZ1QjYd867+lhtsFoG3Of7Cywkrjhd9/rJXu7hY9l/9oljGCr hS+V5epmpq2pYL7jtoYpK+tBClVdVtjXtTLflmciTpvLSQla3XaE/rrBu4gKWjmNMi/z cZPTl2deyVWFu3ZF8+zpjGyJgCga4Dxny/CCCv8r+xy0nz9rcHQBUriH5EF28+2kzOuk JXXFfymbe8k9ytbWqfYSq/Zp/9aHgxa2mzGH4jMS3sJiLuWZwjiCbl3b6vcioMz+w95S oisw== X-Gm-Message-State: AO0yUKWdhRWvwtZ+zjuvQMDzz1UFjsX4r9pntJY2jnCDV48MW4MOQDYw mIFrExqUX7nithqGdIFBobKQmgbJACbXihLbP5KO7A== X-Google-Smtp-Source: AK7set9bcT1WJK3bNzLdNzCM3GT7MVRiio4Cf9CjEyM6ZccSIeJLbnBUW8S/6pcoUzabaR3Zx60eD4L3ccji+cCtwm0= X-Received: by 2002:a17:906:3ed6:b0:87e:975b:fbda with SMTP id d22-20020a1709063ed600b0087e975bfbdamr2210761ejj.93.1675357939150; Thu, 02 Feb 2023 09:12:19 -0800 (PST) MIME-Version: 1.0 References: <20230128085745.18389-1-heinrich.schuchardt@canonical.com> <071ebaa1-2f62-1628-106d-4f4e441c4f0d@prevas.dk> In-Reply-To: From: Simon Glass Date: Thu, 2 Feb 2023 10:12:07 -0700 Message-ID: Subject: Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices() To: Etienne Carriere Cc: Heinrich Schuchardt , Tom Rini , u-boot@lists.denx.de, Andre Przywara , Ilias Apalodimas , Rasmus Villemoes Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean 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 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