Hi Jens,
> On 21 May 2025, at 16:54, Jens Wiklander <jens.wiklan...@linaro.org> wrote:
>
> Hi Bertrand,
>
> On Wed, Apr 16, 2025 at 9:40 AM Bertrand Marquis
> <bertrand.marq...@arm.com> 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.marq...@arm.com>
>> ---
>> 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
