Hi Bertrand,
On Thu, Nov 27, 2025 at 4:52 PM Bertrand Marquis
<[email protected]> wrote:
>
> Rework how Xen accesses the RX/TX buffers shared with the SPMC so that
> ownership and locking are handled centrally.
>
> Move the SPMC RX/TX buffer bases into ffa_rxtx.c as ffa_spmc_rx/ffa_spmc_tx,
> protect them with dedicated ffa_spmc_{rx,tx}_lock spinlocks and expose
> ffa_rxtx_spmc_{rx,tx}_{acquire,release}() helpers instead of the global
> ffa_rx/ffa_tx pointers and ffa_{rx,tx}_buffer_lock.
>
> The RX helpers now always issue FFA_RX_RELEASE when we are done
> consuming data from the SPMC, so partition-info enumeration and shared
> memory paths release the RX buffer on all exit paths. The RX/TX mapping
> code is updated to use the descriptor offsets (rx_region_offs and
> tx_region_offs) rather than hard-coded structure layout, and to use the
> TX acquire/release helpers instead of touching the TX buffer directly.
>
> Signed-off-by: Bertrand Marquis <[email protected]>
> ---
> xen/arch/arm/tee/ffa.c | 22 +-----
> xen/arch/arm/tee/ffa_partinfo.c | 40 +++++-----
> xen/arch/arm/tee/ffa_private.h | 18 ++---
> xen/arch/arm/tee/ffa_rxtx.c | 132 +++++++++++++++++++++++++-------
> xen/arch/arm/tee/ffa_shm.c | 26 ++++---
> 5 files changed, 153 insertions(+), 85 deletions(-)
>
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 47f426e85864..4c1b9a4c3b48 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -48,8 +48,8 @@
> * notification for secure partitions
> * - doesn't support notifications for Xen itself
> *
> - * There are some large locked sections with ffa_tx_buffer_lock and
> - * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
> + * There are some large locked sections with ffa_spmc_tx_lock and
> + * ffa_spmc_rx_lock. Especially the ffa_spmc_tx_lock spinlock used
> * around share_shm() is a very large locked section which can let one VM
> * affect another VM.
> */
> @@ -108,20 +108,6 @@ static const struct ffa_fw_abi ffa_fw_abi_needed[] = {
> FW_ABI(FFA_RUN),
> };
>
> -/*
> - * Our rx/tx buffers shared with the SPMC. FFA_RXTX_PAGE_COUNT is the
> - * number of pages used in each of these buffers.
> - *
> - * The RX buffer is protected from concurrent usage with ffa_rx_buffer_lock.
> - * Note that the SPMC is also tracking the ownership of our RX buffer so
> - * for calls which uses our RX buffer to deliver a result we must call
> - * ffa_rx_release() to let the SPMC know that we're done with the buffer.
> - */
> -void *ffa_rx __read_mostly;
> -void *ffa_tx __read_mostly;
> -DEFINE_SPINLOCK(ffa_rx_buffer_lock);
> -DEFINE_SPINLOCK(ffa_tx_buffer_lock);
> -
> LIST_HEAD(ffa_ctx_head);
> /* RW Lock to protect addition/removal and reading in ffa_ctx_head */
> DEFINE_RWLOCK(ffa_ctx_list_rwlock);
> @@ -612,7 +598,7 @@ static bool ffa_probe_fw(void)
> ffa_fw_abi_needed[i].name);
> }
>
> - if ( !ffa_rxtx_init() )
> + if ( !ffa_rxtx_spmc_init() )
> {
> printk(XENLOG_ERR "ffa: Error during RXTX buffer init\n");
> goto err_no_fw;
> @@ -626,7 +612,7 @@ static bool ffa_probe_fw(void)
> return true;
>
> err_rxtx_destroy:
> - ffa_rxtx_destroy();
> + ffa_rxtx_spmc_destroy();
> err_no_fw:
> ffa_fw_version = 0;
> bitmap_zero(ffa_fw_abi_supported, FFA_ABI_BITMAP_SIZE);
> diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
> index fa56b1587e3b..766b75dffb8c 100644
> --- a/xen/arch/arm/tee/ffa_partinfo.c
> +++ b/xen/arch/arm/tee/ffa_partinfo.c
> @@ -77,28 +77,24 @@ static int32_t ffa_get_sp_partinfo(uint32_t *uuid,
> uint32_t *sp_count,
> {
> int32_t ret;
> uint32_t src_size, real_sp_count;
> - void *src_buf = ffa_rx;
> + void *src_buf;
> uint32_t count = 0;
>
> - /* Do we have a RX buffer with the SPMC */
> - if ( !ffa_rx )
> - return FFA_RET_DENIED;
> -
> /* We need to use the RX buffer to receive the list */
> - spin_lock(&ffa_rx_buffer_lock);
> + 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;
>
> - /* We now own the RX buffer */
> -
> /* Validate the src_size we got */
> if ( src_size < sizeof(struct ffa_partition_info_1_0) ||
> src_size >= FFA_PAGE_SIZE )
> {
> ret = FFA_RET_NOT_SUPPORTED;
> - goto out_release;
> + goto out;
> }
>
> /*
> @@ -114,7 +110,7 @@ static int32_t ffa_get_sp_partinfo(uint32_t *uuid,
> uint32_t *sp_count,
> if ( real_sp_count > (FFA_RXTX_PAGE_COUNT * FFA_PAGE_SIZE) / src_size )
> {
> ret = FFA_RET_NOT_SUPPORTED;
> - goto out_release;
> + goto out;
> }
>
> for ( uint32_t sp_num = 0; sp_num < real_sp_count; sp_num++ )
> @@ -127,7 +123,7 @@ static int32_t ffa_get_sp_partinfo(uint32_t *uuid,
> uint32_t *sp_count,
> if ( dst_buf > (end_buf - dst_size) )
> {
> ret = FFA_RET_NO_MEMORY;
> - goto out_release;
> + goto out;
> }
>
> memcpy(dst_buf, src_buf, MIN(src_size, dst_size));
> @@ -143,10 +139,8 @@ static int32_t ffa_get_sp_partinfo(uint32_t *uuid,
> uint32_t *sp_count,
>
> *sp_count = count;
>
> -out_release:
> - ffa_hyp_rx_release();
> out:
> - spin_unlock(&ffa_rx_buffer_lock);
> + ffa_rxtx_spmc_rx_release();
> return ret;
> }
>
> @@ -378,7 +372,7 @@ static void uninit_subscribers(void)
> XFREE(subscr_vm_destroyed);
> }
>
> -static bool init_subscribers(uint16_t count, uint32_t fpi_size)
> +static bool init_subscribers(void *buf, uint16_t count, uint32_t fpi_size)
> {
> uint16_t n;
> uint16_t c_pos;
> @@ -395,7 +389,7 @@ static bool init_subscribers(uint16_t count, uint32_t
> fpi_size)
> subscr_vm_destroyed_count = 0;
> for ( n = 0; n < count; n++ )
> {
> - fpi = ffa_rx + n * fpi_size;
> + fpi = buf + n * fpi_size;
>
> /*
> * We need to have secure partitions using bit 15 set convention for
> @@ -433,7 +427,7 @@ static bool init_subscribers(uint16_t count, uint32_t
> fpi_size)
>
> for ( c_pos = 0, d_pos = 0, n = 0; n < count; n++ )
> {
> - fpi = ffa_rx + n * fpi_size;
> + fpi = buf + n * fpi_size;
>
> if ( FFA_ID_IS_SECURE(fpi->id) )
> {
> @@ -455,10 +449,14 @@ bool ffa_partinfo_init(void)
> uint32_t fpi_size;
> uint32_t count;
> int e;
> + void *spmc_rx;
>
> if ( !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) ||
> - !ffa_fw_supports_fid(FFA_MSG_SEND_DIRECT_REQ_32) ||
> - !ffa_rx || !ffa_tx )
> + !ffa_fw_supports_fid(FFA_MSG_SEND_DIRECT_REQ_32))
> + return false;
> +
> + spmc_rx = ffa_rxtx_spmc_rx_acquire();
> + if (!spmc_rx)
> return false;
>
> e = ffa_partition_info_get(NULL, 0, &count, &fpi_size);
> @@ -475,10 +473,10 @@ bool ffa_partinfo_init(void)
> goto out;
> }
>
> - ret = init_subscribers(count, fpi_size);
> + ret = init_subscribers(spmc_rx, count, fpi_size);
>
> out:
> - ffa_hyp_rx_release();
> + ffa_rxtx_spmc_rx_release();
> return ret;
> }
>
> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
> index 4272afd37343..cd35c44b8986 100644
> --- a/xen/arch/arm/tee/ffa_private.h
> +++ b/xen/arch/arm/tee/ffa_private.h
> @@ -404,10 +404,6 @@ struct ffa_ctx {
> unsigned long *vm_destroy_bitmap;
> };
>
> -extern void *ffa_rx;
> -extern void *ffa_tx;
> -extern spinlock_t ffa_rx_buffer_lock;
> -extern spinlock_t ffa_tx_buffer_lock;
> extern DECLARE_BITMAP(ffa_fw_abi_supported, FFA_ABI_BITMAP_SIZE);
>
> extern struct list_head ffa_ctx_head;
> @@ -425,8 +421,13 @@ int ffa_partinfo_domain_init(struct domain *d);
> bool ffa_partinfo_domain_destroy(struct domain *d);
> void ffa_handle_partition_info_get(struct cpu_user_regs *regs);
>
> -bool ffa_rxtx_init(void);
> -void ffa_rxtx_destroy(void);
> +bool ffa_rxtx_spmc_init(void);
> +void ffa_rxtx_spmc_destroy(void);
> +void *ffa_rxtx_spmc_rx_acquire(void);
> +void ffa_rxtx_spmc_rx_release(void);
> +void *ffa_rxtx_spmc_tx_acquire(void);
> +void ffa_rxtx_spmc_tx_release(void);
> +
> int32_t ffa_rxtx_domain_init(struct domain *d);
> void ffa_rxtx_domain_destroy(struct domain *d);
> int32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
> @@ -556,11 +557,6 @@ static inline int32_t ffa_simple_call(uint32_t fid,
> register_t a1,
> return ffa_get_ret_code(&resp);
> }
>
> -static inline int32_t ffa_hyp_rx_release(void)
> -{
> - return ffa_simple_call(FFA_RX_RELEASE, 0, 0, 0, 0);
> -}
> -
> static inline bool ffa_fw_supports_fid(uint32_t fid)
> {
> BUILD_BUG_ON(FFA_FNUM_MIN_VALUE > FFA_FNUM_MAX_VALUE);
> diff --git a/xen/arch/arm/tee/ffa_rxtx.c b/xen/arch/arm/tee/ffa_rxtx.c
> index cd467d1dba68..07b01430d139 100644
> --- a/xen/arch/arm/tee/ffa_rxtx.c
> +++ b/xen/arch/arm/tee/ffa_rxtx.c
> @@ -30,6 +30,20 @@ struct ffa_endpoint_rxtx_descriptor_1_1 {
> uint32_t tx_region_offs;
> };
>
> +/*
> + * Our rx/tx buffers shared with the SPMC. FFA_RXTX_PAGE_COUNT is the
> + * number of pages used in each of these buffers.
> + * Each buffer has its own lock to protect from concurrent usage.
> + *
> + * Note that the SPMC is also tracking the ownership of our RX buffer so
> + * for calls which uses our RX buffer to deliver a result we must do an
> + * FFA_RX_RELEASE to let the SPMC know that we're done with the buffer.
> + */
> +static void *ffa_spmc_rx __read_mostly;
> +static void *ffa_spmc_tx __read_mostly;
> +static DEFINE_SPINLOCK(ffa_spmc_rx_lock);
> +static DEFINE_SPINLOCK(ffa_spmc_tx_lock);
> +
> static int32_t ffa_rxtx_map(paddr_t tx_addr, paddr_t rx_addr,
> uint32_t page_count)
> {
> @@ -120,8 +134,9 @@ int32_t ffa_handle_rxtx_map(uint32_t fid, register_t
> tx_addr,
> sizeof(struct ffa_address_range) * 2 >
> FFA_MAX_RXTX_PAGE_COUNT * FFA_PAGE_SIZE);
>
> - spin_lock(&ffa_tx_buffer_lock);
> - rxtx_desc = ffa_tx;
> + rxtx_desc = ffa_rxtx_spmc_tx_acquire();
> + if ( !rxtx_desc )
> + goto err_unmap_rx;
>
> /*
> * We have only one page for each so we pack everything:
> @@ -138,7 +153,7 @@ int32_t ffa_handle_rxtx_map(uint32_t fid, register_t
> tx_addr,
> address_range_array[1]);
>
> /* rx buffer */
> - mem_reg = ffa_tx + sizeof(*rxtx_desc);
> + mem_reg = (void *)rxtx_desc + rxtx_desc->rx_region_offs;
> mem_reg->total_page_count = 1;
> mem_reg->address_range_count = 1;
> mem_reg->reserved = 0;
> @@ -148,7 +163,7 @@ int32_t ffa_handle_rxtx_map(uint32_t fid, register_t
> tx_addr,
> mem_reg->address_range_array[0].reserved = 0;
>
> /* tx buffer */
> - mem_reg = ffa_tx + rxtx_desc->tx_region_offs;
> + mem_reg = (void *)rxtx_desc + rxtx_desc->tx_region_offs;
> mem_reg->total_page_count = 1;
> mem_reg->address_range_count = 1;
> mem_reg->reserved = 0;
> @@ -159,7 +174,7 @@ int32_t ffa_handle_rxtx_map(uint32_t fid, register_t
> tx_addr,
>
> ret = ffa_rxtx_map(0, 0, 0);
>
> - spin_unlock(&ffa_tx_buffer_lock);
> + ffa_rxtx_spmc_tx_release();
>
> if ( ret != FFA_RET_OK )
> goto err_unmap_rx;
> @@ -291,49 +306,114 @@ void ffa_rxtx_domain_destroy(struct domain *d)
> rxtx_unmap(d);
> }
>
> -void ffa_rxtx_destroy(void)
> +void *ffa_rxtx_spmc_rx_acquire(void)
> +{
> + ASSERT(!spin_is_locked(&ffa_spmc_rx_lock));
Is it invalid for two CPUs to concurrently try to acquire the RX buffer?
> +
> + spin_lock(&ffa_spmc_rx_lock);
> +
> + if ( ffa_spmc_rx )
> + return ffa_spmc_rx;
> +
> + spin_unlock(&ffa_spmc_rx_lock);
> + return NULL;
> +}
> +
> +void ffa_rxtx_spmc_rx_release(void)
> +{
> + int32_t ret;
> +
> + ASSERT(spin_is_locked(&ffa_spmc_rx_lock));
> +
> + /* Inform the SPMC that we are done with our RX buffer */
> + ret = ffa_simple_call(FFA_RX_RELEASE, 0, 0, 0, 0);
> + if ( ret != FFA_RET_OK )
> + printk(XENLOG_DEBUG "Error releasing SPMC RX buffer: %d\n", ret);
> +
> + spin_unlock(&ffa_spmc_rx_lock);
> +}
> +
> +void *ffa_rxtx_spmc_tx_acquire(void)
> {
> - bool need_unmap = ffa_tx && ffa_rx;
> + ASSERT(!spin_is_locked(&ffa_spmc_tx_lock));
Is it invalid for two CPUs to concurrently try to acquire the TX buffer?
>
> - if ( ffa_tx )
> + spin_lock(&ffa_spmc_tx_lock);
> +
> + if ( ffa_spmc_tx )
> + return ffa_spmc_tx;
> +
> + spin_unlock(&ffa_spmc_tx_lock);
> + return NULL;
> +}
> +
> +void ffa_rxtx_spmc_tx_release(void)
> +{
> + ASSERT(spin_is_locked(&ffa_spmc_tx_lock));
> +
> + spin_unlock(&ffa_spmc_tx_lock);
> +}
> +
> +void ffa_rxtx_spmc_destroy(void)
> +{
> + bool need_unmap;
> +
> + spin_lock(&ffa_spmc_rx_lock);
> + spin_lock(&ffa_spmc_tx_lock);
> + need_unmap = ffa_spmc_tx && ffa_spmc_rx;
> +
> + if ( ffa_spmc_tx )
> {
> - free_xenheap_pages(ffa_tx, 0);
> - ffa_tx = NULL;
> + free_xenheap_pages(ffa_spmc_tx, 0);
> + ffa_spmc_tx = NULL;
> }
> - if ( ffa_rx )
> + if ( ffa_spmc_rx )
> {
> - free_xenheap_pages(ffa_rx, 0);
> - ffa_rx = NULL;
> + free_xenheap_pages(ffa_spmc_rx, 0);
> + ffa_spmc_rx = NULL;
> }
>
> if ( need_unmap )
> ffa_rxtx_unmap(0);
> +
> + spin_unlock(&ffa_spmc_tx_lock);
> + spin_unlock(&ffa_spmc_rx_lock);
> }
>
> -bool ffa_rxtx_init(void)
> +bool ffa_rxtx_spmc_init(void)
> {
> int32_t e;
> + bool ret = false;
>
> /* Firmware not there or not supporting */
> if ( !ffa_fw_supports_fid(FFA_RXTX_MAP_64) )
> return false;
>
> - ffa_rx = alloc_xenheap_pages(get_order_from_pages(FFA_RXTX_PAGE_COUNT),
> 0);
> - if ( !ffa_rx )
> - return false;
> + spin_lock(&ffa_spmc_rx_lock);
> + spin_lock(&ffa_spmc_tx_lock);
> +
> + ffa_spmc_rx = alloc_xenheap_pages(
> + get_order_from_pages(FFA_RXTX_PAGE_COUNT), 0);
> + if ( !ffa_spmc_rx )
> + goto exit;
>
> - ffa_tx = alloc_xenheap_pages(get_order_from_pages(FFA_RXTX_PAGE_COUNT),
> 0);
> - if ( !ffa_tx )
> - goto err;
> + ffa_spmc_tx = alloc_xenheap_pages(
> + get_order_from_pages(FFA_RXTX_PAGE_COUNT), 0);
> + if ( !ffa_spmc_tx )
> + goto exit;
>
> - e = ffa_rxtx_map(__pa(ffa_tx), __pa(ffa_rx), FFA_RXTX_PAGE_COUNT);
> + e = ffa_rxtx_map(__pa(ffa_spmc_tx), __pa(ffa_spmc_rx),
> + FFA_RXTX_PAGE_COUNT);
> if ( e )
> - goto err;
> + goto exit;
>
> - return true;
> + ret = true;
>
> -err:
> - ffa_rxtx_destroy();
> +exit:
> + spin_unlock(&ffa_spmc_tx_lock);
> + spin_unlock(&ffa_spmc_rx_lock);
>
> - return false;
> + if ( !ret )
> + ffa_rxtx_spmc_destroy();
> +
> + return ret;
> }
> diff --git a/xen/arch/arm/tee/ffa_shm.c b/xen/arch/arm/tee/ffa_shm.c
> index d628c1b70609..b9022797c3bf 100644
> --- a/xen/arch/arm/tee/ffa_shm.c
> +++ b/xen/arch/arm/tee/ffa_shm.c
> @@ -286,9 +286,8 @@ static void init_range(struct ffa_address_range
> *addr_range,
> }
>
> /*
> - * This function uses the ffa_tx buffer to transmit the memory transaction
> - * descriptor. The function depends ffa_tx_buffer_lock to be used to guard
> - * the buffer from concurrent use.
> + * This function uses the ffa_spmc tx buffer to transmit the memory
> transaction
> + * descriptor.
> */
> static int share_shm(struct ffa_shm_mem *shm)
> {
> @@ -298,17 +297,22 @@ static int share_shm(struct ffa_shm_mem *shm)
> struct ffa_address_range *addr_range;
> struct ffa_mem_region *region_descr;
> const unsigned int region_count = 1;
> - void *buf = ffa_tx;
> uint32_t frag_len;
> uint32_t tot_len;
> paddr_t last_pa;
> unsigned int n;
> paddr_t pa;
> + int32_t ret;
> + void *buf;
>
> - ASSERT(spin_is_locked(&ffa_tx_buffer_lock));
> ASSERT(shm->page_count);
>
> + buf = ffa_rxtx_spmc_tx_acquire();
> + if ( !buf )
> + return FFA_RET_NOT_SUPPORTED;
> +
> descr = buf;
> +
> memset(descr, 0, sizeof(*descr));
> descr->sender_id = shm->sender_id;
> descr->handle = shm->handle;
> @@ -340,7 +344,10 @@ static int share_shm(struct ffa_shm_mem *shm)
> tot_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count,
> region_descr->address_range_count);
> if ( tot_len > max_frag_len )
> + {
> + ffa_rxtx_spmc_tx_release();
> return FFA_RET_NOT_SUPPORTED;
I'd prefer adding an out label below before the
ffa_rxtx_spmc_tx_release() call and:
ret = FFA_RET_NOT_SUPPORTED;
goto out;
Cheers,
Jens
> + }
>
> addr_range = region_descr->address_range_array;
> frag_len = ADDR_RANGE_OFFSET(descr->mem_access_count, region_count, 1);
> @@ -360,7 +367,11 @@ static int share_shm(struct ffa_shm_mem *shm)
> init_range(addr_range, pa);
> }
>
> - return ffa_mem_share(tot_len, frag_len, 0, 0, &shm->handle);
> + ret = ffa_mem_share(tot_len, frag_len, 0, 0, &shm->handle);
> +
> + ffa_rxtx_spmc_tx_release();
> +
> + return ret;
> }
>
> static int read_mem_transaction(uint32_t ffa_vers, const void *buf, size_t
> blen,
> @@ -578,10 +589,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
> if ( ret )
> goto out;
>
> - /* Note that share_shm() uses our tx buffer */
> - spin_lock(&ffa_tx_buffer_lock);
> ret = share_shm(shm);
> - spin_unlock(&ffa_tx_buffer_lock);
> if ( ret )
> goto out;
>
> --
> 2.51.2
>
