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 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 ?

Cheers
Bertrand