Hi, On Wed, May 21, 2025 at 5:11 PM Bertrand Marquis <bertrand.marq...@arm.com> wrote: > > 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 ?
That works. It might be worth adding a comment that ctx->guest_vers is accessed unlocked elsewhere, and why that is OK. Cheers, Jens
