Re: [PATCH v2 2/5] xen/arm: ffa: Introduce VM to VM support

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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

> 
>> +        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.

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