Skip to content

Core/Plat: Allow platform specific panic sequence - #6423

Closed
GseoC wants to merge 3 commits into
OP-TEE:masterfrom
GseoC:plat_panic
Closed

Core/Plat: Allow platform specific panic sequence#6423
GseoC wants to merge 3 commits into
OP-TEE:masterfrom
GseoC:plat_panic

Conversation

@GseoC

@GseoC GseoC commented Nov 2, 2023

Copy link
Copy Markdown
Contributor

When panicking, there may be the need to stop the other core or have a specific sequence.

Define a weak plat_panic() function to do such operations and add an implementation for STM32MP1x platforms.

Comment thread core/arch/arm/plat-stm32mp1/main.c Outdated
write_cntv_ctl(read_cntv_ctl() | ARM_CNTXCTL_IMASK);
}

/* SGI9 (secure SGI 1) informs targeted CPU it shall reset */

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.

Reported in #6412 (comment)_

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.

Done

@etienne-lms etienne-lms left a comment

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.

Could you remove the Change-id tag from 1st commit message?

@GseoC

GseoC commented Nov 2, 2023

Copy link
Copy Markdown
Contributor Author

Could you remove the Change-id tag from 1st commit message?

Yes, my bad

Comment thread core/arch/arm/plat-stm32mp1/main.c
Comment thread core/arch/arm/plat-stm32mp1/main.c Outdated
Comment thread core/arch/arm/plat-stm32mp1/main.c Outdated
Comment thread core/arch/arm/plat-stm32mp1/main.c Outdated
Comment thread core/arch/arm/plat-stm32mp1/main.c
Comment thread core/arch/arm/plat-stm32mp1/main.c
Comment thread core/arch/arm/plat-stm32mp1/main.c Outdated
Comment thread core/arch/arm/plat-stm32mp1/main.c Outdated
Comment thread core/arch/arm/plat-stm32mp1/main.c Outdated
Comment thread core/arch/arm/plat-stm32mp1/main.c Outdated

@jforissier jforissier left a comment

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.

For "core: panic: platform specific panic sequence":

One small comment, then:

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

Comment thread core/kernel/panic.c Outdated
GseoC added 3 commits November 2, 2023 16:55
Implement a new function plat_panic() for platform panic sequence.
The default weak implementation traps CPU in an endless loop.
Platforms can override the weak implementation for a specific
panic sequence.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Implement a platform specific sequence where the non-secure context
can be dumped when a panic occurs, should the log level be sufficient.

If there are two cores: raise an interrupt so that is can be stopped as
well.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Add missing error code management
Ass missing cpu_spin_unlock()
Other minor fixes

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
@GseoC

GseoC commented Nov 2, 2023

Copy link
Copy Markdown
Contributor Author

Comments adressed for MP1 implementation and @jforissier 's tag applied

@jenswikl

jenswikl commented Nov 3, 2023

Copy link
Copy Markdown
Contributor

For "core: panic: platform specific panic sequence" please apply:
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

@jenswikl

jenswikl commented Nov 6, 2023

Copy link
Copy Markdown
Contributor

I'm having second thoughts about this. I'm not so keen on all these weak functions acting like platform hooks, especially a platform hook like this that solves the problem for one platform, but by default leaves all other platforms in the dark.

I believe this would make sense as a generic option instead. With something like "core: itr_raise_sgi() updates" from #6394, but with an ITR_CPU_MASK_TO_OTHER_CPUS also it should be straight forward.

@etienne-lms

Copy link
Copy Markdown
Contributor

Would you prefer platforms to statically or dynamically register (or not) a platform panic handler, instead of a weak function? By statically I meant something like how register_ddr() is implemented.

Or an even more generic solution based on a config switches? Something like a pair of switch like CFG_CORE_HALT_CORES_ON_PANIC=y|n + CFG_CORE_HALT_CORES_ON_PANIC_SGI=<number>. Note SGI is an Arm specific naming.

ITR_CPU_MASK_TO_OTHER_CPUS would indeed be nice.

@jenswikl

jenswikl commented Nov 6, 2023

Copy link
Copy Markdown
Contributor

Would you prefer platforms to statically or dynamically register (or not) a platform panic handler, instead of a weak function? By statically I meant something like how register_ddr() is implemented.

No, that's still a platform solution.

Or an even more generic solution based on a config switches? Something like a pair of switch like CFG_CORE_HALT_CORES_ON_PANIC=y|n + CFG_CORE_HALT_CORES_ON_PANIC_SGI=<number>. Note SGI is an Arm specific naming.

Yes, something like this is what I had in mind.

ITR_CPU_MASK_TO_OTHER_CPUS would indeed be nice.

OK, I have a patch for this, I'll create a PR.

@jenswikl

jenswikl commented Nov 6, 2023

Copy link
Copy Markdown
Contributor

Feel free to cherry-pick https://github.com/jenswi-linaro/optee_os/tree/sgi_update and use that. I'm afraid that I've only been able to test ITR_CPU_MASK_TO_THIS_CPU in earlier versions of this patch and ITR_CPU_MASK_TO_OTHER_CPUS is untested.

@GseoC

GseoC commented Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

Feel free to cherry-pick https://github.com/jenswi-linaro/optee_os/tree/sgi_update and use that. I'm afraid that I've only been able to test ITR_CPU_MASK_TO_THIS_CPU in earlier versions of this patch and ITR_CPU_MASK_TO_OTHER_CPUS is untested.

Thank you,

I'll will try something generic and test it on our platforms.
As it will completely rework the way of doing it, I'll create a new P-R when ready.

@GseoC

GseoC commented Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

@GseoC GseoC closed this Nov 14, 2023
@GseoC

GseoC commented Nov 14, 2023

Copy link
Copy Markdown
Contributor Author

Closed, accepted version: #6440

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.

4 participants