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 14:00, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On Thu, May 22, 2025 at 11:11 AM Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx> wrote:
>> 
>> 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
> 
> I was starting to think along those lines, too. :-)
> 
>> 
>> 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 ?
> 
> This is good. It's much easier to follow.

Great, I will put that in v6.

Cheers
Bertrand

> 
> Cheers,
> Jens