arch: xtensa: Fix xtensa error handler

[aasthagr]

Force an early return from exception when a stack check failure is detected at interrupt/exception level. This helps to ensure that ARCH_EXCEPT() does not return.

[dcpleung]

This needs more explaining on why this is here, how this works, and what kind of issues we need to be aware of. Next time someone else looking at this would be confused.

[dcpleung]

I am still hesitant on this. We would have seen the issue on other Xtensa platforms already if the condition has been handled incorrectly. That we haven't seen the problem on other platforms with this common code suggests the issue might be somewhere else.

[edersondisouza]

I am still hesitant on this. We would have seen the issue on other Xtensa platforms already if the condition has been handled incorrectly. That we haven't seen the problem on other platforms with this common code suggests the issue might be somewhere else.

The thing is what does CODE_UNREACHABLE do on different platforms - it may be 'do nothing' vs 'break stuff'. It seems it does nothing on like, qemu_xtensa built with gcc, but may behave differently on other platforms/toolchains.

[aasthagr]

I am still hesitant on this. We would have seen the issue on other Xtensa platforms already if the condition has been handled incorrectly. That we haven't seen the problem on other platforms with this common code suggests the issue might be somewhere else.

The thing is what does CODE_UNREACHABLE do on different platforms - it may be 'do nothing' vs 'break stuff'. It seems it does nothing on like, qemu_xtensa built with gcc, but may behave differently on other platforms/toolchains.

Agree. Different toolchains are behaving differently to what CODE_UNREACHABLE does. But irrespective the xtensa_arch_except should not return which is what this PR would fix. If that returns, having CODE_UNREACHABLE macro when built using xcc identifies the issue.

[dcpleung]

Please do not remove my change request.

[dcpleung]

I prefer manipulating nested as close to the end of that function as possible:

diff --git a/arch/xtensa/core/xtensa-asm2.c b/arch/xtensa/core/xtensa-asm2.c
index 3e31dcaa93..14c68eab6f 100644
--- a/arch/xtensa/core/xtensa-asm2.c
+++ b/arch/xtensa/core/xtensa-asm2.c
@@ -215,6 +215,7 @@ static inline DEF_INT_C_HANDLER(1)
 void *xtensa_excint1_c(int *interrupted_stack)
 {
        int cause, vaddr, *bsa = *(int **)interrupted_stack;
+       bool is_fatal_error = false;
 
        __asm__ volatile("rsr.exccause %0" : "=r"(cause));
 
@@ -240,6 +241,8 @@ void *xtensa_excint1_c(int *interrupted_stack)
                /* Default for exception */
                int reason = K_ERR_CPU_EXCEPTION;
 
+               is_fatal_error = true;
+
                /* We need to distinguish between an ill in xtensa_arch_except,
                 * e.g for k_panic, and any other ill. For exceptions caused by
                 * xtensa_arch_except calls, we also need to pass the reason_p
@@ -253,9 +256,6 @@ void *xtensa_excint1_c(int *interrupted_stack)
                        cause = 63;
                        __asm__ volatile("wsr.exccause %0" : : "r"(cause));
                        reason = bsa[BSA_A2_OFF/4];
-                       /* Skip ILL to RETW */
-                       bsa[BSA_PC_OFF/4] += 3;
-                       pc = (void *)bsa[BSA_PC_OFF/4];
                }
 
                LOG_ERR(" ** FATAL EXCEPTION");
@@ -280,6 +280,36 @@ void *xtensa_excint1_c(int *interrupted_stack)
                                     (void *)interrupted_stack);
        }
 
+
+       if (is_fatal_error) {
+               uint32_t ignore;
+
+               /* We are going to manipulate _current_cpu->nested manually.
+                * Since the error is fatal, for recoverable errors, code
+                * execution must not return back to the current thread as
+                * it is being terminated (via above z_xtensa_fatal_error()).
+                * So we need to prevent more interrupts coming in which
+                * will affect the nested value as we are going outside of
+                * normal interrupt handling procedure.
+                *
+                * Setting nested to 1 has two effects:
+                * 1. Force return_to() to choose a new thread.
+                *    Since the current thread is being terminated, it will
+                *    not be chosen again.
+                * 2. When context switches to the newly chosen thread,
+                *    nested must be zero for normal code execution,
+                *    as that is not in interrupt context at all.
+                *    After returning from this function, the rest of
+                *    interrupt handling code will decrement nested,
+                *    resulting it being zero before switching to another
+                *    thread.
+                */
+               __asm__ volatile("rsil %0, " STRINGIFY(XCHAL_NMILEVEL)
+                                : "=r" (ignore) : : );
+
+               _current_cpu->nested = 1;
+       }
+
        return return_to(interrupted_stack);
 }

[ceolin]

I am still hesitant on this. We would have seen the issue on other Xtensa platforms already if the condition has been handled incorrectly. That we haven't seen the problem on other platforms with this common code suggests the issue might be somewhere else.

The thing is what does CODE_UNREACHABLE do on different platforms - it may be 'do nothing' vs 'break stuff'. It seems it does nothing on like, qemu_xtensa built with gcc, but may behave differently on other platforms/toolchains.

The problem was just noticed when `CODE_UNREACHABLE`` was added, much likely before that the exception happened and the handler returned to thread passing that instruction and the thread just finished further.

[aasthagr]

Please do not remove my change request.

Sorry just re-requested your review. Didnt remove the change request.

[marc-hb]

This commit breaks compilation with RG-2017.8-linux/XtensaTools/bin/xt-xcc used for TGL/ADL/RPL:

zephyr/arch/xtensa/core/xtensa-asm2.c: In function ‘xtensa_excint1_c’:
zephyr/arch/xtensa/core/xtensa-asm2.c:306: error: expected string literal before ‘)’ token

I wish we could upgrade away from this toolchain but it's unfortunately not going to happen:

• XCC RG-2017.8-linux: generated object code is affected by -g2 level (the default level) + longer source paths thesofproject/sof#7114 (2017 toolchain bug closed as wontfix)

cc: @kv2019i , @lgirdwood

[dcpleung]

@marc-hb see if #56790 fixes that.