Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Notif haf #6534

Merged
merged 5 commits into from
Jan 30, 2024
Merged

Notif haf #6534

merged 5 commits into from
Jan 30, 2024

Conversation

jenswi-linaro
Copy link
Contributor

No description provided.

@jenswi-linaro
Copy link
Contributor Author

Fixing the checkpatch warnings.

@jenswi-linaro
Copy link
Contributor Author

I'm investigating the Hafnium error.

@jforissier jforissier mentioned this pull request Dec 7, 2023
Merged
etienne-lms
etienne-lms reviewed Dec 7, 2023
View reviewed changes
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commits
"core: arm64.h: add DAIFBIT_{NATIVE,FOREIGN}_INTR" and
"core: move hafnium.h into hfic.c".

Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> for commit
"core: ffa: exit with native interrupts unmasked", but may have a second view.

@jenswi-linaro jenswi-linaro mentioned this pull request Dec 8, 2023
Merged
@jenswi-linaro
Copy link
Contributor Author

Tags applied. I've fixed a few errors in the commit message for "core: ffa: exit with native interrupts unmasked", it's hopefully easier to understand now.

@jenswi-linaro
Copy link
Contributor Author

It looks like the upgrade to QEMU v8.1.2 included some new prerequisites.

@jforissier
Copy link
Contributor

jforissier commented Dec 8, 2023 •
edited
Loading

It looks like the upgrade to QEMU v8.1.2 included some new prerequisites.

Absolutely 😉 no worries I'll take care of that, it should not take more than a few minutes

Edit: Docker image updated.

@jenswi-linaro
Copy link
Contributor Author

Rebased

@jenswi-linaro
Copy link
Contributor Author

"core: ffa: add notifications with SPMC at S-EL2 or EL3" and "core: hfic: fix HF_INTERRUPT HVC calls" remain to be reviewed.

@jenswi-linaro
Copy link
Contributor Author

Rebased, please help review "core: ffa: add notifications with SPMC at S-EL2 or EL3" and "core: hfic: fix HF_INTERRUPT HVC calls".

@jenswi-linaro
core: arm64.h: add DAIFBIT_{NATIVE,FOREIGN}_INTR
b948fbf 
Adds the two defines DAIFBIT_NATIVE_INTR and DAIFBIT_FOREIGN_INTR based
on DAIFBIT_IRQ and DAIFBIT_FIQ analogous with how
THREAD_EXCP_FOREIGN_INTR and THREAD_EXCP_NATIVE_INTR are defined.

DAIFBIT_NATIVE_INTR and DAIFBIT_FOREIGN_INTR can be used in assembly
instead of using #ifdef CFG_CORE_IRQ_IS_NATIVE_INTR.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
@jenswi-linaro
core: ffa: exit with native interrupts unmasked
6cf1372 
When exiting using the main exit/re-entry loop in
ffa_msg_send_direct_resp(), unmask native interrupts before the SMC
instruction and mask them again on re-entry. This guarantees that native
(aka secure) interrupts are not pending during exit. This also means
that when entering with FFA_INTERRUPT the interrupt will be handled
before thread_spmc_msg_recv() so there is no need to call
interrupt_main_handler() from thread_spmc_msg_recv() any longer.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
@jenswi-linaro
core: move hafnium.h into hfic.c
36480a8 
hafnium.h is only included from hfic.c so move the content into that
file instead. Comments trying to describe the paravirtualized interface
are removed and replaced by a link to official documentation.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
@jenswi-linaro
Copy link
Contributor Author

Rebased to hopefully fix the build error.

@jenswi-linaro
Copy link
Contributor Author

It seems that Rust is unhappy with something. @b49020 @jforissier do you know what's wrong?

etienne-lms
etienne-lms reviewed Jan 29, 2024
View reviewed changes
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> for commits
"core: hfic: fix HF_INTERRUPT HVC calls" and
"core: ffa: add notifications with SPMC at S-EL2 or EL3"
with comments addressed.

core/drivers/hfic.c Outdated
@@ -37,23 +40,28 @@ static struct hfic_data hfic_data __nex_bss;
static void hfic_op_add(struct itr_chip *chip __unused, size_t it __unused,
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove the __unused attribute from it argument.

core/arch/arm/kernel/thread_spmc.c
res = ffa_set_notification(notif_vm_id, my_endpoint_id, flags,
BIT64(do_bottom_half_value));
if (res) {
EMSG("notification set failed with error %d", res);
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer hexadecimal formating? "... %#"PRIx32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, wrong type, I"ll update to int.

@b49020
Copy link
Contributor

b49020 commented Jan 29, 2024

It seems that Rust is unhappy with something. @b49020 @jforissier do you know what's wrong?

It looks like there is some Rust toolchain conflict among buildroot provided one vs the one we have installed. Will have a deeper look to resolve those conflicts.

@jenswi-linaro
core: hfic: fix HF_INTERRUPT HVC calls
6502bf9 
The HF_INTERRUPT HVC calls has until now not been completely correct.
HF_INTERRUPT_ENABLE is used to enable/disable a virtual interrupt on the
current CPU while HF_INTERRUPT_RECONFIGURE is used to globally enable a
physical interrupt. So update the HFIC callbacks accordingly.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
@jenswi-linaro
core: ffa: add notifications with SPMC at S-EL2 or EL3
b1371af 
Adds support for asynchronous notifications via FF-A with SPMC at S-EL2
or EL3.

The SPMC is probed using FFA_FEATURES(FFA_NOTIFICATION_SET) to see if
the SPMC is support FF-A notifications.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
@jenswi-linaro
Copy link
Contributor Author

Comments addressed and tags applied.

@b49020
Copy link
Contributor

b49020 commented Jan 29, 2024

It seems that Rust is unhappy with something. @b49020 @jforissier do you know what's wrong?

It looks like there is some Rust toolchain conflict among buildroot provided one vs the one we have installed. Will have a deeper look to resolve those conflicts.

Since this is affecting all the PRs build, I propose OP-TEE/build#726 for now until I fix the underlying issue.

@jforissier jforissier merged commit 55cd94d into OP-TEE:master Jan 30, 2024
7 checks passed
@jenswi-linaro jenswi-linaro deleted the notif-haf branch January 30, 2024 10:59
odeprez
odeprez reviewed Jan 30, 2024
View reviewed changes
core/arch/arm/kernel/thread_spmc_a64.S
* Native interrupts unmasked while invoking SMC with caller
* provided parameters.
*/
msr daifclr, #DAIFBIT_NATIVE_INTR
Copy link
Contributor

Choose a reason for hiding this comment

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

I gather the change is merged now. Just to state that I retested by reverting this change, and using upstream Hafnium which implements a proper fix https://review.trustedfirmware.org/c/hafnium/hafnium/+/25051
On OP-TEE exiting the SPMC while a virtual interrupt is pending, the SPMC resumes again OP-TEE using the FFA_INTERRUPT interface signaling the virtual interrupt pending.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, good to know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etienne-lms etienne-lms etienne-lms left review comments

@odeprez odeprez odeprez left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jenswi-linaro @jforissier @b49020 @etienne-lms @odeprez