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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 27 Feb 2026, at 11:39, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> 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.

I tried to make the code forward compatible. It is not really a possible case 
right now 
but we could fall into this in the future.

> 
>> +
>> +    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.

Ack will fix in v2

> 
>> +    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.

Memcpy should prevent issues even if accesses are not aligned.
If we had this test, we cannot return an error to the SPMC so we would have to
generate one to the caller. It is simpler i think to handle non-aligned as we 
do not
expect the SPMC to generate such a case.
Tell me if you agree.

> 
>> +
>> +    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.

Very right, i need to remove the warned, printk_once should be enough here.

> 
>> +            {
>> +                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().

True, I will fix that.

> 
>> +    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.

yes, i will rework a bit the cleanup logic here.

Thanks for the review

Cheers
Bertrand

> 
> Cheers,
> Jens
> 
>>     return ret;
>> }
>> 
>> --
>> 2.52.0