Hi Bertrand, On Mon, Mar 2, 2026 at 4:44 PM Bertrand Marquis <[email protected]> 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 <[email protected]> > --- > Changes since v1: > - removed unneeded NULL assignment after XFREE > - remove warned usage and only rely on printk_once to warn on the 15-bit > convention > - rework ffa_partinfo_init cleanup > - ensure we do not do unaligned accesses when building the SP cache > - enforce SPMC partinfo size to be at least 1.1 structure size when > creating and remove tests when using the cache > --- > xen/arch/arm/tee/ffa_partinfo.c | 216 +++++++++++++++++++++----------- > 1 file changed, 146 insertions(+), 70 deletions(-) > > diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c > index 6a6f3ffb822e..b933becaa55a 100644 > --- a/xen/arch/arm/tee/ffa_partinfo.c > +++ b/xen/arch/arm/tee/ffa_partinfo.c > @@ -6,6 +6,8 @@ > #include <xen/const.h> > #include <xen/sizes.h> > #include <xen/types.h> > +#include <xen/unaligned.h> > +#include <xen/xmalloc.h> > > #include <asm/smccc.h> > #include <asm/regs.h> > @@ -33,6 +35,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;
Nit: prefer an empty line here, but it's fixed in a later patch. Looks good: Reviewed-by: Jens Wiklander <[email protected]> Cheers, Jens > static int32_t ffa_partition_info_get(struct ffa_uuid uuid, uint32_t flags, > uint32_t *count, uint32_t *fpi_size) > { > @@ -79,92 +85,84 @@ 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 uint16_t ffa_sp_entry_read_id(const void *entry) > { > - uint32_t src_size; > - > - return ffa_partition_info_get(uuid, FFA_PARTITION_INFO_GET_COUNT_FLAG, > - sp_count, &src_size); > + return get_unaligned_t(uint16_t, > + (const uint8_t *)entry + > + offsetof(struct ffa_partition_info_1_0, id)); > } > > -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 bool ffa_sp_entry_matches_uuid(const void *entry, struct ffa_uuid > uuid) > { > - 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; > + struct ffa_uuid sp_uuid; > > - /* 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; > + if ( ffa_uuid_is_nil(uuid) ) > + return true; > > - ret = ffa_partition_info_get(uuid, 0, &real_sp_count, &src_size); > - if ( ret ) > - goto out; > - notify_fw = true; > + memcpy(&sp_uuid, > + (const uint8_t *)entry + > + offsetof(struct ffa_partition_info_1_1, uuid), > + sizeof(sp_uuid)); > + return ffa_uuid_equal(uuid, sp_uuid); > +} > > - /* Validate the src_size we got */ > - if ( src_size < sizeof(struct ffa_partition_info_1_0) || > - src_size >= FFA_PAGE_SIZE ) > +static int32_t ffa_get_sp_count(struct ffa_uuid uuid, uint32_t *sp_count) > +{ > + uint32_t count = 0; > + uint32_t n; > + > + 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; > + > + /* > + * 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; > > - src_buf += src_size; > + 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 +433,13 @@ 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_count = 0; > + sp_list_entry_size = 0; > +} > + > static void uninit_subscribers(void) > { > subscr_vm_created_count = 0; > @@ -443,6 +448,63 @@ 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; > + > + if ( fpi_size < sizeof(struct ffa_partition_info_1_1) || > + fpi_size >= FFA_PAGE_SIZE ) > + return false; > + > + if ( count > (FFA_RXTX_PAGE_COUNT * FFA_PAGE_SIZE) / fpi_size ) > + return false; > + > + for ( n = 0; n < count; n++ ) > + { > + const uint8_t *entry = src + n * fpi_size; > + uint16_t id = ffa_sp_entry_read_id(entry); > + > + if ( !FFA_ID_IS_SECURE(id) ) > + { > + printk_once(XENLOG_ERR > + "ffa: Firmware is not using bit 15 convention for > IDs !!\n"); > + printk(XENLOG_ERR > + "ffa: Secure partition with id 0x%04x cannot be used\n", > + 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 uint8_t *entry = src + n * fpi_size; > + uint16_t id = ffa_sp_entry_read_id(entry); > + > + if ( !FFA_ID_IS_SECURE(id) ) > + continue; > + > + memcpy(sp_list + idx * fpi_size, entry, fpi_size); > + idx++; > + } > + > + return true; > +} > + > static bool init_subscribers(void *buf, uint16_t count, uint32_t fpi_size) > { > uint16_t n; > @@ -538,7 +600,7 @@ bool ffa_partinfo_init(void) > if ( e ) > { > printk(XENLOG_ERR "ffa: Failed to get list of SPs: %d\n", e); > - goto out; > + goto out_release_rx; > } > notify_fw = true; > > @@ -546,15 +608,29 @@ bool ffa_partinfo_init(void) > { > printk(XENLOG_ERR "ffa: More SPs than the maximum supported: %u - > %u\n", > count, FFA_MAX_NUM_SP); > - goto out; > + goto out_release_rx; > + } > + > + if ( !ffa_sp_list_cache_init(spmc_rx, count, fpi_size) ) > + { > + printk(XENLOG_ERR "ffa: Failed to cache SP list\n"); > + goto out_release_rx; > } > > - ret = init_subscribers(spmc_rx, count, fpi_size); > + if ( !init_subscribers(sp_list, sp_list_count, sp_list_entry_size) ) > + goto out_free_sp_cache; > > -out: > + ret = true; > + goto out_release_rx; > + > +out_free_sp_cache: > + ffa_sp_list_cache_free(); > + > +out_release_rx: > e = ffa_rxtx_spmc_rx_release(notify_fw); > if ( e ) > printk(XENLOG_WARNING "ffa: Error releasing SPMC RX buffer: %d\n", > e); > + > return ret; > } > > -- > 2.52.0 >
