* [XEN PATCH v3 0/5] FF-A notifications
@ 2024-04-26 8:47 Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 1/5] xen/arm: ffa: refactor ffa_handle_call() Jens Wiklander
` (5 more replies)
0 siblings, 6 replies; 22+ messages in thread
From: Jens Wiklander @ 2024-04-26 8:47 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
v2->v3:
- "xen/arm: ffa: support notification" and
"xen/arm: allow dynamically assigned SGI handlers" updated as requestsed,
details in each patch.
v1->v2:
- "xen/arm: ffa: support notification" and
"xen/arm: allow dynamically assigned SGI handlers" updated as requestsed,
details in each patch.
- Added Bertrands R-B for "xen/arm: ffa: refactor ffa_handle_call()",
"xen/arm: ffa: use ACCESS_ONCE()", and
"xen/arm: ffa: simplify ffa_handle_mem_share()"
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 | 12 +-
xen/arch/arm/include/asm/gic.h | 2 +-
xen/arch/arm/irq.c | 18 +-
xen/arch/arm/tee/Makefile | 1 +
xen/arch/arm/tee/ffa.c | 83 +++++--
xen/arch/arm/tee/ffa_notif.c | 378 ++++++++++++++++++++++++++++++++
xen/arch/arm/tee/ffa_partinfo.c | 9 +-
xen/arch/arm/tee/ffa_private.h | 56 ++++-
xen/arch/arm/tee/ffa_shm.c | 33 ++-
xen/include/public/arch-arm.h | 14 ++
10 files changed, 551 insertions(+), 55 deletions(-)
create mode 100644 xen/arch/arm/tee/ffa_notif.c
--
2.34.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [XEN PATCH v3 1/5] xen/arm: ffa: refactor ffa_handle_call()
2024-04-26 8:47 [XEN PATCH v3 0/5] FF-A notifications Jens Wiklander
@ 2024-04-26 8:47 ` Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 2/5] xen/arm: ffa: use ACCESS_ONCE() Jens Wiklander
` (4 subsequent siblings)
5 siblings, 0 replies; 22+ messages in thread
From: Jens Wiklander @ 2024-04-26 8:47 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>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
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] 22+ messages in thread
* [XEN PATCH v3 2/5] xen/arm: ffa: use ACCESS_ONCE()
2024-04-26 8:47 [XEN PATCH v3 0/5] FF-A notifications Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 1/5] xen/arm: ffa: refactor ffa_handle_call() Jens Wiklander
@ 2024-04-26 8:47 ` Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 3/5] xen/arm: ffa: simplify ffa_handle_mem_share() Jens Wiklander
` (3 subsequent siblings)
5 siblings, 0 replies; 22+ messages in thread
From: Jens Wiklander @ 2024-04-26 8:47 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>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
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] 22+ messages in thread
* [XEN PATCH v3 3/5] xen/arm: ffa: simplify ffa_handle_mem_share()
2024-04-26 8:47 [XEN PATCH v3 0/5] FF-A notifications Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 1/5] xen/arm: ffa: refactor ffa_handle_call() Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 2/5] xen/arm: ffa: use ACCESS_ONCE() Jens Wiklander
@ 2024-04-26 8:47 ` Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 4/5] xen/arm: allow dynamically assigned SGI handlers Jens Wiklander
` (2 subsequent siblings)
5 siblings, 0 replies; 22+ messages in thread
From: Jens Wiklander @ 2024-04-26 8:47 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>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
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] 22+ messages in thread
* [XEN PATCH v3 4/5] xen/arm: allow dynamically assigned SGI handlers
2024-04-26 8:47 [XEN PATCH v3 0/5] FF-A notifications Jens Wiklander
` (2 preceding siblings ...)
2024-04-26 8:47 ` [XEN PATCH v3 3/5] xen/arm: ffa: simplify ffa_handle_mem_share() Jens Wiklander
@ 2024-04-26 8:47 ` Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 5/5] xen/arm: ffa: support notification Jens Wiklander
2024-04-26 9:23 ` [XEN PATCH v3 0/5] FF-A notifications Bertrand Marquis
5 siblings, 0 replies; 22+ messages in thread
From: Jens Wiklander @ 2024-04-26 8:47 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. This prepares for a later patch where an FF-A schedule
receiver interrupt handler is installed for an SGI generated by the
secure world.
From the Arm Base System Architecture v1.0C [1]:
"The system shall implement at least eight Non-secure SGIs, assigned to
interrupt IDs 0-7."
gic_route_irq_to_xen() don't gic_set_irq_type() for SGIs since they are
always edge triggered.
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.
[1] https://developer.arm.com/documentation/den0094/
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
v2->v3
- Rename GIC_SGI_MAX to GIC_SGI_STATIC_MAX and rename do_sgi() to
do_static_sgi()
- Update comment in setup_irq() to mention that SGI irq_desc is banked
- Add ASSERT() in do_IRQ() that the irq isn't an SGI before injecting
calling vgic_inject_irq()
- Initialize local_irqs_type[] range for SGIs as IRQ_TYPE_EDGE_RISING
- Adding link to the Arm Base System Architecture v1.0C
v1->v2
- Update patch description as requested
---
xen/arch/arm/gic.c | 12 +++++++-----
xen/arch/arm/include/asm/gic.h | 2 +-
xen/arch/arm/irq.c | 18 ++++++++++++++----
3 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 44c40e86defe..882768252740 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -38,7 +38,7 @@ const struct gic_hw_operations *gic_hw_ops;
static void __init __maybe_unused build_assertions(void)
{
/* Check our enum gic_sgi only covers SGIs */
- BUILD_BUG_ON(GIC_SGI_MAX > NR_GIC_SGI);
+ BUILD_BUG_ON(GIC_SGI_STATIC_MAX > NR_GIC_SGI);
}
void register_gic_ops(const struct gic_hw_operations *ops)
@@ -117,7 +117,9 @@ 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);
+ /* SGIs are always edge-triggered, so there is need to set it */
+ if ( desc->irq >= NR_GIC_SGI)
+ gic_set_irq_type(desc, desc->arch.type);
gic_set_irq_priority(desc, priority);
}
@@ -330,7 +332,7 @@ void gic_disable_cpu(void)
gic_hw_ops->disable_interface();
}
-static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
+static void do_static_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
{
struct irq_desc *desc = irq_to_desc(sgi);
@@ -375,7 +377,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_STATIC_MAX && irq < 1020) )
{
isb();
do_IRQ(regs, irq, is_fiq);
@@ -387,7 +389,7 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
}
else if ( unlikely(irq < 16) )
{
- do_sgi(regs, irq);
+ do_static_sgi(regs, irq);
}
else
{
diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
index 03f209529b13..541f0eeb808a 100644
--- a/xen/arch/arm/include/asm/gic.h
+++ b/xen/arch/arm/include/asm/gic.h
@@ -285,7 +285,7 @@ enum gic_sgi {
GIC_SGI_EVENT_CHECK,
GIC_SGI_DUMP_STATE,
GIC_SGI_CALL_FUNCTION,
- GIC_SGI_MAX,
+ GIC_SGI_STATIC_MAX,
};
/* SGI irq mode types */
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index bcce80a4d624..d7306aa64d50 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -152,7 +152,13 @@ void __init init_IRQ(void)
spin_lock(&local_irqs_type_lock);
for ( irq = 0; irq < NR_LOCAL_IRQS; irq++ )
- local_irqs_type[irq] = IRQ_TYPE_INVALID;
+ {
+ /* SGIs are always edge-triggered */
+ if ( irq < NR_GIC_SGI )
+ local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
+ else
+ local_irqs_type[irq] = IRQ_TYPE_INVALID;
+ }
spin_unlock(&local_irqs_type_lock);
BUG_ON(init_local_irq_data(smp_processor_id()) < 0);
@@ -224,9 +230,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_STATIC_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);
@@ -260,6 +269,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
* The irq cannot be a PPI, we only support delivery of SPIs to
* guests.
*/
+ ASSERT(irq >= NR_GIC_SGI);
vgic_inject_irq(info->d, NULL, info->virq, true);
goto out_no_end;
}
@@ -396,7 +406,7 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
{
gic_route_irq_to_xen(desc, GIC_PRI_IRQ);
/* It's fine to use smp_processor_id() because:
- * For PPI: irq_desc is banked
+ * For SGI and PPI: irq_desc is banked
* For SPI: we don't care for now which CPU will receive the
* interrupt
* TODO: Handle case where SPI is setup on different CPU than
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-26 8:47 [XEN PATCH v3 0/5] FF-A notifications Jens Wiklander
` (3 preceding siblings ...)
2024-04-26 8:47 ` [XEN PATCH v3 4/5] xen/arm: allow dynamically assigned SGI handlers Jens Wiklander
@ 2024-04-26 8:47 ` Jens Wiklander
2024-04-26 9:20 ` Bertrand Marquis
` (2 more replies)
2024-04-26 9:23 ` [XEN PATCH v3 0/5] FF-A notifications Bertrand Marquis
5 siblings, 3 replies; 22+ messages in thread
From: Jens Wiklander @ 2024-04-26 8:47 UTC (permalink / raw)
To: xen-devel
Cc: patches, Jens Wiklander, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk
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.
Update ffa_partinfo_domain_init() to return error code like
ffa_notif_domain_init().
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
v2->v3:
- Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
- Register the Xen SRI handler on each CPU using on_selected_cpus() and
setup_irq()
- Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
doesn't conflict with static SGI handlers
v1->v2:
- Addressing review comments
- Change ffa_handle_notification_{bind,unbind,set}() to take struct
cpu_user_regs *regs as argument.
- Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
an error code.
- Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
---
xen/arch/arm/irq.c | 2 +-
xen/arch/arm/tee/Makefile | 1 +
xen/arch/arm/tee/ffa.c | 55 ++++-
xen/arch/arm/tee/ffa_notif.c | 378 ++++++++++++++++++++++++++++++++
xen/arch/arm/tee/ffa_partinfo.c | 9 +-
xen/arch/arm/tee/ffa_private.h | 56 ++++-
xen/include/public/arch-arm.h | 14 ++
7 files changed, 507 insertions(+), 8 deletions(-)
create mode 100644 xen/arch/arm/tee/ffa_notif.c
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index d7306aa64d50..5224898265a5 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -155,7 +155,7 @@ void __init init_IRQ(void)
{
/* SGIs are always edge-triggered */
if ( irq < NR_GIC_SGI )
- local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
+ local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
else
local_irqs_type[irq] = IRQ_TYPE_INVALID;
}
diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
index f0112a2f922d..7c0f46f7f446 100644
--- a/xen/arch/arm/tee/Makefile
+++ b/xen/arch/arm/tee/Makefile
@@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
obj-$(CONFIG_FFA) += ffa_shm.o
obj-$(CONFIG_FFA) += ffa_partinfo.o
obj-$(CONFIG_FFA) += ffa_rxtx.o
+obj-$(CONFIG_FFA) += ffa_notif.o
obj-y += tee.o
obj-$(CONFIG_OPTEE) += optee.o
diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
index 5209612963e1..912e905a27bd 100644
--- a/xen/arch/arm/tee/ffa.c
+++ b/xen/arch/arm/tee/ffa.c
@@ -39,6 +39,9 @@
* - at most 32 shared memory regions per guest
* o FFA_MSG_SEND_DIRECT_REQ:
* - only supported from a VM to an SP
+ * o FFA_NOTIFICATION_*:
+ * - only supports global notifications, that is, per vCPU notifications
+ * are not supported
*
* There are some large locked sections with ffa_tx_buffer_lock and
* ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
@@ -194,6 +197,8 @@ out:
static void handle_features(struct cpu_user_regs *regs)
{
+ struct domain *d = current->domain;
+ struct ffa_ctx *ctx = d->arch.tee;
uint32_t a1 = get_user_reg(regs, 1);
unsigned int n;
@@ -240,6 +245,30 @@ static void handle_features(struct cpu_user_regs *regs)
BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
ffa_set_regs_success(regs, 0, 0);
break;
+ case FFA_FEATURE_NOTIF_PEND_INTR:
+ if ( ctx->notif.enabled )
+ ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
+ else
+ ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+ break;
+ case FFA_FEATURE_SCHEDULE_RECV_INTR:
+ if ( ctx->notif.enabled )
+ ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
+ else
+ ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+ break;
+
+ case FFA_NOTIFICATION_BIND:
+ case FFA_NOTIFICATION_UNBIND:
+ case FFA_NOTIFICATION_GET:
+ case FFA_NOTIFICATION_SET:
+ case FFA_NOTIFICATION_INFO_GET_32:
+ case FFA_NOTIFICATION_INFO_GET_64:
+ if ( ctx->notif.enabled )
+ ffa_set_regs_success(regs, 0, 0);
+ else
+ ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+ break;
default:
ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
break;
@@ -305,6 +334,22 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
get_user_reg(regs, 1)),
get_user_reg(regs, 3));
break;
+ case FFA_NOTIFICATION_BIND:
+ e = ffa_handle_notification_bind(regs);
+ break;
+ case FFA_NOTIFICATION_UNBIND:
+ e = ffa_handle_notification_unbind(regs);
+ break;
+ case FFA_NOTIFICATION_INFO_GET_32:
+ case FFA_NOTIFICATION_INFO_GET_64:
+ ffa_handle_notification_info_get(regs);
+ return true;
+ case FFA_NOTIFICATION_GET:
+ ffa_handle_notification_get(regs);
+ return true;
+ case FFA_NOTIFICATION_SET:
+ e = ffa_handle_notification_set(regs);
+ break;
default:
gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
@@ -322,6 +367,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
static int ffa_domain_init(struct domain *d)
{
struct ffa_ctx *ctx;
+ int ret;
if ( !ffa_version )
return -ENODEV;
@@ -345,10 +391,11 @@ static int ffa_domain_init(struct domain *d)
* error, so no need for cleanup in this function.
*/
- if ( !ffa_partinfo_domain_init(d) )
- return -EIO;
+ ret = ffa_partinfo_domain_init(d);
+ if ( ret )
+ return ret;
- return 0;
+ return ffa_notif_domain_init(d);
}
static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
@@ -423,6 +470,7 @@ static int ffa_domain_teardown(struct domain *d)
return 0;
ffa_rxtx_domain_destroy(d);
+ ffa_notif_domain_destroy(d);
ffa_domain_teardown_continue(ctx, true /* first_time */);
@@ -502,6 +550,7 @@ static bool ffa_probe(void)
if ( !ffa_partinfo_init() )
goto err_rxtx_destroy;
+ ffa_notif_init();
INIT_LIST_HEAD(&ffa_teardown_head);
init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
new file mode 100644
index 000000000000..6bb0804ee2f8
--- /dev/null
+++ b/xen/arch/arm/tee/ffa_notif.c
@@ -0,0 +1,378 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024 Linaro Limited
+ */
+
+#include <xen/const.h>
+#include <xen/list.h>
+#include <xen/spinlock.h>
+#include <xen/types.h>
+
+#include <asm/smccc.h>
+#include <asm/regs.h>
+
+#include "ffa_private.h"
+
+static bool __ro_after_init notif_enabled;
+
+int ffa_handle_notification_bind(struct cpu_user_regs *regs)
+{
+ struct domain *d = current->domain;
+ uint32_t src_dst = get_user_reg(regs, 1);
+ uint32_t flags = get_user_reg(regs, 2);
+ uint32_t bitmap_lo = get_user_reg(regs, 3);
+ uint32_t bitmap_hi = get_user_reg(regs, 4);
+
+ if ( !notif_enabled )
+ return FFA_RET_NOT_SUPPORTED;
+
+ if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
+ return FFA_RET_INVALID_PARAMETERS;
+
+ if ( flags ) /* Only global notifications are supported */
+ return FFA_RET_DENIED;
+
+ /*
+ * We only support notifications from SP so no need to check the sender
+ * endpoint ID, the SPMC will take care of that for us.
+ */
+ return ffa_simple_call(FFA_NOTIFICATION_BIND, src_dst, flags, bitmap_hi,
+ bitmap_lo);
+}
+
+int ffa_handle_notification_unbind(struct cpu_user_regs *regs)
+{
+ struct domain *d = current->domain;
+ uint32_t src_dst = get_user_reg(regs, 1);
+ uint32_t bitmap_lo = get_user_reg(regs, 3);
+ uint32_t bitmap_hi = get_user_reg(regs, 4);
+
+ if ( !notif_enabled )
+ return FFA_RET_NOT_SUPPORTED;
+
+ if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
+ return FFA_RET_INVALID_PARAMETERS;
+
+ /*
+ * We only support notifications from SP so no need to check the
+ * destination endpoint ID, the SPMC will take care of that for us.
+ */
+ return ffa_simple_call(FFA_NOTIFICATION_UNBIND, src_dst, 0, bitmap_hi,
+ bitmap_lo);
+}
+
+void ffa_handle_notification_info_get(struct cpu_user_regs *regs)
+{
+ struct domain *d = current->domain;
+ struct ffa_ctx *ctx = d->arch.tee;
+ bool pending_global;
+
+ if ( !notif_enabled )
+ {
+ ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+ return;
+ }
+
+ spin_lock(&ctx->notif.lock);
+ pending_global = ctx->notif.secure_pending;
+ ctx->notif.secure_pending = false;
+ spin_unlock(&ctx->notif.lock);
+
+ if ( pending_global )
+ {
+ /* A pending global notification for the guest */
+ ffa_set_regs(regs, FFA_SUCCESS_64, 0,
+ 1U << FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT, ffa_get_vm_id(d),
+ 0, 0, 0, 0);
+ }
+ else
+ {
+ /* Report an error if there where no pending global notification */
+ ffa_set_regs_error(regs, FFA_RET_NO_DATA);
+ }
+}
+
+void ffa_handle_notification_get(struct cpu_user_regs *regs)
+{
+ struct domain *d = current->domain;
+ uint32_t recv = get_user_reg(regs, 1);
+ uint32_t flags = get_user_reg(regs, 2);
+ uint32_t w2 = 0;
+ uint32_t w3 = 0;
+ uint32_t w4 = 0;
+ uint32_t w5 = 0;
+ uint32_t w6 = 0;
+ uint32_t w7 = 0;
+
+ if ( !notif_enabled )
+ {
+ ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
+ return;
+ }
+
+ if ( (recv & 0xFFFFU) != ffa_get_vm_id(d) )
+ {
+ ffa_set_regs_error(regs, FFA_RET_INVALID_PARAMETERS);
+ return;
+ }
+
+ if ( flags & ( FFA_NOTIF_FLAG_BITMAP_SP | FFA_NOTIF_FLAG_BITMAP_SPM ) )
+ {
+ struct arm_smccc_1_2_regs arg = {
+ .a0 = FFA_NOTIFICATION_GET,
+ .a1 = recv,
+ .a2 = flags & ( FFA_NOTIF_FLAG_BITMAP_SP |
+ FFA_NOTIF_FLAG_BITMAP_SPM ),
+ };
+ struct arm_smccc_1_2_regs resp;
+ int32_t e;
+
+ arm_smccc_1_2_smc(&arg, &resp);
+ e = ffa_get_ret_code(&resp);
+ if ( e )
+ {
+ ffa_set_regs_error(regs, e);
+ return;
+ }
+
+ if ( flags & FFA_NOTIF_FLAG_BITMAP_SP )
+ {
+ w2 = resp.a2;
+ w3 = resp.a3;
+ }
+
+ if ( flags & FFA_NOTIF_FLAG_BITMAP_SPM )
+ w6 = resp.a6;
+ }
+
+ ffa_set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, w4, w5, w6, w7);
+}
+
+int ffa_handle_notification_set(struct cpu_user_regs *regs)
+{
+ struct domain *d = current->domain;
+ uint32_t src_dst = get_user_reg(regs, 1);
+ uint32_t flags = get_user_reg(regs, 2);
+ uint32_t bitmap_lo = get_user_reg(regs, 3);
+ uint32_t bitmap_hi = get_user_reg(regs, 4);
+
+ if ( !notif_enabled )
+ return FFA_RET_NOT_SUPPORTED;
+
+ if ( (src_dst >> 16) != ffa_get_vm_id(d) )
+ return FFA_RET_INVALID_PARAMETERS;
+
+ /* Let the SPMC check the destination of the notification */
+ return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags, bitmap_lo,
+ bitmap_hi);
+}
+
+/*
+ * Extract a 16-bit ID (index n) from the successful return value from
+ * FFA_NOTIFICATION_INFO_GET_64 or FFA_NOTIFICATION_INFO_GET_32. IDs are
+ * returned in registers 3 to 7 with four IDs per register for 64-bit
+ * calling convention and two IDs per register for 32-bit calling
+ * convention.
+ */
+static uint16_t get_id_from_resp(struct arm_smccc_1_2_regs *resp,
+ unsigned int n)
+{
+ unsigned int ids_per_reg;
+ unsigned int reg_idx;
+ unsigned int reg_shift;
+
+ if ( smccc_is_conv_64(resp->a0) )
+ ids_per_reg = 4;
+ else
+ ids_per_reg = 2;
+
+ reg_idx = n / ids_per_reg + 3;
+ reg_shift = ( n % ids_per_reg ) * 16;
+
+ switch ( reg_idx )
+ {
+ case 3:
+ return resp->a3 >> reg_shift;
+ case 4:
+ return resp->a4 >> reg_shift;
+ case 5:
+ return resp->a5 >> reg_shift;
+ case 6:
+ return resp->a6 >> reg_shift;
+ case 7:
+ return resp->a7 >> reg_shift;
+ default:
+ ASSERT(0); /* "Can't happen" */
+ return 0;
+ }
+}
+
+static void notif_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], GUEST_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);
+}
+
+struct notif_irq_info {
+ unsigned int irq;
+ int ret;
+ struct irqaction *action;
+};
+
+static void notif_irq_enable(void *info)
+{
+ struct notif_irq_info *irq_info = info;
+
+ irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
+ if ( irq_info->ret )
+ printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
+ irq_info->irq, irq_info->ret);
+}
+
+void ffa_notif_init(void)
+{
+ const struct arm_smccc_1_2_regs arg = {
+ .a0 = FFA_FEATURES,
+ .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
+ };
+ struct notif_irq_info irq_info = { };
+ struct arm_smccc_1_2_regs resp;
+ unsigned int cpu;
+
+ arm_smccc_1_2_smc(&arg, &resp);
+ if ( resp.a0 != FFA_SUCCESS_32 )
+ return;
+
+ irq_info.irq = resp.a2;
+ if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
+ {
+ printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
+ irq_info.irq);
+ return;
+ }
+
+ /*
+ * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
+ * IPI to call notif_irq_enable() on each CPU including the current
+ * CPU. The struct irqaction is preallocated since we can't allocate
+ * memory while in interrupt context.
+ */
+ for_each_online_cpu(cpu)
+ {
+ irq_info.action = xmalloc(struct irqaction);
+ if ( !irq_info.action )
+ {
+ irq_info.ret = -ENOMEM;
+ break;
+ }
+
+ *irq_info.action = (struct irqaction){
+ .handler = notif_irq_handler,
+ .name = "FF-A notif",
+ .free_on_release = 1,
+ };
+
+ on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
+ if ( irq_info.ret )
+ {
+ XFREE(irq_info.action);
+ break;
+ }
+ }
+
+ notif_enabled = !irq_info.ret;
+}
+
+int ffa_notif_domain_init(struct domain *d)
+{
+ struct ffa_ctx *ctx = d->arch.tee;
+ int32_t res;
+
+ if ( !notif_enabled )
+ return 0;
+
+ res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
+ if ( res )
+ return -ENOMEM;
+
+ ctx->notif.enabled = true;
+
+ return 0;
+}
+
+void ffa_notif_domain_destroy(struct domain *d)
+{
+ struct ffa_ctx *ctx = d->arch.tee;
+
+ if ( ctx->notif.enabled )
+ {
+ ffa_notification_bitmap_destroy(ffa_get_vm_id(d));
+ ctx->notif.enabled = false;
+ }
+}
diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
index dc1059584828..93a03c6bc672 100644
--- a/xen/arch/arm/tee/ffa_partinfo.c
+++ b/xen/arch/arm/tee/ffa_partinfo.c
@@ -306,7 +306,7 @@ static void vm_destroy_bitmap_init(struct ffa_ctx *ctx,
}
}
-bool ffa_partinfo_domain_init(struct domain *d)
+int ffa_partinfo_domain_init(struct domain *d)
{
unsigned int count = BITS_TO_LONGS(subscr_vm_destroyed_count);
struct ffa_ctx *ctx = d->arch.tee;
@@ -315,7 +315,7 @@ bool ffa_partinfo_domain_init(struct domain *d)
ctx->vm_destroy_bitmap = xzalloc_array(unsigned long, count);
if ( !ctx->vm_destroy_bitmap )
- return false;
+ return -ENOMEM;
for ( n = 0; n < subscr_vm_created_count; n++ )
{
@@ -330,7 +330,10 @@ bool ffa_partinfo_domain_init(struct domain *d)
}
vm_destroy_bitmap_init(ctx, n);
- return n == subscr_vm_created_count;
+ if ( n != subscr_vm_created_count )
+ return -EIO;
+
+ return 0;
}
bool ffa_partinfo_domain_destroy(struct domain *d)
diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
index 98236cbf14a3..a59c9887774b 100644
--- a/xen/arch/arm/tee/ffa_private.h
+++ b/xen/arch/arm/tee/ffa_private.h
@@ -25,6 +25,7 @@
#define FFA_RET_DENIED -6
#define FFA_RET_RETRY -7
#define FFA_RET_ABORTED -8
+#define FFA_RET_NO_DATA -9
/* FFA_VERSION helpers */
#define FFA_VERSION_MAJOR_SHIFT 16U
@@ -175,6 +176,21 @@
*/
#define FFA_PARTITION_INFO_GET_COUNT_FLAG BIT(0, U)
+/* Flags used in calls to FFA_NOTIFICATION_GET interface */
+#define FFA_NOTIF_FLAG_BITMAP_SP BIT(0, U)
+#define FFA_NOTIF_FLAG_BITMAP_VM BIT(1, U)
+#define FFA_NOTIF_FLAG_BITMAP_SPM BIT(2, U)
+#define FFA_NOTIF_FLAG_BITMAP_HYP BIT(3, U)
+
+#define FFA_NOTIF_INFO_GET_MORE_FLAG BIT(0, U)
+#define FFA_NOTIF_INFO_GET_ID_LIST_SHIFT 12
+#define FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT 7
+#define FFA_NOTIF_INFO_GET_ID_COUNT_MASK 0x1F
+
+/* Feature IDs used with FFA_FEATURES */
+#define FFA_FEATURE_NOTIF_PEND_INTR 0x1U
+#define FFA_FEATURE_SCHEDULE_RECV_INTR 0x2U
+
/* Function IDs */
#define FFA_ERROR 0x84000060U
#define FFA_SUCCESS_32 0x84000061U
@@ -213,6 +229,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 +265,7 @@ struct ffa_ctx {
struct list_head shm_list;
/* Number of allocated shared memory object */
unsigned int shm_count;
+ struct ffa_ctx_notif notif;
/*
* tx_lock is used to serialize access to tx
* rx_lock is used to serialize access to rx
@@ -257,7 +295,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs);
int ffa_handle_mem_reclaim(uint64_t handle, uint32_t flags);
bool ffa_partinfo_init(void);
-bool ffa_partinfo_domain_init(struct domain *d);
+int ffa_partinfo_domain_init(struct domain *d);
bool ffa_partinfo_domain_destroy(struct domain *d);
int32_t ffa_handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
uint32_t w4, uint32_t w5, uint32_t *count,
@@ -271,12 +309,28 @@ uint32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
uint32_t ffa_handle_rxtx_unmap(void);
int32_t ffa_handle_rx_release(void);
+void ffa_notif_init(void);
+int ffa_notif_domain_init(struct domain *d);
+void ffa_notif_domain_destroy(struct domain *d);
+
+int ffa_handle_notification_bind(struct cpu_user_regs *regs);
+int ffa_handle_notification_unbind(struct cpu_user_regs *regs);
+void ffa_handle_notification_info_get(struct cpu_user_regs *regs);
+void ffa_handle_notification_get(struct cpu_user_regs *regs);
+int ffa_handle_notification_set(struct cpu_user_regs *regs);
+
static inline uint16_t ffa_get_vm_id(const struct domain *d)
{
/* +1 since 0 is reserved for the hypervisor in FF-A */
return d->domain_id + 1;
}
+static inline struct domain *ffa_get_domain_by_vm_id(uint16_t vm_id)
+{
+ /* -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,
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index e167e14f8df9..1eac9802aa53 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -505,6 +505,7 @@ typedef uint64_t xen_callback_t;
#define GUEST_MAX_VCPUS 128
/* Interrupts */
+
#define GUEST_TIMER_VIRT_PPI 27
#define GUEST_TIMER_PHYS_S_PPI 29
#define GUEST_TIMER_PHYS_NS_PPI 30
@@ -515,6 +516,19 @@ typedef uint64_t xen_callback_t;
#define GUEST_VIRTIO_MMIO_SPI_FIRST 33
#define GUEST_VIRTIO_MMIO_SPI_LAST 43
+/*
+ * SGI is the preferred delivery mechanism of FF-A pending notifications or
+ * schedule recveive interrupt. SGIs 8-15 are normally not used by a guest
+ * as they in a non-virtualized system typically are assigned to the secure
+ * world. Here we're free to use SGI 8-15 since they are virtual and have
+ * nothing to do with the secure world.
+ *
+ * For partitioning of SGIs see also Arm Base System Architecture v1.0C,
+ * https://developer.arm.com/documentation/den0094/
+ */
+#define GUEST_FFA_NOTIF_PEND_INTR_ID 8
+#define GUEST_FFA_SCHEDULE_RECV_INTR_ID 9
+
/* PSCI functions */
#define PSCI_cpu_suspend 0
#define PSCI_cpu_off 1
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-26 8:47 ` [XEN PATCH v3 5/5] xen/arm: ffa: support notification Jens Wiklander
@ 2024-04-26 9:20 ` Bertrand Marquis
2024-04-26 12:11 ` Jens Wiklander
2024-04-26 18:31 ` Julien Grall
2024-04-26 17:58 ` Julien Grall
2024-04-26 19:07 ` Julien Grall
2 siblings, 2 replies; 22+ messages in thread
From: Bertrand Marquis @ 2024-04-26 9:20 UTC (permalink / raw)
To: Jens Wiklander
Cc: Xen-devel, patches, Stefano Stabellini, Julien Grall,
Michal Orzel, Volodymyr Babchuk
Hi Jens,
> On 26 Apr 2024, at 10:47, 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.
>
> Update ffa_partinfo_domain_init() to return error code like
> ffa_notif_domain_init().
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
> v2->v3:
> - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
> FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
> - Register the Xen SRI handler on each CPU using on_selected_cpus() and
> setup_irq()
> - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
> doesn't conflict with static SGI handlers
>
> v1->v2:
> - Addressing review comments
> - Change ffa_handle_notification_{bind,unbind,set}() to take struct
> cpu_user_regs *regs as argument.
> - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
> an error code.
> - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
> ---
> xen/arch/arm/irq.c | 2 +-
> xen/arch/arm/tee/Makefile | 1 +
> xen/arch/arm/tee/ffa.c | 55 ++++-
> xen/arch/arm/tee/ffa_notif.c | 378 ++++++++++++++++++++++++++++++++
> xen/arch/arm/tee/ffa_partinfo.c | 9 +-
> xen/arch/arm/tee/ffa_private.h | 56 ++++-
> xen/include/public/arch-arm.h | 14 ++
> 7 files changed, 507 insertions(+), 8 deletions(-)
> create mode 100644 xen/arch/arm/tee/ffa_notif.c
>
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index d7306aa64d50..5224898265a5 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -155,7 +155,7 @@ void __init init_IRQ(void)
> {
> /* SGIs are always edge-triggered */
> if ( irq < NR_GIC_SGI )
> - local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
> + local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
> else
> local_irqs_type[irq] = IRQ_TYPE_INVALID;
> }
> diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
> index f0112a2f922d..7c0f46f7f446 100644
> --- a/xen/arch/arm/tee/Makefile
> +++ b/xen/arch/arm/tee/Makefile
> @@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
> obj-$(CONFIG_FFA) += ffa_shm.o
> obj-$(CONFIG_FFA) += ffa_partinfo.o
> obj-$(CONFIG_FFA) += ffa_rxtx.o
> +obj-$(CONFIG_FFA) += ffa_notif.o
> obj-y += tee.o
> obj-$(CONFIG_OPTEE) += optee.o
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 5209612963e1..912e905a27bd 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -39,6 +39,9 @@
> * - at most 32 shared memory regions per guest
> * o FFA_MSG_SEND_DIRECT_REQ:
> * - only supported from a VM to an SP
> + * o FFA_NOTIFICATION_*:
> + * - only supports global notifications, that is, per vCPU notifications
> + * are not supported
> *
> * There are some large locked sections with ffa_tx_buffer_lock and
> * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
> @@ -194,6 +197,8 @@ out:
>
> static void handle_features(struct cpu_user_regs *regs)
> {
> + struct domain *d = current->domain;
> + struct ffa_ctx *ctx = d->arch.tee;
> uint32_t a1 = get_user_reg(regs, 1);
> unsigned int n;
>
> @@ -240,6 +245,30 @@ static void handle_features(struct cpu_user_regs *regs)
> BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
> ffa_set_regs_success(regs, 0, 0);
> break;
> + case FFA_FEATURE_NOTIF_PEND_INTR:
> + if ( ctx->notif.enabled )
> + ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
> + else
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + break;
> + case FFA_FEATURE_SCHEDULE_RECV_INTR:
> + if ( ctx->notif.enabled )
> + ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
> + else
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + break;
> +
> + case FFA_NOTIFICATION_BIND:
> + case FFA_NOTIFICATION_UNBIND:
> + case FFA_NOTIFICATION_GET:
> + case FFA_NOTIFICATION_SET:
> + case FFA_NOTIFICATION_INFO_GET_32:
> + case FFA_NOTIFICATION_INFO_GET_64:
> + if ( ctx->notif.enabled )
> + ffa_set_regs_success(regs, 0, 0);
> + else
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + break;
> default:
> ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> break;
> @@ -305,6 +334,22 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> get_user_reg(regs, 1)),
> get_user_reg(regs, 3));
> break;
> + case FFA_NOTIFICATION_BIND:
> + e = ffa_handle_notification_bind(regs);
> + break;
> + case FFA_NOTIFICATION_UNBIND:
> + e = ffa_handle_notification_unbind(regs);
> + break;
> + case FFA_NOTIFICATION_INFO_GET_32:
> + case FFA_NOTIFICATION_INFO_GET_64:
> + ffa_handle_notification_info_get(regs);
> + return true;
> + case FFA_NOTIFICATION_GET:
> + ffa_handle_notification_get(regs);
> + return true;
> + case FFA_NOTIFICATION_SET:
> + e = ffa_handle_notification_set(regs);
> + break;
>
> default:
> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> @@ -322,6 +367,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> static int ffa_domain_init(struct domain *d)
> {
> struct ffa_ctx *ctx;
> + int ret;
>
> if ( !ffa_version )
> return -ENODEV;
> @@ -345,10 +391,11 @@ static int ffa_domain_init(struct domain *d)
> * error, so no need for cleanup in this function.
> */
>
> - if ( !ffa_partinfo_domain_init(d) )
> - return -EIO;
> + ret = ffa_partinfo_domain_init(d);
> + if ( ret )
> + return ret;
>
> - return 0;
> + return ffa_notif_domain_init(d);
> }
>
> static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
> @@ -423,6 +470,7 @@ static int ffa_domain_teardown(struct domain *d)
> return 0;
>
> ffa_rxtx_domain_destroy(d);
> + ffa_notif_domain_destroy(d);
>
> ffa_domain_teardown_continue(ctx, true /* first_time */);
>
> @@ -502,6 +550,7 @@ static bool ffa_probe(void)
> if ( !ffa_partinfo_init() )
> goto err_rxtx_destroy;
>
> + ffa_notif_init();
> INIT_LIST_HEAD(&ffa_teardown_head);
> init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
>
> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
> new file mode 100644
> index 000000000000..6bb0804ee2f8
> --- /dev/null
> +++ b/xen/arch/arm/tee/ffa_notif.c
> @@ -0,0 +1,378 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 Linaro Limited
> + */
> +
> +#include <xen/const.h>
> +#include <xen/list.h>
> +#include <xen/spinlock.h>
> +#include <xen/types.h>
> +
> +#include <asm/smccc.h>
> +#include <asm/regs.h>
> +
> +#include "ffa_private.h"
> +
> +static bool __ro_after_init notif_enabled;
> +
> +int ffa_handle_notification_bind(struct cpu_user_regs *regs)
> +{
> + struct domain *d = current->domain;
> + uint32_t src_dst = get_user_reg(regs, 1);
> + uint32_t flags = get_user_reg(regs, 2);
> + uint32_t bitmap_lo = get_user_reg(regs, 3);
> + uint32_t bitmap_hi = get_user_reg(regs, 4);
> +
> + if ( !notif_enabled )
> + return FFA_RET_NOT_SUPPORTED;
> +
> + if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> + return FFA_RET_INVALID_PARAMETERS;
> +
> + if ( flags ) /* Only global notifications are supported */
> + return FFA_RET_DENIED;
> +
> + /*
> + * We only support notifications from SP so no need to check the sender
> + * endpoint ID, the SPMC will take care of that for us.
> + */
> + return ffa_simple_call(FFA_NOTIFICATION_BIND, src_dst, flags, bitmap_hi,
> + bitmap_lo);
> +}
> +
> +int ffa_handle_notification_unbind(struct cpu_user_regs *regs)
> +{
> + struct domain *d = current->domain;
> + uint32_t src_dst = get_user_reg(regs, 1);
> + uint32_t bitmap_lo = get_user_reg(regs, 3);
> + uint32_t bitmap_hi = get_user_reg(regs, 4);
> +
> + if ( !notif_enabled )
> + return FFA_RET_NOT_SUPPORTED;
> +
> + if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> + return FFA_RET_INVALID_PARAMETERS;
> +
> + /*
> + * We only support notifications from SP so no need to check the
> + * destination endpoint ID, the SPMC will take care of that for us.
> + */
> + return ffa_simple_call(FFA_NOTIFICATION_UNBIND, src_dst, 0, bitmap_hi,
> + bitmap_lo);
> +}
> +
> +void ffa_handle_notification_info_get(struct cpu_user_regs *regs)
> +{
> + struct domain *d = current->domain;
> + struct ffa_ctx *ctx = d->arch.tee;
> + bool pending_global;
> +
> + if ( !notif_enabled )
> + {
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + return;
> + }
> +
> + spin_lock(&ctx->notif.lock);
> + pending_global = ctx->notif.secure_pending;
> + ctx->notif.secure_pending = false;
> + spin_unlock(&ctx->notif.lock);
> +
> + if ( pending_global )
> + {
> + /* A pending global notification for the guest */
> + ffa_set_regs(regs, FFA_SUCCESS_64, 0,
> + 1U << FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT, ffa_get_vm_id(d),
> + 0, 0, 0, 0);
> + }
> + else
> + {
> + /* Report an error if there where no pending global notification */
> + ffa_set_regs_error(regs, FFA_RET_NO_DATA);
> + }
> +}
> +
> +void ffa_handle_notification_get(struct cpu_user_regs *regs)
> +{
> + struct domain *d = current->domain;
> + uint32_t recv = get_user_reg(regs, 1);
> + uint32_t flags = get_user_reg(regs, 2);
> + uint32_t w2 = 0;
> + uint32_t w3 = 0;
> + uint32_t w4 = 0;
> + uint32_t w5 = 0;
> + uint32_t w6 = 0;
> + uint32_t w7 = 0;
> +
> + if ( !notif_enabled )
> + {
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + return;
> + }
> +
> + if ( (recv & 0xFFFFU) != ffa_get_vm_id(d) )
> + {
> + ffa_set_regs_error(regs, FFA_RET_INVALID_PARAMETERS);
> + return;
> + }
> +
> + if ( flags & ( FFA_NOTIF_FLAG_BITMAP_SP | FFA_NOTIF_FLAG_BITMAP_SPM ) )
> + {
> + struct arm_smccc_1_2_regs arg = {
> + .a0 = FFA_NOTIFICATION_GET,
> + .a1 = recv,
> + .a2 = flags & ( FFA_NOTIF_FLAG_BITMAP_SP |
> + FFA_NOTIF_FLAG_BITMAP_SPM ),
> + };
> + struct arm_smccc_1_2_regs resp;
> + int32_t e;
> +
> + 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;
> + }
In here you never clean the secure_pending flag but in practice there would be
no more pending notification if SP and SPM flags are set so the response to
notification_info_get would wrongly say there is something pending.
I am not completely sure how this could be handled if both SP and SPM are
not set so i would say for now we should at least put a comment in info_get
to keep a note of this fact.
Do you agree ?
> +
> + ffa_set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, w4, w5, w6, w7);
> +}
> +
> +int ffa_handle_notification_set(struct cpu_user_regs *regs)
> +{
> + struct domain *d = current->domain;
> + uint32_t src_dst = get_user_reg(regs, 1);
> + uint32_t flags = get_user_reg(regs, 2);
> + uint32_t bitmap_lo = get_user_reg(regs, 3);
> + uint32_t bitmap_hi = get_user_reg(regs, 4);
> +
> + if ( !notif_enabled )
> + return FFA_RET_NOT_SUPPORTED;
> +
> + if ( (src_dst >> 16) != ffa_get_vm_id(d) )
> + return FFA_RET_INVALID_PARAMETERS;
> +
> + /* Let the SPMC check the destination of the notification */
> + return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags, bitmap_lo,
> + bitmap_hi);
> +}
> +
> +/*
> + * Extract a 16-bit ID (index n) from the successful return value from
> + * FFA_NOTIFICATION_INFO_GET_64 or FFA_NOTIFICATION_INFO_GET_32. IDs are
> + * returned in registers 3 to 7 with four IDs per register for 64-bit
> + * calling convention and two IDs per register for 32-bit calling
> + * convention.
> + */
> +static uint16_t get_id_from_resp(struct arm_smccc_1_2_regs *resp,
> + unsigned int n)
> +{
> + unsigned int ids_per_reg;
> + unsigned int reg_idx;
> + unsigned int reg_shift;
> +
> + if ( smccc_is_conv_64(resp->a0) )
> + ids_per_reg = 4;
> + else
> + ids_per_reg = 2;
> +
> + reg_idx = n / ids_per_reg + 3;
> + reg_shift = ( n % ids_per_reg ) * 16;
> +
> + switch ( reg_idx )
> + {
> + case 3:
> + return resp->a3 >> reg_shift;
> + case 4:
> + return resp->a4 >> reg_shift;
> + case 5:
> + return resp->a5 >> reg_shift;
> + case 6:
> + return resp->a6 >> reg_shift;
> + case 7:
> + return resp->a7 >> reg_shift;
> + default:
> + ASSERT(0); /* "Can't happen" */
> + return 0;
> + }
> +}
> +
> +static void notif_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;
> +
If a notification is pending for a secure partition at this stage we are not
signaling anything so I think this fact should be listed in the limitations to
say that we do not signal any secondary scheduler if a notification is
pending for a secure partition.
> + 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], GUEST_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);
> +}
> +
> +struct notif_irq_info {
> + unsigned int irq;
> + int ret;
> + struct irqaction *action;
> +};
> +
> +static void notif_irq_enable(void *info)
> +{
> + struct notif_irq_info *irq_info = info;
> +
> + irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
> + if ( irq_info->ret )
> + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> + irq_info->irq, irq_info->ret);
> +}
> +
> +void ffa_notif_init(void)
> +{
> + const struct arm_smccc_1_2_regs arg = {
> + .a0 = FFA_FEATURES,
> + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> + };
> + struct notif_irq_info irq_info = { };
> + struct arm_smccc_1_2_regs resp;
> + unsigned int cpu;
> +
> + arm_smccc_1_2_smc(&arg, &resp);
> + if ( resp.a0 != FFA_SUCCESS_32 )
> + return;
> +
> + irq_info.irq = resp.a2;
> + if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
> + {
> + printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
> + irq_info.irq);
> + return;
> + }
> +
> + /*
> + * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> + * IPI to call notif_irq_enable() on each CPU including the current
> + * CPU. The struct irqaction is preallocated since we can't allocate
> + * memory while in interrupt context.
> + */
> + for_each_online_cpu(cpu)
> + {
> + irq_info.action = xmalloc(struct irqaction);
You allocate one action per cpu but you have only one action pointer in your structure
which means you will overload the previously allocated one and lose track.
You should have a table of actions in your structure instead unless one action is
enough and can be reused on all cpus and in this case you should move out of
your loop the allocation part.
> + if ( !irq_info.action )
> + {
> + irq_info.ret = -ENOMEM;
> + break;
> + }
> +
> + *irq_info.action = (struct irqaction){
> + .handler = notif_irq_handler,
> + .name = "FF-A notif",
> + .free_on_release = 1,
> + };
> +
> + on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
> + if ( irq_info.ret )
> + {
> + XFREE(irq_info.action);
> + break;
> + }
> + }
> +
> + notif_enabled = !irq_info.ret;
> +}
> +
> +int ffa_notif_domain_init(struct domain *d)
> +{
> + struct ffa_ctx *ctx = d->arch.tee;
> + int32_t res;
> +
> + if ( !notif_enabled )
> + return 0;
> +
> + res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
> + if ( res )
> + return -ENOMEM;
> +
> + ctx->notif.enabled = true;
> +
> + return 0;
> +}
> +
> +void ffa_notif_domain_destroy(struct domain *d)
> +{
> + struct ffa_ctx *ctx = d->arch.tee;
> +
> + if ( ctx->notif.enabled )
> + {
> + ffa_notification_bitmap_destroy(ffa_get_vm_id(d));
> + ctx->notif.enabled = false;
> + }
> +}
> diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
> index dc1059584828..93a03c6bc672 100644
> --- a/xen/arch/arm/tee/ffa_partinfo.c
> +++ b/xen/arch/arm/tee/ffa_partinfo.c
> @@ -306,7 +306,7 @@ static void vm_destroy_bitmap_init(struct ffa_ctx *ctx,
> }
> }
>
> -bool ffa_partinfo_domain_init(struct domain *d)
> +int ffa_partinfo_domain_init(struct domain *d)
> {
> unsigned int count = BITS_TO_LONGS(subscr_vm_destroyed_count);
> struct ffa_ctx *ctx = d->arch.tee;
> @@ -315,7 +315,7 @@ bool ffa_partinfo_domain_init(struct domain *d)
>
> ctx->vm_destroy_bitmap = xzalloc_array(unsigned long, count);
> if ( !ctx->vm_destroy_bitmap )
> - return false;
> + return -ENOMEM;
>
> for ( n = 0; n < subscr_vm_created_count; n++ )
> {
> @@ -330,7 +330,10 @@ bool ffa_partinfo_domain_init(struct domain *d)
> }
> vm_destroy_bitmap_init(ctx, n);
>
> - return n == subscr_vm_created_count;
> + if ( n != subscr_vm_created_count )
> + return -EIO;
> +
> + return 0;
> }
>
> bool ffa_partinfo_domain_destroy(struct domain *d)
> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
> index 98236cbf14a3..a59c9887774b 100644
> --- a/xen/arch/arm/tee/ffa_private.h
> +++ b/xen/arch/arm/tee/ffa_private.h
> @@ -25,6 +25,7 @@
> #define FFA_RET_DENIED -6
> #define FFA_RET_RETRY -7
> #define FFA_RET_ABORTED -8
> +#define FFA_RET_NO_DATA -9
>
> /* FFA_VERSION helpers */
> #define FFA_VERSION_MAJOR_SHIFT 16U
> @@ -175,6 +176,21 @@
> */
> #define FFA_PARTITION_INFO_GET_COUNT_FLAG BIT(0, U)
>
> +/* Flags used in calls to FFA_NOTIFICATION_GET interface */
> +#define FFA_NOTIF_FLAG_BITMAP_SP BIT(0, U)
> +#define FFA_NOTIF_FLAG_BITMAP_VM BIT(1, U)
> +#define FFA_NOTIF_FLAG_BITMAP_SPM BIT(2, U)
> +#define FFA_NOTIF_FLAG_BITMAP_HYP BIT(3, U)
> +
> +#define FFA_NOTIF_INFO_GET_MORE_FLAG BIT(0, U)
> +#define FFA_NOTIF_INFO_GET_ID_LIST_SHIFT 12
> +#define FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT 7
> +#define FFA_NOTIF_INFO_GET_ID_COUNT_MASK 0x1F
> +
> +/* Feature IDs used with FFA_FEATURES */
> +#define FFA_FEATURE_NOTIF_PEND_INTR 0x1U
> +#define FFA_FEATURE_SCHEDULE_RECV_INTR 0x2U
> +
> /* Function IDs */
> #define FFA_ERROR 0x84000060U
> #define FFA_SUCCESS_32 0x84000061U
> @@ -213,6 +229,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 +265,7 @@ struct ffa_ctx {
> struct list_head shm_list;
> /* Number of allocated shared memory object */
> unsigned int shm_count;
> + struct ffa_ctx_notif notif;
> /*
> * tx_lock is used to serialize access to tx
> * rx_lock is used to serialize access to rx
> @@ -257,7 +295,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs);
> int ffa_handle_mem_reclaim(uint64_t handle, uint32_t flags);
>
> bool ffa_partinfo_init(void);
> -bool ffa_partinfo_domain_init(struct domain *d);
> +int ffa_partinfo_domain_init(struct domain *d);
> bool ffa_partinfo_domain_destroy(struct domain *d);
> int32_t ffa_handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
> uint32_t w4, uint32_t w5, uint32_t *count,
> @@ -271,12 +309,28 @@ uint32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
> uint32_t ffa_handle_rxtx_unmap(void);
> int32_t ffa_handle_rx_release(void);
>
> +void ffa_notif_init(void);
> +int ffa_notif_domain_init(struct domain *d);
> +void ffa_notif_domain_destroy(struct domain *d);
> +
> +int ffa_handle_notification_bind(struct cpu_user_regs *regs);
> +int ffa_handle_notification_unbind(struct cpu_user_regs *regs);
> +void ffa_handle_notification_info_get(struct cpu_user_regs *regs);
> +void ffa_handle_notification_get(struct cpu_user_regs *regs);
> +int ffa_handle_notification_set(struct cpu_user_regs *regs);
> +
> static inline uint16_t ffa_get_vm_id(const struct domain *d)
> {
> /* +1 since 0 is reserved for the hypervisor in FF-A */
> return d->domain_id + 1;
> }
>
> +static inline struct domain *ffa_get_domain_by_vm_id(uint16_t vm_id)
> +{
> + /* -1 to match ffa_get_vm_id() */
> + return get_domain_by_id(vm_id - 1);
> +}
> +
I think there is a global discussion to have on using get_domain_by_vm_id
or rcu_lock_live_remote_domain_by_id to make sure the domain is not
dying when we try to do something with it.
It does not need to be done here as there are other places to adapt but
i wanted to raise the question.
I would like to know what you and also other maintainers think here.
@Julien/Michal/Stefano ?
> static inline void ffa_set_regs(struct cpu_user_regs *regs, register_t v0,
> register_t v1, register_t v2, register_t v3,
> register_t v4, register_t v5, register_t v6,
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index e167e14f8df9..1eac9802aa53 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -505,6 +505,7 @@ typedef uint64_t xen_callback_t;
> #define GUEST_MAX_VCPUS 128
>
> /* Interrupts */
> +
> #define GUEST_TIMER_VIRT_PPI 27
> #define GUEST_TIMER_PHYS_S_PPI 29
> #define GUEST_TIMER_PHYS_NS_PPI 30
> @@ -515,6 +516,19 @@ typedef uint64_t xen_callback_t;
> #define GUEST_VIRTIO_MMIO_SPI_FIRST 33
> #define GUEST_VIRTIO_MMIO_SPI_LAST 43
>
> +/*
> + * SGI is the preferred delivery mechanism of FF-A pending notifications or
> + * schedule recveive interrupt. SGIs 8-15 are normally not used by a guest
> + * as they in a non-virtualized system typically are assigned to the secure
> + * world. Here we're free to use SGI 8-15 since they are virtual and have
> + * nothing to do with the secure world.
> + *
> + * For partitioning of SGIs see also Arm Base System Architecture v1.0C,
> + * https://developer.arm.com/documentation/den0094/
> + */
> +#define GUEST_FFA_NOTIF_PEND_INTR_ID 8
> +#define GUEST_FFA_SCHEDULE_RECV_INTR_ID 9
> +
> /* PSCI functions */
> #define PSCI_cpu_suspend 0
> #define PSCI_cpu_off 1
> --
> 2.34.1
>
Cheers
Bertrand
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 0/5] FF-A notifications
2024-04-26 8:47 [XEN PATCH v3 0/5] FF-A notifications Jens Wiklander
` (4 preceding siblings ...)
2024-04-26 8:47 ` [XEN PATCH v3 5/5] xen/arm: ffa: support notification Jens Wiklander
@ 2024-04-26 9:23 ` Bertrand Marquis
5 siblings, 0 replies; 22+ messages in thread
From: Bertrand Marquis @ 2024-04-26 9:23 UTC (permalink / raw)
To: Jens Wiklander
Cc: Xen-devel, patches, Volodymyr Babchuk, Stefano Stabellini,
Julien Grall, Michal Orzel
Hi Jens
> On 26 Apr 2024, at 10:47, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> 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.
From my point of view:
- patches 1 to 3 are ready to be commited.
- patch 4 will need a R-b from an other maintainer.
- patch 5 has still some stuff to be checked or fixed but could be
handled as a single patch if the rest or the serie is merged.
Regards
Bertrand
>
> Thanks,
> Jens
>
> v2->v3:
> - "xen/arm: ffa: support notification" and
> "xen/arm: allow dynamically assigned SGI handlers" updated as requestsed,
> details in each patch.
>
> v1->v2:
> - "xen/arm: ffa: support notification" and
> "xen/arm: allow dynamically assigned SGI handlers" updated as requestsed,
> details in each patch.
> - Added Bertrands R-B for "xen/arm: ffa: refactor ffa_handle_call()",
> "xen/arm: ffa: use ACCESS_ONCE()", and
> "xen/arm: ffa: simplify ffa_handle_mem_share()"
>
> 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 | 12 +-
> xen/arch/arm/include/asm/gic.h | 2 +-
> xen/arch/arm/irq.c | 18 +-
> xen/arch/arm/tee/Makefile | 1 +
> xen/arch/arm/tee/ffa.c | 83 +++++--
> xen/arch/arm/tee/ffa_notif.c | 378 ++++++++++++++++++++++++++++++++
> xen/arch/arm/tee/ffa_partinfo.c | 9 +-
> xen/arch/arm/tee/ffa_private.h | 56 ++++-
> xen/arch/arm/tee/ffa_shm.c | 33 ++-
> xen/include/public/arch-arm.h | 14 ++
> 10 files changed, 551 insertions(+), 55 deletions(-)
> create mode 100644 xen/arch/arm/tee/ffa_notif.c
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-26 9:20 ` Bertrand Marquis
@ 2024-04-26 12:11 ` Jens Wiklander
2024-04-26 12:19 ` Bertrand Marquis
2024-04-26 18:31 ` Julien Grall
1 sibling, 1 reply; 22+ messages in thread
From: Jens Wiklander @ 2024-04-26 12:11 UTC (permalink / raw)
To: Bertrand Marquis
Cc: Xen-devel, patches, Stefano Stabellini, Julien Grall,
Michal Orzel, Volodymyr Babchuk
Hi Bertrand,
On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Jens,
>
> > On 26 Apr 2024, at 10:47, 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.
> >
> > Update ffa_partinfo_domain_init() to return error code like
> > ffa_notif_domain_init().
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> > v2->v3:
> > - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
> > FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
> > - Register the Xen SRI handler on each CPU using on_selected_cpus() and
> > setup_irq()
> > - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
> > doesn't conflict with static SGI handlers
> >
> > v1->v2:
> > - Addressing review comments
> > - Change ffa_handle_notification_{bind,unbind,set}() to take struct
> > cpu_user_regs *regs as argument.
> > - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
> > an error code.
> > - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
> > ---
> > xen/arch/arm/irq.c | 2 +-
> > xen/arch/arm/tee/Makefile | 1 +
> > xen/arch/arm/tee/ffa.c | 55 ++++-
> > xen/arch/arm/tee/ffa_notif.c | 378 ++++++++++++++++++++++++++++++++
> > xen/arch/arm/tee/ffa_partinfo.c | 9 +-
> > xen/arch/arm/tee/ffa_private.h | 56 ++++-
> > xen/include/public/arch-arm.h | 14 ++
> > 7 files changed, 507 insertions(+), 8 deletions(-)
> > create mode 100644 xen/arch/arm/tee/ffa_notif.c
> >
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index d7306aa64d50..5224898265a5 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -155,7 +155,7 @@ void __init init_IRQ(void)
> > {
> > /* SGIs are always edge-triggered */
> > if ( irq < NR_GIC_SGI )
> > - local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
> > + local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
> > else
> > local_irqs_type[irq] = IRQ_TYPE_INVALID;
> > }
> > diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
> > index f0112a2f922d..7c0f46f7f446 100644
> > --- a/xen/arch/arm/tee/Makefile
> > +++ b/xen/arch/arm/tee/Makefile
> > @@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
> > obj-$(CONFIG_FFA) += ffa_shm.o
> > obj-$(CONFIG_FFA) += ffa_partinfo.o
> > obj-$(CONFIG_FFA) += ffa_rxtx.o
> > +obj-$(CONFIG_FFA) += ffa_notif.o
> > obj-y += tee.o
> > obj-$(CONFIG_OPTEE) += optee.o
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index 5209612963e1..912e905a27bd 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -39,6 +39,9 @@
> > * - at most 32 shared memory regions per guest
> > * o FFA_MSG_SEND_DIRECT_REQ:
> > * - only supported from a VM to an SP
> > + * o FFA_NOTIFICATION_*:
> > + * - only supports global notifications, that is, per vCPU notifications
> > + * are not supported
> > *
> > * There are some large locked sections with ffa_tx_buffer_lock and
> > * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
> > @@ -194,6 +197,8 @@ out:
> >
> > static void handle_features(struct cpu_user_regs *regs)
> > {
> > + struct domain *d = current->domain;
> > + struct ffa_ctx *ctx = d->arch.tee;
> > uint32_t a1 = get_user_reg(regs, 1);
> > unsigned int n;
> >
> > @@ -240,6 +245,30 @@ static void handle_features(struct cpu_user_regs *regs)
> > BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
> > ffa_set_regs_success(regs, 0, 0);
> > break;
> > + case FFA_FEATURE_NOTIF_PEND_INTR:
> > + if ( ctx->notif.enabled )
> > + ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
> > + else
> > + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > + break;
> > + case FFA_FEATURE_SCHEDULE_RECV_INTR:
> > + if ( ctx->notif.enabled )
> > + ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
> > + else
> > + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > + break;
> > +
> > + case FFA_NOTIFICATION_BIND:
> > + case FFA_NOTIFICATION_UNBIND:
> > + case FFA_NOTIFICATION_GET:
> > + case FFA_NOTIFICATION_SET:
> > + case FFA_NOTIFICATION_INFO_GET_32:
> > + case FFA_NOTIFICATION_INFO_GET_64:
> > + if ( ctx->notif.enabled )
> > + ffa_set_regs_success(regs, 0, 0);
> > + else
> > + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > + break;
> > default:
> > ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > break;
> > @@ -305,6 +334,22 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> > get_user_reg(regs, 1)),
> > get_user_reg(regs, 3));
> > break;
> > + case FFA_NOTIFICATION_BIND:
> > + e = ffa_handle_notification_bind(regs);
> > + break;
> > + case FFA_NOTIFICATION_UNBIND:
> > + e = ffa_handle_notification_unbind(regs);
> > + break;
> > + case FFA_NOTIFICATION_INFO_GET_32:
> > + case FFA_NOTIFICATION_INFO_GET_64:
> > + ffa_handle_notification_info_get(regs);
> > + return true;
> > + case FFA_NOTIFICATION_GET:
> > + ffa_handle_notification_get(regs);
> > + return true;
> > + case FFA_NOTIFICATION_SET:
> > + e = ffa_handle_notification_set(regs);
> > + break;
> >
> > default:
> > gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> > @@ -322,6 +367,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> > static int ffa_domain_init(struct domain *d)
> > {
> > struct ffa_ctx *ctx;
> > + int ret;
> >
> > if ( !ffa_version )
> > return -ENODEV;
> > @@ -345,10 +391,11 @@ static int ffa_domain_init(struct domain *d)
> > * error, so no need for cleanup in this function.
> > */
> >
> > - if ( !ffa_partinfo_domain_init(d) )
> > - return -EIO;
> > + ret = ffa_partinfo_domain_init(d);
> > + if ( ret )
> > + return ret;
> >
> > - return 0;
> > + return ffa_notif_domain_init(d);
> > }
> >
> > static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
> > @@ -423,6 +470,7 @@ static int ffa_domain_teardown(struct domain *d)
> > return 0;
> >
> > ffa_rxtx_domain_destroy(d);
> > + ffa_notif_domain_destroy(d);
> >
> > ffa_domain_teardown_continue(ctx, true /* first_time */);
> >
> > @@ -502,6 +550,7 @@ static bool ffa_probe(void)
> > if ( !ffa_partinfo_init() )
> > goto err_rxtx_destroy;
> >
> > + ffa_notif_init();
> > INIT_LIST_HEAD(&ffa_teardown_head);
> > init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
> >
> > diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
> > new file mode 100644
> > index 000000000000..6bb0804ee2f8
> > --- /dev/null
> > +++ b/xen/arch/arm/tee/ffa_notif.c
> > @@ -0,0 +1,378 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (C) 2024 Linaro Limited
> > + */
> > +
> > +#include <xen/const.h>
> > +#include <xen/list.h>
> > +#include <xen/spinlock.h>
> > +#include <xen/types.h>
> > +
> > +#include <asm/smccc.h>
> > +#include <asm/regs.h>
> > +
> > +#include "ffa_private.h"
> > +
> > +static bool __ro_after_init notif_enabled;
> > +
> > +int ffa_handle_notification_bind(struct cpu_user_regs *regs)
> > +{
> > + struct domain *d = current->domain;
> > + uint32_t src_dst = get_user_reg(regs, 1);
> > + uint32_t flags = get_user_reg(regs, 2);
> > + uint32_t bitmap_lo = get_user_reg(regs, 3);
> > + uint32_t bitmap_hi = get_user_reg(regs, 4);
> > +
> > + if ( !notif_enabled )
> > + return FFA_RET_NOT_SUPPORTED;
> > +
> > + if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> > + return FFA_RET_INVALID_PARAMETERS;
> > +
> > + if ( flags ) /* Only global notifications are supported */
> > + return FFA_RET_DENIED;
> > +
> > + /*
> > + * We only support notifications from SP so no need to check the sender
> > + * endpoint ID, the SPMC will take care of that for us.
> > + */
> > + return ffa_simple_call(FFA_NOTIFICATION_BIND, src_dst, flags, bitmap_hi,
> > + bitmap_lo);
> > +}
> > +
> > +int ffa_handle_notification_unbind(struct cpu_user_regs *regs)
> > +{
> > + struct domain *d = current->domain;
> > + uint32_t src_dst = get_user_reg(regs, 1);
> > + uint32_t bitmap_lo = get_user_reg(regs, 3);
> > + uint32_t bitmap_hi = get_user_reg(regs, 4);
> > +
> > + if ( !notif_enabled )
> > + return FFA_RET_NOT_SUPPORTED;
> > +
> > + if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> > + return FFA_RET_INVALID_PARAMETERS;
> > +
> > + /*
> > + * We only support notifications from SP so no need to check the
> > + * destination endpoint ID, the SPMC will take care of that for us.
> > + */
> > + return ffa_simple_call(FFA_NOTIFICATION_UNBIND, src_dst, 0, bitmap_hi,
> > + bitmap_lo);
> > +}
> > +
> > +void ffa_handle_notification_info_get(struct cpu_user_regs *regs)
> > +{
> > + struct domain *d = current->domain;
> > + struct ffa_ctx *ctx = d->arch.tee;
> > + bool pending_global;
> > +
> > + if ( !notif_enabled )
> > + {
> > + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > + return;
> > + }
> > +
> > + spin_lock(&ctx->notif.lock);
> > + pending_global = ctx->notif.secure_pending;
> > + ctx->notif.secure_pending = false;
> > + spin_unlock(&ctx->notif.lock);
> > +
> > + if ( pending_global )
> > + {
> > + /* A pending global notification for the guest */
> > + ffa_set_regs(regs, FFA_SUCCESS_64, 0,
> > + 1U << FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT, ffa_get_vm_id(d),
> > + 0, 0, 0, 0);
> > + }
> > + else
> > + {
> > + /* Report an error if there where no pending global notification */
> > + ffa_set_regs_error(regs, FFA_RET_NO_DATA);
> > + }
> > +}
> > +
> > +void ffa_handle_notification_get(struct cpu_user_regs *regs)
> > +{
> > + struct domain *d = current->domain;
> > + uint32_t recv = get_user_reg(regs, 1);
> > + uint32_t flags = get_user_reg(regs, 2);
> > + uint32_t w2 = 0;
> > + uint32_t w3 = 0;
> > + uint32_t w4 = 0;
> > + uint32_t w5 = 0;
> > + uint32_t w6 = 0;
> > + uint32_t w7 = 0;
> > +
> > + if ( !notif_enabled )
> > + {
> > + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > + return;
> > + }
> > +
> > + if ( (recv & 0xFFFFU) != ffa_get_vm_id(d) )
> > + {
> > + ffa_set_regs_error(regs, FFA_RET_INVALID_PARAMETERS);
> > + return;
> > + }
> > +
> > + if ( flags & ( FFA_NOTIF_FLAG_BITMAP_SP | FFA_NOTIF_FLAG_BITMAP_SPM ) )
> > + {
> > + struct arm_smccc_1_2_regs arg = {
> > + .a0 = FFA_NOTIFICATION_GET,
> > + .a1 = recv,
> > + .a2 = flags & ( FFA_NOTIF_FLAG_BITMAP_SP |
> > + FFA_NOTIF_FLAG_BITMAP_SPM ),
> > + };
> > + struct arm_smccc_1_2_regs resp;
> > + int32_t e;
> > +
> > + 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;
> > + }
>
> In here you never clean the secure_pending flag but in practice there would be
> no more pending notification if SP and SPM flags are set so the response to
> notification_info_get would wrongly say there is something pending.
>
> I am not completely sure how this could be handled if both SP and SPM are
> not set so i would say for now we should at least put a comment in info_get
> to keep a note of this fact.
> Do you agree ?
Good point. I'll add a comment and clear secure_pending.
>
> > +
> > + ffa_set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, w4, w5, w6, w7);
> > +}
> > +
> > +int ffa_handle_notification_set(struct cpu_user_regs *regs)
> > +{
> > + struct domain *d = current->domain;
> > + uint32_t src_dst = get_user_reg(regs, 1);
> > + uint32_t flags = get_user_reg(regs, 2);
> > + uint32_t bitmap_lo = get_user_reg(regs, 3);
> > + uint32_t bitmap_hi = get_user_reg(regs, 4);
> > +
> > + if ( !notif_enabled )
> > + return FFA_RET_NOT_SUPPORTED;
> > +
> > + if ( (src_dst >> 16) != ffa_get_vm_id(d) )
> > + return FFA_RET_INVALID_PARAMETERS;
> > +
> > + /* Let the SPMC check the destination of the notification */
> > + return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags, bitmap_lo,
> > + bitmap_hi);
> > +}
> > +
> > +/*
> > + * Extract a 16-bit ID (index n) from the successful return value from
> > + * FFA_NOTIFICATION_INFO_GET_64 or FFA_NOTIFICATION_INFO_GET_32. IDs are
> > + * returned in registers 3 to 7 with four IDs per register for 64-bit
> > + * calling convention and two IDs per register for 32-bit calling
> > + * convention.
> > + */
> > +static uint16_t get_id_from_resp(struct arm_smccc_1_2_regs *resp,
> > + unsigned int n)
> > +{
> > + unsigned int ids_per_reg;
> > + unsigned int reg_idx;
> > + unsigned int reg_shift;
> > +
> > + if ( smccc_is_conv_64(resp->a0) )
> > + ids_per_reg = 4;
> > + else
> > + ids_per_reg = 2;
> > +
> > + reg_idx = n / ids_per_reg + 3;
> > + reg_shift = ( n % ids_per_reg ) * 16;
> > +
> > + switch ( reg_idx )
> > + {
> > + case 3:
> > + return resp->a3 >> reg_shift;
> > + case 4:
> > + return resp->a4 >> reg_shift;
> > + case 5:
> > + return resp->a5 >> reg_shift;
> > + case 6:
> > + return resp->a6 >> reg_shift;
> > + case 7:
> > + return resp->a7 >> reg_shift;
> > + default:
> > + ASSERT(0); /* "Can't happen" */
> > + return 0;
> > + }
> > +}
> > +
> > +static void notif_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;
> > +
>
> If a notification is pending for a secure partition at this stage we are not
> signaling anything so I think this fact should be listed in the limitations to
> say that we do not signal any secondary scheduler if a notification is
> pending for a secure partition.
I agree, I'll add a note in the limitation.
>
> > + 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], GUEST_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);
> > +}
> > +
> > +struct notif_irq_info {
> > + unsigned int irq;
> > + int ret;
> > + struct irqaction *action;
> > +};
> > +
> > +static void notif_irq_enable(void *info)
> > +{
> > + struct notif_irq_info *irq_info = info;
> > +
> > + irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
> > + if ( irq_info->ret )
> > + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> > + irq_info->irq, irq_info->ret);
> > +}
> > +
> > +void ffa_notif_init(void)
> > +{
> > + const struct arm_smccc_1_2_regs arg = {
> > + .a0 = FFA_FEATURES,
> > + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> > + };
> > + struct notif_irq_info irq_info = { };
> > + struct arm_smccc_1_2_regs resp;
> > + unsigned int cpu;
> > +
> > + arm_smccc_1_2_smc(&arg, &resp);
> > + if ( resp.a0 != FFA_SUCCESS_32 )
> > + return;
> > +
> > + irq_info.irq = resp.a2;
> > + if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
> > + {
> > + printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
> > + irq_info.irq);
> > + return;
> > + }
> > +
> > + /*
> > + * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> > + * IPI to call notif_irq_enable() on each CPU including the current
> > + * CPU. The struct irqaction is preallocated since we can't allocate
> > + * memory while in interrupt context.
> > + */
> > + for_each_online_cpu(cpu)
> > + {
> > + irq_info.action = xmalloc(struct irqaction);
>
> You allocate one action per cpu but you have only one action pointer in your structure
> which means you will overload the previously allocated one and lose track.
>
> You should have a table of actions in your structure instead unless one action is
> enough and can be reused on all cpus and in this case you should move out of
> your loop the allocation part.
That shouldn't be needed because this is done in sequence only one CPU
at a time.
>
> > + if ( !irq_info.action )
> > + {
> > + irq_info.ret = -ENOMEM;
> > + break;
> > + }
> > +
> > + *irq_info.action = (struct irqaction){
> > + .handler = notif_irq_handler,
> > + .name = "FF-A notif",
> > + .free_on_release = 1,
> > + };
> > +
> > + on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
> > + if ( irq_info.ret )
> > + {
> > + XFREE(irq_info.action);
> > + break;
> > + }
> > + }
> > +
> > + notif_enabled = !irq_info.ret;
> > +}
> > +
> > +int ffa_notif_domain_init(struct domain *d)
> > +{
> > + struct ffa_ctx *ctx = d->arch.tee;
> > + int32_t res;
> > +
> > + if ( !notif_enabled )
> > + return 0;
> > +
> > + res = ffa_notification_bitmap_create(ffa_get_vm_id(d), d->max_vcpus);
> > + if ( res )
> > + return -ENOMEM;
> > +
> > + ctx->notif.enabled = true;
> > +
> > + return 0;
> > +}
> > +
> > +void ffa_notif_domain_destroy(struct domain *d)
> > +{
> > + struct ffa_ctx *ctx = d->arch.tee;
> > +
> > + if ( ctx->notif.enabled )
> > + {
> > + ffa_notification_bitmap_destroy(ffa_get_vm_id(d));
> > + ctx->notif.enabled = false;
> > + }
> > +}
> > diff --git a/xen/arch/arm/tee/ffa_partinfo.c b/xen/arch/arm/tee/ffa_partinfo.c
> > index dc1059584828..93a03c6bc672 100644
> > --- a/xen/arch/arm/tee/ffa_partinfo.c
> > +++ b/xen/arch/arm/tee/ffa_partinfo.c
> > @@ -306,7 +306,7 @@ static void vm_destroy_bitmap_init(struct ffa_ctx *ctx,
> > }
> > }
> >
> > -bool ffa_partinfo_domain_init(struct domain *d)
> > +int ffa_partinfo_domain_init(struct domain *d)
> > {
> > unsigned int count = BITS_TO_LONGS(subscr_vm_destroyed_count);
> > struct ffa_ctx *ctx = d->arch.tee;
> > @@ -315,7 +315,7 @@ bool ffa_partinfo_domain_init(struct domain *d)
> >
> > ctx->vm_destroy_bitmap = xzalloc_array(unsigned long, count);
> > if ( !ctx->vm_destroy_bitmap )
> > - return false;
> > + return -ENOMEM;
> >
> > for ( n = 0; n < subscr_vm_created_count; n++ )
> > {
> > @@ -330,7 +330,10 @@ bool ffa_partinfo_domain_init(struct domain *d)
> > }
> > vm_destroy_bitmap_init(ctx, n);
> >
> > - return n == subscr_vm_created_count;
> > + if ( n != subscr_vm_created_count )
> > + return -EIO;
> > +
> > + return 0;
> > }
> >
> > bool ffa_partinfo_domain_destroy(struct domain *d)
> > diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
> > index 98236cbf14a3..a59c9887774b 100644
> > --- a/xen/arch/arm/tee/ffa_private.h
> > +++ b/xen/arch/arm/tee/ffa_private.h
> > @@ -25,6 +25,7 @@
> > #define FFA_RET_DENIED -6
> > #define FFA_RET_RETRY -7
> > #define FFA_RET_ABORTED -8
> > +#define FFA_RET_NO_DATA -9
> >
> > /* FFA_VERSION helpers */
> > #define FFA_VERSION_MAJOR_SHIFT 16U
> > @@ -175,6 +176,21 @@
> > */
> > #define FFA_PARTITION_INFO_GET_COUNT_FLAG BIT(0, U)
> >
> > +/* Flags used in calls to FFA_NOTIFICATION_GET interface */
> > +#define FFA_NOTIF_FLAG_BITMAP_SP BIT(0, U)
> > +#define FFA_NOTIF_FLAG_BITMAP_VM BIT(1, U)
> > +#define FFA_NOTIF_FLAG_BITMAP_SPM BIT(2, U)
> > +#define FFA_NOTIF_FLAG_BITMAP_HYP BIT(3, U)
> > +
> > +#define FFA_NOTIF_INFO_GET_MORE_FLAG BIT(0, U)
> > +#define FFA_NOTIF_INFO_GET_ID_LIST_SHIFT 12
> > +#define FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT 7
> > +#define FFA_NOTIF_INFO_GET_ID_COUNT_MASK 0x1F
> > +
> > +/* Feature IDs used with FFA_FEATURES */
> > +#define FFA_FEATURE_NOTIF_PEND_INTR 0x1U
> > +#define FFA_FEATURE_SCHEDULE_RECV_INTR 0x2U
> > +
> > /* Function IDs */
> > #define FFA_ERROR 0x84000060U
> > #define FFA_SUCCESS_32 0x84000061U
> > @@ -213,6 +229,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 +265,7 @@ struct ffa_ctx {
> > struct list_head shm_list;
> > /* Number of allocated shared memory object */
> > unsigned int shm_count;
> > + struct ffa_ctx_notif notif;
> > /*
> > * tx_lock is used to serialize access to tx
> > * rx_lock is used to serialize access to rx
> > @@ -257,7 +295,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs);
> > int ffa_handle_mem_reclaim(uint64_t handle, uint32_t flags);
> >
> > bool ffa_partinfo_init(void);
> > -bool ffa_partinfo_domain_init(struct domain *d);
> > +int ffa_partinfo_domain_init(struct domain *d);
> > bool ffa_partinfo_domain_destroy(struct domain *d);
> > int32_t ffa_handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
> > uint32_t w4, uint32_t w5, uint32_t *count,
> > @@ -271,12 +309,28 @@ uint32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
> > uint32_t ffa_handle_rxtx_unmap(void);
> > int32_t ffa_handle_rx_release(void);
> >
> > +void ffa_notif_init(void);
> > +int ffa_notif_domain_init(struct domain *d);
> > +void ffa_notif_domain_destroy(struct domain *d);
> > +
> > +int ffa_handle_notification_bind(struct cpu_user_regs *regs);
> > +int ffa_handle_notification_unbind(struct cpu_user_regs *regs);
> > +void ffa_handle_notification_info_get(struct cpu_user_regs *regs);
> > +void ffa_handle_notification_get(struct cpu_user_regs *regs);
> > +int ffa_handle_notification_set(struct cpu_user_regs *regs);
> > +
> > static inline uint16_t ffa_get_vm_id(const struct domain *d)
> > {
> > /* +1 since 0 is reserved for the hypervisor in FF-A */
> > return d->domain_id + 1;
> > }
> >
> > +static inline struct domain *ffa_get_domain_by_vm_id(uint16_t vm_id)
> > +{
> > + /* -1 to match ffa_get_vm_id() */
> > + return get_domain_by_id(vm_id - 1);
> > +}
> > +
>
> I think there is a global discussion to have on using get_domain_by_vm_id
> or rcu_lock_live_remote_domain_by_id to make sure the domain is not
> dying when we try to do something with it.
>
> It does not need to be done here as there are other places to adapt but
> i wanted to raise the question.
>
> I would like to know what you and also other maintainers think here.
> @Julien/Michal/Stefano ?
>
> > static inline void ffa_set_regs(struct cpu_user_regs *regs, register_t v0,
> > register_t v1, register_t v2, register_t v3,
> > register_t v4, register_t v5, register_t v6,
> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index e167e14f8df9..1eac9802aa53 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -505,6 +505,7 @@ typedef uint64_t xen_callback_t;
> > #define GUEST_MAX_VCPUS 128
> >
> > /* Interrupts */
> > +
> > #define GUEST_TIMER_VIRT_PPI 27
> > #define GUEST_TIMER_PHYS_S_PPI 29
> > #define GUEST_TIMER_PHYS_NS_PPI 30
> > @@ -515,6 +516,19 @@ typedef uint64_t xen_callback_t;
> > #define GUEST_VIRTIO_MMIO_SPI_FIRST 33
> > #define GUEST_VIRTIO_MMIO_SPI_LAST 43
> >
> > +/*
> > + * SGI is the preferred delivery mechanism of FF-A pending notifications or
> > + * schedule recveive interrupt. SGIs 8-15 are normally not used by a guest
> > + * as they in a non-virtualized system typically are assigned to the secure
> > + * world. Here we're free to use SGI 8-15 since they are virtual and have
> > + * nothing to do with the secure world.
> > + *
> > + * For partitioning of SGIs see also Arm Base System Architecture v1.0C,
> > + * https://developer.arm.com/documentation/den0094/
> > + */
> > +#define GUEST_FFA_NOTIF_PEND_INTR_ID 8
> > +#define GUEST_FFA_SCHEDULE_RECV_INTR_ID 9
> > +
> > /* PSCI functions */
> > #define PSCI_cpu_suspend 0
> > #define PSCI_cpu_off 1
> > --
> > 2.34.1
> >
Thanks,
Jens
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-26 12:11 ` Jens Wiklander
@ 2024-04-26 12:19 ` Bertrand Marquis
2024-04-26 12:32 ` Jens Wiklander
0 siblings, 1 reply; 22+ messages in thread
From: Bertrand Marquis @ 2024-04-26 12:19 UTC (permalink / raw)
To: Jens Wiklander
Cc: Xen-devel, patches, Stefano Stabellini, Julien Grall,
Michal Orzel, Volodymyr Babchuk
Hi Jens,
> On 26 Apr 2024, at 14:11, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Bertrand,
>
> On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
> <Bertrand.Marquis@arm.com> wrote:
>>
>> Hi Jens,
>>
>>> On 26 Apr 2024, at 10:47, 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.
>>>
>>> Update ffa_partinfo_domain_init() to return error code like
>>> ffa_notif_domain_init().
>>>
>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>>> ---
>>> v2->v3:
>>> - Add a GUEST_ prefix and move FFA_NOTIF_PEND_INTR_ID and
>>> FFA_SCHEDULE_RECV_INTR_ID to public/arch-arm.h
>>> - Register the Xen SRI handler on each CPU using on_selected_cpus() and
>>> setup_irq()
>>> - Check that the SGI ID retrieved with FFA_FEATURE_SCHEDULE_RECV_INTR
>>> doesn't conflict with static SGI handlers
>>>
>>> v1->v2:
>>> - Addressing review comments
>>> - Change ffa_handle_notification_{bind,unbind,set}() to take struct
>>> cpu_user_regs *regs as argument.
>>> - Update ffa_partinfo_domain_init() and ffa_notif_domain_init() to return
>>> an error code.
>>> - Fixing a bug in handle_features() for FFA_FEATURE_SCHEDULE_RECV_INTR.
>>> ---
>>> xen/arch/arm/irq.c | 2 +-
>>> xen/arch/arm/tee/Makefile | 1 +
>>> xen/arch/arm/tee/ffa.c | 55 ++++-
>>> xen/arch/arm/tee/ffa_notif.c | 378 ++++++++++++++++++++++++++++++++
>>> xen/arch/arm/tee/ffa_partinfo.c | 9 +-
>>> xen/arch/arm/tee/ffa_private.h | 56 ++++-
>>> xen/include/public/arch-arm.h | 14 ++
>>> 7 files changed, 507 insertions(+), 8 deletions(-)
>>> create mode 100644 xen/arch/arm/tee/ffa_notif.c
>>>
>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>>> index d7306aa64d50..5224898265a5 100644
>>> --- a/xen/arch/arm/irq.c
>>> +++ b/xen/arch/arm/irq.c
>>> @@ -155,7 +155,7 @@ void __init init_IRQ(void)
>>> {
>>> /* SGIs are always edge-triggered */
>>> if ( irq < NR_GIC_SGI )
>>> - local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
>>> + local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
>>> else
>>> local_irqs_type[irq] = IRQ_TYPE_INVALID;
>>> }
>>> diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
>>> index f0112a2f922d..7c0f46f7f446 100644
>>> --- a/xen/arch/arm/tee/Makefile
>>> +++ b/xen/arch/arm/tee/Makefile
>>> @@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
>>> obj-$(CONFIG_FFA) += ffa_shm.o
>>> obj-$(CONFIG_FFA) += ffa_partinfo.o
>>> obj-$(CONFIG_FFA) += ffa_rxtx.o
>>> +obj-$(CONFIG_FFA) += ffa_notif.o
>>> obj-y += tee.o
>>> obj-$(CONFIG_OPTEE) += optee.o
>>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>>> index 5209612963e1..912e905a27bd 100644
>>> --- a/xen/arch/arm/tee/ffa.c
>>> +++ b/xen/arch/arm/tee/ffa.c
>>> @@ -39,6 +39,9 @@
>>> * - at most 32 shared memory regions per guest
>>> * o FFA_MSG_SEND_DIRECT_REQ:
>>> * - only supported from a VM to an SP
>>> + * o FFA_NOTIFICATION_*:
>>> + * - only supports global notifications, that is, per vCPU notifications
>>> + * are not supported
>>> *
>>> * There are some large locked sections with ffa_tx_buffer_lock and
>>> * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
>>> @@ -194,6 +197,8 @@ out:
>>>
>>> static void handle_features(struct cpu_user_regs *regs)
>>> {
>>> + struct domain *d = current->domain;
>>> + struct ffa_ctx *ctx = d->arch.tee;
>>> uint32_t a1 = get_user_reg(regs, 1);
>>> unsigned int n;
>>>
>>> @@ -240,6 +245,30 @@ static void handle_features(struct cpu_user_regs *regs)
>>> BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
>>> ffa_set_regs_success(regs, 0, 0);
>>> break;
>>> + case FFA_FEATURE_NOTIF_PEND_INTR:
>>> + if ( ctx->notif.enabled )
>>> + ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
>>> + else
>>> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>> + break;
>>> + case FFA_FEATURE_SCHEDULE_RECV_INTR:
>>> + if ( ctx->notif.enabled )
>>> + ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
>>> + else
>>> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>> + break;
>>> +
>>> + case FFA_NOTIFICATION_BIND:
>>> + case FFA_NOTIFICATION_UNBIND:
>>> + case FFA_NOTIFICATION_GET:
>>> + case FFA_NOTIFICATION_SET:
>>> + case FFA_NOTIFICATION_INFO_GET_32:
>>> + case FFA_NOTIFICATION_INFO_GET_64:
>>> + if ( ctx->notif.enabled )
>>> + ffa_set_regs_success(regs, 0, 0);
>>> + else
>>> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>> + break;
>>> default:
>>> ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>> break;
>>> @@ -305,6 +334,22 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>>> get_user_reg(regs, 1)),
>>> get_user_reg(regs, 3));
>>> break;
>>> + case FFA_NOTIFICATION_BIND:
>>> + e = ffa_handle_notification_bind(regs);
>>> + break;
>>> + case FFA_NOTIFICATION_UNBIND:
>>> + e = ffa_handle_notification_unbind(regs);
>>> + break;
>>> + case FFA_NOTIFICATION_INFO_GET_32:
>>> + case FFA_NOTIFICATION_INFO_GET_64:
>>> + ffa_handle_notification_info_get(regs);
>>> + return true;
>>> + case FFA_NOTIFICATION_GET:
>>> + ffa_handle_notification_get(regs);
>>> + return true;
>>> + case FFA_NOTIFICATION_SET:
>>> + e = ffa_handle_notification_set(regs);
>>> + break;
>>>
>>> default:
>>> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
>>> @@ -322,6 +367,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>>> static int ffa_domain_init(struct domain *d)
>>> {
>>> struct ffa_ctx *ctx;
>>> + int ret;
>>>
>>> if ( !ffa_version )
>>> return -ENODEV;
>>> @@ -345,10 +391,11 @@ static int ffa_domain_init(struct domain *d)
>>> * error, so no need for cleanup in this function.
>>> */
>>>
>>> - if ( !ffa_partinfo_domain_init(d) )
>>> - return -EIO;
>>> + ret = ffa_partinfo_domain_init(d);
>>> + if ( ret )
>>> + return ret;
>>>
>>> - return 0;
>>> + return ffa_notif_domain_init(d);
>>> }
>>>
>>> static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
>>> @@ -423,6 +470,7 @@ static int ffa_domain_teardown(struct domain *d)
>>> return 0;
>>>
>>> ffa_rxtx_domain_destroy(d);
>>> + ffa_notif_domain_destroy(d);
>>>
>>> ffa_domain_teardown_continue(ctx, true /* first_time */);
>>>
>>> @@ -502,6 +550,7 @@ static bool ffa_probe(void)
>>> if ( !ffa_partinfo_init() )
>>> goto err_rxtx_destroy;
>>>
>>> + ffa_notif_init();
>>> INIT_LIST_HEAD(&ffa_teardown_head);
>>> init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
>>>
>>> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
>>> new file mode 100644
>>> index 000000000000..6bb0804ee2f8
>>> --- /dev/null
>>> +++ b/xen/arch/arm/tee/ffa_notif.c
>>> @@ -0,0 +1,378 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Copyright (C) 2024 Linaro Limited
>>> + */
>>> +
>>> +#include <xen/const.h>
>>> +#include <xen/list.h>
>>> +#include <xen/spinlock.h>
>>> +#include <xen/types.h>
>>> +
>>> +#include <asm/smccc.h>
>>> +#include <asm/regs.h>
>>> +
>>> +#include "ffa_private.h"
>>> +
>>> +static bool __ro_after_init notif_enabled;
>>> +
>>> +int ffa_handle_notification_bind(struct cpu_user_regs *regs)
>>> +{
>>> + struct domain *d = current->domain;
>>> + uint32_t src_dst = get_user_reg(regs, 1);
>>> + uint32_t flags = get_user_reg(regs, 2);
>>> + uint32_t bitmap_lo = get_user_reg(regs, 3);
>>> + uint32_t bitmap_hi = get_user_reg(regs, 4);
>>> +
>>> + if ( !notif_enabled )
>>> + return FFA_RET_NOT_SUPPORTED;
>>> +
>>> + if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
>>> + return FFA_RET_INVALID_PARAMETERS;
>>> +
>>> + if ( flags ) /* Only global notifications are supported */
>>> + return FFA_RET_DENIED;
>>> +
>>> + /*
>>> + * We only support notifications from SP so no need to check the sender
>>> + * endpoint ID, the SPMC will take care of that for us.
>>> + */
>>> + return ffa_simple_call(FFA_NOTIFICATION_BIND, src_dst, flags, bitmap_hi,
>>> + bitmap_lo);
>>> +}
>>> +
>>> +int ffa_handle_notification_unbind(struct cpu_user_regs *regs)
>>> +{
>>> + struct domain *d = current->domain;
>>> + uint32_t src_dst = get_user_reg(regs, 1);
>>> + uint32_t bitmap_lo = get_user_reg(regs, 3);
>>> + uint32_t bitmap_hi = get_user_reg(regs, 4);
>>> +
>>> + if ( !notif_enabled )
>>> + return FFA_RET_NOT_SUPPORTED;
>>> +
>>> + if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
>>> + return FFA_RET_INVALID_PARAMETERS;
>>> +
>>> + /*
>>> + * We only support notifications from SP so no need to check the
>>> + * destination endpoint ID, the SPMC will take care of that for us.
>>> + */
>>> + return ffa_simple_call(FFA_NOTIFICATION_UNBIND, src_dst, 0, bitmap_hi,
>>> + bitmap_lo);
>>> +}
>>> +
>>> +void ffa_handle_notification_info_get(struct cpu_user_regs *regs)
>>> +{
>>> + struct domain *d = current->domain;
>>> + struct ffa_ctx *ctx = d->arch.tee;
>>> + bool pending_global;
>>> +
>>> + if ( !notif_enabled )
>>> + {
>>> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>> + return;
>>> + }
>>> +
>>> + spin_lock(&ctx->notif.lock);
>>> + pending_global = ctx->notif.secure_pending;
>>> + ctx->notif.secure_pending = false;
>>> + spin_unlock(&ctx->notif.lock);
>>> +
>>> + if ( pending_global )
>>> + {
>>> + /* A pending global notification for the guest */
>>> + ffa_set_regs(regs, FFA_SUCCESS_64, 0,
>>> + 1U << FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT, ffa_get_vm_id(d),
>>> + 0, 0, 0, 0);
>>> + }
>>> + else
>>> + {
>>> + /* Report an error if there where no pending global notification */
>>> + ffa_set_regs_error(regs, FFA_RET_NO_DATA);
>>> + }
>>> +}
>>> +
>>> +void ffa_handle_notification_get(struct cpu_user_regs *regs)
>>> +{
>>> + struct domain *d = current->domain;
>>> + uint32_t recv = get_user_reg(regs, 1);
>>> + uint32_t flags = get_user_reg(regs, 2);
>>> + uint32_t w2 = 0;
>>> + uint32_t w3 = 0;
>>> + uint32_t w4 = 0;
>>> + uint32_t w5 = 0;
>>> + uint32_t w6 = 0;
>>> + uint32_t w7 = 0;
>>> +
>>> + if ( !notif_enabled )
>>> + {
>>> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>>> + return;
>>> + }
>>> +
>>> + if ( (recv & 0xFFFFU) != ffa_get_vm_id(d) )
>>> + {
>>> + ffa_set_regs_error(regs, FFA_RET_INVALID_PARAMETERS);
>>> + return;
>>> + }
>>> +
>>> + if ( flags & ( FFA_NOTIF_FLAG_BITMAP_SP | FFA_NOTIF_FLAG_BITMAP_SPM ) )
>>> + {
>>> + struct arm_smccc_1_2_regs arg = {
>>> + .a0 = FFA_NOTIFICATION_GET,
>>> + .a1 = recv,
>>> + .a2 = flags & ( FFA_NOTIF_FLAG_BITMAP_SP |
>>> + FFA_NOTIF_FLAG_BITMAP_SPM ),
>>> + };
>>> + struct arm_smccc_1_2_regs resp;
>>> + int32_t e;
>>> +
>>> + 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;
>>> + }
>>
>> In here you never clean the secure_pending flag but in practice there would be
>> no more pending notification if SP and SPM flags are set so the response to
>> notification_info_get would wrongly say there is something pending.
>>
>> I am not completely sure how this could be handled if both SP and SPM are
>> not set so i would say for now we should at least put a comment in info_get
>> to keep a note of this fact.
>> Do you agree ?
>
> Good point. I'll add a comment and clear secure_pending.
>
>>
>>> +
>>> + ffa_set_regs(regs, FFA_SUCCESS_32, 0, w2, w3, w4, w5, w6, w7);
>>> +}
>>> +
>>> +int ffa_handle_notification_set(struct cpu_user_regs *regs)
>>> +{
>>> + struct domain *d = current->domain;
>>> + uint32_t src_dst = get_user_reg(regs, 1);
>>> + uint32_t flags = get_user_reg(regs, 2);
>>> + uint32_t bitmap_lo = get_user_reg(regs, 3);
>>> + uint32_t bitmap_hi = get_user_reg(regs, 4);
>>> +
>>> + if ( !notif_enabled )
>>> + return FFA_RET_NOT_SUPPORTED;
>>> +
>>> + if ( (src_dst >> 16) != ffa_get_vm_id(d) )
>>> + return FFA_RET_INVALID_PARAMETERS;
>>> +
>>> + /* Let the SPMC check the destination of the notification */
>>> + return ffa_simple_call(FFA_NOTIFICATION_SET, src_dst, flags, bitmap_lo,
>>> + bitmap_hi);
>>> +}
>>> +
>>> +/*
>>> + * Extract a 16-bit ID (index n) from the successful return value from
>>> + * FFA_NOTIFICATION_INFO_GET_64 or FFA_NOTIFICATION_INFO_GET_32. IDs are
>>> + * returned in registers 3 to 7 with four IDs per register for 64-bit
>>> + * calling convention and two IDs per register for 32-bit calling
>>> + * convention.
>>> + */
>>> +static uint16_t get_id_from_resp(struct arm_smccc_1_2_regs *resp,
>>> + unsigned int n)
>>> +{
>>> + unsigned int ids_per_reg;
>>> + unsigned int reg_idx;
>>> + unsigned int reg_shift;
>>> +
>>> + if ( smccc_is_conv_64(resp->a0) )
>>> + ids_per_reg = 4;
>>> + else
>>> + ids_per_reg = 2;
>>> +
>>> + reg_idx = n / ids_per_reg + 3;
>>> + reg_shift = ( n % ids_per_reg ) * 16;
>>> +
>>> + switch ( reg_idx )
>>> + {
>>> + case 3:
>>> + return resp->a3 >> reg_shift;
>>> + case 4:
>>> + return resp->a4 >> reg_shift;
>>> + case 5:
>>> + return resp->a5 >> reg_shift;
>>> + case 6:
>>> + return resp->a6 >> reg_shift;
>>> + case 7:
>>> + return resp->a7 >> reg_shift;
>>> + default:
>>> + ASSERT(0); /* "Can't happen" */
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> +static void notif_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;
>>> +
>>
>> If a notification is pending for a secure partition at this stage we are not
>> signaling anything so I think this fact should be listed in the limitations to
>> say that we do not signal any secondary scheduler if a notification is
>> pending for a secure partition.
>
> I agree, I'll add a note in the limitation.
>
>>
>>> + 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], GUEST_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);
>>> +}
>>> +
>>> +struct notif_irq_info {
>>> + unsigned int irq;
>>> + int ret;
>>> + struct irqaction *action;
>>> +};
>>> +
>>> +static void notif_irq_enable(void *info)
>>> +{
>>> + struct notif_irq_info *irq_info = info;
>>> +
>>> + irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
>>> + if ( irq_info->ret )
>>> + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
>>> + irq_info->irq, irq_info->ret);
>>> +}
>>> +
>>> +void ffa_notif_init(void)
>>> +{
>>> + const struct arm_smccc_1_2_regs arg = {
>>> + .a0 = FFA_FEATURES,
>>> + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
>>> + };
>>> + struct notif_irq_info irq_info = { };
>>> + struct arm_smccc_1_2_regs resp;
>>> + unsigned int cpu;
>>> +
>>> + arm_smccc_1_2_smc(&arg, &resp);
>>> + if ( resp.a0 != FFA_SUCCESS_32 )
>>> + return;
>>> +
>>> + irq_info.irq = resp.a2;
>>> + if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
>>> + {
>>> + printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
>>> + irq_info.irq);
>>> + return;
>>> + }
>>> +
>>> + /*
>>> + * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
>>> + * IPI to call notif_irq_enable() on each CPU including the current
>>> + * CPU. The struct irqaction is preallocated since we can't allocate
>>> + * memory while in interrupt context.
>>> + */
>>> + for_each_online_cpu(cpu)
>>> + {
>>> + irq_info.action = xmalloc(struct irqaction);
>>
>> You allocate one action per cpu but you have only one action pointer in your structure
>> which means you will overload the previously allocated one and lose track.
>>
>> You should have a table of actions in your structure instead unless one action is
>> enough and can be reused on all cpus and in this case you should move out of
>> your loop the allocation part.
>
> That shouldn't be needed because this is done in sequence only one CPU
> at a time.
Sorry i do not understand here.
You have a loop over each online cpu and on each loop you are assigning
irq_info.action with a newly allocated struct irqaction so you are in practice
overloading on cpu 2 the action that was allocated for cpu 1.
What do you mean by sequence here ?
Cheers
Bertrand
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-26 12:19 ` Bertrand Marquis
@ 2024-04-26 12:32 ` Jens Wiklander
2024-04-26 12:41 ` Bertrand Marquis
0 siblings, 1 reply; 22+ messages in thread
From: Jens Wiklander @ 2024-04-26 12:32 UTC (permalink / raw)
To: Bertrand Marquis
Cc: Xen-devel, patches, Stefano Stabellini, Julien Grall,
Michal Orzel, Volodymyr Babchuk
Hi Bertrand,
On Fri, Apr 26, 2024 at 2:19 PM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Jens,
>
> > On 26 Apr 2024, at 14:11, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Bertrand,
> >
> > On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
> > <Bertrand.Marquis@arm.com> wrote:
> >>
> >> Hi Jens,
> >>
> >>> On 26 Apr 2024, at 10:47, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >>>
[...]
> >>> +struct notif_irq_info {
> >>> + unsigned int irq;
> >>> + int ret;
> >>> + struct irqaction *action;
> >>> +};
> >>> +
> >>> +static void notif_irq_enable(void *info)
> >>> +{
> >>> + struct notif_irq_info *irq_info = info;
> >>> +
> >>> + irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
> >>> + if ( irq_info->ret )
> >>> + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> >>> + irq_info->irq, irq_info->ret);
> >>> +}
> >>> +
> >>> +void ffa_notif_init(void)
> >>> +{
> >>> + const struct arm_smccc_1_2_regs arg = {
> >>> + .a0 = FFA_FEATURES,
> >>> + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> >>> + };
> >>> + struct notif_irq_info irq_info = { };
> >>> + struct arm_smccc_1_2_regs resp;
> >>> + unsigned int cpu;
> >>> +
> >>> + arm_smccc_1_2_smc(&arg, &resp);
> >>> + if ( resp.a0 != FFA_SUCCESS_32 )
> >>> + return;
> >>> +
> >>> + irq_info.irq = resp.a2;
> >>> + if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
> >>> + {
> >>> + printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
> >>> + irq_info.irq);
> >>> + return;
> >>> + }
> >>> +
> >>> + /*
> >>> + * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> >>> + * IPI to call notif_irq_enable() on each CPU including the current
> >>> + * CPU. The struct irqaction is preallocated since we can't allocate
> >>> + * memory while in interrupt context.
> >>> + */
> >>> + for_each_online_cpu(cpu)
> >>> + {
> >>> + irq_info.action = xmalloc(struct irqaction);
> >>
> >> You allocate one action per cpu but you have only one action pointer in your structure
> >> which means you will overload the previously allocated one and lose track.
> >>
> >> You should have a table of actions in your structure instead unless one action is
> >> enough and can be reused on all cpus and in this case you should move out of
> >> your loop the allocation part.
> >
> > That shouldn't be needed because this is done in sequence only one CPU
> > at a time.
>
> Sorry i do not understand here.
> You have a loop over each online cpu and on each loop you are assigning
> irq_info.action with a newly allocated struct irqaction so you are in practice
> overloading on cpu 2 the action that was allocated for cpu 1.
>
> What do you mean by sequence here ?
>
My understanding is that for_each_online_cpu(cpu) loops over each cpu,
one at a time. The call
on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
returns after notif_irq_enable() has returned on the CPU in question
thanks to the "1" (wait) parameter. So once it has returned &irq_info
isn't used by the other CPU any longer and we can assign a new value
to irq_info.action.
Thanks,
Jens
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-26 12:32 ` Jens Wiklander
@ 2024-04-26 12:41 ` Bertrand Marquis
2024-04-26 13:02 ` Jens Wiklander
0 siblings, 1 reply; 22+ messages in thread
From: Bertrand Marquis @ 2024-04-26 12:41 UTC (permalink / raw)
To: Jens Wiklander
Cc: Xen-devel, patches, Stefano Stabellini, Julien Grall,
Michal Orzel, Volodymyr Babchuk
Hi Jens,
> On 26 Apr 2024, at 14:32, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Bertrand,
>
> On Fri, Apr 26, 2024 at 2:19 PM Bertrand Marquis
> <Bertrand.Marquis@arm.com> wrote:
>>
>> Hi Jens,
>>
>>> On 26 Apr 2024, at 14:11, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>>
>>> Hi Bertrand,
>>>
>>> On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
>>> <Bertrand.Marquis@arm.com> wrote:
>>>>
>>>> Hi Jens,
>>>>
>>>>> On 26 Apr 2024, at 10:47, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>>>>
> [...]
>>>>> +struct notif_irq_info {
>>>>> + unsigned int irq;
>>>>> + int ret;
>>>>> + struct irqaction *action;
>>>>> +};
>>>>> +
>>>>> +static void notif_irq_enable(void *info)
>>>>> +{
>>>>> + struct notif_irq_info *irq_info = info;
>>>>> +
>>>>> + irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
>>>>> + if ( irq_info->ret )
>>>>> + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
>>>>> + irq_info->irq, irq_info->ret);
>>>>> +}
>>>>> +
>>>>> +void ffa_notif_init(void)
>>>>> +{
>>>>> + const struct arm_smccc_1_2_regs arg = {
>>>>> + .a0 = FFA_FEATURES,
>>>>> + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
>>>>> + };
>>>>> + struct notif_irq_info irq_info = { };
>>>>> + struct arm_smccc_1_2_regs resp;
>>>>> + unsigned int cpu;
>>>>> +
>>>>> + arm_smccc_1_2_smc(&arg, &resp);
>>>>> + if ( resp.a0 != FFA_SUCCESS_32 )
>>>>> + return;
>>>>> +
>>>>> + irq_info.irq = resp.a2;
>>>>> + if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
>>>>> + {
>>>>> + printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
>>>>> + irq_info.irq);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + /*
>>>>> + * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
>>>>> + * IPI to call notif_irq_enable() on each CPU including the current
>>>>> + * CPU. The struct irqaction is preallocated since we can't allocate
>>>>> + * memory while in interrupt context.
>>>>> + */
>>>>> + for_each_online_cpu(cpu)
>>>>> + {
>>>>> + irq_info.action = xmalloc(struct irqaction);
>>>>
>>>> You allocate one action per cpu but you have only one action pointer in your structure
>>>> which means you will overload the previously allocated one and lose track.
>>>>
>>>> You should have a table of actions in your structure instead unless one action is
>>>> enough and can be reused on all cpus and in this case you should move out of
>>>> your loop the allocation part.
>>>
>>> That shouldn't be needed because this is done in sequence only one CPU
>>> at a time.
>>
>> Sorry i do not understand here.
>> You have a loop over each online cpu and on each loop you are assigning
>> irq_info.action with a newly allocated struct irqaction so you are in practice
>> overloading on cpu 2 the action that was allocated for cpu 1.
>>
>> What do you mean by sequence here ?
>>
>
> My understanding is that for_each_online_cpu(cpu) loops over each cpu,
> one at a time. The call
> on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
> returns after notif_irq_enable() has returned on the CPU in question
> thanks to the "1" (wait) parameter. So once it has returned &irq_info
> isn't used by the other CPU any longer and we can assign a new value
> to irq_info.action.
Right so you loose track of what was assigned so you are not able to
free it.
If that is wanted then why saving this in irq.action as you will only have
there the one allocated for the last online cpu.
Cheers
Bertrand
>
> Thanks,
> Jens
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-26 12:41 ` Bertrand Marquis
@ 2024-04-26 13:02 ` Jens Wiklander
2024-04-26 15:12 ` Bertrand Marquis
0 siblings, 1 reply; 22+ messages in thread
From: Jens Wiklander @ 2024-04-26 13:02 UTC (permalink / raw)
To: Bertrand Marquis
Cc: Xen-devel, patches, Stefano Stabellini, Julien Grall,
Michal Orzel, Volodymyr Babchuk
On Fri, Apr 26, 2024 at 2:41 PM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Jens,
>
> > On 26 Apr 2024, at 14:32, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Bertrand,
> >
> > On Fri, Apr 26, 2024 at 2:19 PM Bertrand Marquis
> > <Bertrand.Marquis@arm.com> wrote:
> >>
> >> Hi Jens,
> >>
> >>> On 26 Apr 2024, at 14:11, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >>>
> >>> Hi Bertrand,
> >>>
> >>> On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
> >>> <Bertrand.Marquis@arm.com> wrote:
> >>>>
> >>>> Hi Jens,
> >>>>
> >>>>> On 26 Apr 2024, at 10:47, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >>>>>
> > [...]
> >>>>> +struct notif_irq_info {
> >>>>> + unsigned int irq;
> >>>>> + int ret;
> >>>>> + struct irqaction *action;
> >>>>> +};
> >>>>> +
> >>>>> +static void notif_irq_enable(void *info)
> >>>>> +{
> >>>>> + struct notif_irq_info *irq_info = info;
> >>>>> +
> >>>>> + irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
> >>>>> + if ( irq_info->ret )
> >>>>> + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> >>>>> + irq_info->irq, irq_info->ret);
> >>>>> +}
> >>>>> +
> >>>>> +void ffa_notif_init(void)
> >>>>> +{
> >>>>> + const struct arm_smccc_1_2_regs arg = {
> >>>>> + .a0 = FFA_FEATURES,
> >>>>> + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> >>>>> + };
> >>>>> + struct notif_irq_info irq_info = { };
> >>>>> + struct arm_smccc_1_2_regs resp;
> >>>>> + unsigned int cpu;
> >>>>> +
> >>>>> + arm_smccc_1_2_smc(&arg, &resp);
> >>>>> + if ( resp.a0 != FFA_SUCCESS_32 )
> >>>>> + return;
> >>>>> +
> >>>>> + irq_info.irq = resp.a2;
> >>>>> + if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
> >>>>> + {
> >>>>> + printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
> >>>>> + irq_info.irq);
> >>>>> + return;
> >>>>> + }
> >>>>> +
> >>>>> + /*
> >>>>> + * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> >>>>> + * IPI to call notif_irq_enable() on each CPU including the current
> >>>>> + * CPU. The struct irqaction is preallocated since we can't allocate
> >>>>> + * memory while in interrupt context.
> >>>>> + */
> >>>>> + for_each_online_cpu(cpu)
> >>>>> + {
> >>>>> + irq_info.action = xmalloc(struct irqaction);
> >>>>
> >>>> You allocate one action per cpu but you have only one action pointer in your structure
> >>>> which means you will overload the previously allocated one and lose track.
> >>>>
> >>>> You should have a table of actions in your structure instead unless one action is
> >>>> enough and can be reused on all cpus and in this case you should move out of
> >>>> your loop the allocation part.
> >>>
> >>> That shouldn't be needed because this is done in sequence only one CPU
> >>> at a time.
> >>
> >> Sorry i do not understand here.
> >> You have a loop over each online cpu and on each loop you are assigning
> >> irq_info.action with a newly allocated struct irqaction so you are in practice
> >> overloading on cpu 2 the action that was allocated for cpu 1.
> >>
> >> What do you mean by sequence here ?
> >>
> >
> > My understanding is that for_each_online_cpu(cpu) loops over each cpu,
> > one at a time. The call
> > on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
> > returns after notif_irq_enable() has returned on the CPU in question
> > thanks to the "1" (wait) parameter. So once it has returned &irq_info
> > isn't used by the other CPU any longer and we can assign a new value
> > to irq_info.action.
>
> Right so you loose track of what was assigned so you are not able to
> free it.
> If that is wanted then why saving this in irq.action as you will only have
> there the one allocated for the last online cpu.
Wouldn't release_irq() free it? An error here is unlikely, but we may
be left with a few installed struct irqaction if it occurs. I can add
a more elaborate error path if it's worth the added complexity.
Thanks,
Jens
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-26 13:02 ` Jens Wiklander
@ 2024-04-26 15:12 ` Bertrand Marquis
0 siblings, 0 replies; 22+ messages in thread
From: Bertrand Marquis @ 2024-04-26 15:12 UTC (permalink / raw)
To: Jens Wiklander
Cc: Xen-devel, patches, Stefano Stabellini, Julien Grall,
Michal Orzel, Volodymyr Babchuk
Hi Jens,
> On 26 Apr 2024, at 15:02, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Fri, Apr 26, 2024 at 2:41 PM Bertrand Marquis
> <Bertrand.Marquis@arm.com> wrote:
>>
>> Hi Jens,
>>
>>> On 26 Apr 2024, at 14:32, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>>
>>> Hi Bertrand,
>>>
>>> On Fri, Apr 26, 2024 at 2:19 PM Bertrand Marquis
>>> <Bertrand.Marquis@arm.com> wrote:
>>>>
>>>> Hi Jens,
>>>>
>>>>> On 26 Apr 2024, at 14:11, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>>>>
>>>>> Hi Bertrand,
>>>>>
>>>>> On Fri, Apr 26, 2024 at 11:20 AM Bertrand Marquis
>>>>> <Bertrand.Marquis@arm.com> wrote:
>>>>>>
>>>>>> Hi Jens,
>>>>>>
>>>>>>> On 26 Apr 2024, at 10:47, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>>>>>>
>>> [...]
>>>>>>> +struct notif_irq_info {
>>>>>>> + unsigned int irq;
>>>>>>> + int ret;
>>>>>>> + struct irqaction *action;
>>>>>>> +};
>>>>>>> +
>>>>>>> +static void notif_irq_enable(void *info)
>>>>>>> +{
>>>>>>> + struct notif_irq_info *irq_info = info;
>>>>>>> +
>>>>>>> + irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
>>>>>>> + if ( irq_info->ret )
>>>>>>> + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
>>>>>>> + irq_info->irq, irq_info->ret);
>>>>>>> +}
>>>>>>> +
>>>>>>> +void ffa_notif_init(void)
>>>>>>> +{
>>>>>>> + const struct arm_smccc_1_2_regs arg = {
>>>>>>> + .a0 = FFA_FEATURES,
>>>>>>> + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
>>>>>>> + };
>>>>>>> + struct notif_irq_info irq_info = { };
>>>>>>> + struct arm_smccc_1_2_regs resp;
>>>>>>> + unsigned int cpu;
>>>>>>> +
>>>>>>> + arm_smccc_1_2_smc(&arg, &resp);
>>>>>>> + if ( resp.a0 != FFA_SUCCESS_32 )
>>>>>>> + return;
>>>>>>> +
>>>>>>> + irq_info.irq = resp.a2;
>>>>>>> + if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
>>>>>>> + {
>>>>>>> + printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
>>>>>>> + irq_info.irq);
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + /*
>>>>>>> + * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
>>>>>>> + * IPI to call notif_irq_enable() on each CPU including the current
>>>>>>> + * CPU. The struct irqaction is preallocated since we can't allocate
>>>>>>> + * memory while in interrupt context.
>>>>>>> + */
>>>>>>> + for_each_online_cpu(cpu)
>>>>>>> + {
>>>>>>> + irq_info.action = xmalloc(struct irqaction);
>>>>>>
>>>>>> You allocate one action per cpu but you have only one action pointer in your structure
>>>>>> which means you will overload the previously allocated one and lose track.
>>>>>>
>>>>>> You should have a table of actions in your structure instead unless one action is
>>>>>> enough and can be reused on all cpus and in this case you should move out of
>>>>>> your loop the allocation part.
>>>>>
>>>>> That shouldn't be needed because this is done in sequence only one CPU
>>>>> at a time.
>>>>
>>>> Sorry i do not understand here.
>>>> You have a loop over each online cpu and on each loop you are assigning
>>>> irq_info.action with a newly allocated struct irqaction so you are in practice
>>>> overloading on cpu 2 the action that was allocated for cpu 1.
>>>>
>>>> What do you mean by sequence here ?
>>>>
>>>
>>> My understanding is that for_each_online_cpu(cpu) loops over each cpu,
>>> one at a time. The call
>>> on_selected_cpus(cpumask_of(cpu), notif_irq_enable, &irq_info, 1);
>>> returns after notif_irq_enable() has returned on the CPU in question
>>> thanks to the "1" (wait) parameter. So once it has returned &irq_info
>>> isn't used by the other CPU any longer and we can assign a new value
>>> to irq_info.action.
>>
>> Right so you loose track of what was assigned so you are not able to
>> free it.
>> If that is wanted then why saving this in irq.action as you will only have
>> there the one allocated for the last online cpu.
>
> Wouldn't release_irq() free it? An error here is unlikely, but we may
> be left with a few installed struct irqaction if it occurs. I can add
> a more elaborate error path if it's worth the added complexity.
I think just add a comment saying that the irqaction will be freed
upon release_irq so we do not keep a reference to it or something
like that and this will be ok.
The code is in fact a bit misleading because the irqaction is used
inside the function called on other cores through the IPI and there
you actually pass the action. Your structure is only there to transport
the information for the IPI handler.
So please add a comment on top of the notif_irq_info to say that
this structure is used to pass information to and back the notif_irq_enable
executed using an IPI on other cores.
Cheers
Bertrand
>
> Thanks,
> Jens
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-26 8:47 ` [XEN PATCH v3 5/5] xen/arm: ffa: support notification Jens Wiklander
2024-04-26 9:20 ` Bertrand Marquis
@ 2024-04-26 17:58 ` Julien Grall
2024-04-29 9:55 ` Jens Wiklander
2024-04-26 19:07 ` Julien Grall
2 siblings, 1 reply; 22+ messages in thread
From: Julien Grall @ 2024-04-26 17:58 UTC (permalink / raw)
To: Jens Wiklander, xen-devel
Cc: patches, Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Hi Jens,
On 26/04/2024 09:47, Jens Wiklander wrote:
> +static void notif_irq_enable(void *info)
> +{
> + struct notif_irq_info *irq_info = info;
> +
> + irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
In v2, you were using request_irq(). But now you seem to be open-coding
it. Can you explain why?
> + if ( irq_info->ret )
> + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> + irq_info->irq, irq_info->ret);
> +}
> +
> +void ffa_notif_init(void)
> +{
> + const struct arm_smccc_1_2_regs arg = {
> + .a0 = FFA_FEATURES,
> + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> + };
> + struct notif_irq_info irq_info = { };
> + struct arm_smccc_1_2_regs resp;
> + unsigned int cpu;
> +
> + arm_smccc_1_2_smc(&arg, &resp);
> + if ( resp.a0 != FFA_SUCCESS_32 )
> + return;
> +
> + irq_info.irq = resp.a2;
> + if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
> + {
> + printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
> + irq_info.irq);
> + return;
> + }
> +
> + /*
> + * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> + * IPI to call notif_irq_enable() on each CPU including the current
> + * CPU. The struct irqaction is preallocated since we can't allocate
> + * memory while in interrupt context.
> + */
> + for_each_online_cpu(cpu)
Even though we currently don't support CPU hotplug, you want to add a
CPU Notifier to also register the IRQ when a CPU is onlined
ffa_notif_init().
For an example, see time.c. We may also want to consider to enable TEE
in presmp_initcalls() so we don't need to have a for_each_online_cpu().
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-26 9:20 ` Bertrand Marquis
2024-04-26 12:11 ` Jens Wiklander
@ 2024-04-26 18:31 ` Julien Grall
1 sibling, 0 replies; 22+ messages in thread
From: Julien Grall @ 2024-04-26 18:31 UTC (permalink / raw)
To: Bertrand Marquis, Jens Wiklander
Cc: Xen-devel, patches, Stefano Stabellini, Michal Orzel, Volodymyr Babchuk
Hi Bertrand,
On 26/04/2024 10:20, Bertrand Marquis wrote:
>> +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);
>> +}
>> +
>
> I think there is a global discussion to have on using get_domain_by_vm_id
> or rcu_lock_live_remote_domain_by_id to make sure the domain is not
> dying when we try to do something with it.
>
> It does not need to be done here as there are other places to adapt but
> i wanted to raise the question.
>
> I would like to know what you and also other maintainers think here.
> @Julien/Michal/Stefano ?
Good question. I think the intention is get_domain_by_id() should be
called when you need a reference for longer.
I would consider to involve the REST in this discussion.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-26 8:47 ` [XEN PATCH v3 5/5] xen/arm: ffa: support notification Jens Wiklander
2024-04-26 9:20 ` Bertrand Marquis
2024-04-26 17:58 ` Julien Grall
@ 2024-04-26 19:07 ` Julien Grall
2024-04-29 7:20 ` Bertrand Marquis
2024-04-29 8:43 ` Jens Wiklander
2 siblings, 2 replies; 22+ messages in thread
From: Julien Grall @ 2024-04-26 19:07 UTC (permalink / raw)
To: Jens Wiklander, xen-devel
Cc: patches, Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Hi Jens,
On 26/04/2024 09:47, Jens Wiklander wrote:
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index d7306aa64d50..5224898265a5 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -155,7 +155,7 @@ void __init init_IRQ(void)
> {
> /* SGIs are always edge-triggered */
> if ( irq < NR_GIC_SGI )
> - local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
> + local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
This changes seems unrelated to this patch. This wants to be separate
(if you actually intended to change it).
> else
> local_irqs_type[irq] = IRQ_TYPE_INVALID;
> }
> diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
> index f0112a2f922d..7c0f46f7f446 100644
> --- a/xen/arch/arm/tee/Makefile
> +++ b/xen/arch/arm/tee/Makefile
> @@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
> obj-$(CONFIG_FFA) += ffa_shm.o
> obj-$(CONFIG_FFA) += ffa_partinfo.o
> obj-$(CONFIG_FFA) += ffa_rxtx.o
> +obj-$(CONFIG_FFA) += ffa_notif.o
> obj-y += tee.o
> obj-$(CONFIG_OPTEE) += optee.o
> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> index 5209612963e1..912e905a27bd 100644
> --- a/xen/arch/arm/tee/ffa.c
> +++ b/xen/arch/arm/tee/ffa.c
> @@ -39,6 +39,9 @@
> * - at most 32 shared memory regions per guest
> * o FFA_MSG_SEND_DIRECT_REQ:
> * - only supported from a VM to an SP
> + * o FFA_NOTIFICATION_*:
> + * - only supports global notifications, that is, per vCPU notifications
> + * are not supported
> *
> * There are some large locked sections with ffa_tx_buffer_lock and
> * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
> @@ -194,6 +197,8 @@ out:
>
> static void handle_features(struct cpu_user_regs *regs)
> {
> + struct domain *d = current->domain;
> + struct ffa_ctx *ctx = d->arch.tee;
> uint32_t a1 = get_user_reg(regs, 1);
> unsigned int n;
>
> @@ -240,6 +245,30 @@ static void handle_features(struct cpu_user_regs *regs)
> BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
> ffa_set_regs_success(regs, 0, 0);
> break;
> + case FFA_FEATURE_NOTIF_PEND_INTR:
> + if ( ctx->notif.enabled )
> + ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
> + else
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + break;
> + case FFA_FEATURE_SCHEDULE_RECV_INTR:
> + if ( ctx->notif.enabled )
> + ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
> + else
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + break;
> +
> + case FFA_NOTIFICATION_BIND:
> + case FFA_NOTIFICATION_UNBIND:
> + case FFA_NOTIFICATION_GET:
> + case FFA_NOTIFICATION_SET:
> + case FFA_NOTIFICATION_INFO_GET_32:
> + case FFA_NOTIFICATION_INFO_GET_64:
> + if ( ctx->notif.enabled )
> + ffa_set_regs_success(regs, 0, 0);
> + else
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + break;
> default:
> ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> break;
> @@ -305,6 +334,22 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> get_user_reg(regs, 1)),
> get_user_reg(regs, 3));
> break;
> + case FFA_NOTIFICATION_BIND:
> + e = ffa_handle_notification_bind(regs);
> + break;
> + case FFA_NOTIFICATION_UNBIND:
> + e = ffa_handle_notification_unbind(regs);
> + break;
> + case FFA_NOTIFICATION_INFO_GET_32:
> + case FFA_NOTIFICATION_INFO_GET_64:
> + ffa_handle_notification_info_get(regs);
> + return true;
> + case FFA_NOTIFICATION_GET:
> + ffa_handle_notification_get(regs);
> + return true;
> + case FFA_NOTIFICATION_SET:
> + e = ffa_handle_notification_set(regs);
> + break;
>
> default:
> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> @@ -322,6 +367,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> static int ffa_domain_init(struct domain *d)
> {
> struct ffa_ctx *ctx;
> + int ret;
>
> if ( !ffa_version )
> return -ENODEV;
> @@ -345,10 +391,11 @@ static int ffa_domain_init(struct domain *d)
> * error, so no need for cleanup in this function.
> */
>
> - if ( !ffa_partinfo_domain_init(d) )
> - return -EIO;
> + ret = ffa_partinfo_domain_init(d);
> + if ( ret )
> + return ret;
>
> - return 0;
> + return ffa_notif_domain_init(d);
> }
>
> static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
> @@ -423,6 +470,7 @@ static int ffa_domain_teardown(struct domain *d)
> return 0;
>
> ffa_rxtx_domain_destroy(d);
> + ffa_notif_domain_destroy(d);
>
> ffa_domain_teardown_continue(ctx, true /* first_time */);
>
> @@ -502,6 +550,7 @@ static bool ffa_probe(void)
> if ( !ffa_partinfo_init() )
> goto err_rxtx_destroy;
>
> + ffa_notif_init();
> INIT_LIST_HEAD(&ffa_teardown_head);
> init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
>
> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
> new file mode 100644
> index 000000000000..6bb0804ee2f8
> --- /dev/null
> +++ b/xen/arch/arm/tee/ffa_notif.c
> @@ -0,0 +1,378 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 Linaro Limited
> + */
> +
> +#include <xen/const.h>
> +#include <xen/list.h>
> +#include <xen/spinlock.h>
> +#include <xen/types.h>
> +
> +#include <asm/smccc.h>
> +#include <asm/regs.h>
> +
> +#include "ffa_private.h"
> +
> +static bool __ro_after_init notif_enabled;
> +
> +int ffa_handle_notification_bind(struct cpu_user_regs *regs)
> +{
> + struct domain *d = current->domain;
> + uint32_t src_dst = get_user_reg(regs, 1);
> + uint32_t flags = get_user_reg(regs, 2);
> + uint32_t bitmap_lo = get_user_reg(regs, 3);
> + uint32_t bitmap_hi = get_user_reg(regs, 4);
> +
> + if ( !notif_enabled )
> + return FFA_RET_NOT_SUPPORTED;
> +
> + if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> + return FFA_RET_INVALID_PARAMETERS;
> +
> + if ( flags ) /* Only global notifications are supported */
> + return FFA_RET_DENIED;
> +
> + /*
> + * We only support notifications from SP so no need to check the sender
> + * endpoint ID, the SPMC will take care of that for us.
> + */
> + return ffa_simple_call(FFA_NOTIFICATION_BIND, src_dst, flags, bitmap_hi,
> + bitmap_lo);
> +}
> +
> +int ffa_handle_notification_unbind(struct cpu_user_regs *regs)
> +{
> + struct domain *d = current->domain;
> + uint32_t src_dst = get_user_reg(regs, 1);
> + uint32_t bitmap_lo = get_user_reg(regs, 3);
> + uint32_t bitmap_hi = get_user_reg(regs, 4);
> +
> + if ( !notif_enabled )
> + return FFA_RET_NOT_SUPPORTED;
> +
> + if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> + return FFA_RET_INVALID_PARAMETERS;
> +
> + /*
> + * We only support notifications from SP so no need to check the
> + * destination endpoint ID, the SPMC will take care of that for us.
> + */
> + return ffa_simple_call(FFA_NOTIFICATION_UNBIND, src_dst, 0, bitmap_hi,
> + bitmap_lo);
> +}
> +
> +void ffa_handle_notification_info_get(struct cpu_user_regs *regs)
> +{
> + struct domain *d = current->domain;
> + struct ffa_ctx *ctx = d->arch.tee;
> + bool pending_global;
> +
> + if ( !notif_enabled )
> + {
> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> + return;
> + }
> +
> + spin_lock(&ctx->notif.lock);
> + pending_global = ctx->notif.secure_pending;
> + ctx->notif.secure_pending = false;
> + spin_unlock(&ctx->notif.lock);
> +
> + if ( pending_global )
> + {
> + /* A pending global notification for the guest */
> + ffa_set_regs(regs, FFA_SUCCESS_64, 0,
> + 1U << FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT, ffa_get_vm_id(d),
> + 0, 0, 0, 0);
> + }
> + else
> + {
> + /* Report an error if there where no pending global notification */
> + ffa_set_regs_error(regs, FFA_RET_NO_DATA);
> + }
> +}
> +static void notif_irq_handler(int irq, void *data)
> +{
> + const struct arm_smccc_1_2_regs arg = {
> + .a0 = FFA_NOTIFICATION_INFO_GET_64,
> + };
> + struct arm_smccc_1_2_regs resp;
> + unsigned int id_pos;
> + unsigned int list_count;
> + uint64_t ids_count;
> + unsigned int n;
> + int32_t res;
> +
> + do {
> + arm_smccc_1_2_smc(&arg, &resp);
> + res = ffa_get_ret_code(&resp);
> + if ( res )
> + {
> + if ( res != FFA_RET_NO_DATA )
> + printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
> + res);
> + return;
> + }
> +
> + ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
> + list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
> + FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
> +
> + id_pos = 0;
> + for ( n = 0; n < list_count; n++ )
> + {
> + unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
> + struct domain *d;
> +
> + d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));
Thinking a bit more about the question from Bertrand about
get_domain_id() vs rcu_lock_domain_by_id(). I am actually not sure
whether either are ok here.
If I am not mistaken, d->arch.tee will be freed as part of the domain
teardown process. This means you can have the following scenario:
CPU0: ffa_get_domain_by_vm_id() (return the domain as it is alive)
CPU1: call domain_kill()
CPU1: teardown is called, free d->arch.tee (the pointer is not set to NULL)
d->arch.tee is now a dangling pointer
CPU0: access d->arch.tee
This implies you may need to gain a global lock (I don't have a better
idea so far) to protect the IRQ handler against domains teardown.
Did I miss anything?
> +
> + if ( d )
> + {
> + struct ffa_ctx *ctx = d->arch.tee;
> +
> + spin_lock(&ctx->notif.lock);
> + ctx->notif.secure_pending = true;
> + spin_unlock(&ctx->notif.lock);
AFAICT, the spinlock is used with IRQ enabled (see
ffa_handle_notification_info_get()) but also in an IRQ handler. So to
prevent deadlock, you will want to use spin_lock_irq* helpers.
That said, I don't think you need a spin_lock(). You could use atomic
operations instead. For the first hunk, you could use
test_and_clear_bool(). E.g.:
if ( test_and_clear_bool(ctx.notif.secure_pending) )
For the second part, it might be fine to use ACCESS_ONCE().
> +
> + /*
> + * Since we're only delivering global notification, always
> + * deliver to the first vCPU. It doesn't matter which we
> + * chose, as long as it's available.
What if vCPU0 is offlined?
> + */
> + vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID,
> + true);
> +
> + put_domain(d);
> + }
> +
> + id_pos += count;
> + }
> +
> + } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
> +}
[..]
> +struct ffa_ctx_notif {
> + bool enabled;
> +
> + /* Used to serialize access to the rest of this struct */
> + spinlock_t lock;
Regardless what I wrote above, I can't seem to find a call to
spin_lock_init(). You will want it to initialize.
Also, it would be best if we keep the two booleans close to each other
so we limit the amount of padding in the structure.
> +
> + /*
> + * True if domain is reported by FFA_NOTIFICATION_INFO_GET to have
> + * pending global notifications.
> + */
> + bool secure_pending;
> +};
>
> struct ffa_ctx {
> void *rx;
> @@ -228,6 +265,7 @@ struct ffa_ctx {
> struct list_head shm_list;
> /* Number of allocated shared memory object */
> unsigned int shm_count;
> + struct ffa_ctx_notif notif;
> /*
> * tx_lock is used to serialize access to tx
> * rx_lock is used to serialize access to rx
> @@ -257,7 +295,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs);
> int ffa_handle_mem_reclaim(uint64_t handle, uint32_t flags);
>
> bool ffa_partinfo_init(void);
> -bool ffa_partinfo_domain_init(struct domain *d);
> +int ffa_partinfo_domain_init(struct domain *d);
> bool ffa_partinfo_domain_destroy(struct domain *d);
> int32_t ffa_handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
> uint32_t w4, uint32_t w5, uint32_t *count,
> @@ -271,12 +309,28 @@ uint32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
> uint32_t ffa_handle_rxtx_unmap(void);
> int32_t ffa_handle_rx_release(void);
>
> +void ffa_notif_init(void);
> +int ffa_notif_domain_init(struct domain *d);
> +void ffa_notif_domain_destroy(struct domain *d);
> +
> +int ffa_handle_notification_bind(struct cpu_user_regs *regs);
> +int ffa_handle_notification_unbind(struct cpu_user_regs *regs);
> +void ffa_handle_notification_info_get(struct cpu_user_regs *regs);
> +void ffa_handle_notification_get(struct cpu_user_regs *regs);
> +int ffa_handle_notification_set(struct cpu_user_regs *regs);
> +
> static inline uint16_t ffa_get_vm_id(const struct domain *d)
> {
> /* +1 since 0 is reserved for the hypervisor in FF-A */
> return d->domain_id + 1;
> }
>
> +static inline struct domain *ffa_get_domain_by_vm_id(uint16_t vm_id)
> +{
Is this expected to be called with vm_id == 0? If not, then I would
consider to add an ASSERT(vm_id != 0). If yes, then I please add a
runtime check and return NULL.
> + /* -1 to match ffa_get_vm_id() */
> + return get_domain_by_id(vm_id - 1);
> +}
> +
> static inline void ffa_set_regs(struct cpu_user_regs *regs, register_t v0,
> register_t v1, register_t v2, register_t v3,
> register_t v4, register_t v5, register_t v6,
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-26 19:07 ` Julien Grall
@ 2024-04-29 7:20 ` Bertrand Marquis
2024-04-29 9:49 ` Jens Wiklander
2024-04-29 20:55 ` Julien Grall
2024-04-29 8:43 ` Jens Wiklander
1 sibling, 2 replies; 22+ messages in thread
From: Bertrand Marquis @ 2024-04-29 7:20 UTC (permalink / raw)
To: Julien Grall
Cc: Jens Wiklander, Xen-devel, patches, Stefano Stabellini,
Michal Orzel, Volodymyr Babchuk
Hi Julien,
> On 26 Apr 2024, at 21:07, Julien Grall <julien@xen.org> wrote:
>
> Hi Jens,
>
> On 26/04/2024 09:47, Jens Wiklander wrote:
>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>> index d7306aa64d50..5224898265a5 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -155,7 +155,7 @@ void __init init_IRQ(void)
>> {
>> /* SGIs are always edge-triggered */
>> if ( irq < NR_GIC_SGI )
>> - local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
>> + local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
>
> This changes seems unrelated to this patch. This wants to be separate (if you actually intended to change it).
>
>> else
>> local_irqs_type[irq] = IRQ_TYPE_INVALID;
>> }
>> diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
>> index f0112a2f922d..7c0f46f7f446 100644
>> --- a/xen/arch/arm/tee/Makefile
>> +++ b/xen/arch/arm/tee/Makefile
>> @@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
>> obj-$(CONFIG_FFA) += ffa_shm.o
>> obj-$(CONFIG_FFA) += ffa_partinfo.o
>> obj-$(CONFIG_FFA) += ffa_rxtx.o
>> +obj-$(CONFIG_FFA) += ffa_notif.o
>> obj-y += tee.o
>> obj-$(CONFIG_OPTEE) += optee.o
>> diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
>> index 5209612963e1..912e905a27bd 100644
>> --- a/xen/arch/arm/tee/ffa.c
>> +++ b/xen/arch/arm/tee/ffa.c
>> @@ -39,6 +39,9 @@
>> * - at most 32 shared memory regions per guest
>> * o FFA_MSG_SEND_DIRECT_REQ:
>> * - only supported from a VM to an SP
>> + * o FFA_NOTIFICATION_*:
>> + * - only supports global notifications, that is, per vCPU notifications
>> + * are not supported
>> *
>> * There are some large locked sections with ffa_tx_buffer_lock and
>> * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
>> @@ -194,6 +197,8 @@ out:
>> static void handle_features(struct cpu_user_regs *regs)
>> {
>> + struct domain *d = current->domain;
>> + struct ffa_ctx *ctx = d->arch.tee;
>> uint32_t a1 = get_user_reg(regs, 1);
>> unsigned int n;
>> @@ -240,6 +245,30 @@ static void handle_features(struct cpu_user_regs *regs)
>> BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
>> ffa_set_regs_success(regs, 0, 0);
>> break;
>> + case FFA_FEATURE_NOTIF_PEND_INTR:
>> + if ( ctx->notif.enabled )
>> + ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
>> + else
>> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>> + break;
>> + case FFA_FEATURE_SCHEDULE_RECV_INTR:
>> + if ( ctx->notif.enabled )
>> + ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
>> + else
>> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>> + break;
>> +
>> + case FFA_NOTIFICATION_BIND:
>> + case FFA_NOTIFICATION_UNBIND:
>> + case FFA_NOTIFICATION_GET:
>> + case FFA_NOTIFICATION_SET:
>> + case FFA_NOTIFICATION_INFO_GET_32:
>> + case FFA_NOTIFICATION_INFO_GET_64:
>> + if ( ctx->notif.enabled )
>> + ffa_set_regs_success(regs, 0, 0);
>> + else
>> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>> + break;
>> default:
>> ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>> break;
>> @@ -305,6 +334,22 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>> get_user_reg(regs, 1)),
>> get_user_reg(regs, 3));
>> break;
>> + case FFA_NOTIFICATION_BIND:
>> + e = ffa_handle_notification_bind(regs);
>> + break;
>> + case FFA_NOTIFICATION_UNBIND:
>> + e = ffa_handle_notification_unbind(regs);
>> + break;
>> + case FFA_NOTIFICATION_INFO_GET_32:
>> + case FFA_NOTIFICATION_INFO_GET_64:
>> + ffa_handle_notification_info_get(regs);
>> + return true;
>> + case FFA_NOTIFICATION_GET:
>> + ffa_handle_notification_get(regs);
>> + return true;
>> + case FFA_NOTIFICATION_SET:
>> + e = ffa_handle_notification_set(regs);
>> + break;
>> default:
>> gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
>> @@ -322,6 +367,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
>> static int ffa_domain_init(struct domain *d)
>> {
>> struct ffa_ctx *ctx;
>> + int ret;
>> if ( !ffa_version )
>> return -ENODEV;
>> @@ -345,10 +391,11 @@ static int ffa_domain_init(struct domain *d)
>> * error, so no need for cleanup in this function.
>> */
>> - if ( !ffa_partinfo_domain_init(d) )
>> - return -EIO;
>> + ret = ffa_partinfo_domain_init(d);
>> + if ( ret )
>> + return ret;
>> - return 0;
>> + return ffa_notif_domain_init(d);
>> }
>> static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
>> @@ -423,6 +470,7 @@ static int ffa_domain_teardown(struct domain *d)
>> return 0;
>> ffa_rxtx_domain_destroy(d);
>> + ffa_notif_domain_destroy(d);
>> ffa_domain_teardown_continue(ctx, true /* first_time */);
>> @@ -502,6 +550,7 @@ static bool ffa_probe(void)
>> if ( !ffa_partinfo_init() )
>> goto err_rxtx_destroy;
>> + ffa_notif_init();
>> INIT_LIST_HEAD(&ffa_teardown_head);
>> init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
>> diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
>> new file mode 100644
>> index 000000000000..6bb0804ee2f8
>> --- /dev/null
>> +++ b/xen/arch/arm/tee/ffa_notif.c
>> @@ -0,0 +1,378 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2024 Linaro Limited
>> + */
>> +
>> +#include <xen/const.h>
>> +#include <xen/list.h>
>> +#include <xen/spinlock.h>
>> +#include <xen/types.h>
>> +
>> +#include <asm/smccc.h>
>> +#include <asm/regs.h>
>> +
>> +#include "ffa_private.h"
>> +
>> +static bool __ro_after_init notif_enabled;
>> +
>> +int ffa_handle_notification_bind(struct cpu_user_regs *regs)
>> +{
>> + struct domain *d = current->domain;
>> + uint32_t src_dst = get_user_reg(regs, 1);
>> + uint32_t flags = get_user_reg(regs, 2);
>> + uint32_t bitmap_lo = get_user_reg(regs, 3);
>> + uint32_t bitmap_hi = get_user_reg(regs, 4);
>> +
>> + if ( !notif_enabled )
>> + return FFA_RET_NOT_SUPPORTED;
>> +
>> + if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
>> + return FFA_RET_INVALID_PARAMETERS;
>> +
>> + if ( flags ) /* Only global notifications are supported */
>> + return FFA_RET_DENIED;
>> +
>> + /*
>> + * We only support notifications from SP so no need to check the sender
>> + * endpoint ID, the SPMC will take care of that for us.
>> + */
>> + return ffa_simple_call(FFA_NOTIFICATION_BIND, src_dst, flags, bitmap_hi,
>> + bitmap_lo);
>> +}
>> +
>> +int ffa_handle_notification_unbind(struct cpu_user_regs *regs)
>> +{
>> + struct domain *d = current->domain;
>> + uint32_t src_dst = get_user_reg(regs, 1);
>> + uint32_t bitmap_lo = get_user_reg(regs, 3);
>> + uint32_t bitmap_hi = get_user_reg(regs, 4);
>> +
>> + if ( !notif_enabled )
>> + return FFA_RET_NOT_SUPPORTED;
>> +
>> + if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
>> + return FFA_RET_INVALID_PARAMETERS;
>> +
>> + /*
>> + * We only support notifications from SP so no need to check the
>> + * destination endpoint ID, the SPMC will take care of that for us.
>> + */
>> + return ffa_simple_call(FFA_NOTIFICATION_UNBIND, src_dst, 0, bitmap_hi,
>> + bitmap_lo);
>> +}
>> +
>> +void ffa_handle_notification_info_get(struct cpu_user_regs *regs)
>> +{
>> + struct domain *d = current->domain;
>> + struct ffa_ctx *ctx = d->arch.tee;
>> + bool pending_global;
>> +
>> + if ( !notif_enabled )
>> + {
>> + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
>> + return;
>> + }
>> +
>> + spin_lock(&ctx->notif.lock);
>> + pending_global = ctx->notif.secure_pending;
>> + ctx->notif.secure_pending = false;
>> + spin_unlock(&ctx->notif.lock);
>> +
>> + if ( pending_global )
>> + {
>> + /* A pending global notification for the guest */
>> + ffa_set_regs(regs, FFA_SUCCESS_64, 0,
>> + 1U << FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT, ffa_get_vm_id(d),
>> + 0, 0, 0, 0);
>> + }
>> + else
>> + {
>> + /* Report an error if there where no pending global notification */
>> + ffa_set_regs_error(regs, FFA_RET_NO_DATA);
>> + }
>> +}
>> +static void notif_irq_handler(int irq, void *data)
>> +{
>> + const struct arm_smccc_1_2_regs arg = {
>> + .a0 = FFA_NOTIFICATION_INFO_GET_64,
>> + };
>> + struct arm_smccc_1_2_regs resp;
>> + unsigned int id_pos;
>> + unsigned int list_count;
>> + uint64_t ids_count;
>> + unsigned int n;
>> + int32_t res;
>> +
>> + do {
>> + arm_smccc_1_2_smc(&arg, &resp);
>> + res = ffa_get_ret_code(&resp);
>> + if ( res )
>> + {
>> + if ( res != FFA_RET_NO_DATA )
>> + printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
>> + res);
>> + return;
>> + }
>> +
>> + ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
>> + list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
>> + FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
>> +
>> + id_pos = 0;
>> + for ( n = 0; n < list_count; n++ )
>> + {
>> + unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
>> + struct domain *d;
>> +
>> + d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));
>
> Thinking a bit more about the question from Bertrand about get_domain_id() vs rcu_lock_domain_by_id(). I am actually not sure whether either are ok here.
>
> If I am not mistaken, d->arch.tee will be freed as part of the domain teardown process. This means you can have the following scenario:
>
> CPU0: ffa_get_domain_by_vm_id() (return the domain as it is alive)
>
> CPU1: call domain_kill()
> CPU1: teardown is called, free d->arch.tee (the pointer is not set to NULL)
>
> d->arch.tee is now a dangling pointer
>
> CPU0: access d->arch.tee
>
> This implies you may need to gain a global lock (I don't have a better idea so far) to protect the IRQ handler against domains teardown.
>
> Did I miss anything?
I think you are right which is why I was thinking to use rcu_lock_live_remote_domain_by_id to only get a reference
to the domain if it is not dying.
From the comment in sched.h:
/*
* rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
* This is the preferred function if the returned domain reference
* is short lived, but it cannot be used if the domain reference needs
* to be kept beyond the current scope (e.g., across a softirq).
* The returned domain reference must be discarded using rcu_unlock_domain().
*/
Now the question of short lived should be challenged but I do not think we can
consider the current code as "long lived".
It would be a good idea to start a mailing list thread discussion with other
maintainers on the subject to confirm.
@Jens: as i will be off for some time, would you mind doing it ?
>
>> +
>> + if ( d )
>> + {
>> + struct ffa_ctx *ctx = d->arch.tee;
>> +
>> + spin_lock(&ctx->notif.lock);
>> + ctx->notif.secure_pending = true;
>> + spin_unlock(&ctx->notif.lock);
>
>
> AFAICT, the spinlock is used with IRQ enabled (see ffa_handle_notification_info_get()) but also in an IRQ handler. So to prevent deadlock, you will want to use spin_lock_irq* helpers.
>
> That said, I don't think you need a spin_lock(). You could use atomic operations instead. For the first hunk, you could use test_and_clear_bool(). E.g.:
>
> if ( test_and_clear_bool(ctx.notif.secure_pending) )
>
> For the second part, it might be fine to use ACCESS_ONCE().
>
>> +
>> + /*
>> + * Since we're only delivering global notification, always
>> + * deliver to the first vCPU. It doesn't matter which we
>> + * chose, as long as it's available.
>
> What if vCPU0 is offlined?
>
>> + */
>> + vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID,
>> + true);
>> +
>> + put_domain(d);
>> + }
>> +
>> + id_pos += count;
>> + }
>> +
>> + } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
>> +}
>
> [..]
>
>> +struct ffa_ctx_notif {
>> + bool enabled;
>> +
>> + /* Used to serialize access to the rest of this struct */
>> + spinlock_t lock;
>
> Regardless what I wrote above, I can't seem to find a call to spin_lock_init(). You will want it to initialize.
>
> Also, it would be best if we keep the two booleans close to each other so we limit the amount of padding in the structure.
>
>> +
>> + /*
>> + * True if domain is reported by FFA_NOTIFICATION_INFO_GET to have
>> + * pending global notifications.
>> + */
>> + bool secure_pending;
>> +};
>> struct ffa_ctx {
>> void *rx;
>> @@ -228,6 +265,7 @@ struct ffa_ctx {
>> struct list_head shm_list;
>> /* Number of allocated shared memory object */
>> unsigned int shm_count;
>> + struct ffa_ctx_notif notif;
>> /*
>> * tx_lock is used to serialize access to tx
>> * rx_lock is used to serialize access to rx
>> @@ -257,7 +295,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs);
>> int ffa_handle_mem_reclaim(uint64_t handle, uint32_t flags);
>> bool ffa_partinfo_init(void);
>> -bool ffa_partinfo_domain_init(struct domain *d);
>> +int ffa_partinfo_domain_init(struct domain *d);
>> bool ffa_partinfo_domain_destroy(struct domain *d);
>> int32_t ffa_handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
>> uint32_t w4, uint32_t w5, uint32_t *count,
>> @@ -271,12 +309,28 @@ uint32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
>> uint32_t ffa_handle_rxtx_unmap(void);
>> int32_t ffa_handle_rx_release(void);
>> +void ffa_notif_init(void);
>> +int ffa_notif_domain_init(struct domain *d);
>> +void ffa_notif_domain_destroy(struct domain *d);
>> +
>> +int ffa_handle_notification_bind(struct cpu_user_regs *regs);
>> +int ffa_handle_notification_unbind(struct cpu_user_regs *regs);
>> +void ffa_handle_notification_info_get(struct cpu_user_regs *regs);
>> +void ffa_handle_notification_get(struct cpu_user_regs *regs);
>> +int ffa_handle_notification_set(struct cpu_user_regs *regs);
>> +
>> static inline uint16_t ffa_get_vm_id(const struct domain *d)
>> {
>> /* +1 since 0 is reserved for the hypervisor in FF-A */
>> return d->domain_id + 1;
>> }
>> +static inline struct domain *ffa_get_domain_by_vm_id(uint16_t vm_id)
>> +{
>
> Is this expected to be called with vm_id == 0? If not, then I would consider to add an ASSERT(vm_id != 0). If yes, then I please add a runtime check and return NULL.
>
>> + /* -1 to match ffa_get_vm_id() */
>> + return get_domain_by_id(vm_id - 1);
>> +}
>> +
>> static inline void ffa_set_regs(struct cpu_user_regs *regs, register_t v0,
>> register_t v1, register_t v2, register_t v3,
>> register_t v4, register_t v5, register_t v6,
Cheers
Bertrand
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-26 19:07 ` Julien Grall
2024-04-29 7:20 ` Bertrand Marquis
@ 2024-04-29 8:43 ` Jens Wiklander
1 sibling, 0 replies; 22+ messages in thread
From: Jens Wiklander @ 2024-04-29 8:43 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, patches, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk
Hi Julien,
On Fri, Apr 26, 2024 at 9:07 PM Julien Grall <julien@xen.org> wrote:
>
> Hi Jens,
>
> On 26/04/2024 09:47, Jens Wiklander wrote:
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index d7306aa64d50..5224898265a5 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -155,7 +155,7 @@ void __init init_IRQ(void)
> > {
> > /* SGIs are always edge-triggered */
> > if ( irq < NR_GIC_SGI )
> > - local_irqs_type[irq] = DT_IRQ_TYPE_EDGE_RISING;
> > + local_irqs_type[irq] = IRQ_TYPE_EDGE_RISING;
>
> This changes seems unrelated to this patch. This wants to be separate
> (if you actually intended to change it).
I'm sorry, my bad. I meant for this to go into "xen/arm: allow
dynamically assigned SGI handlers".
>
> > else
> > local_irqs_type[irq] = IRQ_TYPE_INVALID;
> > }
> > diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
> > index f0112a2f922d..7c0f46f7f446 100644
> > --- a/xen/arch/arm/tee/Makefile
> > +++ b/xen/arch/arm/tee/Makefile
> > @@ -2,5 +2,6 @@ obj-$(CONFIG_FFA) += ffa.o
> > obj-$(CONFIG_FFA) += ffa_shm.o
> > obj-$(CONFIG_FFA) += ffa_partinfo.o
> > obj-$(CONFIG_FFA) += ffa_rxtx.o
> > +obj-$(CONFIG_FFA) += ffa_notif.o
> > obj-y += tee.o
> > obj-$(CONFIG_OPTEE) += optee.o
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index 5209612963e1..912e905a27bd 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -39,6 +39,9 @@
> > * - at most 32 shared memory regions per guest
> > * o FFA_MSG_SEND_DIRECT_REQ:
> > * - only supported from a VM to an SP
> > + * o FFA_NOTIFICATION_*:
> > + * - only supports global notifications, that is, per vCPU notifications
> > + * are not supported
> > *
> > * There are some large locked sections with ffa_tx_buffer_lock and
> > * ffa_rx_buffer_lock. Especially the ffa_tx_buffer_lock spinlock used
> > @@ -194,6 +197,8 @@ out:
> >
> > static void handle_features(struct cpu_user_regs *regs)
> > {
> > + struct domain *d = current->domain;
> > + struct ffa_ctx *ctx = d->arch.tee;
> > uint32_t a1 = get_user_reg(regs, 1);
> > unsigned int n;
> >
> > @@ -240,6 +245,30 @@ static void handle_features(struct cpu_user_regs *regs)
> > BUILD_BUG_ON(PAGE_SIZE != FFA_PAGE_SIZE);
> > ffa_set_regs_success(regs, 0, 0);
> > break;
> > + case FFA_FEATURE_NOTIF_PEND_INTR:
> > + if ( ctx->notif.enabled )
> > + ffa_set_regs_success(regs, GUEST_FFA_NOTIF_PEND_INTR_ID, 0);
> > + else
> > + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > + break;
> > + case FFA_FEATURE_SCHEDULE_RECV_INTR:
> > + if ( ctx->notif.enabled )
> > + ffa_set_regs_success(regs, GUEST_FFA_SCHEDULE_RECV_INTR_ID, 0);
> > + else
> > + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > + break;
> > +
> > + case FFA_NOTIFICATION_BIND:
> > + case FFA_NOTIFICATION_UNBIND:
> > + case FFA_NOTIFICATION_GET:
> > + case FFA_NOTIFICATION_SET:
> > + case FFA_NOTIFICATION_INFO_GET_32:
> > + case FFA_NOTIFICATION_INFO_GET_64:
> > + if ( ctx->notif.enabled )
> > + ffa_set_regs_success(regs, 0, 0);
> > + else
> > + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > + break;
> > default:
> > ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > break;
> > @@ -305,6 +334,22 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> > get_user_reg(regs, 1)),
> > get_user_reg(regs, 3));
> > break;
> > + case FFA_NOTIFICATION_BIND:
> > + e = ffa_handle_notification_bind(regs);
> > + break;
> > + case FFA_NOTIFICATION_UNBIND:
> > + e = ffa_handle_notification_unbind(regs);
> > + break;
> > + case FFA_NOTIFICATION_INFO_GET_32:
> > + case FFA_NOTIFICATION_INFO_GET_64:
> > + ffa_handle_notification_info_get(regs);
> > + return true;
> > + case FFA_NOTIFICATION_GET:
> > + ffa_handle_notification_get(regs);
> > + return true;
> > + case FFA_NOTIFICATION_SET:
> > + e = ffa_handle_notification_set(regs);
> > + break;
> >
> > default:
> > gprintk(XENLOG_ERR, "ffa: unhandled fid 0x%x\n", fid);
> > @@ -322,6 +367,7 @@ static bool ffa_handle_call(struct cpu_user_regs *regs)
> > static int ffa_domain_init(struct domain *d)
> > {
> > struct ffa_ctx *ctx;
> > + int ret;
> >
> > if ( !ffa_version )
> > return -ENODEV;
> > @@ -345,10 +391,11 @@ static int ffa_domain_init(struct domain *d)
> > * error, so no need for cleanup in this function.
> > */
> >
> > - if ( !ffa_partinfo_domain_init(d) )
> > - return -EIO;
> > + ret = ffa_partinfo_domain_init(d);
> > + if ( ret )
> > + return ret;
> >
> > - return 0;
> > + return ffa_notif_domain_init(d);
> > }
> >
> > static void ffa_domain_teardown_continue(struct ffa_ctx *ctx, bool first_time)
> > @@ -423,6 +470,7 @@ static int ffa_domain_teardown(struct domain *d)
> > return 0;
> >
> > ffa_rxtx_domain_destroy(d);
> > + ffa_notif_domain_destroy(d);
> >
> > ffa_domain_teardown_continue(ctx, true /* first_time */);
> >
> > @@ -502,6 +550,7 @@ static bool ffa_probe(void)
> > if ( !ffa_partinfo_init() )
> > goto err_rxtx_destroy;
> >
> > + ffa_notif_init();
> > INIT_LIST_HEAD(&ffa_teardown_head);
> > init_timer(&ffa_teardown_timer, ffa_teardown_timer_callback, NULL, 0);
> >
> > diff --git a/xen/arch/arm/tee/ffa_notif.c b/xen/arch/arm/tee/ffa_notif.c
> > new file mode 100644
> > index 000000000000..6bb0804ee2f8
> > --- /dev/null
> > +++ b/xen/arch/arm/tee/ffa_notif.c
> > @@ -0,0 +1,378 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (C) 2024 Linaro Limited
> > + */
> > +
> > +#include <xen/const.h>
> > +#include <xen/list.h>
> > +#include <xen/spinlock.h>
> > +#include <xen/types.h>
> > +
> > +#include <asm/smccc.h>
> > +#include <asm/regs.h>
> > +
> > +#include "ffa_private.h"
> > +
> > +static bool __ro_after_init notif_enabled;
> > +
> > +int ffa_handle_notification_bind(struct cpu_user_regs *regs)
> > +{
> > + struct domain *d = current->domain;
> > + uint32_t src_dst = get_user_reg(regs, 1);
> > + uint32_t flags = get_user_reg(regs, 2);
> > + uint32_t bitmap_lo = get_user_reg(regs, 3);
> > + uint32_t bitmap_hi = get_user_reg(regs, 4);
> > +
> > + if ( !notif_enabled )
> > + return FFA_RET_NOT_SUPPORTED;
> > +
> > + if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> > + return FFA_RET_INVALID_PARAMETERS;
> > +
> > + if ( flags ) /* Only global notifications are supported */
> > + return FFA_RET_DENIED;
> > +
> > + /*
> > + * We only support notifications from SP so no need to check the sender
> > + * endpoint ID, the SPMC will take care of that for us.
> > + */
> > + return ffa_simple_call(FFA_NOTIFICATION_BIND, src_dst, flags, bitmap_hi,
> > + bitmap_lo);
> > +}
> > +
> > +int ffa_handle_notification_unbind(struct cpu_user_regs *regs)
> > +{
> > + struct domain *d = current->domain;
> > + uint32_t src_dst = get_user_reg(regs, 1);
> > + uint32_t bitmap_lo = get_user_reg(regs, 3);
> > + uint32_t bitmap_hi = get_user_reg(regs, 4);
> > +
> > + if ( !notif_enabled )
> > + return FFA_RET_NOT_SUPPORTED;
> > +
> > + if ( (src_dst & 0xFFFFU) != ffa_get_vm_id(d) )
> > + return FFA_RET_INVALID_PARAMETERS;
> > +
> > + /*
> > + * We only support notifications from SP so no need to check the
> > + * destination endpoint ID, the SPMC will take care of that for us.
> > + */
> > + return ffa_simple_call(FFA_NOTIFICATION_UNBIND, src_dst, 0, bitmap_hi,
> > + bitmap_lo);
> > +}
> > +
> > +void ffa_handle_notification_info_get(struct cpu_user_regs *regs)
> > +{
> > + struct domain *d = current->domain;
> > + struct ffa_ctx *ctx = d->arch.tee;
> > + bool pending_global;
> > +
> > + if ( !notif_enabled )
> > + {
> > + ffa_set_regs_error(regs, FFA_RET_NOT_SUPPORTED);
> > + return;
> > + }
> > +
> > + spin_lock(&ctx->notif.lock);
> > + pending_global = ctx->notif.secure_pending;
> > + ctx->notif.secure_pending = false;
> > + spin_unlock(&ctx->notif.lock);
> > +
> > + if ( pending_global )
> > + {
> > + /* A pending global notification for the guest */
> > + ffa_set_regs(regs, FFA_SUCCESS_64, 0,
> > + 1U << FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT, ffa_get_vm_id(d),
> > + 0, 0, 0, 0);
> > + }
> > + else
> > + {
> > + /* Report an error if there where no pending global notification */
> > + ffa_set_regs_error(regs, FFA_RET_NO_DATA);
> > + }
> > +}
> > +static void notif_irq_handler(int irq, void *data)
> > +{
> > + const struct arm_smccc_1_2_regs arg = {
> > + .a0 = FFA_NOTIFICATION_INFO_GET_64,
> > + };
> > + struct arm_smccc_1_2_regs resp;
> > + unsigned int id_pos;
> > + unsigned int list_count;
> > + uint64_t ids_count;
> > + unsigned int n;
> > + int32_t res;
> > +
> > + do {
> > + arm_smccc_1_2_smc(&arg, &resp);
> > + res = ffa_get_ret_code(&resp);
> > + if ( res )
> > + {
> > + if ( res != FFA_RET_NO_DATA )
> > + printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
> > + res);
> > + return;
> > + }
> > +
> > + ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
> > + list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
> > + FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
> > +
> > + id_pos = 0;
> > + for ( n = 0; n < list_count; n++ )
> > + {
> > + unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
> > + struct domain *d;
> > +
> > + d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));
>
> Thinking a bit more about the question from Bertrand about
> get_domain_id() vs rcu_lock_domain_by_id(). I am actually not sure
> whether either are ok here.
>
> If I am not mistaken, d->arch.tee will be freed as part of the domain
> teardown process. This means you can have the following scenario:
>
> CPU0: ffa_get_domain_by_vm_id() (return the domain as it is alive)
>
> CPU1: call domain_kill()
> CPU1: teardown is called, free d->arch.tee (the pointer is not set to NULL)
>
> d->arch.tee is now a dangling pointer
>
> CPU0: access d->arch.tee
>
> This implies you may need to gain a global lock (I don't have a better
> idea so far) to protect the IRQ handler against domains teardown.
>
> Did I miss anything?
Thanks for the explanation. I'll reply to Bertrands answer.
>
> > +
> > + if ( d )
> > + {
> > + struct ffa_ctx *ctx = d->arch.tee;
> > +
> > + spin_lock(&ctx->notif.lock);
> > + ctx->notif.secure_pending = true;
> > + spin_unlock(&ctx->notif.lock);
>
>
> AFAICT, the spinlock is used with IRQ enabled (see
> ffa_handle_notification_info_get()) but also in an IRQ handler. So to
> prevent deadlock, you will want to use spin_lock_irq* helpers.
>
> That said, I don't think you need a spin_lock(). You could use atomic
> operations instead. For the first hunk, you could use
> test_and_clear_bool(). E.g.:
>
> if ( test_and_clear_bool(ctx.notif.secure_pending) )
>
> For the second part, it might be fine to use ACCESS_ONCE().
Thanks, I'll update the code accordingly.
>
> > +
> > + /*
> > + * Since we're only delivering global notification, always
> > + * deliver to the first vCPU. It doesn't matter which we
> > + * chose, as long as it's available.
>
> What if vCPU0 is offlined?
Good question, would the following work?
for_each_vcpu(d, vcpu)
{
if ( is_vcpu_online(vcpu)
{
vgic_inject_irq(d, vcpu, GUEST_FFA_NOTIF_PEND_INTR_ID, true);
break;
}
}
if ( !vcpu )
printk(XENLOG_ERR "ffa: can't inject NPI, all vCPUs offline\");
>
> > + */
> > + vgic_inject_irq(d, d->vcpu[0], GUEST_FFA_NOTIF_PEND_INTR_ID,
> > + true);
> > +
> > + put_domain(d);
> > + }
> > +
> > + id_pos += count;
> > + }
> > +
> > + } while (resp.a2 & FFA_NOTIF_INFO_GET_MORE_FLAG);
> > +}
>
> [..]
>
> > +struct ffa_ctx_notif {
> > + bool enabled;
> > +
> > + /* Used to serialize access to the rest of this struct */
> > + spinlock_t lock;
>
> Regardless what I wrote above, I can't seem to find a call to
> spin_lock_init(). You will want it to initialize.
>
> Also, it would be best if we keep the two booleans close to each other
> so we limit the amount of padding in the structure.
Good point, I'll remove the spinlock and update the code accordingly.
>
> > +
> > + /*
> > + * True if domain is reported by FFA_NOTIFICATION_INFO_GET to have
> > + * pending global notifications.
> > + */
> > + bool secure_pending;
> > +};
> >
> > struct ffa_ctx {
> > void *rx;
> > @@ -228,6 +265,7 @@ struct ffa_ctx {
> > struct list_head shm_list;
> > /* Number of allocated shared memory object */
> > unsigned int shm_count;
> > + struct ffa_ctx_notif notif;
> > /*
> > * tx_lock is used to serialize access to tx
> > * rx_lock is used to serialize access to rx
> > @@ -257,7 +295,7 @@ void ffa_handle_mem_share(struct cpu_user_regs *regs);
> > int ffa_handle_mem_reclaim(uint64_t handle, uint32_t flags);
> >
> > bool ffa_partinfo_init(void);
> > -bool ffa_partinfo_domain_init(struct domain *d);
> > +int ffa_partinfo_domain_init(struct domain *d);
> > bool ffa_partinfo_domain_destroy(struct domain *d);
> > int32_t ffa_handle_partition_info_get(uint32_t w1, uint32_t w2, uint32_t w3,
> > uint32_t w4, uint32_t w5, uint32_t *count,
> > @@ -271,12 +309,28 @@ uint32_t ffa_handle_rxtx_map(uint32_t fid, register_t tx_addr,
> > uint32_t ffa_handle_rxtx_unmap(void);
> > int32_t ffa_handle_rx_release(void);
> >
> > +void ffa_notif_init(void);
> > +int ffa_notif_domain_init(struct domain *d);
> > +void ffa_notif_domain_destroy(struct domain *d);
> > +
> > +int ffa_handle_notification_bind(struct cpu_user_regs *regs);
> > +int ffa_handle_notification_unbind(struct cpu_user_regs *regs);
> > +void ffa_handle_notification_info_get(struct cpu_user_regs *regs);
> > +void ffa_handle_notification_get(struct cpu_user_regs *regs);
> > +int ffa_handle_notification_set(struct cpu_user_regs *regs);
> > +
> > static inline uint16_t ffa_get_vm_id(const struct domain *d)
> > {
> > /* +1 since 0 is reserved for the hypervisor in FF-A */
> > return d->domain_id + 1;
> > }
> >
> > +static inline struct domain *ffa_get_domain_by_vm_id(uint16_t vm_id)
> > +{
>
> Is this expected to be called with vm_id == 0? If not, then I would
> consider to add an ASSERT(vm_id != 0). If yes, then I please add a
> runtime check and return NULL.
vm_id should not be 0, I'll add an ASSERT() and a check in
notif_irq_handler() that vm_id isn't 0.
Thanks,
Jens
>
> > + /* -1 to match ffa_get_vm_id() */
> > + return get_domain_by_id(vm_id - 1);
> > +}
> > +
> > static inline void ffa_set_regs(struct cpu_user_regs *regs, register_t v0,
> > register_t v1, register_t v2, register_t v3,
> > register_t v4, register_t v5, register_t v6,
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-29 7:20 ` Bertrand Marquis
@ 2024-04-29 9:49 ` Jens Wiklander
2024-04-29 20:55 ` Julien Grall
1 sibling, 0 replies; 22+ messages in thread
From: Jens Wiklander @ 2024-04-29 9:49 UTC (permalink / raw)
To: Bertrand Marquis
Cc: Julien Grall, Xen-devel, patches, Stefano Stabellini,
Michal Orzel, Volodymyr Babchuk
Hi Bertrand,
On Mon, Apr 29, 2024 at 9:20 AM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
[...]
> >> +static void notif_irq_handler(int irq, void *data)
> >> +{
> >> + const struct arm_smccc_1_2_regs arg = {
> >> + .a0 = FFA_NOTIFICATION_INFO_GET_64,
> >> + };
> >> + struct arm_smccc_1_2_regs resp;
> >> + unsigned int id_pos;
> >> + unsigned int list_count;
> >> + uint64_t ids_count;
> >> + unsigned int n;
> >> + int32_t res;
> >> +
> >> + do {
> >> + arm_smccc_1_2_smc(&arg, &resp);
> >> + res = ffa_get_ret_code(&resp);
> >> + if ( res )
> >> + {
> >> + if ( res != FFA_RET_NO_DATA )
> >> + printk(XENLOG_ERR "ffa: notification info get failed: error %d\n",
> >> + res);
> >> + return;
> >> + }
> >> +
> >> + ids_count = resp.a2 >> FFA_NOTIF_INFO_GET_ID_LIST_SHIFT;
> >> + list_count = ( resp.a2 >> FFA_NOTIF_INFO_GET_ID_COUNT_SHIFT ) &
> >> + FFA_NOTIF_INFO_GET_ID_COUNT_MASK;
> >> +
> >> + id_pos = 0;
> >> + for ( n = 0; n < list_count; n++ )
> >> + {
> >> + unsigned int count = ((ids_count >> 2 * n) & 0x3) + 1;
> >> + struct domain *d;
> >> +
> >> + d = ffa_get_domain_by_vm_id(get_id_from_resp(&resp, id_pos));
> >
> > Thinking a bit more about the question from Bertrand about get_domain_id() vs rcu_lock_domain_by_id(). I am actually not sure whether either are ok here.
> >
> > If I am not mistaken, d->arch.tee will be freed as part of the domain teardown process. This means you can have the following scenario:
> >
> > CPU0: ffa_get_domain_by_vm_id() (return the domain as it is alive)
> >
> > CPU1: call domain_kill()
> > CPU1: teardown is called, free d->arch.tee (the pointer is not set to NULL)
> >
> > d->arch.tee is now a dangling pointer
> >
> > CPU0: access d->arch.tee
> >
> > This implies you may need to gain a global lock (I don't have a better idea so far) to protect the IRQ handler against domains teardown.
> >
> > Did I miss anything?
>
> I think you are right which is why I was thinking to use rcu_lock_live_remote_domain_by_id to only get a reference
> to the domain if it is not dying.
>
> From the comment in sched.h:
> /*
> * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
> * This is the preferred function if the returned domain reference
> * is short lived, but it cannot be used if the domain reference needs
> * to be kept beyond the current scope (e.g., across a softirq).
> * The returned domain reference must be discarded using rcu_unlock_domain().
> */
>
> Now the question of short lived should be challenged but I do not think we can
> consider the current code as "long lived".
>
> It would be a good idea to start a mailing list thread discussion with other
> maintainers on the subject to confirm.
> @Jens: as i will be off for some time, would you mind doing it ?
Sure, first I'll send out a new patch set with the current comments
addressed. I'll update to use rcu_lock_live_remote_domain_by_id() both
because it makes more sense than the current code, and also to have a
good base for the discussion.
Thanks,
Jens
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-26 17:58 ` Julien Grall
@ 2024-04-29 9:55 ` Jens Wiklander
0 siblings, 0 replies; 22+ messages in thread
From: Jens Wiklander @ 2024-04-29 9:55 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, patches, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk
Hi Julien,
On Fri, Apr 26, 2024 at 7:58 PM Julien Grall <julien@xen.org> wrote:
>
> Hi Jens,
>
> On 26/04/2024 09:47, Jens Wiklander wrote:
> > +static void notif_irq_enable(void *info)
> > +{
> > + struct notif_irq_info *irq_info = info;
> > +
> > + irq_info->ret = setup_irq(irq_info->irq, 0, irq_info->action);
> In v2, you were using request_irq(). But now you seem to be open-coding
> it. Can you explain why?
It's because request_irq() does a memory allocation that can't be done
in interrupt context.
>
> > + if ( irq_info->ret )
> > + printk(XENLOG_ERR "ffa: request_irq irq %u failed: error %d\n",
> > + irq_info->irq, irq_info->ret);
> > +}
> > +
> > +void ffa_notif_init(void)
> > +{
> > + const struct arm_smccc_1_2_regs arg = {
> > + .a0 = FFA_FEATURES,
> > + .a1 = FFA_FEATURE_SCHEDULE_RECV_INTR,
> > + };
> > + struct notif_irq_info irq_info = { };
> > + struct arm_smccc_1_2_regs resp;
> > + unsigned int cpu;
> > +
> > + arm_smccc_1_2_smc(&arg, &resp);
> > + if ( resp.a0 != FFA_SUCCESS_32 )
> > + return;
> > +
> > + irq_info.irq = resp.a2;
> > + if ( irq_info.irq < GIC_SGI_STATIC_MAX || irq_info.irq >= NR_GIC_SGI )
> > + {
> > + printk(XENLOG_ERR "ffa: notification initialization failed: conflicting SGI %u\n",
> > + irq_info.irq);
> > + return;
> > + }
> > +
> > + /*
> > + * SGIs are per-CPU so we must enable the IRQ on each CPU. We use an
> > + * IPI to call notif_irq_enable() on each CPU including the current
> > + * CPU. The struct irqaction is preallocated since we can't allocate
> > + * memory while in interrupt context.
> > + */
> > + for_each_online_cpu(cpu)
> Even though we currently don't support CPU hotplug, you want to add a
> CPU Notifier to also register the IRQ when a CPU is onlined
> ffa_notif_init().
>
> For an example, see time.c. We may also want to consider to enable TEE
> in presmp_initcalls() so we don't need to have a for_each_online_cpu().
I was considering that too. I'll update the code.
Thanks,
Jens
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [XEN PATCH v3 5/5] xen/arm: ffa: support notification
2024-04-29 7:20 ` Bertrand Marquis
2024-04-29 9:49 ` Jens Wiklander
@ 2024-04-29 20:55 ` Julien Grall
1 sibling, 0 replies; 22+ messages in thread
From: Julien Grall @ 2024-04-29 20:55 UTC (permalink / raw)
To: Bertrand Marquis
Cc: Jens Wiklander, Xen-devel, patches, Stefano Stabellini,
Michal Orzel, Volodymyr Babchuk
Hi Bertrand,
On 29/04/2024 08:20, Bertrand Marquis wrote:
> From the comment in sched.h:
> /*
> * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
> * This is the preferred function if the returned domain reference
> * is short lived, but it cannot be used if the domain reference needs
> * to be kept beyond the current scope (e.g., across a softirq).
> * The returned domain reference must be discarded using rcu_unlock_domain().
> */
>
> Now the question of short lived should be challenged but I do not think we can
> consider the current code as "long lived".
Actually, I am not entirely sure you can use put_domain() in interrupt
context. If you look at the implementation of domain_destroy() it takes
a spin lock without disabling the interrupts.
The same spinlock is taken in domain_create(). So there is a potential
deadlock.
Which means the decision between the two is not only about liveness.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2024-04-29 20:56 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-26 8:47 [XEN PATCH v3 0/5] FF-A notifications Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 1/5] xen/arm: ffa: refactor ffa_handle_call() Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 2/5] xen/arm: ffa: use ACCESS_ONCE() Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 3/5] xen/arm: ffa: simplify ffa_handle_mem_share() Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 4/5] xen/arm: allow dynamically assigned SGI handlers Jens Wiklander
2024-04-26 8:47 ` [XEN PATCH v3 5/5] xen/arm: ffa: support notification Jens Wiklander
2024-04-26 9:20 ` Bertrand Marquis
2024-04-26 12:11 ` Jens Wiklander
2024-04-26 12:19 ` Bertrand Marquis
2024-04-26 12:32 ` Jens Wiklander
2024-04-26 12:41 ` Bertrand Marquis
2024-04-26 13:02 ` Jens Wiklander
2024-04-26 15:12 ` Bertrand Marquis
2024-04-26 18:31 ` Julien Grall
2024-04-26 17:58 ` Julien Grall
2024-04-29 9:55 ` Jens Wiklander
2024-04-26 19:07 ` Julien Grall
2024-04-29 7:20 ` Bertrand Marquis
2024-04-29 9:49 ` Jens Wiklander
2024-04-29 20:55 ` Julien Grall
2024-04-29 8:43 ` Jens Wiklander
2024-04-26 9:23 ` [XEN PATCH v3 0/5] FF-A notifications Bertrand Marquis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).