Skip to content

Interrupt notification over async notif - #5761

Closed
etienne-lms wants to merge 6 commits into
OP-TEE:masterfrom
etienne-lms:notif
Closed

Interrupt notification over async notif#5761
etienne-lms wants to merge 6 commits into
OP-TEE:masterfrom
etienne-lms:notif

Conversation

@etienne-lms

@etienne-lms etienne-lms commented Jan 12, 2023

Copy link
Copy Markdown
Contributor

This P-R proposes 2 changes related to async notification.

The first one is the ability to use a per-cpu interrupt (PPI) as notification vehicle in normal world instead of an SPI. The related change in Linux kernel is available in the LKML:
https://lore.kernel.org/lkml/20230112145424.3791276-2-etienne.carriere@linaro.org/

The second one is an extension of async notif to allow OP-TEE to generate an interrupt event for a Linux kenrel interrupt consumer. Async notif was initially designed to schedule or wake an OP-TEE thread. This new "interrupt notification" feature is designed for when the consumer expects a interrupt event. The related Linux kernel change is available in the LKML:
DT bindings: https://lore.kernel.org/lkml/20230112145424.3791276-3-etienne.carriere@linaro.org/
optee driver: https://lore.kernel.org/lkml/20230112145424.3791276-4-etienne.carriere@linaro.org/

@etienne-lms

Copy link
Copy Markdown
Contributor Author

Updated above description with LKML links.

Comment thread core/arch/arm/tee/entry_fast.c Outdated
Comment thread core/arch/arm/tee/entry_fast.c Outdated

case OPTEE_SMC_GET_IT_NOTIF_VALUE:
if (IS_ENABLED(CFG_CORE_ASYNC_NOTIF))
get_it_value(args);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer get_it_notif_value() and set_it_notif_mask() below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Comment thread core/kernel/notif.c Outdated
if (!alloc_values_inited) {
bit_set(notif_alloc_values, NOTIF_VALUE_DO_BOTTOM_HALF);
if (IS_ENABLED(CFG_CORE_IT_NOTIF))
bit_set(notif_alloc_values, NOTIF_VALUE_DO_IT);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be done unconditionally to make sure it's not found by bit_ffc() below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Legacy implementation allows async notif value 1 be used for standard (see linux kernel impl.).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I don't follow. Can you give a pointer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest OP-TEE and Linux kernel consider notif value 1 as a generic purpose aysnc notif. So we cannot change Linux to unconditionally consider this value as reserved for "it notif". Therefore, on Linux side, this value is reserved for "it notif" only upon IT_NOTIF capability being enabled. In related Linux kernel patch (https://lore.kernel.org/lkml/20230112145424.3791276-4-etienne.carriere@linaro.org/):

+		else if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_IT_NOTIF &&
+			 value == OPTEE_SMC_ASYNC_NOTIF_VALUE_DO_IT)
+			handle_optee_it(optee);

I agree we could have the OP-TEE side to always reserve this value 1 for IT_NOTIF, but we cannot do that on Linux side. So for consistency, I bound it to the capabiliy on both side. Do you think it adds confusions?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. Yes, I think it's slightly confusing on the OP-TEE side, with a value that is only sometimes reserved. On the Linux side, we have no choice, but on the OP-TEE side, I believe it would be clearer to have it permanently reserved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With latest proposal, there is no more reserved async value related to interrupt notification.

This is at the cost of non-secure world will always invoke (smc fastcall) optee to query pending IRQ after what pending async value (thread related) will will queried.

Comment thread core/kernel/notif.c Outdated
@etienne-lms

Copy link
Copy Markdown
Contributor Author

Created #5772 for the PPI change. I'll remove that commit from this series and will focus on CFG_CORE_IT_NOTIF in this P-R.

@etienne-lms etienne-lms changed the title Async notif with a PPI + Interrupt notif Interrupt notification over async notif Jan 18, 2023
Implements notif_async_value_is_pending() helper function to know if
there are pending async values to notify.

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.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Implements interrupt notification support for interrupt event
OP-TEE needs to notify non-secure world about. The feature is embedded
upon new boolean config switch CFG_CORE_IT_NOTIF being enable.

notif_send_it() is used to notifies non-secure world that an interrupt
event is pending: storing pending event in bitstring and raising
non-secure world async notif interrupt.

notif_it_get_value() retrieves a pending interrupt event from the
interrupt event list, if any, and informs whether another interrupt
event is pending.

notif_it_set_mask() masks/unmasks an interrupt for event notification
to non-secure world.

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>
This change defines 2 SMC function IDs for exposing interrupt events
service to non-secure world.

SMC function ID OPTEE_SMC_GET_NOTIF_IT allows non-secure world to
retrieve pending interrupts and also whether there are pending async
values for suspended threaded sequences execution.

SMC function ID OPTEE_SMC_SET_IT_NOTIF_MASK allows non-secure world
to mask/disable and unmask/enable notification of an interrupt event.

The feature is negotiated between non-secure and secure worlds through
capabilities exchange and is enabled only when both worlds support the
capability.

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>
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>
@etienne-lms

Copy link
Copy Markdown
Contributor Author

I re-implemented the feature (PPI part removed) rebased on master (where PPI patch was merged) and split into several commits. The related Linux kernel changes to support interrupt notification must be updated. I'll send a PATCH v2 post and update this comment with a LKML ref (TODO).

Fix notif_it_get_value() against pending interrupts status.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
@etienne-lms

Copy link
Copy Markdown
Contributor Author

Digging deeper, here are improvements to do on the API. I want to minimize the number of world transition during the non-secure interrupt execution. I'll update. I also should come with a test setup we can exercice (e.g. CI with Qemu) implying a test pathc in our OP-TEE Linux fork.

@etienne-lms

Copy link
Copy Markdown
Contributor Author

Closing. Superseded by #5793.

@etienne-lms
etienne-lms deleted the notif branch February 9, 2023 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants