The single lock in struct ffa_ctx is complemented with rx_lock and tx_lock.
The old lock is used for small critical sections, like increasing
shm_count or adding another shm to shm_list.
rx_lock and tx_lock are only acquired using spin_trylock() which for
well-behaving guests should always succeed. Guests using the RX and TX
buffers are expected to serialize accesses before doing the FF-A
request.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
v10->v11
- No changes except resolving conflicts since "xen/arm: ffa: support
sharing large memory ranges" was dropped.
---
xen/arch/arm/tee/ffa.c | 107 +++++++++++++++++++++++++++++------------
1 file changed, 77 insertions(+), 30 deletions(-)
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 75700938c444..eb4a58fec470 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -363,6 +363,13 @@ struct ffa_ctx {
struct list_head shm_list;
/* Number of allocated shared memory object */
unsigned int shm_count;
+ /*
+ * tx_lock is used to serialize access to tx
+ * rx_lock is used to serialize access to rx
+ * lock is used for the rest in this struct
+ */
+ spinlock_t tx_lock;
+ spinlock_t rx_lock;
spinlock_t lock;
};
@@ -768,7 +775,9 @@ static int32_t handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
if ( !ffa_rx )
return FFA_RET_DENIED;
- spin_lock(&ctx->lock);
+ if ( !spin_trylock(&ctx->rx_lock) )
+ return FFA_RET_BUSY;
+
if ( !ctx->page_count || !ctx->rx_is_free )
goto out;
spin_lock(&ffa_rx_buffer_lock);
@@ -819,7 +828,7 @@ out_rx_release:
out_rx_buf_unlock:
spin_unlock(&ffa_rx_buffer_lock);
out:
- spin_unlock(&ctx->lock);
+ spin_unlock(&ctx->rx_lock);
return ret;
}
@@ -830,13 +839,15 @@ static int32_t handle_rx_release(void)
struct domain *d = current->domain;
struct ffa_ctx *ctx = d->arch.tee;
- spin_lock(&ctx->lock);
+ if ( !spin_trylock(&ctx->rx_lock) )
+ return FFA_RET_BUSY;
+
if ( !ctx->page_count || ctx->rx_is_free )
goto out;
ret = FFA_RET_OK;
ctx->rx_is_free = true;
out:
- spin_unlock(&ctx->lock);
+ spin_unlock(&ctx->rx_lock);
return ret;
}
@@ -947,21 +958,43 @@ static void put_shm_pages(struct ffa_shm_mem *shm)
}
}
+static bool inc_ctx_shm_count(struct ffa_ctx *ctx)
+{
+ bool ret = true;
+
+ spin_lock(&ctx->lock);
+ if (ctx->shm_count >= FFA_MAX_SHM_COUNT)
+ ret = false;
+ else
+ ctx->shm_count++;
+ spin_unlock(&ctx->lock);
+
+ return ret;
+}
+
+static void dec_ctx_shm_count(struct ffa_ctx *ctx)
+{
+ spin_lock(&ctx->lock);
+ ASSERT(ctx->shm_count > 0);
+ ctx->shm_count--;
+ spin_unlock(&ctx->lock);
+}
+
static struct ffa_shm_mem *alloc_ffa_shm_mem(struct ffa_ctx *ctx,
unsigned int page_count)
{
struct ffa_shm_mem *shm;
- if ( page_count >= FFA_MAX_SHM_PAGE_COUNT ||
- ctx->shm_count >= FFA_MAX_SHM_COUNT )
+ if ( page_count >= FFA_MAX_SHM_PAGE_COUNT )
+ return NULL;
+ if ( !inc_ctx_shm_count(ctx) )
return NULL;
shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count);
if ( shm )
- {
- ctx->shm_count++;
shm->page_count = page_count;
- }
+ else
+ dec_ctx_shm_count(ctx);
return shm;
}
@@ -971,8 +1004,7 @@ static void free_ffa_shm_mem(struct ffa_ctx *ctx, struct ffa_shm_mem *shm)
if ( !shm )
return;
- ASSERT(ctx->shm_count > 0);
- ctx->shm_count--;
+ dec_ctx_shm_count(ctx);
put_shm_pages(shm);
xfree(shm);
}
@@ -1180,7 +1212,11 @@ static void handle_mem_share(struct cpu_user_regs *regs)
goto out_set_ret;
}
- spin_lock(&ctx->lock);
+ if ( !spin_trylock(&ctx->tx_lock) )
+ {
+ ret = FFA_RET_BUSY;
+ goto out_set_ret;
+ }
if ( frag_len > ctx->page_count * FFA_PAGE_SIZE )
goto out_unlock;
@@ -1272,7 +1308,9 @@ static void handle_mem_share(struct cpu_user_regs *regs)
if ( ret )
goto out;
+ spin_lock(&ctx->lock);
list_add_tail(&shm->list, &ctx->shm_list);
+ spin_unlock(&ctx->lock);
uint64_to_regpair(&handle_hi, &handle_lo, shm->handle);
@@ -1280,7 +1318,7 @@ out:
if ( ret )
free_ffa_shm_mem(ctx, shm);
out_unlock:
- spin_unlock(&ctx->lock);
+ spin_unlock(&ctx->tx_lock);
out_set_ret:
if ( ret == 0)
@@ -1289,6 +1327,18 @@ out_set_ret:
set_regs_error(regs, ret);
}
+/* Must only be called with ctx->lock held */
+static struct ffa_shm_mem *find_shm_mem(struct ffa_ctx *ctx, uint64_t handle)
+{
+ struct ffa_shm_mem *shm;
+
+ list_for_each_entry(shm, &ctx->shm_list, list)
+ if ( shm->handle == handle )
+ return shm;
+
+ return NULL;
+}
+
static int handle_mem_reclaim(uint64_t handle, uint32_t flags)
{
struct domain *d = current->domain;
@@ -1299,29 +1349,26 @@ static int handle_mem_reclaim(uint64_t handle, uint32_t flags)
int ret;
spin_lock(&ctx->lock);
- list_for_each_entry(shm, &ctx->shm_list, list)
- {
- if ( shm->handle == handle )
- goto found_it;
- }
- shm = NULL;
- ret = FFA_RET_INVALID_PARAMETERS;
- goto out;
-found_it:
+ shm = find_shm_mem(ctx, handle);
+ if ( shm )
+ list_del(&shm->list);
+ spin_unlock(&ctx->lock);
+ if ( !shm )
+ return FFA_RET_INVALID_PARAMETERS;
uint64_to_regpair(&handle_hi, &handle_lo, handle);
ret = ffa_mem_reclaim(handle_lo, handle_hi, flags);
+
if ( ret )
{
- shm = NULL;
- goto out;
+ spin_lock(&ctx->lock);
+ list_add_tail(&shm->list, &ctx->shm_list);
+ spin_unlock(&ctx->lock);
+ }
+ else
+ {
+ free_ffa_shm_mem(ctx, shm);
}
-
- list_del(&shm->list);
-
-out:
- free_ffa_shm_mem(ctx, shm);
- spin_unlock(&ctx->lock);
return ret;
}
--
2.34.1Hi Jens,
> On 31 Jul 2023, at 14:15, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> The single lock in struct ffa_ctx is complemented with rx_lock and tx_lock.
>
> The old lock is used for small critical sections, like increasing
> shm_count or adding another shm to shm_list.
>
> rx_lock and tx_lock are only acquired using spin_trylock() which for
> well-behaving guests should always succeed. Guests using the RX and TX
> buffers are expected to serialize accesses before doing the FF-A
> request.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
> v10->v11
> - No changes except resolving conflicts since "xen/arm: ffa: support
> sharing large memory ranges" was dropped.
> ---
> xen/arch/arm/tee/ffa.c | 107 +++++++++++++++++++++++++++++------------
> 1 file changed, 77 insertions(+), 30 deletions(-)
>
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 75700938c444..eb4a58fec470 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -363,6 +363,13 @@ struct ffa_ctx {
> struct list_head shm_list;
> /* Number of allocated shared memory object */
> unsigned int shm_count;
> + /*
> + * tx_lock is used to serialize access to tx
> + * rx_lock is used to serialize access to rx
> + * lock is used for the rest in this struct
> + */
> + spinlock_t tx_lock;
> + spinlock_t rx_lock;
> spinlock_t lock;
> };
>
> @@ -768,7 +775,9 @@ static int32_t handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
> if ( !ffa_rx )
> return FFA_RET_DENIED;
>
> - spin_lock(&ctx->lock);
> + if ( !spin_trylock(&ctx->rx_lock) )
> + return FFA_RET_BUSY;
> +
> if ( !ctx->page_count || !ctx->rx_is_free )
> goto out;
> spin_lock(&ffa_rx_buffer_lock);
> @@ -819,7 +828,7 @@ out_rx_release:
> out_rx_buf_unlock:
> spin_unlock(&ffa_rx_buffer_lock);
> out:
> - spin_unlock(&ctx->lock);
> + spin_unlock(&ctx->rx_lock);
>
> return ret;
> }
> @@ -830,13 +839,15 @@ static int32_t handle_rx_release(void)
> struct domain *d = current->domain;
> struct ffa_ctx *ctx = d->arch.tee;
>
> - spin_lock(&ctx->lock);
> + if ( !spin_trylock(&ctx->rx_lock) )
> + return FFA_RET_BUSY;
> +
> if ( !ctx->page_count || ctx->rx_is_free )
> goto out;
> ret = FFA_RET_OK;
> ctx->rx_is_free = true;
> out:
> - spin_unlock(&ctx->lock);
> + spin_unlock(&ctx->rx_lock);
>
> return ret;
> }
> @@ -947,21 +958,43 @@ static void put_shm_pages(struct ffa_shm_mem *shm)
> }
> }
>
> +static bool inc_ctx_shm_count(struct ffa_ctx *ctx)
> +{
> + bool ret = true;
> +
> + spin_lock(&ctx->lock);
> + if (ctx->shm_count >= FFA_MAX_SHM_COUNT)
> + ret = false;
> + else
> + ctx->shm_count++;
> + spin_unlock(&ctx->lock);
> +
> + return ret;
> +}
> +
> +static void dec_ctx_shm_count(struct ffa_ctx *ctx)
> +{
> + spin_lock(&ctx->lock);
> + ASSERT(ctx->shm_count > 0);
> + ctx->shm_count--;
> + spin_unlock(&ctx->lock);
> +}
> +
> static struct ffa_shm_mem *alloc_ffa_shm_mem(struct ffa_ctx *ctx,
> unsigned int page_count)
> {
> struct ffa_shm_mem *shm;
>
> - if ( page_count >= FFA_MAX_SHM_PAGE_COUNT ||
> - ctx->shm_count >= FFA_MAX_SHM_COUNT )
> + if ( page_count >= FFA_MAX_SHM_PAGE_COUNT )
> + return NULL;
> + if ( !inc_ctx_shm_count(ctx) )
> return NULL;
>
> shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count);
> if ( shm )
> - {
> - ctx->shm_count++;
> shm->page_count = page_count;
> - }
> + else
> + dec_ctx_shm_count(ctx);
>
> return shm;
> }
> @@ -971,8 +1004,7 @@ static void free_ffa_shm_mem(struct ffa_ctx *ctx, struct ffa_shm_mem *shm)
> if ( !shm )
> return;
>
> - ASSERT(ctx->shm_count > 0);
> - ctx->shm_count--;
> + dec_ctx_shm_count(ctx);
> put_shm_pages(shm);
> xfree(shm);
> }
> @@ -1180,7 +1212,11 @@ static void handle_mem_share(struct cpu_user_regs *regs)
> goto out_set_ret;
> }
>
> - spin_lock(&ctx->lock);
> + if ( !spin_trylock(&ctx->tx_lock) )
> + {
> + ret = FFA_RET_BUSY;
> + goto out_set_ret;
> + }
>
> if ( frag_len > ctx->page_count * FFA_PAGE_SIZE )
> goto out_unlock;
> @@ -1272,7 +1308,9 @@ static void handle_mem_share(struct cpu_user_regs *regs)
> if ( ret )
> goto out;
>
> + spin_lock(&ctx->lock);
> list_add_tail(&shm->list, &ctx->shm_list);
> + spin_unlock(&ctx->lock);
>
> uint64_to_regpair(&handle_hi, &handle_lo, shm->handle);
>
> @@ -1280,7 +1318,7 @@ out:
> if ( ret )
> free_ffa_shm_mem(ctx, shm);
> out_unlock:
> - spin_unlock(&ctx->lock);
> + spin_unlock(&ctx->tx_lock);
>
> out_set_ret:
> if ( ret == 0)
> @@ -1289,6 +1327,18 @@ out_set_ret:
> set_regs_error(regs, ret);
> }
>
> +/* Must only be called with ctx->lock held */
> +static struct ffa_shm_mem *find_shm_mem(struct ffa_ctx *ctx, uint64_t handle)
> +{
> + struct ffa_shm_mem *shm;
> +
> + list_for_each_entry(shm, &ctx->shm_list, list)
> + if ( shm->handle == handle )
> + return shm;
> +
> + return NULL;
> +}
> +
> static int handle_mem_reclaim(uint64_t handle, uint32_t flags)
> {
> struct domain *d = current->domain;
> @@ -1299,29 +1349,26 @@ static int handle_mem_reclaim(uint64_t handle, uint32_t flags)
> int ret;
>
> spin_lock(&ctx->lock);
> - list_for_each_entry(shm, &ctx->shm_list, list)
> - {
> - if ( shm->handle == handle )
> - goto found_it;
> - }
> - shm = NULL;
> - ret = FFA_RET_INVALID_PARAMETERS;
> - goto out;
> -found_it:
> + shm = find_shm_mem(ctx, handle);
> + if ( shm )
> + list_del(&shm->list);
> + spin_unlock(&ctx->lock);
> + if ( !shm )
> + return FFA_RET_INVALID_PARAMETERS;
>
> uint64_to_regpair(&handle_hi, &handle_lo, handle);
> ret = ffa_mem_reclaim(handle_lo, handle_hi, flags);
> +
> if ( ret )
> {
> - shm = NULL;
> - goto out;
> + spin_lock(&ctx->lock);
> + list_add_tail(&shm->list, &ctx->shm_list);
> + spin_unlock(&ctx->lock);
> + }
> + else
> + {
> + free_ffa_shm_mem(ctx, shm);
> }
> -
> - list_del(&shm->list);
> -
> -out:
> - free_ffa_shm_mem(ctx, shm);
> - spin_unlock(&ctx->lock);
>
> return ret;
> }
> --
> 2.34.1
>
© 2016 - 2026 Red Hat, Inc.