Core: add platform abort handler + stm32 SERC usage #7253
Conversation
core/arch/arm/kernel/abort.c
Outdated
| @@ -555,6 +560,8 @@ void abort_handler(uint32_t abort_type, struct thread_abort_regs *regs) | |||
|
|
|||
| switch (get_fault_type(&ai)) { | |||
| case FAULT_TYPE_IGNORE: | |||
| /* Allow platform-specific handling */ | |||
| plat_abort_handler(regs); | |||
There was a problem hiding this comment.
I'm not so keen on adding hooks that the compiler can't optimize away if unused.
How about using a config variable so the function call can be optimized away if not needed?
There was a problem hiding this comment.
Ok for using a config switch, how about CFG_EXTERNAL_ABORT_PLAT_HANDLER,
core/include/kernel/abort.h
Outdated
| /* Print abort info + stack dump to the console */ | ||
| void abort_print_error(struct abort_info *ai); | ||
|
|
||
| void abort_handler(uint32_t abort_type, struct thread_abort_regs *regs); | ||
|
|
||
| /* Platform overload, should be implemented in platform code */ | ||
| void plat_abort_handler(struct thread_abort_regs *regs); |
There was a problem hiding this comment.
Isn't it only external aborts we care about here? If so, how about plat_external_abort_handler()?
This function should probably take a struct abort_info *ai argument instead.
There was a problem hiding this comment.
Yes, it is about external aborts. Maybe we can even restrain the call to platform code on external aborts that are not table walks or table updates?
I'm fine with your suggestions
core/arch/arm/kernel/abort.c
Outdated
| @@ -555,6 +560,8 @@ void abort_handler(uint32_t abort_type, struct thread_abort_regs *regs) | |||
|
|
|||
| switch (get_fault_type(&ai)) { | |||
| case FAULT_TYPE_IGNORE: | |||
There was a problem hiding this comment.
We should add a FAULT_TYPE_EXTERNAL_ABORT if we need special treatment for external aborts.
There was a problem hiding this comment.
Ok so that type of fault should be returned when encountering CORE_MMU_FAULT_ASYNC_EXTERNAL if asynchronous and create another CORE_MMU_FAULT_SYNC_EXTERNAL then?
I think we also miss the synchronous external abort cases, looking at the documentation for AARCH64: "Synchronous External abort, not on translation tablewalk or hardware update of translation table." is 0x10 in FSR.
So 0x10 and 0x12->0x17 are all external aborts. We miss these descriptions in core/arch/arm/include/arm64.h
There was a problem hiding this comment.
Yes, I suppose it can be useful to tell synchronous and asynchronous extern aborts apart.
|
Updated with comments addressed. I'm having thoughts on the sharing of bindings between AARCH64 ESR_FSC_x and AARCH32 DFSR because it's supposed to to be architecturally mapped. Please let me know what you think of it as it is now. |
core/arch/arm/kernel/abort.c
Outdated
| void __weak plat_abort_handler(struct abort_info *ai __unused) | ||
| { | ||
| /* Do nothing */ | ||
| } |
There was a problem hiding this comment.
Is this weak function needed?
There was a problem hiding this comment.
For whatever reason, if the switch is enabled and the function is not defined? Given that the switch is intended for this purpose, I think there is indeed no need for this weak function.
There was a problem hiding this comment.
If the switch is enabled, the platform should implement the handler. Having a build error is a nice way to trigger that it's missing IMO.
core/include/kernel/abort.h
Outdated
| /* Print abort info + stack dump to the console */ | ||
| void abort_print_error(struct abort_info *ai); | ||
|
|
||
| void abort_handler(uint32_t abort_type, struct thread_abort_regs *regs); | ||
|
|
||
| /* Platform overload, should be implemented in platform code */ |
There was a problem hiding this comment.
Could you explicitly mention CFG_EXTERNAL_ABORT_PLAT_HANDLER must be enabled to have the platform handler be called. I think the function label should explicitly mention _external_abort_ if called only for external aborts.
There was a problem hiding this comment.
Ok, will change to that
Platforms may have specific code to handle an abort when fault type is FAULT_TYPE_IGNORE. Add plat_abort_handler() that can be overridden at platform level. Signed-off-by: Gatien Chevallier <[email protected]>
When a data abort occurs and its fault type is FAULT_TYPE_IGNORE, it may be an abort generated by the SERC hardware block. Check if a SERC Illegal Access was caught and print the SERC register and panic() if that is the case. Signed-off-by: Gatien Chevallier <[email protected]>
Differentiate Async/Sync external aborts, create a new fault type and introduce CFG_EXTERNAL_ABORT_PLAT_HANDLER switch. Signed-off-by: Gatien Chevallier <[email protected]>
Synchronize with the updates of the other patch. Signed-off-by: Gatien Chevallier <[email protected]>
Type fixes and rename to plat_external_abort_handler Signed-off-by: Gatien Chevallier <[email protected]>
Copyright fix and use renamed function. Signed-off-by: Gatien Chevallier <[email protected]>
GseoC
changed the title
|
Removed "RFC" from the P-R title, comments addressed |
Implement a platform abort handler to handle use cases such as data aborts generated by peripherals on the bus.
These kinds of abort could be caused by platform-specific features, hence the weak function.