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

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Mon, Mar 2, 2026 at 9:51 AM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> 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.

At the moment, it's only ffa_sp_list_cache_init(), which initializes
and puts things in the SP. So as long as ffa_sp_list_cache_init() and
this function are in sync, there should be no problem. It simplifies
things if we can trust the SP cache to be usable.

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

We dereference fpi below, and depending on compiler flags and pointer
types, memcpy() might not be safe with unaligned pointers.
>From 6.3.2.3 Pointers, paragraph 7, in the C standard:
"A pointer to an object type may be converted to a pointer to a
different object type. If the
resulting pointer is not correctly aligned for the referenced type,
the behavior is
undefined."

I've seen past examples where the compiler optimized memcpy() in a way
that breaks with unaligned pointers.

We don't expect the test above to fail, but if it does we will not use
the secure firmware. I think refusing unexpected sizes is even
simpler. It should make finding eventual errors much easier.

So my question above is whether it's worth checking that fpi_size is
well-aligned, or if it's so unlikely that we don't need to consider
it.

Cheers,
Jens

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