| Message ID | 8f0928b4e94b47d6fed201dcd8cfb1068573b297.1742824138.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | FF-A VM to VM support | expand |
Hi, > On 24 Mar 2025, at 14:53, Bertrand Marquis <Bertrand.Marquis@arm.com> 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 <bertrand.marquis@arm.com> > --- > Changes in v4: > - properly handle SPMC version 1.0 header size case in partinfo_get > - switch to local counting variables instead of *pointer += 1 form > - coding style issue with missing spaces in if () > 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 | 274 +++++++++++++++++++++----------- > xen/arch/arm/tee/ffa_private.h | 12 ++ > 4 files changed, 218 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..406c57b95f77 100644 > --- a/xen/arch/arm/tee/ffa_partinfo.c > +++ b/xen/arch/arm/tee/ffa_partinfo.c > @@ -63,9 +63,156 @@ 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; > + uint32_t 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 */ This comment should have been removed. I will fix it on next version of might be possible to do on commit if there are no further comments here. Cheers Bertrand
Hi Bertrand, On Mon, Mar 24, 2025 at 2:58 PM Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > > Hi, > > > On 24 Mar 2025, at 14:53, Bertrand Marquis <Bertrand.Marquis@arm.com> 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 <bertrand.marquis@arm.com> > > --- > > Changes in v4: > > - properly handle SPMC version 1.0 header size case in partinfo_get > > - switch to local counting variables instead of *pointer += 1 form > > - coding style issue with missing spaces in if () > > 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 | 274 +++++++++++++++++++++----------- > > xen/arch/arm/tee/ffa_private.h | 12 ++ > > 4 files changed, 218 insertions(+), 91 deletions(-) With the comment you mention below fixed, please add: Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Cheers, Jens > > > > 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..406c57b95f77 100644 > > --- a/xen/arch/arm/tee/ffa_partinfo.c > > +++ b/xen/arch/arm/tee/ffa_partinfo.c > > @@ -63,9 +63,156 @@ 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; > > + uint32_t 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 */ > > This comment should have been removed. > I will fix it on next version of might be possible to do on commit > if there are no further comments here. > > Cheers > Bertrand >
Hi Bertrand, I didn't dig in the spec and neither some of the callers. So I will mainly focus on the implementation from Xen PoV. On 24/03/2025 13:53, Bertrand Marquis 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 <bertrand.marquis@arm.com> > --- > Changes in v4: > - properly handle SPMC version 1.0 header size case in partinfo_get > - switch to local counting variables instead of *pointer += 1 form > - coding style issue with missing spaces in if () > 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 | 274 +++++++++++++++++++++----------- > xen/arch/arm/tee/ffa_private.h | 12 ++ > 4 files changed, 218 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..406c57b95f77 100644 > --- a/xen/arch/arm/tee/ffa_partinfo.c > +++ b/xen/arch/arm/tee/ffa_partinfo.c > @@ -63,9 +63,156 @@ 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; > + uint32_t 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_0) ) > + { > + ret = FFA_RET_NOT_SUPPORTED; > + goto out_release; > + } > + > + for ( uint32_t sp_num = 0; sp_num < real_sp_count; sp_num++ ) What's the upper limit of real_sp_count? Asking just in case we need to handle preemption. > + { > + 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 ) Can "dst_buf + dst_size" overflow? Also, NIT: can you add parenthese to make clear about precedence? > + { > + ret = FFA_RET_NO_MEMORY; > + goto out_release; > + } > + > + memcpy(dst_buf, src_buf, MIN(src_size, dst_size)); What's the maximum size of src_size and dst_size? > + if ( dst_size > src_size ) > + memset(dst_buf + src_size, 0, dst_size - src_size); > + > + dst_buf += dst_size; > + count++; > + } > + > + src_buf += src_size; > + } > + > + *sp_count = count; > + > +out_release: > + ffa_hyp_rx_release(); > +out: > + spin_unlock(&ffa_rx_buffer_lock); > + return ret; > +} > + > +static uint32_t ffa_get_vm_count(void) Is this meant to be called often? If so, can we instead have a counter that will be incremented when the vTEE is first initialized and then decremented when the VM is destroyed? > +{ > + struct domain *d = current->domain; > + struct domain *dom; NIT: "d" and "dom" are a bit too close. Could we rename "d" with "curr_d"? > + uint32_t vm_count = 0; > + > + /* Count the number of VM with FF-A support */ This comment implies this is including the current VM. But ... > + 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 ) ... here you are excluding it. Also, it sounds like this is support by the OS rather than the VM itself. Is that correct? > + vm_count++;> + } > + rcu_read_unlock(&domlist_read_lock); > +> + return vm_count; OOI, I guess this value is just used as an hint? Asking because the number of domains can change at any point. > +} > + > +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; > + uint32_t count = 0; > + > + rcu_read_lock(&domlist_read_lock); > + for_each_domain( dom ) We can have quite a lot of domains in the system. So how can we ensure this is not hogging a pCPU? I would be ok to delay the work, but we need a TODO so we remember to address it before this is security supported. > + { > + 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 ) Same question as the other similar check. > + { > + 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; > + count++; > + } > + } > + *vm_count = count; > + > +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 +222,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 +236,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 ) > { > - 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; > + /* > + * 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; > + } > > - goto out; > - } > + if ( ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) ) > + { > + ret = ffa_get_sp_count(uuid, &ffa_sp_count); > + if ( ret ) > + goto out; > + } > > - if ( flags ) > - { > - ret = FFA_RET_INVALID_PARAMETERS; > - goto out; > - } > + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) ) > + ffa_vm_count = ffa_get_vm_count(); > > - 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); > - > - if ( ret ) > - goto out_rx_hyp_unlock; > + ret = ffa_get_sp_partinfo(uuid, &ffa_sp_count, dst_buf, end_buf, > + dst_size); > > - /* > - * 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. > - */ > + if ( ret ) > + goto out_rx_release; > > - /* 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; > + dst_buf += ffa_sp_count * dst_size; > } > > - if ( ctx->page_count * FFA_PAGE_SIZE < ffa_sp_count * dst_size ) > - { > - ret = FFA_RET_NO_MEMORY; > - goto out_rx_hyp_release; > - } > + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) ) > + ret = ffa_get_vm_partinfo(&ffa_vm_count, dst_buf, end_buf, 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--; > - > - 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) Cheers,
Hi Julien, > On 27 Mar 2025, at 00:14, Julien Grall <julien@xen.org> wrote: > > Hi Bertrand, > > I didn't dig in the spec and neither some of the callers. So I will mainly focus on the implementation from Xen PoV. Perfectly ok I think, Jens review should cover the spec point of view. > > On 24/03/2025 13:53, Bertrand Marquis 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 <bertrand.marquis@arm.com> >> --- >> Changes in v4: >> - properly handle SPMC version 1.0 header size case in partinfo_get >> - switch to local counting variables instead of *pointer += 1 form >> - coding style issue with missing spaces in if () >> 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 | 274 +++++++++++++++++++++----------- >> xen/arch/arm/tee/ffa_private.h | 12 ++ >> 4 files changed, 218 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..406c57b95f77 100644 >> --- a/xen/arch/arm/tee/ffa_partinfo.c >> +++ b/xen/arch/arm/tee/ffa_partinfo.c >> @@ -63,9 +63,156 @@ 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; >> + uint32_t 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_0) ) >> + { >> + ret = FFA_RET_NOT_SUPPORTED; >> + goto out_release; >> + } >> + >> + for ( uint32_t sp_num = 0; sp_num < real_sp_count; sp_num++ ) > > What's the upper limit of real_sp_count? Asking just in case we need to handle preemption. In theory that would be 32k but in practice the number of entries we can receive is limited by the size of the exchange area we have with the secure world. This area is currently defined to be one page and each entry is 8 byte in the case where firmware is 1.0 (24 bytes otherwise). This is an upper limit of 500 entries. Now I do think this is completely unrealistic to imagine a secure world with 500 SPs so If you are ok I would rather define an upper limit in Xen (I would say 64 SPs) that can be changed in the code (through a define). This call currently does not support preemption in the FF-A spec (and that is something i will check for future versions) so I would have no solution to "continue" it. Would the "define" solution be acceptable for now ? > >> + { >> + 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 ) > > Can "dst_buf + dst_size" overflow? Well it would mean that the memory corresponding to the buffer in Xen is mapped at a virtual address at the end of the 64bit space. dst_size is set in the code (see ffa_handle_partition_info_get) so can only be 8 or 24. Probability is low but cannot be ruled out so I will modify the code to check for overflow. > > Also, NIT: can you add parenthese to make clear about precedence? ack > >> + { >> + ret = FFA_RET_NO_MEMORY; >> + goto out_release; >> + } >> + >> + memcpy(dst_buf, src_buf, MIN(src_size, dst_size)); > > What's the maximum size of src_size and dst_size? dst_size is set in the code (8 or 24) so there should be no issue here. With the overflow checked upper we should be safe here. > >> + if ( dst_size > src_size ) >> + memset(dst_buf + src_size, 0, dst_size - src_size); >> + >> + dst_buf += dst_size; >> + count++; >> + } >> + >> + src_buf += src_size; >> + } >> + >> + *sp_count = count; >> + >> +out_release: >> + ffa_hyp_rx_release(); >> +out: >> + spin_unlock(&ffa_rx_buffer_lock); >> + return ret; >> +} >> + >> +static uint32_t ffa_get_vm_count(void) > > Is this meant to be called often? If so, can we instead have a counter that will be incremented when the vTEE is first initialized and then decremented when the VM is destroyed? As of now we could have a global counter that we increase or decrease when a domain version is negociated and when a domain is destroyed. We could also have some kind of global save of the result to be returned to a guest. But I did not do that because: - cpu time required to update the list would be taken out an FF-A call for FFA_VERSION of a VM which would require a global lock to protect the data - when we will have filtering the data will be per VM (which would make the initial update more complex) - incoming we have a notion of UUID and filtering depending on the requested UUID which will make the global value only useable in a limited number of cases. I have 2 pending series on top of this one which would have to remove such optimisations. At the end this is definitely not something supposed to call often (linux driver is calling it once during init). > >> +{ >> + struct domain *d = current->domain; > > + struct domain *dom; > > NIT: "d" and "dom" are a bit too close. Could we rename "d" with "curr_d"? i will go with curr_d dest_d to make this clearer. > >> + uint32_t vm_count = 0; >> + >> + /* Count the number of VM with FF-A support */ > > This comment implies this is including the current VM. But ... > >> + 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 ) > > ... here you are excluding it. Also, it sounds like this is support by the OS rather than the VM itself. Is that correct? I have a comment to explain that one in a different serie that i will put here. Basically before 1.2, the spec was a bit blurry on if or not the result should include the calling VM and in fact Linux driver (before 1.2) was ending with an error if its own data was included in the result hence this filter. I will add a comment for that. > > > + vm_count++;> + } >> + rcu_read_unlock(&domlist_read_lock); > > +> + return vm_count; > > OOI, I guess this value is just used as an hint? Asking because the number of domains can change at any point. Definitely yes. The data is what it is when called but anything could change after. This is mostly used as hint by callers. > >> +} >> + >> +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; >> + uint32_t count = 0; >> + >> + rcu_read_lock(&domlist_read_lock); >> + for_each_domain( dom ) > > We can have quite a lot of domains in the system. So how can we ensure this is not hogging a pCPU? > > I would be ok to delay the work, but we need a TODO so we remember to address it before this is security supported. Definitely something not addressed in the spec for now that needs addressing (as explained earlier). We have other calls that can be "cut" and this one will need to support it in the future. For now there is not much we can do and limit the number of SPs we allow will not really solve the issue in the VM to VM case so I will add a big TODO as you suggest. > >> + { >> + 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 ) > > Same question as the other similar check. Same answer and i will add an overflow check. > >> + { >> + 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; >> + count++; >> + } >> + } >> + *vm_count = count; >> + >> +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 +222,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 +236,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 ) >> { >> - 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; >> + /* >> + * 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; >> + } >> - goto out; >> - } >> + if ( ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) ) >> + { >> + ret = ffa_get_sp_count(uuid, &ffa_sp_count); >> + if ( ret ) >> + goto out; >> + } >> - if ( flags ) >> - { >> - ret = FFA_RET_INVALID_PARAMETERS; >> - goto out; >> - } >> + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) ) >> + ffa_vm_count = ffa_get_vm_count(); >> - 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); >> - >> - if ( ret ) >> - goto out_rx_hyp_unlock; >> + ret = ffa_get_sp_partinfo(uuid, &ffa_sp_count, dst_buf, end_buf, >> + dst_size); >> - /* >> - * 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. >> - */ >> + if ( ret ) >> + goto out_rx_release; >> - /* 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; >> + dst_buf += ffa_sp_count * dst_size; >> } >> - if ( ctx->page_count * FFA_PAGE_SIZE < ffa_sp_count * dst_size ) >> - { >> - ret = FFA_RET_NO_MEMORY; >> - goto out_rx_hyp_release; >> - } >> + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) ) >> + ret = ffa_get_vm_partinfo(&ffa_vm_count, dst_buf, end_buf, 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--; >> - >> - 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) > > Cheers, Thanks a lot those are very useful comments and feedback for the code and the spec :-) Cheers Bertrand > > -- > Julien Grall
Hi Bertrand, On 27/03/2025 08:25, Bertrand Marquis wrote: >> On 27 Mar 2025, at 00:14, Julien Grall <julien@xen.org> wrote: >>> +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; >>> + uint32_t 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_0) ) >>> + { >>> + ret = FFA_RET_NOT_SUPPORTED; >>> + goto out_release; >>> + } >>> + >>> + for ( uint32_t sp_num = 0; sp_num < real_sp_count; sp_num++ ) >> >> What's the upper limit of real_sp_count? Asking just in case we need to handle preemption. > > In theory that would be 32k but in practice the number of entries we can receive is > limited by the size of the exchange area we have with the secure world. > > This area is currently defined to be one page and each entry is 8 byte in the > case where firmware is 1.0 (24 bytes otherwise). > > This is an upper limit of 500 entries. > > Now I do think this is completely unrealistic to imagine a secure world with 500 SPs > so If you are ok I would rather define an upper limit in Xen (I would say 64 SPs) that > can be changed in the code (through a define). > > This call currently does not support preemption in the FF-A spec (and that is something > i will check for future versions) so I would have no solution to "continue" it. > Would the "define" solution be acceptable for now ? I think the define solution is acceptable for now and possibly longer. It is an easy way to avoid dealing with preemption. [...] >>> +static uint32_t ffa_get_vm_count(void) >> >> Is this meant to be called often? If so, can we instead have a counter that will be incremented when the vTEE is first initialized and then decremented when the VM is destroyed? > > As of now we could have a global counter that we increase or decrease > when a domain version is negociated and when a domain is destroyed. > > We could also have some kind of global save of the result to be returned > to a guest. > > But I did not do that because: > - cpu time required to update the list would be taken out an FF-A call > for FFA_VERSION of a VM which would require a global lock to protect > the data I would have thought an atomic counter would be sufficient. Is there anything you had in mind? > - when we will have filtering the data will be per VM (which would make > the initial update more complex) > - incoming we have a notion of UUID and filtering depending on the > requested UUID which will make the global value only useable in a > limited number of cases. > > I have 2 pending series on top of this one which would have to remove > such optimisations. > > At the end this is definitely not something supposed to call often (linux > driver is calling it once during init). I think it was a mistake for me to asked whether this is called often or not. When it comes to security, what matter is whether a malicious guest could indirectly call ffa_get_vm_count() and delay any work on the pCPU (Xen is not preemptible). We don't have to resolve this now. But this will need to be addressed before we can we consider FFA security supported. So we should at least add it in the list of issue at the top of the file. > >> >>> +{ >>> + struct domain *d = current->domain; >>> + struct domain *dom; >> >> NIT: "d" and "dom" are a bit too close. Could we rename "d" with "curr_d"? > > i will go with curr_d dest_d to make this clearer. > >> >>> + uint32_t vm_count = 0; >>> + >>> + /* Count the number of VM with FF-A support */ >> >> This comment implies this is including the current VM. But ... >> >>> + 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 ) >> >> ... here you are excluding it. Also, it sounds like this is support by the OS rather than the VM itself. Is that correct? > > I have a comment to explain that one in a different serie that i will put here. > > Basically before 1.2, the spec was a bit blurry on if or not the result should include the > calling VM and in fact Linux driver (before 1.2) was ending with an error if its own data > was included in the result hence this filter. Thanks for the clarification. Just to clarify... > > I will add a comment for that. ... will the comment be added in this patch? > >> >>> + vm_count++;> + } >>> + rcu_read_unlock(&domlist_read_lock); >>> +> + return vm_count; >> >> OOI, I guess this value is just used as an hint? Asking because the number of domains can change at any point. > > Definitely yes. The data is what it is when called but anything could change after. > > This is mostly used as hint by callers. Does this mean we would always return a fixed number? Asking because this would solve nicely the preemption problem in ffa_get_vm_count(). Cheers,
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..406c57b95f77 100644 --- a/xen/arch/arm/tee/ffa_partinfo.c +++ b/xen/arch/arm/tee/ffa_partinfo.c @@ -63,9 +63,156 @@ 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; + uint32_t 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_0) ) + { + 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, MIN(src_size, dst_size)); + if ( dst_size > src_size ) + memset(dst_buf + src_size, 0, dst_size - src_size); + + dst_buf += dst_size; + count++; + } + + src_buf += src_size; + } + + *sp_count = count; + +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; + uint32_t 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; + count++; + } + } + *vm_count = count; + +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 +222,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 +236,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 ) { - 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; + /* + * 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; + } - goto out; - } + if ( ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) ) + { + ret = ffa_get_sp_count(uuid, &ffa_sp_count); + if ( ret ) + goto out; + } - if ( flags ) - { - ret = FFA_RET_INVALID_PARAMETERS; - goto out; - } + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) ) + ffa_vm_count = ffa_get_vm_count(); - 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); - - if ( ret ) - goto out_rx_hyp_unlock; + ret = ffa_get_sp_partinfo(uuid, &ffa_sp_count, dst_buf, end_buf, + dst_size); - /* - * 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. - */ + if ( ret ) + goto out_rx_release; - /* 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; + dst_buf += ffa_sp_count * dst_size; } - if ( ctx->page_count * FFA_PAGE_SIZE < ffa_sp_count * dst_size ) - { - ret = FFA_RET_NO_MEMORY; - goto out_rx_hyp_release; - } + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) ) + ret = ffa_get_vm_partinfo(&ffa_vm_count, dst_buf, end_buf, 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--; - - 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)
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 <bertrand.marquis@arm.com> --- Changes in v4: - properly handle SPMC version 1.0 header size case in partinfo_get - switch to local counting variables instead of *pointer += 1 form - coding style issue with missing spaces in if () 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 | 274 +++++++++++++++++++++----------- xen/arch/arm/tee/ffa_private.h | 12 ++ 4 files changed, 218 insertions(+), 91 deletions(-)