Hi Jens,
> On 24 Mar 2025, at 11:48, Jens Wiklander <jens.wiklan...@linaro.org> wrote:
>
> Hi Bertrand,
>
> On Mon, Mar 24, 2025 at 10:15 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 Optee could be non functional (if FF-A is
>
> Prefer OP-TEE
Right i fixed the first instance but not the second one.
Will fix in the next version.
>
>> 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 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 | 12 +++-
>> xen/arch/arm/tee/ffa_notif.c | 104 ++++++++++++++++-------------------
>> 2 files changed, 59 insertions(+), 57 deletions(-)
>
> Reviewed-by: Jens Wiklander <jens.wiklan...@linaro.org>
Thanks
Bertrand
>
> Cheers,
> Jens
>
>>
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index e41ab5f8ada6..0627625efe60 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
>> @@ -561,6 +562,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 f6df2f15bb00..86bef6b3b2ab 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 )
>> + 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,18 +47,14 @@ 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)
>> @@ -71,7 +63,7 @@ void ffa_handle_notification_info_get(struct cpu_user_regs
>> *regs)
>> struct ffa_ctx *ctx = d->arch.tee;
>> bool notif_pending;
>>
>> - if ( !notif_enabled )
>> + if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !fw_notif_enabled )
>> {
>> ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>> return;
>> @@ -108,7 +100,7 @@ void ffa_handle_notification_get(struct cpu_user_regs
>> *regs)
>> uint32_t w6 = 0;
>> uint32_t w7 = 0;
>>
>> - if ( !notif_enabled )
>> + if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) && !fw_notif_enabled )
>> {
>> ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>> return;
>> @@ -120,7 +112,8 @@ void ffa_handle_notification_get(struct cpu_user_regs
>> *regs)
>> return;
>> }
>>
>> - if ( flags & ( FFA_NOTIF_FLAG_BITMAP_SP | FFA_NOTIF_FLAG_BITMAP_SPM ) )
>> + if ( fw_notif_enabled && (flags & ( FFA_NOTIF_FLAG_BITMAP_SP |
>> + FFA_NOTIF_FLAG_BITMAP_SPM )) )
>> {
>> struct arm_smccc_1_2_regs arg = {
>> .a0 = FFA_NOTIFICATION_GET,
>> @@ -177,15 +170,14 @@ int ffa_handle_notification_set(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 >> 16) != ffa_get_vm_id(d) )
>> return FFA_RET_INVALID_PARAMETERS;
>>
>> - /* Let the SPMC check the destination of the notification */
>> - return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags, bitmap_lo,
>> - bitmap_hi);
>> + if ( FFA_ID_IS_SECURE(src_dst >> 16) && fw_notif_enabled )
>> + return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags,
>> bitmap_lo,
>> + bitmap_hi);
>> +
>> + return FFA_RET_NOT_SUPPORTED;
>> }
>>
>> #ifdef CONFIG_FFA_VM_TO_VM
>> @@ -371,7 +363,7 @@ void ffa_notif_init_interrupt(void)
>> {
>> int ret;
>>
>> - if ( notif_enabled && notif_sri_irq < NR_GIC_SGI )
>> + if ( fw_notif_enabled && notif_sri_irq < NR_GIC_SGI )
>> {
>> /*
>> * An error here is unlikely since the primary CPU has already
>> @@ -402,41 +394,41 @@ void ffa_notif_init(void)
>> int ret;
>>
>> /* Only enable fw notification if all ABIs we need are supported */
>> - if ( !(ffa_fw_supports_fid(FFA_NOTIFICATION_BITMAP_CREATE) &&
>> - ffa_fw_supports_fid(FFA_NOTIFICATION_BITMAP_DESTROY) &&
>> - ffa_fw_supports_fid(FFA_NOTIFICATION_GET) &&
>> - ffa_fw_supports_fid(FFA_NOTIFICATION_INFO_GET_64)) )
>> - return;
>> -
>> - arm_smccc_1_2_smc(&arg, &resp);
>> - if ( resp.a0 != FFA_SUCCESS_32 )
>> - return;
>> -
>> - irq = resp.a2;
>> - notif_sri_irq = irq;
>> - if ( irq >= NR_GIC_SGI )
>> - irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
>> - ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL);
>> - if ( ret )
>> + if ( ffa_fw_supports_fid(FFA_NOTIFICATION_BITMAP_CREATE) &&
>> + ffa_fw_supports_fid(FFA_NOTIFICATION_BITMAP_DESTROY) &&
>> + ffa_fw_supports_fid(FFA_NOTIFICATION_GET) &&
>> + ffa_fw_supports_fid(FFA_NOTIFICATION_INFO_GET_64) )
>> {
>> - printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
>> - irq, ret);
>> - return;
>> - }
>> + arm_smccc_1_2_smc(&arg, &resp);
>> + if ( resp.a0 != FFA_SUCCESS_32 )
>> + return;
>>
>> - notif_enabled = true;
>> + irq = resp.a2;
>> + notif_sri_irq = irq;
>> + if ( irq >= NR_GIC_SGI )
>> + irq_set_type(irq, IRQ_TYPE_EDGE_RISING);
>> + ret = request_irq(irq, 0, notif_irq_handler, "FF-A notif", NULL);
>> + if ( ret )
>> + {
>> + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
>> + irq, ret);
>> + return;
>> + }
>> + fw_notif_enabled = true;
>> + }
>> }
>>
>> int ffa_notif_domain_init(struct domain *d)
>> {
>> int32_t res;
>>
>> - if ( !notif_enabled )
>> - return 0;
>> + if ( fw_notif_enabled )
>> + {
>>
>> - res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
>> - if ( res )
>> - return -ENOMEM;
>> + res = ffa_notification_bitmap_create(ffa_get_vm_id(d),
>> d->max_vcpus);
>> + if ( res )
>> + return -ENOMEM;
>> + }
>>
>> return 0;
>> }
>> @@ -447,6 +439,6 @@ void ffa_notif_domain_destroy(struct domain *d)
>> * Call bitmap_destroy even if bitmap create failed as the SPMC will
>> * return a DENIED error that we will ignore.
>> */
>> - if ( notif_enabled )
>> + if ( fw_notif_enabled )
>> ffa_notification_bitmap_destroy(ffa_get_vm_id(d));
>> }
>> --
>> 2.47.1
