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

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi Bertrand,

On Mon, Mar 24, 2025 at 2:58 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi,
>
> > On 24 Mar 2025, at 14:53, 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 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@xxxxxxxxxx>

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
>