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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

Thanks a lot for your review and comment, this is very helpful.

> On 4 Dec 2023, at 20:24, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Jens,
> 
> On 04/12/2023 07:55, Jens Wiklander 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.
>> If the FF-A memory reclaim request fails, return -ERESTART to retry
>> again. This will effectively block the destruction of the guest until
>> all memory has been reclaimed.
>> Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
>> ---
>> Hi,
>> This patch is a bit crude, but gets the job done. In a well designed
>> system this might even be good enough since the SP or the secure world
>> will let the memory be reclaimed and we can move on. But, if for some
>> reason reclaiming the memory is refused it must not be possible to reuse
>> the memory.
> 
> IIUC, we are trying to harden against a buggy SP. Is that correct?

This is not hardening as this is a possible scenario with a correctly 
implemented SP.
This is valid for the SP to not be able to relinquish the memory directly so we 
have
to take this possibility into account and retry.

What is not expected if for the SP to never release the memory hence the 
possible
"todo" at the end of the code where i think we might have to implement a counter
to bound the possible number of loops but as always the question is how many...

In this case the only solution would be to park the memory as suggested after
but we are not completely sure where hence the RFC.

> 
>> These shared memory ranges are typically quite small compared to the
>> total memory usage of a guest so it would be an improvement if only
>> refused shared memory ranges where set aside from future reuse while the
>> guest was destroyed and other resources made available for reuse. This
>> could be done by for instance assign the refused shared memory ranges
>> to a dummy VM like DOMID_IO.
> 
> I like the idea to use a dummy VM, but I don't think DOMID_IO is right. Once 
> teardown has completed, the domain will stay around until the last reference 
> on all pages are dropped. At this point, the amount of memory left-over is 
> minimum (this is mostly bookeeping in Xen).
> 
> From the userland PoV, the domain will still show-up in the list but tools 
> like "xl list" will show "(null)". They are called zombie domains.
> 
> So I would consider to keep the same domain around. The advantage is you can 
> call "xl destroy" again to retry the operation.

In this scenario the "restart" implementation here is right but how could we 
park the VM as "zombie" instead of busy looping in
the "kill" loop of userland ?

Also we need to release all the memory of the VM but the one shared with the 
SP. 

I will let Jens answer the more implementation questions here after and try to 
help on the more "system" ones.

> 
>> Thanks,
>> Jens
>> ---
>>  xen/arch/arm/tee/ffa.c | 36 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index 183528d13388..9c596462a8a2 100644
>> --- a/xen/arch/arm/tee/ffa.c
>> +++ b/xen/arch/arm/tee/ffa.c
>> @@ -1539,6 +1539,7 @@ static bool is_in_subscr_list(const uint16_t *subscr, 
>> uint16_t start,
>>  static int ffa_domain_teardown(struct domain *d)
>>  {
>>      struct ffa_ctx *ctx = d->arch.tee;
>> +    struct ffa_shm_mem *shm, *tmp;
>>      unsigned int n;
>>      int32_t res;
>>  @@ -1564,10 +1565,45 @@ static int ffa_domain_teardown(struct domain *d)
>>              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 this function is called again due to -ERESTART below, make sure
>> +     * not to send the FFA_MSG_SEND_VM_DESTROYED's.
>> +     */
>> +    subscr_vm_destroyed_count = 0;
> 
> AFAICT, this variable is global. So wouldn't you effectively break other 
> domain if let say the unmapping error is temporary?
> 
>>        if ( ctx->rx )
>>          rxtx_unmap(ctx);
>>  +
>> +    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);
> 
> Is this call expensive? If so, we may need to handle continuation here.

This call should not be expensive in the normal case as memory is reclaimable
so there is no processing required in the SP and all is done in the SPMC which
should basically just return a yes or no depending on a state for the handle.

So I think this is the best trade.

@Jens: One thing to consider is that a Destroy might get a retry or busy answer 
and we
will have to issue it again and this is not considered in the current 
implementation.

After discussing the subject internally we could in fact consider that if an SP 
cannot release
some memory shared with the VM destroyed, it should tell it by returning 
"retry" to the message.
Here that could simplify things by doing a strategy where:
- we retry on the VM_DESTROY message if required
- if some memory is not reclaimable we check if we could park it and make the 
VM a zombie.
What do you think ?


> 
>> +        if ( res )
>> +        {
>> +            printk(XENLOG_INFO, "ffa: Failed to reclaim handle %#lx : %d\n",
>> +                   shm->handle, res);
> 
> I think you want to use XENLOG_G_INFO to use the guest ratelimit. Also, I 
> would suggest to print the domain ID in the logs (see '%pd').
> 
> 
>> +        }
>> +        else
>> +        {
>> +            printk(XENLOG_DEBUG, "ffa: Reclaimed handle %#lx\n", 
>> shm->handle);
> 
> Same here. You want to use XENLOG_G_DEBUG and print the domain ID.
> 
>> +            ctx->shm_count--;
>> +            list_del(&shm->list);
>> +        }
>> +    }
> 
> NIT: New line here please for clarity.
> 
>> +    if ( !list_empty(&ctx->shm_list) )
>> +    {
>> +        printk(XENLOG_INFO, "ffa: Remaining unclaimed handles, retrying\n");
> 
> Same as the other printks.
> 
>> +        /*
>> +         * TODO: add a timeout where we either panic or let the guest be
>> +         * fully destroyed.
>> +         */
> Timeout with proper handling would be a solution. I am not sure about 
> panic-ing. Do you think the TEE would be in a bad state if we can't release 
> memory?
> 
>> +        return -ERESTART;
>> +    }
>> +
>>      XFREE(d->arch.tee);
>>        return 0;
> 
> Cheers,
> 

Cheers
Bertrand

> -- 
> Julien Grall