Hi Jens,
> On 22 May 2025, at 14:00, Jens Wiklander <jens.wiklan...@linaro.org> wrote:
>
> Hi Bertrand,
>
> On Thu, May 22, 2025 at 11:11 AM Bertrand Marquis
> <bertrand.marq...@arm.com> wrote:
>>
>> Hi Jens,
>>
>>> On 22 May 2025, at 10:30, Jens Wiklander <jens.wiklan...@linaro.org> wrote:
>>>
>>> Hi,
>>>
>>> On Thu, May 22, 2025 at 10:18 AM Bertrand Marquis
>>> <bertrand.marq...@arm.com> wrote:
>>>>
>>>> Hi Jens,
>>>>
>>>>> On 22 May 2025, at 10:00, Jens Wiklander <jens.wiklan...@linaro.org>
>>>>> wrote:
>>>>>
>>>>> Hi Bertrand,
>>>>>
>>>>> On Wed, Apr 16, 2025 at 9:40 AM Bertrand Marquis
>>>>> <bertrand.marq...@arm.com> 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.marq...@arm.com>
>>>>>> ---
>>>>>> 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
