[RFC] Asynchronous notifications #4601

jenswi-linaro · 2021-05-10T12:08:46Z

This is a preview and RFC of the asynchronous notifications feature.

Note this is tested on QEMU virt (v7) together with the kernel patches in PR linaro-swg/linux#93

I'm in particular interested in feedback on this from a SCMI point of view.
Other aspects to keep in mind is how this harmonizes with asynchronous notifications in FF-A 1.1

jforissier

A few comments already, although I have not looked at everything.

core/arch/arm/include/sm/optee_smc.h

core/arch/arm/kernel/link_dummies_paged.c

jforissier · 2021-05-10T14:01:24Z

core/arch/arm/tee/entry_fast.c

@@ -181,6 +190,22 @@ void __weak tee_entry_fast(struct thread_smc_args *args)
 	__tee_entry_fast(args);
 }

+#if CFG_CORE_ASYNC_NOTIF_INTID


if (CFG_CORE_ASYNC_NOTIF_INTID) { ... } in the function body instead?

core/arch/arm/tee/entry_fast.c

core/include/kernel/notif.h

jenswi-linaro · 2021-05-10T19:44:20Z

Rebased on master on top the commits merged already. Split up a bit as requested.

jforissier

A couple more things. This looks quite useful to me, especially as a remember past discussions with people requesting that kind of functionality.

core/arch/arm/include/sm/optee_smc.h

core/kernel/notif.c

core/include/kernel/notif.h

jenswi-linaro · 2021-05-27T07:44:08Z

Update

etienne-lms

only minor comments

etienne-lms · 2021-05-27T09:06:46Z

core/arch/arm/include/sm/optee_smc.h

+ * Retrieve a value of notifications pended since the last call of this
+ * function.
+ *
+ * OP-TEE keeps a records of all posted values. When an interrupts is


s/records/record/
s/interrupts/interrupt/

etienne-lms · 2021-05-27T09:07:20Z

core/arch/arm/include/sm/optee_smc.h

+ *
+ * OP-TEE keeps a records of all posted values. When an interrupts is
+ * received which indicates that there are posted values this function
+ * should be called until all pended values has been retrieved. When a


s/has/have/

etienne-lms · 2021-05-27T09:07:46Z

core/arch/arm/include/sm/optee_smc.h

+ * should be called until all pended values has been retrieved. When a
+ * value is retrieved it's cleared from the record in secure world.
+ *
+ * It is expected that this function is called from and interrupt handler


etienne-lms · 2021-05-27T09:16:12Z

core/arch/arm/tee/entry_fast.c

@@ -240,6 +263,19 @@ void __tee_entry_fast(struct thread_smc_args *args)
 		break;
 #endif

+	case OPTEE_SMC_ENABLE_ASYNC_NOTIF:
+		if (notif_async_is_enabled())
+			notif_deliver_atomic_event(NOTIF_EVENT_STARTED);


should set args->a0.

etienne-lms · 2021-05-27T09:19:47Z

core/include/kernel/notif.h

+ * NOTIF_VALUE_MAX	    for mutex and condvar wait/wakeup
+ *
+ * Any value can be signalled with notif_send_sync() while only the ones
+ * <= CFG_CORE_ASYNC_NOTIF_MAX can be signalled with notif_send_async().


s/CFG_CORE_ASYNC_NOTIF_MAX/NOTIF_ASYNC_VALUE_MAX/

etienne-lms · 2021-05-27T09:24:10Z

core/include/kernel/notif.h

+ * has been delivered again.
+ *
+ * Note that while a @NOTIF_EVENT_STOPPED is being delivered at the same
+ * time may a @NOTIF_EVENT_STARTED be delivered again so a driver is


maybe replace a driver with non-secure world?
(or did I misunderstood?)

A driver is an OP-TEE driver in this case.
I'll remove "shared" from "shared internal state" since I guess that's what put you on the wrong track.

etienne-lms · 2021-05-27T09:28:22Z

core/include/kernel/notif.h

+}
+#endif
+
+TEE_Result notif_alloc_async_value(uint32_t *val);


s/val/value/g ?

etienne-lms · 2021-05-27T09:35:18Z

core/kernel/notif.c

+
+	cpu_spin_unlock_xrestore(&notif_lock, old_itr_status);
+
+	if (bit < 0)


maybe a DMSG() trace here to help identifying the out-of-mem reporter?

I'm not sure that helps much. This is likely to happen when the drivers are initializing so there's bound some more interesting print or perhaps even a panic.

etienne-lms · 2021-05-27T09:38:04Z

core/kernel/notif.c

+bool notif_async_is_started(void)
+{
+	uint32_t old_itr_status = 0;
+	bool ret;


etienne-lms · 2021-05-27T09:45:58Z

core/tee/entry_std.c

+		if (notif_async_is_enabled())
+			notif_deliver_event(NOTIF_EVENT_DO_BOTTOM_HALF);
+		else
+			goto err;


why not rv = OPTEE_SMC_RETURN_EBADCMD; straight?

I thought it nice to get the "Unknown cmd..." print in such a case.

jenswi-linaro · 2021-05-27T11:53:14Z

Update

jforissier · 2021-05-31T12:04:00Z

Reviewed-by: Jerome Forissier <jerome@forissier.org>

jenswi-linaro · 2021-05-31T12:40:17Z

Squashed and tags applied

jforissier · 2021-06-14T07:21:40Z

@etienne-lms do you want to provide a review or ack tag?

etienne-lms · 2021-06-14T09:35:48Z

Sorry. Yes, Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> with a late minor comment for commit "core: add asynchronous notifications".

jenswi-linaro · 2021-06-14T11:21:15Z

Sorry. Yes, Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> with a late minor comment for commit "core: add asynchronous notifications".

I'm sorry, which comment?

etienne-lms

oops! this one.

etienne-lms · 2021-06-14T09:25:01Z

core/kernel/notif.c

+
+	COMPILE_TIME_ASSERT(NOTIF_VALUE_DO_BOTTOM_HALF ==
+			    OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_BOTTOM_HALF);
+	COMPILE_TIME_ASSERT(!CFG_CORE_ASYNC_NOTIF_INTID ||


minor: this function mandates CFG_CORE_ASYNC_NOTIF_INTID!=0
note it is already embedded only upon CFG_CORE_ASYNC_NOTIF_INTID!=0 (line 16)

jenswi-linaro · 2021-06-14T12:09:10Z

Addressed comment, applied tag and rebased on master. We should wait with the merging this until we have made some more progress with upstreaming the kernel patches.

jforissier · 2021-06-14T12:22:01Z

We should wait with the merging this until we have made some more progress with upstreaming the kernel patches.

Sure.

JensVankeirsbilck · 2021-10-25T07:29:41Z

I know and understand this is not high up the priority list, but any idea when this functionality would become available? I would like to test it, to see if it provides the functionality I seek (i.e. periodic tasks, largely independent of the REE).

jenswi-linaro · 2021-10-25T08:17:30Z

Rebased, "core: drivers: gic.h: define PPI and SPI bases" moved closer to the top, and added two "review" commits.

jenswi-linaro · 2021-10-25T08:24:59Z

I know and understand this is not high up the priority list, but any idea when this functionality would become available? I would like to test it, to see if it provides the functionality I seek (i.e. periodic tasks, largely independent of the REE).

I'm aiming for the v5.17 Linux kernel with the kernel driver counterpart of this (it seems we're going to be too close to the v5.16 merge window). I expect we can merge this earlier than that though. You can help out by testing this and replying on the kernel mailing list where the patches are posted. I expect to post the v7 patchset later this week so if you can do you test based on that it would be great. I can add you in CC if you like, which address should I use in that case?

jforissier · 2021-10-25T08:33:56Z

For commit "core: drivers: gic.h: define PPI and SPI bases":
Reviewed-by: Jerome Forissier <jerome@forissier.org>

JensVankeirsbilck · 2021-10-25T08:41:23Z

I'm aiming for the v5.17 Linux kernel with the kernel driver counterpart of this (it seems we're going to be too close to the v5.16 merge window). I expect we can merge this earlier than that though. You can help out by testing this and replying on the kernel mailing list where the patches are posted. I expect to post the v7 patchset later this week so if you can do you test based on that it would be great. I can add you in CC if you like, which address should I use in that case?

Not sure I fully understand what you mean here, but I'm glad to help where I can. Since this is work-related for me, feel free to use my work-email: jens.vankeirsbilck at kuleuven.be . Will I be able to test this by just updating my QEMUv7 repo (which I'm using now) ?

jenswi-linaro · 2021-10-25T09:16:23Z

Will I be able to test this by just updating my QEMUv7 repo (which I'm using now) ?

Yes, just switch to this branch for optee_os with this and update linux with this branch https://git.linaro.org/people/jens.wiklander/linux-tee.git/log/?h=async_notif_v6

Build as usual and you should get something like:

D/TC:0   notif_send_async:105 0x0
D/TC:? 1 read_console:112 got 0x20

on the secure uart when pressing space bar.

jforissier

For commit "core: add asynchronous notifications":

Minor comments below, once addressed:
Reviewed-by: Jerome Forissier <jerome@forissier.org>

core/arch/arm/include/sm/optee_smc.h

core/arch/arm/tee/entry_fast.c

core/include/kernel/notif.h

core/include/optee_rpc_cmd.h

mk/config.mk

core/kernel/notif.c

jenswi-linaro · 2021-10-25T17:22:30Z

Squashed in the update and applied Jerome's R-B on "core: add asynchronous notifications"
There are some old R-B's in this PR, but I've skipped those for now since you might want to take another look first.

jenswi-linaro · 2021-10-25T17:24:29Z

Update

panda-balarka · 2021-10-25T18:59:45Z

@jenswi-linaro Is there any way I can test this before its upstreamed. I'm anyway trying to bring up periodic tasks on OPTEE as mentioned in issues #4910 and #4920. I could test it out and try to merge some changes on an actual HW if possible.

jenswi-linaro · 2021-10-25T21:19:05Z

@jenswi-linaro Is there any way I can test this before its upstreamed. I'm anyway trying to bring up periodic tasks on OPTEE as mentioned in issues #4910 and #4920. I could test it out and try to merge some changes on an actual HW if possible.

#4601 (comment)

JensVankeirsbilck · 2021-10-26T15:52:52Z

Yes, just switch to this branch for optee_os with this and update linux with this branch https://git.linaro.org/people/jens.wiklander/linux-tee.git/log/?h=async_notif_v6

Build as usual and you should get something like:
D/TC:0   notif_send_async:105 0x0
D/TC:? 1 read_console:112 got 0x20
on the secure uart when pressing space bar.

@jenswi-linaro , indeed. Because I didn't want to overwrite my existing QEMU project, I created a new one using a local default.xml manifest for the repo tool, pointing to your repositories. @panda-balarka I've attached my manifest here, maybe it helps you set up a project too. Due to upload restrictions I renamed the file to .txt extension, to use it with repo, just revert it back to .xml
default.txt

I'll know have a look at the code that produces that output to figure out how to connect it to a timer interrupt and take it from there. Thanks for expediting this so I can play around with it.

JensVankeirsbilck · 2021-10-29T08:55:46Z

@jenswi-linaro I've been going through the code, and as far as I can see, the code for that UART Notif test is located in /core/arch/arm/plat-vexpress/main.c

While I understand that code, I'm struggling seeing how OPTEE / the notification code differentiates between different notifications, especially to launch the bottom half of the driver.
For example, let's say I want to use these notifications for a Timer. In the Timer ISR, I would execute some functionality and then would need to launch the bottom half, so that means calling notif_send_async with NOTIF_VALUE_DO_BOTTOM_HALF. But I only want to trigger my Timer bottom half of course, and not the UART driver bottom half. So how should this be done? The documentation page about notifications https://optee.readthedocs.io/en/latest/architecture/core.html#notifications doesn't really help in this regard.
Am I missing something?

jenswi-linaro · 2021-10-29T10:37:08Z

All drivers which has registered for bottom half processing will be called each time. I don't expect that many callbacks registered that it will be a performance problem. It should be noted that entering secure world is much more expensive than calling a registered callback. I suppose we could have multiple queues for idle and triggered callbacks, but it seems more trouble than it's worth today.

A registered callback should be capable of telling if it has any work to do when called.

Clarifies the responsibilities of the caller when calling with struct optee_msg_arg as argument. Reviewed-by: Jerome Forissier <jerome@forissier.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Adds the two defines GIC_PPI_BASE and GIC_SPI_BASE to tell the base of the ranges for PPIs and SPIs respectively. Reviewed-by: Jerome Forissier <jerome@forissier.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Adds a new interface for synchronous notifications. The old RPC interface based on OPTEE_RPC_CMD_WAIT_QUEUE is renamed to OPTEE_RPC_CMD_NOTIFICATION in order to match the new interface. Reviewed-by: Jerome Forissier <jerome@forissier.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Adds support for asynchronous notifications from secure world to normal world. This allows a design with a top half and bottom half type of driver where the top half runs in secure interrupt context and a notifications tells normal world to schedule a yielding call to do the bottom half processing. The protocol is defined in optee_msg.h optee_rpc_cmd.h and optee_smc.h. A notification consists of a 32-bit value which normal world can retrieve using a fastcall into secure world. OP-TEE is currently only supporting the value 0-63 where 0 has a special meaning. When 0 is sent it means that normal world is supposed to make a yielding call OPTEE_MSG_CMD_DO_BOTTOM_HALF. The notification framework in OP-TEE defines an interface where drivers can register a callback which is called on each yielding bottom half call. Notification capability is negotiated with the normal world while it initializes its driver. If both sides supports these notifications then they are enabled. CFG_CORE_ASYNC_NOTIF_GIC_INTID is added to define the hardware interrupt used to notify normal world. This is added to the DTB in case OP-TEE can is configured with CFG_DT=y. Other cases requires the normal world DTB to be kept in sync with this. Reviewed-by: Jerome Forissier <jerome@forissier.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

When asynchronous notifications are enabled the console driver in qemu is configured as a top half and bottom half driver allowing basic testing of the notification framework. Reviewed-by: Jerome Forissier <jerome@forissier.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro · 2021-12-28T17:00:55Z

Rebased on master.
The Linux kernel pull request has been merged into the arm-soc tree, see https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git/commit/?id=c6e5bdae04a3

We've now reached a point where we can merge this.

jforissier · 2021-12-28T21:19:30Z

We've now reached a point where we can merge this.

Absolutely, thanks!

th0ma5b · 2022-05-19T15:14:27Z

Could you help clarifying a few points:

Regarding

optee_os/core/kernel/notif.c

Line 190 in 799c1d1

nd->yielding_cb(nd, ev);

Is yielding_cb() intentionally called in the case of a NOTIF_EVENT_STOPPED event?
Could you confirm that if a bottom half "A" blocks/waits, then the remaining driver bottom halves will not run until bottom half "A" call-back has resumed and returned?

jenswi-linaro · 2022-05-20T09:07:37Z

Yes, and yes.

OP-TEE community is working on asynchronous notification support ([1] and [2]) and is likely to merge TEE_GEN_CAP_ASYNC_NOTIF as 1 << 4. This change moves OCALL capability bit position bit field to upper half. We shall sync back with Linux once TEE_GEN_CAP_OCALL is officially upstream. Link: [1] OP-TEE/optee_os#4601 Link: [2] linaro-swg/linux#93 Change-Id: I8b2f3ca3d2f288990d420f4d8071c9fca88f561c Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>

jenswi-linaro mentioned this pull request May 10, 2021

[RFC] Asynchronous notifications linaro-swg/linux#93

Closed

jenswi-linaro force-pushed the async_notif branch 3 times, most recently from 857fa2e to e014fc6 Compare May 10, 2021 13:24

jforissier reviewed May 10, 2021

View reviewed changes

jenswi-linaro force-pushed the async_notif branch 2 times, most recently from de23615 to 393ab15 Compare May 10, 2021 19:43

jenswi-linaro mentioned this pull request May 17, 2021

architecture: core: add a section on notifications OP-TEE/optee_docs#122

Merged

jforissier reviewed May 26, 2021

View reviewed changes

core/arch/arm/include/sm/optee_smc.h Outdated Show resolved Hide resolved

core/kernel/notif.c Outdated Show resolved Hide resolved

core/include/kernel/notif.h Show resolved Hide resolved

etienne-lms reviewed May 27, 2021

View reviewed changes

jenswi-linaro mentioned this pull request May 31, 2021

TA in OPTEE waveup by a secure interrupt? #4653

Closed

jenswi-linaro force-pushed the async_notif branch from a2faf92 to 7f3de17 Compare May 31, 2021 12:40

etienne-lms reviewed Jun 14, 2021

View reviewed changes

jenswi-linaro force-pushed the async_notif branch 2 times, most recently from ca25df6 to 14cf2f9 Compare June 14, 2021 12:08

jenswi-linaro added the enhancement label Jul 2, 2021

JensVankeirsbilck mentioned this pull request Jul 6, 2021

Question: how to use Early TA? #4729

Closed

jenswi-linaro mentioned this pull request Jul 29, 2021

Calling PTA from optee_os core #4776

Closed

JensVankeirsbilck mentioned this pull request Oct 11, 2021

periodic timer interrupt in Secure World i.MX6ULL EVK #2925

Closed

jenswi-linaro force-pushed the async_notif branch from 14cf2f9 to 245bae4 Compare October 25, 2021 08:16

jforissier approved these changes Oct 25, 2021

View reviewed changes

jenswi-linaro force-pushed the async_notif branch from 245bae4 to ec188cf Compare October 25, 2021 17:24

panda-balarka mentioned this pull request Oct 29, 2021

sunxi-sun50i_a64: Spurious interrupt triggered after FIQ #4920

Closed

jenswi-linaro added 5 commits December 28, 2021 17:37

core: optee_smc.h: clarify calls with struct optee_msg_arg

12b1e29

Clarifies the responsibilities of the caller when calling with struct optee_msg_arg as argument. Reviewed-by: Jerome Forissier <jerome@forissier.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

core: drivers: gic.h: define PPI and SPI bases

2c695ca

Adds the two defines GIC_PPI_BASE and GIC_SPI_BASE to tell the base of the ranges for PPIs and SPIs respectively. Reviewed-by: Jerome Forissier <jerome@forissier.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro force-pushed the async_notif branch from a2861b6 to 78bec5e Compare December 28, 2021 16:54

jforissier merged commit 7b06f6c into OP-TEE:master Dec 28, 2021

jenswi-linaro deleted the async_notif branch December 28, 2021 22:32


		cpu_spin_unlock_xrestore(&notif_lock, old_itr_status);

		if (bit < 0)

[RFC] Asynchronous notifications #4601

[RFC] Asynchronous notifications #4601

Conversation

jenswi-linaro commented May 10, 2021 • edited Loading

jforissier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented May 10, 2021

jforissier left a comment

Choose a reason for hiding this comment

jenswi-linaro commented May 27, 2021

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented May 27, 2021

jforissier commented May 31, 2021

jenswi-linaro commented May 31, 2021

jforissier commented Jun 14, 2021

etienne-lms commented Jun 14, 2021

jenswi-linaro commented Jun 14, 2021

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Jun 14, 2021

jforissier commented Jun 14, 2021

JensVankeirsbilck commented Oct 25, 2021

jenswi-linaro commented Oct 25, 2021

jenswi-linaro commented Oct 25, 2021

jforissier commented Oct 25, 2021

JensVankeirsbilck commented Oct 25, 2021

jenswi-linaro commented Oct 25, 2021

jforissier left a comment

Choose a reason for hiding this comment

jenswi-linaro commented Oct 25, 2021 • edited Loading

jenswi-linaro commented Oct 25, 2021

panda-balarka commented Oct 25, 2021

jenswi-linaro commented Oct 25, 2021

JensVankeirsbilck commented Oct 26, 2021

JensVankeirsbilck commented Oct 29, 2021

jenswi-linaro commented Oct 29, 2021

jenswi-linaro commented Dec 28, 2021

jforissier commented Dec 28, 2021

th0ma5b commented May 19, 2022

jenswi-linaro commented May 20, 2022

jenswi-linaro commented May 10, 2021 •

edited

Loading

jenswi-linaro commented Oct 25, 2021 •

edited

Loading