[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v3] xen/arm: ffa: reclaim shared memory on guest destroy
Hi Bertrand, On Thu, Feb 1, 2024 at 2:57 PM Bertrand Marquis <Bertrand.Marquis@xxxxxxx> wrote: > > Hi Jens, > > > On 17 Jan 2024, at 12:06, Jens Wiklander <jens.wiklander@xxxxxxxxxx> 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. > > > > A few minor style fixes with a removed empty line here and an added new > > line there. > > > > Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx> > > --- > > > > v3: > > - Mentioning in the commit message that there are some style fixes > > - Addressing review comments > > - Refactor the ffa_domain_teardown() path to let > > ffa_domain_teardown_continue() do most of the work. > > > > 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 | 253 +++++++++++++++++++++++++++++++++-------- > > 1 file changed, 204 insertions(+), 49 deletions(-) > > > > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > > index 0793c1c7585d..80ebbf4f01c6 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> > > @@ -144,6 +145,12 @@ > > */ > > #define FFA_MAX_SHM_COUNT 32 > > > > +/* > > + * The time we wait until trying to tear down a domain again if it was > > + * blocked initially. > > + */ > > +#define FFA_CTX_TEARDOWN_DELAY SECONDS(1) > > + > > /* FF-A-1.1-REL0 section 10.9.2 Memory region handle, page 167 */ > > #define FFA_HANDLE_HYP_FLAG BIT(63, ULL) > > #define FFA_HANDLE_INVALID 0xffffffffffffffffULL > > @@ -384,11 +391,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 +404,15 @@ struct ffa_ctx { > > spinlock_t tx_lock; > > spinlock_t rx_lock; > > spinlock_t lock; > > + /* Used if domain can't be torn 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 +447,12 @@ static void *ffa_tx __read_mostly; > > static DEFINE_SPINLOCK(ffa_rx_buffer_lock); > > static DEFINE_SPINLOCK(ffa_tx_buffer_lock); > > > > + > > +/* Used to track domains that could not be torn 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 = { > > @@ -853,7 +870,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; > > @@ -992,53 +1008,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 need to keep 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); > > } > > @@ -1306,7 +1344,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; > > @@ -1350,7 +1388,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); > > > > @@ -1401,7 +1439,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; > > @@ -1486,6 +1524,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; > > @@ -1501,11 +1574,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++ ) > > { > > @@ -1515,65 +1591,141 @@ 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; > > - return -EIO; > > + break; > > } > > } > > - ctx->create_signal_count = subscr_vm_created_count; > > - > > - INIT_LIST_HEAD(&ctx->shm_list); > > + vm_destroy_bitmap_init(ctx, n); > > + if ( n != subscr_vm_created_count ) > > + return -EIO; > > > > 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); > > + > > I think here we should dinstinguish cases depending on the error received: > - retry: we should keep the bit set and retry later > - other: unset the bit as there is not much we can do but log it. Makes sense. > > The interrupted case can happen, we handle that already in the direct message > request but we will get back here if that does not work after some retries so > putting it in the other category is ok i think. In the case of FFA_MSG_SEND_VM_DESTROYED is think it makes more sense to also retry FFA_RET_INTERRUPTED later. To me it seems that there will be an even better chance for an eventual non-secure interrupt to be handled if wait a little before retrying. So, if you don't mind, I'll skip clearing the bit below for both FFA_RET_INTERRUPTED and FFA_RET_RETRY. > > > + 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; > > > > - if ( !ctx ) > > - return 0; > > + list_for_each_entry_safe(shm, tmp, &ctx->shm_list, list) > > + { > > + register_t handle_hi; > > + register_t handle_lo; > > > > - for ( n = 0; n < subscr_vm_destroyed_count; n++ ) > > + uint64_to_regpair(&handle_hi, &handle_lo, shm->handle); > > + res = ffa_mem_reclaim(handle_lo, handle_hi, 0); > > Same here as for the VM_DESTROYED, there are some error code that we should > not reiterate on as they will also fail next time: > - invalid parameters: stop and log an error, this will always fail > - no memory: for now I would say do the same as invalid parameter with a > comment > maybe because we could retry and memory coud be available next time > - abort: stop and log an error, this is a major fault on the other side > - denied: retry. I agree, I'll propose something like that in the v4. > > > + 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 void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool > > first_time) > > +{ > > + struct ffa_ctx *next_ctx = NULL; > > + > > + send_vm_destroyed(ctx->teardown_d); > > + reclaim_shms(ctx->teardown_d); > > + > > + if ( ctx->shm_count ) > > + { > > If i do understand right you only retry if there are remaining SHM to reclaim > but what > if you have destroy messages that were not sent successfully, shouldn't we > reiterate > in that case to send the destroy message again ? I agree, I'll fix. Cheers, Jens > > > + printk(XENLOG_G_INFO "%pd: ffa: Remaining unclaimed handles, > > retrying\n", ctx->teardown_d); > > + > > + ctx->teardown_expire = NOW() + FFA_CTX_TEARDOWN_DELAY; > > + > > + spin_lock(&ffa_teardown_lock); > > + list_add_tail(&ctx->teardown_list, &ffa_teardown_head); > > + /* Need to set a new timer for the next ctx in line */ > > + next_ctx = list_first_entry(&ffa_teardown_head, struct ffa_ctx, > > + teardown_list); > > + spin_unlock(&ffa_teardown_lock); > > + } > > + else > > { > > /* > > - * 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() might have been called (via put_domain() in > > + * reclaim_shms()), 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); > > - > > - 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); > > + /* Only check if there has been a change to the teardown queue */ > > + if ( !first_time ) > > + { > > + spin_lock(&ffa_teardown_lock); > > + next_ctx = list_first_entry_or_null(&ffa_teardown_head, > > + struct ffa_ctx, > > teardown_list); > > + spin_unlock(&ffa_teardown_lock); > > + } > > } > > > > + if ( next_ctx ) > > + set_timer(&ffa_teardown_timer, next_ctx->teardown_expire); > > +} > > + > > +static void ffa_teardown_timer_callback(void *arg) > > +{ > > + struct ffa_ctx *ctx; > > + > > + spin_lock(&ffa_teardown_lock); > > + ctx = list_first_entry_or_null(&ffa_teardown_head, struct ffa_ctx, > > + teardown_list); > > + if ( ctx ) > > + list_del(&ctx->teardown_list); > > + spin_unlock(&ffa_teardown_lock); > > + > > + if ( ctx ) > > + ffa_domain_teardown_continue(ctx, false /* !first_time */); > > + else > > + printk(XENLOG_G_ERR "%s: teardown list is empty\n", __func__); > > +} > > + > > +/* 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); > > > > - XFREE(d->arch.tee); > > + ffa_domain_teardown_continue(ctx, true /* first_time */); > > > > return 0; > > } > > @@ -1739,6 +1891,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 > > > > Regards > Bertrand > >
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |