Re: [PATCH v1 02/12] xen/arm: ffa: per-VM FFA_VERSION negotiation state

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 9 Dec 2025, at 09:45, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On Fri, Dec 5, 2025 at 11:37 AM Bertrand Marquis
> <bertrand.marquis@xxxxxxx> 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 <bertrand.marquis@xxxxxxx>
>> ---
>> 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
>>