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 84048C05027 for ; Thu, 2 Feb 2023 08:17:56 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 736AC85C8F; Thu, 2 Feb 2023 09:17:54 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="vBqz30kG"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 8EFDB85AC1; Thu, 2 Feb 2023 09:17:52 +0100 (CET) Received: from mail-ed1-x529.google.com (mail-ed1-x529.google.com [IPv6:2a00:1450:4864:20::529]) (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 97D3D85CD6 for ; Thu, 2 Feb 2023 09:17:49 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=etienne.carriere@linaro.org Received: by mail-ed1-x529.google.com with SMTP id q19so1171133edd.2 for ; Thu, 02 Feb 2023 00:17:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.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=BaFLWqJ4/2n28qpXZQdm3DErdmBcM18Y04mPv72w4+o=; b=vBqz30kGDIGtlYtMLjiOHsos3ALE5UfjliCMqJZUA2J6x7ZSa27c01uouhBuSUnixF 8gI1bd8CZ0L6M9+0qLRySb1vlu6A5JgXV9koF/OgNqY4HkWzcXaRE8yzFz1hdc86SEt2 czHDUuC5FPyjQnNlDoAi6VnIUFGiAVzSYpCJRBYr3H/aqHBO67G2sW22w9Wo5lXs9ILF Xfx7m0RzA7Mrz8WNgORcRRiVSeobhQgNrvoQio2Lyi0U5SfYLPCmuCxXyKclWNiRIoky Sck2HfkrZ0rC8Ua59/pCndlGu5H7Bn7sA+NBqQQRJaZImxAI4WR6CZmtBiNHTgs1V5wz QAQg== 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=BaFLWqJ4/2n28qpXZQdm3DErdmBcM18Y04mPv72w4+o=; b=0/Tg4qCgFQcggetc59JbXBVFyZJ7WuwiRxLAoQNaqCJyVuH/Pwg88C0zOBPSnymrl4 fEINYnZZAgV3vperAKk74AzlF+qi1ee48aAayWzYwj8EdwtU36219YEHBO3Fm+V1tw4R 2J9ScGQ4+GUJMGgS8zGWS1FYwdX2l7GVytUPR4w7nLJLtVpAGFm+YTkjjnNhbh3gmuCd XyvwV/SGrBLryggK9SJZLvMMduvgPDEFRZJG/DuO3ugVnbQ2rlp/TpROHKcpGerkdrq1 f5Y2X7sdn7xpG51vnUHgh6LF3xuA3Vd2l98x51FwcqK1o0xOC7BTkJVoXQS9kMtUN2yk vFfA== X-Gm-Message-State: AO0yUKU88CiUDcyZ+E33Wi49zLSZWZMvEcnyRxk2jBpt9SLlR8uYrcZT mBB21MoJ9enCCKDEqsWIeFglkxK/evWG+WH+YBFpIA== X-Google-Smtp-Source: AK7set/wPc2++D+iF40kM6q2Ku8afiupElThHkAsOR0Nor2TQQJ9luaMwnAhFUe/PCptEL2B/XRF09W/UZeBaoquYzY= X-Received: by 2002:a05:6402:344f:b0:4a1:ca2b:986e with SMTP id l15-20020a056402344f00b004a1ca2b986emr1606493edc.41.1675325869117; Thu, 02 Feb 2023 00:17:49 -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: Etienne Carriere Date: Thu, 2 Feb 2023 09:17:38 +0100 Message-ID: Subject: Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices() To: Heinrich Schuchardt Cc: 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 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. > > Best regards > > Heinrich >