Re: [PATCH 4/4] xen/arm: ffa: Add cached GET_REGS support

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Mon, Mar 2, 2026 at 9:58 AM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> > On 27 Feb 2026, at 15:56, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > Hi Bertrand,
> >
> > On Wed, Feb 25, 2026 at 11:02 AM Bertrand Marquis
> > <bertrand.marquis@xxxxxxx> wrote:
> >>
> >> FF-A v1.2 defines PARTITION_INFO_GET_REGS for register-based partition
> >> info retrieval, but Xen currently only supports the buffer-based GET
> >> path for guests.
> >>
> >> Implement GET_REGS using the cached SP list and VM entries, including
> >> the register window layout and input validation. Track VM list changes
> >> via the partinfo tag and use it to validate GET_REGS tag inputs. Ensure
> >> that when a non-Nil UUID is specified, the UUID fields in both GET and
> >> GET_REGS results are MBZ as required by the specification.
> >>
> >> PARTITION_INFO_GET_REGS is available to v1.2 guests, returning cached SP
> >> entries and VM entries with UUIDs zeroed for non-Nil UUID queries.
> >>
> >> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> >> ---
> >> xen/arch/arm/tee/ffa.c          |  16 +++
> >> xen/arch/arm/tee/ffa_partinfo.c | 211 ++++++++++++++++++++++++++++++++
> >> xen/arch/arm/tee/ffa_private.h  |   4 +-
> >> 3 files changed, 230 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> >> index aa43ae2595d7..d56eb20c2239 100644
> >> --- a/xen/arch/arm/tee/ffa.c
> >> +++ b/xen/arch/arm/tee/ffa.c
> >> @@ -44,6 +44,11 @@
> >>  *   - doesn't support signalling the secondary scheduler of pending
> >>  *     notification for secure partitions
> >>  *   - doesn't support notifications for Xen itself
> >> + * o FFA_PARTITION_INFO_GET/GET_REGS:
> >> + *   - v1.0 guests may see duplicate SP IDs when firmware provides UUIDs
> >> + *   - SP list is cached at init; SPMC tag changes are not tracked
> >> + *     between calls
> >> + *   - SP list is capped at FFA_MAX_NUM_SP entries
> >>  *
> >>  * There are some large locked sections with ffa_spmc_tx_lock and
> >>  * ffa_spmc_rx_lock. Especially the ffa_spmc_tx_lock spinlock used
> >> @@ -188,6 +193,7 @@ static bool ffa_negotiate_version(struct cpu_user_regs 
> >> *regs)
> >>             write_lock(&ffa_ctx_list_rwlock);
> >>             list_add_tail(&ctx->ctx_list, &ffa_ctx_head);
> >>             write_unlock(&ffa_ctx_list_rwlock);
> >> +            ffa_partinfo_inc_tag();
> >>         }
> >>
> >>         goto out_continue;
> >> @@ -341,6 +347,12 @@ static void handle_features(struct cpu_user_regs 
> >> *regs)
> >>     case FFA_FEATURE_SCHEDULE_RECV_INTR:
> >>         ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
> >>         break;
> >> +    case FFA_PARTITION_INFO_GET_REGS:
> >> +        if ( ACCESS_ONCE(ctx->guest_vers) >= FFA_VERSION_1_2 )
> >> +            ffa_set_regs_success(regs, 0, 0);
> >> +        else
> >> +            ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> >> +        break;
> >>
> >>     case FFA_NOTIFICATION_BIND:
> >>     case FFA_NOTIFICATION_UNBIND:
> >> @@ -402,6 +414,9 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> >>     case FFA_PARTITION_INFO_GET:
> >>         ffa_handle_partition_info_get(regs);
> >>         return true;
> >> +    case FFA_PARTITION_INFO_GET_REGS:
> >> +        ffa_handle_partition_info_get_regs(regs);
> >> +        return true;
> >>     case FFA_RX_RELEASE:
> >>         e = ffa_rx_release(ctx);
> >>         break;
> >> @@ -629,6 +644,7 @@ static int ffa_domain_teardown(struct domain *d)
> >>         write_lock(&ffa_ctx_list_rwlock);
> >>         list_del(&ctx->ctx_list);
> >>         write_unlock(&ffa_ctx_list_rwlock);
> >> +        ffa_partinfo_inc_tag();
> >>     }
> >>
> >>     ffa_rxtx_domain_destroy(d);
> >> diff --git a/xen/arch/arm/tee/ffa_partinfo.c 
> >> b/xen/arch/arm/tee/ffa_partinfo.c
> >> index d7f9b9f7153c..1c7b3579f798 100644
> >> --- a/xen/arch/arm/tee/ffa_partinfo.c
> >> +++ b/xen/arch/arm/tee/ffa_partinfo.c
> >> @@ -28,10 +28,39 @@ struct ffa_partition_info_1_1 {
> >>     uint8_t uuid[16];
> >> };
> >>
> >> +/* Registers a3..a17 (15 regs) carry partition descriptors, 3 regs each. 
> >> */
> >> +#define FFA_PARTINFO_REG_MAX_ENTRIES \
> >> +    ((15 * sizeof(uint64_t)) / sizeof(struct ffa_partition_info_1_1))
> >> +
> >> /* SP list cache (secure endpoints only); populated at init. */
> >> static void *sp_list __read_mostly;
> >> static uint32_t sp_list_count __read_mostly;
> >> static uint32_t sp_list_entry_size __read_mostly;
> >> +
> >> +/* SP list is static; tag only moves when VMs are added/removed. */
> >> +static atomic_t ffa_partinfo_tag = ATOMIC_INIT(1);
> >> +
> >> +void ffa_partinfo_inc_tag(void)
> >> +{
> >> +    atomic_inc(&ffa_partinfo_tag);
> >
> > Do we need to worry about this value wrapping? Is wrapping permitted?
>
> wrapping is permitted, as the end this value is used to ensure changes in
> the middle of info_get_regs are detected. Having enough changes in the
> middle for this to wrap and end up un-detected by the caller is near to 
> impossible.
> In any case, the status we return is a snapshot which have changed as soon as
> the result is returned so i would consider this a best effort (even if the 
> probability
> for this to happen is very very near to 0).

I'm sorry for being unclear. atomic_t is implemented as struct { int
counter; }, wrapping the counter element is undefined behaviour. But
it will take quite some time before we get there and perhaps the
assembly implementation of atomic_inc() is expected to mitigate the
undefined behaviour part.

>
> >
> >> +}
> >> +
> >> +static inline uint16_t ffa_partinfo_get_tag(void)
> >> +{
> >> +    /*
> >> +     * Tag moves with VM list changes only.
> >> +     *
> >> +     * Limitation: we cannot detect an SPMC tag change between calls 
> >> because we
> >> +     * do not retain the previous SPMC tag; we only refresh it via the 
> >> mandatory
> >> +     * start_index=0 call and assume it stays stable while combined_tag 
> >> (our
> >> +     * VM/SP-count tag) is used for guest validation. This means SPMC tag
> >> +     * changes alone will not trigger RETRY.
> >> +     */
> >> +    if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> >> +        return atomic_read(&ffa_partinfo_tag) & GENMASK(15, 0);
> >> +    else
> >> +        return 1;
> >> +}
> >> static int32_t ffa_partition_info_get(struct ffa_uuid uuid, uint32_t flags,
> >>                                       uint32_t *count, uint32_t *fpi_size)
> >> {
> >> @@ -125,6 +154,7 @@ static int32_t ffa_get_sp_partinfo(struct ffa_uuid 
> >> uuid, uint32_t *sp_count,
> >>     for ( n = 0; n < sp_list_count; n++ )
> >>     {
> >>         void *entry = sp_list + n * sp_list_entry_size;
> >> +        void *dst_pos;
> >>
> >>         if ( !ffa_sp_entry_matches_uuid(entry, uuid) )
> >>             continue;
> >> @@ -136,11 +166,20 @@ static int32_t ffa_get_sp_partinfo(struct ffa_uuid 
> >> uuid, uint32_t *sp_count,
> >>          * This is a non-compliance to the specification but 1.0 VMs should
> >>          * handle that on their own to simplify Xen implementation.
> >>          */
> >> +        dst_pos = *dst_buf;
> >>         ret = ffa_copy_info(dst_buf, end_buf, entry, dst_size,
> >>                             sp_list_entry_size);
> >>         if ( ret )
> >>             return ret;
> >>
> >> +        if ( !ffa_uuid_is_nil(uuid) &&
> >> +             dst_size >= sizeof(struct ffa_partition_info_1_1) )
> >> +        {
> >> +            struct ffa_partition_info_1_1 *fpi = dst_pos;
> >> +
> >> +            memset(fpi->uuid, 0, sizeof(fpi->uuid));
> >> +        }
> >> +
> >>         count++;
> >>     }
> >>
> >> @@ -152,6 +191,38 @@ static int32_t ffa_get_sp_partinfo(struct ffa_uuid 
> >> uuid, uint32_t *sp_count,
> >>     return FFA_RET_OK;
> >> }
> >>
> >> +static uint16_t ffa_get_sp_partinfo_regs(struct ffa_uuid uuid,
> >> +                                         uint16_t start_index,
> >> +                                         uint64_t *out_regs,
> >> +                                         uint16_t max_entries)
> >> +{
> >> +    uint32_t idx = 0;
> >> +    uint16_t filled = 0;
> >> +    uint32_t n;
> >> +
> >> +    for ( n = 0; n < sp_list_count && filled < max_entries; n++ )
> >> +    {
> >> +        void *entry = sp_list + n * sp_list_entry_size;
> >> +
> >> +        if ( !ffa_sp_entry_matches_uuid(entry, uuid) )
> >> +            continue;
> >> +
> >> +        if ( idx++ < start_index )
> >> +            continue;
> >> +
> >> +        memcpy(&out_regs[filled * 3], entry,
> >> +               sizeof(struct ffa_partition_info_1_1));
> >> +        if ( !ffa_uuid_is_nil(uuid) )
> >> +        {
> >> +            out_regs[filled * 3 + 1] = 0;
> >> +            out_regs[filled * 3 + 2] = 0;
> >> +        }
> >> +        filled++;
> >> +    }
> >> +
> >> +    return filled;
> >> +}
> >> +
> >> static int32_t ffa_get_vm_partinfo(struct ffa_uuid uuid, uint32_t 
> >> start_index,
> >>                                    uint32_t *vm_count, void **dst_buf,
> >>                                    void *end_buf, uint32_t dst_size)
> >> @@ -368,6 +439,146 @@ out:
> >>     }
> >> }
> >>
> >> +void ffa_handle_partition_info_get_regs(struct cpu_user_regs *regs)
> >> +{
> >> +    struct domain *d = current->domain;
> >> +    struct ffa_ctx *ctx = d->arch.tee;
> >> +    struct ffa_uuid uuid;
> >> +    uint32_t sp_count = 0, vm_count = 0, total_count;
> >> +    uint16_t start_index, tag;
> >> +    uint16_t num_entries = 0;
> >> +    uint64_t x3 = get_user_reg(regs, 3);
> >> +    int32_t ret = FFA_RET_OK;
> >> +    uint64_t out_regs[18] = { 0 };
> >> +    unsigned int n;
> >> +    uint16_t tag_out;
> >> +
> >> +    if ( ACCESS_ONCE(ctx->guest_vers) < FFA_VERSION_1_2 )
> >> +    {
> >> +        ret = FFA_RET_NOT_SUPPORTED;
> >> +        goto out;
> >> +    }
> >> +
> >> +    /*
> >> +     * Registers a3..a17 (15 regs) carry partition descriptors, 3 regs 
> >> each.
> >> +     * For FF-A 1.2, that yields a maximum of 5 entries per GET_REGS call.
> >> +     * Enforce the assumed layout so window sizing stays correct.
> >> +     */
> >> +    BUILD_BUG_ON(FFA_PARTINFO_REG_MAX_ENTRIES != 5);
> >> +
> >> +    for ( n = 4; n <= 17; n++ )
> >> +    {
> >> +        if ( get_user_reg(regs, n) )
> >> +        {
> >
> > x4-x17 are SBZ, so I think we should only ignore them.
>
> Ack, this was added to satisfy the compliance suite but this
> has been solved since. I will remove.
>
> >
> >> +            ret = FFA_RET_INVALID_PARAMETERS;
> >> +            goto out;
> >> +        }
> >> +    }
> >> +
> >> +    if ( x3 >> 32 )
> >
> > Same here: Bits[63:32] are SBZ.
>
> Same here.
>
> >
> >> +    {
> >> +        ret = FFA_RET_INVALID_PARAMETERS;
> >> +        goto out;
> >> +    }
> >> +
> >> +    start_index = x3 & GENMASK(15, 0);
> >> +    tag = (x3 >> 16) & GENMASK(15, 0);
> >> +
> >> +    /* Start index must allow room for up to 5 entries without 16-bit 
> >> overflow. */
> >> +    if ( start_index > (GENMASK(15, 0) - (FFA_PARTINFO_REG_MAX_ENTRIES - 
> >> 1)) )
> >> +    {
> >> +        ret = FFA_RET_INVALID_PARAMETERS;
> >> +        goto out;
> >> +    }
> >> +
> >> +    uuid.val[0] = get_user_reg(regs, 1);
> >> +    uuid.val[1] = get_user_reg(regs, 2);
> >> +
> >> +    if ( sp_list_count &&
> >> +         sp_list_entry_size != sizeof(struct ffa_partition_info_1_1) )
> >> +    {
> >> +        ret = FFA_RET_NOT_SUPPORTED;
> >
> > This can't happen. But I guess a sp_list_entry_size > sizeof(struct
> > ffa_partition_info_1_1) might be supported to be future proof.
>
> Right now we have FFA_PARTINFO_REG_MAX_ENTRIES enforcing the
> structure to be 1.1 size. If this is not true in the future we will have to 
> modify
> this.
>
> This is not really future proof and i will check if i can rework this.
>
> >
> >> +        goto out;
> >> +    }
> >> +
> >> +    tag_out = ffa_partinfo_get_tag();
> >> +
> >> +    if ( start_index == 0 )
> >> +    {
> >> +        if ( tag )
> >> +        {
> >> +            ret = FFA_RET_INVALID_PARAMETERS;
> >> +            goto out;
> >> +        }
> >> +    }
> >> +    else if ( tag != tag_out )
> >> +    {
> >> +        ret = FFA_RET_RETRY;
> >> +        goto out;
> >> +    }
> >> +
> >> +    if ( ffa_uuid_is_nil(uuid) )
> >> +    {
> >> +        if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> >> +            vm_count = get_ffa_vm_count();
> >> +        else
> >> +            vm_count = 1; /* Caller VM only */
> >> +    }
> >> +
> >> +    ret = ffa_get_sp_count(uuid, &sp_count);
> >> +    if ( ret )
> >> +        goto out;
> >> +
> >> +    total_count = sp_count + vm_count;
> >> +
> >> +    if ( total_count == 0 || start_index >= total_count )
> >> +    {
> >> +        ret = FFA_RET_INVALID_PARAMETERS;
> >> +        goto out;
> >> +    }
> >> +
> >> +    if ( start_index < sp_count )
> >> +        num_entries = ffa_get_sp_partinfo_regs(uuid, start_index, 
> >> &out_regs[3],
> >> +                                               
> >> FFA_PARTINFO_REG_MAX_ENTRIES);
> >> +
> >> +    if ( num_entries < FFA_PARTINFO_REG_MAX_ENTRIES )
> >> +    {
> >> +        uint32_t vm_start = start_index > sp_count ?
> >> +                            start_index - sp_count : 0;
> >> +        uint32_t filled = 0;
> >> +        void *vm_dst = &out_regs[3 + num_entries * 3];
> >> +        void *vm_end = &out_regs[18];
> >> +
> >> +        ret = ffa_get_vm_partinfo(uuid, vm_start, &filled, &vm_dst, 
> >> vm_end,
> >> +                                  sizeof(struct ffa_partition_info_1_1));
> >> +        if ( ret != FFA_RET_OK && ret != FFA_RET_NO_MEMORY )
> >> +            goto out;
> >> +
> >> +        num_entries += filled;
> >> +    }
> >> +
> >> +    if ( num_entries == 0 )
> >> +    {
> >> +        ret = FFA_RET_INVALID_PARAMETERS;
> >> +        goto out;
> >> +    }
> >> +
> >
> > What if the tag read with ffa_partinfo_get_tag() has changed?
>
> As said this is a best effort, we provide a snapshot.
> Now i could check and compare the tag at the end to handle this case.
>
> I will check if i can make this a bit stronger by comparing the tag at the
> beginning and the end or try to handle it differently (get its value while we
> have the rwlock on the list of VMs maybe).

Checking the tag at the end to let the caller retry might be enough.

Cheers,
Jens

>
> Cheers
> Bertrand
>
> >
> > Cheers,
> > Jens
> >
> >> +    out_regs[0] = FFA_SUCCESS_64;
> >> +    out_regs[2] = ((uint64_t)sizeof(struct ffa_partition_info_1_1) << 48) 
> >> |
> >> +                  ((uint64_t)tag_out << 32) |
> >> +                  ((uint64_t)(start_index + num_entries - 1) << 16) |
> >> +                  ((uint64_t)(total_count - 1) & GENMASK(15, 0));
> >> +
> >> +    for ( n = 0; n < ARRAY_SIZE(out_regs); n++ )
> >> +        set_user_reg(regs, n, out_regs[n]);
> >> +
> >> +    return;
> >> +
> >> +out:
> >> +    if ( ret )
> >> +        ffa_set_regs_error(regs, ret);
> >> +}
> >> +
> >> static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
> >>                                       uint8_t msg)
> >> {
> >> diff --git a/xen/arch/arm/tee/ffa_private.h 
> >> b/xen/arch/arm/tee/ffa_private.h
> >> index 1a632983c860..c291f32b56ff 100644
> >> --- a/xen/arch/arm/tee/ffa_private.h
> >> +++ b/xen/arch/arm/tee/ffa_private.h
> >> @@ -289,7 +289,7 @@
> >> #define FFA_MSG_SEND2                   0x84000086U
> >> #define FFA_CONSOLE_LOG_32              0x8400008AU
> >> #define FFA_CONSOLE_LOG_64              0xC400008AU
> >> -#define FFA_PARTITION_INFO_GET_REGS     0x8400008BU
> >> +#define FFA_PARTITION_INFO_GET_REGS     0xC400008BU
> >> #define FFA_MSG_SEND_DIRECT_REQ2        0xC400008DU
> >> #define FFA_MSG_SEND_DIRECT_RESP2       0xC400008EU
> >>
> >> @@ -452,6 +452,8 @@ bool ffa_partinfo_init(void);
> >> int32_t ffa_partinfo_domain_init(struct domain *d);
> >> bool ffa_partinfo_domain_destroy(struct domain *d);
> >> void ffa_handle_partition_info_get(struct cpu_user_regs *regs);
> >> +void ffa_handle_partition_info_get_regs(struct cpu_user_regs *regs);
> >> +void ffa_partinfo_inc_tag(void);
> >>
> >> int32_t ffa_endpoint_domain_lookup(uint16_t endpoint_id, struct domain 
> >> **d_out,
> >>                                    struct ffa_ctx **ctx_out);
> >> --
> >> 2.52.0
>
>