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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 24 Mar 2025, at 11:24, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> On Mon, Mar 24, 2025 at 10:15 AM 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.
>> 
>> 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@xxxxxxx>
>> ---
>> 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.

You are right this should not be here.

v1.0 structure size should be ok, i will just need to memset to 0 the UUID
part if destination size is bigger.

I will fix that.

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

This is why i did not use the ++ form.

I will switch to a local variable if you think this is better.

Cheers
Bertrand


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