Re: [PATCH v5 3/6] xen/arm: ffa: Introduce VM to VM support

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 21 May 2025, at 18:34, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi,
> 
> On Wed, May 21, 2025 at 5:11 PM Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx> wrote:
>> 
>> Hi Jens,
>> 
>>> On 21 May 2025, at 16:54, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On Wed, Apr 16, 2025 at 9:40 AM Bertrand Marquis
>>> <bertrand.marquis@xxxxxxx> wrote:
>>>> 
>>>> Create a CONFIG_FFA_VM_TO_VM parameter to activate FFA communication
>>>> between VMs.
>>>> When activated list VMs in the system with FF-A support in part_info_get.
>>>> 
>>>> When VM to VM is activated, Xen will be tainted as Insecure and a
>>>> message is displayed to the user during the boot as there is no
>>>> filtering of VMs in FF-A so any VM can communicate or see any other VM
>>>> in the system.
>>>> 
>>>> WARNING: There is no filtering for now and all VMs are listed !!
>>>> 
>>>> This patch is reorganizing the ffa_ctx structure to make clear which
>>>> lock is protecting what parts.
>>>> 
>>>> This patch is introducing a chain list of the ffa_ctx with a FFA Version
>>>> negociated allowing to create the partinfo results for VMs without
>>>> taking a lock on the global domain list in Xen.
>>>> 
>>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>>>> ---
>>>> Changes in v5:
>>>> - remove invalid comment about 1.1 firmware support
>>>> - rename variables from d and dom to curr_d and dest_d (Julien)
>>>> - add a TODO in the code for potential holding for long of the CPU
>>>> (Julien)
>>>> - use an atomic global variable to store the number of VMs instead of
>>>> recomputing the value each time (Julien)
>>>> - add partinfo information in ffa_ctx (id, cpus and 64bit) and create a
>>>> chain list of ctx. Use this chain list to create the partinfo result
>>>> without holding a global lock to prevent concurrency issues.
>>>> - Move some changes in a preparation patch modifying partinfo for sps to
>>>> reduce this patch size and make the review easier
>>>> Changes in v4:
>>>> - properly handle SPMC version 1.0 header size case in partinfo_get
>>>> - switch to local counting variables instead of *pointer += 1 form
>>>> - coding style issue with missing spaces in if ()
>>>> Changes in v3:
>>>> - break partinfo_get in several sub functions to make the implementation
>>>> easier to understand and lock handling easier
>>>> - rework implementation to check size along the way and prevent previous
>>>> implementation limits which had to check that the number of VMs or SPs
>>>> did not change
>>>> - taint Xen as INSECURE when VM to VM is enabled
>>>> Changes in v2:
>>>> - Switch ifdef to IS_ENABLED
>>>> - dom was not switched to d as requested by Jan because there is already
>>>> a variable d pointing to the current domain and it must not be
>>>> shadowed.
>>>> ---
>>>> xen/arch/arm/tee/Kconfig        |  11 ++++
>>>> xen/arch/arm/tee/ffa.c          |  47 +++++++++++++-
>>>> xen/arch/arm/tee/ffa_partinfo.c |  95 ++++++++++++++++++++++++---
>>>> xen/arch/arm/tee/ffa_private.h  | 112 ++++++++++++++++++++++++++------
>>>> 4 files changed, 233 insertions(+), 32 deletions(-)
>>>> 
>>>> diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig
>>>> index c5b0f88d7522..88a4c4c99154 100644
>>>> --- a/xen/arch/arm/tee/Kconfig
>>>> +++ b/xen/arch/arm/tee/Kconfig
>>>> @@ -28,5 +28,16 @@ config FFA
>>>> 
>>>>         [1] https://developer.arm.com/documentation/den0077/latest
>>>> 
>>>> +config FFA_VM_TO_VM
>>>> +    bool "Enable FF-A between VMs (UNSUPPORTED)" if UNSUPPORTED
>>>> +    default n
>>>> +    depends on FFA
>>>> +    help
>>>> +      This option enables to use FF-A between VMs.
>>>> +      This is experimental and there is no access control so any
>>>> +      guest can communicate with any other guest.
>>>> +
>>>> +      If unsure, say N.
>>>> +
>>>> endmenu
>>>> 
>>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>>>> index 3bbdd7168a6b..c1c4c0957091 100644
>>>> --- a/xen/arch/arm/tee/ffa.c
>>>> +++ b/xen/arch/arm/tee/ffa.c
>>>> @@ -118,6 +118,13 @@ void *ffa_tx __read_mostly;
>>>> DEFINE_SPINLOCK(ffa_rx_buffer_lock);
>>>> DEFINE_SPINLOCK(ffa_tx_buffer_lock);
>>>> 
>>>> +struct list_head ffa_ctx_head;
>>>> +/* Lock to protect addition/removal in ffa_ctx_head */
>>>> +DEFINE_SPINLOCK(ffa_ctx_list_lock);
>>>> +
>>>> +#ifdef CONFIG_FFA_VM_TO_VM
>>>> +atomic_t ffa_vm_count;
>>>> +#endif
>>>> 
>>>> /* Used to track domains that could not be torn down immediately. */
>>>> static struct timer ffa_teardown_timer;
>>>> @@ -160,10 +167,21 @@ static void handle_version(struct cpu_user_regs 
>>>> *regs)
>>>>     */
>>>>    if ( FFA_VERSION_MAJOR(vers) == FFA_MY_VERSION_MAJOR )
>>>>    {
>>>> +        uint32_t old_vers = ACCESS_ONCE(ctx->guest_vers);
>>>> +
>>>>        if ( FFA_VERSION_MINOR(vers) > FFA_MY_VERSION_MINOR )
>>>> -            ctx->guest_vers = FFA_MY_VERSION;
>>>> +            ACCESS_ONCE(ctx->guest_vers) = FFA_MY_VERSION;
>>>>        else
>>>> -            ctx->guest_vers = vers;
>>>> +            ACCESS_ONCE(ctx->guest_vers) = vers;
>>> 
>>> What is the ACCESS_ONCE() scheme intended for?
>>> 
>>>> +
>>>> +        if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !old_vers )
>>> 
>>> If handle_version() is called concurrently on two CPUs, it might be
>>> possible for a context to be added twice.
>>> How about a separate flag to indicate whether a context has been added
>>> to the list?
>> 
>> I wanted to avoid having guest_vers as atomic or introduce an other lock.
>> But i think now that this is kind of impossible to avoid and this way to do
>> it is wrong.
>> 
>> I will take the context lock to do this processing to avoid this corner case
>> and remove the ACCESS_ONCE to protect modifications to guest_vers:
>> 
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index b86c88cefa8c..306edb97863a 100644
>> --- a/xen/arch/arm/tee/ffa.c
>> +++ b/xen/arch/arm/tee/ffa.c
>> @@ -158,6 +158,7 @@ static void handle_version(struct cpu_user_regs *regs)
>>     struct domain *d = current->domain;
>>     struct ffa_ctx *ctx = d->arch.tee;
>>     uint32_t vers = get_user_reg(regs, 1);
>> +    uint32_t old_vers;
>> 
>>     /*
>>      * Guest will use the version it requested if it is our major and minor
>> @@ -167,12 +168,14 @@ static void handle_version(struct cpu_user_regs *regs)
>>      */
>>     if ( FFA_VERSION_MAJOR(vers) == FFA_MY_VERSION_MAJOR )
>>     {
>> -        uint32_t old_vers = ACCESS_ONCE(ctx->guest_vers);
>> +        spin_lock(&ctx->lock);
>> +        old_vers = ctx->guest_vers;
>> 
>>         if ( FFA_VERSION_MINOR(vers) > FFA_MY_VERSION_MINOR )
>> -            ACCESS_ONCE(ctx->guest_vers) = FFA_MY_VERSION;
>> +           ctx->guest_vers = FFA_MY_VERSION;
>>         else
>> -            ACCESS_ONCE(ctx->guest_vers) = vers;
>> +           ctx->guest_vers = vers;
>> +        spin_unlock(&ctx->lock);
>> 
>>         if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !old_vers )
>>         {
>> 
>> What do you think ?
> 
> That works. It might be worth adding a comment that ctx->guest_vers is
> accessed unlocked elsewhere, and why that is OK.

In fact there is an other ACCESS_ONCE on guest_vers that i will remove
as it in teardown and there is no need for it.

I will add a comment in the ffa_private.h to state that modification are 
protected
by the context lock.

This code will be reworked in the next serie with v1.2 support as the guidance
became clearer in the v1.2 spec and this will remove some issues.

Cheers
Bertrand


> 
> Cheers,
> Jens