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

[Abdellatif El Khlifi]

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.

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.

To be approved by the kernel DT maintainers, the node should have a use case
in the kernel which is not the case.

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.

This method is used by many boards in U-Boot.

Example: /delete-node/ mm-comms-buf at 02000000;

Corstone-1000 can have a DT overlay that deletes the node just before running Linux (in the boot command).

If you are happy with this solution, I can do the following:

1/ create a reserved-memory memory node for Corstone-1000,
     adding mm-comms-buf sub-node which defines the buffer address, size and offset.

With this we can get rid of the configs and the #defines: FFA_SHARED_MM_BUF_ADDR,
FFA_SHARED_MM_BUF_OFFSET and FFA_SHARED_MM_BUF_SIZE.

Also, we will avoid setting 0 as default values for the address, size and offset.

2/ the FF-A specific code in efi_variable_tee.c will try to find the mm-comms-buf
reserved memory node. When found, it reads the buffer address, size and offset.

3/ adding #ifdef CONFIG_ARM_FFA_TRANSPORT in lib/efi_loader/efi_variable_tee.c
for the FF-A specific code.

4/ make EFI_MM_COMM_TEE depends on OPTEE only

What do you think guys ?

Kind regards
Abdellatif