Hi Julien, > On 8 Aug 2025, at 18:53, Julien Grall <[email protected]> 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 <[email protected]> > > With two remarks below: > > Acked-by: Julien Grall <[email protected]> > >> --- >> 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
