Hi Ilias, On Wed, 2 Aug 2023 at 08:00, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > Hi Simon, > > On Wed, 2 Aug 2023 at 16:55, Simon Glass <s...@chromium.org> wrote: > > > > Hi Ilias, > > > > On Wed, 2 Aug 2023 at 07:48, Ilias Apalodimas > > <ilias.apalodi...@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > On Wed, 2 Aug 2023 at 16:44, Simon Glass <s...@chromium.org> wrote: > > > > > > > > Hi Ilias, > > > > > > > > On Wed, 2 Aug 2023 at 07:43, Ilias Apalodimas > > > > <ilias.apalodi...@linaro.org> wrote: > > > > > > > > > > On Wed, 2 Aug 2023 at 16:42, Simon Glass <s...@chromium.org> wrote: > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > On Wed, 2 Aug 2023 at 07:38, Ilias Apalodimas > > > > > > <ilias.apalodi...@linaro.org> wrote: > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > On Wed, 2 Aug 2023 at 16:34, Simon Glass <s...@chromium.org> > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > > > > > On Wed, 2 Aug 2023 at 07:27, Ilias Apalodimas > > > > > > > > <ilias.apalodi...@linaro.org> wrote: > > > > > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 2 Aug 2023 at 16:09, Simon Glass <s...@chromium.org> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > > > > > > > > > On Wed, 2 Aug 2023 at 07:02, Ilias Apalodimas > > > > > > > > > > <ilias.apalodi...@linaro.org> wrote: > > > > > > > > > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > > > > > > > > > On Wed, 2 Aug 2023 at 15:52, Simon Glass > > > > > > > > > > > <s...@chromium.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 2 Aug 2023 at 00:52, Ilias Apalodimas > > > > > > > > > > > > <ilias.apalodi...@linaro.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 1 Aug 2023 at 19:19, Tom Rini > > > > > > > > > > > > > <tr...@konsulko.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Aug 01, 2023 at 05:10:08PM +0100, > > > > > > > > > > > > > > Abdellatif El Khlifi wrote: > > > > > > > > > > > > > > > Hi guys, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Aug 01, 2023 at 11:00:57AM -0400, Tom > > > > > > > > > > > > > > > Rini wrote: > > > > > > > > > > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > > > > > > > > > > Changelog: > > > > > > > > > > > > > > > > > > > > > > > =============== > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > v17: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > * show a debug message rather > > > > > > > > > > > > > > > > > > > > > > > than an error when FF-A is not > > > > > > > > > > > > > > > > > > > > > > > detected > > > > > > > > > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > > > > > > > > diff --git > > > > > > > > > > > > > > > > > > > > > > > a/lib/efi_loader/Kconfig > > > > > > > > > > > > > > > > > > > > > > > b/lib/efi_loader/Kconfig > > > > > > > > > > > > > > > > > > > > > > > index c5835e6ef6..8fbadb9201 > > > > > > > > > > > > > > > > > > > > > > > 100644 > > > > > > > > > > > > > > > > > > > > > > > --- a/lib/efi_loader/Kconfig > > > > > > > > > > > > > > > > > > > > > > > +++ b/lib/efi_loader/Kconfig > > > > > > > > > > > > > > > > > > > > > > > @@ -55,13 +55,53 @@ config > > > > > > > > > > > > > > > > > > > > > > > EFI_VARIABLE_FILE_STORE > > > > > > > > > > > > > > > > > > > > > > > stored as file > > > > > > > > > > > > > > > > > > > > > > > /ubootefi.var on the EFI system > > > > > > > > > > > > > > > > > > > > > > > partition. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > config EFI_MM_COMM_TEE > > > > > > > > > > > > > > > > > > > > > > > - bool "UEFI variables > > > > > > > > > > > > > > > > > > > > > > > storage service via OP-TEE" > > > > > > > > > > > > > > > > > > > > > > > - depends on OPTEE > > > > > > > > > > > > > > > > > > > > > > > + bool "UEFI variables > > > > > > > > > > > > > > > > > > > > > > > storage service via the trusted > > > > > > > > > > > > > > > > > > > > > > > world" > > > > > > > > > > > > > > > > > > > > > > > + depends on OPTEE && > > > > > > > > > > > > > > > > > > > > > > > ARM_FFA_TRANSPORT > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > You didn't get my changes in here > > > > > > > > > > > > > > > > > > > > > > however. If you can do > > > > > > > > > > > > > > > > > > > > > > EFI_MM_COMM_TEE > > > > > > > > > > > > > > > > > > > > > > without ARM_FFA_TRANSPORT (as > > > > > > > > > > > > > > > > > > > > > > lx2160ardb_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 depend > > > > > > > > > > > > > > > > > > > > > > on this, and the FF-A specific > > > > > > > > > > > > > > > > > > > > > > variable depend on > > > > > > > > > > > > > > > > > > > > > > ARM_FFA_TRANSPORT. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Abdellatif hinted at what's going on > > > > > > > > > > > > > > > > > > > > > here. When I added this Kconfig > > > > > > > > > > > > > > > > > > > > > option to lx2160 FF-A wasn't > > > > > > > > > > > > > > > > > > > > > implemented yet. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The defconfig has existed since May > > > > > > > > > > > > > > > > > > > > 2020, which is when you added > > > > > > > > > > > > > > > > > > > > EFI_MM_COMM_TEE itself too. So I think > > > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > hide this complexity. > > > > > > > > > > > > > > > > > > > > > We had two options, either make > > > > > > > > > > > > > > > > > > > > > Kconfig options for either FF-A or the > > > > > > > > > > > > > > > > > > > > > traditional SMCs and remove the > > > > > > > > > > > > > > > > > > > > > dependencies, or piggyback on FF-As > > > > > > > > > > > > > > > > > > > > > discovery mechanism and make the > > > > > > > > > > > > > > > > > > > > > choice 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 > > > > > > > > > > > > > > > > > > > > run-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 run > > > > > > > > > > > > > > > > > > > > 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 rest > > > > > > > > > > > > > > > > > > > > of the firmware stack supports > > > > > > > > > > > > > > > > > > > > here. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > That's a fair point. FF-A in theory has > > > > > > > > > > > > > > > > > > > APIs to discover memory. > > > > > > > > > > > > > > > > > > > Abdellatif, why do we need the Kconfigs > > > > > > > > > > > > > > > > > > > for shared memory on FF-A? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The statically carved out MM shared buffer > > > > > > > > > > > > > > > > > > address, size and offset cannot be > > > > > > > > > > > > > > > > > > discovered by FF-A ABIs. > > > > > > > > > > > > > > > > > > The MM communication driver in U-Boot could > > > > > > > > > > > > > > > > > > allocate the buffer and share it with the > > > > > > > > > > > > > > > > > > MM SP but > > > > > > > > > > > > > > > > > > we do not implement that support currently > > > > > > > > > > > > > > > > > > in either U-Boot or UEFI. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Ok, that's a bit unfortunate, but Tom is > > > > > > > > > > > > > > > > > right. Having the FF-A > > > > > > > > > > > > > > > > > addresses show up is as confusing as having > > > > > > > > > > > > > > > > > Kconfig options for > > > > > > > > > > > > > > > > > discrete options. The whole point of my > > > > > > > > > > > > > > > > > suggestion was to make users' > > > > > > > > > > > > > > > > > lives easier. Apologies for the confusion > > > > > > > > > > > > > > > > > but can you bring back the > > > > > > > > > > > > > > > > > ifdefs? Looking at the patch this should be > > > > > > > > > > > > > > > > > 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, going > > > > > > > > > > > > > > > > back to #ifdef's is > > > > > > > > > > > > > > > > fine, especially since default values of 0 are > > > > > > > > > > > > > > > > nonsense in this case > > > > > > > > > > > > > > > > (and as Heinrich's patch re SYS_MALLOC_LEN > > > > > > > > > > > > > > > > shows, dangerous since 0 != > > > > > > > > > > > > > > > > 0x0 once we do string comparisons). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I'd like to give some context why it's important > > > > > > > > > > > > > > > for Corstone-1000 platform > > > > > > > > > > > > > > > that the DT passed to the kernel matches the > > > > > > > > > > > > > > > official 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 kernel > > > > > > > > > > > > > > > DT) . The test fails if there > > > > > > > > > > > > > > > is a mismatch. So, if we add a DT node in U-Boot > > > > > > > > > > > > > > > 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, the > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > example, bootph-all (etc) > > > > > > > > > > > > > > are in dt-schema and so can be in the upstream > > > > > > > > > > > > > > kernel 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 to > > > > > > > > > > > > > > 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 will > > > > > > > > > > > > > delete that, > > > > > > > > > > > > > otherwise we will just get rid of the entire node. > > > > > > > > > > > > > That should be > > > > > > > > > > > > > pretty easy to maintain and extend. U-Boot will then > > > > > > > > > > > > > be able to use > > > > > > > > > > > > > it;s internal bidings without polluting the DT we > > > > > > > > > > > > > handover to the > > > > > > > > > > > > > kernel. > > > > > > > > > > > > > > > > > > > > > > > > This is not pollution - we have moved past that now and > > > > > > > > > > > > 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 them. > > > > > > > > > > > > > > > > > > > > > > > > We should add whatever bindings we need to make U-Boot > > > > > > > > > > > > work > > > > > > > > > > > > efficiently and correctly. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The cases that are already accepted make sense. Things > > > > > > > > > > > like the > > > > > > > > > > > public part of the certificates used to authenticate > > > > > > > > > > > capsule 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 > > > > > > > > > > > those 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 the > > > > > > > > > > DT that > > > > > > > > > > means nothing to U-Boot. Many of the bindings chosen by > > > > > > > > > > Linux are > > > > > > > > > > wildly inefficient for U-Boot to implement. > > > > > > > > > > > > > > > > > > > > We don't need to strip anything. This is not pollution. It > > > > > > > > > > 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 > > > > > > > > > certification on > > > > > > > > > > > > > > > > U-Boot > > > > > > > > > > > > > > > > > every single platform that uses nonupstream nodes, so > > > > > > > > > cleaning that up > > > > > > > > > is needed. If people care enough and upstream those bindings > > > > > > > > > we can > > > > > > > > > preserve them > > > > > > > > > > > > > > > > Yes I agree, and the bindings that are added need to be > > > > > > > > upstream 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 public > > > > > > > > key. > > > > > > > > > > > > > > He is already aware, he is working on a PoC that does exactly > > > > > > > what 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. > > > > 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? > > > > Either: > > - we upstream the U-Boot bindings, or > > - we allow U-Boot to put whatever it wants in there and accept it > > The latter is not an option. U-Boot is the only certified systemready > bootloader and you are trying to break that.
I am missing something here, to say the least. > > > > > 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. > > If you read up a few mail before you'll figure out no one has to do > that manually. We can have an array that applies to all internal > bindings and clean them up for all boards. > > You've tried to upstream bindings for years and only a handful has > been accepted. Can you find something here? I remember perhaps sending one (uart clock-frequency?), but it was mostly the discussions with a kernel maintainer that made me see it was a waste of time. Of course my memory is not that great. It is certainly true that I gave up a long time ago and only restarted due to a change of guard. > So delaying the certification process because of > internal U-Boot ABI issues isn't really sane for me. I am not suggesting that...we just need to upstream our bindings. That is all. If there is no path to upstream, then we either have a bad binding or we have a process / people problem. Either way, we should fix it and carry on. Regards, Simon
