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 38055C63797 for ; Wed, 1 Feb 2023 15:21:24 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id B4B6485C84; Wed, 1 Feb 2023 16:21:21 +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="fuhBbAna"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 2233A85C81; Wed, 1 Feb 2023 16:21:19 +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 E268B85C8B for ; Wed, 1 Feb 2023 16:21:13 +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 m26so17213069qtp.9 for ; Wed, 01 Feb 2023 07:21:13 -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=hEd0flqJdz23YJkPmKULmTwXajwGj9Kll25NAs7Tewo=; b=fuhBbAna08arwXBXmVx1SUNqndLmH1Bare48FCl44ozKvXEQPL9YIIryP2O91eAc0b YroWN6Ts2OU+j5WsV+BrQzd34+2uN5E2ZwdInppM62MErASnp5DRiWU0vtEeBFX9Mt3d IuALmiNJktC8nG2YVA3VwwvAGttl6v1s9Cj/U= 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=hEd0flqJdz23YJkPmKULmTwXajwGj9Kll25NAs7Tewo=; b=UzTXzWT9bMNxRC6jO5py3l6k1ttqiUZH6UAq1DGvMJH8TqyI+aN59FryY/Kf0STUqf eY3e8Vp5L88+Hbzv8eLjK4hCfkEk5RA9TMMQ6cZn1qImZGGCau7WmUPMvcSNJvq/Ut6j yEXQv91s8eaMOSmu7OjepthK4OlgYCV0GKR8jYb8/ENrOGToIn7qaofxC0fKAQqDCzVT Cym1rLO3m8SbjO47WepEp1cFFfSsEZuOp9lP7xQT9qoUlLKM9fqZ6c1wRkIJBJI/ee9f XzjDEJssYsTbM5zi6ioAGKVK6o9Y4BtYpdcgmZk2QbjKlK52gIxmJSps7HZD9GWxlehi 2b5A== X-Gm-Message-State: AO0yUKX8yhtzpX4cE2JJm1b11ZiAbYekZ/bGSdVJ0RaIAoDx0tjX71i3 IlGMMVtPIhMdBBi58IJ+KriA6w== X-Google-Smtp-Source: AK7set9Ew5R/sJOupT3EcTA5bO6oij1jAkNi766HxkQZRWGQr5QJB53Gs5aTEJNUVhvFIzyyogyt5g== X-Received: by 2002:a05:622a:15d1:b0:3ad:202f:8797 with SMTP id d17-20020a05622a15d100b003ad202f8797mr5379673qty.9.1675264872457; Wed, 01 Feb 2023 07:21:12 -0800 (PST) Received: from bill-the-cat (2603-6081-7b00-6400-d3bc-07cf-bf94-8c52.res6.spectrum.com. [2603:6081:7b00:6400:d3bc:7cf:bf94:8c52]) by smtp.gmail.com with ESMTPSA id s1-20020ac85281000000b003b63238615fsm11869796qtn.46.2023.02.01.07.21.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 Feb 2023 07:21:12 -0800 (PST) Date: Wed, 1 Feb 2023 10:21:10 -0500 From: Tom Rini To: Mark Kettenis Cc: Rasmus Villemoes , ilias.apalodimas@linaro.org, heinrich.schuchardt@canonical.com, u-boot@lists.denx.de, andre.przywara@arm.com 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> <871qn9y2h5.fsf@bloch.sibelius.xs4all.nl> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="EufUcUQsDFwJaT/H" Content-Disposition: inline In-Reply-To: <871qn9y2h5.fsf@bloch.sibelius.xs4all.nl> 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 --EufUcUQsDFwJaT/H Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 01, 2023 at 01:49:58PM +0100, Mark Kettenis wrote: > > Date: Wed, 1 Feb 2023 09:32:54 +0100 > > From: Rasmus Villemoes > >=20 > > 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 wrot= e: > > >>>> > > >>>>> The UEFI specification requires for ExitBootServices() that "the = boot > > >>>>> services watchdog timer is disabled". We already disable the soft= ware > > >>>>> 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_b= oottime.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_s= ervices(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_s= ervices(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 spec= is > > >>>> unhelpfully wrong if it insists this? > > >>> > > >>> Because, to be clear, stopping hardware watchdogs is not to be done= =2E The > > >>> one in-tree caller of wdt_stop_all is very questionable. You cannot > > >>> seriously stop a watchdog until someone else can hopefully resume i= t as > > >>> that violates the function of a hardware watchdog. A pure software > > >>> watchdog is one thing, and a hardware watchdog is another. I feel l= ike > > >>> the most likely answer here is that someone needs to, still, push b= ack > > >>> to the UEFI specification to get hardware watchdogs better understo= od > > >>> and handled, as it must never be stopped once started and if you ca= nnot > > >>> reach the next stage in time, that's an engineering issue to resolv= e. 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 o= ver > > >>> 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 -> watch= dog > > >> gets re-initialized. In that case you are *hoping* that device w= on'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 a= llowed > > >> value. The efi-stub doesn't have any driver to refresh the wd, s= o we > > >> will again rely on the wd driver coming up and refreshing the tim= ers. > > >=20 > > > 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". > > >=20 > >=20 > > Indeed. > >=20 > > 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. >=20 > Well, for EFI the point where the handover happens is when the payload > calls ExitBootServices(). After that point the payload "owns" the > hardware exposed to it in the device tree. And the only way for the > payload to call into U-Boot at that point is through EFI runtime > services, which are not supposed to touch the hardware that is now > under control of the payload. With the exception of the ResetSystem() > EFI runtime service of course. But that call will never return and > therefore souldn't lead to any conflicts. So nothing will pet the > watchdog anymore. >=20 > The discussion is about what exactly should happen to the watchdog > when ExitBootServices() gets called. The UEFI specification says that > the (unspecified) watchdog should be disabled at that point. Which is > something that makes no sense to many people with experience in > embedded systems and may even be technically impossible on some > hardware. So U-Boot doesn't follow the spec in this regard. >=20 > What U-Boot currently does actually works just fine in most cases. > You just have to make sure that the payload takes over the petting of > the watchdog in a timely fashion after calling ExitBootServices(). > But that is not really different from what happens if you boot a Linux > kernel using one of the "legacy" boot methods that U-Boot provides > (e.g. using the "bootm" command). >=20 > The "problem" here really is that every 6 months somebody fails to > include the watchdog driver in their Linux kernel, reads the UEFI > spec, decides U-Boot is violating the spec and comes up with a diff to > "fix" U-Boot. And then we have the same discussion again. The > "solution" to that probablem is to get the UEFI spec changes to allow > for U-Boot's behaviour. But nobody here cares enough about that to > actually makes that happen. Yes, this. And I am hoping that at this point I can motivate one of the many people that do already have contacts with, and accounts in the various UEFI forums and are otherwise familiar with the processes to get this addressed. There are groups that want UEFI to be a serious choice in embedded usage and fixing the spec to understand how hardware watchdogs are used in the embedded space for very good reason will benefit them. I'm quite happy to have U-Boot continue to violate this specific part of the specification until then. --=20 Tom --EufUcUQsDFwJaT/H Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmPag18ACgkQFHw5/5Y0 tywyOgv/X4QfSoDlXzcVDOKFj1pNSTiP8vEslK9Ldxv7q9BXkQC8a7suiQr+w776 NmyHiyF6Yx0jMKZnupbEHONKkoe45DoEFylHlpLaNKlm6aHN3XMX8lqu0xbSgwOb 2s0bgIDVF0JV9SBReaU6z3v+EDTRht39uSzOwp0MxP2NVuZ3GQDNB/toedELuh5V I/3WsxUBjUj3YT3Li5jmxFnWiQM2T/Ki4PQbtLsYNQnm5g/DyO67UiYokk5zOzfM CDXBYeWn9B21oU0GzFJa/tBbQ1sDR3OYrWxuCHHHUiRC+3VAo3ETXr61JN9S9QO+ HwJDd7MCcDeeFoTKCtlb5Ik9RdgXrUsrCX/lTaHzXElmlLfHeIDl1iElicaCaFjO +GTQNorZ3oYJERScoRskKdPoHOaFMCBSrJfLPHn6aPQqsyWk+KRGjygDN4HfsQyY KTcw+528tLsmZjaSFoZtuXVfXH/XGs6qKi3AaPkE57gTiBnD342aQd9p2xXG+14f GBVXyWm+ =ERcL -----END PGP SIGNATURE----- --EufUcUQsDFwJaT/H--