Re: [PATCH 02/12] arm-ffa: add FF-A bus runtime support

[Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>]

Hi Harsimran,

> On 2026-04-24T17:31:50, Harsimran Singh Tungal
> <harsimransingh.tungal@arm.com> wrote:
> > arm-ffa: add FF-A bus runtime support
> >
> > Enable FF-A runtime transport for EFI services
> >
> > This patch introduces the FF-A runtime infrastructure that enables
> > U-Boot to use the Arm Firmware Framework for Arm A-profile (FF-A)
> > transport layer after ExitBootServices().
> > The runtime transport provides persistent, runtime-safe
> > implementations of key FF-A functions used by EFI runtime services.
> >
> > Summary of key changes:
> > -----------------------
> >   - Add drivers/firmware/arm-ffa/arm-ffa-runtime.c implementing FF-A
> >     runtime operations.
> >   - Introduce include/arm_ffa_runtime.h exporting FF-A runtime
> >     functions and data structures.
> >   - Move FF-A error mapping into runtime context.
> >   - Introduce invoke_ffa_fn_runtime() as runtime safe SMC wrapper.
> >   - Add runtime SMC wrapper for sandbox emulation.
> >   - Add ARM_FFA_RT_MODE Kconfig to enable runtime support.
> > [...]
> >
> > drivers/firmware/arm-ffa/Kconfig           |  11 ++
> >  drivers/firmware/arm-ffa/Makefile          |   4 +-
> >  drivers/firmware/arm-ffa/arm-ffa-runtime.c | 287 +++++++++++++++++++++++++++++
> >  drivers/firmware/arm-ffa/arm-ffa-uclass.c  | 111 ++---------
> >  drivers/firmware/arm-ffa/arm-ffa.c         |  16 +-
> >  drivers/firmware/arm-ffa/ffa-emul-uclass.c |  12 ++
> >  include/arm_ffa.h                          |  16 +-
> >  include/arm_ffa_priv.h                     |  24 ++-
> >  include/arm_ffa_runtime.h                  | 183 ++++++++++++++++++
> >  test/dm/ffa.c                              |   6 +-
> >  10 files changed, 551 insertions(+), 119 deletions(-)
> 
> > diff --git a/drivers/firmware/arm-ffa/arm-ffa-runtime.c b/drivers/firmware/arm-ffa/arm-ffa-runtime.c
> > @@ -0,0 +1,287 @@
> > +     if (priv)
> > +             ffa_copy_runtime_priv(&priv->rt);
> > +     else
> > +             log_err("FF-A: Entering RT mode without FF-A runtime data information\n");
> > +
> > +     ffa_enable_runtime_context();
> 
> This enables the runtime context unconditionally, even after logging
> the priv data is missing. The runtime then comes up with id == 0 and
> fwk_version == 0 and any later FF-A call silently uses bogus values.
> Please return after the log_err() so ffa_get_status_runtime_context()
> keeps returning false on the failure path.
> 
> > diff --git a/drivers/firmware/arm-ffa/arm-ffa-runtime.c b/drivers/firmware/arm-ffa/arm-ffa-runtime.c
> > @@ -0,0 +1,287 @@
> > +#include <arm_ffa_runtime.h>
> > +#include <arm_ffa_priv.h>
> > +#include <log.h>
> > +#include <asm/global_data.h>
> > +#include <linux/errno.h>
> > +#include <linux/types.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> 
> gd doesn't seem to be referenced in this file?
> 
> > diff --git a/drivers/firmware/arm-ffa/arm-ffa-runtime.c b/drivers/firmware/arm-ffa/arm-ffa-runtime.c
> > @@ -0,0 +1,287 @@
> > + * after the FF-A bus device has successfully probed and U-Boot’s FF-A
> 
> This (and the same line in arm_ffa_runtime.h) should use a normal '
> instead of the UTF-8 right single quotation mark.
> 
> > diff --git a/drivers/firmware/arm-ffa/arm-ffa-runtime.c b/drivers/firmware/arm-ffa/arm-ffa-runtime.c
> > @@ -0,0 +1,287 @@
> > +#if CONFIG_IS_ENABLED(ARM_FFA_RT_MODE)
> > +static void EFIAPI ffa_rt_exit_boot_services_notify(struct efi_event *event,
> > +                                                 void *context)
> 
> CONFIG_IS_ENABLED() is only meaningful for symbols with an SPL twin.
> ARM_FFA_RT_MODE has none, and the uclass side already uses
> IS_ENABLED(CONFIG_ARM_FFA_RT_MODE) - please use IS_ENABLED() here and
> in arm_ffa_runtime.h.
> 
> > diff --git a/drivers/firmware/arm-ffa/arm-ffa-runtime.c b/drivers/firmware/arm-ffa/arm-ffa-runtime.c
> > @@ -0,0 +1,287 @@
> > +     if (is_smc64) {
> > +             req_mode = FFA_SMC_64(FFA_MSG_SEND_DIRECT_REQ);
> > +     } else {
> > +             req_mode = FFA_SMC_32(FFA_MSG_SEND_DIRECT_REQ);
> > +     }
> 
> Single-statement bodies should not be braced. Please run patman or
> checkpatch.pl on your patches. Also, the two efi_memset_runtime()
> calls below clear fresh stack variables. Please initialise them at
> declaration ('ffa_value_t ffa_args_rt = {}') and drop the explicit
> memset()s; the compiler's zero-init is just as runtime-safe and is
> what the boot path already does.
> 
> Also could you run checkpatch / patman over your patches for v2?
> 
> > diff --git a/drivers/firmware/arm-ffa/arm-ffa-uclass.c b/drivers/firmware/arm-ffa/arm-ffa-uclass.c
> > @@ -1047,6 +961,11 @@ static int ffa_do_probe(struct udevice *dev)
> > +     if (IS_ENABLED(CONFIG_ARM_FFA_RT_MODE)) {
> > +             ret = ffa_setup_efi_exit_boot_services_event(uc_priv);
> > +             if (ret)
> > +                     return ret;
> > +     }
> 
> If event creation fails here, the RX/TX buffers mapped above and the
> partition cache are leaked, i,e. the existing failure path for
> ffa_cache_partitions_info() at least calls
> ffa_unmap_rxtx_buffers_hdlr(). Please follow the same pattern, or move
> event registration before resource allocation.
> 
> > diff --git a/drivers/firmware/arm-ffa/Kconfig b/drivers/firmware/arm-ffa/Kconfig
> > @@ -41,3 +44,11 @@ config ARM_FFA_TRANSPORT
> > +config ARM_FFA_RT_MODE
> > +     bool "Enable FF-A runtime support"
> > +     depends on ARM_FFA_TRANSPORT && EFI_LOADER
> > +     default y
> 
> Just to check, does every existing FF-A user want this on? 'default y'
> silently enables a new transport path (and the ExitBootServices
> callback) for everyone with ARM_FFA_TRANSPORT && EFI_LOADER. I'd
> suggest defaulting, or at minimum 'default y if CORSTONE1000' or
> similar, until other boards have opted in.
> 
> > diff --git a/include/arm_ffa_priv.h b/include/arm_ffa_priv.h
> > @@ -212,9 +227,8 @@ struct ffa_partitions {
> > -     u32 fwk_version;
> > +     struct ffa_priv_runtime rt;
> >       struct udevice *emul;
> > -     u16 id;
> 
> Just to clarify the model: 'struct ffa_priv' now embeds 'struct
> ffa_priv_runtime rt' at boot, and at ExitBootServices we copy the
> whole sub-struct (including 'use_ffa_runtime', always false here) into
> the resident 'ffa_priv_rt', then immediately overwrite use_ffa_runtime
> via ffa_enable_runtime_context(). Layering would be clearer if
> 'use_ffa_runtime' lived only on the resident copy and ffa_priv_runtime
> carried just the data shipped across the boundary (fwk_version, id).
> 
> Regards,
> Simon

Simon’s comments make sense to me. Please address them.

Aside from that, everything looks good in this commit.

Regards,
Abdellatif