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

Wed Aug 2 15:59:35 CEST 2023

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 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.  So delaying the certification process because of
internal U-Boot ABI issues isn't really sane for me.

Thanks
/Ilias

>
> Regards,
> Simon