Hi,
On Thu, Mar 20, 2025 at 4:47 PM Bertrand Marquis
<[email protected]> wrote:
>
> Hi Jens,
>
> Thanks a lot for the review.
>
> > On 20 Mar 2025, at 15:20, Jens Wiklander <[email protected]> wrote:
> >
> > Hi Bertrand,
> >
> > On Mon, Mar 10, 2025 at 3:51 PM 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.
> >>
> >> WARNING: There is no filtering for now and all VMs are listed !!
> >>
> >> Signed-off-by: Bertrand Marquis <[email protected]>
> >> ---
> >> 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.
>
> >
> >> + 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;
}
}
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
>
>
