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
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
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.marquis@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 )
+ 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
+
+ if ( notif_pending )
{
/* A pending global notification for the guest */
ffa_set_regs(regs, FFA_SUCCESS_64, 0,
@@ -103,7 +101,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;
@@ -115,7 +113,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,
@@ -170,15 +169,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
@@ -190,7 +188,7 @@ void ffa_raise_rx_buffer_full(struct domain *d)
return;
if ( !test_and_set_bool(ctx->notif.buff_full_pending) )
- vgic_inject_irq(d, d->vcpu[0], notif_sri_irq, true);
+ vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID, true);
}
#endif
@@ -363,7 +361,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
@@ -394,41 +392,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;
}
@@ -439,6 +437,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));
}
diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
index 7af1eca2d0c4..291396c8f635 100644
--- a/xen/arch/arm/tee/ffa_partinfo.c
+++ b/xen/arch/arm/tee/ffa_partinfo.c
@@ -130,7 +130,8 @@ void ffa_handle_partition_info_get(struct cpu_user_regs *regs)
goto out;
}
- if ( !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
+ if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) &&
+ !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
{
/* Just give an empty partition list to the caller */
ret = FFA_RET_OK;
--
2.47.1Hi Bertrand,
On Mon, Mar 10, 2025 at 3:51 PM Bertrand Marquis
<bertrand.marquis@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.
> 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/
> 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.marquis@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.
> + 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);
> +
> + if ( notif_pending )
> {
> /* A pending global notification for the guest */
> ffa_set_regs(regs, FFA_SUCCESS_64, 0,
> @@ -103,7 +101,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;
> @@ -115,7 +113,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 )) )
Please end the previous line with the '|' operator.
> {
> struct arm_smccc_1_2_regs arg = {
> .a0 = FFA_NOTIFICATION_GET,
> @@ -170,15 +169,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
> @@ -190,7 +188,7 @@ void ffa_raise_rx_buffer_full(struct domain *d)
> return;
>
> if ( !test_and_set_bool(ctx->notif.buff_full_pending) )
> - vgic_inject_irq(d, d->vcpu[0], notif_sri_irq, true);
> + vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID, true);
Please move this to the patch "xen/arm: ffa: Add buffer full
notification support"
> }
> #endif
>
> @@ -363,7 +361,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
> @@ -394,41 +392,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;
> }
> @@ -439,6 +437,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));
> }
> diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
> index 7af1eca2d0c4..291396c8f635 100644
> --- a/xen/arch/arm/tee/ffa_partinfo.c
> +++ b/xen/arch/arm/tee/ffa_partinfo.c
> @@ -130,7 +130,8 @@ void ffa_handle_partition_info_get(struct cpu_user_regs *regs)
> goto out;
> }
>
> - if ( !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
> + if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) &&
Doesn't this belong in the patch "xen/arm: ffa: Introduce VM to VM support"?
And wouldn't ffa_vm_count make more sense?
Cheers,
Jens
> + !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
> {
> /* Just give an empty partition list to the caller */
> ret = FFA_RET_OK;
> --
> 2.47.1
>
Hi Jens,
> On 21 Mar 2025, at 09:55, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Bertrand,
>
> On Mon, Mar 10, 2025 at 3:51 PM Bertrand Marquis
> <bertrand.marquis@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.marquis@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.
>
>> +
>> + if ( notif_pending )
>> {
>> /* A pending global notification for the guest */
>> ffa_set_regs(regs, FFA_SUCCESS_64, 0,
>> @@ -103,7 +101,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;
>> @@ -115,7 +113,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 )) )
>
> Please end the previous line with the '|' operator.
ack
>
>> {
>> struct arm_smccc_1_2_regs arg = {
>> .a0 = FFA_NOTIFICATION_GET,
>> @@ -170,15 +169,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
>> @@ -190,7 +188,7 @@ void ffa_raise_rx_buffer_full(struct domain *d)
>> return;
>>
>> if ( !test_and_set_bool(ctx->notif.buff_full_pending) )
>> - vgic_inject_irq(d, d->vcpu[0], notif_sri_irq, true);
>> + vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID, true);
>
> Please move this to the patch "xen/arm: ffa: Add buffer full
> notification support"
Ack
>
>> }
>> #endif
>>
>> @@ -363,7 +361,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
>> @@ -394,41 +392,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;
>> }
>> @@ -439,6 +437,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));
>> }
>> diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
>> index 7af1eca2d0c4..291396c8f635 100644
>> --- a/xen/arch/arm/tee/ffa_partinfo.c
>> +++ b/xen/arch/arm/tee/ffa_partinfo.c
>> @@ -130,7 +130,8 @@ void ffa_handle_partition_info_get(struct cpu_user_regs *regs)
>> goto out;
>> }
>>
>> - if ( !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
>> + if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) &&
>
> Doesn't this belong in the patch "xen/arm: ffa: Introduce VM to VM support"?
> And wouldn't ffa_vm_count make more sense?
ack. I will modify that and fold it into the VM to VM support patch.
Cheers
Bertrand
>
> Cheers,
> Jens
>
>> + !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
>> {
>> /* Just give an empty partition list to the caller */
>> ret = FFA_RET_OK;
>> --
>> 2.47.1
Hi,
On Fri, Mar 21, 2025 at 10:25 AM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Jens,
>
> > On 21 Mar 2025, at 09:55, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Bertrand,
> >
> > On Mon, Mar 10, 2025 at 3:51 PM Bertrand Marquis
> > <bertrand.marquis@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.marquis@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?
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.
Cheers,
Jens
>
> >
> >> +
> >> + if ( notif_pending )
> >> {
> >> /* A pending global notification for the guest */
> >> ffa_set_regs(regs, FFA_SUCCESS_64, 0,
> >> @@ -103,7 +101,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;
> >> @@ -115,7 +113,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 )) )
> >
> > Please end the previous line with the '|' operator.
>
> ack
>
> >
> >> {
> >> struct arm_smccc_1_2_regs arg = {
> >> .a0 = FFA_NOTIFICATION_GET,
> >> @@ -170,15 +169,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
> >> @@ -190,7 +188,7 @@ void ffa_raise_rx_buffer_full(struct domain *d)
> >> return;
> >>
> >> if ( !test_and_set_bool(ctx->notif.buff_full_pending) )
> >> - vgic_inject_irq(d, d->vcpu[0], notif_sri_irq, true);
> >> + vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID, true);
> >
> > Please move this to the patch "xen/arm: ffa: Add buffer full
> > notification support"
>
> Ack
>
> >
> >> }
> >> #endif
> >>
> >> @@ -363,7 +361,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
> >> @@ -394,41 +392,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;
> >> }
> >> @@ -439,6 +437,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));
> >> }
> >> diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
> >> index 7af1eca2d0c4..291396c8f635 100644
> >> --- a/xen/arch/arm/tee/ffa_partinfo.c
> >> +++ b/xen/arch/arm/tee/ffa_partinfo.c
> >> @@ -130,7 +130,8 @@ void ffa_handle_partition_info_get(struct cpu_user_regs *regs)
> >> goto out;
> >> }
> >>
> >> - if ( !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
> >> + if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) &&
> >
> > Doesn't this belong in the patch "xen/arm: ffa: Introduce VM to VM support"?
> > And wouldn't ffa_vm_count make more sense?
>
> ack. I will modify that and fold it into the VM to VM support patch.
>
> Cheers
> Bertrand
>
> >
> > Cheers,
> > Jens
> >
> >> + !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
> >> {
> >> /* Just give an empty partition list to the caller */
> >> ret = FFA_RET_OK;
> >> --
> >> 2.47.1
>
>
Hi Jens,
> On 21 Mar 2025, at 11:09, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi,
>
> On Fri, Mar 21, 2025 at 10:25 AM Bertrand Marquis
> <Bertrand.Marquis@arm.com> wrote:
>>
>> Hi Jens,
>>
>>> On 21 Mar 2025, at 09:55, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>>
>>> Hi Bertrand,
>>>
>>> On Mon, Mar 10, 2025 at 3:51 PM Bertrand Marquis
>>> <bertrand.marquis@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.marquis@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 ?
Cheers
Bertrand
> Cheers,
> Jens
>
>>
>>>
>>>> +
>>>> + if ( notif_pending )
>>>> {
>>>> /* A pending global notification for the guest */
>>>> ffa_set_regs(regs, FFA_SUCCESS_64, 0,
>>>> @@ -103,7 +101,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;
>>>> @@ -115,7 +113,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 )) )
>>>
>>> Please end the previous line with the '|' operator.
>>
>> ack
>>
>>>
>>>> {
>>>> struct arm_smccc_1_2_regs arg = {
>>>> .a0 = FFA_NOTIFICATION_GET,
>>>> @@ -170,15 +169,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
>>>> @@ -190,7 +188,7 @@ void ffa_raise_rx_buffer_full(struct domain *d)
>>>> return;
>>>>
>>>> if ( !test_and_set_bool(ctx->notif.buff_full_pending) )
>>>> - vgic_inject_irq(d, d->vcpu[0], notif_sri_irq, true);
>>>> + vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID, true);
>>>
>>> Please move this to the patch "xen/arm: ffa: Add buffer full
>>> notification support"
>>
>> Ack
>>
>>>
>>>> }
>>>> #endif
>>>>
>>>> @@ -363,7 +361,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
>>>> @@ -394,41 +392,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;
>>>> }
>>>> @@ -439,6 +437,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));
>>>> }
>>>> diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
>>>> index 7af1eca2d0c4..291396c8f635 100644
>>>> --- a/xen/arch/arm/tee/ffa_partinfo.c
>>>> +++ b/xen/arch/arm/tee/ffa_partinfo.c
>>>> @@ -130,7 +130,8 @@ void ffa_handle_partition_info_get(struct cpu_user_regs *regs)
>>>> goto out;
>>>> }
>>>>
>>>> - if ( !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
>>>> + if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) &&
>>>
>>> Doesn't this belong in the patch "xen/arm: ffa: Introduce VM to VM support"?
>>> And wouldn't ffa_vm_count make more sense?
>>
>> ack. I will modify that and fold it into the VM to VM support patch.
>>
>> Cheers
>> Bertrand
>>
>>>
>>> Cheers,
>>> Jens
>>>
>>>> + !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
>>>> {
>>>> /* Just give an empty partition list to the caller */
>>>> ret = FFA_RET_OK;
>>>> --
>>>> 2.47.1
Hi Bertrand,
On Fri, Mar 21, 2025 at 2:47 PM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Jens,
>
> > On 21 Mar 2025, at 11:09, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi,
> >
> > On Fri, Mar 21, 2025 at 10:25 AM Bertrand Marquis
> > <Bertrand.Marquis@arm.com> wrote:
> >>
> >> Hi Jens,
> >>
> >>> On 21 Mar 2025, at 09:55, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >>>
> >>> Hi Bertrand,
> >>>
> >>> On Mon, Mar 10, 2025 at 3:51 PM Bertrand Marquis
> >>> <bertrand.marquis@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.marquis@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. :-)
Cheers,
Jens
>
> Cheers
> Bertrand
>
> > Cheers,
> > Jens
> >
> >>
> >>>
> >>>> +
> >>>> + if ( notif_pending )
> >>>> {
> >>>> /* A pending global notification for the guest */
> >>>> ffa_set_regs(regs, FFA_SUCCESS_64, 0,
> >>>> @@ -103,7 +101,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;
> >>>> @@ -115,7 +113,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 )) )
> >>>
> >>> Please end the previous line with the '|' operator.
> >>
> >> ack
> >>
> >>>
> >>>> {
> >>>> struct arm_smccc_1_2_regs arg = {
> >>>> .a0 = FFA_NOTIFICATION_GET,
> >>>> @@ -170,15 +169,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
> >>>> @@ -190,7 +188,7 @@ void ffa_raise_rx_buffer_full(struct domain *d)
> >>>> return;
> >>>>
> >>>> if ( !test_and_set_bool(ctx->notif.buff_full_pending) )
> >>>> - vgic_inject_irq(d, d->vcpu[0], notif_sri_irq, true);
> >>>> + vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID, true);
> >>>
> >>> Please move this to the patch "xen/arm: ffa: Add buffer full
> >>> notification support"
> >>
> >> Ack
> >>
> >>>
> >>>> }
> >>>> #endif
> >>>>
> >>>> @@ -363,7 +361,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
> >>>> @@ -394,41 +392,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;
> >>>> }
> >>>> @@ -439,6 +437,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));
> >>>> }
> >>>> diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
> >>>> index 7af1eca2d0c4..291396c8f635 100644
> >>>> --- a/xen/arch/arm/tee/ffa_partinfo.c
> >>>> +++ b/xen/arch/arm/tee/ffa_partinfo.c
> >>>> @@ -130,7 +130,8 @@ void ffa_handle_partition_info_get(struct cpu_user_regs *regs)
> >>>> goto out;
> >>>> }
> >>>>
> >>>> - if ( !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
> >>>> + if ( !IS_ENABLED(CONFIG_FFA_VM_TO_VM) &&
> >>>
> >>> Doesn't this belong in the patch "xen/arm: ffa: Introduce VM to VM support"?
> >>> And wouldn't ffa_vm_count make more sense?
> >>
> >> ack. I will modify that and fold it into the VM to VM support patch.
> >>
> >> Cheers
> >> Bertrand
> >>
> >>>
> >>> Cheers,
> >>> Jens
> >>>
> >>>> + !ffa_fw_supports_fid(FFA_PARTITION_INFO_GET) )
> >>>> {
> >>>> /* Just give an empty partition list to the caller */
> >>>> ret = FFA_RET_OK;
> >>>> --
> >>>> 2.47.1
>
>
Hi Jens,
> On 21 Mar 2025, at 15:00, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Bertrand,
>
> On Fri, Mar 21, 2025 at 2:47 PM Bertrand Marquis
> <Bertrand.Marquis@arm.com> wrote:
>>
>> Hi Jens,
>>
>>> On 21 Mar 2025, at 11:09, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>>
>>> Hi,
>>>
>>> On Fri, Mar 21, 2025 at 10:25 AM Bertrand Marquis
>>> <Bertrand.Marquis@arm.com> wrote:
>>>>
>>>> Hi Jens,
>>>>
>>>>> On 21 Mar 2025, at 09:55, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>>>>
>>>>> Hi Bertrand,
>>>>>
>>>>> On Mon, Mar 10, 2025 at 3:51 PM Bertrand Marquis
>>>>> <bertrand.marquis@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.marquis@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
© 2016 - 2025 Red Hat, Inc.