[PATCH v17 09/10] arm_ffa: efi: introduce FF-A MM communication

Wed Aug 2 18:10:52 CEST 2023

Hi Ilias,

On Wed, 2 Aug 2023 at 08:00, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Simon,
>
> On Wed, 2 Aug 2023 at 16:55, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 2 Aug 2023 at 07:48, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Wed, 2 Aug 2023 at 16:44, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Wed, 2 Aug 2023 at 07:43, Ilias Apalodimas
> > > > <ilias.apalodimas at linaro.org> wrote:
> > > > >
> > > > > On Wed, 2 Aug 2023 at 16:42, Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Ilias,
> > > > > >
> > > > > > On Wed, 2 Aug 2023 at 07:38, Ilias Apalodimas
> > > > > > <ilias.apalodimas at linaro.org> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Wed, 2 Aug 2023 at 16:34, Simon Glass <sjg at chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Ilias,
> > > > > > > >
> > > > > > > > On Wed, 2 Aug 2023 at 07:27, Ilias Apalodimas
> > > > > > > > <ilias.apalodimas at linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Simon,
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Wed, 2 Aug 2023 at 16:09, Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Ilias,
> > > > > > > > > >
> > > > > > > > > > On Wed, 2 Aug 2023 at 07:02, Ilias Apalodimas
> > > > > > > > > > <ilias.apalodimas at linaro.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Simon,
> > > > > > > > > > >
> > > > > > > > > > > On Wed, 2 Aug 2023 at 15:52, Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Ilias,
> > > > > > > > > > > >
> > > > > > > > > > > > On Wed, 2 Aug 2023 at 00:52, Ilias Apalodimas
> > > > > > > > > > > > <ilias.apalodimas at linaro.org> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Tue, 1 Aug 2023 at 19:19, Tom Rini <trini at 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