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 33DD9C05027 for ; Sat, 4 Feb 2023 00:20:43 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id AE60885FDC; Sat, 4 Feb 2023 01:20:40 +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="m9Qd3all"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id BA3C585FDD; Sat, 4 Feb 2023 01:20:37 +0100 (CET) Received: from mail-ej1-x62e.google.com (mail-ej1-x62e.google.com [IPv6:2a00:1450:4864:20::62e]) (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 7DCF685FD2 for ; Sat, 4 Feb 2023 01:20:34 +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-x62e.google.com with SMTP id bk15so19747831ejb.9 for ; Fri, 03 Feb 2023 16:20:34 -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=aUXrOiG/3LtorQPpuSJ4rEtmo204vjfdnim1PrxwSuI=; b=m9Qd3all2oRJaZ/qB0wzR//6sMGRMDcOutgPppauAA4HXRPhdpn4eIFQ0OcOyN7Whk ByROfHQyeEmgftrJTKqtOflMfvE4ddvJDv5k1HN4aKGSJBpRswD+i+z+o8NfBu9shrre gWcXl27EqT59Pfkym7caHcDfNdvqOScxFMICA= 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=aUXrOiG/3LtorQPpuSJ4rEtmo204vjfdnim1PrxwSuI=; b=iWWIPHBom8hMckZhcGQ/5xb0kvxUrn+42suAoHqgDq8h4OPS32VQra9cCILn19yvRV xl1Ml2u5iSZf8ciLG0GpnLl1NnohLKXbX/KZjBfTy6ibDdXWwSk+EsycsBcwcXMmsJ7U kHNXvAaB39MHMMJDSm/YqWMY0XjeJi89Wqsp8FcvXEZo16Y/L2xh9dF0Iwdr96tZniZ0 +f9UYzErkIa+pjwskOmMgSxZs1fZIaZfb/Zog/Boi5ROWu+HB7auv77jqoVHxeQA6I++ DKHKVrWLPGmX8reXQJ4FS3yNHR3cHN4DkUhBBZ14IB5D6cbo3JoAKrD8ZwjGfwiQVD2o GjZQ== X-Gm-Message-State: AO0yUKUndVs4TjZS2dv7XVHyFTvmIe9tdlc6+/MCuXW3QHInNmnd9l82 s+e1UOT6jvx9xwdT2z4hrbG0cUhoMsKLz0IwIQBqfA== X-Google-Smtp-Source: AK7set8zW4L+HBmac2N74nQLJIcrDbLyzQZ9rbVqKWHlOUcM1iJiph+UChdEEFkEu2LYc42Fn5qW7ui5/4fd0xWy/FU= X-Received: by 2002:a17:906:3ed6:b0:87e:975b:fbda with SMTP id d22-20020a1709063ed600b0087e975bfbdamr3758044ejj.93.1675470033719; Fri, 03 Feb 2023 16:20:33 -0800 (PST) MIME-Version: 1.0 References: <071ebaa1-2f62-1628-106d-4f4e441c4f0d@prevas.dk> In-Reply-To: From: Simon Glass Date: Fri, 3 Feb 2023 17:20:21 -0700 Message-ID: Subject: Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices() To: Tom Rini Cc: Etienne Carriere , Heinrich Schuchardt , 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 Fri, 3 Feb 2023 at 08:51, Tom Rini wrote: > > On Thu, Feb 02, 2023 at 07:15:35PM -0700, Simon Glass wrote: > > Hi Tom, > > > > On Thu, 2 Feb 2023 at 10:22, Tom Rini 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 > > > > 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. > > > > > > 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? > > Same way we have for the last 20+ years? This isn't a new thing by far. I mean, when you pause U-Boot with your JTAG debugger, how does it avoid resetting and losing your progress? On-chip watchdogs handle this, but if it is something that is started externally to the chip and generates a reset signal for the system, then I don't see how it can work. Regards, Simon