|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 4/4] xen/arm: ffa: Add cached GET_REGS support
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?
> +}
> +
> +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
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |