xen/arch/arm/tee/ffa.c | 270 ++++++++++++++++++++++++++++++++++------- 1 file changed, 224 insertions(+), 46 deletions(-)
When an FF-A enabled guest is destroyed it may leave behind memory
shared with SPs. This memory must be reclaimed before it's reused or an
SP may make changes to memory used by a new unrelated guest. So when the
domain is teared down add FF-A requests to reclaim all remaining shared
memory.
SPs in the secure world are notified using VM_DESTROYED that a guest has
been destroyed. An SP is supposed to relinquish all shared memory to allow
reclaiming the memory. The relinquish operation may need to be delayed if
the shared memory is for instance part of a DMA operation.
The domain reference counter is increased when the first FF-A shared
memory is registered and the counter is decreased again when the last
shared memory is reclaimed. If FF-A shared memory registrations remain
at the end of of ffa_domain_teardown() a timer is set to try to reclaim
the shared memory every second until the memory is reclaimed.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
Hi,
This is a follow-up to the RFC patch. In this patch, I take an explicit
reference on the domain when FF-A shared memory is registered. I've
discovered that it might not be strictly necessary until all the shared
pages are released with put_page() they also keep a reference to the
domain.
I'm not entirely sure what is the status of the shared memory when the
domain has turned into a zombie. We still hold references on the shared
pages until put_page() is called on each. Is that enough to guarantee that
they will not be reused?
Thanks,
Jens
v2:
- Update commit message to match the new implementation
- Using a per domain bitfield to keep track of which SPs has been notified
with VM_DESTROYED
- Holding a domain reference counter to keep the domain as a zombie domain
while there still is shared memory registrations remaining to be reclaimed
- Using a timer to retry reclaiming remaining shared memory registrations
---
xen/arch/arm/tee/ffa.c | 270 ++++++++++++++++++++++++++++++++++-------
1 file changed, 224 insertions(+), 46 deletions(-)
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 183528d13388..22906a6e1cff 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -54,6 +54,7 @@
#include <xen/mm.h>
#include <xen/sched.h>
#include <xen/sizes.h>
+#include <xen/timer.h>
#include <xen/types.h>
#include <asm/event.h>
@@ -384,11 +385,6 @@ struct ffa_ctx {
unsigned int page_count;
/* FF-A version used by the guest */
uint32_t guest_vers;
- /*
- * Number of SPs that we have sent a VM created signal to, used in
- * ffa_domain_teardown() to know which SPs need to be signalled.
- */
- uint16_t create_signal_count;
bool rx_is_free;
/* Used shared memory objects, struct ffa_shm_mem */
struct list_head shm_list;
@@ -402,6 +398,15 @@ struct ffa_ctx {
spinlock_t tx_lock;
spinlock_t rx_lock;
spinlock_t lock;
+ /* Used if domain can't be teared down immediately */
+ struct domain *teardown_d;
+ struct list_head teardown_list;
+ s_time_t teardown_expire;
+ /*
+ * Used for ffa_domain_teardown() to keep track of which SPs should be
+ * notified that this guest is being destroyed.
+ */
+ unsigned long vm_destroy_bitmap[];
};
struct ffa_shm_mem {
@@ -436,6 +441,12 @@ static void *ffa_tx __read_mostly;
static DEFINE_SPINLOCK(ffa_rx_buffer_lock);
static DEFINE_SPINLOCK(ffa_tx_buffer_lock);
+
+/* Used if domain can't be teared down immediately */
+static struct timer ffa_teardown_timer;
+static struct list_head ffa_teardown_head;
+static DEFINE_SPINLOCK(ffa_teardown_lock);
+
static bool ffa_get_version(uint32_t *vers)
{
const struct arm_smccc_1_2_regs arg = {
@@ -850,7 +861,6 @@ static int32_t handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
goto out_rx_release;
}
-
memcpy(ctx->rx, ffa_rx, sz);
}
ctx->rx_is_free = false;
@@ -989,53 +999,75 @@ static void put_shm_pages(struct ffa_shm_mem *shm)
}
}
-static bool inc_ctx_shm_count(struct ffa_ctx *ctx)
+static bool inc_ctx_shm_count(struct domain *d, struct ffa_ctx *ctx)
{
bool ret = true;
spin_lock(&ctx->lock);
+
+ /*
+ * If this is the first shm added, increase the domain reference
+ * counter as we may need to domain around a bit longer to reclaim the
+ * shared memory in the teardown path.
+ */
+ if ( !ctx->shm_count )
+ get_knownalive_domain(d);
+
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)
+static void dec_ctx_shm_count(struct domain *d, struct ffa_ctx *ctx)
{
spin_lock(&ctx->lock);
+
ASSERT(ctx->shm_count > 0);
ctx->shm_count--;
+
+ /*
+ * If this was the last shm removed, let go of the domain reference we
+ * took in inc_ctx_shm_count() above.
+ */
+ if ( !ctx->shm_count )
+ put_domain(d);
+
spin_unlock(&ctx->lock);
}
-static struct ffa_shm_mem *alloc_ffa_shm_mem(struct ffa_ctx *ctx,
+static struct ffa_shm_mem *alloc_ffa_shm_mem(struct domain *d,
unsigned int page_count)
{
+ struct ffa_ctx *ctx = d->arch.tee;
struct ffa_shm_mem *shm;
if ( page_count >= FFA_MAX_SHM_PAGE_COUNT )
return NULL;
- if ( !inc_ctx_shm_count(ctx) )
+ if ( !inc_ctx_shm_count(d, ctx) )
return NULL;
shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count);
if ( shm )
shm->page_count = page_count;
else
- dec_ctx_shm_count(ctx);
+ dec_ctx_shm_count(d, ctx);
return shm;
}
-static void free_ffa_shm_mem(struct ffa_ctx *ctx, struct ffa_shm_mem *shm)
+static void free_ffa_shm_mem(struct domain *d, struct ffa_shm_mem *shm)
{
+ struct ffa_ctx *ctx = d->arch.tee;
+
if ( !shm )
return;
- dec_ctx_shm_count(ctx);
+ dec_ctx_shm_count(d, ctx);
put_shm_pages(shm);
xfree(shm);
}
@@ -1303,7 +1335,7 @@ static void handle_mem_share(struct cpu_user_regs *regs)
goto out_unlock;
}
- shm = alloc_ffa_shm_mem(ctx, page_count);
+ shm = alloc_ffa_shm_mem(d, page_count);
if ( !shm )
{
ret = FFA_RET_NO_MEMORY;
@@ -1347,7 +1379,7 @@ static void handle_mem_share(struct cpu_user_regs *regs)
out:
if ( ret )
- free_ffa_shm_mem(ctx, shm);
+ free_ffa_shm_mem(d, shm);
out_unlock:
spin_unlock(&ctx->tx_lock);
@@ -1398,7 +1430,7 @@ static int handle_mem_reclaim(uint64_t handle, uint32_t flags)
}
else
{
- free_ffa_shm_mem(ctx, shm);
+ free_ffa_shm_mem(d, shm);
}
return ret;
@@ -1481,6 +1513,41 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
}
}
+static bool is_in_subscr_list(const uint16_t *subscr, uint16_t start,
+ uint16_t end, uint16_t sp_id)
+{
+ unsigned int n;
+
+ for ( n = start; n < end; n++ )
+ {
+ if ( subscr[n] == sp_id )
+ return true;
+ }
+
+ return false;
+}
+
+static void vm_destroy_bitmap_init(struct ffa_ctx *ctx,
+ unsigned int create_signal_count)
+{
+ unsigned int n;
+
+ for ( n = 0; n < subscr_vm_destroyed_count; n++ )
+ {
+ /*
+ * Skip SPs subscribed to the VM created event that never was
+ * notified of the VM creation due to an error during
+ * ffa_domain_init().
+ */
+ if ( is_in_subscr_list(subscr_vm_created, create_signal_count,
+ subscr_vm_created_count,
+ subscr_vm_destroyed[n]) )
+ continue;
+
+ set_bit(n, ctx->vm_destroy_bitmap);
+ }
+}
+
static int ffa_domain_init(struct domain *d)
{
struct ffa_ctx *ctx;
@@ -1496,11 +1563,14 @@ static int ffa_domain_init(struct domain *d)
if ( d->domain_id >= UINT16_MAX)
return -ERANGE;
- ctx = xzalloc(struct ffa_ctx);
+ ctx = xzalloc_flex_struct(struct ffa_ctx, vm_destroy_bitmap,
+ BITS_TO_LONGS(subscr_vm_destroyed_count));
if ( !ctx )
return -ENOMEM;
d->arch.tee = ctx;
+ ctx->teardown_d = d;
+ INIT_LIST_HEAD(&ctx->shm_list);
for ( n = 0; n < subscr_vm_created_count; n++ )
{
@@ -1510,64 +1580,169 @@ static int ffa_domain_init(struct domain *d)
{
printk(XENLOG_ERR "ffa: Failed to report creation of vm_id %u to %u: res %d\n",
get_vm_id(d), subscr_vm_created[n], res);
- ctx->create_signal_count = n;
+ vm_destroy_bitmap_init(ctx, n);
return -EIO;
}
}
- ctx->create_signal_count = subscr_vm_created_count;
-
- INIT_LIST_HEAD(&ctx->shm_list);
+ vm_destroy_bitmap_init(ctx, subscr_vm_created_count);
return 0;
}
-static bool is_in_subscr_list(const uint16_t *subscr, uint16_t start,
- uint16_t end, uint16_t sp_id)
+static void send_vm_destroyed(struct domain *d)
{
+ struct ffa_ctx *ctx = d->arch.tee;
unsigned int n;
+ int32_t res;
- for ( n = start; n < end; n++ )
+ for ( n = 0; n < subscr_vm_destroyed_count; n++ )
{
- if ( subscr[n] == sp_id )
- return true;
- }
+ if ( !test_bit(n, ctx->vm_destroy_bitmap) )
+ continue;
- return false;
+ res = ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d),
+ FFA_MSG_SEND_VM_DESTROYED);
+
+ if ( res )
+ {
+ printk(XENLOG_ERR "%pd: ffa: Failed to report destruction of vm_id %u to %u: res %d\n",
+ d, get_vm_id(d), subscr_vm_destroyed[n], res);
+ }
+ else
+ {
+ clear_bit(n, ctx->vm_destroy_bitmap);
+ }
+ }
}
-/* This function is supposed to undo what ffa_domain_init() has done */
-static int ffa_domain_teardown(struct domain *d)
+static void reclaim_shms(struct domain *d)
{
struct ffa_ctx *ctx = d->arch.tee;
- unsigned int n;
+ struct ffa_shm_mem *shm, *tmp;
int32_t res;
+ list_for_each_entry_safe(shm, tmp, &ctx->shm_list, list)
+ {
+ register_t handle_hi;
+ register_t handle_lo;
+
+ uint64_to_regpair(&handle_hi, &handle_lo, shm->handle);
+ res = ffa_mem_reclaim(handle_lo, handle_hi, 0);
+ if ( res )
+ {
+ printk(XENLOG_G_INFO "%pd: ffa: Failed to reclaim handle %#lx : %d\n",
+ d, shm->handle, res);
+ }
+ else
+ {
+ printk(XENLOG_G_DEBUG "%pd: ffa: Reclaimed handle %#lx\n",
+ d, shm->handle);
+ list_del(&shm->list);
+ free_ffa_shm_mem(d, shm);
+ }
+ }
+}
+
+static bool ffa_domain_teardown_continue(struct ffa_ctx *ctx)
+{
+ send_vm_destroyed(ctx->teardown_d);
+ reclaim_shms(ctx->teardown_d);
+
+ if ( ctx->shm_count )
+ {
+ printk(XENLOG_G_INFO "%pd: ffa: Remaining unclaimed handles, retrying\n", ctx->teardown_d);
+ return false;
+ }
+ else
+ {
+ return true;
+ }
+}
+
+static void ffa_teardown_timer_callback(void *arg)
+{
+ struct ffa_ctx *ctx;
+ bool do_free;
+
+ spin_lock(&ffa_teardown_lock);
+ ctx = list_first_entry_or_null(&ffa_teardown_head, struct ffa_ctx,
+ teardown_list);
+ spin_unlock(&ffa_teardown_lock);
+
if ( !ctx )
- return 0;
+ {
+ printk(XENLOG_G_ERR "%s: teardown list is empty\n", __func__);
+ return;
+ }
- for ( n = 0; n < subscr_vm_destroyed_count; n++ )
+ do_free = ffa_domain_teardown_continue(ctx);
+
+ spin_lock(&ffa_teardown_lock);
+ list_del(&ctx->teardown_list);
+ if ( !do_free )
+ {
+ ctx->teardown_expire = NOW() + SECONDS(1);
+ list_add_tail(&ctx->teardown_list, &ffa_teardown_head);
+ }
+ spin_unlock(&ffa_teardown_lock);
+
+ if ( do_free )
{
/*
- * Skip SPs subscribed to the VM created event that never was
- * notified of the VM creation due to an error during
- * ffa_domain_init().
+ * domain_destroy() has likely been called (via put_domain() in
+ * reclaim_shms()) by now, so we can't touch the domain
+ * structure anymore.
*/
- if ( is_in_subscr_list(subscr_vm_created, ctx->create_signal_count,
- subscr_vm_created_count,
- subscr_vm_destroyed[n]) )
- continue;
+ xfree(ctx);
+ }
- res = ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d),
- FFA_MSG_SEND_VM_DESTROYED);
+ spin_lock(&ffa_teardown_lock);
+ ctx = list_first_entry_or_null(&ffa_teardown_head, struct ffa_ctx,
+ teardown_list);
+ spin_unlock(&ffa_teardown_lock);
- if ( res )
- printk(XENLOG_ERR "ffa: Failed to report destruction of vm_id %u to %u: res %d\n",
- get_vm_id(d), subscr_vm_destroyed[n], res);
- }
+ if ( ctx )
+ set_timer(&ffa_teardown_timer, ctx->teardown_expire);
+}
+
+static void ffa_queue_ctx_teardown(struct ffa_ctx *ctx)
+{
+ ctx->teardown_expire = NOW() + SECONDS(1);
+
+ spin_lock(&ffa_teardown_lock);
+
+ /*
+ * The timer is already active if there are queued teardown entries.
+ */
+ if ( list_empty(&ffa_teardown_head) )
+ set_timer(&ffa_teardown_timer, ctx->teardown_expire);
+
+ list_add_tail(&ctx->teardown_list, &ffa_teardown_head);
+
+ spin_unlock(&ffa_teardown_lock);
+}
+
+/* This function is supposed to undo what ffa_domain_init() has done */
+static int ffa_domain_teardown(struct domain *d)
+{
+ struct ffa_ctx *ctx = d->arch.tee;
+
+ if ( !ctx )
+ return 0;
if ( ctx->rx )
rxtx_unmap(ctx);
+ send_vm_destroyed(d);
+ reclaim_shms(d);
+
+ if ( ctx->shm_count )
+ {
+ printk(XENLOG_G_INFO "%pd: ffa: Remaining unclaimed handles, retrying\n", d);
+ ffa_queue_ctx_teardown(ctx);
+ return 0;
+ }
+
XFREE(d->arch.tee);
return 0;
@@ -1733,6 +1908,9 @@ static bool ffa_probe(void)
if ( !init_sps() )
goto err_free_ffa_tx;
+ INIT_LIST_HEAD(&ffa_teardown_head);
+ init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
+
return true;
err_free_ffa_tx:
--
2.34.1Hi Jens,
Very sorry for the long delay.
> On 13 Dec 2023, at 12:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> When an FF-A enabled guest is destroyed it may leave behind memory
> shared with SPs. This memory must be reclaimed before it's reused or an
> SP may make changes to memory used by a new unrelated guest. So when the
> domain is teared down add FF-A requests to reclaim all remaining shared
> memory.
>
> SPs in the secure world are notified using VM_DESTROYED that a guest has
> been destroyed. An SP is supposed to relinquish all shared memory to allow
> reclaiming the memory. The relinquish operation may need to be delayed if
> the shared memory is for instance part of a DMA operation.
>
> The domain reference counter is increased when the first FF-A shared
> memory is registered and the counter is decreased again when the last
> shared memory is reclaimed. If FF-A shared memory registrations remain
> at the end of of ffa_domain_teardown() a timer is set to try to reclaim
> the shared memory every second until the memory is reclaimed.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
> Hi,
>
> This is a follow-up to the RFC patch. In this patch, I take an explicit
> reference on the domain when FF-A shared memory is registered. I've
> discovered that it might not be strictly necessary until all the shared
> pages are released with put_page() they also keep a reference to the
> domain.
>
> I'm not entirely sure what is the status of the shared memory when the
> domain has turned into a zombie. We still hold references on the shared
> pages until put_page() is called on each. Is that enough to guarantee that
> they will not be reused?
>
> Thanks,
> Jens
>
> v2:
> - Update commit message to match the new implementation
> - Using a per domain bitfield to keep track of which SPs has been notified
> with VM_DESTROYED
> - Holding a domain reference counter to keep the domain as a zombie domain
> while there still is shared memory registrations remaining to be reclaimed
> - Using a timer to retry reclaiming remaining shared memory registrations
>
> ---
> xen/arch/arm/tee/ffa.c | 270 ++++++++++++++++++++++++++++++++++-------
> 1 file changed, 224 insertions(+), 46 deletions(-)
>
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 183528d13388..22906a6e1cff 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -54,6 +54,7 @@
> #include <xen/mm.h>
> #include <xen/sched.h>
> #include <xen/sizes.h>
> +#include <xen/timer.h>
> #include <xen/types.h>
>
> #include <asm/event.h>
> @@ -384,11 +385,6 @@ struct ffa_ctx {
> unsigned int page_count;
> /* FF-A version used by the guest */
> uint32_t guest_vers;
> - /*
> - * Number of SPs that we have sent a VM created signal to, used in
> - * ffa_domain_teardown() to know which SPs need to be signalled.
> - */
> - uint16_t create_signal_count;
> bool rx_is_free;
> /* Used shared memory objects, struct ffa_shm_mem */
> struct list_head shm_list;
> @@ -402,6 +398,15 @@ struct ffa_ctx {
> spinlock_t tx_lock;
> spinlock_t rx_lock;
> spinlock_t lock;
> + /* Used if domain can't be teared down immediately */
> + struct domain *teardown_d;
> + struct list_head teardown_list;
> + s_time_t teardown_expire;
> + /*
> + * Used for ffa_domain_teardown() to keep track of which SPs should be
> + * notified that this guest is being destroyed.
> + */
> + unsigned long vm_destroy_bitmap[];
> };
>
> struct ffa_shm_mem {
> @@ -436,6 +441,12 @@ static void *ffa_tx __read_mostly;
> static DEFINE_SPINLOCK(ffa_rx_buffer_lock);
> static DEFINE_SPINLOCK(ffa_tx_buffer_lock);
>
> +
> +/* Used if domain can't be teared down immediately */
Please adapt the comment as this for all domains.
Suggest: Used to track domains that could not be teared down immediately.
> +static struct timer ffa_teardown_timer;
> +static struct list_head ffa_teardown_head;
> +static DEFINE_SPINLOCK(ffa_teardown_lock);
> +
> static bool ffa_get_version(uint32_t *vers)
> {
> const struct arm_smccc_1_2_regs arg = {
> @@ -850,7 +861,6 @@ static int32_t handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
> goto out_rx_release;
> }
>
> -
You have several style changes like that in the patch.
Those are not major so I am ok if you keep them in the patch but please
mention in the commit messages that you do some code style fixes.
> memcpy(ctx->rx, ffa_rx, sz);
> }
> ctx->rx_is_free = false;
> @@ -989,53 +999,75 @@ static void put_shm_pages(struct ffa_shm_mem *shm)
> }
> }
>
> -static bool inc_ctx_shm_count(struct ffa_ctx *ctx)
> +static bool inc_ctx_shm_count(struct domain *d, struct ffa_ctx *ctx)
> {
> bool ret = true;
>
> spin_lock(&ctx->lock);
> +
> + /*
> + * If this is the first shm added, increase the domain reference
> + * counter as we may need to domain around a bit longer to reclaim the
This sentence needs fixing (need to keep the domain..)
> + * shared memory in the teardown path.
> + */
> + if ( !ctx->shm_count )
> + get_knownalive_domain(d);
> +
> 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)
> +static void dec_ctx_shm_count(struct domain *d, struct ffa_ctx *ctx)
> {
> spin_lock(&ctx->lock);
> +
> ASSERT(ctx->shm_count > 0);
> ctx->shm_count--;
> +
> + /*
> + * If this was the last shm removed, let go of the domain reference we
> + * took in inc_ctx_shm_count() above.
> + */
> + if ( !ctx->shm_count )
> + put_domain(d);
> +
> spin_unlock(&ctx->lock);
> }
>
> -static struct ffa_shm_mem *alloc_ffa_shm_mem(struct ffa_ctx *ctx,
> +static struct ffa_shm_mem *alloc_ffa_shm_mem(struct domain *d,
> unsigned int page_count)
> {
> + struct ffa_ctx *ctx = d->arch.tee;
> struct ffa_shm_mem *shm;
>
> if ( page_count >= FFA_MAX_SHM_PAGE_COUNT )
> return NULL;
> - if ( !inc_ctx_shm_count(ctx) )
> + if ( !inc_ctx_shm_count(d, ctx) )
> return NULL;
>
> shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count);
> if ( shm )
> shm->page_count = page_count;
> else
> - dec_ctx_shm_count(ctx);
> + dec_ctx_shm_count(d, ctx);
>
> return shm;
> }
>
> -static void free_ffa_shm_mem(struct ffa_ctx *ctx, struct ffa_shm_mem *shm)
> +static void free_ffa_shm_mem(struct domain *d, struct ffa_shm_mem *shm)
> {
> + struct ffa_ctx *ctx = d->arch.tee;
> +
> if ( !shm )
> return;
>
> - dec_ctx_shm_count(ctx);
> + dec_ctx_shm_count(d, ctx);
> put_shm_pages(shm);
> xfree(shm);
> }
> @@ -1303,7 +1335,7 @@ static void handle_mem_share(struct cpu_user_regs *regs)
> goto out_unlock;
> }
>
> - shm = alloc_ffa_shm_mem(ctx, page_count);
> + shm = alloc_ffa_shm_mem(d, page_count);
> if ( !shm )
> {
> ret = FFA_RET_NO_MEMORY;
> @@ -1347,7 +1379,7 @@ static void handle_mem_share(struct cpu_user_regs *regs)
>
> out:
> if ( ret )
> - free_ffa_shm_mem(ctx, shm);
> + free_ffa_shm_mem(d, shm);
> out_unlock:
> spin_unlock(&ctx->tx_lock);
>
> @@ -1398,7 +1430,7 @@ static int handle_mem_reclaim(uint64_t handle, uint32_t flags)
> }
> else
> {
> - free_ffa_shm_mem(ctx, shm);
> + free_ffa_shm_mem(d, shm);
> }
>
> return ret;
> @@ -1481,6 +1513,41 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> }
> }
>
> +static bool is_in_subscr_list(const uint16_t *subscr, uint16_t start,
> + uint16_t end, uint16_t sp_id)
> +{
> + unsigned int n;
> +
> + for ( n = start; n < end; n++ )
> + {
> + if ( subscr[n] == sp_id )
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static void vm_destroy_bitmap_init(struct ffa_ctx *ctx,
> + unsigned int create_signal_count)
> +{
> + unsigned int n;
> +
> + for ( n = 0; n < subscr_vm_destroyed_count; n++ )
> + {
> + /*
> + * Skip SPs subscribed to the VM created event that never was
> + * notified of the VM creation due to an error during
> + * ffa_domain_init().
> + */
> + if ( is_in_subscr_list(subscr_vm_created, create_signal_count,
> + subscr_vm_created_count,
> + subscr_vm_destroyed[n]) )
> + continue;
> +
> + set_bit(n, ctx->vm_destroy_bitmap);
> + }
> +}
> +
> static int ffa_domain_init(struct domain *d)
> {
> struct ffa_ctx *ctx;
> @@ -1496,11 +1563,14 @@ static int ffa_domain_init(struct domain *d)
> if ( d->domain_id >= UINT16_MAX)
> return -ERANGE;
>
> - ctx = xzalloc(struct ffa_ctx);
> + ctx = xzalloc_flex_struct(struct ffa_ctx, vm_destroy_bitmap,
> + BITS_TO_LONGS(subscr_vm_destroyed_count));
> if ( !ctx )
> return -ENOMEM;
>
> d->arch.tee = ctx;
> + ctx->teardown_d = d;
> + INIT_LIST_HEAD(&ctx->shm_list);
>
> for ( n = 0; n < subscr_vm_created_count; n++ )
> {
> @@ -1510,64 +1580,169 @@ static int ffa_domain_init(struct domain *d)
> {
> printk(XENLOG_ERR "ffa: Failed to report creation of vm_id %u to %u: res %d\n",
> get_vm_id(d), subscr_vm_created[n], res);
> - ctx->create_signal_count = n;
> + vm_destroy_bitmap_init(ctx, n);
Here you could just break and ..
> return -EIO;
> }
> }
> - ctx->create_signal_count = subscr_vm_created_count;
> -
> - INIT_LIST_HEAD(&ctx->shm_list);
> + vm_destroy_bitmap_init(ctx, subscr_vm_created_count);
call with n and return -EIO if n != vm_created_count.
>
> return 0;
> }
>
> -static bool is_in_subscr_list(const uint16_t *subscr, uint16_t start,
> - uint16_t end, uint16_t sp_id)
> +static void send_vm_destroyed(struct domain *d)
> {
> + struct ffa_ctx *ctx = d->arch.tee;
> unsigned int n;
> + int32_t res;
>
> - for ( n = start; n < end; n++ )
> + for ( n = 0; n < subscr_vm_destroyed_count; n++ )
> {
> - if ( subscr[n] == sp_id )
> - return true;
> - }
> + if ( !test_bit(n, ctx->vm_destroy_bitmap) )
> + continue;
>
> - return false;
> + res = ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d),
> + FFA_MSG_SEND_VM_DESTROYED);
> +
> + if ( res )
> + {
> + printk(XENLOG_ERR "%pd: ffa: Failed to report destruction of vm_id %u to %u: res %d\n",
> + d, get_vm_id(d), subscr_vm_destroyed[n], res);
> + }
> + else
> + {
> + clear_bit(n, ctx->vm_destroy_bitmap);
> + }
> + }
> }
>
> -/* This function is supposed to undo what ffa_domain_init() has done */
> -static int ffa_domain_teardown(struct domain *d)
> +static void reclaim_shms(struct domain *d)
> {
> struct ffa_ctx *ctx = d->arch.tee;
> - unsigned int n;
> + struct ffa_shm_mem *shm, *tmp;
> int32_t res;
>
> + list_for_each_entry_safe(shm, tmp, &ctx->shm_list, list)
> + {
> + register_t handle_hi;
> + register_t handle_lo;
> +
> + uint64_to_regpair(&handle_hi, &handle_lo, shm->handle);
> + res = ffa_mem_reclaim(handle_lo, handle_hi, 0);
> + if ( res )
> + {
> + printk(XENLOG_G_INFO "%pd: ffa: Failed to reclaim handle %#lx : %d\n",
> + d, shm->handle, res);
> + }
> + else
> + {
> + printk(XENLOG_G_DEBUG "%pd: ffa: Reclaimed handle %#lx\n",
> + d, shm->handle);
> + list_del(&shm->list);
> + free_ffa_shm_mem(d, shm);
> + }
> + }
> +}
> +
> +static bool ffa_domain_teardown_continue(struct ffa_ctx *ctx)
> +{
> + send_vm_destroyed(ctx->teardown_d);
> + reclaim_shms(ctx->teardown_d);
> +
> + if ( ctx->shm_count )
> + {
> + printk(XENLOG_G_INFO "%pd: ffa: Remaining unclaimed handles, retrying\n", ctx->teardown_d);
> + return false;
> + }
> + else
> + {
> + return true;
> + }
> +}
> +
> +static void ffa_teardown_timer_callback(void *arg)
> +{
> + struct ffa_ctx *ctx;
> + bool do_free;
> +
> + spin_lock(&ffa_teardown_lock);
> + ctx = list_first_entry_or_null(&ffa_teardown_head, struct ffa_ctx,
> + teardown_list);
> + spin_unlock(&ffa_teardown_lock);
> +
> if ( !ctx )
> - return 0;
> + {
> + printk(XENLOG_G_ERR "%s: teardown list is empty\n", __func__);
> + return;
> + }
>
> - for ( n = 0; n < subscr_vm_destroyed_count; n++ )
> + do_free = ffa_domain_teardown_continue(ctx);
> +
> + spin_lock(&ffa_teardown_lock);
> + list_del(&ctx->teardown_list);
> + if ( !do_free )
> + {
> + ctx->teardown_expire = NOW() + SECONDS(1);
> + list_add_tail(&ctx->teardown_list, &ffa_teardown_head);
> + }
> + spin_unlock(&ffa_teardown_lock);
> +
> + if ( do_free )
> {
> /*
> - * Skip SPs subscribed to the VM created event that never was
> - * notified of the VM creation due to an error during
> - * ffa_domain_init().
> + * domain_destroy() has likely been called (via put_domain() in
> + * reclaim_shms()) by now, so we can't touch the domain
> + * structure anymore.
> */
> - if ( is_in_subscr_list(subscr_vm_created, ctx->create_signal_count,
> - subscr_vm_created_count,
> - subscr_vm_destroyed[n]) )
> - continue;
> + xfree(ctx);
> + }
>
> - res = ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d),
> - FFA_MSG_SEND_VM_DESTROYED);
> + spin_lock(&ffa_teardown_lock);
> + ctx = list_first_entry_or_null(&ffa_teardown_head, struct ffa_ctx,
> + teardown_list);
Why not using list_empty here ?
> + spin_unlock(&ffa_teardown_lock);
>
> - if ( res )
> - printk(XENLOG_ERR "ffa: Failed to report destruction of vm_id %u to %u: res %d\n",
> - get_vm_id(d), subscr_vm_destroyed[n], res);
> - }
> + if ( ctx )
> + set_timer(&ffa_teardown_timer, ctx->teardown_expire);
I am a bit lost in the teardown_expire here.
Why not just always set the timer to NOW() + SECONDS(1) ?
Don't you have the risk of using an entry where the value was set previously and would now
be in the past ?
By the way could you create a proper define for the SECONDS(1) part so
that one wanting to do this more or less frequently would just have to change
one define value instead ?
> +}
> +
> +static void ffa_queue_ctx_teardown(struct ffa_ctx *ctx)
> +{
> + ctx->teardown_expire = NOW() + SECONDS(1);
> +
> + spin_lock(&ffa_teardown_lock);
> +
> + /*
> + * The timer is already active if there are queued teardown entries.
> + */
> + if ( list_empty(&ffa_teardown_head) )
> + set_timer(&ffa_teardown_timer, ctx->teardown_expire);
> +
> + list_add_tail(&ctx->teardown_list, &ffa_teardown_head);
> +
> + spin_unlock(&ffa_teardown_lock);
> +}
> +
> +/* This function is supposed to undo what ffa_domain_init() has done */
> +static int ffa_domain_teardown(struct domain *d)
> +{
> + struct ffa_ctx *ctx = d->arch.tee;
> +
> + if ( !ctx )
> + return 0;
>
> if ( ctx->rx )
> rxtx_unmap(ctx);
>
> + send_vm_destroyed(d);
> + reclaim_shms(d);
> +
> + if ( ctx->shm_count )
> + {
> + printk(XENLOG_G_INFO "%pd: ffa: Remaining unclaimed handles, retrying\n", d);
This block of code is the same as ffa_domain_teardown_continue so you could just call it and ..
> + ffa_queue_ctx_teardown(ctx);
call this if it returns false.
Overall i am a bit thinking that we could just have a generic function for one
context doing:
- try send vm_destroy
- try reclaim shms
- if succeed free tee
- if not put the context at the end of the teardown list and refire the timer if needed
It feels that we have a bit of code duplication here that might be possible to reduce a bit.
> + return 0;
> + }
> +
> XFREE(d->arch.tee);
>
> return 0;
> @@ -1733,6 +1908,9 @@ static bool ffa_probe(void)
> if ( !init_sps() )
> goto err_free_ffa_tx;
>
> + INIT_LIST_HEAD(&ffa_teardown_head);
> + init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
> +
> return true;
>
> err_free_ffa_tx:
> --
> 2.34.1
Cheers
Bertrand
Hi Bertrand,
On Mon, Jan 15, 2024 at 12:15 PM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Jens,
>
> Very sorry for the long delay.
>
> > On 13 Dec 2023, at 12:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > When an FF-A enabled guest is destroyed it may leave behind memory
> > shared with SPs. This memory must be reclaimed before it's reused or an
> > SP may make changes to memory used by a new unrelated guest. So when the
> > domain is teared down add FF-A requests to reclaim all remaining shared
> > memory.
> >
> > SPs in the secure world are notified using VM_DESTROYED that a guest has
> > been destroyed. An SP is supposed to relinquish all shared memory to allow
> > reclaiming the memory. The relinquish operation may need to be delayed if
> > the shared memory is for instance part of a DMA operation.
> >
> > The domain reference counter is increased when the first FF-A shared
> > memory is registered and the counter is decreased again when the last
> > shared memory is reclaimed. If FF-A shared memory registrations remain
> > at the end of of ffa_domain_teardown() a timer is set to try to reclaim
> > the shared memory every second until the memory is reclaimed.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> > Hi,
> >
> > This is a follow-up to the RFC patch. In this patch, I take an explicit
> > reference on the domain when FF-A shared memory is registered. I've
> > discovered that it might not be strictly necessary until all the shared
> > pages are released with put_page() they also keep a reference to the
> > domain.
> >
> > I'm not entirely sure what is the status of the shared memory when the
> > domain has turned into a zombie. We still hold references on the shared
> > pages until put_page() is called on each. Is that enough to guarantee that
> > they will not be reused?
> >
> > Thanks,
> > Jens
> >
> > v2:
> > - Update commit message to match the new implementation
> > - Using a per domain bitfield to keep track of which SPs has been notified
> > with VM_DESTROYED
> > - Holding a domain reference counter to keep the domain as a zombie domain
> > while there still is shared memory registrations remaining to be reclaimed
> > - Using a timer to retry reclaiming remaining shared memory registrations
> >
> > ---
> > xen/arch/arm/tee/ffa.c | 270 ++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 224 insertions(+), 46 deletions(-)
> >
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index 183528d13388..22906a6e1cff 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -54,6 +54,7 @@
> > #include <xen/mm.h>
> > #include <xen/sched.h>
> > #include <xen/sizes.h>
> > +#include <xen/timer.h>
> > #include <xen/types.h>
> >
> > #include <asm/event.h>
> > @@ -384,11 +385,6 @@ struct ffa_ctx {
> > unsigned int page_count;
> > /* FF-A version used by the guest */
> > uint32_t guest_vers;
> > - /*
> > - * Number of SPs that we have sent a VM created signal to, used in
> > - * ffa_domain_teardown() to know which SPs need to be signalled.
> > - */
> > - uint16_t create_signal_count;
> > bool rx_is_free;
> > /* Used shared memory objects, struct ffa_shm_mem */
> > struct list_head shm_list;
> > @@ -402,6 +398,15 @@ struct ffa_ctx {
> > spinlock_t tx_lock;
> > spinlock_t rx_lock;
> > spinlock_t lock;
> > + /* Used if domain can't be teared down immediately */
> > + struct domain *teardown_d;
> > + struct list_head teardown_list;
> > + s_time_t teardown_expire;
> > + /*
> > + * Used for ffa_domain_teardown() to keep track of which SPs should be
> > + * notified that this guest is being destroyed.
> > + */
> > + unsigned long vm_destroy_bitmap[];
> > };
> >
> > struct ffa_shm_mem {
> > @@ -436,6 +441,12 @@ static void *ffa_tx __read_mostly;
> > static DEFINE_SPINLOCK(ffa_rx_buffer_lock);
> > static DEFINE_SPINLOCK(ffa_tx_buffer_lock);
> >
> > +
> > +/* Used if domain can't be teared down immediately */
>
> Please adapt the comment as this for all domains.
> Suggest: Used to track domains that could not be teared down immediately.
I'll update.
>
> > +static struct timer ffa_teardown_timer;
> > +static struct list_head ffa_teardown_head;
> > +static DEFINE_SPINLOCK(ffa_teardown_lock);
> > +
> > static bool ffa_get_version(uint32_t *vers)
> > {
> > const struct arm_smccc_1_2_regs arg = {
> > @@ -850,7 +861,6 @@ static int32_t handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
> > goto out_rx_release;
> > }
> >
> > -
>
> You have several style changes like that in the patch.
> Those are not major so I am ok if you keep them in the patch but please
> mention in the commit messages that you do some code style fixes.
OK
>
> > memcpy(ctx->rx, ffa_rx, sz);
> > }
> > ctx->rx_is_free = false;
> > @@ -989,53 +999,75 @@ static void put_shm_pages(struct ffa_shm_mem *shm)
> > }
> > }
> >
> > -static bool inc_ctx_shm_count(struct ffa_ctx *ctx)
> > +static bool inc_ctx_shm_count(struct domain *d, struct ffa_ctx *ctx)
> > {
> > bool ret = true;
> >
> > spin_lock(&ctx->lock);
> > +
> > + /*
> > + * If this is the first shm added, increase the domain reference
> > + * counter as we may need to domain around a bit longer to reclaim the
>
> This sentence needs fixing (need to keep the domain..)
OK, I'll remove the word "may".
>
> > + * shared memory in the teardown path.
> > + */
> > + if ( !ctx->shm_count )
> > + get_knownalive_domain(d);
> > +
> > 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)
> > +static void dec_ctx_shm_count(struct domain *d, struct ffa_ctx *ctx)
> > {
> > spin_lock(&ctx->lock);
> > +
> > ASSERT(ctx->shm_count > 0);
> > ctx->shm_count--;
> > +
> > + /*
> > + * If this was the last shm removed, let go of the domain reference we
> > + * took in inc_ctx_shm_count() above.
> > + */
> > + if ( !ctx->shm_count )
> > + put_domain(d);
> > +
> > spin_unlock(&ctx->lock);
> > }
> >
> > -static struct ffa_shm_mem *alloc_ffa_shm_mem(struct ffa_ctx *ctx,
> > +static struct ffa_shm_mem *alloc_ffa_shm_mem(struct domain *d,
> > unsigned int page_count)
> > {
> > + struct ffa_ctx *ctx = d->arch.tee;
> > struct ffa_shm_mem *shm;
> >
> > if ( page_count >= FFA_MAX_SHM_PAGE_COUNT )
> > return NULL;
> > - if ( !inc_ctx_shm_count(ctx) )
> > + if ( !inc_ctx_shm_count(d, ctx) )
> > return NULL;
> >
> > shm = xzalloc_flex_struct(struct ffa_shm_mem, pages, page_count);
> > if ( shm )
> > shm->page_count = page_count;
> > else
> > - dec_ctx_shm_count(ctx);
> > + dec_ctx_shm_count(d, ctx);
> >
> > return shm;
> > }
> >
> > -static void free_ffa_shm_mem(struct ffa_ctx *ctx, struct ffa_shm_mem *shm)
> > +static void free_ffa_shm_mem(struct domain *d, struct ffa_shm_mem *shm)
> > {
> > + struct ffa_ctx *ctx = d->arch.tee;
> > +
> > if ( !shm )
> > return;
> >
> > - dec_ctx_shm_count(ctx);
> > + dec_ctx_shm_count(d, ctx);
> > put_shm_pages(shm);
> > xfree(shm);
> > }
> > @@ -1303,7 +1335,7 @@ static void handle_mem_share(struct cpu_user_regs *regs)
> > goto out_unlock;
> > }
> >
> > - shm = alloc_ffa_shm_mem(ctx, page_count);
> > + shm = alloc_ffa_shm_mem(d, page_count);
> > if ( !shm )
> > {
> > ret = FFA_RET_NO_MEMORY;
> > @@ -1347,7 +1379,7 @@ static void handle_mem_share(struct cpu_user_regs *regs)
> >
> > out:
> > if ( ret )
> > - free_ffa_shm_mem(ctx, shm);
> > + free_ffa_shm_mem(d, shm);
> > out_unlock:
> > spin_unlock(&ctx->tx_lock);
> >
> > @@ -1398,7 +1430,7 @@ static int handle_mem_reclaim(uint64_t handle, uint32_t flags)
> > }
> > else
> > {
> > - free_ffa_shm_mem(ctx, shm);
> > + free_ffa_shm_mem(d, shm);
> > }
> >
> > return ret;
> > @@ -1481,6 +1513,41 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> > }
> > }
> >
> > +static bool is_in_subscr_list(const uint16_t *subscr, uint16_t start,
> > + uint16_t end, uint16_t sp_id)
> > +{
> > + unsigned int n;
> > +
> > + for ( n = start; n < end; n++ )
> > + {
> > + if ( subscr[n] == sp_id )
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +static void vm_destroy_bitmap_init(struct ffa_ctx *ctx,
> > + unsigned int create_signal_count)
> > +{
> > + unsigned int n;
> > +
> > + for ( n = 0; n < subscr_vm_destroyed_count; n++ )
> > + {
> > + /*
> > + * Skip SPs subscribed to the VM created event that never was
> > + * notified of the VM creation due to an error during
> > + * ffa_domain_init().
> > + */
> > + if ( is_in_subscr_list(subscr_vm_created, create_signal_count,
> > + subscr_vm_created_count,
> > + subscr_vm_destroyed[n]) )
> > + continue;
> > +
> > + set_bit(n, ctx->vm_destroy_bitmap);
> > + }
> > +}
> > +
> > static int ffa_domain_init(struct domain *d)
> > {
> > struct ffa_ctx *ctx;
> > @@ -1496,11 +1563,14 @@ static int ffa_domain_init(struct domain *d)
> > if ( d->domain_id >= UINT16_MAX)
> > return -ERANGE;
> >
> > - ctx = xzalloc(struct ffa_ctx);
> > + ctx = xzalloc_flex_struct(struct ffa_ctx, vm_destroy_bitmap,
> > + BITS_TO_LONGS(subscr_vm_destroyed_count));
> > if ( !ctx )
> > return -ENOMEM;
> >
> > d->arch.tee = ctx;
> > + ctx->teardown_d = d;
> > + INIT_LIST_HEAD(&ctx->shm_list);
> >
> > for ( n = 0; n < subscr_vm_created_count; n++ )
> > {
> > @@ -1510,64 +1580,169 @@ static int ffa_domain_init(struct domain *d)
> > {
> > printk(XENLOG_ERR "ffa: Failed to report creation of vm_id %u to %u: res %d\n",
> > get_vm_id(d), subscr_vm_created[n], res);
> > - ctx->create_signal_count = n;
> > + vm_destroy_bitmap_init(ctx, n);
>
> Here you could just break and ..
>
> > return -EIO;
> > }
> > }
> > - ctx->create_signal_count = subscr_vm_created_count;
> > -
> > - INIT_LIST_HEAD(&ctx->shm_list);
> > + vm_destroy_bitmap_init(ctx, subscr_vm_created_count);
>
> call with n and return -EIO if n != vm_created_count.
OK
>
> >
> > return 0;
> > }
> >
> > -static bool is_in_subscr_list(const uint16_t *subscr, uint16_t start,
> > - uint16_t end, uint16_t sp_id)
> > +static void send_vm_destroyed(struct domain *d)
> > {
> > + struct ffa_ctx *ctx = d->arch.tee;
> > unsigned int n;
> > + int32_t res;
> >
> > - for ( n = start; n < end; n++ )
> > + for ( n = 0; n < subscr_vm_destroyed_count; n++ )
> > {
> > - if ( subscr[n] == sp_id )
> > - return true;
> > - }
> > + if ( !test_bit(n, ctx->vm_destroy_bitmap) )
> > + continue;
> >
> > - return false;
> > + res = ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d),
> > + FFA_MSG_SEND_VM_DESTROYED);
> > +
> > + if ( res )
> > + {
> > + printk(XENLOG_ERR "%pd: ffa: Failed to report destruction of vm_id %u to %u: res %d\n",
> > + d, get_vm_id(d), subscr_vm_destroyed[n], res);
> > + }
> > + else
> > + {
> > + clear_bit(n, ctx->vm_destroy_bitmap);
> > + }
> > + }
> > }
> >
> > -/* This function is supposed to undo what ffa_domain_init() has done */
> > -static int ffa_domain_teardown(struct domain *d)
> > +static void reclaim_shms(struct domain *d)
> > {
> > struct ffa_ctx *ctx = d->arch.tee;
> > - unsigned int n;
> > + struct ffa_shm_mem *shm, *tmp;
> > int32_t res;
> >
> > + list_for_each_entry_safe(shm, tmp, &ctx->shm_list, list)
> > + {
> > + register_t handle_hi;
> > + register_t handle_lo;
> > +
> > + uint64_to_regpair(&handle_hi, &handle_lo, shm->handle);
> > + res = ffa_mem_reclaim(handle_lo, handle_hi, 0);
> > + if ( res )
> > + {
> > + printk(XENLOG_G_INFO "%pd: ffa: Failed to reclaim handle %#lx : %d\n",
> > + d, shm->handle, res);
> > + }
> > + else
> > + {
> > + printk(XENLOG_G_DEBUG "%pd: ffa: Reclaimed handle %#lx\n",
> > + d, shm->handle);
> > + list_del(&shm->list);
> > + free_ffa_shm_mem(d, shm);
> > + }
> > + }
> > +}
> > +
> > +static bool ffa_domain_teardown_continue(struct ffa_ctx *ctx)
> > +{
> > + send_vm_destroyed(ctx->teardown_d);
> > + reclaim_shms(ctx->teardown_d);
> > +
> > + if ( ctx->shm_count )
> > + {
> > + printk(XENLOG_G_INFO "%pd: ffa: Remaining unclaimed handles, retrying\n", ctx->teardown_d);
> > + return false;
> > + }
> > + else
> > + {
> > + return true;
> > + }
> > +}
> > +
> > +static void ffa_teardown_timer_callback(void *arg)
> > +{
> > + struct ffa_ctx *ctx;
> > + bool do_free;
> > +
> > + spin_lock(&ffa_teardown_lock);
> > + ctx = list_first_entry_or_null(&ffa_teardown_head, struct ffa_ctx,
> > + teardown_list);
> > + spin_unlock(&ffa_teardown_lock);
> > +
> > if ( !ctx )
> > - return 0;
> > + {
> > + printk(XENLOG_G_ERR "%s: teardown list is empty\n", __func__);
> > + return;
> > + }
> >
> > - for ( n = 0; n < subscr_vm_destroyed_count; n++ )
> > + do_free = ffa_domain_teardown_continue(ctx);
> > +
> > + spin_lock(&ffa_teardown_lock);
> > + list_del(&ctx->teardown_list);
> > + if ( !do_free )
> > + {
> > + ctx->teardown_expire = NOW() + SECONDS(1);
> > + list_add_tail(&ctx->teardown_list, &ffa_teardown_head);
> > + }
> > + spin_unlock(&ffa_teardown_lock);
> > +
> > + if ( do_free )
> > {
> > /*
> > - * Skip SPs subscribed to the VM created event that never was
> > - * notified of the VM creation due to an error during
> > - * ffa_domain_init().
> > + * domain_destroy() has likely been called (via put_domain() in
> > + * reclaim_shms()) by now, so we can't touch the domain
> > + * structure anymore.
> > */
> > - if ( is_in_subscr_list(subscr_vm_created, ctx->create_signal_count,
> > - subscr_vm_created_count,
> > - subscr_vm_destroyed[n]) )
> > - continue;
> > + xfree(ctx);
> > + }
> >
> > - res = ffa_direct_req_send_vm(subscr_vm_destroyed[n], get_vm_id(d),
> > - FFA_MSG_SEND_VM_DESTROYED);
> > + spin_lock(&ffa_teardown_lock);
> > + ctx = list_first_entry_or_null(&ffa_teardown_head, struct ffa_ctx,
> > + teardown_list);
>
> Why not using list_empty here ?
Because we need the first ctx if there is any in the list. If there's
more than one, it's not the one we used above since that's last in the
list now.
>
> > + spin_unlock(&ffa_teardown_lock);
> >
> > - if ( res )
> > - printk(XENLOG_ERR "ffa: Failed to report destruction of vm_id %u to %u: res %d\n",
> > - get_vm_id(d), subscr_vm_destroyed[n], res);
> > - }
> > + if ( ctx )
> > + set_timer(&ffa_teardown_timer, ctx->teardown_expire);
>
> I am a bit lost in the teardown_expire here.
>
> Why not just always set the timer to NOW() + SECONDS(1) ?
teardown_expire is assigned when ctx is added to the list. The
elements in the list is ordered with the one to expire the soonest
first. So two ctx'es in the list are not necessarily torn down with
one seconds delay between them, it depends on when they were added to
the list.
> Don't you have the risk of using an entry where the value was set previously and would now
> be in the past ?
Yes, there's a risk of that. Is that a problem? The reason I'm doing
it one at a time is to take as few CPU cycles at a time and at the
same time try to tear down after the expected delay.
>
> By the way could you create a proper define for the SECONDS(1) part so
> that one wanting to do this more or less frequently would just have to change
> one define value instead ?
Sure, I'll fix.
>
> > +}
> > +
> > +static void ffa_queue_ctx_teardown(struct ffa_ctx *ctx)
> > +{
> > + ctx->teardown_expire = NOW() + SECONDS(1);
> > +
> > + spin_lock(&ffa_teardown_lock);
> > +
> > + /*
> > + * The timer is already active if there are queued teardown entries.
> > + */
> > + if ( list_empty(&ffa_teardown_head) )
> > + set_timer(&ffa_teardown_timer, ctx->teardown_expire);
> > +
> > + list_add_tail(&ctx->teardown_list, &ffa_teardown_head);
> > +
> > + spin_unlock(&ffa_teardown_lock);
> > +}
> > +
> > +/* This function is supposed to undo what ffa_domain_init() has done */
> > +static int ffa_domain_teardown(struct domain *d)
> > +{
> > + struct ffa_ctx *ctx = d->arch.tee;
> > +
> > + if ( !ctx )
> > + return 0;
> >
> > if ( ctx->rx )
> > rxtx_unmap(ctx);
> >
> > + send_vm_destroyed(d);
> > + reclaim_shms(d);
> > +
> > + if ( ctx->shm_count )
> > + {
> > + printk(XENLOG_G_INFO "%pd: ffa: Remaining unclaimed handles, retrying\n", d);
>
> This block of code is the same as ffa_domain_teardown_continue so you could just call it and ..
>
> > + ffa_queue_ctx_teardown(ctx);
>
> call this if it returns false.
>
> Overall i am a bit thinking that we could just have a generic function for one
> context doing:
> - try send vm_destroy
> - try reclaim shms
> - if succeed free tee
> - if not put the context at the end of the teardown list and refire the timer if needed
>
> It feels that we have a bit of code duplication here that might be possible to reduce a bit.
>
I'll try to reduce the code duplication.
Thanks,
Jens
>
> > + return 0;
> > + }
> > +
> > XFREE(d->arch.tee);
> >
> > return 0;
> > @@ -1733,6 +1908,9 @@ static bool ffa_probe(void)
> > if ( !init_sps() )
> > goto err_free_ffa_tx;
> >
> > + INIT_LIST_HEAD(&ffa_teardown_head);
> > + init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
> > +
> > return true;
> >
> > err_free_ffa_tx:
> > --
> > 2.34.1
>
> Cheers
> Bertrand
>
© 2016 - 2024 Red Hat, Inc.