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 C1AB3C05027 for ; Fri, 3 Feb 2023 15:51:27 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id EB83385C73; Fri, 3 Feb 2023 16:51:24 +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="Mh0tnBHB"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id ED55885BA6; Fri, 3 Feb 2023 16:51:21 +0100 (CET) Received: from mail-qt1-x836.google.com (mail-qt1-x836.google.com [IPv6:2607:f8b0:4864:20::836]) (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 DA92E85A49 for ; Fri, 3 Feb 2023 16:51:16 +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-x836.google.com with SMTP id g8so5845735qtq.13 for ; Fri, 03 Feb 2023 07:51:16 -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=lqbnKXWbUT/Cl5r7euSmej3BBqM0AO6CGaIKnJt8dQo=; b=Mh0tnBHBRq/tOfXq8wEkseHlBCl29tLpFl1/UvUnj7ellU4bbEEmWQ2fvG+JhhGUcg 10HWNMVZOxEQ6o3yzMAJq2mU50N2b98yOHHbANGIQLMlEaTW+gc1LAMlPiOiSuZ631h8 QAXAOjs7mhXtJMXucH+yJX/X4PlCNTBJZBlYo= 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=lqbnKXWbUT/Cl5r7euSmej3BBqM0AO6CGaIKnJt8dQo=; b=ZFw1sIWxlDp/0g0euElIFwQNHBYhmsb9drnQY1YmkqT4THVBwDMu+NhMgPM8OUfC4H VWJxaoAsSz0VR6zDZo0Zc6pqZv/3CcFNDj/s7rI0foUC44S0a14oGSAR0Vx1EETCD+Vt 1rpMfwYJR2p0Zk2zQtT/L30+7fg/vP72OSSwIVOwUIJpNvT36pEh1X/GVSJScuxMFTdX j9KIm/rzIola5epRWULp1ODKPlU3X1a8qRb++5L2tlTdJYGzIBtXQEmSuzlOyMaLvH9a /WPZ0Y3zbTPOwgUb3mmG1lbypmkokm7XMZC2TD1NHafoHH6WYcoqVNUEGnlx8QAPfNBj h2/g== X-Gm-Message-State: AO0yUKXCjvWsflh031hNmV+iEKjQEqSSKBOKn8ARvyeqkoMYuMrK1q4O 6v/LW41pxXJtj1lu+Qb1bXIMQQ== X-Google-Smtp-Source: AK7set+o/+hXjoKnFMN6xw0aoTqWyudTk+pD3kGzI059W44FutidcmWo60b3Dc7a8f2jvrzw24bccQ== X-Received: by 2002:ac8:7fc3:0:b0:3b6:2b39:4373 with SMTP id b3-20020ac87fc3000000b003b62b394373mr19872361qtk.23.1675439475460; Fri, 03 Feb 2023 07:51:15 -0800 (PST) Received: from bill-the-cat (2603-6081-7b00-6400-d049-1ce1-b939-5115.res6.spectrum.com. [2603:6081:7b00:6400:d049:1ce1:b939:5115]) by smtp.gmail.com with ESMTPSA id t15-20020a05620a450f00b006fa43e139b5sm2060966qkp.59.2023.02.03.07.51.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 03 Feb 2023 07:51:14 -0800 (PST) Date: Fri, 3 Feb 2023 10:51:12 -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: <071ebaa1-2f62-1628-106d-4f4e441c4f0d@prevas.dk> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="BV1pzKwbwKrlGqrr" 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 --BV1pzKwbwKrlGqrr Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Feb 02, 2023 at 07:15:35PM -0700, Simon Glass wrote: > Hi Tom, >=20 > 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 wro= te: > > > > > >>> 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 Schuchar= dt wrote: > > > > > >>>>> > > > > > >>>>>> The UEFI specification requires for ExitBootServices() tha= t "the boot > > > > > >>>>>> services watchdog timer is disabled". We already disable t= he software > > > > > >>>>>> watchdog. We should additionally disable the hardware watc= hdogs. > > > > > >>>>>> > > > > > >>>>>> 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_loade= r/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 !=3D EFI_SUCCESS) > > > > > >>>>> > > > > > >>>>> I thought we had rejected going down this path since the UE= FI 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 r= esume it as > > > > > >>>> that violates the function of a hardware watchdog. A pure so= ftware > > > > > >>>> 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 u= nderstood > > > > > >>>> 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 watc= hdog 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 hi= ghest allowed > > > > > >>> value. The efi-stub doesn't have any driver to refresh t= he 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 ca= nnot be > > > > > >> disabled, it's not function that the watchdog supports. Someon= e needs to > > > > > >> go and talk with the UEFI specification people and get this ad= dressed. > > > > > >> 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 dereg= istering > > > > > > them from the cyclic framework), even if the payload still perf= orms > > > > > > 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 register= s. > > > > > > > > > > > > Unrelated, but does anybody know who "the UEFI specification pe= ople" are > > > > > > and how to reach out? > > > > > > > > > > > > Rasmus > > > > > > > > > > > > > > > > After ExitBootServices() the memory occupied by U-Boot will be re= used by > > > > > the operating system. Don't expect any U-Boot interrupt vector co= de to > > > > > exist after this point. > > > > > > > > > > If the hardware watchdog is not configured to immediately reset t= he CPU > > > > > but create an interrupt instead, anything may happen. > > > > > > > > > > @Tom > > > > > Are all hardware watchdogs used in U-Boot configured to immediate= ly > > > > > 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 h= ave 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. >=20 > 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. --=20 Tom --BV1pzKwbwKrlGqrr Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmPdLW4ACgkQFHw5/5Y0 tyxBkgwAsWC+IbqssnKqU2IePe/zNT16clM8GGP/K/qRtmc/KSnUzXPD58ihvP3B vr9rlsGPmNx/duqfRp23kWOgLo0zLimdsWj4W9MWXpZUFE4zA/+/CbrsvBlbBvHD Z+MIK+fazXr784cdeCNRtzxXJCq6DZtgy9Zpe5y0+ZRofSQoOUP9jpiV2Z/+JvjY XIDtADSIlxGJc+g5p+nHUTz9VB4G/nQGy4fyVn/uimU3AxTEkJI04Pn6jqyRIMst JYtAVSno5JyZ/3WE/B4kYtPZoWJyMlPgkkogGszwsVb+Bdcd15ojaLki55vYxkCD w80Sq/LVzQLAmsRoswEGCi1P+LLgZ20xX/EOKHbGwujhXPZATCXBR2mzrvjYuPO4 7JpMHfG/7TJernOfu5Ec/hPXsSuYT3CZlqbyOaVRBOyfwjt21qdcsrmNrJfl/qTs d8GgK92OhWlFF716MHB4TeGqRR1AkDjXjipC5MHJaBw+5qeL/CsvmsBUEf2XwUbe ABBZ/sn2 =bOzB -----END PGP SIGNATURE----- --BV1pzKwbwKrlGqrr--