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:30, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi,
> 
> On Thu, May 22, 2025 at 10:18 AM Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx> wrote:
>> 
>> 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.
> 
> I meant the "else" isn't needed, the "if" is still needed, as you explain.
> 
>> 
>> 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.
> 
> Yes, this is an improvement. A return at the end of an if block makes
> the eventual following "else" redundant. I suppose there are coding
> styles where it's still preferred. I'm not sure if that applies in
> Xen, though.

I now get what you mean and you would like the return false to be in a else.

Relooking at the code, i actually do not like the fact that we do so much in
exit and i think i can move a bit the code to be clearer:
- put the fw init in a sub function
- create a vm_to_vm init function
- in probe call both functions and do the common init part if at least one 
succeded

Something like this:
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 57b648a22840..42dfc71a12d7 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -478,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.
@@ -528,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);

@@ -584,7 +553,7 @@ static bool ffa_probe(void)

     ffa_notif_init();

-    goto exit;
+    return true;

 err_rxtx_destroy:
     ffa_rxtx_destroy();
@@ -593,23 +562,59 @@ 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);
-    }
+    return false;
+}

-    if ( IS_ENABLED(CONFIG_FFA_VM_TO_VM) )
-    {
+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");
-        return true;
-    }
-    else if ( ffa_fw_version )
-        return true;

-    return false;
+    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;
 }

 static const struct tee_mediator_ops ffa_ops =

I think this makes the code cleaner as we also get the proper error handling 
with goto
inside the fw probe instead of the previous one which was trying to handle both 
cases.

What do you think ?

Cheers
Bertrand