Hi Jens, > On 9 Dec 2025, at 09:45, Jens Wiklander <[email protected]> wrote: > > Hi Bertrand, > > On Fri, Dec 5, 2025 at 11:37 AM 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_tmp: >> - guest_vers is the version negotiated or 0 if no version has been >> negotiated. This must be used with ACCESS_ONCE when reading it without >> the spinlock taken. >> - guest_vers_tmp stores the version currently requested by a VM. >> >> The version requested is the one actually negotiated once a call >> different from FFA_VERSION is done to allow several attempts and as >> requested by FF-A specification. >> We always return our implementation version FFA_MY_VERSION, even if the >> version requested was different, as requested by FF-A specification. >> >> Any call other than FFA_VERSION is rejected until a version has been >> requested. >> >> Update all places in the code where guest_vers is used to use >> ACCESS_ONCE. >> >> 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]> >> --- >> Changes in v1: >> - remove the guest_vers_negotiated and use guest_vers = 0 as condition >> for a version being negotiated instead >> - introduce guest_vers_tmp to store a requested version until it is >> becoming the one negotiated. >> - remove not needed if negotiated condition. >> - use ACCESS_ONCE when reading guest_vers and use guest_vers == 0 as >> condition for a version being negotiated. >> - Update FF-A version handling comment in ffa_private.h >> --- >> xen/arch/arm/tee/ffa.c | 101 +++++++++++++++++++++++++------- >> xen/arch/arm/tee/ffa_msg.c | 2 +- >> xen/arch/arm/tee/ffa_partinfo.c | 4 +- >> xen/arch/arm/tee/ffa_private.h | 26 ++++++-- >> xen/arch/arm/tee/ffa_shm.c | 3 +- >> 5 files changed, 105 insertions(+), 31 deletions(-) >> >> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c >> index 2b4e24750d52..aadd6c21e7f2 100644 >> --- a/xen/arch/arm/tee/ffa.c >> +++ b/xen/arch/arm/tee/ffa.c >> @@ -158,31 +158,38 @@ 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->guest_vers_lock); >> + >> + /* If negotiation already published, continue without handling. */ >> + if ( ACCESS_ONCE(ctx->guest_vers) ) >> + goto out_continue; >> + >> + if ( fid != FFA_VERSION ) >> { >> - spin_lock(&ctx->lock); >> - old_vers = ctx->guest_vers; >> + if ( !ctx->guest_vers_tmp ) >> + { >> + out_vers = 0; >> + goto out_handled; >> + } >> >> - if ( FFA_VERSION_MINOR(vers) > FFA_MY_VERSION_MINOR ) >> - ctx->guest_vers = FFA_MY_VERSION; >> - else >> - ctx->guest_vers = vers; >> - spin_unlock(&ctx->lock); >> + /* >> + * 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. >> + * Finalize negotiation: publish guest_vers once, then never change. >> + */ >> + ACCESS_ONCE(ctx->guest_vers) = ctx->guest_vers_tmp; >> >> - if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !old_vers ) >> + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) ) >> { >> /* One more VM with FF-A support available */ >> inc_ffa_vm_count(); >> @@ -190,8 +197,48 @@ static void handle_version(struct cpu_user_regs *regs) >> list_add_tail(&ctx->ctx_list, &ffa_ctx_head); >> write_unlock(&ffa_ctx_list_rwlock); >> } >> + >> + goto out_continue; >> } >> - ffa_set_regs(regs, FFA_MY_VERSION, 0, 0, 0, 0, 0, 0, 0); >> + >> + /* >> + * guest_vers_tmp stores the version selected by the guest (lower minor >> may >> + * require reduced data structures). However, the value returned to the >> + * guest via FFA_VERSION is always FFA_MY_VERSION, the implementation >> + * version, as required by FF-A. The two values intentionally differ. >> + */ >> + >> + /* >> + * Return our highest implementation version on request different than >> our >> + * major and mark negotiated version as our implementation version. >> + */ >> + if ( FFA_VERSION_MAJOR(in_vers) != FFA_MY_VERSION_MAJOR ) >> + { >> + ctx->guest_vers_tmp = FFA_MY_VERSION; >> + goto out_handled; >> + } >> + >> + /* >> + * Use our minor version if a greater minor was requested or the >> requested >> + * minor if it is lower than ours was requested. >> + */ >> + if ( FFA_VERSION_MINOR(in_vers) > FFA_MY_VERSION_MINOR ) >> + ctx->guest_vers_tmp = FFA_MY_VERSION; >> + else >> + ctx->guest_vers_tmp = in_vers; >> + >> +out_handled: >> + spin_unlock(&ctx->guest_vers_lock); >> + if ( out_vers ) >> + ffa_set_regs(regs, out_vers, 0, 0, 0, 0, 0, 0, 0); >> + else >> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED); >> + return true; >> + >> +out_continue: >> + spin_unlock(&ctx->guest_vers_lock); >> + >> + return false; >> } >> >> static void handle_features(struct cpu_user_regs *regs) >> @@ -274,10 +321,17 @@ static bool ffa_handle_call(struct cpu_user_regs *regs) >> if ( !ctx ) >> return false; >> >> + /* A version must be negotiated first */ >> + if ( !ACCESS_ONCE(ctx->guest_vers) ) >> + { >> + if ( ffa_negotiate_version(regs) ) >> + return true; >> + } >> + >> switch ( fid ) >> { >> case FFA_VERSION: >> - handle_version(regs); >> + ffa_set_regs(regs, FFA_MY_VERSION, 0, 0, 0, 0, 0, 0, 0); >> return true; >> case FFA_ID_GET: >> ffa_set_regs_success(regs, ffa_get_vm_id(d), 0); >> @@ -371,6 +425,11 @@ static int ffa_domain_init(struct domain *d) >> d->arch.tee = ctx; >> ctx->teardown_d = d; >> INIT_LIST_HEAD(&ctx->shm_list); >> + spin_lock_init(&ctx->lock); >> + spin_lock_init(&ctx->guest_vers_lock); >> + ctx->guest_vers = 0; >> + ctx->guest_vers_tmp = 0; >> + INIT_LIST_HEAD(&ctx->ctx_list); >> >> ctx->ffa_id = ffa_get_vm_id(d); >> ctx->num_vcpus = d->max_vcpus; >> @@ -452,7 +511,7 @@ static int ffa_domain_teardown(struct domain *d) >> if ( !ctx ) >> return 0; >> >> - if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) && ctx->guest_vers ) >> + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) && ACCESS_ONCE(ctx->guest_vers) ) >> { >> dec_ffa_vm_count(); >> write_lock(&ffa_ctx_list_rwlock); >> diff --git a/xen/arch/arm/tee/ffa_msg.c b/xen/arch/arm/tee/ffa_msg.c >> index c20c5bec0f76..2c2ebc9c5cd6 100644 >> --- a/xen/arch/arm/tee/ffa_msg.c >> +++ b/xen/arch/arm/tee/ffa_msg.c >> @@ -113,7 +113,7 @@ static int32_t ffa_msg_send2_vm(uint16_t dst_id, const >> void *src_buf, >> } >> >> dst_ctx = dst_d->arch.tee; >> - if ( !dst_ctx->guest_vers ) >> + if ( !ACCESS_ONCE(dst_ctx->guest_vers) ) >> { >> ret = FFA_RET_INVALID_PARAMETERS; >> goto out_unlock; >> diff --git a/xen/arch/arm/tee/ffa_partinfo.c >> b/xen/arch/arm/tee/ffa_partinfo.c >> index fa56b1587e3b..ec5a53ed1cab 100644 >> --- a/xen/arch/arm/tee/ffa_partinfo.c >> +++ b/xen/arch/arm/tee/ffa_partinfo.c >> @@ -238,7 +238,7 @@ void ffa_handle_partition_info_get(struct cpu_user_regs >> *regs) >> * use the v1.0 structure size in the destination buffer. >> * Otherwise use the size of the highest version we support, here 1.1. >> */ >> - if ( ctx->guest_vers == FFA_VERSION_1_0 ) >> + if ( ACCESS_ONCE(ctx->guest_vers) == FFA_VERSION_1_0 ) >> dst_size = sizeof(struct ffa_partition_info_1_0); >> else >> dst_size = sizeof(struct ffa_partition_info_1_1); >> @@ -250,7 +250,7 @@ void ffa_handle_partition_info_get(struct cpu_user_regs >> *regs) >> * FF-A v1.0 has w5 MBZ while v1.1 allows >> * FFA_PARTITION_INFO_GET_COUNT_FLAG to be non-zero. >> */ >> - if ( ctx->guest_vers == FFA_VERSION_1_0 || >> + if ( ACCESS_ONCE(ctx->guest_vers) == FFA_VERSION_1_0 || >> flags != FFA_PARTITION_INFO_GET_COUNT_FLAG ) >> { >> ret = FFA_RET_INVALID_PARAMETERS; >> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h >> index 8d01da0009d3..4e4ac7fd7bc4 100644 >> --- a/xen/arch/arm/tee/ffa_private.h >> +++ b/xen/arch/arm/tee/ffa_private.h >> @@ -355,12 +355,6 @@ struct ffa_ctx { >> * Global data accessed with lock locked. >> */ >> spinlock_t lock; >> - /* >> - * FF-A version negotiated by the guest, only modifications to >> - * this field are done with the lock held as this is expected to >> - * be done once at init by a guest. >> - */ >> - uint32_t guest_vers; >> /* Number of 4kB pages in each of rx/rx_pg and tx/tx_pg */ >> unsigned int page_count; >> /* Number of allocated shared memory object */ >> @@ -368,6 +362,26 @@ struct ffa_ctx { >> /* Used shared memory objects, struct ffa_shm_mem */ >> struct list_head shm_list; >> >> + /* >> + * FF-A version handling >> + * guest_vers is the single published negotiated version. It is 0 until >> + * negotiation completes, after which it is set once and never changes. >> + * Negotiation uses guest_vers_tmp under guest_vers_lock; when a >> + * non-VERSION ABI is invoked, Xen finalizes negotiation by publishing >> + * guest_vers using ACCESS_ONCE() store. >> + * Readers use ACCESS_ONCE(guest_vers) != 0 to detect availability and >> + * can consume guest_vers without barriers because it never changes once >> + * published. >> + */ >> + spinlock_t guest_vers_lock; >> + /* >> + * Published negotiated version. Zero means "not negotiated yet". >> + * Once non-zero, it never changes. >> + */ >> + uint32_t guest_vers; > > It might be worth mentioning that this field must always be accessed > using the ACCESS_ONCE() macro.
yes you are right, the overall comment is only mentioning how to check using ACCESS_ONCE if version is negotiated but is not very clear. I will add a comment on top of guest_vers: guest_vers must always be accessed using ACCESS_ONCE. Cheers Bertrand > > Cheers, > Jens > >> + /* Temporary version used during negotiation under guest_vers_lock */ >> + uint32_t guest_vers_tmp; >> + >> /* >> * Rx buffer, accessed with rx_lock locked. >> * rx_is_free is used to serialize access. >> diff --git a/xen/arch/arm/tee/ffa_shm.c b/xen/arch/arm/tee/ffa_shm.c >> index d628c1b70609..dad3da192247 100644 >> --- a/xen/arch/arm/tee/ffa_shm.c >> +++ b/xen/arch/arm/tee/ffa_shm.c >> @@ -495,7 +495,8 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs) >> if ( frag_len > ctx->page_count * FFA_PAGE_SIZE ) >> goto out_unlock; >> >> - ret = read_mem_transaction(ctx->guest_vers, ctx->tx, frag_len, &trans); >> + ret = read_mem_transaction(ACCESS_ONCE(ctx->guest_vers), ctx->tx, >> + frag_len, &trans); >> if ( ret ) >> goto out_unlock; >> >> -- >> 2.51.2 >>
