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. Regards, Simon
