* [XEN PATCH v1 0/5] FF-A notifications
@ 2024-04-09 15:36 Jens Wiklander
2024-04-09 15:36 ` [XEN PATCH v1 1/5] xen/arm: ffa: refactor ffa_handle_call() Jens Wiklander
` (4 more replies)
0 siblings, 5 replies; 21+ messages in thread
From: Jens Wiklander @ 2024-04-09 15:36 UTC (permalink / raw)
To: xen-devel
Cc: patches, Jens Wiklander, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel
Hi,
This patch set adds support for FF-A notifications. We only support
global notifications, per vCPU notifications remain unsupported.
The first three patches are further cleanup and can be merged before the
rest if desired.
A physical SGI is used to make Xen aware of pending FF-A notifications. The
physical SGI is selected by the SPMC in the secure world. Since it must not
already be used by Xen the SPMC is in practice forced to donate one of the
secure SGIs, but that's normally not a problem. The SGI handling in Xen is
updated to support registration of handlers for SGIs that aren't statically
assigned, that is, SGI IDs above GIC_SGI_MAX.
Thanks,
Jens
Jens Wiklander (5):
xen/arm: ffa: refactor ffa_handle_call()
xen/arm: ffa: use ACCESS_ONCE()
xen/arm: ffa: simplify ffa_handle_mem_share()
xen/arm: allow dynamically assigned SGI handlers
xen/arm: ffa: support notification
xen/arch/arm/gic.c | 5 +-
xen/arch/arm/irq.c | 7 +-
xen/arch/arm/tee/Makefile | 1 +
xen/arch/arm/tee/ffa.c | 86 +++++++--
xen/arch/arm/tee/ffa_notif.c | 319 +++++++++++++++++++++++++++++++++
xen/arch/arm/tee/ffa_private.h | 71 ++++++++
xen/arch/arm/tee/ffa_shm.c | 33 ++--
7 files changed, 480 insertions(+), 42 deletions(-)
create mode 100644 xen/arch/arm/tee/ffa_notif.c
--
2.34.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [XEN PATCH v1 1/5] xen/arm: ffa: refactor ffa_handle_call()
2024-04-09 15:36 [XEN PATCH v1 0/5] FF-A notifications Jens Wiklander
@ 2024-04-09 15:36 ` Jens Wiklander
2024-04-10 7:04 ` Bertrand Marquis
2024-04-09 15:36 ` [XEN PATCH v1 2/5] xen/arm: ffa: use ACCESS_ONCE() Jens Wiklander
` (3 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Jens Wiklander @ 2024-04-09 15:36 UTC (permalink / raw)
To: xen-devel
Cc: patches, Jens Wiklander, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel
Refactors the large switch block in ffa_handle_call() to use common code
for the simple case where it's either an error code or success with no
further parameters.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
xen/arch/arm/tee/ffa.c | 30 ++++++++++--------------------
1 file changed, 10 insertions(+), 20 deletions(-)
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 8665201e34a9..5209612963e1 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -273,18 +273,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
case FFA_RXTX_MAP_64:
e = ffa_handle_rxtx_map(fid, get_user_reg(regs, 1),
get_user_reg(regs, 2), get_user_reg(regs, 3));
- if ( e )
- ffa_set_regs_error(regs, e);
- else
- ffa_set_regs_success(regs, 0, 0);
- return true;
+ break;
case FFA_RXTX_UNMAP:
e = ffa_handle_rxtx_unmap();
- if ( e )
- ffa_set_regs_error(regs, e);
- else
- ffa_set_regs_success(regs, 0, 0);
- return true;
+ break;
case FFA_PARTITION_INFO_GET:
e = ffa_handle_partition_info_get(get_user_reg(regs, 1),
get_user_reg(regs, 2),
@@ -299,11 +291,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
return true;
case FFA_RX_RELEASE:
e = ffa_handle_rx_release();
- if ( e )
- ffa_set_regs_error(regs, e);
- else
- ffa_set_regs_success(regs, 0, 0);
- return true;
+ break;
case FFA_MSG_SEND_DIRECT_REQ_32:
case FFA_MSG_SEND_DIRECT_REQ_64:
handle_msg_send_direct_req(regs, fid);
@@ -316,17 +304,19 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
e = ffa_handle_mem_reclaim(regpair_to_uint64(get_user_reg(regs, 2),
get_user_reg(regs, 1)),
get_user_reg(regs, 3));
- if ( e )
- ffa_set_regs_error(regs, e);
- else
- ffa_set_regs_success(regs, 0, 0);
- return true;
+ break;
default:
gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
return true;
}
+
+ if ( e )
+ ffa_set_regs_error(regs, e);
+ else
+ ffa_set_regs_success(regs, 0, 0);
+ return true;
}
static int ffa_domain_init(struct domain *d)
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [XEN PATCH v1 2/5] xen/arm: ffa: use ACCESS_ONCE()
2024-04-09 15:36 [XEN PATCH v1 0/5] FF-A notifications Jens Wiklander
2024-04-09 15:36 ` [XEN PATCH v1 1/5] xen/arm: ffa: refactor ffa_handle_call() Jens Wiklander
@ 2024-04-09 15:36 ` Jens Wiklander
2024-04-10 7:05 ` Bertrand Marquis
2024-04-09 15:36 ` [XEN PATCH v1 3/5] xen/arm: ffa: simplify ffa_handle_mem_share() Jens Wiklander
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Jens Wiklander @ 2024-04-09 15:36 UTC (permalink / raw)
To: xen-devel
Cc: patches, Jens Wiklander, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel
Replace read_atomic() with ACCESS_ONCE() to match the intended use, that
is, to prevent the compiler from (via optimization) reading shared
memory more than once.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
xen/arch/arm/tee/ffa_shm.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/tee/ffa_shm.c b/xen/arch/arm/tee/ffa_shm.c
index eed9ad2d2986..75a5b66aeb4c 100644
--- a/xen/arch/arm/tee/ffa_shm.c
+++ b/xen/arch/arm/tee/ffa_shm.c
@@ -7,6 +7,7 @@
#include <xen/sizes.h>
#include <xen/types.h>
#include <xen/mm.h>
+#include <xen/lib.h>
#include <xen/list.h>
#include <xen/spinlock.h>
@@ -171,8 +172,8 @@ static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
for ( n = 0; n < range_count; n++ )
{
- page_count = read_atomic(&range[n].page_count);
- addr = read_atomic(&range[n].address);
+ page_count = ACCESS_ONCE(range[n].page_count);
+ addr = ACCESS_ONCE(range[n].address);
for ( m = 0; m < page_count; m++ )
{
if ( pg_idx >= shm->page_count )
@@ -527,13 +528,13 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
goto out_unlock;
mem_access = ctx->tx + trans.mem_access_offs;
- if ( read_atomic(&mem_access->access_perm.perm) != FFA_MEM_ACC_RW )
+ if ( ACCESS_ONCE(mem_access->access_perm.perm) != FFA_MEM_ACC_RW )
{
ret = FFA_RET_NOT_SUPPORTED;
goto out_unlock;
}
- region_offs = read_atomic(&mem_access->region_offs);
+ region_offs = ACCESS_ONCE(mem_access->region_offs);
if ( sizeof(*region_descr) + region_offs > frag_len )
{
ret = FFA_RET_NOT_SUPPORTED;
@@ -541,8 +542,8 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
}
region_descr = ctx->tx + region_offs;
- range_count = read_atomic(®ion_descr->address_range_count);
- page_count = read_atomic(®ion_descr->total_page_count);
+ range_count = ACCESS_ONCE(region_descr->address_range_count);
+ page_count = ACCESS_ONCE(region_descr->total_page_count);
if ( !page_count )
{
@@ -557,7 +558,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
goto out_unlock;
}
shm->sender_id = trans.sender_id;
- shm->ep_id = read_atomic(&mem_access->access_perm.endpoint_id);
+ shm->ep_id = ACCESS_ONCE(mem_access->access_perm.endpoint_id);
/*
* Check that the Composite memory region descriptor fits.
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [XEN PATCH v1 3/5] xen/arm: ffa: simplify ffa_handle_mem_share()
2024-04-09 15:36 [XEN PATCH v1 0/5] FF-A notifications Jens Wiklander
2024-04-09 15:36 ` [XEN PATCH v1 1/5] xen/arm: ffa: refactor ffa_handle_call() Jens Wiklander
2024-04-09 15:36 ` [XEN PATCH v1 2/5] xen/arm: ffa: use ACCESS_ONCE() Jens Wiklander
@ 2024-04-09 15:36 ` Jens Wiklander
2024-04-10 7:07 ` Bertrand Marquis
2024-04-09 15:36 ` [XEN PATCH v1 4/5] xen/arm: allow dynamically assigned SGI handlers Jens Wiklander
2024-04-09 15:36 ` [XEN PATCH v1 5/5] xen/arm: ffa: support notification Jens Wiklander
4 siblings, 1 reply; 21+ messages in thread
From: Jens Wiklander @ 2024-04-09 15:36 UTC (permalink / raw)
To: xen-devel
Cc: patches, Jens Wiklander, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel
Simplify ffa_handle_mem_share() by removing the start_page_idx and
last_page_idx parameters from get_shm_pages() and check that the number
of pages matches expectations at the end of get_shm_pages().
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
xen/arch/arm/tee/ffa_shm.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/xen/arch/arm/tee/ffa_shm.c b/xen/arch/arm/tee/ffa_shm.c
index 75a5b66aeb4c..370d83ec5cf8 100644
--- a/xen/arch/arm/tee/ffa_shm.c
+++ b/xen/arch/arm/tee/ffa_shm.c
@@ -159,10 +159,9 @@ static int32_t ffa_mem_reclaim(uint32_t handle_lo, uint32_t handle_hi,
*/
static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
const struct ffa_address_range *range,
- uint32_t range_count, unsigned int start_page_idx,
- unsigned int *last_page_idx)
+ uint32_t range_count)
{
- unsigned int pg_idx = start_page_idx;
+ unsigned int pg_idx = 0;
gfn_t gfn;
unsigned int n;
unsigned int m;
@@ -191,7 +190,9 @@ static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
}
}
- *last_page_idx = pg_idx;
+ /* The ranges must add up */
+ if ( pg_idx < shm->page_count )
+ return FFA_RET_INVALID_PARAMETERS;
return FFA_RET_OK;
}
@@ -460,7 +461,6 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
struct domain *d = current->domain;
struct ffa_ctx *ctx = d->arch.tee;
struct ffa_shm_mem *shm = NULL;
- unsigned int last_page_idx = 0;
register_t handle_hi = 0;
register_t handle_lo = 0;
int ret = FFA_RET_DENIED;
@@ -570,15 +570,9 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
goto out;
}
- ret = get_shm_pages(d, shm, region_descr->address_range_array, range_count,
- 0, &last_page_idx);
+ ret = get_shm_pages(d, shm, region_descr->address_range_array, range_count);
if ( ret )
goto out;
- if ( last_page_idx != shm->page_count )
- {
- ret = FFA_RET_INVALID_PARAMETERS;
- goto out;
- }
/* Note that share_shm() uses our tx buffer */
spin_lock(&ffa_tx_buffer_lock);
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [XEN PATCH v1 4/5] xen/arm: allow dynamically assigned SGI handlers
2024-04-09 15:36 [XEN PATCH v1 0/5] FF-A notifications Jens Wiklander
` (2 preceding siblings ...)
2024-04-09 15:36 ` [XEN PATCH v1 3/5] xen/arm: ffa: simplify ffa_handle_mem_share() Jens Wiklander
@ 2024-04-09 15:36 ` Jens Wiklander
2024-04-10 13:24 ` Michal Orzel
2024-04-09 15:36 ` [XEN PATCH v1 5/5] xen/arm: ffa: support notification Jens Wiklander
4 siblings, 1 reply; 21+ messages in thread
From: Jens Wiklander @ 2024-04-09 15:36 UTC (permalink / raw)
To: xen-devel
Cc: patches, Jens Wiklander, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk
Updates so request_irq() can be used with a dynamically assigned SGI irq
as input.
gic_route_irq_to_xen() don't gic_set_irq_type() for SGIs since they have
their type assigned earlier during boot
gic_interrupt() is updated to route the dynamically assigned SGIs to
do_IRQ() instead of do_sgi(). The latter still handles the statically
assigned SGI handlers like for instance GIC_SGI_CALL_FUNCTION.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
xen/arch/arm/gic.c | 5 +++--
xen/arch/arm/irq.c | 7 +++++--
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 44c40e86defe..e9aeb7138455 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -117,7 +117,8 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
desc->handler = gic_hw_ops->gic_host_irq_type;
- gic_set_irq_type(desc, desc->arch.type);
+ if ( desc->irq >= NR_GIC_SGI)
+ gic_set_irq_type(desc, desc->arch.type);
gic_set_irq_priority(desc, priority);
}
@@ -375,7 +376,7 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
/* Reading IRQ will ACK it */
irq = gic_hw_ops->read_irq();
- if ( likely(irq >= 16 && irq < 1020) )
+ if ( likely(irq >= GIC_SGI_MAX && irq < 1020) )
{
isb();
do_IRQ(regs, irq, is_fiq);
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index bcce80a4d624..fdb214560978 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -224,9 +224,12 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
perfc_incr(irqs);
- ASSERT(irq >= 16); /* SGIs do not come down this path */
+ /* Statically assigned SGIs do not come down this path */
+ ASSERT(irq >= GIC_SGI_MAX);
- if ( irq < 32 )
+ if ( irq < NR_GIC_SGI )
+ perfc_incr(ipis);
+ else if ( irq < NR_GIC_LOCAL_IRQS )
perfc_incr(ppis);
else
perfc_incr(spis);
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [XEN PATCH v1 5/5] xen/arm: ffa: support notification
2024-04-09 15:36 [XEN PATCH v1 0/5] FF-A notifications Jens Wiklander
` (3 preceding siblings ...)
2024-04-09 15:36 ` [XEN PATCH v1 4/5] xen/arm: allow dynamically assigned SGI handlers Jens Wiklander
@ 2024-04-09 15:36 ` Jens Wiklander
2024-04-10 7:49 ` Bertrand Marquis
2024-04-10 15:45 ` Jens Wiklander
4 siblings, 2 replies; 21+ messages in thread
From: Jens Wiklander @ 2024-04-09 15:36 UTC (permalink / raw)
To: xen-devel
Cc: patches, Jens Wiklander, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel
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 retrieves the notifications using the
FF-A ABI and deliver them to their destinations.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
xen/arch/arm/tee/Makefile | 1 +
xen/arch/arm/tee/ffa.c | 58 ++++++
xen/arch/arm/tee/ffa_notif.c | 319 +++++++++++++++++++++++++++++++++
xen/arch/arm/tee/ffa_private.h | 71 ++++++++
4 files changed, 449 insertions(+)
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..ce9757bfeed1 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, 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, FFA_NOTIF_PEND_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,30 @@ 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(get_user_reg(regs, 1),
+ get_user_reg(regs, 2),
+ get_user_reg(regs, 3),
+ get_user_reg(regs, 4));
+ break;
+ case FFA_NOTIFICATION_UNBIND:
+ e = ffa_handle_notification_unbind(get_user_reg(regs, 1),
+ get_user_reg(regs, 3),
+ get_user_reg(regs, 4));
+ 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(get_user_reg(regs, 1),
+ get_user_reg(regs, 2),
+ get_user_reg(regs, 3),
+ get_user_reg(regs, 4));
+ break;
default:
gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
@@ -348,6 +401,9 @@ static int ffa_domain_init(struct domain *d)
if ( !ffa_partinfo_domain_init(d) )
return -EIO;
+ if ( !ffa_notif_domain_init(d) )
+ return -ENOMEM;
+
return 0;
}
@@ -423,6 +479,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 +559,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..0173ee515362
--- /dev/null
+++ b/xen/arch/arm/tee/ffa_notif.c
@@ -0,0 +1,319 @@
+/* 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(uint32_t src_dst, uint32_t flags,
+ uint32_t bitmap_lo, uint32_t bitmap_hi)
+{
+ struct domain *d = current->domain;
+
+ if ( !notif_enabled )
+ return FFA_RET_NOT_SUPPORTED;
+
+ if ( (src_dst & 0xffff) != 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(uint32_t src_dst, uint32_t bitmap_lo,
+ uint32_t bitmap_hi)
+{
+ struct domain *d = current->domain;
+
+ if ( !notif_enabled )
+ return FFA_RET_NOT_SUPPORTED;
+
+ if ( (src_dst & 0xffff) != 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);
+ }
+}
+
+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 & 0xffff) != 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;
+
+ 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(uint32_t src_dst, uint32_t flags,
+ uint32_t bitmap_lo, uint32_t bitmap_hi)
+{
+ struct domain *d = current->domain;
+
+ if ( !notif_enabled )
+ return FFA_RET_NOT_SUPPORTED;
+
+ if ( (src_dst >> 16) != ffa_get_vm_id(d) )
+ return FFA_RET_INVALID_PARAMETERS;
+
+ /*
+ * 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_SET, src_dst, flags, bitmap_lo,
+ bitmap_hi);
+}
+
+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_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));
+
+ if ( d )
+ {
+ struct ffa_ctx *ctx = d->arch.tee;
+
+ spin_lock(&ctx->notif.lock);
+ ctx->notif.secure_pending = true;
+ spin_unlock(&ctx->notif.lock);
+
+ /*
+ * 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.
+ */
+ vgic_inject_irq(d, d->vcpu[0], FFA_NOTIF_PEND_INTR_ID, true);
+
+ put_domain(d);
+ }
+
+ id_pos += count;
+ }
+
+ } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
+}
+
+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(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;
+ 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);
+ notif_enabled = !ret;
+}
+
+bool ffa_notif_domain_init(struct domain *d)
+{
+ struct ffa_ctx *ctx = d->arch.tee;
+ int32_t res;
+
+ if ( !notif_enabled )
+ return true;
+
+ res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
+ if ( res )
+ return false;
+
+ ctx->notif.enabled = true;
+
+ return true;
+}
+
+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_private.h b/xen/arch/arm/tee/ffa_private.h
index 98236cbf14a3..26c2af164d38 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
@@ -60,6 +61,8 @@
*/
#define FFA_PAGE_SIZE SZ_4K
+#define FFA_NOTIF_BITMAP_SIZE 64
+
/*
* The number of pages used for each of the RX and TX buffers shared with
* the SPMC.
@@ -97,6 +100,18 @@
*/
#define FFA_MAX_SHM_COUNT 32
+/*
+ * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely
+ * unused, but that may change.
+ *
+ * SGI is the preferred delivery mechanism. 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.
+ */
+#define FFA_NOTIF_PEND_INTR_ID 8
+#define FFA_SCHEDULE_RECV_INTR_ID 9
+
/*
* The time we wait until trying to tear down a domain again if it was
* blocked initially.
@@ -175,6 +190,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 +243,27 @@
#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;
+
+ /* Used to serialize access to the rest of this struct */
+ spinlock_t lock;
+
+ /*
+ * 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 +279,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
@@ -271,12 +323,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);
+bool ffa_notif_domain_init(struct domain *d);
+void ffa_notif_domain_destroy(struct domain *d);
+
+int ffa_handle_notification_bind(uint32_t src_dst, uint32_t flags,
+ uint32_t bitmap_lo, uint32_t bitmap_hi);
+int ffa_handle_notification_unbind(uint32_t src_dst, uint32_t bitmap_lo,
+ uint32_t bitmap_hi);
+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(uint32_t src_dst, uint32_t flags,
+ uint32_t bitmap_lo, uint32_t bitmap_hi);
+
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)
+{
+ /* -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,
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 1/5] xen/arm: ffa: refactor ffa_handle_call()
2024-04-09 15:36 ` [XEN PATCH v1 1/5] xen/arm: ffa: refactor ffa_handle_call() Jens Wiklander
@ 2024-04-10 7:04 ` Bertrand Marquis
0 siblings, 0 replies; 21+ messages in thread
From: Bertrand Marquis @ 2024-04-10 7:04 UTC (permalink / raw)
To: Jens Wiklander
Cc: Xen-devel, patches, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Michal Orzel
Hi Jens,
> On 9 Apr 2024, at 17:36, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Refactors the large switch block in ffa_handle_call() to use common code
> for the simple case where it's either an error code or success with no
> further parameters.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
> xen/arch/arm/tee/ffa.c | 30 ++++++++++--------------------
> 1 file changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 8665201e34a9..5209612963e1 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -273,18 +273,10 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> case FFA_RXTX_MAP_64:
> e = ffa_handle_rxtx_map(fid, get_user_reg(regs, 1),
> get_user_reg(regs, 2), get_user_reg(regs, 3));
> - if ( e )
> - ffa_set_regs_error(regs, e);
> - else
> - ffa_set_regs_success(regs, 0, 0);
> - return true;
> + break;
> case FFA_RXTX_UNMAP:
> e = ffa_handle_rxtx_unmap();
> - if ( e )
> - ffa_set_regs_error(regs, e);
> - else
> - ffa_set_regs_success(regs, 0, 0);
> - return true;
> + break;
> case FFA_PARTITION_INFO_GET:
> e = ffa_handle_partition_info_get(get_user_reg(regs, 1),
> get_user_reg(regs, 2),
> @@ -299,11 +291,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> return true;
> case FFA_RX_RELEASE:
> e = ffa_handle_rx_release();
> - if ( e )
> - ffa_set_regs_error(regs, e);
> - else
> - ffa_set_regs_success(regs, 0, 0);
> - return true;
> + break;
> case FFA_MSG_SEND_DIRECT_REQ_32:
> case FFA_MSG_SEND_DIRECT_REQ_64:
> handle_msg_send_direct_req(regs, fid);
> @@ -316,17 +304,19 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> e = ffa_handle_mem_reclaim(regpair_to_uint64(get_user_reg(regs, 2),
> get_user_reg(regs, 1)),
> get_user_reg(regs, 3));
> - if ( e )
> - ffa_set_regs_error(regs, e);
> - else
> - ffa_set_regs_success(regs, 0, 0);
> - return true;
> + break;
>
> default:
> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> return true;
> }
> +
> + if ( e )
> + ffa_set_regs_error(regs, e);
> + else
> + ffa_set_regs_success(regs, 0, 0);
> + return true;
> }
>
> static int ffa_domain_init(struct domain *d)
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 2/5] xen/arm: ffa: use ACCESS_ONCE()
2024-04-09 15:36 ` [XEN PATCH v1 2/5] xen/arm: ffa: use ACCESS_ONCE() Jens Wiklander
@ 2024-04-10 7:05 ` Bertrand Marquis
0 siblings, 0 replies; 21+ messages in thread
From: Bertrand Marquis @ 2024-04-10 7:05 UTC (permalink / raw)
To: Jens Wiklander
Cc: Xen-devel, patches, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Michal Orzel
Hi Jens,
> On 9 Apr 2024, at 17:36, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Replace read_atomic() with ACCESS_ONCE() to match the intended use, that
> is, to prevent the compiler from (via optimization) reading shared
> memory more than once.
This definitely makes sense.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
> xen/arch/arm/tee/ffa_shm.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/tee/ffa_shm.c b/xen/arch/arm/tee/ffa_shm.c
> index eed9ad2d2986..75a5b66aeb4c 100644
> --- a/xen/arch/arm/tee/ffa_shm.c
> +++ b/xen/arch/arm/tee/ffa_shm.c
> @@ -7,6 +7,7 @@
> #include <xen/sizes.h>
> #include <xen/types.h>
> #include <xen/mm.h>
> +#include <xen/lib.h>
> #include <xen/list.h>
> #include <xen/spinlock.h>
>
> @@ -171,8 +172,8 @@ static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
>
> for ( n = 0; n < range_count; n++ )
> {
> - page_count = read_atomic(&range[n].page_count);
> - addr = read_atomic(&range[n].address);
> + page_count = ACCESS_ONCE(range[n].page_count);
> + addr = ACCESS_ONCE(range[n].address);
> for ( m = 0; m < page_count; m++ )
> {
> if ( pg_idx >= shm->page_count )
> @@ -527,13 +528,13 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
> goto out_unlock;
>
> mem_access = ctx->tx + trans.mem_access_offs;
> - if ( read_atomic(&mem_access->access_perm.perm) != FFA_MEM_ACC_RW )
> + if ( ACCESS_ONCE(mem_access->access_perm.perm) != FFA_MEM_ACC_RW )
> {
> ret = FFA_RET_NOT_SUPPORTED;
> goto out_unlock;
> }
>
> - region_offs = read_atomic(&mem_access->region_offs);
> + region_offs = ACCESS_ONCE(mem_access->region_offs);
> if ( sizeof(*region_descr) + region_offs > frag_len )
> {
> ret = FFA_RET_NOT_SUPPORTED;
> @@ -541,8 +542,8 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
> }
>
> region_descr = ctx->tx + region_offs;
> - range_count = read_atomic(®ion_descr->address_range_count);
> - page_count = read_atomic(®ion_descr->total_page_count);
> + range_count = ACCESS_ONCE(region_descr->address_range_count);
> + page_count = ACCESS_ONCE(region_descr->total_page_count);
>
> if ( !page_count )
> {
> @@ -557,7 +558,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
> goto out_unlock;
> }
> shm->sender_id = trans.sender_id;
> - shm->ep_id = read_atomic(&mem_access->access_perm.endpoint_id);
> + shm->ep_id = ACCESS_ONCE(mem_access->access_perm.endpoint_id);
>
> /*
> * Check that the Composite memory region descriptor fits.
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 3/5] xen/arm: ffa: simplify ffa_handle_mem_share()
2024-04-09 15:36 ` [XEN PATCH v1 3/5] xen/arm: ffa: simplify ffa_handle_mem_share() Jens Wiklander
@ 2024-04-10 7:07 ` Bertrand Marquis
0 siblings, 0 replies; 21+ messages in thread
From: Bertrand Marquis @ 2024-04-10 7:07 UTC (permalink / raw)
To: Jens Wiklander
Cc: Xen-devel, patches, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Michal Orzel
Hi Jens,
> On 9 Apr 2024, at 17:36, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Simplify ffa_handle_mem_share() by removing the start_page_idx and
> last_page_idx parameters from get_shm_pages() and check that the number
> of pages matches expectations at the end of get_shm_pages().
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
> xen/arch/arm/tee/ffa_shm.c | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/arm/tee/ffa_shm.c b/xen/arch/arm/tee/ffa_shm.c
> index 75a5b66aeb4c..370d83ec5cf8 100644
> --- a/xen/arch/arm/tee/ffa_shm.c
> +++ b/xen/arch/arm/tee/ffa_shm.c
> @@ -159,10 +159,9 @@ static int32_t ffa_mem_reclaim(uint32_t handle_lo, uint32_t handle_hi,
> */
> static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
> const struct ffa_address_range *range,
> - uint32_t range_count, unsigned int start_page_idx,
> - unsigned int *last_page_idx)
> + uint32_t range_count)
> {
> - unsigned int pg_idx = start_page_idx;
> + unsigned int pg_idx = 0;
> gfn_t gfn;
> unsigned int n;
> unsigned int m;
> @@ -191,7 +190,9 @@ static int get_shm_pages(struct domain *d, struct ffa_shm_mem *shm,
> }
> }
>
> - *last_page_idx = pg_idx;
> + /* The ranges must add up */
> + if ( pg_idx < shm->page_count )
> + return FFA_RET_INVALID_PARAMETERS;
>
> return FFA_RET_OK;
> }
> @@ -460,7 +461,6 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
> struct domain *d = current->domain;
> struct ffa_ctx *ctx = d->arch.tee;
> struct ffa_shm_mem *shm = NULL;
> - unsigned int last_page_idx = 0;
> register_t handle_hi = 0;
> register_t handle_lo = 0;
> int ret = FFA_RET_DENIED;
> @@ -570,15 +570,9 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs)
> goto out;
> }
>
> - ret = get_shm_pages(d, shm, region_descr->address_range_array, range_count,
> - 0, &last_page_idx);
> + ret = get_shm_pages(d, shm, region_descr->address_range_array, range_count);
> if ( ret )
> goto out;
> - if ( last_page_idx != shm->page_count )
> - {
> - ret = FFA_RET_INVALID_PARAMETERS;
> - goto out;
> - }
>
> /* Note that share_shm() uses our tx buffer */
> spin_lock(&ffa_tx_buffer_lock);
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 5/5] xen/arm: ffa: support notification
2024-04-09 15:36 ` [XEN PATCH v1 5/5] xen/arm: ffa: support notification Jens Wiklander
@ 2024-04-10 7:49 ` Bertrand Marquis
2024-04-10 14:27 ` Jens Wiklander
2024-04-10 15:45 ` Jens Wiklander
1 sibling, 1 reply; 21+ messages in thread
From: Bertrand Marquis @ 2024-04-10 7:49 UTC (permalink / raw)
To: Jens Wiklander
Cc: Xen-devel, patches, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Michal Orzel
Hi Jens,
> On 9 Apr 2024, at 17:36, 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 retrieves the notifications using the
> FF-A ABI and deliver them to their destinations.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
> xen/arch/arm/tee/Makefile | 1 +
> xen/arch/arm/tee/ffa.c | 58 ++++++
> xen/arch/arm/tee/ffa_notif.c | 319 +++++++++++++++++++++++++++++++++
> xen/arch/arm/tee/ffa_private.h | 71 ++++++++
> 4 files changed, 449 insertions(+)
> 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..ce9757bfeed1 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, 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, FFA_NOTIF_PEND_INTR_ID, 0);
This should return the RECV_INTR, not the PEND one.
> + 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,30 @@ 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(get_user_reg(regs, 1),
> + get_user_reg(regs, 2),
> + get_user_reg(regs, 3),
> + get_user_reg(regs, 4));
I would suggest to pass regs and handle the get_user_regs in the function.
> + break;
> + case FFA_NOTIFICATION_UNBIND:
> + e = ffa_handle_notification_unbind(get_user_reg(regs, 1),
> + get_user_reg(regs, 3),
> + get_user_reg(regs, 4));
same here
> + 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(get_user_reg(regs, 1),
> + get_user_reg(regs, 2),
> + get_user_reg(regs, 3),
> + get_user_reg(regs, 4));
same here
> + break;
>
> default:
> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> @@ -348,6 +401,9 @@ static int ffa_domain_init(struct domain *d)
> if ( !ffa_partinfo_domain_init(d) )
> return -EIO;
>
> + if ( !ffa_notif_domain_init(d) )
> + return -ENOMEM;
Having this function deciding on the return code is a bit weird.
I would suggest to have ffa_notif_domain_init returning an int
and deciding on the error code and this one just returning the
error if !=0.
If possible the same principle should be applied for the partinfo.
> +
> return 0;
> }
>
> @@ -423,6 +479,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 +559,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..0173ee515362
> --- /dev/null
> +++ b/xen/arch/arm/tee/ffa_notif.c
> @@ -0,0 +1,319 @@
> +/* 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(uint32_t src_dst, uint32_t flags,
> + uint32_t bitmap_lo, uint32_t bitmap_hi)
> +{
> + struct domain *d = current->domain;
> +
> + if ( !notif_enabled )
> + return FFA_RET_NOT_SUPPORTED;
> +
> + if ( (src_dst & 0xffff) != ffa_get_vm_id(d) )
> + return FFA_RET_INVALID_PARAMETERS;
s/0xffff/0xFFFFU/
> +
> + 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(uint32_t src_dst, uint32_t bitmap_lo,
> + uint32_t bitmap_hi)
> +{
> + struct domain *d = current->domain;
> +
> + if ( !notif_enabled )
> + return FFA_RET_NOT_SUPPORTED;
> +
> + if ( (src_dst & 0xffff) != ffa_get_vm_id(d) )
> + return FFA_RET_INVALID_PARAMETERS;
s/0xffff/0xFFFFU/
> +
> + /*
> + * 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);
> + }
> +}
> +
> +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 & 0xffff) != ffa_get_vm_id(d) )
s/0xffff/0xFFFFU/
> + {
> + 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;
> +
> + 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(uint32_t src_dst, uint32_t flags,
> + uint32_t bitmap_lo, uint32_t bitmap_hi)
> +{
> + struct domain *d = current->domain;
> +
> + if ( !notif_enabled )
> + return FFA_RET_NOT_SUPPORTED;
> +
> + if ( (src_dst >> 16) != ffa_get_vm_id(d) )
> + return FFA_RET_INVALID_PARAMETERS;
This needs some checking as i would have used the lowest bits here
for the source and not the highest. The spec is using the same description
for all ABIs so I am wondering if you are not using the destination instead of
the source here.
> +
> + /*
> + * 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_SET, src_dst, flags, bitmap_lo,
> + bitmap_hi);
> +}
> +
The following function would deserve some explanation in a comment
to clear up a bit what is done here and why.
> +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_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));
> +
> + if ( d )
> + {
> + struct ffa_ctx *ctx = d->arch.tee;
> +
> + spin_lock(&ctx->notif.lock);
> + ctx->notif.secure_pending = true;
> + spin_unlock(&ctx->notif.lock);
> +
> + /*
> + * 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.
> + */
> + vgic_inject_irq(d, d->vcpu[0], FFA_NOTIF_PEND_INTR_ID, true);
> +
> + put_domain(d);
> + }
> +
> + id_pos += count;
> + }
> +
> + } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
> +}
> +
> +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(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;
> + 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);
> + notif_enabled = !ret;
> +}
> +
> +bool ffa_notif_domain_init(struct domain *d)
> +{
> + struct ffa_ctx *ctx = d->arch.tee;
> + int32_t res;
> +
> + if ( !notif_enabled )
> + return true;
> +
> + res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
> + if ( res )
> + return false;
> +
> + ctx->notif.enabled = true;
> +
> + return true;
> +}
> +
> +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_private.h b/xen/arch/arm/tee/ffa_private.h
> index 98236cbf14a3..26c2af164d38 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
> @@ -60,6 +61,8 @@
> */
> #define FFA_PAGE_SIZE SZ_4K
>
> +#define FFA_NOTIF_BITMAP_SIZE 64
> +
This does not seem to be used.
> /*
> * The number of pages used for each of the RX and TX buffers shared with
> * the SPMC.
> @@ -97,6 +100,18 @@
> */
> #define FFA_MAX_SHM_COUNT 32
>
> +/*
> + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely
> + * unused, but that may change.
I am a bit wondering what your TODO means here.
Do you mean that we should have a way to "allocate a free SGI" ?
> + *
> + * SGI is the preferred delivery mechanism. 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.
> + */
> +#define FFA_NOTIF_PEND_INTR_ID 8
> +#define FFA_SCHEDULE_RECV_INTR_ID 9
> +
> /*
> * The time we wait until trying to tear down a domain again if it was
> * blocked initially.
> @@ -175,6 +190,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 +243,27 @@
> #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;
> +
> + /* Used to serialize access to the rest of this struct */
> + spinlock_t lock;
> +
> + /*
> + * 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 +279,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
> @@ -271,12 +323,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);
> +bool ffa_notif_domain_init(struct domain *d);
> +void ffa_notif_domain_destroy(struct domain *d);
> +
> +int ffa_handle_notification_bind(uint32_t src_dst, uint32_t flags,
> + uint32_t bitmap_lo, uint32_t bitmap_hi);
> +int ffa_handle_notification_unbind(uint32_t src_dst, uint32_t bitmap_lo,
> + uint32_t bitmap_hi);
> +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(uint32_t src_dst, uint32_t flags,
> + uint32_t bitmap_lo, uint32_t bitmap_hi);
> +
> 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)
> +{
> + /* -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,
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 4/5] xen/arm: allow dynamically assigned SGI handlers
2024-04-09 15:36 ` [XEN PATCH v1 4/5] xen/arm: allow dynamically assigned SGI handlers Jens Wiklander
@ 2024-04-10 13:24 ` Michal Orzel
2024-04-11 6:12 ` Jens Wiklander
0 siblings, 1 reply; 21+ messages in thread
From: Michal Orzel @ 2024-04-10 13:24 UTC (permalink / raw)
To: Jens Wiklander, xen-devel
Cc: patches, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hi Jens,
On 09/04/2024 17:36, Jens Wiklander wrote:
>
>
> Updates so request_irq() can be used with a dynamically assigned SGI irq
> as input.
At this point it would be handy to mention the use case for which you need to add this support
>
> gic_route_irq_to_xen() don't gic_set_irq_type() for SGIs since they have
> their type assigned earlier during boot
Could you elaborate more on that? Do you mean that SGIs are always edge triggered and there's no need
for setting the type?
>
> gic_interrupt() is updated to route the dynamically assigned SGIs to
> do_IRQ() instead of do_sgi(). The latter still handles the statically
> assigned SGI handlers like for instance GIC_SGI_CALL_FUNCTION.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Other than that, it LGTM:
Acked-by: Michal Orzel <michal.orzel@amd.com>
but I would like other maintainers (especially Julien) to cross check it.
~Michal
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 5/5] xen/arm: ffa: support notification
2024-04-10 7:49 ` Bertrand Marquis
@ 2024-04-10 14:27 ` Jens Wiklander
2024-04-10 14:47 ` Bertrand Marquis
2024-04-10 15:41 ` Bertrand Marquis
0 siblings, 2 replies; 21+ messages in thread
From: Jens Wiklander @ 2024-04-10 14:27 UTC (permalink / raw)
To: Bertrand Marquis
Cc: Xen-devel, patches, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Michal Orzel
On Wed, Apr 10, 2024 at 9:49 AM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Jens,
>
> > On 9 Apr 2024, at 17:36, 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 retrieves the notifications using the
> > FF-A ABI and deliver them to their destinations.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> > xen/arch/arm/tee/Makefile | 1 +
> > xen/arch/arm/tee/ffa.c | 58 ++++++
> > xen/arch/arm/tee/ffa_notif.c | 319 +++++++++++++++++++++++++++++++++
> > xen/arch/arm/tee/ffa_private.h | 71 ++++++++
> > 4 files changed, 449 insertions(+)
> > 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..ce9757bfeed1 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, 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, FFA_NOTIF_PEND_INTR_ID, 0);
>
> This should return the RECV_INTR, not the PEND one.
Thanks, I'll fix it.
>
> > + 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,30 @@ 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(get_user_reg(regs, 1),
> > + get_user_reg(regs, 2),
> > + get_user_reg(regs, 3),
> > + get_user_reg(regs, 4));
>
> I would suggest to pass regs and handle the get_user_regs in the function.
OK
>
> > + break;
> > + case FFA_NOTIFICATION_UNBIND:
> > + e = ffa_handle_notification_unbind(get_user_reg(regs, 1),
> > + get_user_reg(regs, 3),
> > + get_user_reg(regs, 4));
>
> same here
OK
>
> > + 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(get_user_reg(regs, 1),
> > + get_user_reg(regs, 2),
> > + get_user_reg(regs, 3),
> > + get_user_reg(regs, 4));
>
> same here
OK
>
> > + break;
> >
> > default:
> > gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> > @@ -348,6 +401,9 @@ static int ffa_domain_init(struct domain *d)
> > if ( !ffa_partinfo_domain_init(d) )
> > return -EIO;
> >
> > + if ( !ffa_notif_domain_init(d) )
> > + return -ENOMEM;
>
> Having this function deciding on the return code is a bit weird.
> I would suggest to have ffa_notif_domain_init returning an int
> and deciding on the error code and this one just returning the
> error if !=0.
>
> If possible the same principle should be applied for the partinfo.
OK, I'll fix it.
>
> > +
> > return 0;
> > }
> >
> > @@ -423,6 +479,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 +559,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..0173ee515362
> > --- /dev/null
> > +++ b/xen/arch/arm/tee/ffa_notif.c
> > @@ -0,0 +1,319 @@
> > +/* 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(uint32_t src_dst, uint32_t flags,
> > + uint32_t bitmap_lo, uint32_t bitmap_hi)
> > +{
> > + struct domain *d = current->domain;
> > +
> > + if ( !notif_enabled )
> > + return FFA_RET_NOT_SUPPORTED;
> > +
> > + if ( (src_dst & 0xffff) != ffa_get_vm_id(d) )
> > + return FFA_RET_INVALID_PARAMETERS;
>
> s/0xffff/0xFFFFU/
OK
>
> > +
> > + 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(uint32_t src_dst, uint32_t bitmap_lo,
> > + uint32_t bitmap_hi)
> > +{
> > + struct domain *d = current->domain;
> > +
> > + if ( !notif_enabled )
> > + return FFA_RET_NOT_SUPPORTED;
> > +
> > + if ( (src_dst & 0xffff) != ffa_get_vm_id(d) )
> > + return FFA_RET_INVALID_PARAMETERS;
>
> s/0xffff/0xFFFFU/
OK
>
> > +
> > + /*
> > + * 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);
> > + }
> > +}
> > +
> > +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 & 0xffff) != ffa_get_vm_id(d) )
> s/0xffff/0xFFFFU/
OK
>
> > + {
> > + 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;
> > +
> > + 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(uint32_t src_dst, uint32_t flags,
> > + uint32_t bitmap_lo, uint32_t bitmap_hi)
> > +{
> > + struct domain *d = current->domain;
> > +
> > + if ( !notif_enabled )
> > + return FFA_RET_NOT_SUPPORTED;
> > +
> > + if ( (src_dst >> 16) != ffa_get_vm_id(d) )
> > + return FFA_RET_INVALID_PARAMETERS;
>
> This needs some checking as i would have used the lowest bits here
> for the source and not the highest. The spec is using the same description
> for all ABIs so I am wondering if you are not using the destination instead of
> the source here.
This is a bit tricky because not all ABI functions define Sender and
Receiver in the same way. For FFA_NOTIFICATION_BIND it's the Sender
and Receiver of the notification, while for instance,
FFA_MSG_SEND_DIRECT_REQ defines it as the Sender and Receiver of the
message.
When the Hypervisor invokes FFA_NOTIFICATION_SET it's the Sender and
Receiver of the notification, that is, the guest is the same as the
sender of the notification. So the guest ID should go into BIT[31:16],
and the receiver of the notification in BIT[15:0].
When the guest invokes FFA_NOTIFICATION_SET the Hypervisor is
requested to signal notifications to the Sender endpoint BIT[31:16].
What's expected in BIT[15:0] isn't mentioned so I assume the
Hypervisor should ignore it since it already knows the guest ID.
Following that analysis, we should replace the if statement above with:
src_dst = ((uint32_t)ffa_get_vm_id(d) << 16) | (src_dst >> 16)
But I'm not certain I've understood the specification correctly, in
particular the part where the guest invokes FFA_NOTIFICATION_SET.
What's your take on this?
I don't use this function in my tests so it's perhaps better to wait
with the implementation of this function until it's used.
>
> > +
> > + /*
> > + * 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_SET, src_dst, flags, bitmap_lo,
> > + bitmap_hi);
> > +}
> > +
>
> The following function would deserve some explanation in a comment
> to clear up a bit what is done here and why.
I'll add a description.
>
> > +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_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));
> > +
> > + if ( d )
> > + {
> > + struct ffa_ctx *ctx = d->arch.tee;
> > +
> > + spin_lock(&ctx->notif.lock);
> > + ctx->notif.secure_pending = true;
> > + spin_unlock(&ctx->notif.lock);
> > +
> > + /*
> > + * 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.
> > + */
> > + vgic_inject_irq(d, d->vcpu[0], FFA_NOTIF_PEND_INTR_ID, true);
> > +
> > + put_domain(d);
> > + }
> > +
> > + id_pos += count;
> > + }
> > +
> > + } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
> > +}
> > +
> > +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(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;
> > + 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);
> > + notif_enabled = !ret;
> > +}
> > +
> > +bool ffa_notif_domain_init(struct domain *d)
> > +{
> > + struct ffa_ctx *ctx = d->arch.tee;
> > + int32_t res;
> > +
> > + if ( !notif_enabled )
> > + return true;
> > +
> > + res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
> > + if ( res )
> > + return false;
> > +
> > + ctx->notif.enabled = true;
> > +
> > + return true;
> > +}
> > +
> > +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_private.h b/xen/arch/arm/tee/ffa_private.h
> > index 98236cbf14a3..26c2af164d38 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
> > @@ -60,6 +61,8 @@
> > */
> > #define FFA_PAGE_SIZE SZ_4K
> >
> > +#define FFA_NOTIF_BITMAP_SIZE 64
> > +
>
> This does not seem to be used.
You're right, I'll remove it.
>
> > /*
> > * The number of pages used for each of the RX and TX buffers shared with
> > * the SPMC.
> > @@ -97,6 +100,18 @@
> > */
> > #define FFA_MAX_SHM_COUNT 32
> >
> > +/*
> > + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely
> > + * unused, but that may change.
>
> I am a bit wondering what your TODO means here.
> Do you mean that we should have a way to "allocate a free SGI" ?
As long as only the FF-A mediator adds a special meaning to certain
virtual SGIs in the 8-15 range it might be overkill with an allocator.
But if other parts start to do the same we may have conflicts if it's
not managed centrally.
Thanks,
Jens
>
> > + *
> > + * SGI is the preferred delivery mechanism. 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.
> > + */
> > +#define FFA_NOTIF_PEND_INTR_ID 8
> > +#define FFA_SCHEDULE_RECV_INTR_ID 9
> > +
> > /*
> > * The time we wait until trying to tear down a domain again if it was
> > * blocked initially.
> > @@ -175,6 +190,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 +243,27 @@
> > #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;
> > +
> > + /* Used to serialize access to the rest of this struct */
> > + spinlock_t lock;
> > +
> > + /*
> > + * 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 +279,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
> > @@ -271,12 +323,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);
> > +bool ffa_notif_domain_init(struct domain *d);
> > +void ffa_notif_domain_destroy(struct domain *d);
> > +
> > +int ffa_handle_notification_bind(uint32_t src_dst, uint32_t flags,
> > + uint32_t bitmap_lo, uint32_t bitmap_hi);
> > +int ffa_handle_notification_unbind(uint32_t src_dst, uint32_t bitmap_lo,
> > + uint32_t bitmap_hi);
> > +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(uint32_t src_dst, uint32_t flags,
> > + uint32_t bitmap_lo, uint32_t bitmap_hi);
> > +
> > 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)
> > +{
> > + /* -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,
> > --
> > 2.34.1
> >
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 5/5] xen/arm: ffa: support notification
2024-04-10 14:27 ` Jens Wiklander
@ 2024-04-10 14:47 ` Bertrand Marquis
2024-04-10 15:41 ` Bertrand Marquis
1 sibling, 0 replies; 21+ messages in thread
From: Bertrand Marquis @ 2024-04-10 14:47 UTC (permalink / raw)
To: Jens Wiklander
Cc: Xen-devel, patches, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Michal Orzel
Hi Jens,
> On 10 Apr 2024, at 16:27, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Wed, Apr 10, 2024 at 9:49 AM Bertrand Marquis
> <Bertrand.Marquis@arm.com> wrote:
>>
>> Hi Jens,
>>
>>> On 9 Apr 2024, at 17:36, 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 retrieves the notifications using the
>>> FF-A ABI and deliver them to their destinations.
>>>
>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>>> ---
>>> xen/arch/arm/tee/Makefile | 1 +
>>> xen/arch/arm/tee/ffa.c | 58 ++++++
>>> xen/arch/arm/tee/ffa_notif.c | 319 +++++++++++++++++++++++++++++++++
>>> xen/arch/arm/tee/ffa_private.h | 71 ++++++++
>>> 4 files changed, 449 insertions(+)
>>> 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..ce9757bfeed1 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, 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, FFA_NOTIF_PEND_INTR_ID, 0);
>>
>> This should return the RECV_INTR, not the PEND one.
>
> Thanks, I'll fix it.
>
>>
>>> + 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,30 @@ 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(get_user_reg(regs, 1),
>>> + get_user_reg(regs, 2),
>>> + get_user_reg(regs, 3),
>>> + get_user_reg(regs, 4));
>>
>> I would suggest to pass regs and handle the get_user_regs in the function.
>
> OK
>
>>
>>> + break;
>>> + case FFA_NOTIFICATION_UNBIND:
>>> + e = ffa_handle_notification_unbind(get_user_reg(regs, 1),
>>> + get_user_reg(regs, 3),
>>> + get_user_reg(regs, 4));
>>
>> same here
>
> OK
>
>>
>>> + 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(get_user_reg(regs, 1),
>>> + get_user_reg(regs, 2),
>>> + get_user_reg(regs, 3),
>>> + get_user_reg(regs, 4));
>>
>> same here
>
> OK
>
>>
>>> + break;
>>>
>>> default:
>>> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
>>> @@ -348,6 +401,9 @@ static int ffa_domain_init(struct domain *d)
>>> if ( !ffa_partinfo_domain_init(d) )
>>> return -EIO;
>>>
>>> + if ( !ffa_notif_domain_init(d) )
>>> + return -ENOMEM;
>>
>> Having this function deciding on the return code is a bit weird.
>> I would suggest to have ffa_notif_domain_init returning an int
>> and deciding on the error code and this one just returning the
>> error if !=0.
>>
>> If possible the same principle should be applied for the partinfo.
>
> OK, I'll fix it.
>
>>
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -423,6 +479,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 +559,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..0173ee515362
>>> --- /dev/null
>>> +++ b/xen/arch/arm/tee/ffa_notif.c
>>> @@ -0,0 +1,319 @@
>>> +/* 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(uint32_t src_dst, uint32_t flags,
>>> + uint32_t bitmap_lo, uint32_t bitmap_hi)
>>> +{
>>> + struct domain *d = current->domain;
>>> +
>>> + if ( !notif_enabled )
>>> + return FFA_RET_NOT_SUPPORTED;
>>> +
>>> + if ( (src_dst & 0xffff) != ffa_get_vm_id(d) )
>>> + return FFA_RET_INVALID_PARAMETERS;
>>
>> s/0xffff/0xFFFFU/
>
> OK
>
>>
>>> +
>>> + 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(uint32_t src_dst, uint32_t bitmap_lo,
>>> + uint32_t bitmap_hi)
>>> +{
>>> + struct domain *d = current->domain;
>>> +
>>> + if ( !notif_enabled )
>>> + return FFA_RET_NOT_SUPPORTED;
>>> +
>>> + if ( (src_dst & 0xffff) != ffa_get_vm_id(d) )
>>> + return FFA_RET_INVALID_PARAMETERS;
>>
>> s/0xffff/0xFFFFU/
>
> OK
>
>>
>>> +
>>> + /*
>>> + * 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);
>>> + }
>>> +}
>>> +
>>> +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 & 0xffff) != ffa_get_vm_id(d) )
>> s/0xffff/0xFFFFU/
>
> OK
>
>>
>>> + {
>>> + 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;
>>> +
>>> + 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(uint32_t src_dst, uint32_t flags,
>>> + uint32_t bitmap_lo, uint32_t bitmap_hi)
>>> +{
>>> + struct domain *d = current->domain;
>>> +
>>> + if ( !notif_enabled )
>>> + return FFA_RET_NOT_SUPPORTED;
>>> +
>>> + if ( (src_dst >> 16) != ffa_get_vm_id(d) )
>>> + return FFA_RET_INVALID_PARAMETERS;
>>
>> This needs some checking as i would have used the lowest bits here
>> for the source and not the highest. The spec is using the same description
>> for all ABIs so I am wondering if you are not using the destination instead of
>> the source here.
>
> This is a bit tricky because not all ABI functions define Sender and
> Receiver in the same way. For FFA_NOTIFICATION_BIND it's the Sender
> and Receiver of the notification, while for instance,
> FFA_MSG_SEND_DIRECT_REQ defines it as the Sender and Receiver of the
> message.
>
> When the Hypervisor invokes FFA_NOTIFICATION_SET it's the Sender and
> Receiver of the notification, that is, the guest is the same as the
> sender of the notification. So the guest ID should go into BIT[31:16],
> and the receiver of the notification in BIT[15:0].
>
> When the guest invokes FFA_NOTIFICATION_SET the Hypervisor is
> requested to signal notifications to the Sender endpoint BIT[31:16].
> What's expected in BIT[15:0] isn't mentioned so I assume the
> Hypervisor should ignore it since it already knows the guest ID.
>
> Following that analysis, we should replace the if statement above with:
> src_dst = ((uint32_t)ffa_get_vm_id(d) << 16) | (src_dst >> 16)
>
> But I'm not certain I've understood the specification correctly, in
> particular the part where the guest invokes FFA_NOTIFICATION_SET.
>
> What's your take on this?
>
> I don't use this function in my tests so it's perhaps better to wait
> with the implementation of this function until it's used.
You are right, this is a bit imprecise.
In notification_set Sender is defined as "signal to the sender" and then saying that "sender" is the VM when the hypervisor push it to the SPMC.
I will dig on that and come back to you on this.
Cheers
Bertrand
>
>>
>>> +
>>> + /*
>>> + * 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_SET, src_dst, flags, bitmap_lo,
>>> + bitmap_hi);
>>> +}
>>> +
>>
>> The following function would deserve some explanation in a comment
>> to clear up a bit what is done here and why.
>
> I'll add a description.
>
>>
>>> +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_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));
>>> +
>>> + if ( d )
>>> + {
>>> + struct ffa_ctx *ctx = d->arch.tee;
>>> +
>>> + spin_lock(&ctx->notif.lock);
>>> + ctx->notif.secure_pending = true;
>>> + spin_unlock(&ctx->notif.lock);
>>> +
>>> + /*
>>> + * 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.
>>> + */
>>> + vgic_inject_irq(d, d->vcpu[0], FFA_NOTIF_PEND_INTR_ID, true);
>>> +
>>> + put_domain(d);
>>> + }
>>> +
>>> + id_pos += count;
>>> + }
>>> +
>>> + } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
>>> +}
>>> +
>>> +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(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;
>>> + 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);
>>> + notif_enabled = !ret;
>>> +}
>>> +
>>> +bool ffa_notif_domain_init(struct domain *d)
>>> +{
>>> + struct ffa_ctx *ctx = d->arch.tee;
>>> + int32_t res;
>>> +
>>> + if ( !notif_enabled )
>>> + return true;
>>> +
>>> + res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
>>> + if ( res )
>>> + return false;
>>> +
>>> + ctx->notif.enabled = true;
>>> +
>>> + return true;
>>> +}
>>> +
>>> +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_private.h b/xen/arch/arm/tee/ffa_private.h
>>> index 98236cbf14a3..26c2af164d38 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
>>> @@ -60,6 +61,8 @@
>>> */
>>> #define FFA_PAGE_SIZE SZ_4K
>>>
>>> +#define FFA_NOTIF_BITMAP_SIZE 64
>>> +
>>
>> This does not seem to be used.
>
> You're right, I'll remove it.
>
>>
>>> /*
>>> * The number of pages used for each of the RX and TX buffers shared with
>>> * the SPMC.
>>> @@ -97,6 +100,18 @@
>>> */
>>> #define FFA_MAX_SHM_COUNT 32
>>>
>>> +/*
>>> + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely
>>> + * unused, but that may change.
>>
>> I am a bit wondering what your TODO means here.
>> Do you mean that we should have a way to "allocate a free SGI" ?
>
> As long as only the FF-A mediator adds a special meaning to certain
> virtual SGIs in the 8-15 range it might be overkill with an allocator.
> But if other parts start to do the same we may have conflicts if it's
> not managed centrally.
>
> Thanks,
> Jens
>
>>
>>> + *
>>> + * SGI is the preferred delivery mechanism. 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.
>>> + */
>>> +#define FFA_NOTIF_PEND_INTR_ID 8
>>> +#define FFA_SCHEDULE_RECV_INTR_ID 9
>>> +
>>> /*
>>> * The time we wait until trying to tear down a domain again if it was
>>> * blocked initially.
>>> @@ -175,6 +190,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 +243,27 @@
>>> #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;
>>> +
>>> + /* Used to serialize access to the rest of this struct */
>>> + spinlock_t lock;
>>> +
>>> + /*
>>> + * 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 +279,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
>>> @@ -271,12 +323,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);
>>> +bool ffa_notif_domain_init(struct domain *d);
>>> +void ffa_notif_domain_destroy(struct domain *d);
>>> +
>>> +int ffa_handle_notification_bind(uint32_t src_dst, uint32_t flags,
>>> + uint32_t bitmap_lo, uint32_t bitmap_hi);
>>> +int ffa_handle_notification_unbind(uint32_t src_dst, uint32_t bitmap_lo,
>>> + uint32_t bitmap_hi);
>>> +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(uint32_t src_dst, uint32_t flags,
>>> + uint32_t bitmap_lo, uint32_t bitmap_hi);
>>> +
>>> 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)
>>> +{
>>> + /* -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,
>>> --
>>> 2.34.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 5/5] xen/arm: ffa: support notification
2024-04-10 14:27 ` Jens Wiklander
2024-04-10 14:47 ` Bertrand Marquis
@ 2024-04-10 15:41 ` Bertrand Marquis
2024-04-10 15:47 ` Jens Wiklander
1 sibling, 1 reply; 21+ messages in thread
From: Bertrand Marquis @ 2024-04-10 15:41 UTC (permalink / raw)
To: Jens Wiklander
Cc: Xen-devel, patches, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Michal Orzel
Hi Jens,
> On 10 Apr 2024, at 16:27, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Wed, Apr 10, 2024 at 9:49 AM Bertrand Marquis
> <Bertrand.Marquis@arm.com> wrote:
>>
>> Hi Jens,
>>
>>> On 9 Apr 2024, at 17:36, 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 retrieves the notifications using the
>>> FF-A ABI and deliver them to their destinations.
>>>
>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>>> ---
>>> xen/arch/arm/tee/Makefile | 1 +
>>> xen/arch/arm/tee/ffa.c | 58 ++++++
>>> xen/arch/arm/tee/ffa_notif.c | 319 +++++++++++++++++++++++++++++++++
>>> xen/arch/arm/tee/ffa_private.h | 71 ++++++++
>>> 4 files changed, 449 insertions(+)
>>> 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..ce9757bfeed1 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, 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, FFA_NOTIF_PEND_INTR_ID, 0);
>>
>> This should return the RECV_INTR, not the PEND one.
>
> Thanks, I'll fix it.
>
>>
>>> + 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,30 @@ 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(get_user_reg(regs, 1),
>>> + get_user_reg(regs, 2),
>>> + get_user_reg(regs, 3),
>>> + get_user_reg(regs, 4));
>>
>> I would suggest to pass regs and handle the get_user_regs in the function.
>
> OK
>
>>
>>> + break;
>>> + case FFA_NOTIFICATION_UNBIND:
>>> + e = ffa_handle_notification_unbind(get_user_reg(regs, 1),
>>> + get_user_reg(regs, 3),
>>> + get_user_reg(regs, 4));
>>
>> same here
>
> OK
>
>>
>>> + 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(get_user_reg(regs, 1),
>>> + get_user_reg(regs, 2),
>>> + get_user_reg(regs, 3),
>>> + get_user_reg(regs, 4));
>>
>> same here
>
> OK
>
>>
>>> + break;
>>>
>>> default:
>>> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
>>> @@ -348,6 +401,9 @@ static int ffa_domain_init(struct domain *d)
>>> if ( !ffa_partinfo_domain_init(d) )
>>> return -EIO;
>>>
>>> + if ( !ffa_notif_domain_init(d) )
>>> + return -ENOMEM;
>>
>> Having this function deciding on the return code is a bit weird.
>> I would suggest to have ffa_notif_domain_init returning an int
>> and deciding on the error code and this one just returning the
>> error if !=0.
>>
>> If possible the same principle should be applied for the partinfo.
>
> OK, I'll fix it.
>
>>
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -423,6 +479,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 +559,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..0173ee515362
>>> --- /dev/null
>>> +++ b/xen/arch/arm/tee/ffa_notif.c
>>> @@ -0,0 +1,319 @@
>>> +/* 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(uint32_t src_dst, uint32_t flags,
>>> + uint32_t bitmap_lo, uint32_t bitmap_hi)
>>> +{
>>> + struct domain *d = current->domain;
>>> +
>>> + if ( !notif_enabled )
>>> + return FFA_RET_NOT_SUPPORTED;
>>> +
>>> + if ( (src_dst & 0xffff) != ffa_get_vm_id(d) )
>>> + return FFA_RET_INVALID_PARAMETERS;
>>
>> s/0xffff/0xFFFFU/
>
> OK
>
>>
>>> +
>>> + 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(uint32_t src_dst, uint32_t bitmap_lo,
>>> + uint32_t bitmap_hi)
>>> +{
>>> + struct domain *d = current->domain;
>>> +
>>> + if ( !notif_enabled )
>>> + return FFA_RET_NOT_SUPPORTED;
>>> +
>>> + if ( (src_dst & 0xffff) != ffa_get_vm_id(d) )
>>> + return FFA_RET_INVALID_PARAMETERS;
>>
>> s/0xffff/0xFFFFU/
>
> OK
>
>>
>>> +
>>> + /*
>>> + * 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);
>>> + }
>>> +}
>>> +
>>> +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 & 0xffff) != ffa_get_vm_id(d) )
>> s/0xffff/0xFFFFU/
>
> OK
>
>>
>>> + {
>>> + 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;
>>> +
>>> + 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(uint32_t src_dst, uint32_t flags,
>>> + uint32_t bitmap_lo, uint32_t bitmap_hi)
>>> +{
>>> + struct domain *d = current->domain;
>>> +
>>> + if ( !notif_enabled )
>>> + return FFA_RET_NOT_SUPPORTED;
>>> +
>>> + if ( (src_dst >> 16) != ffa_get_vm_id(d) )
>>> + return FFA_RET_INVALID_PARAMETERS;
>>
>> This needs some checking as i would have used the lowest bits here
>> for the source and not the highest. The spec is using the same description
>> for all ABIs so I am wondering if you are not using the destination instead of
>> the source here.
>
> This is a bit tricky because not all ABI functions define Sender and
> Receiver in the same way. For FFA_NOTIFICATION_BIND it's the Sender
> and Receiver of the notification, while for instance,
> FFA_MSG_SEND_DIRECT_REQ defines it as the Sender and Receiver of the
> message.
>
> When the Hypervisor invokes FFA_NOTIFICATION_SET it's the Sender and
> Receiver of the notification, that is, the guest is the same as the
> sender of the notification. So the guest ID should go into BIT[31:16],
> and the receiver of the notification in BIT[15:0].
>
> When the guest invokes FFA_NOTIFICATION_SET the Hypervisor is
> requested to signal notifications to the Sender endpoint BIT[31:16].
> What's expected in BIT[15:0] isn't mentioned so I assume the
> Hypervisor should ignore it since it already knows the guest ID.
>
> Following that analysis, we should replace the if statement above with:
> src_dst = ((uint32_t)ffa_get_vm_id(d) << 16) | (src_dst >> 16)
>
> But I'm not certain I've understood the specification correctly, in
> particular the part where the guest invokes FFA_NOTIFICATION_SET.
>
> What's your take on this?
>
> I don't use this function in my tests so it's perhaps better to wait
> with the implementation of this function until it's used.
I discussed this internally and in fact we have a typo in the
NOTIFICATION_SET description that we have to fix (first bullet of
description should say receiver and not sender)
So the implementation should check the lowest bits to be the caller ID
for all calls except notification_set where it should check the highest bits.
In notification_set the lowest bits are encoding the ID of the endpoint to
which a notification must be generated.
Tell me if this is clearing it up :-)
Cheers
Bertrand
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 5/5] xen/arm: ffa: support notification
2024-04-09 15:36 ` [XEN PATCH v1 5/5] xen/arm: ffa: support notification Jens Wiklander
2024-04-10 7:49 ` Bertrand Marquis
@ 2024-04-10 15:45 ` Jens Wiklander
2024-04-10 16:30 ` Bertrand Marquis
1 sibling, 1 reply; 21+ messages in thread
From: Jens Wiklander @ 2024-04-10 15:45 UTC (permalink / raw)
To: xen-devel
Cc: patches, Volodymyr Babchuk, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel
On Tue, Apr 9, 2024 at 5:36 PM 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 retrieves the notifications using the
> FF-A ABI and deliver them to their destinations.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
> xen/arch/arm/tee/Makefile | 1 +
> xen/arch/arm/tee/ffa.c | 58 ++++++
> xen/arch/arm/tee/ffa_notif.c | 319 +++++++++++++++++++++++++++++++++
> xen/arch/arm/tee/ffa_private.h | 71 ++++++++
> 4 files changed, 449 insertions(+)
> 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..ce9757bfeed1 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, 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, FFA_NOTIF_PEND_INTR_ID, 0);
> + else
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + break;
With the recently posted kernel patch
https://lore.kernel.org/all/20240410-ffa_npi_support-v1-3-1a5223391bd1@arm.com/
we need to decide which feature interrupt to return since the kernel
will only install a handle for the first it finds. Right now I propose
to to not report FFA_FEATURE_SCHEDULE_RECV_INTR. When the time comes
to use a secondary scheduler we'll need to report the SRI instead.
Cheers,
Jens
> +
> + 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,30 @@ 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(get_user_reg(regs, 1),
> + get_user_reg(regs, 2),
> + get_user_reg(regs, 3),
> + get_user_reg(regs, 4));
> + break;
> + case FFA_NOTIFICATION_UNBIND:
> + e = ffa_handle_notification_unbind(get_user_reg(regs, 1),
> + get_user_reg(regs, 3),
> + get_user_reg(regs, 4));
> + 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(get_user_reg(regs, 1),
> + get_user_reg(regs, 2),
> + get_user_reg(regs, 3),
> + get_user_reg(regs, 4));
> + break;
>
> default:
> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> @@ -348,6 +401,9 @@ static int ffa_domain_init(struct domain *d)
> if ( !ffa_partinfo_domain_init(d) )
> return -EIO;
>
> + if ( !ffa_notif_domain_init(d) )
> + return -ENOMEM;
> +
> return 0;
> }
>
> @@ -423,6 +479,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 +559,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..0173ee515362
> --- /dev/null
> +++ b/xen/arch/arm/tee/ffa_notif.c
> @@ -0,0 +1,319 @@
> +/* 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(uint32_t src_dst, uint32_t flags,
> + uint32_t bitmap_lo, uint32_t bitmap_hi)
> +{
> + struct domain *d = current->domain;
> +
> + if ( !notif_enabled )
> + return FFA_RET_NOT_SUPPORTED;
> +
> + if ( (src_dst & 0xffff) != 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(uint32_t src_dst, uint32_t bitmap_lo,
> + uint32_t bitmap_hi)
> +{
> + struct domain *d = current->domain;
> +
> + if ( !notif_enabled )
> + return FFA_RET_NOT_SUPPORTED;
> +
> + if ( (src_dst & 0xffff) != 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);
> + }
> +}
> +
> +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 & 0xffff) != 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;
> +
> + 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(uint32_t src_dst, uint32_t flags,
> + uint32_t bitmap_lo, uint32_t bitmap_hi)
> +{
> + struct domain *d = current->domain;
> +
> + if ( !notif_enabled )
> + return FFA_RET_NOT_SUPPORTED;
> +
> + if ( (src_dst >> 16) != ffa_get_vm_id(d) )
> + return FFA_RET_INVALID_PARAMETERS;
> +
> + /*
> + * 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_SET, src_dst, flags, bitmap_lo,
> + bitmap_hi);
> +}
> +
> +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_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));
> +
> + if ( d )
> + {
> + struct ffa_ctx *ctx = d->arch.tee;
> +
> + spin_lock(&ctx->notif.lock);
> + ctx->notif.secure_pending = true;
> + spin_unlock(&ctx->notif.lock);
> +
> + /*
> + * 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.
> + */
> + vgic_inject_irq(d, d->vcpu[0], FFA_NOTIF_PEND_INTR_ID, true);
> +
> + put_domain(d);
> + }
> +
> + id_pos += count;
> + }
> +
> + } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
> +}
> +
> +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(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;
> + 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);
> + notif_enabled = !ret;
> +}
> +
> +bool ffa_notif_domain_init(struct domain *d)
> +{
> + struct ffa_ctx *ctx = d->arch.tee;
> + int32_t res;
> +
> + if ( !notif_enabled )
> + return true;
> +
> + res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
> + if ( res )
> + return false;
> +
> + ctx->notif.enabled = true;
> +
> + return true;
> +}
> +
> +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_private.h b/xen/arch/arm/tee/ffa_private.h
> index 98236cbf14a3..26c2af164d38 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
> @@ -60,6 +61,8 @@
> */
> #define FFA_PAGE_SIZE SZ_4K
>
> +#define FFA_NOTIF_BITMAP_SIZE 64
> +
> /*
> * The number of pages used for each of the RX and TX buffers shared with
> * the SPMC.
> @@ -97,6 +100,18 @@
> */
> #define FFA_MAX_SHM_COUNT 32
>
> +/*
> + * TODO How to manage the available SGIs? SGI 8-15 seem to be entirely
> + * unused, but that may change.
> + *
> + * SGI is the preferred delivery mechanism. 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.
> + */
> +#define FFA_NOTIF_PEND_INTR_ID 8
> +#define FFA_SCHEDULE_RECV_INTR_ID 9
> +
> /*
> * The time we wait until trying to tear down a domain again if it was
> * blocked initially.
> @@ -175,6 +190,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 +243,27 @@
> #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;
> +
> + /* Used to serialize access to the rest of this struct */
> + spinlock_t lock;
> +
> + /*
> + * 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 +279,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
> @@ -271,12 +323,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);
> +bool ffa_notif_domain_init(struct domain *d);
> +void ffa_notif_domain_destroy(struct domain *d);
> +
> +int ffa_handle_notification_bind(uint32_t src_dst, uint32_t flags,
> + uint32_t bitmap_lo, uint32_t bitmap_hi);
> +int ffa_handle_notification_unbind(uint32_t src_dst, uint32_t bitmap_lo,
> + uint32_t bitmap_hi);
> +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(uint32_t src_dst, uint32_t flags,
> + uint32_t bitmap_lo, uint32_t bitmap_hi);
> +
> 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)
> +{
> + /* -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,
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 5/5] xen/arm: ffa: support notification
2024-04-10 15:41 ` Bertrand Marquis
@ 2024-04-10 15:47 ` Jens Wiklander
0 siblings, 0 replies; 21+ messages in thread
From: Jens Wiklander @ 2024-04-10 15:47 UTC (permalink / raw)
To: Bertrand Marquis
Cc: Xen-devel, patches, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Michal Orzel
Hi Bertrand,
On Wed, Apr 10, 2024 at 5:41 PM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Jens,
>
> > On 10 Apr 2024, at 16:27, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > On Wed, Apr 10, 2024 at 9:49 AM Bertrand Marquis
> > <Bertrand.Marquis@arm.com> wrote:
> >>
> >> Hi Jens,
> >>
> >>> On 9 Apr 2024, at 17:36, 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 retrieves the notifications using the
> >>> FF-A ABI and deliver them to their destinations.
> >>>
> >>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> >>> ---
> >>> xen/arch/arm/tee/Makefile | 1 +
> >>> xen/arch/arm/tee/ffa.c | 58 ++++++
> >>> xen/arch/arm/tee/ffa_notif.c | 319 +++++++++++++++++++++++++++++++++
> >>> xen/arch/arm/tee/ffa_private.h | 71 ++++++++
> >>> 4 files changed, 449 insertions(+)
> >>> 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..ce9757bfeed1 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, 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, FFA_NOTIF_PEND_INTR_ID, 0);
> >>
> >> This should return the RECV_INTR, not the PEND one.
> >
> > Thanks, I'll fix it.
> >
> >>
> >>> + 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,30 @@ 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(get_user_reg(regs, 1),
> >>> + get_user_reg(regs, 2),
> >>> + get_user_reg(regs, 3),
> >>> + get_user_reg(regs, 4));
> >>
> >> I would suggest to pass regs and handle the get_user_regs in the function.
> >
> > OK
> >
> >>
> >>> + break;
> >>> + case FFA_NOTIFICATION_UNBIND:
> >>> + e = ffa_handle_notification_unbind(get_user_reg(regs, 1),
> >>> + get_user_reg(regs, 3),
> >>> + get_user_reg(regs, 4));
> >>
> >> same here
> >
> > OK
> >
> >>
> >>> + 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(get_user_reg(regs, 1),
> >>> + get_user_reg(regs, 2),
> >>> + get_user_reg(regs, 3),
> >>> + get_user_reg(regs, 4));
> >>
> >> same here
> >
> > OK
> >
> >>
> >>> + break;
> >>>
> >>> default:
> >>> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> >>> @@ -348,6 +401,9 @@ static int ffa_domain_init(struct domain *d)
> >>> if ( !ffa_partinfo_domain_init(d) )
> >>> return -EIO;
> >>>
> >>> + if ( !ffa_notif_domain_init(d) )
> >>> + return -ENOMEM;
> >>
> >> Having this function deciding on the return code is a bit weird.
> >> I would suggest to have ffa_notif_domain_init returning an int
> >> and deciding on the error code and this one just returning the
> >> error if !=0.
> >>
> >> If possible the same principle should be applied for the partinfo.
> >
> > OK, I'll fix it.
> >
> >>
> >>> +
> >>> return 0;
> >>> }
> >>>
> >>> @@ -423,6 +479,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 +559,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..0173ee515362
> >>> --- /dev/null
> >>> +++ b/xen/arch/arm/tee/ffa_notif.c
> >>> @@ -0,0 +1,319 @@
> >>> +/* 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(uint32_t src_dst, uint32_t flags,
> >>> + uint32_t bitmap_lo, uint32_t bitmap_hi)
> >>> +{
> >>> + struct domain *d = current->domain;
> >>> +
> >>> + if ( !notif_enabled )
> >>> + return FFA_RET_NOT_SUPPORTED;
> >>> +
> >>> + if ( (src_dst & 0xffff) != ffa_get_vm_id(d) )
> >>> + return FFA_RET_INVALID_PARAMETERS;
> >>
> >> s/0xffff/0xFFFFU/
> >
> > OK
> >
> >>
> >>> +
> >>> + 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(uint32_t src_dst, uint32_t bitmap_lo,
> >>> + uint32_t bitmap_hi)
> >>> +{
> >>> + struct domain *d = current->domain;
> >>> +
> >>> + if ( !notif_enabled )
> >>> + return FFA_RET_NOT_SUPPORTED;
> >>> +
> >>> + if ( (src_dst & 0xffff) != ffa_get_vm_id(d) )
> >>> + return FFA_RET_INVALID_PARAMETERS;
> >>
> >> s/0xffff/0xFFFFU/
> >
> > OK
> >
> >>
> >>> +
> >>> + /*
> >>> + * 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);
> >>> + }
> >>> +}
> >>> +
> >>> +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 & 0xffff) != ffa_get_vm_id(d) )
> >> s/0xffff/0xFFFFU/
> >
> > OK
> >
> >>
> >>> + {
> >>> + 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;
> >>> +
> >>> + 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(uint32_t src_dst, uint32_t flags,
> >>> + uint32_t bitmap_lo, uint32_t bitmap_hi)
> >>> +{
> >>> + struct domain *d = current->domain;
> >>> +
> >>> + if ( !notif_enabled )
> >>> + return FFA_RET_NOT_SUPPORTED;
> >>> +
> >>> + if ( (src_dst >> 16) != ffa_get_vm_id(d) )
> >>> + return FFA_RET_INVALID_PARAMETERS;
> >>
> >> This needs some checking as i would have used the lowest bits here
> >> for the source and not the highest. The spec is using the same description
> >> for all ABIs so I am wondering if you are not using the destination instead of
> >> the source here.
> >
> > This is a bit tricky because not all ABI functions define Sender and
> > Receiver in the same way. For FFA_NOTIFICATION_BIND it's the Sender
> > and Receiver of the notification, while for instance,
> > FFA_MSG_SEND_DIRECT_REQ defines it as the Sender and Receiver of the
> > message.
> >
> > When the Hypervisor invokes FFA_NOTIFICATION_SET it's the Sender and
> > Receiver of the notification, that is, the guest is the same as the
> > sender of the notification. So the guest ID should go into BIT[31:16],
> > and the receiver of the notification in BIT[15:0].
> >
> > When the guest invokes FFA_NOTIFICATION_SET the Hypervisor is
> > requested to signal notifications to the Sender endpoint BIT[31:16].
> > What's expected in BIT[15:0] isn't mentioned so I assume the
> > Hypervisor should ignore it since it already knows the guest ID.
> >
> > Following that analysis, we should replace the if statement above with:
> > src_dst = ((uint32_t)ffa_get_vm_id(d) << 16) | (src_dst >> 16)
> >
> > But I'm not certain I've understood the specification correctly, in
> > particular the part where the guest invokes FFA_NOTIFICATION_SET.
> >
> > What's your take on this?
> >
> > I don't use this function in my tests so it's perhaps better to wait
> > with the implementation of this function until it's used.
>
> I discussed this internally and in fact we have a typo in the
> NOTIFICATION_SET description that we have to fix (first bullet of
> description should say receiver and not sender)
>
> So the implementation should check the lowest bits to be the caller ID
> for all calls except notification_set where it should check the highest bits.
>
> In notification_set the lowest bits are encoding the ID of the endpoint to
> which a notification must be generated.
>
> Tell me if this is clearing it up :-)
Thanks for looking into this. It's clear now. I'll update the code
accordingly in the next version.
Cheers,
Jens
>
> Cheers
> Bertrand
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 5/5] xen/arm: ffa: support notification
2024-04-10 15:45 ` Jens Wiklander
@ 2024-04-10 16:30 ` Bertrand Marquis
2024-04-11 5:43 ` Jens Wiklander
0 siblings, 1 reply; 21+ messages in thread
From: Bertrand Marquis @ 2024-04-10 16:30 UTC (permalink / raw)
To: Jens Wiklander
Cc: Xen-devel, patches, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Michal Orzel
Hi Jens,
> On 10 Apr 2024, at 17:45, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Tue, Apr 9, 2024 at 5:36 PM 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 retrieves the notifications using the
>> FF-A ABI and deliver them to their destinations.
>>
>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>> ---
>> xen/arch/arm/tee/Makefile | 1 +
>> xen/arch/arm/tee/ffa.c | 58 ++++++
>> xen/arch/arm/tee/ffa_notif.c | 319 +++++++++++++++++++++++++++++++++
>> xen/arch/arm/tee/ffa_private.h | 71 ++++++++
>> 4 files changed, 449 insertions(+)
>> 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..ce9757bfeed1 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, 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, FFA_NOTIF_PEND_INTR_ID, 0);
>> + else
>> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>> + break;
>
> With the recently posted kernel patch
> https://lore.kernel.org/all/20240410-ffa_npi_support-v1-3-1a5223391bd1@arm.com/
> we need to decide which feature interrupt to return since the kernel
> will only install a handle for the first it finds. Right now I propose
> to to not report FFA_FEATURE_SCHEDULE_RECV_INTR. When the time comes
> to use a secondary scheduler we'll need to report the SRI instead.
We just had a meeting with Sudeep to discuss that matter and he agreed that
he would register the interrupt handler for all the interrupts available so that
we can keep a model where we will generate SRIs only to a secondary scheduler
and NPI for notification interrupts (so that the VM does not do a INFO_GET when
not required).
We will have to report both as any VM could act as secondary scheduler for SPs
in theory (we might need at some point a parameter for that) but for now those
should only be generated to Dom0 if there are pending notifications for SPs.
Cheers
Bertrand
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 5/5] xen/arm: ffa: support notification
2024-04-10 16:30 ` Bertrand Marquis
@ 2024-04-11 5:43 ` Jens Wiklander
0 siblings, 0 replies; 21+ messages in thread
From: Jens Wiklander @ 2024-04-11 5:43 UTC (permalink / raw)
To: Bertrand Marquis
Cc: Xen-devel, patches, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Michal Orzel
Hi Bertrand,
On Wed, Apr 10, 2024 at 6:30 PM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Jens,
>
> > On 10 Apr 2024, at 17:45, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > On Tue, Apr 9, 2024 at 5:36 PM 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 retrieves the notifications using the
> >> FF-A ABI and deliver them to their destinations.
> >>
> >> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> >> ---
[snip]
> >> + case FFA_FEATURE_NOTIF_PEND_INTR:
> >> + if ( ctx->notif.enabled )
> >> + ffa_set_regs_success(regs, 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, FFA_NOTIF_PEND_INTR_ID, 0);
> >> + else
> >> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> >> + break;
> >
> > With the recently posted kernel patch
> > https://lore.kernel.org/all/20240410-ffa_npi_support-v1-3-1a5223391bd1@arm.com/
> > we need to decide which feature interrupt to return since the kernel
> > will only install a handle for the first it finds. Right now I propose
> > to to not report FFA_FEATURE_SCHEDULE_RECV_INTR. When the time comes
> > to use a secondary scheduler we'll need to report the SRI instead.
>
>
> We just had a meeting with Sudeep to discuss that matter and he agreed that
> he would register the interrupt handler for all the interrupts available so that
> we can keep a model where we will generate SRIs only to a secondary scheduler
> and NPI for notification interrupts (so that the VM does not do a INFO_GET when
> not required).
>
> We will have to report both as any VM could act as secondary scheduler for SPs
> in theory (we might need at some point a parameter for that) but for now those
> should only be generated to Dom0 if there are pending notifications for SPs.
OK, thanks. I'll keep both then.
Cheers,
Jens
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 4/5] xen/arm: allow dynamically assigned SGI handlers
2024-04-10 13:24 ` Michal Orzel
@ 2024-04-11 6:12 ` Jens Wiklander
2024-04-11 6:44 ` Michal Orzel
0 siblings, 1 reply; 21+ messages in thread
From: Jens Wiklander @ 2024-04-11 6:12 UTC (permalink / raw)
To: Michal Orzel
Cc: xen-devel, patches, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
Hi Michal,
On Wed, Apr 10, 2024 at 3:24 PM Michal Orzel <michal.orzel@amd.com> wrote:
>
> Hi Jens,
>
> On 09/04/2024 17:36, Jens Wiklander wrote:
> >
> >
> > Updates so request_irq() can be used with a dynamically assigned SGI irq
> > as input.
> At this point it would be handy to mention the use case for which you need to add this support
OK, I'll add something like:
This prepares for a later patch where an FF-A schedule receiver
interrupt handler is installed for an SGI generated by the secure
world.
>
> >
> > gic_route_irq_to_xen() don't gic_set_irq_type() for SGIs since they have
> > their type assigned earlier during boot
> Could you elaborate more on that? Do you mean that SGIs are always edge triggered and there's no need
> for setting the type?
Yes, see https://developer.arm.com/documentation/ihi0069/h
4.4 Software Generated Interrupts
SGIs are typically used for inter-processor communication, and are
generated by a write to an SGI register in the
GIC. SGIs can be either Group 0 or Group 1 interrupts, and they can
support only edge-triggered behavior.
How about:
SGI should only be configured as edge triggered.
Thanks,
Jens
>
> >
> > gic_interrupt() is updated to route the dynamically assigned SGIs to
> > do_IRQ() instead of do_sgi(). The latter still handles the statically
> > assigned SGI handlers like for instance GIC_SGI_CALL_FUNCTION.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> Other than that, it LGTM:
> Acked-by: Michal Orzel <michal.orzel@amd.com>
>
> but I would like other maintainers (especially Julien) to cross check it.
>
> ~Michal
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 4/5] xen/arm: allow dynamically assigned SGI handlers
2024-04-11 6:12 ` Jens Wiklander
@ 2024-04-11 6:44 ` Michal Orzel
2024-04-11 7:05 ` Jens Wiklander
0 siblings, 1 reply; 21+ messages in thread
From: Michal Orzel @ 2024-04-11 6:44 UTC (permalink / raw)
To: Jens Wiklander
Cc: xen-devel, patches, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
Hi Jens,
On 11/04/2024 08:12, Jens Wiklander wrote:
>
>
> Hi Michal,
>
> On Wed, Apr 10, 2024 at 3:24 PM Michal Orzel <michal.orzel@amd.com> wrote:
>>
>> Hi Jens,
>>
>> On 09/04/2024 17:36, Jens Wiklander wrote:
>>>
>>>
>>> Updates so request_irq() can be used with a dynamically assigned SGI irq
>>> as input.
>> At this point it would be handy to mention the use case for which you need to add this support
>
> OK, I'll add something like:
> This prepares for a later patch where an FF-A schedule receiver
> interrupt handler is installed for an SGI generated by the secure
> world.
ok
>
>>
>>>
>>> gic_route_irq_to_xen() don't gic_set_irq_type() for SGIs since they have
>>> their type assigned earlier during boot
>> Could you elaborate more on that? Do you mean that SGIs are always edge triggered and there's no need
>> for setting the type?
>
> Yes, see https://developer.arm.com/documentation/ihi0069/h
> 4.4 Software Generated Interrupts
> SGIs are typically used for inter-processor communication, and are
> generated by a write to an SGI register in the
> GIC. SGIs can be either Group 0 or Group 1 interrupts, and they can
> support only edge-triggered behavior.
Exactly. But you wrote "have their type assigned earlier during boot" which is not true.
There is no write to ICFGR0 in Xen codebase. They are implicitly edge triggered.
So I would write:
"... for SGIs since they are always edge triggered"
~Michal
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [XEN PATCH v1 4/5] xen/arm: allow dynamically assigned SGI handlers
2024-04-11 6:44 ` Michal Orzel
@ 2024-04-11 7:05 ` Jens Wiklander
0 siblings, 0 replies; 21+ messages in thread
From: Jens Wiklander @ 2024-04-11 7:05 UTC (permalink / raw)
To: Michal Orzel
Cc: xen-devel, patches, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
Hi Michal,
On Thu, Apr 11, 2024 at 8:44 AM Michal Orzel <michal.orzel@amd.com> wrote:
>
> Hi Jens,
>
> On 11/04/2024 08:12, Jens Wiklander wrote:
> >
> >
> > Hi Michal,
> >
> > On Wed, Apr 10, 2024 at 3:24 PM Michal Orzel <michal.orzel@amd.com> wrote:
> >>
> >> Hi Jens,
> >>
> >> On 09/04/2024 17:36, Jens Wiklander wrote:
> >>>
> >>>
> >>> Updates so request_irq() can be used with a dynamically assigned SGI irq
> >>> as input.
> >> At this point it would be handy to mention the use case for which you need to add this support
> >
> > OK, I'll add something like:
> > This prepares for a later patch where an FF-A schedule receiver
> > interrupt handler is installed for an SGI generated by the secure
> > world.
> ok
>
> >
> >>
> >>>
> >>> gic_route_irq_to_xen() don't gic_set_irq_type() for SGIs since they have
> >>> their type assigned earlier during boot
> >> Could you elaborate more on that? Do you mean that SGIs are always edge triggered and there's no need
> >> for setting the type?
> >
> > Yes, see https://developer.arm.com/documentation/ihi0069/h
> > 4.4 Software Generated Interrupts
> > SGIs are typically used for inter-processor communication, and are
> > generated by a write to an SGI register in the
> > GIC. SGIs can be either Group 0 or Group 1 interrupts, and they can
> > support only edge-triggered behavior.
> Exactly. But you wrote "have their type assigned earlier during boot" which is not true.
> There is no write to ICFGR0 in Xen codebase. They are implicitly edge triggered.
> So I would write:
> "... for SGIs since they are always edge triggered"
I'll use that instead.
Thanks,
Jens
>
> ~Michal
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2024-04-11 7:06 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-09 15:36 [XEN PATCH v1 0/5] FF-A notifications Jens Wiklander
2024-04-09 15:36 ` [XEN PATCH v1 1/5] xen/arm: ffa: refactor ffa_handle_call() Jens Wiklander
2024-04-10 7:04 ` Bertrand Marquis
2024-04-09 15:36 ` [XEN PATCH v1 2/5] xen/arm: ffa: use ACCESS_ONCE() Jens Wiklander
2024-04-10 7:05 ` Bertrand Marquis
2024-04-09 15:36 ` [XEN PATCH v1 3/5] xen/arm: ffa: simplify ffa_handle_mem_share() Jens Wiklander
2024-04-10 7:07 ` Bertrand Marquis
2024-04-09 15:36 ` [XEN PATCH v1 4/5] xen/arm: allow dynamically assigned SGI handlers Jens Wiklander
2024-04-10 13:24 ` Michal Orzel
2024-04-11 6:12 ` Jens Wiklander
2024-04-11 6:44 ` Michal Orzel
2024-04-11 7:05 ` Jens Wiklander
2024-04-09 15:36 ` [XEN PATCH v1 5/5] xen/arm: ffa: support notification Jens Wiklander
2024-04-10 7:49 ` Bertrand Marquis
2024-04-10 14:27 ` Jens Wiklander
2024-04-10 14:47 ` Bertrand Marquis
2024-04-10 15:41 ` Bertrand Marquis
2024-04-10 15:47 ` Jens Wiklander
2024-04-10 15:45 ` Jens Wiklander
2024-04-10 16:30 ` Bertrand Marquis
2024-04-11 5:43 ` Jens Wiklander
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.