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 AD7B5C61DA4 for ; Thu, 2 Feb 2023 17:22:36 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 6091685D39; Thu, 2 Feb 2023 18:22:34 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=konsulko.com 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=konsulko.com header.i=@konsulko.com header.b="i6dy1f3N"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 9C49A85D3E; Thu, 2 Feb 2023 18:22:31 +0100 (CET) Received: from mail-qt1-x833.google.com (mail-qt1-x833.google.com [IPv6:2607:f8b0:4864:20::833]) (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 2046E85D1F for ; Thu, 2 Feb 2023 18:22:28 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=trini@konsulko.com Received: by mail-qt1-x833.google.com with SMTP id z5so2636836qtn.8 for ; Thu, 02 Feb 2023 09:22:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=B4otVY0yUD9noDNj/1l7CXEDN5bZMwCYi4UgMs9eAIU=; b=i6dy1f3NdXcRmLUnx1cACaPp17Q4yXn48LfYzOGHLG4tuNxraiZkwUKKLKJUgDDKmy yDSOQshHv4CIn+U7B9jX8XdK8rotjAMx5DiCYaclDtYqqh/XdXoG6ozUl4I2ZwP91mun 5F1fbw1x26qfJYkCGUK8kvQZw7rNGn2N03B2c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=B4otVY0yUD9noDNj/1l7CXEDN5bZMwCYi4UgMs9eAIU=; b=IciZz7HEclNhYcyWoeePHhiE/W/1h5QPQTWnyCAvfT91TKk624T+Kous9JDH671Z57 HNyKgNaHWae5Pv3QXm6BYjnZN16HvgL4YGbv6SNcKeNo+uc0r2nvz7bwFG0V+umsMoyD AbxA7LWFMnXoy5M+OU/wBw0plgEhnAp3PcvAIYXJaf0/6aJ4jio6fiy0GOrz9QErt0MA pjHvpnLydmYNh7RY+iR5QPY3tP5My1oV6e3AurRBVkywLWbXNwuJjrTlIyVAdBA3Xadj j0NGBsHe6XvmfY1p0ctUaawYfm0jciIf7cltKzqej/BqtfF69ZeNOlEPyU5POiNTocJV 3A0g== X-Gm-Message-State: AO0yUKUxwyWAJ6iyfxMx68llTXrSHOFW4jWGQ8B+347L8iWvJj8iCY/p OWoxGi3M3mxz9bNTqjgFr592NQ== X-Google-Smtp-Source: AK7set+cAEwJ/Wr6TjZsPO+ZW93+AN4RLTSfvi6kHCyR+evJELj2nOrws+hhEC3wxmrfMmagi9RACQ== X-Received: by 2002:a05:622a:1984:b0:3b6:3165:d887 with SMTP id u4-20020a05622a198400b003b63165d887mr12021630qtc.59.1675358546839; Thu, 02 Feb 2023 09:22:26 -0800 (PST) Received: from bill-the-cat (2603-6081-7b00-6400-648e-c327-f936-ec17.res6.spectrum.com. [2603:6081:7b00:6400:648e:c327:f936:ec17]) by smtp.gmail.com with ESMTPSA id ej10-20020a05622a4f8a00b003b2ea9b76d0sm3833026qtb.34.2023.02.02.09.22.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 02 Feb 2023 09:22:26 -0800 (PST) Date: Thu, 2 Feb 2023 12:22:24 -0500 From: Tom Rini To: Simon Glass Cc: Etienne Carriere , Heinrich Schuchardt , u-boot@lists.denx.de, Andre Przywara , Ilias Apalodimas , Rasmus Villemoes Subject: Re: [PATCH 1/1] efi_loader: stop watchdogs in ExitBootServices() Message-ID: References: <20230128085745.18389-1-heinrich.schuchardt@canonical.com> <071ebaa1-2f62-1628-106d-4f4e441c4f0d@prevas.dk> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="UYIje6C6G9rjywN0" Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett 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 --UYIje6C6G9rjywN0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Feb 02, 2023 at 10:12:07AM -0700, Simon Glass wrote: > Hi, >=20 > 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 w= rote: > > > >>>>> > > > >>>>>> The UEFI specification requires for ExitBootServices() that "t= he boot > > > >>>>>> services watchdog timer is disabled". We already disable the s= oftware > > > >>>>>> watchdog. We should additionally disable the hardware watchdog= s. > > > >>>>>> > > > >>>>>> 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/ef= i_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_boo= t_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_boo= t_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 !=3D EFI_SUCCESS) > > > >>>>> > > > >>>>> I thought we had rejected going down this path since the UEFI s= pec is > > > >>>>> unhelpfully wrong if it insists this? > > > >>>> > > > >>>> Because, to be clear, stopping hardware watchdogs is not to be d= one. The > > > >>>> one in-tree caller of wdt_stop_all is very questionable. You can= not > > > >>>> seriously stop a watchdog until someone else can hopefully resum= e it as > > > >>>> that violates the function of a hardware watchdog. A pure softwa= re > > > >>>> watchdog is one thing, and a hardware watchdog is another. I fee= l like > > > >>>> the most likely answer here is that someone needs to, still, pus= h back > > > >>>> to the UEFI specification to get hardware watchdogs better under= stood > > > >>>> 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 res= olve. My > > > >>>> first guess is that ExitBootServices should service the watchdog= one > > > >>>> last time to ensure the largest window of time for the OS to tak= e 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 -> w= atchdog > > > >>> gets re-initialized. In that case you are *hoping* that devi= ce 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 highes= t allowed > > > >>> value. The efi-stub doesn't have any driver to refresh the w= d, 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 prev= ious > > > >> thread about this it was noted that some hardware watchdogs cannot= be > > > >> disabled, it's not function that the watchdog supports. Someone ne= eds to > > > >> go and talk with the UEFI specification people and get this addres= sed. > > > >> 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 pay= load > > > > is now ready to assume responsibility, so on the U-Boot side we stop > > > > _petting_ the watchdog(s)" (i.e. nowadays that would mean deregiste= ring > > > > them from the cyclic framework), even if the payload still performs > > > > calls into U-Boot where we would otherwise use the opportunity to f= eed > > > > 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 pay= load > > > > 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 C= PU > > > 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. >=20 > 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. --=20 Tom --UYIje6C6G9rjywN0 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmPb8UwACgkQFHw5/5Y0 tyyn+Qv/aOfBpGC+in5LA1PQMvD13dt0xj4r3uG4k2LpQFpkBAl6HoEgwzzVaJPM GdFu7y5Oih2+2n+/TJVCZIOB0E5t6s03iFs7yu1bHFNi6lSdBxSifdG4GDQhop6s cd48pXxzK4vZ9eaXXMNhSjLqBDAUfSlJiloFmIJRKNKuwfxsEX44dy972nO8Ul4A Xc0hwQ4N7kF8tmtOvVMlyDr4rTBkzkJ+ShXiPlihtffqC+FGjXwn658elRnXuvfU 7OFxPcsUdJr6kRLXLtHLT+CHtEH0L9GXeYhj4f2gtDl7ImebhXCVE4ulkgkUMqmG I8mJNwEr5uC1VtoehZ7u9lF2sGX/Zc4VXmMNKgiCKXU2qoNbpwBQa0vKA4dgd0af odVJpKjXXhanuSAz+kQv5A460Bwhse30Wg/d8mQNlRMh7wB+feHGc+7eNV/ROU02 tufcSGHvQDxoPBZ++2/mYPWXU7dy1XtCGuKtLiEKsB46ya2e4b9fOTn6tPkz2437 gZmglUSo =OLao -----END PGP SIGNATURE----- --UYIje6C6G9rjywN0--