Add support for FF-A notifications, currently limited to an SP (Secure
Partition) sending an asynchronous notification to a guest.
Guests and Xen itself are made aware of pending notifications with an
interrupt. The interrupt handler triggers a tasklet to retrieve the
notifications using the FF-A ABI and deliver them to their destinations.
Update ffa_partinfo_domain_init() to return error code like
ffa_notif_domain_init().
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
v4->v5:
- Move the freeing of d->arch.tee to the new TEE mediator free_domain_ctx
callback to make the context accessible during rcu_lock_domain_by_id()
from a tasklet
- Initialize interrupt handlers for secondary CPUs from the new TEE mediator
init_interrupt() callback
- Restore the ffa_probe() from v3, that is, remove the
presmp_initcall(ffa_init) approach and use ffa_probe() as usual now that we
have the init_interrupt callback.
- A tasklet is added to handle the Schedule Receiver interrupt. The tasklet
finds each relevant domain with rcu_lock_domain_by_id() which now is enough
to guarantee that the FF-A context can be accessed.
- The notification interrupt handler only schedules the notification
tasklet mentioned above
v3->v4:
- Add another note on FF-A limitations
- Clear secure_pending in ffa_handle_notification_get() if both SP and SPM
bitmaps are retrieved
- ASSERT that ffa_rcu_lock_domain_by_vm_id() isn't passed the vm_id FF-A
uses for Xen itself
- Replace the get_domain_by_id() call done via ffa_get_domain_by_vm_id() in
notif_irq_handler() with a call to rcu_lock_live_remote_domain_by_id() via
ffa_rcu_lock_domain_by_vm_id()
- Remove spinlock in struct ffa_ctx_notif and use atomic functions as needed
to access and update the secure_pending field
- In notif_irq_handler(), look for the first online CPU instead of assuming
that the first CPU is online
- Initialize FF-A via presmp_initcall() before the other CPUs are online,
use register_cpu_notifier() to install the interrupt handler
notif_irq_handler()
- Update commit message to reflect recent updates
v2->v3:
- Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
- Register the Xen SRI handler on each CPU using on_selected_cpus() and
setup_irq()
- Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
doesn't conflict with static SGI handlers
v1->v2:
- Addressing review comments
- Change ffa_handle_notification_{bind,unbind,set}() to take struct
cpu_user_regs *regs as argument.
- Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
an error code.
- Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
---
xen/arch/arm/tee/Makefile | 1 +
xen/arch/arm/tee/ffa.c | 72 +++++-
xen/arch/arm/tee/ffa_notif.c | 409 ++++++++++++++++++++++++++++++++
xen/arch/arm/tee/ffa_partinfo.c | 9 +-
xen/arch/arm/tee/ffa_private.h | 56 ++++-
xen/arch/arm/tee/tee.c | 2 +-
xen/include/public/arch-arm.h | 14 ++
7 files changed, 548 insertions(+), 15 deletions(-)
create mode 100644 xen/arch/arm/tee/ffa_notif.c
diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
index f0112a2f922d..7c0f46f7f446 100644
--- a/xen/arch/arm/tee/Makefile
+++ b/xen/arch/arm/tee/Makefile
@@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
obj-$(CONFIG_FFA) += ffa_shm.o
obj-$(CONFIG_FFA) += ffa_partinfo.o
obj-$(CONFIG_FFA) += ffa_rxtx.o
+obj-$(CONFIG_FFA) += ffa_notif.o
obj-y += tee.o
obj-$(CONFIG_OPTEE) += optee.o
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 5209612963e1..d644ee97cd5b 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -39,6 +39,12 @@
* - at most 32 shared memory regions per guest
* o FFA_MSG_SEND_DIRECT_REQ:
* - only supported from a VM to an SP
+ * o FFA_NOTIFICATION_*:
+ * - only supports global notifications, that is, per vCPU notifications
+ * are not supported
+ * - doesn't support signalling the secondary scheduler of pending
+ * notification for secure partitions
+ * - doesn't support notifications for Xen itself
*
* There are some large locked sections with ffa_tx_buffer_lock and
* ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
@@ -194,6 +200,8 @@ out:
static void handle_features(struct cpu_user_regs *regs)
{
+ struct domain *d = current->domain;
+ struct ffa_ctx *ctx = d->arch.tee;
uint32_t a1 = get_user_reg(regs, 1);
unsigned int n;
@@ -240,6 +248,30 @@ static void handle_features(struct cpu_user_regs *regs)
BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
ffa_set_regs_success(regs, 0, 0);
break;
+ case FFA_FEATURE_NOTIF_PEND_INTR:
+ if ( ctx->notif.enabled )
+ ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
+ else
+ ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+ break;
+ case FFA_FEATURE_SCHEDULE_RECV_INTR:
+ if ( ctx->notif.enabled )
+ ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
+ else
+ ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+ break;
+
+ case FFA_NOTIFICATION_BIND:
+ case FFA_NOTIFICATION_UNBIND:
+ case FFA_NOTIFICATION_GET:
+ case FFA_NOTIFICATION_SET:
+ case FFA_NOTIFICATION_INFO_GET_32:
+ case FFA_NOTIFICATION_INFO_GET_64:
+ if ( ctx->notif.enabled )
+ ffa_set_regs_success(regs, 0, 0);
+ else
+ ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+ break;
default:
ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
break;
@@ -305,6 +337,22 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
get_user_reg(regs, 1)),
get_user_reg(regs, 3));
break;
+ case FFA_NOTIFICATION_BIND:
+ e = ffa_handle_notification_bind(regs);
+ break;
+ case FFA_NOTIFICATION_UNBIND:
+ e = ffa_handle_notification_unbind(regs);
+ break;
+ case FFA_NOTIFICATION_INFO_GET_32:
+ case FFA_NOTIFICATION_INFO_GET_64:
+ ffa_handle_notification_info_get(regs);
+ return true;
+ case FFA_NOTIFICATION_GET:
+ ffa_handle_notification_get(regs);
+ return true;
+ case FFA_NOTIFICATION_SET:
+ e = ffa_handle_notification_set(regs);
+ break;
default:
gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
@@ -322,6 +370,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
static int ffa_domain_init(struct domain *d)
{
struct ffa_ctx *ctx;
+ int ret;
if ( !ffa_version )
return -ENODEV;
@@ -345,10 +394,11 @@ static int ffa_domain_init(struct domain *d)
* error, so no need for cleanup in this function.
*/
- if ( !ffa_partinfo_domain_init(d) )
- return -EIO;
+ ret = ffa_partinfo_domain_init(d);
+ if ( ret )
+ return ret;
- return 0;
+ return ffa_notif_domain_init(d);
}
static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
@@ -376,13 +426,6 @@ static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
}
else
{
- /*
- * domain_destroy() might have been called (via put_domain() in
- * ffa_reclaim_shms()), so we can't touch the domain structure
- * anymore.
- */
- xfree(ctx);
-
/* Only check if there has been a change to the teardown queue */
if ( !first_time )
{
@@ -423,12 +466,18 @@ static int ffa_domain_teardown(struct domain *d)
return 0;
ffa_rxtx_domain_destroy(d);
+ ffa_notif_domain_destroy(d);
ffa_domain_teardown_continue(ctx, true /* first_time */);
return 0;
}
+static void ffa_free_domain_ctx(struct domain *d)
+{
+ XFREE(d->arch.tee);
+}
+
static int ffa_relinquish_resources(struct domain *d)
{
return 0;
@@ -502,6 +551,7 @@ static bool ffa_probe(void)
if ( !ffa_partinfo_init() )
goto err_rxtx_destroy;
+ ffa_notif_init();
INIT_LIST_HEAD(&ffa_teardown_head);
init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
@@ -517,8 +567,10 @@ err_rxtx_destroy:
static const struct tee_mediator_ops ffa_ops =
{
.probe = ffa_probe,
+ .init_interrupt = ffa_notif_init_interrupt,
.domain_init = ffa_domain_init,
.domain_teardown = ffa_domain_teardown,
+ .free_domain_ctx = ffa_free_domain_ctx,
.relinquish_resources = ffa_relinquish_resources,
.handle_call = ffa_handle_call,
};
diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
new file mode 100644
index 000000000000..e8e8b62590b3
--- /dev/null
+++ b/xen/arch/arm/tee/ffa_notif.c
@@ -0,0 +1,409 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024 Linaro Limited
+ */
+
+#include <xen/const.h>
+#include <xen/cpu.h>
+#include <xen/list.h>
+#include <xen/notifier.h>
+#include <xen/spinlock.h>
+#include <xen/tasklet.h>
+#include <xen/types.h>
+
+#include <asm/smccc.h>
+#include <asm/regs.h>
+
+#include "ffa_private.h"
+
+static bool __ro_after_init notif_enabled;
+static unsigned int __ro_after_init notif_sri_irq;
+
+int ffa_handle_notification_bind(struct cpu_user_regs *regs)
+{
+ struct domain *d = current->domain;
+ uint32_t src_dst = get_user_reg(regs, 1);
+ uint32_t flags = get_user_reg(regs, 2);
+ 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_hi,
+ bitmap_lo);
+}
+
+int ffa_handle_notification_unbind(struct cpu_user_regs *regs)
+{
+ struct domain *d = current->domain;
+ uint32_t src_dst = get_user_reg(regs, 1);
+ 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_hi,
+ bitmap_lo);
+}
+
+void ffa_handle_notification_info_get(struct cpu_user_regs *regs)
+{
+ struct domain *d = current->domain;
+ struct ffa_ctx *ctx = d->arch.tee;
+
+ if ( !notif_enabled )
+ {
+ ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+ return;
+ }
+
+ if ( test_and_clear_bool(ctx->notif.secure_pending) )
+ {
+ /* A pending global notification for the guest */
+ ffa_set_regs(regs, FFA_SUCCESS_64, 0,
+ 1U << FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT, ffa_get_vm_id(d),
+ 0, 0, 0, 0);
+ }
+ else
+ {
+ /* Report an error if there where no pending global notification */
+ ffa_set_regs_error(regs, FFA_RET_NO_DATA);
+ }
+}
+
+void ffa_handle_notification_get(struct cpu_user_regs *regs)
+{
+ struct domain *d = current->domain;
+ uint32_t recv = get_user_reg(regs, 1);
+ uint32_t flags = get_user_reg(regs, 2);
+ uint32_t w2 = 0;
+ uint32_t w3 = 0;
+ uint32_t w4 = 0;
+ uint32_t w5 = 0;
+ uint32_t w6 = 0;
+ uint32_t w7 = 0;
+
+ if ( !notif_enabled )
+ {
+ ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+ return;
+ }
+
+ if ( (recv & 0xFFFFU) != ffa_get_vm_id(d) )
+ {
+ ffa_set_regs_error(regs, FFA_RET_INVALID_PARAMETERS);
+ return;
+ }
+
+ if ( flags & ( FFA_NOTIF_FLAG_BITMAP_SP | FFA_NOTIF_FLAG_BITMAP_SPM ) )
+ {
+ struct arm_smccc_1_2_regs arg = {
+ .a0 = FFA_NOTIFICATION_GET,
+ .a1 = recv,
+ .a2 = flags & ( FFA_NOTIF_FLAG_BITMAP_SP |
+ FFA_NOTIF_FLAG_BITMAP_SPM ),
+ };
+ struct arm_smccc_1_2_regs resp;
+ int32_t e;
+
+ /*
+ * Clear secure pending if both FFA_NOTIF_FLAG_BITMAP_SP and
+ * FFA_NOTIF_FLAG_BITMAP_SPM are set since secure world can't have
+ * any more pending notifications.
+ */
+ if ( ( flags & FFA_NOTIF_FLAG_BITMAP_SP ) &&
+ ( flags & FFA_NOTIF_FLAG_BITMAP_SPM ) )
+ {
+ struct ffa_ctx *ctx = d->arch.tee;
+
+ ACCESS_ONCE(ctx->notif.secure_pending) = false;
+ }
+
+ arm_smccc_1_2_smc(&arg, &resp);
+ e = ffa_get_ret_code(&resp);
+ if ( e )
+ {
+ ffa_set_regs_error(regs, e);
+ return;
+ }
+
+ if ( flags & FFA_NOTIF_FLAG_BITMAP_SP )
+ {
+ w2 = resp.a2;
+ w3 = resp.a3;
+ }
+
+ if ( flags & FFA_NOTIF_FLAG_BITMAP_SPM )
+ w6 = resp.a6;
+ }
+
+ ffa_set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, w4, w5, w6, w7);
+}
+
+int ffa_handle_notification_set(struct cpu_user_regs *regs)
+{
+ struct domain *d = current->domain;
+ uint32_t src_dst = get_user_reg(regs, 1);
+ uint32_t flags = get_user_reg(regs, 2);
+ 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);
+}
+
+/*
+ * Extract a 16-bit ID (index n) from the successful return value from
+ * FFA_NOTIFICATION_INFO_GET_64 or FFA_NOTIFICATION_INFO_GET_32. IDs are
+ * returned in registers 3 to 7 with four IDs per register for 64-bit
+ * calling convention and two IDs per register for 32-bit calling
+ * convention.
+ */
+static uint16_t get_id_from_resp(struct arm_smccc_1_2_regs *resp,
+ unsigned int n)
+{
+ unsigned int ids_per_reg;
+ unsigned int reg_idx;
+ unsigned int reg_shift;
+
+ if ( smccc_is_conv_64(resp->a0) )
+ ids_per_reg = 4;
+ else
+ ids_per_reg = 2;
+
+ reg_idx = n / ids_per_reg + 3;
+ reg_shift = ( n % ids_per_reg ) * 16;
+
+ switch ( reg_idx )
+ {
+ case 3:
+ return resp->a3 >> reg_shift;
+ case 4:
+ return resp->a4 >> reg_shift;
+ case 5:
+ return resp->a5 >> reg_shift;
+ case 6:
+ return resp->a6 >> reg_shift;
+ case 7:
+ return resp->a7 >> reg_shift;
+ default:
+ ASSERT(0); /* "Can't happen" */
+ return 0;
+ }
+}
+
+static void notif_vm_pend_intr(uint16_t vm_id)
+{
+ struct ffa_ctx *ctx;
+ struct domain *d;
+ struct vcpu *v;
+
+ /*
+ * vm_id == 0 means a notifications pending for Xen itself, but
+ * we don't support that yet.
+ */
+ if ( !vm_id )
+ return;
+
+ d = ffa_rcu_lock_domain_by_vm_id(vm_id);
+ if ( !d )
+ return;
+
+ ctx = d->arch.tee;
+ if ( !ctx )
+ goto out_unlock;
+
+ /*
+ * arch.tee is freed from complete_domain_destroy() so the RCU lock
+ * guarantees that the data structure isn't freed while we're accessing
+ * it.
+ */
+ ACCESS_ONCE(ctx->notif.secure_pending) = true;
+
+ /*
+ * Since we're only delivering global notification, always
+ * deliver to the first online vCPU. It doesn't matter
+ * which we chose, as long as it's available.
+ */
+ for_each_vcpu(d, v)
+ {
+ if ( is_vcpu_online(v) )
+ {
+ vgic_inject_irq(d, v, GUEST_FFA_NOTIF_PEND_INTR_ID,
+ true);
+ break;
+ }
+ }
+ if ( !v )
+ printk(XENLOG_ERR "ffa: can't inject NPI, all vCPUs offline\n");
+
+out_unlock:
+ rcu_unlock_domain(d);
+}
+
+static void notif_sri_action(void *unused)
+{
+ const struct arm_smccc_1_2_regs arg = {
+ .a0 = FFA_NOTIFICATION_INFO_GET_64,
+ };
+ struct arm_smccc_1_2_regs resp;
+ unsigned int id_pos;
+ unsigned int list_count;
+ uint64_t ids_count;
+ unsigned int n;
+ int32_t res;
+
+ do {
+ arm_smccc_1_2_smc(&arg, &resp);
+ res = ffa_get_ret_code(&resp);
+ if ( res )
+ {
+ if ( res != FFA_RET_NO_DATA )
+ printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
+ res);
+ return;
+ }
+
+ ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
+ list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
+ FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
+
+ id_pos = 0;
+ for ( n = 0; n < list_count; n++ )
+ {
+ unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
+ uint16_t vm_id = get_id_from_resp(&resp, id_pos);
+
+ notif_vm_pend_intr(vm_id);
+
+ id_pos += count;
+ }
+
+ } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
+}
+
+static DECLARE_TASKLET(notif_sri_tasklet, notif_sri_action, NULL);
+
+static void notif_irq_handler(int irq, void *data)
+{
+ tasklet_schedule(¬if_sri_tasklet);
+}
+
+static int32_t ffa_notification_bitmap_create(uint16_t vm_id,
+ uint32_t vcpu_count)
+{
+ return ffa_simple_call(FFA_NOTIFICATION_BITMAP_CREATE, vm_id, vcpu_count,
+ 0, 0);
+}
+
+static int32_t ffa_notification_bitmap_destroy(uint16_t vm_id)
+{
+ return ffa_simple_call(FFA_NOTIFICATION_BITMAP_DESTROY, vm_id, 0, 0, 0);
+}
+
+void ffa_notif_init_interrupt(void)
+{
+ int ret;
+
+ if ( notif_enabled && notif_sri_irq < NR_GIC_SGI )
+ {
+ /*
+ * An error here is unlikely since the primary CPU has already
+ * succeeded in installing the interrupt handler. If this fails it
+ * may lead to a problem with notifictaions.
+ *
+ * The CPUs without an notification handler installed will fail to
+ * trigger on the SGI indicating that there are notifications
+ * pending, while the SPMC in the secure world will not notice that
+ * the interrupt was lost.
+ */
+ ret = request_irq(notif_sri_irq, 0, notif_irq_handler, "FF-A notif",
+ NULL);
+ if ( ret )
+ printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
+ notif_sri_irq, ret);
+ }
+}
+
+void ffa_notif_init(void)
+{
+ const struct arm_smccc_1_2_regs arg = {
+ .a0 = FFA_FEATURES,
+ .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
+ };
+ struct arm_smccc_1_2_regs resp;
+ unsigned int irq;
+ int ret;
+
+ 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 )
+ {
+ printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
+ irq, ret);
+ return;
+ }
+
+ notif_enabled = true;
+}
+
+int ffa_notif_domain_init(struct domain *d)
+{
+ struct ffa_ctx *ctx = d->arch.tee;
+ int32_t res;
+
+ if ( !notif_enabled )
+ return 0;
+
+ res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
+ if ( res )
+ return -ENOMEM;
+
+ ctx->notif.enabled = true;
+
+ return 0;
+}
+
+void ffa_notif_domain_destroy(struct domain *d)
+{
+ struct ffa_ctx *ctx = d->arch.tee;
+
+ if ( ctx->notif.enabled )
+ {
+ ffa_notification_bitmap_destroy(ffa_get_vm_id(d));
+ ctx->notif.enabled = false;
+ }
+}
diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
index dc1059584828..93a03c6bc672 100644
--- a/xen/arch/arm/tee/ffa_partinfo.c
+++ b/xen/arch/arm/tee/ffa_partinfo.c
@@ -306,7 +306,7 @@ static void vm_destroy_bitmap_init(struct ffa_ctx *ctx,
}
}
-bool ffa_partinfo_domain_init(struct domain *d)
+int ffa_partinfo_domain_init(struct domain *d)
{
unsigned int count = BITS_TO_LONGS(subscr_vm_destroyed_count);
struct ffa_ctx *ctx = d->arch.tee;
@@ -315,7 +315,7 @@ bool ffa_partinfo_domain_init(struct domain *d)
ctx->vm_destroy_bitmap = xzalloc_array(unsigned long, count);
if ( !ctx->vm_destroy_bitmap )
- return false;
+ return -ENOMEM;
for ( n = 0; n < subscr_vm_created_count; n++ )
{
@@ -330,7 +330,10 @@ bool ffa_partinfo_domain_init(struct domain *d)
}
vm_destroy_bitmap_init(ctx, n);
- return n == subscr_vm_created_count;
+ if ( n != subscr_vm_created_count )
+ return -EIO;
+
+ return 0;
}
bool ffa_partinfo_domain_destroy(struct domain *d)
diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
index 98236cbf14a3..7c6b06f686fc 100644
--- a/xen/arch/arm/tee/ffa_private.h
+++ b/xen/arch/arm/tee/ffa_private.h
@@ -25,6 +25,7 @@
#define FFA_RET_DENIED -6
#define FFA_RET_RETRY -7
#define FFA_RET_ABORTED -8
+#define FFA_RET_NO_DATA -9
/* FFA_VERSION helpers */
#define FFA_VERSION_MAJOR_SHIFT 16U
@@ -175,6 +176,21 @@
*/
#define FFA_PARTITION_INFO_GET_COUNT_FLAG BIT(0, U)
+/* Flags used in calls to FFA_NOTIFICATION_GET interface */
+#define FFA_NOTIF_FLAG_BITMAP_SP BIT(0, U)
+#define FFA_NOTIF_FLAG_BITMAP_VM BIT(1, U)
+#define FFA_NOTIF_FLAG_BITMAP_SPM BIT(2, U)
+#define FFA_NOTIF_FLAG_BITMAP_HYP BIT(3, U)
+
+#define FFA_NOTIF_INFO_GET_MORE_FLAG BIT(0, U)
+#define FFA_NOTIF_INFO_GET_ID_LIST_SHIFT 12
+#define FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT 7
+#define FFA_NOTIF_INFO_GET_ID_COUNT_MASK 0x1F
+
+/* Feature IDs used with FFA_FEATURES */
+#define FFA_FEATURE_NOTIF_PEND_INTR 0x1U
+#define FFA_FEATURE_SCHEDULE_RECV_INTR 0x2U
+
/* Function IDs */
#define FFA_ERROR 0x84000060U
#define FFA_SUCCESS_32 0x84000061U
@@ -213,6 +229,24 @@
#define FFA_MEM_FRAG_TX 0x8400007BU
#define FFA_MSG_SEND 0x8400006EU
#define FFA_MSG_POLL 0x8400006AU
+#define FFA_NOTIFICATION_BITMAP_CREATE 0x8400007DU
+#define FFA_NOTIFICATION_BITMAP_DESTROY 0x8400007EU
+#define FFA_NOTIFICATION_BIND 0x8400007FU
+#define FFA_NOTIFICATION_UNBIND 0x84000080U
+#define FFA_NOTIFICATION_SET 0x84000081U
+#define FFA_NOTIFICATION_GET 0x84000082U
+#define FFA_NOTIFICATION_INFO_GET_32 0x84000083U
+#define FFA_NOTIFICATION_INFO_GET_64 0xC4000083U
+
+struct ffa_ctx_notif {
+ bool enabled;
+
+ /*
+ * True if domain is reported by FFA_NOTIFICATION_INFO_GET to have
+ * pending global notifications.
+ */
+ bool secure_pending;
+};
struct ffa_ctx {
void *rx;
@@ -228,6 +262,7 @@ struct ffa_ctx {
struct list_head shm_list;
/* Number of allocated shared memory object */
unsigned int shm_count;
+ struct ffa_ctx_notif notif;
/*
* tx_lock is used to serialize access to tx
* rx_lock is used to serialize access to rx
@@ -257,7 +292,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs);
int ffa_handle_mem_reclaim(uint64_t handle, uint32_t flags);
bool ffa_partinfo_init(void);
-bool ffa_partinfo_domain_init(struct domain *d);
+int ffa_partinfo_domain_init(struct domain *d);
bool ffa_partinfo_domain_destroy(struct domain *d);
int32_t ffa_handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
uint32_t w4, uint32_t w5, uint32_t *count,
@@ -271,12 +306,31 @@ uint32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
uint32_t ffa_handle_rxtx_unmap(void);
int32_t ffa_handle_rx_release(void);
+void ffa_notif_init(void);
+void ffa_notif_init_interrupt(void);
+int ffa_notif_domain_init(struct domain *d);
+void ffa_notif_domain_destroy(struct domain *d);
+
+int ffa_handle_notification_bind(struct cpu_user_regs *regs);
+int ffa_handle_notification_unbind(struct cpu_user_regs *regs);
+void ffa_handle_notification_info_get(struct cpu_user_regs *regs);
+void ffa_handle_notification_get(struct cpu_user_regs *regs);
+int ffa_handle_notification_set(struct cpu_user_regs *regs);
+
static inline uint16_t ffa_get_vm_id(const struct domain *d)
{
/* +1 since 0 is reserved for the hypervisor in FF-A */
return d->domain_id + 1;
}
+static inline struct domain *ffa_rcu_lock_domain_by_vm_id(uint16_t vm_id)
+{
+ ASSERT(vm_id);
+
+ /* -1 to match ffa_get_vm_id() */
+ return rcu_lock_domain_by_id(vm_id - 1);
+}
+
static inline void ffa_set_regs(struct cpu_user_regs *regs, register_t v0,
register_t v1, register_t v2, register_t v3,
register_t v4, register_t v5, register_t v6,
diff --git a/xen/arch/arm/tee/tee.c b/xen/arch/arm/tee/tee.c
index cb65f187f51f..1e890ecc2932 100644
--- a/xen/arch/arm/tee/tee.c
+++ b/xen/arch/arm/tee/tee.c
@@ -94,7 +94,7 @@ static int __init tee_init(void)
return 0;
}
-__initcall(tee_init);
+presmp_initcall(tee_init);
void __init init_tee_interrupt(void)
{
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 289af81bd69d..e2412a17474e 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -505,6 +505,7 @@ typedef uint64_t xen_callback_t;
#define GUEST_MAX_VCPUS 128
/* Interrupts */
+
#define GUEST_TIMER_VIRT_PPI 27
#define GUEST_TIMER_PHYS_S_PPI 29
#define GUEST_TIMER_PHYS_NS_PPI 30
@@ -515,6 +516,19 @@ typedef uint64_t xen_callback_t;
#define GUEST_VIRTIO_MMIO_SPI_FIRST 33
#define GUEST_VIRTIO_MMIO_SPI_LAST 43
+/*
+ * SGI is the preferred delivery mechanism of FF-A pending notifications or
+ * schedule recveive interrupt. SGIs 8-15 are normally not used by a guest
+ * as they in a non-virtualized system typically are assigned to the secure
+ * world. Here we're free to use SGI 8-15 since they are virtual and have
+ * nothing to do with the secure world.
+ *
+ * For partitioning of SGIs see also Arm Base System Architecture v1.0C,
+ * https://developer.arm.com/documentation/den0094/
+ */
+#define GUEST_FFA_NOTIF_PEND_INTR_ID 8
+#define GUEST_FFA_SCHEDULE_RECV_INTR_ID 9
+
/* PSCI functions */
#define PSCI_cpu_suspend 0
#define PSCI_cpu_off 1
--
2.34.1Hi Jens,
> On 29 May 2024, at 09:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Add support for FF-A notifications, currently limited to an SP (Secure
> Partition) sending an asynchronous notification to a guest.
>
> Guests and Xen itself are made aware of pending notifications with an
> interrupt. The interrupt handler triggers a tasklet to retrieve the
> notifications using the FF-A ABI and deliver them to their destinations.
>
> Update ffa_partinfo_domain_init() to return error code like
> ffa_notif_domain_init().
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
> v4->v5:
> - Move the freeing of d->arch.tee to the new TEE mediator free_domain_ctx
> callback to make the context accessible during rcu_lock_domain_by_id()
> from a tasklet
> - Initialize interrupt handlers for secondary CPUs from the new TEE mediator
> init_interrupt() callback
> - Restore the ffa_probe() from v3, that is, remove the
> presmp_initcall(ffa_init) approach and use ffa_probe() as usual now that we
> have the init_interrupt callback.
> - A tasklet is added to handle the Schedule Receiver interrupt. The tasklet
> finds each relevant domain with rcu_lock_domain_by_id() which now is enough
> to guarantee that the FF-A context can be accessed.
> - The notification interrupt handler only schedules the notification
> tasklet mentioned above
>
> v3->v4:
> - Add another note on FF-A limitations
> - Clear secure_pending in ffa_handle_notification_get() if both SP and SPM
> bitmaps are retrieved
> - ASSERT that ffa_rcu_lock_domain_by_vm_id() isn't passed the vm_id FF-A
> uses for Xen itself
> - Replace the get_domain_by_id() call done via ffa_get_domain_by_vm_id() in
> notif_irq_handler() with a call to rcu_lock_live_remote_domain_by_id() via
> ffa_rcu_lock_domain_by_vm_id()
> - Remove spinlock in struct ffa_ctx_notif and use atomic functions as needed
> to access and update the secure_pending field
> - In notif_irq_handler(), look for the first online CPU instead of assuming
> that the first CPU is online
> - Initialize FF-A via presmp_initcall() before the other CPUs are online,
> use register_cpu_notifier() to install the interrupt handler
> notif_irq_handler()
> - Update commit message to reflect recent updates
>
> v2->v3:
> - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
> FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
> - Register the Xen SRI handler on each CPU using on_selected_cpus() and
> setup_irq()
> - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
> doesn't conflict with static SGI handlers
>
> v1->v2:
> - Addressing review comments
> - Change ffa_handle_notification_{bind,unbind,set}() to take struct
> cpu_user_regs *regs as argument.
> - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
> an error code.
> - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
> ---
> xen/arch/arm/tee/Makefile | 1 +
> xen/arch/arm/tee/ffa.c | 72 +++++-
> xen/arch/arm/tee/ffa_notif.c | 409 ++++++++++++++++++++++++++++++++
> xen/arch/arm/tee/ffa_partinfo.c | 9 +-
> xen/arch/arm/tee/ffa_private.h | 56 ++++-
> xen/arch/arm/tee/tee.c | 2 +-
> xen/include/public/arch-arm.h | 14 ++
> 7 files changed, 548 insertions(+), 15 deletions(-)
> create mode 100644 xen/arch/arm/tee/ffa_notif.c
>
> diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
> index f0112a2f922d..7c0f46f7f446 100644
> --- a/xen/arch/arm/tee/Makefile
> +++ b/xen/arch/arm/tee/Makefile
> @@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
> obj-$(CONFIG_FFA) += ffa_shm.o
> obj-$(CONFIG_FFA) += ffa_partinfo.o
> obj-$(CONFIG_FFA) += ffa_rxtx.o
> +obj-$(CONFIG_FFA) += ffa_notif.o
> obj-y += tee.o
> obj-$(CONFIG_OPTEE) += optee.o
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 5209612963e1..d644ee97cd5b 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -39,6 +39,12 @@
> * - at most 32 shared memory regions per guest
> * o FFA_MSG_SEND_DIRECT_REQ:
> * - only supported from a VM to an SP
> + * o FFA_NOTIFICATION_*:
> + * - only supports global notifications, that is, per vCPU notifications
> + * are not supported
> + * - doesn't support signalling the secondary scheduler of pending
> + * notification for secure partitions
> + * - doesn't support notifications for Xen itself
> *
> * There are some large locked sections with ffa_tx_buffer_lock and
> * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
> @@ -194,6 +200,8 @@ out:
>
> static void handle_features(struct cpu_user_regs *regs)
> {
> + struct domain *d = current->domain;
> + struct ffa_ctx *ctx = d->arch.tee;
> uint32_t a1 = get_user_reg(regs, 1);
> unsigned int n;
>
> @@ -240,6 +248,30 @@ static void handle_features(struct cpu_user_regs *regs)
> BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
> ffa_set_regs_success(regs, 0, 0);
> break;
> + case FFA_FEATURE_NOTIF_PEND_INTR:
> + if ( ctx->notif.enabled )
> + ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
> + else
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + break;
> + case FFA_FEATURE_SCHEDULE_RECV_INTR:
> + if ( ctx->notif.enabled )
> + ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
> + else
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + break;
> +
> + case FFA_NOTIFICATION_BIND:
> + case FFA_NOTIFICATION_UNBIND:
> + case FFA_NOTIFICATION_GET:
> + case FFA_NOTIFICATION_SET:
> + case FFA_NOTIFICATION_INFO_GET_32:
> + case FFA_NOTIFICATION_INFO_GET_64:
> + if ( ctx->notif.enabled )
> + ffa_set_regs_success(regs, 0, 0);
> + else
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + break;
> default:
> ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> break;
> @@ -305,6 +337,22 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> get_user_reg(regs, 1)),
> get_user_reg(regs, 3));
> break;
> + case FFA_NOTIFICATION_BIND:
> + e = ffa_handle_notification_bind(regs);
> + break;
> + case FFA_NOTIFICATION_UNBIND:
> + e = ffa_handle_notification_unbind(regs);
> + break;
> + case FFA_NOTIFICATION_INFO_GET_32:
> + case FFA_NOTIFICATION_INFO_GET_64:
> + ffa_handle_notification_info_get(regs);
> + return true;
> + case FFA_NOTIFICATION_GET:
> + ffa_handle_notification_get(regs);
> + return true;
> + case FFA_NOTIFICATION_SET:
> + e = ffa_handle_notification_set(regs);
> + break;
>
> default:
> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> @@ -322,6 +370,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> static int ffa_domain_init(struct domain *d)
> {
> struct ffa_ctx *ctx;
> + int ret;
>
> if ( !ffa_version )
> return -ENODEV;
> @@ -345,10 +394,11 @@ static int ffa_domain_init(struct domain *d)
> * error, so no need for cleanup in this function.
> */
>
> - if ( !ffa_partinfo_domain_init(d) )
> - return -EIO;
> + ret = ffa_partinfo_domain_init(d);
> + if ( ret )
> + return ret;
>
> - return 0;
> + return ffa_notif_domain_init(d);
> }
>
> static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
> @@ -376,13 +426,6 @@ static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
> }
> else
> {
> - /*
> - * domain_destroy() might have been called (via put_domain() in
> - * ffa_reclaim_shms()), so we can't touch the domain structure
> - * anymore.
> - */
> - xfree(ctx);
> -
> /* Only check if there has been a change to the teardown queue */
> if ( !first_time )
> {
> @@ -423,12 +466,18 @@ static int ffa_domain_teardown(struct domain *d)
> return 0;
>
> ffa_rxtx_domain_destroy(d);
> + ffa_notif_domain_destroy(d);
>
> ffa_domain_teardown_continue(ctx, true /* first_time */);
>
> return 0;
> }
>
> +static void ffa_free_domain_ctx(struct domain *d)
> +{
> + XFREE(d->arch.tee);
> +}
> +
> static int ffa_relinquish_resources(struct domain *d)
> {
> return 0;
> @@ -502,6 +551,7 @@ static bool ffa_probe(void)
> if ( !ffa_partinfo_init() )
> goto err_rxtx_destroy;
>
> + ffa_notif_init();
> INIT_LIST_HEAD(&ffa_teardown_head);
> init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
>
> @@ -517,8 +567,10 @@ err_rxtx_destroy:
> static const struct tee_mediator_ops ffa_ops =
> {
> .probe = ffa_probe,
> + .init_interrupt = ffa_notif_init_interrupt,
With the previous change on init secondary i would suggest to
have a ffa_init_secondary here actually calling the ffa_notif_init_interrupt.
> .domain_init = ffa_domain_init,
> .domain_teardown = ffa_domain_teardown,
> + .free_domain_ctx = ffa_free_domain_ctx,
> .relinquish_resources = ffa_relinquish_resources,
> .handle_call = ffa_handle_call,
> };
> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
> new file mode 100644
> index 000000000000..e8e8b62590b3
> --- /dev/null
> +++ b/xen/arch/arm/tee/ffa_notif.c
> @@ -0,0 +1,409 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 Linaro Limited
> + */
> +
> +#include <xen/const.h>
> +#include <xen/cpu.h>
> +#include <xen/list.h>
> +#include <xen/notifier.h>
> +#include <xen/spinlock.h>
> +#include <xen/tasklet.h>
> +#include <xen/types.h>
> +
> +#include <asm/smccc.h>
> +#include <asm/regs.h>
> +
> +#include "ffa_private.h"
> +
> +static bool __ro_after_init notif_enabled;
> +static unsigned int __ro_after_init notif_sri_irq;
> +
> +int ffa_handle_notification_bind(struct cpu_user_regs *regs)
> +{
> + struct domain *d = current->domain;
> + uint32_t src_dst = get_user_reg(regs, 1);
> + uint32_t flags = get_user_reg(regs, 2);
> + 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_hi,
> + bitmap_lo);
> +}
> +
> +int ffa_handle_notification_unbind(struct cpu_user_regs *regs)
> +{
> + struct domain *d = current->domain;
> + uint32_t src_dst = get_user_reg(regs, 1);
> + 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_hi,
> + bitmap_lo);
> +}
> +
> +void ffa_handle_notification_info_get(struct cpu_user_regs *regs)
> +{
> + struct domain *d = current->domain;
> + struct ffa_ctx *ctx = d->arch.tee;
> +
> + if ( !notif_enabled )
> + {
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + return;
> + }
> +
> + if ( test_and_clear_bool(ctx->notif.secure_pending) )
> + {
> + /* A pending global notification for the guest */
> + ffa_set_regs(regs, FFA_SUCCESS_64, 0,
> + 1U << FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT, ffa_get_vm_id(d),
> + 0, 0, 0, 0);
> + }
> + else
> + {
> + /* Report an error if there where no pending global notification */
> + ffa_set_regs_error(regs, FFA_RET_NO_DATA);
> + }
> +}
> +
> +void ffa_handle_notification_get(struct cpu_user_regs *regs)
> +{
> + struct domain *d = current->domain;
> + uint32_t recv = get_user_reg(regs, 1);
> + uint32_t flags = get_user_reg(regs, 2);
> + uint32_t w2 = 0;
> + uint32_t w3 = 0;
> + uint32_t w4 = 0;
> + uint32_t w5 = 0;
> + uint32_t w6 = 0;
> + uint32_t w7 = 0;
> +
> + if ( !notif_enabled )
> + {
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + return;
> + }
> +
> + if ( (recv & 0xFFFFU) != ffa_get_vm_id(d) )
> + {
> + ffa_set_regs_error(regs, FFA_RET_INVALID_PARAMETERS);
> + return;
> + }
> +
> + if ( flags & ( FFA_NOTIF_FLAG_BITMAP_SP | FFA_NOTIF_FLAG_BITMAP_SPM ) )
> + {
> + struct arm_smccc_1_2_regs arg = {
> + .a0 = FFA_NOTIFICATION_GET,
> + .a1 = recv,
> + .a2 = flags & ( FFA_NOTIF_FLAG_BITMAP_SP |
> + FFA_NOTIF_FLAG_BITMAP_SPM ),
> + };
> + struct arm_smccc_1_2_regs resp;
> + int32_t e;
> +
> + /*
> + * Clear secure pending if both FFA_NOTIF_FLAG_BITMAP_SP and
> + * FFA_NOTIF_FLAG_BITMAP_SPM are set since secure world can't have
> + * any more pending notifications.
> + */
> + if ( ( flags & FFA_NOTIF_FLAG_BITMAP_SP ) &&
> + ( flags & FFA_NOTIF_FLAG_BITMAP_SPM ) )
> + {
> + struct ffa_ctx *ctx = d->arch.tee;
> +
> + ACCESS_ONCE(ctx->notif.secure_pending) = false;
> + }
> +
> + arm_smccc_1_2_smc(&arg, &resp);
> + e = ffa_get_ret_code(&resp);
> + if ( e )
> + {
> + ffa_set_regs_error(regs, e);
> + return;
> + }
> +
> + if ( flags & FFA_NOTIF_FLAG_BITMAP_SP )
> + {
> + w2 = resp.a2;
> + w3 = resp.a3;
> + }
> +
> + if ( flags & FFA_NOTIF_FLAG_BITMAP_SPM )
> + w6 = resp.a6;
> + }
> +
> + ffa_set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, w4, w5, w6, w7);
> +}
> +
> +int ffa_handle_notification_set(struct cpu_user_regs *regs)
> +{
> + struct domain *d = current->domain;
> + uint32_t src_dst = get_user_reg(regs, 1);
> + uint32_t flags = get_user_reg(regs, 2);
> + 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);
> +}
> +
> +/*
> + * Extract a 16-bit ID (index n) from the successful return value from
> + * FFA_NOTIFICATION_INFO_GET_64 or FFA_NOTIFICATION_INFO_GET_32. IDs are
> + * returned in registers 3 to 7 with four IDs per register for 64-bit
> + * calling convention and two IDs per register for 32-bit calling
> + * convention.
> + */
> +static uint16_t get_id_from_resp(struct arm_smccc_1_2_regs *resp,
> + unsigned int n)
> +{
> + unsigned int ids_per_reg;
> + unsigned int reg_idx;
> + unsigned int reg_shift;
> +
> + if ( smccc_is_conv_64(resp->a0) )
> + ids_per_reg = 4;
> + else
> + ids_per_reg = 2;
> +
> + reg_idx = n / ids_per_reg + 3;
> + reg_shift = ( n % ids_per_reg ) * 16;
> +
> + switch ( reg_idx )
> + {
> + case 3:
> + return resp->a3 >> reg_shift;
> + case 4:
> + return resp->a4 >> reg_shift;
> + case 5:
> + return resp->a5 >> reg_shift;
> + case 6:
> + return resp->a6 >> reg_shift;
> + case 7:
> + return resp->a7 >> reg_shift;
> + default:
> + ASSERT(0); /* "Can't happen" */
> + return 0;
> + }
> +}
> +
> +static void notif_vm_pend_intr(uint16_t vm_id)
> +{
> + struct ffa_ctx *ctx;
> + struct domain *d;
> + struct vcpu *v;
> +
> + /*
> + * vm_id == 0 means a notifications pending for Xen itself, but
> + * we don't support that yet.
> + */
> + if ( !vm_id )
> + return;
> +
> + d = ffa_rcu_lock_domain_by_vm_id(vm_id);
> + if ( !d )
> + return;
> +
> + ctx = d->arch.tee;
> + if ( !ctx )
> + goto out_unlock;
In both previous cases you are silently ignoring an interrupt
due to an internal error.
Is this something that we should trace ? maybe just debug ?
Could you add a comment to explain why this could happen
(when possible) or not and the possible side effects ?
The second one would be a notification for a domain without
FF-A enabled which is ok but i am a bit more wondering on
the first one.
> +
> + /*
> + * arch.tee is freed from complete_domain_destroy() so the RCU lock
> + * guarantees that the data structure isn't freed while we're accessing
> + * it.
> + */
> + ACCESS_ONCE(ctx->notif.secure_pending) = true;
> +
> + /*
> + * Since we're only delivering global notification, always
> + * deliver to the first online vCPU. It doesn't matter
> + * which we chose, as long as it's available.
> + */
> + for_each_vcpu(d, v)
> + {
> + if ( is_vcpu_online(v) )
> + {
> + vgic_inject_irq(d, v, GUEST_FFA_NOTIF_PEND_INTR_ID,
> + true);
> + break;
> + }
> + }
> + if ( !v )
> + printk(XENLOG_ERR "ffa: can't inject NPI, all vCPUs offline\n");
> +
> +out_unlock:
> + rcu_unlock_domain(d);
> +}
> +
> +static void notif_sri_action(void *unused)
> +{
> + const struct arm_smccc_1_2_regs arg = {
> + .a0 = FFA_NOTIFICATION_INFO_GET_64,
> + };
> + struct arm_smccc_1_2_regs resp;
> + unsigned int id_pos;
> + unsigned int list_count;
> + uint64_t ids_count;
> + unsigned int n;
> + int32_t res;
> +
> + do {
> + arm_smccc_1_2_smc(&arg, &resp);
> + res = ffa_get_ret_code(&resp);
> + if ( res )
> + {
> + if ( res != FFA_RET_NO_DATA )
> + printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
> + res);
> + return;
> + }
> +
> + ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
> + list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
> + FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
> +
> + id_pos = 0;
> + for ( n = 0; n < list_count; n++ )
> + {
> + unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
> + uint16_t vm_id = get_id_from_resp(&resp, id_pos);
> +
> + notif_vm_pend_intr(vm_id);
> +
> + id_pos += count;
> + }
> +
> + } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
> +}
> +
> +static DECLARE_TASKLET(notif_sri_tasklet, notif_sri_action, NULL);
> +
> +static void notif_irq_handler(int irq, void *data)
> +{
> + tasklet_schedule(¬if_sri_tasklet);
> +}
> +
> +static int32_t ffa_notification_bitmap_create(uint16_t vm_id,
> + uint32_t vcpu_count)
> +{
> + return ffa_simple_call(FFA_NOTIFICATION_BITMAP_CREATE, vm_id, vcpu_count,
> + 0, 0);
> +}
> +
> +static int32_t ffa_notification_bitmap_destroy(uint16_t vm_id)
> +{
> + return ffa_simple_call(FFA_NOTIFICATION_BITMAP_DESTROY, vm_id, 0, 0, 0);
> +}
> +
> +void ffa_notif_init_interrupt(void)
> +{
> + int ret;
> +
> + if ( notif_enabled && notif_sri_irq < NR_GIC_SGI )
> + {
> + /*
> + * An error here is unlikely since the primary CPU has already
> + * succeeded in installing the interrupt handler. If this fails it
> + * may lead to a problem with notifictaions.
> + *
> + * The CPUs without an notification handler installed will fail to
> + * trigger on the SGI indicating that there are notifications
> + * pending, while the SPMC in the secure world will not notice that
> + * the interrupt was lost.
> + */
> + ret = request_irq(notif_sri_irq, 0, notif_irq_handler, "FF-A notif",
> + NULL);
> + if ( ret )
> + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> + notif_sri_irq, ret);
> + }
> +}
> +
> +void ffa_notif_init(void)
> +{
> + const struct arm_smccc_1_2_regs arg = {
> + .a0 = FFA_FEATURES,
> + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> + };
> + struct arm_smccc_1_2_regs resp;
> + unsigned int irq;
> + int ret;
> +
> + 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 )
> + {
> + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> + irq, ret);
> + return;
> + }
> +
> + notif_enabled = true;
> +}
> +
> +int ffa_notif_domain_init(struct domain *d)
> +{
> + struct ffa_ctx *ctx = d->arch.tee;
> + int32_t res;
> +
> + if ( !notif_enabled )
> + return 0;
> +
> + res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
> + if ( res )
> + return -ENOMEM;
> +
> + ctx->notif.enabled = true;
> +
> + return 0;
> +}
> +
> +void ffa_notif_domain_destroy(struct domain *d)
> +{
> + struct ffa_ctx *ctx = d->arch.tee;
> +
> + if ( ctx->notif.enabled )
> + {
> + ffa_notification_bitmap_destroy(ffa_get_vm_id(d));
> + ctx->notif.enabled = false;
> + }
> +}
> diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
> index dc1059584828..93a03c6bc672 100644
> --- a/xen/arch/arm/tee/ffa_partinfo.c
> +++ b/xen/arch/arm/tee/ffa_partinfo.c
> @@ -306,7 +306,7 @@ static void vm_destroy_bitmap_init(struct ffa_ctx *ctx,
> }
> }
>
> -bool ffa_partinfo_domain_init(struct domain *d)
> +int ffa_partinfo_domain_init(struct domain *d)
> {
> unsigned int count = BITS_TO_LONGS(subscr_vm_destroyed_count);
> struct ffa_ctx *ctx = d->arch.tee;
> @@ -315,7 +315,7 @@ bool ffa_partinfo_domain_init(struct domain *d)
>
> ctx->vm_destroy_bitmap = xzalloc_array(unsigned long, count);
> if ( !ctx->vm_destroy_bitmap )
> - return false;
> + return -ENOMEM;
>
> for ( n = 0; n < subscr_vm_created_count; n++ )
> {
> @@ -330,7 +330,10 @@ bool ffa_partinfo_domain_init(struct domain *d)
> }
> vm_destroy_bitmap_init(ctx, n);
>
> - return n == subscr_vm_created_count;
> + if ( n != subscr_vm_created_count )
> + return -EIO;
> +
> + return 0;
> }
>
> bool ffa_partinfo_domain_destroy(struct domain *d)
> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
> index 98236cbf14a3..7c6b06f686fc 100644
> --- a/xen/arch/arm/tee/ffa_private.h
> +++ b/xen/arch/arm/tee/ffa_private.h
> @@ -25,6 +25,7 @@
> #define FFA_RET_DENIED -6
> #define FFA_RET_RETRY -7
> #define FFA_RET_ABORTED -8
> +#define FFA_RET_NO_DATA -9
>
> /* FFA_VERSION helpers */
> #define FFA_VERSION_MAJOR_SHIFT 16U
> @@ -175,6 +176,21 @@
> */
> #define FFA_PARTITION_INFO_GET_COUNT_FLAG BIT(0, U)
>
> +/* Flags used in calls to FFA_NOTIFICATION_GET interface */
> +#define FFA_NOTIF_FLAG_BITMAP_SP BIT(0, U)
> +#define FFA_NOTIF_FLAG_BITMAP_VM BIT(1, U)
> +#define FFA_NOTIF_FLAG_BITMAP_SPM BIT(2, U)
> +#define FFA_NOTIF_FLAG_BITMAP_HYP BIT(3, U)
> +
> +#define FFA_NOTIF_INFO_GET_MORE_FLAG BIT(0, U)
> +#define FFA_NOTIF_INFO_GET_ID_LIST_SHIFT 12
> +#define FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT 7
> +#define FFA_NOTIF_INFO_GET_ID_COUNT_MASK 0x1F
> +
> +/* Feature IDs used with FFA_FEATURES */
> +#define FFA_FEATURE_NOTIF_PEND_INTR 0x1U
> +#define FFA_FEATURE_SCHEDULE_RECV_INTR 0x2U
> +
> /* Function IDs */
> #define FFA_ERROR 0x84000060U
> #define FFA_SUCCESS_32 0x84000061U
> @@ -213,6 +229,24 @@
> #define FFA_MEM_FRAG_TX 0x8400007BU
> #define FFA_MSG_SEND 0x8400006EU
> #define FFA_MSG_POLL 0x8400006AU
> +#define FFA_NOTIFICATION_BITMAP_CREATE 0x8400007DU
> +#define FFA_NOTIFICATION_BITMAP_DESTROY 0x8400007EU
> +#define FFA_NOTIFICATION_BIND 0x8400007FU
> +#define FFA_NOTIFICATION_UNBIND 0x84000080U
> +#define FFA_NOTIFICATION_SET 0x84000081U
> +#define FFA_NOTIFICATION_GET 0x84000082U
> +#define FFA_NOTIFICATION_INFO_GET_32 0x84000083U
> +#define FFA_NOTIFICATION_INFO_GET_64 0xC4000083U
> +
> +struct ffa_ctx_notif {
> + bool enabled;
> +
> + /*
> + * True if domain is reported by FFA_NOTIFICATION_INFO_GET to have
> + * pending global notifications.
> + */
> + bool secure_pending;
> +};
>
> struct ffa_ctx {
> void *rx;
> @@ -228,6 +262,7 @@ struct ffa_ctx {
> struct list_head shm_list;
> /* Number of allocated shared memory object */
> unsigned int shm_count;
> + struct ffa_ctx_notif notif;
> /*
> * tx_lock is used to serialize access to tx
> * rx_lock is used to serialize access to rx
> @@ -257,7 +292,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs);
> int ffa_handle_mem_reclaim(uint64_t handle, uint32_t flags);
>
> bool ffa_partinfo_init(void);
> -bool ffa_partinfo_domain_init(struct domain *d);
> +int ffa_partinfo_domain_init(struct domain *d);
> bool ffa_partinfo_domain_destroy(struct domain *d);
> int32_t ffa_handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
> uint32_t w4, uint32_t w5, uint32_t *count,
> @@ -271,12 +306,31 @@ uint32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
> uint32_t ffa_handle_rxtx_unmap(void);
> int32_t ffa_handle_rx_release(void);
>
> +void ffa_notif_init(void);
> +void ffa_notif_init_interrupt(void);
> +int ffa_notif_domain_init(struct domain *d);
> +void ffa_notif_domain_destroy(struct domain *d);
> +
> +int ffa_handle_notification_bind(struct cpu_user_regs *regs);
> +int ffa_handle_notification_unbind(struct cpu_user_regs *regs);
> +void ffa_handle_notification_info_get(struct cpu_user_regs *regs);
> +void ffa_handle_notification_get(struct cpu_user_regs *regs);
> +int ffa_handle_notification_set(struct cpu_user_regs *regs);
> +
> static inline uint16_t ffa_get_vm_id(const struct domain *d)
> {
> /* +1 since 0 is reserved for the hypervisor in FF-A */
> return d->domain_id + 1;
> }
>
> +static inline struct domain *ffa_rcu_lock_domain_by_vm_id(uint16_t vm_id)
> +{
> + ASSERT(vm_id);
> +
> + /* -1 to match ffa_get_vm_id() */
> + return rcu_lock_domain_by_id(vm_id - 1);
> +}
> +
> static inline void ffa_set_regs(struct cpu_user_regs *regs, register_t v0,
> register_t v1, register_t v2, register_t v3,
> register_t v4, register_t v5, register_t v6,
> diff --git a/xen/arch/arm/tee/tee.c b/xen/arch/arm/tee/tee.c
> index cb65f187f51f..1e890ecc2932 100644
> --- a/xen/arch/arm/tee/tee.c
> +++ b/xen/arch/arm/tee/tee.c
> @@ -94,7 +94,7 @@ static int __init tee_init(void)
> return 0;
> }
>
> -__initcall(tee_init);
> +presmp_initcall(tee_init);
>
> void __init init_tee_interrupt(void)
> {
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 289af81bd69d..e2412a17474e 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -505,6 +505,7 @@ typedef uint64_t xen_callback_t;
> #define GUEST_MAX_VCPUS 128
>
> /* Interrupts */
> +
> #define GUEST_TIMER_VIRT_PPI 27
> #define GUEST_TIMER_PHYS_S_PPI 29
> #define GUEST_TIMER_PHYS_NS_PPI 30
> @@ -515,6 +516,19 @@ typedef uint64_t xen_callback_t;
> #define GUEST_VIRTIO_MMIO_SPI_FIRST 33
> #define GUEST_VIRTIO_MMIO_SPI_LAST 43
>
> +/*
> + * SGI is the preferred delivery mechanism of FF-A pending notifications or
> + * schedule recveive interrupt. SGIs 8-15 are normally not used by a guest
> + * as they in a non-virtualized system typically are assigned to the secure
> + * world. Here we're free to use SGI 8-15 since they are virtual and have
> + * nothing to do with the secure world.
> + *
> + * For partitioning of SGIs see also Arm Base System Architecture v1.0C,
> + * https://developer.arm.com/documentation/den0094/
> + */
> +#define GUEST_FFA_NOTIF_PEND_INTR_ID 8
> +#define GUEST_FFA_SCHEDULE_RECV_INTR_ID 9
> +
> /* PSCI functions */
> #define PSCI_cpu_suspend 0
> #define PSCI_cpu_off 1
> --
> 2.34.1
>
Cheers
Bertrand
Hi Bertrand,
On Fri, May 31, 2024 at 4:28 PM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Jens,
>
> > On 29 May 2024, at 09:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Add support for FF-A notifications, currently limited to an SP (Secure
> > Partition) sending an asynchronous notification to a guest.
> >
> > Guests and Xen itself are made aware of pending notifications with an
> > interrupt. The interrupt handler triggers a tasklet to retrieve the
> > notifications using the FF-A ABI and deliver them to their destinations.
> >
> > Update ffa_partinfo_domain_init() to return error code like
> > ffa_notif_domain_init().
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> > v4->v5:
> > - Move the freeing of d->arch.tee to the new TEE mediator free_domain_ctx
> > callback to make the context accessible during rcu_lock_domain_by_id()
> > from a tasklet
> > - Initialize interrupt handlers for secondary CPUs from the new TEE mediator
> > init_interrupt() callback
> > - Restore the ffa_probe() from v3, that is, remove the
> > presmp_initcall(ffa_init) approach and use ffa_probe() as usual now that we
> > have the init_interrupt callback.
> > - A tasklet is added to handle the Schedule Receiver interrupt. The tasklet
> > finds each relevant domain with rcu_lock_domain_by_id() which now is enough
> > to guarantee that the FF-A context can be accessed.
> > - The notification interrupt handler only schedules the notification
> > tasklet mentioned above
> >
> > v3->v4:
> > - Add another note on FF-A limitations
> > - Clear secure_pending in ffa_handle_notification_get() if both SP and SPM
> > bitmaps are retrieved
> > - ASSERT that ffa_rcu_lock_domain_by_vm_id() isn't passed the vm_id FF-A
> > uses for Xen itself
> > - Replace the get_domain_by_id() call done via ffa_get_domain_by_vm_id() in
> > notif_irq_handler() with a call to rcu_lock_live_remote_domain_by_id() via
> > ffa_rcu_lock_domain_by_vm_id()
> > - Remove spinlock in struct ffa_ctx_notif and use atomic functions as needed
> > to access and update the secure_pending field
> > - In notif_irq_handler(), look for the first online CPU instead of assuming
> > that the first CPU is online
> > - Initialize FF-A via presmp_initcall() before the other CPUs are online,
> > use register_cpu_notifier() to install the interrupt handler
> > notif_irq_handler()
> > - Update commit message to reflect recent updates
> >
> > v2->v3:
> > - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
> > FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
> > - Register the Xen SRI handler on each CPU using on_selected_cpus() and
> > setup_irq()
> > - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
> > doesn't conflict with static SGI handlers
> >
> > v1->v2:
> > - Addressing review comments
> > - Change ffa_handle_notification_{bind,unbind,set}() to take struct
> > cpu_user_regs *regs as argument.
> > - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
> > an error code.
> > - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
> > ---
> > xen/arch/arm/tee/Makefile | 1 +
> > xen/arch/arm/tee/ffa.c | 72 +++++-
> > xen/arch/arm/tee/ffa_notif.c | 409 ++++++++++++++++++++++++++++++++
> > xen/arch/arm/tee/ffa_partinfo.c | 9 +-
> > xen/arch/arm/tee/ffa_private.h | 56 ++++-
> > xen/arch/arm/tee/tee.c | 2 +-
> > xen/include/public/arch-arm.h | 14 ++
> > 7 files changed, 548 insertions(+), 15 deletions(-)
> > create mode 100644 xen/arch/arm/tee/ffa_notif.c
> >
[...]
> >
> > @@ -517,8 +567,10 @@ err_rxtx_destroy:
> > static const struct tee_mediator_ops ffa_ops =
> > {
> > .probe = ffa_probe,
> > + .init_interrupt = ffa_notif_init_interrupt,
>
> With the previous change on init secondary i would suggest to
> have a ffa_init_secondary here actually calling the ffa_notif_init_interrupt.
Yes, that makes sense. I'll update.
>
> > .domain_init = ffa_domain_init,
> > .domain_teardown = ffa_domain_teardown,
> > + .free_domain_ctx = ffa_free_domain_ctx,
> > .relinquish_resources = ffa_relinquish_resources,
> > .handle_call = ffa_handle_call,
> > };
> > diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
> > new file mode 100644
> > index 000000000000..e8e8b62590b3
> > --- /dev/null
> > +++ b/xen/arch/arm/tee/ffa_notif.c
[...]
> > +static void notif_vm_pend_intr(uint16_t vm_id)
> > +{
> > + struct ffa_ctx *ctx;
> > + struct domain *d;
> > + struct vcpu *v;
> > +
> > + /*
> > + * vm_id == 0 means a notifications pending for Xen itself, but
> > + * we don't support that yet.
> > + */
> > + if ( !vm_id )
> > + return;
> > +
> > + d = ffa_rcu_lock_domain_by_vm_id(vm_id);
> > + if ( !d )
> > + return;
> > +
> > + ctx = d->arch.tee;
> > + if ( !ctx )
> > + goto out_unlock;
>
> In both previous cases you are silently ignoring an interrupt
> due to an internal error.
> Is this something that we should trace ? maybe just debug ?
>
> Could you add a comment to explain why this could happen
> (when possible) or not and the possible side effects ?
>
> The second one would be a notification for a domain without
> FF-A enabled which is ok but i am a bit more wondering on
> the first one.
The SPMC must be out of sync in both cases. I've been looking for a
window where that can happen, but I can't find any. SPMC is called
with FFA_NOTIFICATION_BITMAP_DESTROY during domain teardown so the
SPMC shouldn't try to deliver any notifications after that.
In the second case, the domain ID might have been reused for a domain
without FF-A enabled, but the SPMC should have known that already.
I'll add comments and prints since these errors indicate a bug
somewhere.
>
> > +
> > + /*
> > + * arch.tee is freed from complete_domain_destroy() so the RCU lock
> > + * guarantees that the data structure isn't freed while we're accessing
> > + * it.
> > + */
> > + ACCESS_ONCE(ctx->notif.secure_pending) = true;
> > +
> > + /*
> > + * Since we're only delivering global notification, always
> > + * deliver to the first online vCPU. It doesn't matter
> > + * which we chose, as long as it's available.
> > + */
> > + for_each_vcpu(d, v)
> > + {
> > + if ( is_vcpu_online(v) )
> > + {
> > + vgic_inject_irq(d, v, GUEST_FFA_NOTIF_PEND_INTR_ID,
> > + true);
> > + break;
> > + }
> > + }
> > + if ( !v )
> > + printk(XENLOG_ERR "ffa: can't inject NPI, all vCPUs offline\n");
> > +
> > +out_unlock:
> > + rcu_unlock_domain(d);
> > +}
Thanks,
Jens
Hi Jens,
On 03/06/2024 10:01, Jens Wiklander wrote:
> On Fri, May 31, 2024 at 4:28 PM Bertrand Marquis
> <Bertrand.Marquis@arm.com> wrote:
>>
>> Hi Jens,
>>
>>> On 29 May 2024, at 09:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>>
>>> Add support for FF-A notifications, currently limited to an SP (Secure
>>> Partition) sending an asynchronous notification to a guest.
>>>
>>> Guests and Xen itself are made aware of pending notifications with an
>>> interrupt. The interrupt handler triggers a tasklet to retrieve the
>>> notifications using the FF-A ABI and deliver them to their destinations.
>>>
>>> Update ffa_partinfo_domain_init() to return error code like
>>> ffa_notif_domain_init().
>>>
>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>>> ---
>>> v4->v5:
>>> - Move the freeing of d->arch.tee to the new TEE mediator free_domain_ctx
>>> callback to make the context accessible during rcu_lock_domain_by_id()
>>> from a tasklet
>>> - Initialize interrupt handlers for secondary CPUs from the new TEE mediator
>>> init_interrupt() callback
>>> - Restore the ffa_probe() from v3, that is, remove the
>>> presmp_initcall(ffa_init) approach and use ffa_probe() as usual now that we
>>> have the init_interrupt callback.
>>> - A tasklet is added to handle the Schedule Receiver interrupt. The tasklet
>>> finds each relevant domain with rcu_lock_domain_by_id() which now is enough
>>> to guarantee that the FF-A context can be accessed.
>>> - The notification interrupt handler only schedules the notification
>>> tasklet mentioned above
>>>
>>> v3->v4:
>>> - Add another note on FF-A limitations
>>> - Clear secure_pending in ffa_handle_notification_get() if both SP and SPM
>>> bitmaps are retrieved
>>> - ASSERT that ffa_rcu_lock_domain_by_vm_id() isn't passed the vm_id FF-A
>>> uses for Xen itself
>>> - Replace the get_domain_by_id() call done via ffa_get_domain_by_vm_id() in
>>> notif_irq_handler() with a call to rcu_lock_live_remote_domain_by_id() via
>>> ffa_rcu_lock_domain_by_vm_id()
>>> - Remove spinlock in struct ffa_ctx_notif and use atomic functions as needed
>>> to access and update the secure_pending field
>>> - In notif_irq_handler(), look for the first online CPU instead of assuming
>>> that the first CPU is online
>>> - Initialize FF-A via presmp_initcall() before the other CPUs are online,
>>> use register_cpu_notifier() to install the interrupt handler
>>> notif_irq_handler()
>>> - Update commit message to reflect recent updates
>>>
>>> v2->v3:
>>> - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
>>> FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
>>> - Register the Xen SRI handler on each CPU using on_selected_cpus() and
>>> setup_irq()
>>> - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
>>> doesn't conflict with static SGI handlers
>>>
>>> v1->v2:
>>> - Addressing review comments
>>> - Change ffa_handle_notification_{bind,unbind,set}() to take struct
>>> cpu_user_regs *regs as argument.
>>> - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
>>> an error code.
>>> - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
>>> ---
>>> xen/arch/arm/tee/Makefile | 1 +
>>> xen/arch/arm/tee/ffa.c | 72 +++++-
>>> xen/arch/arm/tee/ffa_notif.c | 409 ++++++++++++++++++++++++++++++++
>>> xen/arch/arm/tee/ffa_partinfo.c | 9 +-
>>> xen/arch/arm/tee/ffa_private.h | 56 ++++-
>>> xen/arch/arm/tee/tee.c | 2 +-
>>> xen/include/public/arch-arm.h | 14 ++
>>> 7 files changed, 548 insertions(+), 15 deletions(-)
>>> create mode 100644 xen/arch/arm/tee/ffa_notif.c
>>>
> [...]
>>>
>>> @@ -517,8 +567,10 @@ err_rxtx_destroy:
>>> static const struct tee_mediator_ops ffa_ops =
>>> {
>>> .probe = ffa_probe,
>>> + .init_interrupt = ffa_notif_init_interrupt,
>>
>> With the previous change on init secondary i would suggest to
>> have a ffa_init_secondary here actually calling the ffa_notif_init_interrupt.
>
> Yes, that makes sense. I'll update.
>
>>
>>> .domain_init = ffa_domain_init,
>>> .domain_teardown = ffa_domain_teardown,
>>> + .free_domain_ctx = ffa_free_domain_ctx,
>>> .relinquish_resources = ffa_relinquish_resources,
>>> .handle_call = ffa_handle_call,
>>> };
>>> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
>>> new file mode 100644
>>> index 000000000000..e8e8b62590b3
>>> --- /dev/null
>>> +++ b/xen/arch/arm/tee/ffa_notif.c
> [...]
>>> +static void notif_vm_pend_intr(uint16_t vm_id)
>>> +{
>>> + struct ffa_ctx *ctx;
>>> + struct domain *d;
>>> + struct vcpu *v;
>>> +
>>> + /*
>>> + * vm_id == 0 means a notifications pending for Xen itself, but
>>> + * we don't support that yet.
>>> + */
>>> + if ( !vm_id )
>>> + return;
>>> +
>>> + d = ffa_rcu_lock_domain_by_vm_id(vm_id);
>>> + if ( !d )
>>> + return;
>>> +
>>> + ctx = d->arch.tee;
>>> + if ( !ctx )
>>> + goto out_unlock;
>>
>> In both previous cases you are silently ignoring an interrupt
>> due to an internal error.
>> Is this something that we should trace ? maybe just debug ?
>>
>> Could you add a comment to explain why this could happen
>> (when possible) or not and the possible side effects ?
>>
>> The second one would be a notification for a domain without
>> FF-A enabled which is ok but i am a bit more wondering on
>> the first one.
>
> The SPMC must be out of sync in both cases. I've been looking for a
> window where that can happen, but I can't find any. SPMC is called
> with FFA_NOTIFICATION_BITMAP_DESTROY during domain teardown so the
> SPMC shouldn't try to deliver any notifications after that.
I don't think I agree with the conclusion. I believe, this can also
happen in normal operation.
For example, the SPMC could have trigger the interrupt before
FFA_NOTIFICATION_BITMAP_DESTROY but Xen didn't handle the interrupt (or
run the tasklet) until later.
This could be at the time where the domain has been fully destroyed or
even when...
> In the second case, the domain ID might have been reused for a domain
> without FF-A enabled, but the SPMC should have known that already.
... a new domain has been created. Although, the latter is rather unlikely.
So what if the new domain has FFA enabled? Is there any potential
security issue?
Cheers,
--
Julien Grall
Hi Julien,
On Mon, Jun 3, 2024 at 11:12 AM Julien Grall <julien@xen.org> wrote:
>
> Hi Jens,
>
> On 03/06/2024 10:01, Jens Wiklander wrote:
> > On Fri, May 31, 2024 at 4:28 PM Bertrand Marquis
> > <Bertrand.Marquis@arm.com> wrote:
> >>
> >> Hi Jens,
> >>
> >>> On 29 May 2024, at 09:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >>>
> >>> Add support for FF-A notifications, currently limited to an SP (Secure
> >>> Partition) sending an asynchronous notification to a guest.
> >>>
> >>> Guests and Xen itself are made aware of pending notifications with an
> >>> interrupt. The interrupt handler triggers a tasklet to retrieve the
> >>> notifications using the FF-A ABI and deliver them to their destinations.
> >>>
> >>> Update ffa_partinfo_domain_init() to return error code like
> >>> ffa_notif_domain_init().
> >>>
> >>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> >>> ---
> >>> v4->v5:
> >>> - Move the freeing of d->arch.tee to the new TEE mediator free_domain_ctx
> >>> callback to make the context accessible during rcu_lock_domain_by_id()
> >>> from a tasklet
> >>> - Initialize interrupt handlers for secondary CPUs from the new TEE mediator
> >>> init_interrupt() callback
> >>> - Restore the ffa_probe() from v3, that is, remove the
> >>> presmp_initcall(ffa_init) approach and use ffa_probe() as usual now that we
> >>> have the init_interrupt callback.
> >>> - A tasklet is added to handle the Schedule Receiver interrupt. The tasklet
> >>> finds each relevant domain with rcu_lock_domain_by_id() which now is enough
> >>> to guarantee that the FF-A context can be accessed.
> >>> - The notification interrupt handler only schedules the notification
> >>> tasklet mentioned above
> >>>
> >>> v3->v4:
> >>> - Add another note on FF-A limitations
> >>> - Clear secure_pending in ffa_handle_notification_get() if both SP and SPM
> >>> bitmaps are retrieved
> >>> - ASSERT that ffa_rcu_lock_domain_by_vm_id() isn't passed the vm_id FF-A
> >>> uses for Xen itself
> >>> - Replace the get_domain_by_id() call done via ffa_get_domain_by_vm_id() in
> >>> notif_irq_handler() with a call to rcu_lock_live_remote_domain_by_id() via
> >>> ffa_rcu_lock_domain_by_vm_id()
> >>> - Remove spinlock in struct ffa_ctx_notif and use atomic functions as needed
> >>> to access and update the secure_pending field
> >>> - In notif_irq_handler(), look for the first online CPU instead of assuming
> >>> that the first CPU is online
> >>> - Initialize FF-A via presmp_initcall() before the other CPUs are online,
> >>> use register_cpu_notifier() to install the interrupt handler
> >>> notif_irq_handler()
> >>> - Update commit message to reflect recent updates
> >>>
> >>> v2->v3:
> >>> - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
> >>> FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
> >>> - Register the Xen SRI handler on each CPU using on_selected_cpus() and
> >>> setup_irq()
> >>> - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
> >>> doesn't conflict with static SGI handlers
> >>>
> >>> v1->v2:
> >>> - Addressing review comments
> >>> - Change ffa_handle_notification_{bind,unbind,set}() to take struct
> >>> cpu_user_regs *regs as argument.
> >>> - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
> >>> an error code.
> >>> - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
> >>> ---
> >>> xen/arch/arm/tee/Makefile | 1 +
> >>> xen/arch/arm/tee/ffa.c | 72 +++++-
> >>> xen/arch/arm/tee/ffa_notif.c | 409 ++++++++++++++++++++++++++++++++
> >>> xen/arch/arm/tee/ffa_partinfo.c | 9 +-
> >>> xen/arch/arm/tee/ffa_private.h | 56 ++++-
> >>> xen/arch/arm/tee/tee.c | 2 +-
> >>> xen/include/public/arch-arm.h | 14 ++
> >>> 7 files changed, 548 insertions(+), 15 deletions(-)
> >>> create mode 100644 xen/arch/arm/tee/ffa_notif.c
> >>>
> > [...]
> >>>
> >>> @@ -517,8 +567,10 @@ err_rxtx_destroy:
> >>> static const struct tee_mediator_ops ffa_ops =
> >>> {
> >>> .probe = ffa_probe,
> >>> + .init_interrupt = ffa_notif_init_interrupt,
> >>
> >> With the previous change on init secondary i would suggest to
> >> have a ffa_init_secondary here actually calling the ffa_notif_init_interrupt.
> >
> > Yes, that makes sense. I'll update.
> >
> >>
> >>> .domain_init = ffa_domain_init,
> >>> .domain_teardown = ffa_domain_teardown,
> >>> + .free_domain_ctx = ffa_free_domain_ctx,
> >>> .relinquish_resources = ffa_relinquish_resources,
> >>> .handle_call = ffa_handle_call,
> >>> };
> >>> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
> >>> new file mode 100644
> >>> index 000000000000..e8e8b62590b3
> >>> --- /dev/null
> >>> +++ b/xen/arch/arm/tee/ffa_notif.c
> > [...]
> >>> +static void notif_vm_pend_intr(uint16_t vm_id)
> >>> +{
> >>> + struct ffa_ctx *ctx;
> >>> + struct domain *d;
> >>> + struct vcpu *v;
> >>> +
> >>> + /*
> >>> + * vm_id == 0 means a notifications pending for Xen itself, but
> >>> + * we don't support that yet.
> >>> + */
> >>> + if ( !vm_id )
> >>> + return;
> >>> +
> >>> + d = ffa_rcu_lock_domain_by_vm_id(vm_id);
> >>> + if ( !d )
> >>> + return;
> >>> +
> >>> + ctx = d->arch.tee;
> >>> + if ( !ctx )
> >>> + goto out_unlock;
> >>
> >> In both previous cases you are silently ignoring an interrupt
> >> due to an internal error.
> >> Is this something that we should trace ? maybe just debug ?
> >>
> >> Could you add a comment to explain why this could happen
> >> (when possible) or not and the possible side effects ?
> >>
> >> The second one would be a notification for a domain without
> >> FF-A enabled which is ok but i am a bit more wondering on
> >> the first one.
> >
> > The SPMC must be out of sync in both cases. I've been looking for a
> > window where that can happen, but I can't find any. SPMC is called
> > with FFA_NOTIFICATION_BITMAP_DESTROY during domain teardown so the
> > SPMC shouldn't try to deliver any notifications after that.
>
> I don't think I agree with the conclusion. I believe, this can also
> happen in normal operation.
>
> For example, the SPMC could have trigger the interrupt before
> FFA_NOTIFICATION_BITMAP_DESTROY but Xen didn't handle the interrupt (or
> run the tasklet) until later.
You're right, there is a window. Delayed handling is OK since
FFA_NOTIFICATION_INFO_GET_64 is invoked from the tasklet, but there is
a window if the tasklet is suspended or another core destroys the
domain before the tasklet has called ffa_rcu_lock_domain_by_vm_id().
So far it's harmless and I guess we can afford a print.
>
> This could be at the time where the domain has been fully destroyed or
> even when...
>
> > In the second case, the domain ID might have been reused for a domain
> > without FF-A enabled, but the SPMC should have known that already.
>
> ... a new domain has been created. Although, the latter is rather unlikely.
>
> So what if the new domain has FFA enabled? Is there any potential
> security issue?
In this case, we'll inject an NPI in the guest, but when it invokes
FFA_NOTIFICATION_GET it will get accurate information from the SPMC.
The worst case is a spurious NPI. This shouldn't be a security issue.
Thanks,
Jens
On 03/06/2024 10:50, Jens Wiklander wrote:
> Hi Julien,
Hi Jens,
> On Mon, Jun 3, 2024 at 11:12 AM Julien Grall <julien@xen.org> wrote:
>>
>> Hi Jens,
>>
>> On 03/06/2024 10:01, Jens Wiklander wrote:
>>> On Fri, May 31, 2024 at 4:28 PM Bertrand Marquis
>>> <Bertrand.Marquis@arm.com> wrote:
>>>>
>>>> Hi Jens,
>>>>
>>>>> On 29 May 2024, at 09:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>>>>
>>>>> Add support for FF-A notifications, currently limited to an SP (Secure
>>>>> Partition) sending an asynchronous notification to a guest.
>>>>>
>>>>> Guests and Xen itself are made aware of pending notifications with an
>>>>> interrupt. The interrupt handler triggers a tasklet to retrieve the
>>>>> notifications using the FF-A ABI and deliver them to their destinations.
>>>>>
>>>>> Update ffa_partinfo_domain_init() to return error code like
>>>>> ffa_notif_domain_init().
>>>>>
>>>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>>>>> ---
>>>>> v4->v5:
>>>>> - Move the freeing of d->arch.tee to the new TEE mediator free_domain_ctx
>>>>> callback to make the context accessible during rcu_lock_domain_by_id()
>>>>> from a tasklet
>>>>> - Initialize interrupt handlers for secondary CPUs from the new TEE mediator
>>>>> init_interrupt() callback
>>>>> - Restore the ffa_probe() from v3, that is, remove the
>>>>> presmp_initcall(ffa_init) approach and use ffa_probe() as usual now that we
>>>>> have the init_interrupt callback.
>>>>> - A tasklet is added to handle the Schedule Receiver interrupt. The tasklet
>>>>> finds each relevant domain with rcu_lock_domain_by_id() which now is enough
>>>>> to guarantee that the FF-A context can be accessed.
>>>>> - The notification interrupt handler only schedules the notification
>>>>> tasklet mentioned above
>>>>>
>>>>> v3->v4:
>>>>> - Add another note on FF-A limitations
>>>>> - Clear secure_pending in ffa_handle_notification_get() if both SP and SPM
>>>>> bitmaps are retrieved
>>>>> - ASSERT that ffa_rcu_lock_domain_by_vm_id() isn't passed the vm_id FF-A
>>>>> uses for Xen itself
>>>>> - Replace the get_domain_by_id() call done via ffa_get_domain_by_vm_id() in
>>>>> notif_irq_handler() with a call to rcu_lock_live_remote_domain_by_id() via
>>>>> ffa_rcu_lock_domain_by_vm_id()
>>>>> - Remove spinlock in struct ffa_ctx_notif and use atomic functions as needed
>>>>> to access and update the secure_pending field
>>>>> - In notif_irq_handler(), look for the first online CPU instead of assuming
>>>>> that the first CPU is online
>>>>> - Initialize FF-A via presmp_initcall() before the other CPUs are online,
>>>>> use register_cpu_notifier() to install the interrupt handler
>>>>> notif_irq_handler()
>>>>> - Update commit message to reflect recent updates
>>>>>
>>>>> v2->v3:
>>>>> - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
>>>>> FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
>>>>> - Register the Xen SRI handler on each CPU using on_selected_cpus() and
>>>>> setup_irq()
>>>>> - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
>>>>> doesn't conflict with static SGI handlers
>>>>>
>>>>> v1->v2:
>>>>> - Addressing review comments
>>>>> - Change ffa_handle_notification_{bind,unbind,set}() to take struct
>>>>> cpu_user_regs *regs as argument.
>>>>> - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
>>>>> an error code.
>>>>> - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
>>>>> ---
>>>>> xen/arch/arm/tee/Makefile | 1 +
>>>>> xen/arch/arm/tee/ffa.c | 72 +++++-
>>>>> xen/arch/arm/tee/ffa_notif.c | 409 ++++++++++++++++++++++++++++++++
>>>>> xen/arch/arm/tee/ffa_partinfo.c | 9 +-
>>>>> xen/arch/arm/tee/ffa_private.h | 56 ++++-
>>>>> xen/arch/arm/tee/tee.c | 2 +-
>>>>> xen/include/public/arch-arm.h | 14 ++
>>>>> 7 files changed, 548 insertions(+), 15 deletions(-)
>>>>> create mode 100644 xen/arch/arm/tee/ffa_notif.c
>>>>>
>>> [...]
>>>>>
>>>>> @@ -517,8 +567,10 @@ err_rxtx_destroy:
>>>>> static const struct tee_mediator_ops ffa_ops =
>>>>> {
>>>>> .probe = ffa_probe,
>>>>> + .init_interrupt = ffa_notif_init_interrupt,
>>>>
>>>> With the previous change on init secondary i would suggest to
>>>> have a ffa_init_secondary here actually calling the ffa_notif_init_interrupt.
>>>
>>> Yes, that makes sense. I'll update.
>>>
>>>>
>>>>> .domain_init = ffa_domain_init,
>>>>> .domain_teardown = ffa_domain_teardown,
>>>>> + .free_domain_ctx = ffa_free_domain_ctx,
>>>>> .relinquish_resources = ffa_relinquish_resources,
>>>>> .handle_call = ffa_handle_call,
>>>>> };
>>>>> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
>>>>> new file mode 100644
>>>>> index 000000000000..e8e8b62590b3
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/arm/tee/ffa_notif.c
>>> [...]
>>>>> +static void notif_vm_pend_intr(uint16_t vm_id)
>>>>> +{
>>>>> + struct ffa_ctx *ctx;
>>>>> + struct domain *d;
>>>>> + struct vcpu *v;
>>>>> +
>>>>> + /*
>>>>> + * vm_id == 0 means a notifications pending for Xen itself, but
>>>>> + * we don't support that yet.
>>>>> + */
>>>>> + if ( !vm_id )
>>>>> + return;
>>>>> +
>>>>> + d = ffa_rcu_lock_domain_by_vm_id(vm_id);
>>>>> + if ( !d )
>>>>> + return;
>>>>> +
>>>>> + ctx = d->arch.tee;
>>>>> + if ( !ctx )
>>>>> + goto out_unlock;
>>>>
>>>> In both previous cases you are silently ignoring an interrupt
>>>> due to an internal error.
>>>> Is this something that we should trace ? maybe just debug ?
>>>>
>>>> Could you add a comment to explain why this could happen
>>>> (when possible) or not and the possible side effects ?
>>>>
>>>> The second one would be a notification for a domain without
>>>> FF-A enabled which is ok but i am a bit more wondering on
>>>> the first one.
>>>
>>> The SPMC must be out of sync in both cases. I've been looking for a
>>> window where that can happen, but I can't find any. SPMC is called
>>> with FFA_NOTIFICATION_BITMAP_DESTROY during domain teardown so the
>>> SPMC shouldn't try to deliver any notifications after that.
>>
>> I don't think I agree with the conclusion. I believe, this can also
>> happen in normal operation.
>>
>> For example, the SPMC could have trigger the interrupt before
>> FFA_NOTIFICATION_BITMAP_DESTROY but Xen didn't handle the interrupt (or
>> run the tasklet) until later.
>
> You're right, there is a window. Delayed handling is OK since
> FFA_NOTIFICATION_INFO_GET_64 is invoked from the tasklet, but there is
> a window if the tasklet is suspended or another core destroys the
> domain before the tasklet has called ffa_rcu_lock_domain_by_vm_id().
> So far it's harmless and I guess we can afford a print.
I think it would confuse more the user than anything else because this
is an expected race. If we wanted to print a message, then I would argue
it should be in the case where...
>
>>
>> This could be at the time where the domain has been fully destroyed or
>> even when...
>>
>>> In the second case, the domain ID might have been reused for a domain
>>> without FF-A enabled, but the SPMC should have known that already.
>>
>> ... a new domain has been created. Although, the latter is rather unlikely.
>>
>> So what if the new domain has FFA enabled? Is there any potential
>> security issue?
>
> In this case, we'll inject an NPI in the guest, but when it invokes
> FFA_NOTIFICATION_GET it will get accurate information from the SPMC.
> The worst case is a spurious NPI. This shouldn't be a security issue.
... we inject the interrupt to the "wrong" domain. But I also understand
that it would be difficult for Xen to detect it.
So I would say no print should be needed. Bertrand, what do you think?
Cheers,
--
Julien Grall
Hi,
> On 4 Jun 2024, at 12:24, Julien Grall <julien@xen.org> wrote:
>
>
>
> On 03/06/2024 10:50, Jens Wiklander wrote:
>> Hi Julien,
>
> Hi Jens,
>
>
>> On Mon, Jun 3, 2024 at 11:12 AM Julien Grall <julien@xen.org> wrote:
>>>
>>> Hi Jens,
>>>
>>> On 03/06/2024 10:01, Jens Wiklander wrote:
>>>> On Fri, May 31, 2024 at 4:28 PM Bertrand Marquis
>>>> <Bertrand.Marquis@arm.com> wrote:
>>>>>
>>>>> Hi Jens,
>>>>>
>>>>>> On 29 May 2024, at 09:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>>>>>
>>>>>> Add support for FF-A notifications, currently limited to an SP (Secure
>>>>>> Partition) sending an asynchronous notification to a guest.
>>>>>>
>>>>>> Guests and Xen itself are made aware of pending notifications with an
>>>>>> interrupt. The interrupt handler triggers a tasklet to retrieve the
>>>>>> notifications using the FF-A ABI and deliver them to their destinations.
>>>>>>
>>>>>> Update ffa_partinfo_domain_init() to return error code like
>>>>>> ffa_notif_domain_init().
>>>>>>
>>>>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>>>>>> ---
>>>>>> v4->v5:
>>>>>> - Move the freeing of d->arch.tee to the new TEE mediator free_domain_ctx
>>>>>> callback to make the context accessible during rcu_lock_domain_by_id()
>>>>>> from a tasklet
>>>>>> - Initialize interrupt handlers for secondary CPUs from the new TEE mediator
>>>>>> init_interrupt() callback
>>>>>> - Restore the ffa_probe() from v3, that is, remove the
>>>>>> presmp_initcall(ffa_init) approach and use ffa_probe() as usual now that we
>>>>>> have the init_interrupt callback.
>>>>>> - A tasklet is added to handle the Schedule Receiver interrupt. The tasklet
>>>>>> finds each relevant domain with rcu_lock_domain_by_id() which now is enough
>>>>>> to guarantee that the FF-A context can be accessed.
>>>>>> - The notification interrupt handler only schedules the notification
>>>>>> tasklet mentioned above
>>>>>>
>>>>>> v3->v4:
>>>>>> - Add another note on FF-A limitations
>>>>>> - Clear secure_pending in ffa_handle_notification_get() if both SP and SPM
>>>>>> bitmaps are retrieved
>>>>>> - ASSERT that ffa_rcu_lock_domain_by_vm_id() isn't passed the vm_id FF-A
>>>>>> uses for Xen itself
>>>>>> - Replace the get_domain_by_id() call done via ffa_get_domain_by_vm_id() in
>>>>>> notif_irq_handler() with a call to rcu_lock_live_remote_domain_by_id() via
>>>>>> ffa_rcu_lock_domain_by_vm_id()
>>>>>> - Remove spinlock in struct ffa_ctx_notif and use atomic functions as needed
>>>>>> to access and update the secure_pending field
>>>>>> - In notif_irq_handler(), look for the first online CPU instead of assuming
>>>>>> that the first CPU is online
>>>>>> - Initialize FF-A via presmp_initcall() before the other CPUs are online,
>>>>>> use register_cpu_notifier() to install the interrupt handler
>>>>>> notif_irq_handler()
>>>>>> - Update commit message to reflect recent updates
>>>>>>
>>>>>> v2->v3:
>>>>>> - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
>>>>>> FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
>>>>>> - Register the Xen SRI handler on each CPU using on_selected_cpus() and
>>>>>> setup_irq()
>>>>>> - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
>>>>>> doesn't conflict with static SGI handlers
>>>>>>
>>>>>> v1->v2:
>>>>>> - Addressing review comments
>>>>>> - Change ffa_handle_notification_{bind,unbind,set}() to take struct
>>>>>> cpu_user_regs *regs as argument.
>>>>>> - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
>>>>>> an error code.
>>>>>> - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
>>>>>> ---
>>>>>> xen/arch/arm/tee/Makefile | 1 +
>>>>>> xen/arch/arm/tee/ffa.c | 72 +++++-
>>>>>> xen/arch/arm/tee/ffa_notif.c | 409 ++++++++++++++++++++++++++++++++
>>>>>> xen/arch/arm/tee/ffa_partinfo.c | 9 +-
>>>>>> xen/arch/arm/tee/ffa_private.h | 56 ++++-
>>>>>> xen/arch/arm/tee/tee.c | 2 +-
>>>>>> xen/include/public/arch-arm.h | 14 ++
>>>>>> 7 files changed, 548 insertions(+), 15 deletions(-)
>>>>>> create mode 100644 xen/arch/arm/tee/ffa_notif.c
>>>>>>
>>>> [...]
>>>>>>
>>>>>> @@ -517,8 +567,10 @@ err_rxtx_destroy:
>>>>>> static const struct tee_mediator_ops ffa_ops =
>>>>>> {
>>>>>> .probe = ffa_probe,
>>>>>> + .init_interrupt = ffa_notif_init_interrupt,
>>>>>
>>>>> With the previous change on init secondary i would suggest to
>>>>> have a ffa_init_secondary here actually calling the ffa_notif_init_interrupt.
>>>>
>>>> Yes, that makes sense. I'll update.
>>>>
>>>>>
>>>>>> .domain_init = ffa_domain_init,
>>>>>> .domain_teardown = ffa_domain_teardown,
>>>>>> + .free_domain_ctx = ffa_free_domain_ctx,
>>>>>> .relinquish_resources = ffa_relinquish_resources,
>>>>>> .handle_call = ffa_handle_call,
>>>>>> };
>>>>>> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..e8e8b62590b3
>>>>>> --- /dev/null
>>>>>> +++ b/xen/arch/arm/tee/ffa_notif.c
>>>> [...]
>>>>>> +static void notif_vm_pend_intr(uint16_t vm_id)
>>>>>> +{
>>>>>> + struct ffa_ctx *ctx;
>>>>>> + struct domain *d;
>>>>>> + struct vcpu *v;
>>>>>> +
>>>>>> + /*
>>>>>> + * vm_id == 0 means a notifications pending for Xen itself, but
>>>>>> + * we don't support that yet.
>>>>>> + */
>>>>>> + if ( !vm_id )
>>>>>> + return;
>>>>>> +
>>>>>> + d = ffa_rcu_lock_domain_by_vm_id(vm_id);
>>>>>> + if ( !d )
>>>>>> + return;
>>>>>> +
>>>>>> + ctx = d->arch.tee;
>>>>>> + if ( !ctx )
>>>>>> + goto out_unlock;
>>>>>
>>>>> In both previous cases you are silently ignoring an interrupt
>>>>> due to an internal error.
>>>>> Is this something that we should trace ? maybe just debug ?
>>>>>
>>>>> Could you add a comment to explain why this could happen
>>>>> (when possible) or not and the possible side effects ?
>>>>>
>>>>> The second one would be a notification for a domain without
>>>>> FF-A enabled which is ok but i am a bit more wondering on
>>>>> the first one.
>>>>
>>>> The SPMC must be out of sync in both cases. I've been looking for a
>>>> window where that can happen, but I can't find any. SPMC is called
>>>> with FFA_NOTIFICATION_BITMAP_DESTROY during domain teardown so the
>>>> SPMC shouldn't try to deliver any notifications after that.
>>>
>>> I don't think I agree with the conclusion. I believe, this can also
>>> happen in normal operation.
>>>
>>> For example, the SPMC could have trigger the interrupt before
>>> FFA_NOTIFICATION_BITMAP_DESTROY but Xen didn't handle the interrupt (or
>>> run the tasklet) until later.
>> You're right, there is a window. Delayed handling is OK since
>> FFA_NOTIFICATION_INFO_GET_64 is invoked from the tasklet, but there is
>> a window if the tasklet is suspended or another core destroys the
>> domain before the tasklet has called ffa_rcu_lock_domain_by_vm_id().
>> So far it's harmless and I guess we can afford a print.
>
> I think it would confuse more the user than anything else because this is an expected race. If we wanted to print a message, then I would argue it should be in the case where...
>
>>>
>>> This could be at the time where the domain has been fully destroyed or
>>> even when...
>>>
>>>> In the second case, the domain ID might have been reused for a domain
>>>> without FF-A enabled, but the SPMC should have known that already.
>>>
>>> ... a new domain has been created. Although, the latter is rather unlikely.
>>>
>>> So what if the new domain has FFA enabled? Is there any potential
>>> security issue?
>> In this case, we'll inject an NPI in the guest, but when it invokes
>> FFA_NOTIFICATION_GET it will get accurate information from the SPMC.
>> The worst case is a spurious NPI. This shouldn't be a security issue.
>
> ... we inject the interrupt to the "wrong" domain. But I also understand that it would be difficult for Xen to detect it.
>
> So I would say no print should be needed. Bertrand, what do you think?
Yes i agree that it could confuse the user.
I would just put comments to explain the situations where it could occur in the code.
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
Hi,
On Wed, Jun 5, 2024 at 12:45 PM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi,
>
> > On 4 Jun 2024, at 12:24, Julien Grall <julien@xen.org> wrote:
> >
> >
> >
> > On 03/06/2024 10:50, Jens Wiklander wrote:
> >> Hi Julien,
> >
> > Hi Jens,
> >
> >
> >> On Mon, Jun 3, 2024 at 11:12 AM Julien Grall <julien@xen.org> wrote:
> >>>
> >>> Hi Jens,
> >>>
> >>> On 03/06/2024 10:01, Jens Wiklander wrote:
> >>>> On Fri, May 31, 2024 at 4:28 PM Bertrand Marquis
> >>>> <Bertrand.Marquis@arm.com> wrote:
> >>>>>
> >>>>> Hi Jens,
> >>>>>
> >>>>>> On 29 May 2024, at 09:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >>>>>>
> >>>>>> Add support for FF-A notifications, currently limited to an SP (Secure
> >>>>>> Partition) sending an asynchronous notification to a guest.
> >>>>>>
> >>>>>> Guests and Xen itself are made aware of pending notifications with an
> >>>>>> interrupt. The interrupt handler triggers a tasklet to retrieve the
> >>>>>> notifications using the FF-A ABI and deliver them to their destinations.
> >>>>>>
> >>>>>> Update ffa_partinfo_domain_init() to return error code like
> >>>>>> ffa_notif_domain_init().
> >>>>>>
> >>>>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> >>>>>> ---
> >>>>>> v4->v5:
> >>>>>> - Move the freeing of d->arch.tee to the new TEE mediator free_domain_ctx
> >>>>>> callback to make the context accessible during rcu_lock_domain_by_id()
> >>>>>> from a tasklet
> >>>>>> - Initialize interrupt handlers for secondary CPUs from the new TEE mediator
> >>>>>> init_interrupt() callback
> >>>>>> - Restore the ffa_probe() from v3, that is, remove the
> >>>>>> presmp_initcall(ffa_init) approach and use ffa_probe() as usual now that we
> >>>>>> have the init_interrupt callback.
> >>>>>> - A tasklet is added to handle the Schedule Receiver interrupt. The tasklet
> >>>>>> finds each relevant domain with rcu_lock_domain_by_id() which now is enough
> >>>>>> to guarantee that the FF-A context can be accessed.
> >>>>>> - The notification interrupt handler only schedules the notification
> >>>>>> tasklet mentioned above
> >>>>>>
> >>>>>> v3->v4:
> >>>>>> - Add another note on FF-A limitations
> >>>>>> - Clear secure_pending in ffa_handle_notification_get() if both SP and SPM
> >>>>>> bitmaps are retrieved
> >>>>>> - ASSERT that ffa_rcu_lock_domain_by_vm_id() isn't passed the vm_id FF-A
> >>>>>> uses for Xen itself
> >>>>>> - Replace the get_domain_by_id() call done via ffa_get_domain_by_vm_id() in
> >>>>>> notif_irq_handler() with a call to rcu_lock_live_remote_domain_by_id() via
> >>>>>> ffa_rcu_lock_domain_by_vm_id()
> >>>>>> - Remove spinlock in struct ffa_ctx_notif and use atomic functions as needed
> >>>>>> to access and update the secure_pending field
> >>>>>> - In notif_irq_handler(), look for the first online CPU instead of assuming
> >>>>>> that the first CPU is online
> >>>>>> - Initialize FF-A via presmp_initcall() before the other CPUs are online,
> >>>>>> use register_cpu_notifier() to install the interrupt handler
> >>>>>> notif_irq_handler()
> >>>>>> - Update commit message to reflect recent updates
> >>>>>>
> >>>>>> v2->v3:
> >>>>>> - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
> >>>>>> FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
> >>>>>> - Register the Xen SRI handler on each CPU using on_selected_cpus() and
> >>>>>> setup_irq()
> >>>>>> - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
> >>>>>> doesn't conflict with static SGI handlers
> >>>>>>
> >>>>>> v1->v2:
> >>>>>> - Addressing review comments
> >>>>>> - Change ffa_handle_notification_{bind,unbind,set}() to take struct
> >>>>>> cpu_user_regs *regs as argument.
> >>>>>> - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
> >>>>>> an error code.
> >>>>>> - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
> >>>>>> ---
> >>>>>> xen/arch/arm/tee/Makefile | 1 +
> >>>>>> xen/arch/arm/tee/ffa.c | 72 +++++-
> >>>>>> xen/arch/arm/tee/ffa_notif.c | 409 ++++++++++++++++++++++++++++++++
> >>>>>> xen/arch/arm/tee/ffa_partinfo.c | 9 +-
> >>>>>> xen/arch/arm/tee/ffa_private.h | 56 ++++-
> >>>>>> xen/arch/arm/tee/tee.c | 2 +-
> >>>>>> xen/include/public/arch-arm.h | 14 ++
> >>>>>> 7 files changed, 548 insertions(+), 15 deletions(-)
> >>>>>> create mode 100644 xen/arch/arm/tee/ffa_notif.c
> >>>>>>
> >>>> [...]
> >>>>>>
> >>>>>> @@ -517,8 +567,10 @@ err_rxtx_destroy:
> >>>>>> static const struct tee_mediator_ops ffa_ops =
> >>>>>> {
> >>>>>> .probe = ffa_probe,
> >>>>>> + .init_interrupt = ffa_notif_init_interrupt,
> >>>>>
> >>>>> With the previous change on init secondary i would suggest to
> >>>>> have a ffa_init_secondary here actually calling the ffa_notif_init_interrupt.
> >>>>
> >>>> Yes, that makes sense. I'll update.
> >>>>
> >>>>>
> >>>>>> .domain_init = ffa_domain_init,
> >>>>>> .domain_teardown = ffa_domain_teardown,
> >>>>>> + .free_domain_ctx = ffa_free_domain_ctx,
> >>>>>> .relinquish_resources = ffa_relinquish_resources,
> >>>>>> .handle_call = ffa_handle_call,
> >>>>>> };
> >>>>>> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..e8e8b62590b3
> >>>>>> --- /dev/null
> >>>>>> +++ b/xen/arch/arm/tee/ffa_notif.c
> >>>> [...]
> >>>>>> +static void notif_vm_pend_intr(uint16_t vm_id)
> >>>>>> +{
> >>>>>> + struct ffa_ctx *ctx;
> >>>>>> + struct domain *d;
> >>>>>> + struct vcpu *v;
> >>>>>> +
> >>>>>> + /*
> >>>>>> + * vm_id == 0 means a notifications pending for Xen itself, but
> >>>>>> + * we don't support that yet.
> >>>>>> + */
> >>>>>> + if ( !vm_id )
> >>>>>> + return;
> >>>>>> +
> >>>>>> + d = ffa_rcu_lock_domain_by_vm_id(vm_id);
> >>>>>> + if ( !d )
> >>>>>> + return;
> >>>>>> +
> >>>>>> + ctx = d->arch.tee;
> >>>>>> + if ( !ctx )
> >>>>>> + goto out_unlock;
> >>>>>
> >>>>> In both previous cases you are silently ignoring an interrupt
> >>>>> due to an internal error.
> >>>>> Is this something that we should trace ? maybe just debug ?
> >>>>>
> >>>>> Could you add a comment to explain why this could happen
> >>>>> (when possible) or not and the possible side effects ?
> >>>>>
> >>>>> The second one would be a notification for a domain without
> >>>>> FF-A enabled which is ok but i am a bit more wondering on
> >>>>> the first one.
> >>>>
> >>>> The SPMC must be out of sync in both cases. I've been looking for a
> >>>> window where that can happen, but I can't find any. SPMC is called
> >>>> with FFA_NOTIFICATION_BITMAP_DESTROY during domain teardown so the
> >>>> SPMC shouldn't try to deliver any notifications after that.
> >>>
> >>> I don't think I agree with the conclusion. I believe, this can also
> >>> happen in normal operation.
> >>>
> >>> For example, the SPMC could have trigger the interrupt before
> >>> FFA_NOTIFICATION_BITMAP_DESTROY but Xen didn't handle the interrupt (or
> >>> run the tasklet) until later.
> >> You're right, there is a window. Delayed handling is OK since
> >> FFA_NOTIFICATION_INFO_GET_64 is invoked from the tasklet, but there is
> >> a window if the tasklet is suspended or another core destroys the
> >> domain before the tasklet has called ffa_rcu_lock_domain_by_vm_id().
> >> So far it's harmless and I guess we can afford a print.
> >
> > I think it would confuse more the user than anything else because this is an expected race. If we wanted to print a message, then I would argue it should be in the case where...
> >
> >>>
> >>> This could be at the time where the domain has been fully destroyed or
> >>> even when...
> >>>
> >>>> In the second case, the domain ID might have been reused for a domain
> >>>> without FF-A enabled, but the SPMC should have known that already.
> >>>
> >>> ... a new domain has been created. Although, the latter is rather unlikely.
> >>>
> >>> So what if the new domain has FFA enabled? Is there any potential
> >>> security issue?
> >> In this case, we'll inject an NPI in the guest, but when it invokes
> >> FFA_NOTIFICATION_GET it will get accurate information from the SPMC.
> >> The worst case is a spurious NPI. This shouldn't be a security issue.
> >
> > ... we inject the interrupt to the "wrong" domain. But I also understand that it would be difficult for Xen to detect it.
> >
> > So I would say no print should be needed. Bertrand, what do you think?
>
> Yes i agree that it could confuse the user.
> I would just put comments to explain the situations where it could occur in the code.
Thanks, I'll add comments for the next version.
Cheers,
Jens
© 2016 - 2025 Red Hat, Inc.