Re: [PATCH 2/4] xen/arm: ffa: Cache SP partition info at init

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Wed, Feb 25, 2026 at 11:02 AM Bertrand Marquis
<bertrand.marquis@xxxxxxx> wrote:
>
> FFA_PARTITION_INFO_GET currently queries the SPMC on each call and walks the
> RX buffer every time. The SP list is expected to be static, so repeated
> firmware calls and validation are unnecessary.
>
> Cache the SPMC partition-info list at init time, keeping only secure
> endpoints, and reuse the cached entries for SP count and partition-info
> responses. Initialize the VM create/destroy subscriber lists from the cached
> list and free the cache on init failure.
>
> SP partition info now reflects the init-time snapshot and will not change.
>
> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> ---
>  xen/arch/arm/tee/ffa_partinfo.c | 205 +++++++++++++++++++++-----------
>  1 file changed, 138 insertions(+), 67 deletions(-)
>
> diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
> index 6a6f3ffb822e..8a3eac25f99f 100644
> --- a/xen/arch/arm/tee/ffa_partinfo.c
> +++ b/xen/arch/arm/tee/ffa_partinfo.c
> @@ -6,6 +6,7 @@
>  #include <xen/const.h>
>  #include <xen/sizes.h>
>  #include <xen/types.h>
> +#include <xen/xmalloc.h>
>
>  #include <asm/smccc.h>
>  #include <asm/regs.h>
> @@ -33,6 +34,10 @@ static uint16_t subscr_vm_created_count __read_mostly;
>  static uint16_t *subscr_vm_destroyed __read_mostly;
>  static uint16_t subscr_vm_destroyed_count __read_mostly;
>
> +/* SP list cache (secure endpoints only); populated at init. */
> +static void *sp_list __read_mostly;
> +static uint32_t sp_list_count __read_mostly;
> +static uint32_t sp_list_entry_size __read_mostly;
>  static int32_t ffa_partition_info_get(struct ffa_uuid uuid, uint32_t flags,
>                                        uint32_t *count, uint32_t *fpi_size)
>  {
> @@ -79,92 +84,78 @@ static int32_t ffa_copy_info(void **dst, void *dst_end, 
> const void *src,
>      return FFA_RET_OK;
>  }
>
> -static int32_t ffa_get_sp_count(struct ffa_uuid uuid, uint32_t *sp_count)
> +static bool ffa_sp_entry_matches_uuid(const void *entry, struct ffa_uuid 
> uuid)
>  {
> -    uint32_t src_size;
> +    const struct ffa_partition_info_1_1 *fpi = entry;
> +    struct ffa_uuid sp_uuid;
> +
> +    if ( ffa_uuid_is_nil(uuid) )
> +        return true;
>
> -    return ffa_partition_info_get(uuid, FFA_PARTITION_INFO_GET_COUNT_FLAG,
> -                                  sp_count, &src_size);
> +    if ( sp_list_entry_size < sizeof(*fpi) )
> +        return false;

This can never happen since we don't accept SPMC below version 1.1. We
even have a check to ensure the SPMC doesn't return a too-small
spi_size.

> +
> +    memcpy(&sp_uuid, fpi->uuid, sizeof(sp_uuid));
> +    return ffa_uuid_equal(uuid, sp_uuid);
>  }
>
> -static int32_t ffa_get_sp_partinfo(struct ffa_uuid uuid, uint32_t *sp_count,
> -                                   void **dst_buf, void *end_buf,
> -                                   uint32_t dst_size)
> +static int32_t ffa_get_sp_count(struct ffa_uuid uuid, uint32_t *sp_count)
>  {
> -    int32_t ret;
> -    int32_t release_ret;
> -    uint32_t src_size, real_sp_count;
> -    void *src_buf;
>      uint32_t count = 0;
> -    bool notify_fw = false;
> -
> -    /* We need to use the RX buffer to receive the list */
> -    src_buf = ffa_rxtx_spmc_rx_acquire();
> -    if ( !src_buf )
> -        return FFA_RET_DENIED;
> -
> -    ret = ffa_partition_info_get(uuid, 0, &real_sp_count, &src_size);
> -    if ( ret )
> -        goto out;
> -    notify_fw = true;
> +    uint32_t n;
>
> -    /* Validate the src_size we got */
> -    if ( src_size < sizeof(struct ffa_partition_info_1_0) ||
> -         src_size >= FFA_PAGE_SIZE )
> +    for ( n = 0; n < sp_list_count; n++ )
>      {
> -        ret = FFA_RET_NOT_SUPPORTED;
> -        goto out;
> +        void *entry = sp_list + n * sp_list_entry_size;
> +
> +        if ( ffa_sp_entry_matches_uuid(entry, uuid) )
> +            count++;
>      }
>
> -    /*
> -     * Limit the maximum time we hold the CPU by limiting the number of SPs.
> -     * We just ignore the extra ones as this is tested during init in
> -     * ffa_partinfo_init so the only possible reason is SP have been added
> -     * since boot.
> -     */
> -    if ( real_sp_count > FFA_MAX_NUM_SP )
> -        real_sp_count = FFA_MAX_NUM_SP;
> +    *sp_count = count;
>
> -    /* Make sure the data fits in our buffer */
> -    if ( real_sp_count > (FFA_RXTX_PAGE_COUNT * FFA_PAGE_SIZE) / src_size )
> -    {
> -        ret = FFA_RET_NOT_SUPPORTED;
> -        goto out;
> -    }
> +    if ( !ffa_uuid_is_nil(uuid) && !count )
> +        return FFA_RET_INVALID_PARAMETERS;
>
> -    for ( uint32_t sp_num = 0; sp_num < real_sp_count; sp_num++ )
> -    {
> -        struct ffa_partition_info_1_1 *fpi = src_buf;
> +    return FFA_RET_OK;
> +}
>
> -        /* filter out SP not following bit 15 convention if any */
> -        if ( FFA_ID_IS_SECURE(fpi->id) )
> -        {
> -            /*
> -             * If VM is 1.0 but firmware is 1.1 we could have several entries
> -             * with the same ID but different UUIDs. In this case the VM will
> -             * get a list with several time the same ID.
> -             * This is a non-compliance to the specification but 1.0 VMs 
> should
> -             * handle that on their own to simplify Xen implementation.
> -             */
> +static int32_t ffa_get_sp_partinfo(struct ffa_uuid uuid, uint32_t *sp_count,
> +                                   void **dst_buf, void *end_buf,
> +                                   uint32_t dst_size)
> +{
> +    int32_t ret;
> +    uint32_t count = 0;
> +    uint32_t n;
>
> -            ret = ffa_copy_info(dst_buf, end_buf, src_buf, dst_size, 
> src_size);
> -            if ( ret )
> -                goto out;
> +    for ( n = 0; n < sp_list_count; n++ )
> +    {
> +        void *entry = sp_list + n * sp_list_entry_size;
>
> -            count++;
> -        }
> +        if ( !ffa_sp_entry_matches_uuid(entry, uuid) )
> +            continue;
>
> -        src_buf += src_size;
> +        /*
> +         * If VM is 1.0 but firmware is 1.1 we could have several entries
> +         * with the same ID but different UUIDs. In this case the VM will
> +         * get a list with several time the same ID.
> +         * This is a non-compliance to the specification but 1.0 VMs should
> +         * handle that on their own to simplify Xen implementation.
> +         */
> +        ret = ffa_copy_info(dst_buf, end_buf, entry, dst_size,
> +                            sp_list_entry_size);
> +        if ( ret )
> +            return ret;
> +
> +        count++;
>      }
>
>      *sp_count = count;
>
> -out:
> -    release_ret = ffa_rxtx_spmc_rx_release(notify_fw);
> -    if ( release_ret )
> -        gprintk(XENLOG_WARNING,
> -                "ffa: Error releasing SPMC RX buffer: %d\n", release_ret);
> -    return ret;
> +    if ( !ffa_uuid_is_nil(uuid) && !count )
> +        return FFA_RET_INVALID_PARAMETERS;
> +
> +    return FFA_RET_OK;
>  }
>
>  static int32_t ffa_get_vm_partinfo(struct ffa_uuid uuid, uint32_t 
> start_index,
> @@ -435,6 +426,14 @@ static int32_t ffa_direct_req_send_vm(uint16_t sp_id, 
> uint16_t vm_id,
>      return res;
>  }
>
> +static void ffa_sp_list_cache_free(void)
> +{
> +    XFREE(sp_list);
> +    sp_list = NULL;

XFREE() is already setting sp_list to NULL.

> +    sp_list_count = 0;
> +    sp_list_entry_size = 0;
> +}
> +
>  static void uninit_subscribers(void)
>  {
>          subscr_vm_created_count = 0;
> @@ -443,6 +442,68 @@ static void uninit_subscribers(void)
>          XFREE(subscr_vm_destroyed);
>  }
>
> +static bool ffa_sp_list_cache_init(const void *buf, uint32_t count,
> +                                   uint32_t fpi_size)
> +{
> +    const uint8_t *src = buf;
> +    uint32_t secure_count = 0;
> +    uint32_t n, idx = 0;
> +    bool warned = false;
> +
> +    if ( fpi_size < sizeof(struct ffa_partition_info_1_0) ||
> +         fpi_size >= FFA_PAGE_SIZE )
> +        return false;

Would it make sense to check that fpi_size is well aligned with struct
ffa_partition_info_1_0? If it's an odd size, we'll make unaligned
accesses below with memcpy(). But perhaps that's a bit much. The SPMC
isn't supposed to provide garbage.

> +
> +    if ( count > (FFA_RXTX_PAGE_COUNT * FFA_PAGE_SIZE) / fpi_size )
> +        return false;
> +
> +    for ( n = 0; n < count; n++ )
> +    {
> +        const struct ffa_partition_info_1_0 *fpi =
> +            (const void *)(src + n * fpi_size);
> +
> +        if ( !FFA_ID_IS_SECURE(fpi->id) )
> +        {
> +            if ( !warned )

Is this needed? Doesn't printk_once() already ensure this? Or did you
happen to leave printk_once() by mistake and meant for this to be
printed once each time ffa_sp_list_cache_init() is called, since
"warned" isn't static.

> +            {
> +                printk_once(XENLOG_ERR
> +                            "ffa: Firmware is not using bit 15 convention 
> for IDs !!\n");
> +                warned = true;
> +            }
> +            printk(XENLOG_ERR
> +                   "ffa: Secure partition with id 0x%04x cannot be used\n",
> +                   fpi->id);
> +            continue;
> +        }
> +
> +        secure_count++;
> +    }
> +
> +    if ( secure_count )
> +    {
> +        sp_list = xzalloc_bytes(secure_count * fpi_size);
> +        if ( !sp_list )
> +            return false;
> +    }
> +
> +    sp_list_count = secure_count;
> +    sp_list_entry_size = fpi_size;
> +
> +    for ( n = 0; n < count; n++ )
> +    {
> +        const struct ffa_partition_info_1_0 *fpi =
> +            (const void *)(src + n * fpi_size);
> +
> +        if ( !FFA_ID_IS_SECURE(fpi->id) )
> +            continue;
> +
> +        memcpy(sp_list + idx * fpi_size, fpi, fpi_size);
> +        idx++;
> +    }
> +
> +    return true;
> +}
> +
>  static bool init_subscribers(void *buf, uint16_t count, uint32_t fpi_size)
>  {
>      uint16_t n;
> @@ -549,12 +610,22 @@ bool ffa_partinfo_init(void)
>          goto out;
>      }
>
> -    ret = init_subscribers(spmc_rx, count, fpi_size);
> +    if ( !ffa_sp_list_cache_init(spmc_rx, count, fpi_size) )
> +    {
> +        printk(XENLOG_ERR "ffa: Failed to cache SP list\n");
> +        goto out;
> +    }
> +
> +    ret = init_subscribers(sp_list, sp_list_count, sp_list_entry_size);
>
>  out:
>      e = ffa_rxtx_spmc_rx_release(notify_fw);
>      if ( e )
>          printk(XENLOG_WARNING "ffa: Error releasing SPMC RX buffer: %d\n", 
> e);
> +    if ( !ret )
> +        uninit_subscribers();

ret is initially false and can only be set to true if
init_subscribers() returns true. So there's never any point in calling
uninit_subscribers().

> +    if ( !ret )
> +        ffa_sp_list_cache_free();

ret can be false even if ffa_sp_list_cache_init() hasn't been called
yet. Calling ffa_sp_list_cache_free() anyway is harmless, but not
elegant.

Cheers,
Jens

>      return ret;
>  }
>
> --
> 2.52.0
>