[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
Hi Jens,
On 26/04/2024 09:47, Jens Wiklander wrote:
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index d7306aa64d50..5224898265a5 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -155,7 +155,7 @@ void __init init_IRQ(void)
{
/* SGIs are always edge-triggered */
if ( irq < NR_GIC_SGI )
- local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
+ local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
This changes seems unrelated to this patch. This wants to be separate
(if you actually intended to change it).
else
local_irqs_type[irq] = IRQ_TYPE_INVALID;
}
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..912e905a27bd 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -39,6 +39,9 @@
* - 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
*
* 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 +197,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 +245,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 +334,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 +367,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 +391,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)
@@ -423,6 +470,7 @@ 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 */);
@@ -502,6 +550,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);
diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
new file mode 100644
index 000000000000..6bb0804ee2f8
--- /dev/null
+++ b/xen/arch/arm/tee/ffa_notif.c
@@ -0,0 +1,378 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024 Linaro Limited
+ */
+
+#include <xen/const.h>
+#include <xen/list.h>
+#include <xen/spinlock.h>
+#include <xen/types.h>
+
+#include <asm/smccc.h>
+#include <asm/regs.h>
+
+#include "ffa_private.h"
+
+static bool __ro_after_init notif_enabled;
+
+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;
+ bool pending_global;
+
+ if ( !notif_enabled )
+ {
+ ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+ return;
+ }
+
+ spin_lock(&ctx->notif.lock);
+ pending_global = ctx->notif.secure_pending;
+ ctx->notif.secure_pending = false;
+ spin_unlock(&ctx->notif.lock);
+
+ if ( pending_global )
+ {
+ /* 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);
+ }
+}
+static void notif_irq_handler(int irq, void *data)
+{
+ 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;
+ struct domain *d;
+
+ d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));
Thinking a bit more about the question from Bertrand about
get_domain_id() vs rcu_lock_domain_by_id(). I am actually not sure
whether either are ok here.
If I am not mistaken, d->arch.tee will be freed as part of the domain
teardown process. This means you can have the following scenario:
CPU0: ffa_get_domain_by_vm_id() (return the domain as it is alive)
CPU1: call domain_kill()
CPU1: teardown is called, free d->arch.tee (the pointer is not set to NULL)
d->arch.tee is now a dangling pointer
CPU0: access d->arch.tee
This implies you may need to gain a global lock (I don't have a better
idea so far) to protect the IRQ handler against domains teardown.
Did I miss anything?
+
+ if ( d )
+ {
+ struct ffa_ctx *ctx = d->arch.tee;
+
+ spin_lock(&ctx->notif.lock);
+ ctx->notif.secure_pending = true;
+ spin_unlock(&ctx->notif.lock);
AFAICT, the spinlock is used with IRQ enabled (see
ffa_handle_notification_info_get()) but also in an IRQ handler. So to
prevent deadlock, you will want to use spin_lock_irq* helpers.
That said, I don't think you need a spin_lock(). You could use atomic
operations instead. For the first hunk, you could use
test_and_clear_bool(). E.g.:
if ( test_and_clear_bool(ctx.notif.secure_pending) )
For the second part, it might be fine to use ACCESS_ONCE().
+
+ /*
+ * Since we're only delivering global notification, always
+ * deliver to the first vCPU. It doesn't matter which we
+ * chose, as long as it's available.
What if vCPU0 is offlined?
+ */
+ vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID,
+ true);
+
+ put_domain(d);
+ }
+
+ id_pos += count;
+ }
+
+ } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
+}
[..]
+struct ffa_ctx_notif {
+ bool enabled;
+
+ /* Used to serialize access to the rest of this struct */
+ spinlock_t lock;
Regardless what I wrote above, I can't seem to find a call to
spin_lock_init(). You will want it to initialize.
Also, it would be best if we keep the two booleans close to each other
so we limit the amount of padding in the structure.
+
+ /*
+ * 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 +265,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 +295,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 +309,28 @@ 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);
+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_get_domain_by_vm_id(uint16_t vm_id)
+{
Is this expected to be called with vm_id == 0? If not, then I would
consider to add an ASSERT(vm_id != 0). If yes, then I please add a
runtime check and return NULL.
+ /* -1 to match ffa_get_vm_id() */
+ return get_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,
Cheers,
--
Julien Grall
|
