[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 2/5] xen/arm: ffa: Introduce VM to VM support
Hi Jens, > On 20 Mar 2025, at 18:06, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote: > > Hi, > > On Thu, Mar 20, 2025 at 4:47 PM Bertrand Marquis > <Bertrand.Marquis@xxxxxxx> wrote: >> >> Hi Jens, >> >> Thanks a lot for the review. >> >>> On 20 Mar 2025, at 15:20, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote: >>> >>> Hi Bertrand, >>> >>> On Mon, Mar 10, 2025 at 3:51 PM Bertrand Marquis >>> <bertrand.marquis@xxxxxxx> 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. >>>> >>>> WARNING: There is no filtering for now and all VMs are listed !! >>>> >>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx> >>>> --- >>>> 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_partinfo.c | 144 +++++++++++++++++++++++++------- >>>> xen/arch/arm/tee/ffa_private.h | 12 +++ >>>> 3 files changed, 137 insertions(+), 30 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_partinfo.c >>>> b/xen/arch/arm/tee/ffa_partinfo.c >>>> index c0510ceb8338..7af1eca2d0c4 100644 >>>> --- a/xen/arch/arm/tee/ffa_partinfo.c >>>> +++ b/xen/arch/arm/tee/ffa_partinfo.c >>>> @@ -77,7 +77,23 @@ void ffa_handle_partition_info_get(struct cpu_user_regs >>>> *regs) >>>> }; >>>> uint32_t src_size, dst_size; >>>> void *dst_buf; >>>> - uint32_t ffa_sp_count = 0; >>>> + uint32_t ffa_vm_count = 0, ffa_sp_count = 0; >>>> + >>> >>> This function is getting quite large and it's hard to follow the >>> different lock states. How about splitting it into various helper >>> functions? >> >> Yes I agree. >> I will try to find a good way to split this in smaller chunks. >> >>> >>>> + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) ) >>>> + { >>>> + struct domain *dom; >>>> + >>>> + /* Count the number of VM with FF-A support */ >>> >>> Why do we need this now? Isn't it enough to count them below when we >>> fill in dst_buf? >> >> We need it in 3 places in the code after: >> - to return the number of endpoint in case the flag is set (see final return) >> - to check that there is enough space in the RX buffer >> - to prevent going through the list of domains if none is to be listed anyway > > Got it, thanks. The first point is important, the others are > essentially optimizations. With your previous point and me splitting things this will not be the case anymore. > >> >>> >>>> + 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) >>>> + ffa_vm_count++; >>>> + } >>>> + rcu_read_unlock(&domlist_read_lock); >>>> + } >>>> >>>> /* >>>> * If the guest is v1.0, he does not get back the entry size so we must >>>> @@ -127,33 +143,38 @@ void ffa_handle_partition_info_get(struct >>>> cpu_user_regs *regs) >>>> >>>> dst_buf = ctx->rx; >>>> >>>> - if ( !ffa_rx ) >>>> + /* If not supported, we have ffa_sp_count=0 */ >>>> + if ( ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) ) >>>> { >>>> - ret = FFA_RET_DENIED; >>>> - goto out_rx_release; >>>> - } >>>> + if ( !ffa_rx ) >>>> + { >>>> + ret = FFA_RET_DENIED; >>>> + goto out_rx_release; >>>> + } >>>> >>>> - spin_lock(&ffa_rx_buffer_lock); >>>> + spin_lock(&ffa_rx_buffer_lock); >>>> >>>> - ret = ffa_partition_info_get(uuid, 0, &ffa_sp_count, &src_size); >>>> + ret = ffa_partition_info_get(uuid, 0, &ffa_sp_count, &src_size); >>>> >>>> - if ( ret ) >>>> - goto out_rx_hyp_unlock; >>>> + if ( ret ) >>>> + goto out_rx_hyp_unlock; >>>> >>>> - /* >>>> - * 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. >>>> - */ >>>> + /* >>>> + * 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; >>>> + /* 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 ) >>>> + if ( ctx->page_count * FFA_PAGE_SIZE < >>>> + (ffa_sp_count + ffa_vm_count) * dst_size ) >>>> { >>>> ret = FFA_RET_NO_MEMORY; >>>> goto out_rx_hyp_release; >>>> @@ -182,25 +203,88 @@ void ffa_handle_partition_info_get(struct >>>> cpu_user_regs *regs) >>>> } >>>> } >>>> >>>> + if ( ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) ) >>>> + { >>>> + ffa_hyp_rx_release(); >>>> + spin_unlock(&ffa_rx_buffer_lock); >>>> + } >>>> + >>>> + if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) && ffa_vm_count ) >>>> + { >>>> + struct domain *dom; >>>> + uint32_t curr = 0; >>>> + >>>> + /* add the VM informations if any */ >>>> + 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 ( curr == ffa_vm_count ) >>>> + { >>>> + /* >>>> + * The number of domains changed since we counted >>>> them, we >>>> + * can add new ones if there is enough space in the >>>> + * destination buffer so check it or go out with an >>>> error. >>>> + */ >>> >>> Why do we care if the number has changed? If it fits, all is good >>> anyway and we're also updating ffa_vm_count with curr after the loop. >> >> Well this is exactly the point here, we check if we have enough space to >> return the data with the new domains and if not we return an error. >> >> The point here is to make sure that if there are more domains than when >> we counted first we check that we have enough size and the update at the >> end is to handle the case where we have actually less domains than when >> we counted first. >> >> If the number has changed we only care because we need to make sure >> we have enough space and because we need to return the right number >> to the caller. >> >> Please tell me how you would like me to change this because I do not >> understand what I should modify here. > > I was thinking something like: > for_each_domain( dom ) > { > struct ffa_ctx *vm = dom->arch.tee; > > if (dom != d && vm != NULL && vm->guest_vers != 0) > { > ... > ffa_vm_count++; > if ( ctx->page_count * FFA_PAGE_SIZE < > (ffa_sp_count + ffa_vm_count) * dst_size ) > ... error out > > ... copy data > dst_buf += dst_size; > } > } Ok got you. I will modify things to simplify and just check along the way if there is enough space to add an extra entry instead of trying to handle a corner case where the number changes in the middle that will make things simpler. Stay tuned for v3 ;-) Cheers Bertrand > > Cheers, > Jens > >> >> Cheers >> Bertrand >> >>> >>> Cheers, >>> Jens >>> >>>> + ffa_vm_count++; >>>> + if ( ctx->page_count * FFA_PAGE_SIZE < >>>> + (ffa_sp_count + ffa_vm_count) * dst_size ) >>>> + { >>>> + ret = FFA_RET_NO_MEMORY; >>>> + rcu_read_unlock(&domlist_read_lock); >>>> + goto out_rx_release; >>>> + } >>>> + } >>>> + >>>> + 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; >>>> + curr++; >>>> + } >>>> + } >>>> + rcu_read_unlock(&domlist_read_lock); >>>> + >>>> + /* the number of domains could have reduce since the initial >>>> count */ >>>> + ffa_vm_count = curr; >>>> + } >>>> + >>>> + goto out; >>>> + >>>> 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 ) >>>> - ffa_rx_release(d); >>>> + 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |