On Mon, Mar 24, 2025 at 10:15 AM Bertrand Marquis
<[email protected]> wrote:
>
> Create a CONFIG_FFA_VM_TO_VM parameter to activate FFA communication
> between VMs.
> When activated list VMs in the system with FF-A support in part_info_get.
>
> When VM to VM is activated, Xen will be tainted as Insecure and a
> message is displayed to the user during the boot as there is no
> filtering of VMs in FF-A so any VM can communicate or see any other VM
> in the system.
>
> WARNING: There is no filtering for now and all VMs are listed !!
>
> Signed-off-by: Bertrand Marquis <[email protected]>
> ---
> Changes in v3:
> - break partinfo_get in several sub functions to make the implementation
> easier to understand and lock handling easier
> - rework implementation to check size along the way and prevent previous
> implementation limits which had to check that the number of VMs or SPs
> did not change
> - taint Xen as INSECURE when VM to VM is enabled
> Changes in v2:
> - Switch ifdef to IS_ENABLED
> - dom was not switched to d as requested by Jan because there is already
> a variable d pointing to the current domain and it must not be
> shadowed.
> ---
> xen/arch/arm/tee/Kconfig | 11 ++
> xen/arch/arm/tee/ffa.c | 12 ++
> xen/arch/arm/tee/ffa_partinfo.c | 270 +++++++++++++++++++++-----------
> xen/arch/arm/tee/ffa_private.h | 12 ++
> 4 files changed, 214 insertions(+), 91 deletions(-)
>
> diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig
> index c5b0f88d7522..88a4c4c99154 100644
> --- a/xen/arch/arm/tee/Kconfig
> +++ b/xen/arch/arm/tee/Kconfig
> @@ -28,5 +28,16 @@ config FFA
>
> [1] https://developer.arm.com/documentation/den0077/latest
>
> +config FFA_VM_TO_VM
> + bool "Enable FF-A between VMs (UNSUPPORTED)" if UNSUPPORTED
> + default n
> + depends on FFA
> + help
> + This option enables to use FF-A between VMs.
> + This is experimental and there is no access control so any
> + guest can communicate with any other guest.
> +
> + If unsure, say N.
> +
> endmenu
>
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 3bbdd7168a6b..e41ab5f8ada6 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -464,6 +464,18 @@ static bool ffa_probe(void)
> printk(XENLOG_INFO "ARM FF-A Mediator version %u.%u\n",
> FFA_MY_VERSION_MAJOR, FFA_MY_VERSION_MINOR);
>
> + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> + {
> + /*
> + * When FFA VM to VM is enabled, the current implementation does not
> + * offer any way to limit which VM can communicate with which VM
> using
> + * FF-A.
> + * Signal this in the xen console and taint the system as insecure.
> + * TODO: Introduce a solution to limit what a VM can do through FFA.
> + */
> + printk(XENLOG_ERR "ffa: VM to VM is enabled, system is insecure
> !!\n");
> + add_taint(TAINT_MACHINE_INSECURE);
> + }
> /*
> * psci_init_smccc() updates this value with what's reported by EL-3
> * or secure world.
> diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
> index c0510ceb8338..93847b36cf2f 100644
> --- a/xen/arch/arm/tee/ffa_partinfo.c
> +++ b/xen/arch/arm/tee/ffa_partinfo.c
> @@ -63,9 +63,152 @@ static int32_t ffa_partition_info_get(uint32_t *uuid,
> uint32_t flags,
> return ret;
> }
>
> -void ffa_handle_partition_info_get(struct cpu_user_regs *regs)
> +static int32_t ffa_get_sp_count(uint32_t *uuid, uint32_t *sp_count)
> +{
> + uint32_t src_size;
> +
> + return ffa_partition_info_get(uuid, FFA_PARTITION_INFO_GET_COUNT_FLAG,
> + sp_count, &src_size);
> +}
> +
> +static int32_t ffa_get_sp_partinfo(uint32_t *uuid, uint32_t *sp_count,
> + void *dst_buf, void *end_buf,
> + uint32_t dst_size)
> {
> int32_t ret;
> + uint32_t src_size, real_sp_count;
> + void *src_buf = ffa_rx;
> +
> + *sp_count = 0;
> +
> + /* Do we have a RX buffer with the SPMC */
> + if ( !ffa_rx )
> + return FFA_RET_DENIED;
> +
> + /* We need to use the RX buffer to receive the list */
> + spin_lock(&ffa_rx_buffer_lock);
> +
> + ret = ffa_partition_info_get(uuid, 0, &real_sp_count, &src_size);
> + if ( ret )
> + goto out;
> +
> + /* We now own the RX buffer */
> +
> + /* We only support a 1.1 firmware version */
> + if ( src_size != sizeof(struct ffa_partition_info_1_1) )
Before this change, we supported firmware version 1.0. It looks like
the code below should work with "src_size < sizeof(struct
ffa_partition_info_1_0)" as we had before.
> + {
> + ret = FFA_RET_NOT_SUPPORTED;
> + goto out_release;
> + }
> +
> + for ( uint32_t sp_num = 0; sp_num < real_sp_count; sp_num++ )
> + {
> + struct ffa_partition_info_1_1 *fpi = src_buf;
> +
> + /* filter out SP not following bit 15 convention if any */
> + if ( FFA_ID_IS_SECURE(fpi->id) )
> + {
> + if ( dst_buf + dst_size > end_buf )
> + {
> + ret = FFA_RET_NO_MEMORY;
> + goto out_release;
> + }
> +
> + memcpy(dst_buf, src_buf, dst_size);
> + dst_buf += dst_size;
> + *sp_count += 1;
This is subtle and easily misunderstood. Normally you'd use the ++
operator when incrementing by one, but that doesn't work as intended
unless written as (*sp_count)++. I prefer using a local variable and
updating *sp_count with a normal assignment when the loop is done. If
you agree, please update ffa_get_vm_partinfo() too since it has the
same pattern.
Cheers,
Jens
> + }
> +
> + src_buf += src_size;
> + }
> +
> +out_release:
> + ffa_hyp_rx_release();
> +out:
> + spin_unlock(&ffa_rx_buffer_lock);
> + return ret;
> +}
> +
> +static uint32_t ffa_get_vm_count(void)
> +{
> + struct domain *d = current->domain;
> + struct domain *dom;
> + uint32_t vm_count = 0;
> +
> + /* Count the number of VM with FF-A support */
> + rcu_read_lock(&domlist_read_lock);
> + for_each_domain( dom )
> + {
> + struct ffa_ctx *vm = dom->arch.tee;
> +
> + if (dom != d && vm != NULL && vm->guest_vers != 0)
> + vm_count++;
> + }
> + rcu_read_unlock(&domlist_read_lock);
> +
> + return vm_count;
> +}
> +
> +static int32_t ffa_get_vm_partinfo(uint32_t *vm_count, void *dst_buf,
> + void *end_buf, uint32_t dst_size)
> +{
> + struct domain *d = current->domain;
> + struct domain *dom;
> + int32_t ret = FFA_RET_OK;
> +
> + *vm_count = 0;
> +
> + rcu_read_lock(&domlist_read_lock);
> + for_each_domain( dom )
> + {
> + struct ffa_ctx *vm = dom->arch.tee;
> +
> + /*
> + * we do not add the VM calling to the list and only VMs with
> + * FF-A support
> + */
> + if ( dom != d && vm != NULL && vm->guest_vers != 0 )
> + {
> + /*
> + * We do not have UUID info for VMs so use
> + * the 1.0 structure so that we set UUIDs to
> + * zero using memset
> + */
> + struct ffa_partition_info_1_0 srcvm;
> +
> + if ( dst_buf + dst_size > end_buf )
> + {
> + ret = FFA_RET_NO_MEMORY;
> + goto out;
> + }
> +
> + srcvm.id = ffa_get_vm_id(dom);
> + srcvm.execution_context = dom->max_vcpus;
> + srcvm.partition_properties = FFA_PART_VM_PROP;
> + if ( is_64bit_domain(dom) )
> + srcvm.partition_properties |= FFA_PART_PROP_AARCH64_STATE;
> +
> + memcpy(dst_buf, &srcvm, MIN(sizeof(srcvm), dst_size));
> +
> + if ( dst_size > sizeof(srcvm) )
> + memset(dst_buf + sizeof(srcvm), 0,
> + dst_size - sizeof(srcvm));
> +
> + dst_buf += dst_size;
> + *vm_count += 1;
> + }
> + }
> +
> +out:
> + rcu_read_unlock(&domlist_read_lock);
> +
> + return ret;
> +}
> +
> +
> +void ffa_handle_partition_info_get(struct cpu_user_regs *regs)
> +{
> + int32_t ret = FFA_RET_OK;
> struct domain *d = current->domain;
> struct ffa_ctx *ctx = d->arch.tee;
> uint32_t flags = get_user_reg(regs, 5);
> @@ -75,9 +218,9 @@ void ffa_handle_partition_info_get(struct cpu_user_regs
> *regs)
> get_user_reg(regs, 3),
> get_user_reg(regs, 4),
> };
> - uint32_t src_size, dst_size;
> - void *dst_buf;
> - uint32_t ffa_sp_count = 0;
> + uint32_t dst_size = 0;
> + void *dst_buf, *end_buf;
> + uint32_t ffa_vm_count = 0, ffa_sp_count = 0;
>
> /*
> * If the guest is v1.0, he does not get back the entry size so we must
> @@ -89,118 +232,63 @@ void ffa_handle_partition_info_get(struct cpu_user_regs
> *regs)
> else
> dst_size = sizeof(struct ffa_partition_info_1_1);
>
> - /*
> - * FF-A v1.0 has w5 MBZ while v1.1 allows
> - * FFA_PARTITION_INFO_GET_COUNT_FLAG to be non-zero.
> - *
> - * FFA_PARTITION_INFO_GET_COUNT is only using registers and not the
> - * rxtx buffer so do the partition_info_get directly.
> - */
> - if ( flags == FFA_PARTITION_INFO_GET_COUNT_FLAG &&
> - ctx->guest_vers == FFA_VERSION_1_1 )
> + /* Only count requested */
> + if ( flags )
> {
> + /*
> + * FF-A v1.0 has w5 MBZ while v1.1 allows
> + * FFA_PARTITION_INFO_GET_COUNT_FLAG to be non-zero.
> + */
> + if ( ctx->guest_vers == FFA_VERSION_1_0 ||
> + flags != FFA_PARTITION_INFO_GET_COUNT_FLAG )
> + {
> + ret = FFA_RET_INVALID_PARAMETERS;
> + goto out;
> + }
> +
> if ( ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
> - ret = ffa_partition_info_get(uuid, flags, &ffa_sp_count,
> - &src_size);
> - else
> - ret = FFA_RET_OK;
> + {
> + ret = ffa_get_sp_count(uuid, &ffa_sp_count);
> + if ( ret )
> + goto out;
> + }
>
> - goto out;
> - }
> + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> + ffa_vm_count = ffa_get_vm_count();
>
> - if ( flags )
> - {
> - ret = FFA_RET_INVALID_PARAMETERS;
> - goto out;
> - }
> -
> - if ( !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
> - {
> - /* Just give an empty partition list to the caller */
> - ret = FFA_RET_OK;
> goto out;
> }
>
> + /* Get the RX buffer to write the list of partitions */
> ret = ffa_rx_acquire(d);
> if ( ret != FFA_RET_OK )
> goto out;
>
> dst_buf = ctx->rx;
> + end_buf = ctx->rx + ctx->page_count * FFA_PAGE_SIZE;
>
> - if ( !ffa_rx )
> + if ( ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
> {
> - ret = FFA_RET_DENIED;
> - goto out_rx_release;
> - }
> -
> - spin_lock(&ffa_rx_buffer_lock);
> -
> - ret = ffa_partition_info_get(uuid, 0, &ffa_sp_count, &src_size);
> + ret = ffa_get_sp_partinfo(uuid, &ffa_sp_count, dst_buf, end_buf,
> + dst_size);
>
> - if ( ret )
> - goto out_rx_hyp_unlock;
> + if ( ret )
> + goto out_rx_release;
>
> - /*
> - * ffa_partition_info_get() succeeded so we now own the RX buffer we
> - * share with the SPMC. We must give it back using ffa_hyp_rx_release()
> - * once we've copied the content.
> - */
> -
> - /* we cannot have a size smaller than 1.0 structure */
> - if ( src_size < sizeof(struct ffa_partition_info_1_0) )
> - {
> - ret = FFA_RET_NOT_SUPPORTED;
> - goto out_rx_hyp_release;
> - }
> -
> - if ( ctx->page_count * FFA_PAGE_SIZE < ffa_sp_count * dst_size )
> - {
> - ret = FFA_RET_NO_MEMORY;
> - goto out_rx_hyp_release;
> + dst_buf += ffa_sp_count * dst_size;
> }
>
> - if ( ffa_sp_count > 0 )
> - {
> - uint32_t n, n_limit = ffa_sp_count;
> - void *src_buf = ffa_rx;
> -
> - /* copy the secure partitions info */
> - for ( n = 0; n < n_limit; n++ )
> - {
> - struct ffa_partition_info_1_1 *fpi = src_buf;
> -
> - /* filter out SP not following bit 15 convention if any */
> - if ( FFA_ID_IS_SECURE(fpi->id) )
> - {
> - memcpy(dst_buf, src_buf, dst_size);
> - dst_buf += dst_size;
> - }
> - else
> - ffa_sp_count--;
> + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> + ret = ffa_get_vm_partinfo(&ffa_vm_count, dst_buf, end_buf, dst_size);
>
> - src_buf += src_size;
> - }
> - }
> -
> -out_rx_hyp_release:
> - ffa_hyp_rx_release();
> -out_rx_hyp_unlock:
> - spin_unlock(&ffa_rx_buffer_lock);
> out_rx_release:
> - /*
> - * The calling VM RX buffer only contains data to be used by the VM if
> the
> - * call was successful, in which case the VM has to release the buffer
> - * once it has used the data.
> - * If something went wrong during the call, we have to release the RX
> - * buffer back to the SPMC as the VM will not do it.
> - */
> - if ( ret != FFA_RET_OK )
> + if ( ret )
> ffa_rx_release(d);
> out:
> if ( ret )
> ffa_set_regs_error(regs, ret);
> else
> - ffa_set_regs_success(regs, ffa_sp_count, dst_size);
> + ffa_set_regs_success(regs, ffa_sp_count + ffa_vm_count, dst_size);
> }
>
> static int32_t ffa_direct_req_send_vm(uint16_t sp_id, uint16_t vm_id,
> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
> index c4cd65538908..bd6877d8c632 100644
> --- a/xen/arch/arm/tee/ffa_private.h
> +++ b/xen/arch/arm/tee/ffa_private.h
> @@ -187,6 +187,18 @@
> */
> #define FFA_PARTITION_INFO_GET_COUNT_FLAG BIT(0, U)
>
> +/*
> + * Partition properties we give for a normal world VM:
> + * - can send direct message but not receive them
> + * - can handle indirect messages
> + * - can receive notifications
> + * 32/64 bit flag is set depending on the VM
> + */
> +#define FFA_PART_VM_PROP (FFA_PART_PROP_DIRECT_REQ_SEND | \
> + FFA_PART_PROP_INDIRECT_MSGS | \
> + FFA_PART_PROP_RECV_NOTIF | \
> + FFA_PART_PROP_IS_PE_ID)
> +
> /* Flags used in calls to FFA_NOTIFICATION_GET interface */
> #define FFA_NOTIF_FLAG_BITMAP_SP BIT(0, U)
> #define FFA_NOTIF_FLAG_BITMAP_VM BIT(1, U)
> --
> 2.47.1
>
