Re: [PATCH v7 6/6] xen/arm: ffa: Enable VM to VM without firmware

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

A gentle ping for someone to review this :-)

Thanks
Bertrand

> On 17 Jul 2025, at 14:11, Bertrand Marquis <Bertrand.Marquis@xxxxxxx> wrote:
> 
> When VM to VM support is activated and there is no suitable FF-A support
> in the firmware, enable FF-A support for VMs to allow using it for VM to
> VM communications.
> If there is OP-TEE running in the secure world and using the non FF-A
> communication system, having CONFIG_FFA_VM_TO_VM could be non functional
> (if optee is probed first) or OP-TEE could be non functional (if FF-A is
> probed first) so it is not recommended to activate the configuration
> option for such systems.
> 
> To make buffer full notification work between VMs when there is no
> firmware, rework the notification handling and modify the global flag to
> only be used as check for firmware notification support instead.
> 
> Also split probe function into one for firmware and one for vm to vm to
> make the implementation clearer.
> 
> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> Reviewed-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> 
> ---
> Changes in v7:
> - add Jens R-b
> Changes in v6:
> - split probe into fw and vm_to_vm probe
> Changes in v5:
> - init ctx list when there is no firmware
> - rework init a bit to prevent duplicates
> - Remove Jens R-b due to changes done
> Changes in v4:
> - Fix Optee to OP-TEE in commit message
> - Add Jens R-b
> Changes in v3:
> - fix typos in commit message
> - add spaces around <<
> - move notification id fix back into buffer full patch
> - fix | position in if
> Changes in v2:
> - replace ifdef with IS_ENABLED when possible
> ---
> xen/arch/arm/tee/ffa.c       |  93 ++++++++++++++++++-------------
> xen/arch/arm/tee/ffa_notif.c | 104 ++++++++++++++++-------------------
> 2 files changed, 104 insertions(+), 93 deletions(-)
> 
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index be71eda4869f..df38b4e15b36 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -345,8 +345,9 @@ static int ffa_domain_init(struct domain *d)
>     struct ffa_ctx *ctx;
>     int ret;
> 
> -    if ( !ffa_fw_version )
> +    if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !ffa_fw_version )
>         return -ENODEV;
> +
>     /*
>      * We are using the domain_id + 1 as the FF-A ID for VMs as FF-A ID 0 is
>      * reserved for the hypervisor and we only support secure endpoints using
> @@ -477,38 +478,12 @@ static void ffa_init_secondary(void)
>     ffa_notif_init_interrupt();
> }
> 
> -static bool ffa_probe(void)
> +static bool ffa_probe_fw(void)
> {
>     uint32_t vers;
>     unsigned int major_vers;
>     unsigned int minor_vers;
> 
> -    /*
> -     * FF-A often works in units of 4K pages and currently it's assumed
> -     * that we can map memory using that granularity. See also the comment
> -     * above the FFA_PAGE_SIZE define.
> -     *
> -     * It is possible to support a PAGE_SIZE larger than 4K in Xen, but
> -     * until that is fully handled in this code make sure that we only use
> -     * 4K page sizes.
> -     */
> -    BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
> -
> -    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.
> @@ -527,11 +502,6 @@ static bool ffa_probe(void)
>         goto err_no_fw;
>     }
> 
> -    /* Some sanity check in case we update the version we support */
> -    BUILD_BUG_ON(FFA_MIN_SPMC_VERSION > FFA_MY_VERSION);
> -    BUILD_BUG_ON(FFA_VERSION_MAJOR(FFA_MIN_SPMC_VERSION) !=
> -                                   FFA_MY_VERSION_MAJOR);
> -
>     major_vers = FFA_VERSION_MAJOR(vers);
>     minor_vers = FFA_VERSION_MINOR(vers);
> 
> @@ -582,10 +552,6 @@ static bool ffa_probe(void)
>         goto err_rxtx_destroy;
> 
>     ffa_notif_init();
> -    INIT_LIST_HEAD(&ffa_teardown_head);
> -    INIT_LIST_HEAD(&ffa_ctx_head);
> -    rwlock_init(&ffa_ctx_list_rwlock);
> -    init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
> 
>     return true;
> 
> @@ -599,6 +565,59 @@ err_no_fw:
>     return false;
> }
> 
> +static bool ffa_probe_vm_to_vm(void)
> +{
> +    if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
> +        return false;
> +
> +    /*
> +     * 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);
> +
> +    return true;
> +}
> +
> +static bool ffa_probe(void)
> +{
> +    /*
> +     * FF-A often works in units of 4K pages and currently it's assumed
> +     * that we can map memory using that granularity. See also the comment
> +     * above the FFA_PAGE_SIZE define.
> +     *
> +     * It is possible to support a PAGE_SIZE larger than 4K in Xen, but
> +     * until that is fully handled in this code make sure that we only use
> +     * 4K page sizes.
> +     */
> +    BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
> +
> +    /* Some sanity check in case we update the version we support */
> +    BUILD_BUG_ON(FFA_MIN_SPMC_VERSION > FFA_MY_VERSION);
> +    BUILD_BUG_ON(FFA_VERSION_MAJOR(FFA_MIN_SPMC_VERSION) !=
> +                                   FFA_MY_VERSION_MAJOR);
> +
> +    printk(XENLOG_INFO "ARM FF-A Mediator version %u.%u\n",
> +           FFA_MY_VERSION_MAJOR, FFA_MY_VERSION_MINOR);
> +
> +    if ( !ffa_probe_fw() && !ffa_probe_vm_to_vm() )
> +        return false;
> +
> +    if ( !ffa_fw_version )
> +        printk(XENLOG_INFO "ARM FF-A only available between VMs\n");
> +
> +    INIT_LIST_HEAD(&ffa_teardown_head);
> +    INIT_LIST_HEAD(&ffa_ctx_head);
> +    rwlock_init(&ffa_ctx_list_rwlock);
> +    init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
> +
> +    return true;
> +}
> +
> static const struct tee_mediator_ops ffa_ops =
> {
>     .probe = ffa_probe,
> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
> index f6df2f15bb00..86bef6b3b2ab 100644
> --- a/xen/arch/arm/tee/ffa_notif.c
> +++ b/xen/arch/arm/tee/ffa_notif.c
> @@ -16,7 +16,7 @@
> 
> #include "ffa_private.h"
> 
> -static bool __ro_after_init notif_enabled;
> +static bool __ro_after_init fw_notif_enabled;
> static unsigned int __ro_after_init notif_sri_irq;
> 
> int ffa_handle_notification_bind(struct cpu_user_regs *regs)
> @@ -27,21 +27,17 @@ int ffa_handle_notification_bind(struct cpu_user_regs 
> *regs)
>     uint32_t bitmap_lo = get_user_reg(regs, 3);
>     uint32_t bitmap_hi = get_user_reg(regs, 4);
> 
> -    if ( !notif_enabled )
> -        return FFA_RET_NOT_SUPPORTED;
> -
>     if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
>         return FFA_RET_INVALID_PARAMETERS;
> 
>     if ( flags )    /* Only global notifications are supported */
>         return FFA_RET_DENIED;
> 
> -    /*
> -     * We only support notifications from SP so no need to check the sender
> -     * endpoint ID, the SPMC will take care of that for us.
> -     */
> -    return ffa_simple_call(FFA_NOTIFICATION_BIND, src_dst, flags, bitmap_lo,
> -                           bitmap_hi);
> +    if ( FFA_ID_IS_SECURE(src_dst >> 16) && fw_notif_enabled )
> +        return ffa_simple_call(FFA_NOTIFICATION_BIND, src_dst, flags,
> +                               bitmap_lo, bitmap_hi);
> +
> +    return FFA_RET_NOT_SUPPORTED;
> }
> 
> int ffa_handle_notification_unbind(struct cpu_user_regs *regs)
> @@ -51,18 +47,14 @@ int ffa_handle_notification_unbind(struct cpu_user_regs 
> *regs)
>     uint32_t bitmap_lo = get_user_reg(regs, 3);
>     uint32_t bitmap_hi = get_user_reg(regs, 4);
> 
> -    if ( !notif_enabled )
> -        return FFA_RET_NOT_SUPPORTED;
> -
>     if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
>         return FFA_RET_INVALID_PARAMETERS;
> 
> -    /*
> -     * We only support notifications from SP so no need to check the
> -     * destination endpoint ID, the SPMC will take care of that for us.
> -     */
> -    return  ffa_simple_call(FFA_NOTIFICATION_UNBIND, src_dst, 0, bitmap_lo,
> -                            bitmap_hi);
> +    if ( FFA_ID_IS_SECURE(src_dst >> 16) && fw_notif_enabled )
> +        return  ffa_simple_call(FFA_NOTIFICATION_UNBIND, src_dst, 0, 
> bitmap_lo,
> +                                bitmap_hi);
> +
> +    return FFA_RET_NOT_SUPPORTED;
> }
> 
> void ffa_handle_notification_info_get(struct cpu_user_regs *regs)
> @@ -71,7 +63,7 @@ void ffa_handle_notification_info_get(struct cpu_user_regs 
> *regs)
>     struct ffa_ctx *ctx = d->arch.tee;
>     bool notif_pending;
> 
> -    if ( !notif_enabled )
> +    if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !fw_notif_enabled )
>     {
>         ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>         return;
> @@ -108,7 +100,7 @@ void ffa_handle_notification_get(struct cpu_user_regs 
> *regs)
>     uint32_t w6 = 0;
>     uint32_t w7 = 0;
> 
> -    if ( !notif_enabled )
> +    if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !fw_notif_enabled )
>     {
>         ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>         return;
> @@ -120,7 +112,8 @@ void ffa_handle_notification_get(struct cpu_user_regs 
> *regs)
>         return;
>     }
> 
> -    if ( flags & ( FFA_NOTIF_FLAG_BITMAP_SP | FFA_NOTIF_FLAG_BITMAP_SPM ) )
> +    if ( fw_notif_enabled && (flags & ( FFA_NOTIF_FLAG_BITMAP_SP |
> +                                        FFA_NOTIF_FLAG_BITMAP_SPM )) )
>     {
>         struct arm_smccc_1_2_regs arg = {
>             .a0 = FFA_NOTIFICATION_GET,
> @@ -177,15 +170,14 @@ int ffa_handle_notification_set(struct cpu_user_regs 
> *regs)
>     uint32_t bitmap_lo = get_user_reg(regs, 3);
>     uint32_t bitmap_hi = get_user_reg(regs, 4);
> 
> -    if ( !notif_enabled )
> -        return FFA_RET_NOT_SUPPORTED;
> -
>     if ( (src_dst >> 16) != ffa_get_vm_id(d) )
>         return FFA_RET_INVALID_PARAMETERS;
> 
> -    /* Let the SPMC check the destination of the notification */
> -    return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags, bitmap_lo,
> -                           bitmap_hi);
> +    if ( FFA_ID_IS_SECURE(src_dst >> 16) && fw_notif_enabled )
> +        return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags, 
> bitmap_lo,
> +                               bitmap_hi);
> +
> +    return FFA_RET_NOT_SUPPORTED;
> }
> 
> #ifdef CONFIG_FFA_VM_TO_VM
> @@ -371,7 +363,7 @@ void ffa_notif_init_interrupt(void)
> {
>     int ret;
> 
> -    if ( notif_enabled && notif_sri_irq < NR_GIC_SGI )
> +    if ( fw_notif_enabled && notif_sri_irq < NR_GIC_SGI )
>     {
>         /*
>          * An error here is unlikely since the primary CPU has already
> @@ -402,41 +394,41 @@ void ffa_notif_init(void)
>     int ret;
> 
>     /* Only enable fw notification if all ABIs we need are supported */
> -    if ( !(ffa_fw_supports_fid(FFA_NOTIFICATION_BITMAP_CREATE) &&
> -           ffa_fw_supports_fid(FFA_NOTIFICATION_BITMAP_DESTROY) &&
> -           ffa_fw_supports_fid(FFA_NOTIFICATION_GET) &&
> -           ffa_fw_supports_fid(FFA_NOTIFICATION_INFO_GET_64)) )
> -        return;
> -
> -    arm_smccc_1_2_smc(&arg, &resp);
> -    if ( resp.a0 != FFA_SUCCESS_32 )
> -        return;
> -
> -    irq = resp.a2;
> -    notif_sri_irq = irq;
> -    if ( irq >= NR_GIC_SGI )
> -        irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
> -    ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL);
> -    if ( ret )
> +    if ( ffa_fw_supports_fid(FFA_NOTIFICATION_BITMAP_CREATE) &&
> +         ffa_fw_supports_fid(FFA_NOTIFICATION_BITMAP_DESTROY) &&
> +         ffa_fw_supports_fid(FFA_NOTIFICATION_GET) &&
> +         ffa_fw_supports_fid(FFA_NOTIFICATION_INFO_GET_64) )
>     {
> -        printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> -               irq, ret);
> -        return;
> -    }
> +        arm_smccc_1_2_smc(&arg, &resp);
> +        if ( resp.a0 != FFA_SUCCESS_32 )
> +            return;
> 
> -    notif_enabled = true;
> +        irq = resp.a2;
> +        notif_sri_irq = irq;
> +        if ( irq >= NR_GIC_SGI )
> +            irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
> +        ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL);
> +        if ( ret )
> +        {
> +            printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> +                   irq, ret);
> +            return;
> +        }
> +        fw_notif_enabled = true;
> +    }
> }
> 
> int ffa_notif_domain_init(struct domain *d)
> {
>     int32_t res;
> 
> -    if ( !notif_enabled )
> -        return 0;
> +    if ( fw_notif_enabled )
> +    {
> 
> -    res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
> -    if ( res )
> -        return -ENOMEM;
> +        res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
> +        if ( res )
> +            return -ENOMEM;
> +    }
> 
>     return 0;
> }
> @@ -447,6 +439,6 @@ void ffa_notif_domain_destroy(struct domain *d)
>      * Call bitmap_destroy even if bitmap create failed as the SPMC will
>      * return a DENIED error that we will ignore.
>      */
> -    if ( notif_enabled )
> +    if ( fw_notif_enabled )
>         ffa_notification_bitmap_destroy(ffa_get_vm_id(d));
> }
> -- 
> 2.47.1
> 
>