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 :-)
Cheers
Bertrand
>
> Cheers,
> Jens
>
>> +
>> + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
>> + {
>> + /* 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);
>> + }
>> + }
>> +
>> + goto out_continue;
>> + }
>> +
>> + /*
>> + * guest_vers 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 = FFA_MY_VERSION;
>> + goto out_handled;
>> }
>> - ffa_set_regs(regs, FFA_MY_VERSION, 0, 0, 0, 0, 0, 0, 0);
>> +
>> + /*
>> + * 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 = FFA_MY_VERSION;
>> + else
>> + ctx->guest_vers = 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 +323,17 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>> if ( !ctx )
>> return false;
>>
>> + /* A version must be negotiated first */
>> + if ( !ctx->guest_vers_negotiated )
>> + {
>> + 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 +427,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_negotiated = false;
>> + INIT_LIST_HEAD(&ctx->ctx_list);
>>
>> ctx->ffa_id = ffa_get_vm_id(d);
>> ctx->num_vcpus = d->max_vcpus;
>> @@ -452,7 +513,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) && ctx->guest_vers_negotiated )
>> {
>> 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..dec429cbf160 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 ( !dst_ctx->guest_vers_negotiated )
>> {
>> ret = FFA_RET_INVALID_PARAMETERS;
>> goto out_unlock;
>> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
>> index d7e6b6f5ef45..88b85c7c453a 100644
>> --- a/xen/arch/arm/tee/ffa_private.h
>> +++ b/xen/arch/arm/tee/ffa_private.h
>> @@ -354,12 +354,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 */
>> @@ -367,6 +361,21 @@ struct ffa_ctx {
>> /* Used shared memory objects, struct ffa_shm_mem */
>> struct list_head shm_list;
>>
>> + /*
>> + * FF-A version handling
>> + * guest_vers and guest_vers_negotiated are only written with
>> + * guest_vers_lock held. Reads do not take the lock, but ordering is
>> + * guaranteed because the writer updates guest_vers first and then
>> + * guest_vers_negotiated while holding the lock, ensuring any reader
>> + * that observes guest_vers_negotiated == true also sees the final
>> + * guest_vers value.
>> + * The ffa_ctx is added to the ctx_list only when a version
>> + * has been negotiated and locked.
>> + */
>> + spinlock_t guest_vers_lock;
>> + uint32_t guest_vers;
>> + bool guest_vers_negotiated;
>> +
>> /*
>> * Rx buffer, accessed with rx_lock locked.
>> * rx_is_free is used to serialize access.
>> --
>> 2.51.2
