Hi, On Tue, Dec 2, 2025 at 5:26 PM Bertrand Marquis <[email protected]> wrote: > > Hi Jens, > > > On 2 Dec 2025, at 12:33, Jens Wiklander <[email protected]> wrote: > > > > Hi Bertrand, > > > > > > On Thu, Nov 27, 2025 at 4:52 PM Bertrand Marquis > > <[email protected]> wrote: > >> > >> Track FF-A version negotiation per VM and enforce that no FF-A ABI > >> (other than FFA_VERSION) is processed before a guest has selected a > >> version. > >> > >> Each ffa_ctx gains a dedicated guest_vers_lock, a negotiated version > >> (guest_vers) and a guest_vers_negotiated flag. guest_vers records the > >> version requested by the guest so the mediator can provide data > >> structures compatible with older minor versions. The value returned to > >> the guest by FFA_VERSION is always FFA_MY_VERSION, the implementation > >> version, as required by FF-A. > >> > >> FFA_VERSION may be issued multiple times. Negotiation becomes final > >> only when a non-FFA_VERSION ABI is invoked, in accordance with the > >> FF-A requirement that the version cannot change once any other ABI has > >> been used. Before this point, non-FFA_VERSION ABIs are rejected if no > >> valid version has been provided. > >> > >> Once negotiation completes, the context is added to the global FF-A > >> VM list (when VM-to-VM is enabled) and the version may not be modified > >> for the lifetime of the VM. All VM-to-VM paths and teardown logic are > >> updated to use the guest_vers_negotiated flag. > >> > >> This prevents partially initialised contexts from using the mediator > >> and complies with the FF-A 1.2 FFA_VERSION semantics. > >> > >> Signed-off-by: Bertrand Marquis <[email protected]> > >> --- > >> xen/arch/arm/tee/ffa.c | 115 +++++++++++++++++++++++++-------- > >> xen/arch/arm/tee/ffa_msg.c | 2 +- > >> xen/arch/arm/tee/ffa_private.h | 21 ++++-- > >> 3 files changed, 104 insertions(+), 34 deletions(-) > >> > >> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c > >> index 2b4e24750d52..3309ca875ec4 100644 > >> --- a/xen/arch/arm/tee/ffa.c > >> +++ b/xen/arch/arm/tee/ffa.c > >> @@ -158,40 +158,89 @@ static bool ffa_abi_supported(uint32_t id) > >> return !ffa_simple_call(FFA_FEATURES, id, 0, 0, 0); > >> } > >> > >> -static void handle_version(struct cpu_user_regs *regs) > >> +static bool ffa_negotiate_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; > >> + uint32_t fid = get_user_reg(regs, 0); > >> + uint32_t in_vers = get_user_reg(regs, 1); > >> + uint32_t out_vers = FFA_MY_VERSION; > >> > >> - /* > >> - * Guest will use the version it requested if it is our major and > >> minor > >> - * lower or equals to ours. If the minor is greater, our version will > >> be > >> - * used. > >> - * In any case return our version to the caller. > >> - */ > >> - if ( FFA_VERSION_MAJOR(vers) == FFA_MY_VERSION_MAJOR ) > >> - { > >> - spin_lock(&ctx->lock); > >> - old_vers = ctx->guest_vers; > >> + spin_lock(&ctx->guest_vers_lock); > >> > >> - if ( FFA_VERSION_MINOR(vers) > FFA_MY_VERSION_MINOR ) > >> - ctx->guest_vers = FFA_MY_VERSION; > >> - else > >> - ctx->guest_vers = vers; > >> - spin_unlock(&ctx->lock); > >> + /* Handle FFA_VERSION races from different vCPUs. */ > >> + if ( ctx->guest_vers_negotiated ) > >> + goto out_continue; > >> > >> - if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !old_vers ) > >> + if ( fid != FFA_VERSION ) > >> + { > >> + if ( !ctx->guest_vers ) > >> { > >> - /* One more VM with FF-A support available */ > >> - inc_ffa_vm_count(); > >> - write_lock(&ffa_ctx_list_rwlock); > >> - list_add_tail(&ctx->ctx_list, &ffa_ctx_head); > >> - write_unlock(&ffa_ctx_list_rwlock); > >> + out_vers = 0; > >> + goto out_handled; > >> } > >> + > >> + /* > >> + * A successful FFA_VERSION call does not freeze negotiation. > >> Guests > >> + * are allowed to issue multiple FFA_VERSION attempts (e.g. > >> probing > >> + * several minor versions). Negotiation becomes final only when a > >> + * non-VERSION ABI is invoked, as required by the FF-A > >> specification. > >> + */ > >> + if ( !ctx->guest_vers_negotiated ) > > > > ctx->guest_vers_negotiated is always false here, due to the check above. > > Absolutely, I will remove the if here so that we set version to negotiated on > the first pass and do not come back here after. > > > > >> + { > >> + ctx->guest_vers_negotiated = true; > > > > I'm on thin ice here, but I think that barriers or some other > > primitives are needed to close the gap if ffa_handle_call() is called > > concurrently during these conditions: > > ctx->guest_vers_negotiated == false > > CPU0 called with FFA_VERSION 1.1 -> sets ctx->guest_vers = 1.1 > > CPU1 called with a valid FF-A ID != FFA_VERSION -> sets > > ctx->guest_vers_negotiated = true > > CPU2 called with a valid FF-A ID != FFA_VERSION -> guarantee is > > missing that CPU2 will observe the updated ctx->guest_vers if it > > observes the updated ctx->guest_vers_negotiated > > Definitely you are right and the combination of guest_vers and > guest_vers_negotiated has an issue with ordering. > > I think the following modification should solve this: > - remove guest_vers_negotiated and use guest_vers = 0 as test for > version negotiated used with ACCESS_ONCE > - introduced a guest_vers_tmp only accessed under the lock to store > the temporary agreed version until negotiation is done > - during negotiation done copy tmp into guest_vers with a previous > write barrier before and ACCESS_ONCE to ensure visibility > > Tell if that sounds right :-)
That should work. I think we may be able to skip an explicit barrier or even ACCESS_ONCE since the other CPUs will either see the updated version or enter ffa_negotiate_version() to take the lock (which includes a barrier) for synchronized access. Cheers, Jens
