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 15:00, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On Fri, Mar 21, 2025 at 2:47 PM Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx> wrote:
>> 
>> 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 ?
> 
> Yes, this is the way. :-)

Well in fact there is a mistake in this way and I had to do something
a bit different.

When we have a notification get, we can only clear secure_pending
if FFA_NOTIF_FLAG_BITMAP_SP(M) are passed.

So i think i have to do the following:
- secure_pending: set and clean as they are now
- vm_pending: set when raising buf full and clean in info_get or get
  with FFA_NOTIF_FLAG_BITMAP_HYP set
- buff_full_pending only cleaned in get with 
  FFA_NOTIF_FLAG_BITMAP_HYP set

This way info_get would still return something if a get is done but with
only flags to retrieve secure and there is a buff full notif pending or with
only HYP and secure ones pending.

Giving something like that:

--- a/xen/arch/arm/tee/ffa_notif.c
+++ b/xen/arch/arm/tee/ffa_notif.c
@@ -69,6 +69,7 @@ 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;

     if ( !notif_enabled )
     {
@@ -76,7 +77,11 @@ void ffa_handle_notification_info_get(struct cpu_user_regs 
*regs)
         return;
     }

-    if ( test_and_clear_bool(ctx->notif.secure_pending) )
+    notif_pending = test_and_clear_bool(ctx->notif.secure_pending);
+    if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
+        notif_pending |= test_and_clear_bool(ctx->notif.vm_pending)
+
+    if ( notif_pending )
     {
         /* A pending global notification for the guest */
         ffa_set_regs(regs, FFA_SUCCESS_64, 0,
@@ -153,11 +158,13 @@ void ffa_handle_notification_get(struct cpu_user_regs 
*regs)
             w6 = resp.a6;
     }

-#ifdef CONFIG_FFA_VM_TO_VM
-    if ( flags & FFA_NOTIF_FLAG_BITMAP_HYP &&
+    if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) &&
+          flags & FFA_NOTIF_FLAG_BITMAP_HYP &&
           test_and_clear_bool(ctx->notif.buff_full_pending) )
+    {
+        ACCESS_ONCE(ctx->notif.vm_pending) = false;
         w7 = FFA_NOTIF_RX_BUFFER_FULL;
-#endif
+    }

     ffa_set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, w4, w5, w6, w7);
 }
@@ -189,7 +196,8 @@ void ffa_raise_rx_buffer_full(struct domain *d)
     if ( !ctx )
         return;

-    if ( !test_and_set_bool(ctx->notif.buff_full_pending) )
+    ACCESS_ONCE(ctx->notif.buff_full_pending) = true
+    if ( !test_and_set_bool(ctx->notif.vm_pending) )
         vgic_inject_irq(d, d->vcpu[0], notif_sri_irq, true);
 }

Cheers
Bertrand