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

[Ilias Apalodimas]

Hi Abdellatif

On Mon, 31 Jul 2023 at 14:46, Abdellatif El Khlifi
<abdellatif.elkhlifi at arm.com> wrote:
>
> Hi Ilias,
>
> On Mon, Jul 31, 2023 at 12:38:16PM +0300, Ilias Apalodimas 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?

Thanks
/Ilias



>
> Simon suggested we use build configs to set the buffer address, size and offset since we don't want
> a DT node for the MM firmware.
>
> Kind regards
> Abdellatif