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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 8 Aug 2025, at 18:53, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On 17/07/2025 13:11, Bertrand Marquis 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 in parallel
>> by using rwlock which only ensure addition/removal of entries are
>> protected.
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> 
> With two remarks below:
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
>> ---
>> Changes in v7:
>> - protect ffa_ctx list with a rw lock to allow several partinfo_get in
>>   parallel but protect adding/removing entries.
>> Changes in v6:
>> - remove ACCESS_ONCE for guest_vers access and take the context lock
>>   before modifying it
>> - move guest_vers in context declaration to fields protected by the
>>   context lock and add a comment to state that lock in only needed when
>>   modifying it
>> 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 | 100 ++++++++++++++++++++++++---
>>  xen/arch/arm/tee/ffa_private.h  | 117 ++++++++++++++++++++++++++------
>>  4 files changed, 245 insertions(+), 30 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..be71eda4869f 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;
> 
> A more common pattern is to use LIST_HEAD(ffa_ctx_head) and ...
> 
>> +/* RW Lock to protect addition/removal and reading in ffa_ctx_head */
>> +rwlock_t ffa_ctx_list_rwlock;
> 
> ... DEFINE_RWLOCK(ffa_ctx_list_rwlock) which will also initialize list/rwlock 
> for you. So no need for extra code further down and less risk if the 
> variables are used before they are initialized.

I will modify those and push a v7 to the mailing list today.

Thanks again.

Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall