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 87F4EC001E0 for ; Wed, 2 Aug 2023 15:38:27 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 92E2486863; Wed, 2 Aug 2023 17:38:25 +0200 (CEST) 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="fj9qbVBw"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 68AD18686A; Wed, 2 Aug 2023 17:38:24 +0200 (CEST) Received: from mail-yw1-x112f.google.com (mail-yw1-x112f.google.com [IPv6:2607:f8b0:4864:20::112f]) (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 43ACF86857 for ; Wed, 2 Aug 2023 17:38:21 +0200 (CEST) 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-yw1-x112f.google.com with SMTP id 00721157ae682-5862a6ae535so38026487b3.0 for ; Wed, 02 Aug 2023 08:38:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; t=1690990700; x=1691595500; 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=osHhtLWZJLXCjEafdcPS2ctujY+Eipfw8m2Spk7VVL0=; b=fj9qbVBwm6kO1+kHpuT3e2K1AFsc6w25rxq6MG4umD8hilwagGJMx+GDU7SkTVHsWX 2Ipmp+JKKLRrR6RnIJzpV0IqBPzJlGhv5OP2AJ6jTJNLGrNXDTXmBMZjA3qxiL4eAggn OP9NzwT3Ya/J7n4qmXeHAmhBA+zFSe7s3UXpI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690990700; x=1691595500; 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=osHhtLWZJLXCjEafdcPS2ctujY+Eipfw8m2Spk7VVL0=; b=ejRMegKrM8kmfBsCcz0qS6YfdF8Huh3bBWPuswnTwsVSbdqaFUd+M2OGOV4vRTjbvV 66776ZLWU1Io7TAw4n5rgqjLDC+wIMfQwyGsHvg/KYn1HNRDP+IC0BRYklvrbfcnIgT/ WlffFAPyNDZWcoznO6XHv7raPTD9LWLC4qfj+rPNPwnsD4mx1cMkDDW9qjLa0jyOdtkr oM9sR0gQIWLpndKmDlgLM4s/sGuM2jjbJnueMozlYFMAIenS6BViePjMl0+vbkCWd8aD yRcELkkCRz5OMfNO4ypVoGj/so6TfOLgEICwpnvt9yMRAN9lmFvpi2uVYveOT5lTxzWx MJsw== X-Gm-Message-State: ABy/qLYvDF6e/o70rmIZKmQqbASy0kMOO5BoHrIXQxeQJoJ6wGGcOYd1 TDB+2+jUnXRUWOV5Loh2nx8vf6umGJzOF+LtNQL4rQ== X-Google-Smtp-Source: APBJJlGyVOhgE43ZgMqFfv38AnTMJ9VWBb/JTIz+RxrrcNgN5G2g8J80mHsXWqZrI9TnwVujdvK4qg== X-Received: by 2002:a81:920f:0:b0:56d:502:43d4 with SMTP id j15-20020a81920f000000b0056d050243d4mr14769890ywg.11.1690990699891; Wed, 02 Aug 2023 08:38:19 -0700 (PDT) Received: from bill-the-cat (2603-6081-7b00-6400-38a3-d353-a65e-b181.res6.spectrum.com. [2603:6081:7b00:6400:38a3:d353:a65e:b181]) by smtp.gmail.com with ESMTPSA id h128-20020a0df786000000b00565d056a74bsm4695888ywf.139.2023.08.02.08.38.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 02 Aug 2023 08:38:19 -0700 (PDT) Date: Wed, 2 Aug 2023 11:38:17 -0400 From: Tom Rini To: Simon Glass Cc: Ilias Apalodimas , Abdellatif El Khlifi , achin.gupta@arm.com, nd@arm.com, u-boot@lists.denx.de, jens.wiklander@linaro.org Subject: Re: [PATCH v17 09/10] arm_ffa: efi: introduce FF-A MM communication Message-ID: <20230802153817.GY3630934@bill-the-cat> References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="IlvZNDNyLZ99UC1Z" 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.8 at phobos.denx.de X-Virus-Status: Clean --IlvZNDNyLZ99UC1Z Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Aug 02, 2023 at 07:55:18AM -0600, Simon Glass wrote: > Hi Ilias, >=20 > On Wed, 2 Aug 2023 at 07:48, Ilias Apalodimas > wrote: > > > > Hi Simon, > > > > On Wed, 2 Aug 2023 at 16:44, Simon Glass wrote: > > > > > > Hi Ilias, > > > > > > On Wed, 2 Aug 2023 at 07:43, Ilias Apalodimas > > > wrote: > > > > > > > > On Wed, 2 Aug 2023 at 16:42, Simon Glass wrote: > > > > > > > > > > Hi Ilias, > > > > > > > > > > On Wed, 2 Aug 2023 at 07:38, Ilias Apalodimas > > > > > wrote: > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > On Wed, 2 Aug 2023 at 16:34, Simon Glass wro= te: > > > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > > > On Wed, 2 Aug 2023 at 07:27, Ilias Apalodimas > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 2 Aug 2023 at 16:09, Simon Glass = wrote: > > > > > > > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > > > > > > > On Wed, 2 Aug 2023 at 07:02, Ilias Apalodimas > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > > > > > > > On Wed, 2 Aug 2023 at 15:52, Simon Glass wrote: > > > > > > > > > > > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > > > > > > > > > > > On Wed, 2 Aug 2023 at 00:52, Ilias Apalodimas > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 1 Aug 2023 at 19:19, Tom Rini wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Aug 01, 2023 at 05:10:08PM +0100, Abdella= tif El Khlifi wrote: > > > > > > > > > > > > > > Hi guys, > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Aug 01, 2023 at 11:00:57AM -0400, Tom R= ini wrote: > > > > > > > > > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > > > > > > > > > Changelog: > > > > > > > > > > > > > > > > > > > > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > v17: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > * show a debug message rather t= han an error when FF-A is not detected > > > > > > > > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > > > > > > > diff --git a/lib/efi_loader/Kco= nfig b/lib/efi_loader/Kconfig > > > > > > > > > > > > > > > > > > > > > > index c5835e6ef6..8fbadb9201 10= 0644 > > > > > > > > > > > > > > > > > > > > > > --- a/lib/efi_loader/Kconfig > > > > > > > > > > > > > > > > > > > > > > +++ b/lib/efi_loader/Kconfig > > > > > > > > > > > > > > > > > > > > > > @@ -55,13 +55,53 @@ config EFI_= VARIABLE_FILE_STORE > > > > > > > > > > > > > > > > > > > > > > stored as file /ubootef= i.var on the EFI system partition. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > config EFI_MM_COMM_TEE > > > > > > > > > > > > > > > > > > > > > > - bool "UEFI variables stor= age service via OP-TEE" > > > > > > > > > > > > > > > > > > > > > > - depends on OPTEE > > > > > > > > > > > > > > > > > > > > > > + bool "UEFI variables stor= age service via the trusted world" > > > > > > > > > > > > > > > > > > > > > > + depends on OPTEE && ARM_F= FA_TRANSPORT > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > You didn't get my changes in here= however. If you can do EFI_MM_COMM_TEE > > > > > > > > > > > > > > > > > > > > > without ARM_FFA_TRANSPORT (as lx2= 160ardb_tfa_stmm_defconfig does) then > > > > > > > > > > > > > > > > > > > > > you don't make this option depend= on . If FF-A is only > > > > > > > > > > > > > > > > > > > > > for use here, you make FF-A depen= d on this, and the FF-A specific > > > > > > > > > > > > > > > > > > > > > variable depend on ARM_FFA_TRANSP= ORT. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Abdellatif hinted at what's going o= n here. When I added this Kconfig > > > > > > > > > > > > > > > > > > > > option to lx2160 FF-A wasn't implem= ented yet. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The defconfig has existed since May 2= 020, which is when you added > > > > > > > > > > > > > > > > > > > EFI_MM_COMM_TEE itself too. So I thi= nk it's that no one did the check I > > > > > > > > > > > > > > > > > > > did until now and saw this series was= disabling what was on the other > > > > > > > > > > > > > > > > > > > platform. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Since FF-A isn't a new > > > > > > > > > > > > > > > > > > > > communication mechanism but builds = upon the existing SMCs to build an > > > > > > > > > > > > > > > > > > > > easier API, I asked Abdellatif to h= ide this complexity. > > > > > > > > > > > > > > > > > > > > We had two options, either make Kco= nfig options for either FF-A or the > > > > > > > > > > > > > > > > > > > > traditional SMCs and remove the dep= endencies, or piggyback on FF-As > > > > > > > > > > > > > > > > > > > > discovery mechanism and make the ch= oice at runtime. The latter has a > > > > > > > > > > > > > > > > > > > > small impact on code size, but imho= makes developers' life a lot > > > > > > > > > > > > > > > > > > > > easier. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I'm not sure how much you can do a ru= n-time option here since you're > > > > > > > > > > > > > > > > > > > setting a bunch of default values for= FF-A to 0 in Kconfig. If we're > > > > > > > > > > > > > > > > > > > supposed to be able to get them at ru= n time, we shouldn't need a Kconfig > > > > > > > > > > > > > > > > > > > option at all. I'm also not sure how= valid a use case it is where we > > > > > > > > > > > > > > > > > > > won't know at build time what the res= t of the firmware stack supports > > > > > > > > > > > > > > > > > > > here. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > That's a fair point. FF-A in theory ha= s APIs to discover memory. > > > > > > > > > > > > > > > > > > Abdellatif, why do we need the Kconfigs= for shared memory on FF-A? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The statically carved out MM shared buffe= r address, size and offset cannot be discovered by FF-A ABIs. > > > > > > > > > > > > > > > > > The MM communication driver in U-Boot cou= ld allocate the buffer and share it with the MM SP but > > > > > > > > > > > > > > > > > we do not implement that support currentl= y in either U-Boot or UEFI. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Ok, that's a bit unfortunate, but Tom is ri= ght. Having the FF-A > > > > > > > > > > > > > > > > addresses show up is as confusing as having= Kconfig options for > > > > > > > > > > > > > > > > discrete options. The whole point of my su= ggestion was to make users' > > > > > > > > > > > > > > > > lives easier. Apologies for the confusion = but can you bring back the > > > > > > > > > > > > > > > > ifdefs? Looking at the patch this should b= e minimal just use > > > > > > > > > > > > > > > > ifdef ARM_FFA_TRANSPORT and ifndef ARM_FFA_= TRANSPORT. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Tom you prefer that as well? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Pending an answer to Jens' feedback, yes, goi= ng back to #ifdef's is > > > > > > > > > > > > > > > fine, especially since default values of 0 ar= e nonsense in this case > > > > > > > > > > > > > > > (and as Heinrich's patch re SYS_MALLOC_LEN sh= ows, dangerous since 0 !=3D > > > > > > > > > > > > > > > 0x0 once we do string comparisons). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I'd like to give some context why it's importan= t for Corstone-1000 platform > > > > > > > > > > > > > > that the DT passed to the kernel matches the of= ficial kernel DT. > > > > > > > > > > > > > > > > > > > > > > > > > > Note that we've set aside the "should this be in = DT or not" question. > > > > > > > > > > > > > > > > > > > > > > > > > > > There is a SystemReady IR 2.0 test checking the= DT. It compares the DT > > > > > > > > > > > > > > passed by U-Boot with a reference DT (the kerne= l DT) . The test fails if there > > > > > > > > > > > > > > is a mismatch. So, if we add a DT node in U-Boo= t and the node is not upstreamed > > > > > > > > > > > > > > to the kernel DT, the DT test will fail. > > > > > > > > > > > > > > > > > > > > > > > > > > This is overall good and progress. > > > > > > > > > > > > > > > > > > > > > > > > > > > To be approved by the kernel DT maintainers, th= e node should have a use case > > > > > > > > > > > > > > in the kernel which is not the case. > > > > > > > > > > > > > > > > > > > > > > > > > > This is, I believe / hope wrong. It needs to be = in the dt-schema > > > > > > > > > > > > > repository, not strictly "the kernel". For examp= le, bootph-all (etc) > > > > > > > > > > > > > are in dt-schema and so can be in the upstream ke= rnel but are not used > > > > > > > > > > > > > in the kernel itself. > > > > > > > > > > > > > > > > > > > > > > > > > > > There is a solution for this which is deleting = the node we don't want to pass to > > > > > > > > > > > > > > the kernel using delete-node in the U-Boot DT. > > > > > > > > > > > > > > > > > > > > > > > > > > Something like this will likely be needed, in the= end, at least for some > > > > > > > > > > > > > cases. But the goal is that everything gets in t= o dt-schema. > > > > > > > > > > > > > > > > > > > > > > > > We are already working on U-Boot on that. The idea= is rather simple. > > > > > > > > > > > > We will have an array with nodes and node entries. = Before we boot up > > > > > > > > > > > > we'll scan that array, if a node entry exists we wi= ll delete that, > > > > > > > > > > > > otherwise we will just get rid of the entire node. = That should be > > > > > > > > > > > > pretty easy to maintain and extend. U-Boot will th= en be able to use > > > > > > > > > > > > it;s internal bidings without polluting the DT we h= andover to the > > > > > > > > > > > > kernel. > > > > > > > > > > > > > > > > > > > > > > This is not pollution - we have moved past that now a= nd Linux has > > > > > > > > > > > accepted some U-Boot bindings. This is the DT and if = there are things > > > > > > > > > > > in it that are not related to Linux, it can ignore th= em. > > > > > > > > > > > > > > > > > > > > > > We should add whatever bindings we need to make U-Boo= t work > > > > > > > > > > > efficiently and correctly. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The cases that are already accepted make sense. Things= like the > > > > > > > > > > public part of the certificates used to authenticate ca= psule updates > > > > > > > > > > or the encoding of the recent a/b update nodes are not = needed in any > > > > > > > > > > way in an OS. Those don't make sense to upstream and t= hose are > > > > > > > > > > polluting the DT and need stripping > > > > > > > > > > > > > > > > > > It doesn't matter that Linux doesn't *need* it. If it is = there it will > > > > > > > > > have to accommodate it. We have loads of Linux stuff in t= he DT that > > > > > > > > > means nothing to U-Boot. Many of the bindings chosen by L= inux are > > > > > > > > > wildly inefficient for U-Boot to implement. > > > > > > > > > > > > > > > > > > We don't need to strip anything. This is not pollution. I= t is a > > > > > > > > > binding agreement between projects. > > > > > > > > > > > > > > > > > > > > > > > > > I am not a maintainer, but I doubt they view it that way. = In any > > > > > > > > > > > > > > Who? > > > > > > > > > > > > > > > case, the DT produced by u-boot fails to pass the certifica= tion on > > > > > > > > > > > > > > U-Boot > > > > > > > > > > > > > > > every single platform that uses nonupstream nodes, so clean= ing that up > > > > > > > > is needed. If people care enough and upstream those bindin= gs we can > > > > > > > > preserve them > > > > > > > > > > > > > > Yes I agree, and the bindings that are added need to be upstr= eam in > > > > > > > dt-schema. This applies also to the work that Linaro does. > > > > > > > > > > > > > > I will mention this to Sugosh as well as he is adding a publi= c key. > > > > > > > > > > > > He is already aware, he is working on a PoC that does exactly w= hat I > > > > > > described. Once we verify devices are starting to pass the > > > > > > SystemReady2.0 certification he will send an RFC > > > > > > > > > > What PoC? You mean bindings? > > > > > > > > stripping of bindings that are not upstreamed in the dt-schema > > > > > > If that is what you want to do, then the binding needs to go upstream > > > before we accept his patches. > > > > This makes no sense whatsoever. Sughosh isn't adding anything new. > > Having the public portion on the DT file is something you insisted > > upon years ago and even reverted patches from me that had the key as a > > Kconfig option (which notably suffered from non of these problems, or > > the increased complexity we are adding now). What Sughosh is adding > > is autogenerating that, instead of having to manually stitch the DT. >=20 > Yes and it forms part of the machine's DT and needs to have a binding, > just like the Binman nodes need a binding. Isn't this the whole point? >=20 > Either: > - we upstream the U-Boot bindings, or > - we allow U-Boot to put whatever it wants in there and accept it >=20 > I much prefer the first option since we can run validation on it, as > I'm sure you would prefer. But removing nodes before booting just > because we haven't upstreamed things is just going to lead to no one > bothering to do it. I think we're talking past each other now. All of the following are true: - It has historically been hard to get bindings accepted that are not used in the linux kernel. - It is not only possible but reasonable today to get bindings accepted that are not used in the linux kernel. - There are many bindings outstanding today that need to be adjusted, upstreamed and accepted to dt-schema. - For SystemReady certification, U-Boot _is_ an important part of the ecosystem. - U-Boot as a project sees this as important (and I personally see this as important). - SystemReady is going to be more strict about what can / can't be in the device tree. This is good because it will prioritize accepted bindings rather than "garbage" bindings. - Today, one can just chain up "fdt" commands to make this happen. - In the future, we'll have a cleaner way to do this. This is good because it means we won't hide these behind environment strings and so forth to "sneak" in, but instead be pragmatic about it. Heck, we should make one of the conditions for being listed there be a link to discussion on upstreaming OR why it's inappropriate. This will make sure it doesn't stifle the upstreaming of bindings. - Using non-upstreamed bindings in the device tree passed to the kernel is at least as big a deal, if not bigger a deal, for the vendor kernel tree than it is for U-Boot. --=20 Tom --IlvZNDNyLZ99UC1Z Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmTKeGYACgkQFHw5/5Y0 tywx+wv/Wq2o4p+r41JzsxLgt/29lNZJWO47dIRorXuq5jjtsgvyIydnqVtUU603 ZrurqPcnxe33AOB78j6lNMY0lIeOBt937aBIBk57LP5kOK/4DKPtVBrJ1mp1ZQ88 faVhSOS4maN69Kc7CXeb7l049x05eWamnGDzEvZaNE1AsLI0EFGvw9utPoWIXwoQ vsRuLV8CP+qTPElGKeTUbgn1LQCoE7KXJVPv7o27LRqknefcu6u7iHctQ1sI5yqe 1TjTr5YClf/WFtwI/V16zpUXbMyi/BRnX36ht50BAKj4H6UEk7tI9Ts5C3qE/Wdv EaDAF1pS++JYTyBb4EyF6+gua1hQdrr7w063kNMmiGLUeT9P7nJslwDx9QF8NHoC oEuDBlCnk9RpJvAn701rfJL14do7fR9e5c6ByTr/FNPE/WJ3GYaeow6eJgx+Jr+u 6pLbUA34Vis/8hi6Tvr+GvUN/4cO4ImRotOwvyKuPBUu/L8SQSWT66gvwCRQlQRs BKfsyNK2 =Zjtf -----END PGP SIGNATURE----- --IlvZNDNyLZ99UC1Z--