Re: [PATCH v3 3/4] arm_ffa: introduce Arm FF-A low-level driver

Abdellatif El Khlifi
2022-11-22T13:50:28Z

On Fri, Nov 18, 2022 at 01:50:26PM -0700, Simon Glass wrote:
> Hi,
>
> On Wed, 16 Nov 2022 at 06:03, Abdellatif El Khlifi
> <abdellatif.elkhlifi@arm.com> wrote:
> >
> > On Tue, Nov 15, 2022 at 08:24:24AM -0700, Simon Glass wrote:
> > > Hi,
> > >
> > > On Mon, 1 Aug 2022 at 11:21, Abdellatif El Khlifi
> > > <abdellatif.elkhlifi@arm.com> wrote:
> > > >
> > > > Add the driver implementing Arm Firmware Framework for Armv8-A v1.0
> > > >
> > > > The Firmware Framework for Arm A-profile processors (FF-A)
> > > > describes interfaces (ABIs) that standardize communication
> > > > between the Secure World and Normal World leveraging TrustZone
> > > > technology.
> > > >
> > > > This driver uses 64-bit registers as per SMCCCv1.2 spec and comes
> > > > on top of the SMCCC layer. The driver provides the FF-A ABIs needed for
> > > > querying the FF-A framework from the secure world.
> > > >
> > > > 32-bit version of the ABIs is supported and 64-bit version of FFA_RXTX_MAP
> > > > and FFA_MSG_SEND_DIRECT_{REQ, RESP}.
> > > >
> > > > In u-boot FF-A design, FF-A is considered as a discoverable bus.
> > > > The Secure World is considered as one entity to communicate with
> > > > using the FF-A bus. FF-A communication is handled by one device and
> > > > one instance (the bus). This FF-A driver takes care of all the
> > > > interactions between Normal world and Secure World.
> > > >
> > > > The driver exports its operations to be used by upper layers.
> > > >
> > > > Exported operations:
> > > >
> > > > - partition_info_get
> > > > - sync_send_receive
> > > > - rxtx_unmap
> > > >
> > > > This implementation provides an optional feature to copy the driver data
> > > > to EFI runtime area.
> > > >
> > > > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > > > Cc: Tom Rini <trini@konsulko.com>
> > > > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > Cc: Jens Wiklander <jens.wiklander@linaro.org>
> > > > ---
> > > > MAINTAINERS | 6 +
> > > > common/board_r.c | 7 +
> > > > drivers/Kconfig | 2 +
> > > > drivers/Makefile | 1 +
> > > > drivers/arm-ffa/Kconfig | 33 +
> > > > drivers/arm-ffa/Makefile | 7 +
> > > > drivers/arm-ffa/arm-ffa-uclass.c | 16 +
> > > > drivers/arm-ffa/arm_ffa_prv.h | 219 ++++
> > > > drivers/arm-ffa/core.c | 1338 ++++++++++++++++++++
> > > > drivers/arm-ffa/efi_ffa_runtime_data_mgr.c | 94 ++
> > > > include/arm_ffa.h | 132 ++
> > > > include/dm/uclass-id.h | 1 +
> > > > include/uuid.h | 8 +
> > > > lib/efi_loader/efi_boottime.c | 17 +
> > > > lib/uuid.c | 65 +
> > > > 15 files changed, 1946 insertions(+)
> > > > create mode 100644 drivers/arm-ffa/Kconfig
> > > > create mode 100644 drivers/arm-ffa/Makefile
> > > > create mode 100644 drivers/arm-ffa/arm-ffa-uclass.c
> > > > create mode 100644 drivers/arm-ffa/arm_ffa_prv.h
> > > > create mode 100644 drivers/arm-ffa/core.c
> > > > create mode 100644 drivers/arm-ffa/.c
> > > > create mode 100644 include/arm_ffa.h
> > >
> > > Please add something to doc/ so people know what this is.
> > >
> > > Since you are adding a new uclass you need a sandbox driver and tests.
> > >
> > > The driver appears to have no operations, but there is a bus_ops. The
> > > ops should go in the driver, I suspect, and should pass the device as
> > > the first arg.
> > >
> > > Can FFA_ERR_STAT_SUCCESS be 0 so you don't have to sprinkle the code with it?
> > >
> > > Why is it using EFI things? Can this driver only be used with UEFI? I
> > > hope not, if it is an official way of updating firmware.
> > >
> > > Please don't add more things to board_r.c - we are trying to remove
> > > this init over time. If it is a device it should be probed as needed.
> > >
> > > Is there a device tree binding?
> > >
> > > Also should this go in drivers/misc instead of creating a whole new subdir?
> >
> > Hi Simon, thanks for reviewing.
> >
> > All the above comments have already been addressed in the new versions
> > of the patchset. Please refer to the latest version v7 [1].
> >
> > By the way I'd like to highlight the following:
> >
> > - The FF-A driver documentation is at doc/arch/arm64.ffa.rst, please
> > refer to it since it provides helpful details about the FF-A support
> > in U-Boot
>
> OK
>
> > - The patchset comes with Sandbox driver and tests [2]
>
> OK I suppose I saw that previously and forgot.
>
> > - The driver has operations defined in struct ffa_bus_ops (include/arm_ffa.h).
> > ffa_bus_ops_get() gets the ops. All these are in the driver (drivers/firmware/arm-ffa/core.c)
>
> Can you please push a tree somewhere so I can look?

OK, I'll share the link to a public branch so you can refer to once it's created.

>
> > - The FF-A bus has only 1 device. No multiple instances. So passing the
> > device doesn't make sense in our case
>
> It must still pass the device.

I added this feature in patchset v8. Please refer to the latest patchset when reviewing [1].

[1]: https://lore.kernel.org/all/20221122131751.22747-1-abdellatif.elkhlifi@arm.com/

>
> > - FFA_ERR_STAT_SUCCESS has been removed and replaced with 0
>
> OK
>
> > - The driver is independent from EFI and can be compiled without EFI
>
> Oh, so what is ffa_copy_runtime_data() for?

This has already been dropped in newer versions of the pachset. Could you please refer to the latest ? (v8)
>
> > - FF-A bus discovery has been removed from the initcall level (board_r.c)
>
> Good.
>
> > Discovery is done on demand. Clients can call ffa_bus_discover() when
> > they want to use the FF-A bus. As an example of how clients initiate
> > discovery please refer to the FF-A MM comms client [3].
>
> Is there a command to do this?

If you mean whether there is a command to showcase this, yes.

I already created the armffa command as an example of how to use the FF-A bus
and how clients can discover it.

Please refer to this commit [1].

[1]: https://lore.kernel.org/all/20221122131751.22747-6-abdellatif.elkhlifi@arm.com/

>
> > - As done in the Linux kernel, the FF-A bus doesn't have a device tree
> > binding since there is no peripheral associated with FF-A. At the
> > early stages of this patchset, we double checked with the device tree
> > maintainer and the decision was no device tree for FF-A
>
> Sorry, but you must add one.

We had a discussion on this on April 2022 with Rob Herring. We decided to do
the same as the FF-A kernel driver: no device tree, we perform a manual bus discovery.

Rob Herring stated the following:

- There is not a 'kernel device tree' and a 'u-boot device tree'. There is only 1
- The FFA DT binding was rejected in favor of making FFA discoverable.
The FFA spec was amended to address that. DT is only for what we
failed to make discoverable. For hardware, we're stuck with it. We
shouldn't repeat that for software interfaces.

For more details please refer to [1].

[1]: https://lore.kernel.org/all/CAL_Jsq+dwHqwDdALTAwU-VYW2=2kYCyHpv7mUP0zR04CwH101Q@mail.gmail.com/

>
> > - The links below are from the U-Boot mailing list mirror in lore.kernel.org
>
> Regards,
> Simon
>
>
> >
> > Cheers.
> >
> > [1]: https://lore.kernel.org/all/20221107192055.21669-1-abdellatif.elkhlifi@arm.com/
> > [2]: https://lore.kernel.org/all/20221107192055.21669-7-abdellatif.elkhlifi@arm.com/
> > https://lore.kernel.org/all/20221107192055.21669-8-abdellatif.elkhlifi@arm.com/
> > https://lore.kernel.org/all/20221107192055.21669-9-abdellatif.elkhlifi@arm.com/
> > [3]: https://lore.kernel.org/all/20221107192055.21669-10-abdellatif.elkhlifi@arm.com/
> > >
> > > Regards,
> > > Simon