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

[Julien Grall <julien@xxxxxxx>]

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.
Cheers,

--
Julien Grall