Hi Jens,
> On 21 Mar 2025, at 15:00, Jens Wiklander <jens.wiklan...@linaro.org> wrote:
>
> Hi Bertrand,
>
> On Fri, Mar 21, 2025 at 2:47 PM Bertrand Marquis
> <bertrand.marq...@arm.com> wrote:
>>
>> Hi Jens,
>>
>>> On 21 Mar 2025, at 11:09, Jens Wiklander <jens.wiklan...@linaro.org> wrote:
>>>
>>> Hi,
>>>
>>> On Fri, Mar 21, 2025 at 10:25 AM Bertrand Marquis
>>> <bertrand.marq...@arm.com> wrote:
>>>>
>>>> Hi Jens,
>>>>
>>>>> On 21 Mar 2025, at 09:55, Jens Wiklander <jens.wiklan...@linaro.org>
>>>>> wrote:
>>>>>
>>>>> Hi Bertrand,
>>>>>
>>>>> On Mon, Mar 10, 2025 at 3:51 PM 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 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.marq...@arm.com>
>>>>>> ---
>>>>>> 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
