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

Bertrand Marquis posted 4 patches 5 days, 17 hours ago
There is a newer version of this series
[PATCH 4/4] xen/arm: ffa: Add cached GET_REGS support
Posted by Bertrand Marquis 5 days, 17 hours ago
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@arm.com>
---
 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);
+}
+
+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) )
+        {
+            ret = FFA_RET_INVALID_PARAMETERS;
+            goto out;
+        }
+    }
+
+    if ( x3 >> 32 )
+    {
+        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;
+        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;
+    }
+
+    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
Re: [PATCH 4/4] xen/arm: ffa: Add cached GET_REGS support
Posted by Jens Wiklander 3 days, 12 hours ago
Hi Bertrand,

On Wed, Feb 25, 2026 at 11:02 AM Bertrand Marquis
<bertrand.marquis@arm.com> 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@arm.com>
> ---
>  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?

> +}
> +
> +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.

> +            ret = FFA_RET_INVALID_PARAMETERS;
> +            goto out;
> +        }
> +    }
> +
> +    if ( x3 >> 32 )

Same here: Bits[63:32] are SBZ.

> +    {
> +        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.

> +        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?

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
>
Re: [PATCH 4/4] xen/arm: ffa: Add cached GET_REGS support
Posted by Bertrand Marquis 18 hours ago
Hi Jens,

> On 27 Feb 2026, at 15:56, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> 
> Hi Bertrand,
> 
> On Wed, Feb 25, 2026 at 11:02 AM Bertrand Marquis
> <bertrand.marquis@arm.com> 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@arm.com>
>> ---
>> 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).

> 
>> +}
>> +
>> +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).

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


Re: [PATCH 4/4] xen/arm: ffa: Add cached GET_REGS support
Posted by Jens Wiklander 17 hours ago
Hi Bertrand,

On Mon, Mar 2, 2026 at 9:58 AM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Jens,
>
> > On 27 Feb 2026, at 15:56, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Bertrand,
> >
> > On Wed, Feb 25, 2026 at 11:02 AM Bertrand Marquis
> > <bertrand.marquis@arm.com> 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@arm.com>
> >> ---
> >> 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
>
>
Re: [PATCH 4/4] xen/arm: ffa: Add cached GET_REGS support
Posted by Bertrand Marquis 16 hours ago
Hi Jens,

> On 2 Mar 2026, at 11:06, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> 
> Hi Bertrand,
> 
> On Mon, Mar 2, 2026 at 9:58 AM Bertrand Marquis
> <Bertrand.Marquis@arm.com> wrote:
>> 
>> Hi Jens,
>> 
>>> On 27 Feb 2026, at 15:56, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On Wed, Feb 25, 2026 at 11:02 AM Bertrand Marquis
>>> <bertrand.marquis@arm.com> 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@arm.com>
>>>> ---
>>>> 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.

On arm the atomic operations are implemented using and assembler add which
is not typed.
At the end we mask the value to use only 15 bits so we will wrap around more often
than the counter itself.
So i do not think we have an undefined behavior here.

> 
>> 
>>> 
>>>> +}
>>>> +
>>>> +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.

i will give that a try in v2.

Thanks a lot for the review.
I will try to push v2 before the end of the week if not sooner.

Cheers
Bertrand

> 
> 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