Re: [PATCH v2 5/5] xen/arm: ffa: Enable VM to VM without firmware

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 21 Mar 2025, at 11:09, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi,
> 
> On Fri, Mar 21, 2025 at 10:25 AM Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx> wrote:
>> 
>> Hi Jens,
>> 
>>> On 21 Mar 2025, at 09:55, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On Mon, Mar 10, 2025 at 3:51 PM 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 Optee running in the secure world and using the non FF-A
>>> 
>>> It's spelled OP-TEE in text, and optee or OPTEE in identifiers.
>> 
>> ack
>> 
>>> 
>>>> communication system, having CONFIG_FFA_VM_TO_VM could be non functional
>>>> (if optee is probed first) or Optee 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 not
>>> 
>>> s/not/no/
>> 
>> ack
>> 
>>> 
>>>> firmware, rework the notification handling and modify the global flag to
>>>> only be used as check for firmware notification support instead.
>>>> 
>>>> Modify part_info_get to return the list of VMs when there is no firmware
>>>> support.
>>>> 
>>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>>>> ---
>>>> Changes in v2:
>>>> - replace ifdef with IS_ENABLED when possible
>>>> ---
>>>> xen/arch/arm/tee/ffa.c          |  12 +++-
>>>> xen/arch/arm/tee/ffa_notif.c    | 114 ++++++++++++++++----------------
>>>> xen/arch/arm/tee/ffa_partinfo.c |   3 +-
>>>> 3 files changed, 69 insertions(+), 60 deletions(-)
>>>> 
>>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>>>> index 3bbdd7168a6b..f6582d2e855a 100644
>>>> --- a/xen/arch/arm/tee/ffa.c
>>>> +++ b/xen/arch/arm/tee/ffa.c
>>>> @@ -324,8 +324,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
>>>> @@ -549,6 +550,15 @@ err_no_fw:
>>>>    bitmap_zero(ffa_fw_abi_supported, FFA_ABI_BITMAP_SIZE);
>>>>    printk(XENLOG_WARNING "ARM FF-A No firmware support\n");
>>>> 
>>>> +    if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
>>>> +    {
>>>> +        INIT_LIST_HEAD(&ffa_teardown_head);
>>>> +        init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, 
>>>> NULL, 0);
>>>> +
>>>> +        printk(XENLOG_INFO "ARM FF-A only available between VMs\n");
>>>> +        return true;
>>>> +    }
>>>> +
>>>>    return false;
>>>> }
>>>> 
>>>> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
>>>> index d19aa5c5bef6..0673e53a9def 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 )
>>> 
>>> Please add space before and after '>>', here and in the function below
>>> in this patch.
>> 
>> ack
>> 
>>> 
>>>> +        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,32 +47,34 @@ 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)
>>>> {
>>>>    struct domain *d = current->domain;
>>>>    struct ffa_ctx *ctx = d->arch.tee;
>>>> +    bool notif_pending = false;
>>>> 
>>>> -    if ( !notif_enabled )
>>>> +    if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !fw_notif_enabled )
>>>>    {
>>>>        ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>>>        return;
>>>>    }
>>>> 
>>>> -    if ( test_and_clear_bool(ctx->notif.secure_pending) )
>>>> +    notif_pending = ctx->notif.secure_pending;
>>>> +#ifdef CONFIG_FFA_VM_TO_VM
>>>> +    notif_pending |= ctx->notif.buff_full_pending;
>>>> +#endif
>>> 
>>> Shouldn't ctx->notif.secure_pending and ctx->notif.secure_pending be
>>> cleared also, like:
>>> notif_pending = test_and_clear_bool(ctx->notif.secure_pending) ||
>>>               test_and_clear_bool(ctx->notif.buff_full_pending);
>> 
>> Notification info get is returning who has pending notification but only
>> notification get should erase pending notifications.
> 
> FFA_NOTIFICATION_INFO_GET can return a "More pending notifications
> flag" in w2/x2 to inform the caller that it should call
> FFA_NOTIFICATION_INFO_GET again to get the remaining receiver
> endpoints. How can the ABI know where to resume the next time if the
> previous pending receivers aren't cleared?

I just checked the specification and you are right.
It is explicitly saying that "Information about pending notifications is
returned only once".

> 
> The more pending notifications flag will not be needed here as we're
> dealing with a single endpoint at a time so it might be more of a
> philosophical question. I don't think it causes problems for the guest
> to keep secure_pending unchanged for FFA_NOTIFICATION_INFO_GET, but we
> should mention the changed behaviour in the commit message.
> 

I agree I should discard the secure_pending flag in the implementation but
I need to find a solution for the buffer full notification as I still need to 
signal
it in notification get even if notification info get was called.

I will do the following:
- change secure_pending into pending_notif.
- set pending_notif when current secure_pending is set
- set pending_notif and buff_full_pending on indirect message
- clean pending_notif in notif_info_get
- clean pending_notif and buff_full in notif_get

Do you agree this is the right path ?

Cheers
Bertrand

> Cheers,
> Jens
> 
>> 
>>> 
>>>> +
>>>> +    if ( notif_pending )
>>>>    {
>>>>        /* A pending global notification for the guest */
>>>>        ffa_set_regs(regs, FFA_SUCCESS_64, 0,
>>>> @@ -103,7 +101,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;
>>>> @@ -115,7 +113,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 )) )
>>> 
>>> Please end the previous line with the '|' operator.
>> 
>> ack
>> 
>>> 
>>>>    {
>>>>        struct arm_smccc_1_2_regs arg = {
>>>>            .a0 = FFA_NOTIFICATION_GET,
>>>> @@ -170,15 +169,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
>>>> @@ -190,7 +188,7 @@ void ffa_raise_rx_buffer_full(struct domain *d)
>>>>        return;
>>>> 
>>>>    if ( !test_and_set_bool(ctx->notif.buff_full_pending) )
>>>> -        vgic_inject_irq(d, d->vcpu[0], notif_sri_irq, true);
>>>> +        vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID, 
>>>> true);
>>> 
>>> Please move this to the patch "xen/arm: ffa: Add buffer full
>>> notification support"
>> 
>> Ack
>> 
>>> 
>>>> }
>>>> #endif
>>>> 
>>>> @@ -363,7 +361,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
>>>> @@ -394,41 +392,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;
>>>> }
>>>> @@ -439,6 +437,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));
>>>> }
>>>> diff --git a/xen/arch/arm/tee/ffa_partinfo.c 
>>>> b/xen/arch/arm/tee/ffa_partinfo.c
>>>> index 7af1eca2d0c4..291396c8f635 100644
>>>> --- a/xen/arch/arm/tee/ffa_partinfo.c
>>>> +++ b/xen/arch/arm/tee/ffa_partinfo.c
>>>> @@ -130,7 +130,8 @@ void ffa_handle_partition_info_get(struct 
>>>> cpu_user_regs *regs)
>>>>        goto out;
>>>>    }
>>>> 
>>>> -    if ( !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
>>>> +    if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) &&
>>> 
>>> Doesn't this belong in the patch "xen/arm: ffa: Introduce VM to VM support"?
>>> And wouldn't ffa_vm_count make more sense?
>> 
>> ack. I will modify that and fold it into the VM to VM support patch.
>> 
>> Cheers
>> Bertrand
>> 
>>> 
>>> Cheers,
>>> Jens
>>> 
>>>> +         !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
>>>>    {
>>>>        /* Just give an empty partition list to the caller */
>>>>        ret = FFA_RET_OK;
>>>> --
>>>> 2.47.1