(take 2) NS Interrupt notification over async notif#5793
(take 2) NS Interrupt notification over async notif#5793etienne-lms wants to merge 9 commits intoOP-TEE:masterfrom
Conversation
etienne-lms
force-pushed
the
async-it-3
branch
from
4a9b534 to
669ab24
Compare
jenswi-linaro
left a comment
jenswi-linaro
left a comment
There was a problem hiding this comment.
Comments for "libutils: add bit_ffs_from()"
jenswi-linaro
left a comment
jenswi-linaro
left a comment
There was a problem hiding this comment.
Comments for "core: notif: interrupt notification"
jenswi-linaro
left a comment
jenswi-linaro
left a comment
There was a problem hiding this comment.
For commit "core: notif: fix input comment typo"
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
jenswi-linaro
left a comment
jenswi-linaro
left a comment
There was a problem hiding this comment.
Comments for "core: arm: smc: entry service for interrupt notification"
Why must normal world use a capability to tell OP-TEE that it wants to use interrupt notifications? Isn't it enough that normal world tries to enable or configure it to tell that?
| * as the second MSG arg struct for | ||
| * OPTEE_SMC_CALL_WITH_ARG | ||
| * Bit[31:8]: Reserved (MBZ) | ||
| * Bit[23:8]: The maximum interrupt event notification number |
There was a problem hiding this comment.
We should have a few words about how these "interrupt event notification numbers" are chosen and what they represent.
There was a problem hiding this comment.
Ok .The way I saw that is that it is platform specific and part of the platform bindings.
There was a problem hiding this comment.
Yes, that's fine. It should be clear then that these values may very well be completely unrelated to physical interrupt numbers or the GIC counterpart or whatever.
jenswi-linaro
left a comment
jenswi-linaro
left a comment
There was a problem hiding this comment.
Comments for "pta: it_notif: non-services for interrupt notification"
jenswi-linaro
left a comment
jenswi-linaro
left a comment
There was a problem hiding this comment.
Comments for "core: notif: add status helper functions for embedded tests"
jenswi-linaro
left a comment
jenswi-linaro
left a comment
There was a problem hiding this comment.
Comment for "core: pta: test: invoke pta command to test interrupt notif"
jenswi-linaro
left a comment
jenswi-linaro
left a comment
There was a problem hiding this comment.
Comment for "core: notif: add set_mask() fastcall callback handler"
|
It's unfortunate that we started to abbreviate interrupt as "IT", here it's clearly too short without a proper context. At other places, we abbreviate it as "ITR" or "INTR". The latter is the clearest but "ITR" isn't too bad either. |
|
Please squash the updates. |
etienne-lms
left a comment
etienne-lms
left a comment
There was a problem hiding this comment.
I'll squash the fixup commits.
etienne-lms
force-pushed
the
async-it-3
branch
from
c37d1b1 to
57100e2
Compare
|
Squashed fixup commits (+ few added fixes) and rebased on master. |
|
Appended |
|
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
|
keep alive. |
|
thanks |
|
Rebased on master (fixup commits not squashed). |
|
I've replaced the ITR_NOTIF PTA with SMC functions for interrupt notif activation/wakeup configuration. This change reveverts a commit from the series. I think I should squash the fixup commit... |
etienne-lms
left a comment
etienne-lms
left a comment
There was a problem hiding this comment.
Thanks for the review comments.
|
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
|
keep alive |
Adds bitstring function bit_ffs_from() that mimics bit_ffs() but looks from a start bit position given as argument, and defines bit_ffs() based on bit_ffs_from(). Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Fixes inline comment typo in OP-TEE standard SMCs description and CFG_CORE_ASYNC_NOTIF switch description. Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Implements interrupt notification support for events OP-TEE notifies to normal world as interrupt event multiplex on async notif physical interrupt. Such events can be logical events generated by OP-TEE as when the SCMI server emits a message to its non-secure agent, or physical events as a interrupt event dedicated to non-secure world but caught from a secure interrupt controller driver because. Interrupt identifier numbers (often referred to as itr_num) are platform specific. The feature is embedded upon new boolean config switch CFG_CORE_ITR_NOTIF=y. The feature is quite related to async notif as they share resources (the physical interrupt used to notify non-secure world) and use a similar mechanism to retrieve pending interrupt events. Platform driver willing to notify a interrupt event to non-secure must register the interrupt number with core functions notif_itr_register(). Function notif_itr_raise_event() allows core to notify non-secure world that an interrupt event is pending: storing pending event in bitstring and raising normal world async notif interrupt. Function notif_get_pending() retrieves pending interrupt events from the interrupt notif record, if any, informs whether other interrupt events are pending, retrieves do bottom half event and informs whether there are other pending async values than do bottom half. SMC fastcall function ID OPTEE_SMC_FUNCID_GET_NOTIF_ITR allows normal world to collect pending interrupt events from the async notif interrupt context. Core functions notif_itr_set_mask() masks/unmasks an interrupt notification. SMC fastcall function ID OPTEE_SMC_NOTIF_ITR_SET_MASK allows normal world to call this function. Core functions notif_itr_set_state() and notif_itr_set_wakeup() are services to enable and disable the interruption notification and to set or clear interrupt wakeup capability possibly used for the interrupt to wakeup the platform during low power states. SMC standard function IDs OPTEE_SMC_NOTIF_ITR_SET_STATE and OPTEE_SMC_NOTIF_ITR_SET_WAKEUP allow normal world to call the core API functions. Co-developed-by: Pascal Paillet <p.paillet@foss.st.com> Signed-off-by: Pascal Paillet <p.paillet@foss.st.com> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Defines NOTIF_ITR_VALUE_MAX from notif.h header file visible from core source files to ease addition of test interrupt lines for interrupt notification embedded tests. Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Adds notif_it_is_pending() and notif_it_is_masked() helper functions for use in interrupt notification embedded tests. Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Adds a command to the invoke test PTA to play with interrupt notifiers. The test uses interrupt lines not consumed by the Linux driver hence Linux will mask them since unused. A straight forward test is to unmask some interrupt events, raise them and check that they've been masked back by the normal world OS. Test is embedded upon boolean config switch CFG_ITR_NOTIF_TEST that depends on CFG_TEE_CORE_EMBED_INTERNAL_TESTS and CFG_CORE_ITR_NOTIF. Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Enables interrupt notifiers in platform vexpress-qemu_virt when embedded test are enabled. Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Default enable async notif on vexpress-qemuv8a platform flavor to exercise async and interrupt notif tests in OP-TEE OS CI tests. Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Enables async notif with interrupt notification using GIC PPI 15 as non-secure interrupt notifier for STM32MP13 variants. Enables interrupt notification for STM32MP13 variants that can generate up to 8 interrupt events in non-secure world. Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
|
Squashed fixup commits with few few changes: Note this is still related to a change in Linux kernel (see linaro-swg/linux#112) and OP-TEE/optee_test#663. |
|
For "libutils: add bit_ffs_from()" please apply: How about submitting "libutils: add bit_ffs_from()" and "core: notif: fix input comment typo" in a separate PR to get them out of the way? |
|
Ok, I've created #6009. |
| * | ||
| * @set_mask Fasctcall callback for (un)masking the event or NULL if not used | ||
| * @set_state Callback for enabling/disabling the event or NULL if not used | ||
| * @set_wakeup Callback for enabling/disabling standby wakeup source or NULL |
There was a problem hiding this comment.
Do you have an example of what this callback should do and when it's called?
There was a problem hiding this comment.
It is aimed to be called by non-secure world from enable_irq_wake() and disable_irq_wake() in Linux kernel (see https://elixir.bootlin.com/linux/v6.3/source/include/linux/interrupt.h#L482) or like. Such a call is relayed to OP-TEE core through optee_itr_notif_set_wake() handler.
There was a problem hiding this comment.
Yes, but what does it mean for OP-TEE? I don't think we should have to read the Linux kernel code to figure out what a callback in OP-TEE is supposed to do.
There was a problem hiding this comment.
edited: wrong discussion thread, sorry
struct notif_itr_ops is intended to 'interrupt notification', a service exposed to non-secure world.
For wake up source handled inside OP-TEE, OP-TEE drivers should use OP-TEE internal means (for example we could add a .set_wakeup handler field in struct itr_chip, once #5954 is reviewed), not this ops.
There was a problem hiding this comment.
Yes, but what does it mean for OP-TEE? I don't think we should have to read the Linux kernel code to figure out what a callback in OP-TEE is supposed to do.
This is a handler straight related to a service exposed to non-secure world. Maybe I should recall that in the description:
* struct notif_itr_ops - Interrupt notifier operations
*
* Operation handlers called upon an interrupt notification management
* request issues by non-secure world.
*
* ...
| * struct notif_itr_ops - Interrupt notifier operations | ||
| * | ||
| * @set_mask Fasctcall callback for (un)masking the event or NULL if not used | ||
| * @set_state Callback for enabling/disabling the event or NULL if not used |
There was a problem hiding this comment.
Do you have an example of what this callback should do compared to set_mask() and when it's called?
There was a problem hiding this comment.
This is intended to enable or disable the interrupt when the Linux driver consuming the interrupt need to.
There was a problem hiding this comment.
Yes, so from an OP-TEE point of view, what is the difference? What will be done differently inside OP-TEE?
There was a problem hiding this comment.
struct notif_itr_ops is intended to 'interrupt notification', a service exposed to non-secure world.
For wake up source handled inside OP-TEE, OP-TEE drivers should use OP-TEE internal means that are interrupt_mask(), interrupt_unmask(), interrupt_enable() and interrupt_disable() (API functions based on #5954 is reviewed). We could also add a .set_wakeup handler field in struct itr_chip for wakeup source support, if needed.
| * @itr_num Interrupt identifier provided by normal world | ||
| * @do_mask True to mask the event, false to unmask the event | ||
| * | ||
| * This function is called from a fastcall context |
There was a problem hiding this comment.
This is very useful information. How about adding this kind of information to all the functions called "directly" via an ABI call?
There was a problem hiding this comment.
Only notif_itr_set_mask() and notif_get_pending() (below) are called from a fastcall SMC function ID execution context. Other functions are call from thread contexts.
There was a problem hiding this comment.
The thread ABI calls are interesting too.
There was a problem hiding this comment.
Do you mean, for example in the description right below?
/*
* Activate (enable) or deactivate (disable) an interrupt notification
* Called upon a request from non-secure world, e.g. on a call to
* OPTEE_SMC_NOTIF_ITR_SET_STATE SMC function ID.
*
* @itr_num Interrupt identifier provided by normal world
* @do_enable True to enable the event detection, false disable it
*/
TEE_Result notif_itr_set_state(unsigned int itr_num, bool do_enable);
|
I've created OP-TEE/optee_docs#198 to describe the feature. |
|
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
|
keep alive |
|
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
3 participants
Supersedes #5761
Adds OP-TEE interrupt event notification to normal world with SMC transport, using existing async notif support.
Adds 2 SMC fastcall funcIDs to retrieve interrupt events and to mask an interrupt.
Adds an Itr Notif PTA to expose interrupt notif config means: set active state, set wakeup state.
Adds Itr Notif tests in Invoke test PTA: triggers and test some interrupt notifications.
Related regression tests in PR-640, tested on qemu_virt and stm32mp15.
Related to a change series in Linux kernel posted to LKML:
PATCH v2 1/3: dt bindings, PATCH v2 2/3: optee irqchip and PATCH v2 3/3: state/wakeup services.