Core: add platform abort handler + stm32 SERC usage #7253

GseoC · 2025-01-30T17:26:10Z

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.

core/include/kernel/abort.h

jenswi-linaro · 2025-01-31T07:40:12Z

core/arch/arm/kernel/abort.c

@@ -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);


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?

Ok for using a config switch, how about CFG_EXTERNAL_ABORT_PLAT_HANDLER,

jenswi-linaro · 2025-01-31T07:42:08Z

core/include/kernel/abort.h

 /* 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);


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.

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

jenswi-linaro · 2025-01-31T07:43:13Z

core/arch/arm/kernel/abort.c

@@ -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:


We should add a FAULT_TYPE_EXTERNAL_ABORT if we need special treatment for external aborts.

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

Yes, I suppose it can be useful to tell synchronous and asynchronous extern aborts apart.

GseoC · 2025-01-31T14:53:05Z

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/mm/core_mmu_lpae.c

core/include/drivers/stm32_serc.h

core/arch/arm/kernel/abort.c

etienne-lms · 2025-02-05T16:15:51Z

core/arch/arm/kernel/abort.c

+void __weak plat_abort_handler(struct abort_info *ai __unused)
+{
+	/* Do nothing */
+}


Is this weak function needed?

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.

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.

etienne-lms · 2025-02-05T16:20:15Z

core/include/kernel/abort.h

 /* 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 */


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.

Ok, will change to that

GseoC · 2025-02-07T09:04:35Z

Removed "RFC" from the P-R title, comments addressed

etienne-lms · 2025-02-07T14:33:45Z

core/include/kernel/abort.h

 /* 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.
+ * CFG_EXTERNAL_ABORT_PLAT_HANDLER must be set to have the platform handler


"overload" is not applicable since there is no weak implementation. I suggest to rephrase to
"Platform specific handler for external abort exceptions.".

Prefer "enable" over "set" at line 38.

etienne-lms · 2025-02-07T15:08:17Z

core/arch/arm/mm/core_mmu_lpae.c

@@ -1444,6 +1444,8 @@ enum core_mmu_fault core_mmu_get_fault_type(uint32_t fault_descr)
 	assert(fault_descr & FSR_LPAE);

 	switch (fault_descr & FSR_STATUS_MASK) {
+	case 0x8: /* b01000 Asynchronous extern abort */
+		return CORE_MMU_FAULT_ASYNC_EXTERNAL;


If I'm right, case 0x8 (0b010000) is a synchronous external abort.
while case 0x9 (0b010001) is an asynchronous external abort.

By the way, I think the debug event is wrong. Debug event is indeed 0b100010 but that's 0x22 not 0x12. Will deserve a specific commit (outside the scope of this P-R).

There is indeed an error on the value here, it is 0x10 for synchronous external abort.
Extract from ARM A32 documentation when TTCBR.EAE == 1:

0b000000 Address size fault in translation table base register.
0b000001 Address size fault, level 1.
0b000010 Address size fault, level 2.
0b000011 Address size fault, level 3.
0b000101 Translation fault, level 1
0b000110 Translation fault, level 2.
0b000111 Translation fault, level 3.
0b001001 Access flag fault, level 1.
0b001010 Access flag fault, level 2.
0b001011 Access flag fault, level 3.
0b001101 Permission fault, level 1.
0b001110 Permission fault, level 2.
0b001111 Permission fault, level 3.
0b010000 Synchronous External abort, not on translation table walk.
0b010001 Asynchronous SError exception.
0b010101 Synchronous External abort on translation table walk,level 1.
0b010110 Synchronous External abort on translation table walk,level 2.
0b010111 Synchronous External abort on translation table walk,level 3.
0b011000 Synchronous parity or ECC error on memory access,not on translation table walk. FEAT_RAS is not implemented
0b011001 Asynchronous SError exception, from a parity or ECC error on memory access. FEAT_RAS is not implemented
0b011101 Synchronous parity or ECC error on memory access on translation table walk, level 1. FEAT_RAS is not implemented
0b011110 Synchronous parity or ECC error on memory access on translation table walk, level 2. FEAT_RAS is not implemented
0b011111 Synchronous parity or ECC error on memory access on translation table walk, level 3. FEAT_RAS is not implemented
0b100001 Alignment fault. 0b100010 Debug exception.
0b110000 TLB conflict abort.
0b110100 IMPLEMENTATION DEFINED fault (Lockdown).
0b110101 IMPLEMENTATION DEFINED fault (Unsupported Exclusive access).

I will also add a patch to fix the debug event part

etienne-lms · 2025-02-14T12:06:45Z

core/arch/arm/arm.mk

@@ -207,6 +207,10 @@ endif
 # this in platform config if different.
 CFG_MAX_CACHE_LINE_SHIFT ?= 6

+# CFG_EXTERNAL_ABORT_PLAT_HANDLER is used to implement platform-specific
+# handling of external abort using the plat_external_abort_handler() function.


I suggest to replace "using" with "implementing".

etienne-lms · 2025-02-14T12:35:38Z

core/arch/arm/plat-stm32mp2/main.c

+
+void plat_external_abort_handler(struct abort_info *ai __unused)
+{
+	/* If fault is ignored, it could be due to a SERC event */


Prefer /* External abort may be due to SERC events */.

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]>

Fix ARM32 LPAE values for external aborts and add some for table walks. Rework some comments. Signed-off-by: Gatien Chevallier <[email protected]>

Rework comment Signed-off-by: Gatien Chevallier <[email protected]>

Fix use case where stm32_serc_handle_ilac() is called before SERC driver is probed. Nothing to do in this case. Signed-off-by: Gatien Chevallier <[email protected]>

GseoC · 2025-02-17T11:51:53Z

Comments addressed and added core: fix debug event fault value for ARM32 with LPAE.

I can squash the commits if necessary

According to ARM documentation, the debug event fault value is indeed 0b100010, which is 0x22, not 0x12. Fix this value in core_mmu_get_fault_type(). Fixes: 0eff3e9 ("arm32: Adds LPAE support") Signed-off-by: Gatien Chevallier <[email protected]>

jenswi-linaro reviewed Jan 31, 2025

View reviewed changes

GseoC commented Jan 31, 2025

View reviewed changes

core/arch/arm/mm/core_mmu_lpae.c Show resolved Hide resolved

etienne-lms reviewed Feb 5, 2025

View reviewed changes

GseoC force-pushed the plat-abort branch from 6e2b153 to 885ccf1 Compare February 7, 2025 09:04

GseoC changed the title ~~(RFC)Core: add platform abort handler + stm32 SERC usage~~ Core: add platform abort handler + stm32 SERC usage Feb 7, 2025

etienne-lms reviewed Feb 14, 2025

View reviewed changes

GseoC added 9 commits February 17, 2025 10:28

core: add platform-specific abort handler

8f72cd9

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]>

[Review] core: add platform-specific abort handler

7f6ba07

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]>

[Review] plat-stm32mp2: add platform-specific abort handler

1c8ada1

Synchronize with the updates of the other patch. Signed-off-by: Gatien Chevallier <[email protected]>

[Review] core: add platform-specific abort handler

a66f034

Type fixes and rename to plat_external_abort_handler Signed-off-by: Gatien Chevallier <[email protected]>

[Review] plat-stm32mp2: add platform-specific abort handler

7cb8476

Copyright fix and use renamed function. Signed-off-by: Gatien Chevallier <[email protected]>

[Review] core: add platform-specific abort handler

3488b62

Fix ARM32 LPAE values for external aborts and add some for table walks. Rework some comments. Signed-off-by: Gatien Chevallier <[email protected]>

[Review] plat-stm32mp2: add platform-specific abort handler

c1b5798

Rework comment Signed-off-by: Gatien Chevallier <[email protected]>

[Review] plat-stm32mp2: add platform-specific abort handler

02fb8e3

Fix use case where stm32_serc_handle_ilac() is called before SERC driver is probed. Nothing to do in this case. Signed-off-by: Gatien Chevallier <[email protected]>

GseoC force-pushed the plat-abort branch from 885ccf1 to 8ab635b Compare February 17, 2025 11:51

GseoC force-pushed the plat-abort branch from 8ab635b to b211c2d Compare February 17, 2025 11:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: add platform abort handler + stm32 SERC usage #7253

Core: add platform abort handler + stm32 SERC usage #7253

GseoC commented Jan 30, 2025

jenswi-linaro Jan 31, 2025

GseoC Jan 31, 2025

jenswi-linaro Jan 31, 2025

GseoC Jan 31, 2025

jenswi-linaro Jan 31, 2025

GseoC Jan 31, 2025 •

edited

Loading

jenswi-linaro Jan 31, 2025

GseoC commented Jan 31, 2025

etienne-lms Feb 5, 2025

GseoC Feb 7, 2025

etienne-lms Feb 7, 2025

etienne-lms Feb 5, 2025

GseoC Feb 7, 2025

GseoC commented Feb 7, 2025

etienne-lms Feb 7, 2025

etienne-lms Feb 7, 2025

GseoC Feb 17, 2025 •

edited

Loading

GseoC Feb 17, 2025

etienne-lms Feb 14, 2025

etienne-lms Feb 14, 2025

GseoC commented Feb 17, 2025

		@@ -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:

Core: add platform abort handler + stm32 SERC usage #7253

Are you sure you want to change the base?

Core: add platform abort handler + stm32 SERC usage #7253

Conversation

GseoC commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GseoC Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GseoC commented Jan 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GseoC commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GseoC Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GseoC commented Feb 17, 2025

GseoC Jan 31, 2025 •

edited

Loading

GseoC Feb 17, 2025 •

edited

Loading