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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jens,

> On 22 May 2025, at 10:00, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On Wed, Apr 16, 2025 at 9:40 AM 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.
>> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>> 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       |  24 ++++++--
>> xen/arch/arm/tee/ffa_notif.c | 104 ++++++++++++++++-------------------
>> 2 files changed, 67 insertions(+), 61 deletions(-)
>> 
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index c1c4c0957091..b86c88cefa8c 100644
>> --- a/xen/arch/arm/tee/ffa.c
>> +++ b/xen/arch/arm/tee/ffa.c
>> @@ -342,8 +342,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
>> @@ -579,11 +580,8 @@ static bool ffa_probe(void)
>>         goto err_rxtx_destroy;
>> 
>>     ffa_notif_init();
>> -    INIT_LIST_HEAD(&ffa_teardown_head);
>> -    INIT_LIST_HEAD(&ffa_ctx_head);
>> -    init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
>> 
>> -    return true;
>> +    goto exit;
>> 
>> err_rxtx_destroy:
>>     ffa_rxtx_destroy();
>> @@ -592,6 +590,22 @@ err_no_fw:
>>     bitmap_zero(ffa_fw_abi_supported, FFA_ABI_BITMAP_SIZE);
>>     printk(XENLOG_WARNING "ARM FF-A No firmware support\n");
>> 
>> +exit:
>> +    if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) || ffa_fw_version )
>> +    {
>> +        INIT_LIST_HEAD(&ffa_teardown_head);
>> +        INIT_LIST_HEAD(&ffa_ctx_head);
>> +        init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 
>> 0);
>> +    }
>> +
>> +    if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
>> +    {
>> +        printk(XENLOG_INFO "ARM FF-A only available between VMs\n");
> 
> This should only be printed if ffa_fw_version == 0

Right i will fix but ...

> 
>> +        return true;
>> +    }
>> +    else if ( ffa_fw_version )
> 
> The else isn't needed.

the else is needed so that we return true and not false.

We have 3 cases:
- firmware is there: return true
- firmware not there but vm to vm enable: return true
- otherwise: return false

I will modify it like this to make it clearer:
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 57b648a22840..768b4e9ec968 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -601,13 +601,13 @@ exit:
         init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
     }

-    if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
+    if ( ffa_fw_version )
+        return true;
+    else if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
     {
         printk(XENLOG_INFO "ARM FF-A only available between VMs\n");
         return true;
     }
-    else if ( ffa_fw_version )
-        return true;

     return false;
 }

Tell me if you agree.

Cheers
Bertrand