Re: [XEN PATCH v2] xen/arm: ffa: reclaim shared memory on guest destroy

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

Very sorry for the long delay.

> On 13 Dec 2023, at 12:25, 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.
> 
> Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> ---
> 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