Add the optional interrupt stack to the Xtensa architecture #2014

Ouss4 · 2020-10-17T14:28:15Z

Summary

This PR has to commits:

Add the interrupt stack to the Xtensa architecture.
Extends the interrupt stack to SMP mode (dual-core in case of ESP32)

Impact

The option defaults to disabled, so no impact in current configurations.

Testing

Testing in hardware and QEMU with ostest.

Signed-off-by: Abdelatif Guettouche <[email protected]>

Ouss4 · 2020-10-17T14:49:32Z

@masayuki2009 is the use of XXX_intstack_base() in SMP case correct in XXX_dumpstate?
I based my change on other architectures, but it looks as if "base" is used instead of "top".
This not something related (only) to this PR but to dumpstate in general.

masayuki2009 · 2020-10-18T00:33:00Z

@masayuki2009 is the use of XXX_intstack_base() in SMP case correct in XXX_dumpstate?
I based my change on other architectures, but it looks as if "base" is used instead of "top".
This not something related (only) to this PR but to dumpstate in general.

@Ouss4
Thanks for the comments.
I will check the code later.

masayuki2009 · 2020-10-18T08:47:45Z

@masayuki2009 is the use of XXX_intstack_base() in SMP case correct in XXX_dumpstate?
I based my change on other architectures, but it looks as if "base" is used instead of "top".
This not something related (only) to this PR but to dumpstate in general.

@Ouss4
Thanks for the comments.
I will check the code later.

Hi @Ouss4,

I checked both imx6 (armv7-a) and cxd56xx (armv7-m) and found that
arm_assert.c expects arm_intstack_base() to return the top of the
interrupt stack for the current CPU.

static void up_dumpstate(void)
{
...
#ifdef CONFIG_SMP                                                                                                                                                             
  istackbase = (uint32_t)arm_intstack_base();                                                                                                                                 
#else                                                                                                                                                                         
  istackbase = (uint32_t)&g_intstackbase;                                                                                                                                     
#endif                                                                                                                                                                        
  istacksize = (CONFIG_ARCH_INTERRUPTSTACK & ~7);

Actually, &g_intstackbase stores "top" of the interrupt stack for non-SMP,
but the naming might be confusing. Anyway, we need to follow the same rule for SMP case.

However, I also noticed that arm_intstack_base() for imx6 and cxd56xx were incorrect.
Currently they return "bottom" of the interrupt stack for the current CPU.
So I will need to fix them later.

Ouss4 · 2020-10-18T09:16:13Z

However, I also noticed that arm_intstack_base() for imx6 and cxd56xx were incorrect.
Currently they return "bottom" of the interrupt stack for the current CPU.
So I will need to fix them later.

Yes, this is what I was reffering to. arm_intstack_base is returning the bottom of the stack while it was used assuming it returns the top of the stack.

This function is used for debugging only in arm_assert (arch_dumpstate for some architectures) and arm_checkstack. Its usage in xxx_checkstack is correct though.

masayuki2009 · 2020-10-19T01:11:39Z

However, I also noticed that arm_intstack_base() for imx6 and cxd56xx were incorrect.
Currently they return "bottom" of the interrupt stack for the current CPU.
So I will need to fix them later.

Yes, this is what I was reffering to. arm_intstack_base is returning the bottom of the stack while it was used assuming it returns the top of the stack.

This function is used for debugging only in arm_assert (arch_dumpstate for some architectures) and arm_checkstack. Its usage in xxx_checkstack is correct though.

@Ouss4

Hmm, I also found the following code in arm_initialize.c and arm_checkstack.c
I think we need to fix these as well.

in arm_initialize.c
...
#if defined(CONFIG_STACK_COLORATION) && CONFIG_ARCH_INTERRUPTSTACK > 3                                                                                                                       
static inline void up_color_intstack(void)                                                                                                                                                   
{                                                                                                                                                                                            
#ifdef CONFIG_SMP                                                                                                                                                                            
  uint32_t *ptr = (uint32_t *)arm_intstack_base();                                                                                                                                           
#else                                                                                                                                                                                        
  uint32_t *ptr = (uint32_t *)&g_intstackalloc;                                                                                                                                              
#endif                                                                                                                                                                                       
  ssize_t size;                                                                                                                                                                              
                                                                                                                                                                                             
  for (size = (CONFIG_ARCH_INTERRUPTSTACK & ~3);                                                                                                                                             
       size > 0;                                                                                                                                                                             
       size -= sizeof(uint32_t))                                                                                                                                                             
    {                                                                                                                                                                                        
      *ptr++ = INTSTACK_COLOR;                                                                                                                                                               
    }                                                                                                                                                                                        
}  
...

in arm_checkstack.c
...
#if CONFIG_ARCH_INTERRUPTSTACK > 3                                                                                                                                            
size_t up_check_intstack(void)                                                                                                                                                
{                                                                                                                                                                             
#ifdef CONFIG_SMP                                                                                                                                                             
  return do_stackcheck((FAR void *)arm_intstack_base(),                                                                                                                       
                        INT32_ALIGN_DOWN(CONFIG_ARCH_INTERRUPTSTACK));                                                                                                        
#else                                                                                                                                                                         
  return do_stackcheck((FAR void *)&g_intstackalloc,                                                                                                                          
                        INT32_ALIGN_DOWN(CONFIG_ARCH_INTERRUPTSTACK));                                                                                                        
#endif                                                                                                                                                                        
}   
...

Ouss4 · 2020-10-19T08:40:43Z

Hmm, I also found the following code in arm_initialize.c and arm_checkstack.c
I think we need to fix these as well.

@masayuki2009 Yes, however the use in arm_initialize.c and arm_checkstack.c is correct. Both expect to get the bottom of the stack. The only issue is arm_assert that uses arm_intstack_base expecting to get the "top" of the stack.

masayuki2009 · 2020-10-19T10:37:55Z

Hmm, I also found the following code in arm_initialize.c and arm_checkstack.c
I think we need to fix these as well.

@masayuki2009 Yes, however the use in arm_initialize.c and arm_checkstack.c is correct. Both expect to get the bottom of the stack. The only issue is arm_assert that uses arm_intstack_base expecting to get the "top" of the stack.

@Ouss4

Yeah, but the current use of arm_intstack_base() in in arm_initialize.c and arm_checkstack.c is very confusing for me. So I will introduce arm_intstack_allock() which returns the "bottom" address of the stack and will be called in arm_initialize.c and arm_checkstack.c Also, I will modify arm_intstack_base() so that I can return the "top" address of the stack. These modifications will be consistent with non-SMP case.

What do you think?

Ouss4 · 2020-10-19T11:04:35Z

Hmm, I also found the following code in arm_initialize.c and arm_checkstack.c
I think we need to fix these as well.

@masayuki2009 Yes, however the use in arm_initialize.c and arm_checkstack.c is correct. Both expect to get the bottom of the stack. The only issue is arm_assert that uses arm_intstack_base expecting to get the "top" of the stack.

@Ouss4

Yeah, but the current use of arm_intstack_base() in in arm_initialize.c and arm_checkstack.c is very confusing for me. So I will introduce arm_intstack_allock() which returns the "bottom" address of the stack and will be called in arm_initialize.c and arm_checkstack.c Also, I will modify arm_intstack_base() so that I can return the "top" address of the stack. These modifications will be consistent with non-SMP case.

What do you think?

I agree, this is the correct way to fix it. This will eliminate any confusion and be consistent with non-SMP case.
I'll modify this PR to include these changes too.

masayuki2009 · 2020-10-19T13:27:31Z

I agree, this is the correct way to fix it. This will eliminate any confusion and be consistent with non-SMP case.
I'll modify this PR to include these changes too.

@Ouss4
I wrote my code and pushed to https://github.com/masayuki2009/incubator-nuttx/tree/fix_arm_intstack_for_smp
I think the code is now simpler than before and basic test passed.
However, I will do more testing tomorrow and then send a PR.

Signed-off-by: Abdelatif Guettouche <[email protected]>

masayuki2009 · 2020-10-21T13:46:49Z

arch/xtensa/src/common/xtensa_int_handlers.S

@@ -68,6 +68,19 @@
 #include "chip_macros.h"
 #include "xtensa_timer.h"

+#if !defined(CONFIG_SMP) && CONFIG_ARCH_INTERRUPTSTACK > 15
+ .data
+ .align     16


For Xtensa architecture, does .align 16 mean 16-byte alignment?
For ARM architecture, .align n means 2^n but it confuses me. So I will refactor them for ARM later.

However, we can use .balign instead of .align.
Please see the following URL.
https://developer.arm.com/documentation/dui0742/c/migrating-arm-syntax-assembly-code-to-gnu-syntax/alignment

Yes, it means 16-byte alignement. (ref. https://sourceware.org/binutils/docs-2.25/as/Align.html)
Here is the relevant text:

The way the required alignment is specified varies from system to system. For ....and xtensa, the first expression is the alignment request in bytes.

There is also a clearer example here.

masayuki2009 · 2020-10-21T13:56:02Z

arch/xtensa/src/common/xtensa_int_handlers.S

+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+  setintstack a13 a14
+#endif
+


I think we need to adjust (i.e. subtract 16?) the stack pointer (a1?) to avoid data corruption, because g_cpu_intstack_top does not include offset now.

I'll run more tests with the adjustment this evening and verify the memories. Thanks for the reminder.
(Yes, a1 is sp)

@masayuki2009 I don't think that's necessary, same as the discussion we were having with ARM, the SP is decremented on procedures entry.

@Ouss4
Sorry for confusing you.

No problem, there was no confusion :)
I did look here and there to see how others are doing the same thing and I found it similar to what's proposed by this PR.

acassis · 2020-10-26T11:33:43Z

Hi @Ouss4 and @masayuki2009 is it done? LGTM???

Ouss4 · 2020-10-26T11:36:11Z

Hi @Ouss4 and @masayuki2009 is it done? LGTM???

It is done but I'm also monitoring #2061 to see if there is anything to add here.
Let's wait until that one is merged first.

masayuki2009 · 2020-10-26T22:44:10Z

LGTM

common/gnu/fork.S 29: unknown instruction .syntax unified --^ [asarm] (error apache#2230) common/gnu/fork.S 81: bad directive .type up_fork , function ------------------^ [asarm] (error apache#2067) armv7-m/arm_saveusercontext.S 31: unknown instruction .syntax unified --^ [asarm] (error apache#2230) armv7-m/arm_saveusercontext.S 55: bad directive .type up_saveusercontext , % function --^ [asarm] (error apache#2004) armv7-m/arm_saveusercontext.S 65: not within valid register range str r12 , [ r0 , ( 4 * ( ( ( 12 ) + ( 16 ) ) + 4 ) ) ] ------^ [asarm] (error apache#2004) armv7-m/arm_saveusercontext.S 66: not within valid register range str r14 , [ r0 , ( 4 * ( ( ( 12 ) + ( 16 ) ) + 5 ) ) ] ------^ [asarm] (error apache#2004) armv7-m/arm_saveusercontext.S 67: not within valid register range str r14 , [ r0 , ( 4 * ( ( ( 12 ) + ( 16 ) ) + 6 ) ) ] ------^ [asarm] (error apache#2014) armv7-m/arm_saveusercontext.S 72: expected a register str r1 , [ r0 , ( 4 * ( ( ( 12 ) + ( 16 ) ) + 7 ) ) ] ------------------^ [asarm] (error apache#2004) armv7-m/arm_saveusercontext.S 75: not within valid register range add r1 , r0 , ( 4 * ( ( ( 12 ) + ( 16 ) ) + 8 ) ) -----------^ [asarm] (error apache#2071) armv7-m/arm_saveusercontext.S 89: bad parameter stmia r0 ! , { r2 - r11 } --------^ [asarm] (error apache#2014) armv7-m/arm_saveusercontext.S 93: expected a register mov r1 , - 1 -----------^ Signed-off-by: yanghuatao <[email protected]>

common/gnu/fork.S 29: unknown instruction .syntax unified --^ [asarm] (error #2230) common/gnu/fork.S 81: bad directive .type up_fork , function ------------------^ [asarm] (error #2067) armv7-m/arm_saveusercontext.S 31: unknown instruction .syntax unified --^ [asarm] (error #2230) armv7-m/arm_saveusercontext.S 55: bad directive .type up_saveusercontext , % function --^ [asarm] (error #2004) armv7-m/arm_saveusercontext.S 65: not within valid register range str r12 , [ r0 , ( 4 * ( ( ( 12 ) + ( 16 ) ) + 4 ) ) ] ------^ [asarm] (error #2004) armv7-m/arm_saveusercontext.S 66: not within valid register range str r14 , [ r0 , ( 4 * ( ( ( 12 ) + ( 16 ) ) + 5 ) ) ] ------^ [asarm] (error #2004) armv7-m/arm_saveusercontext.S 67: not within valid register range str r14 , [ r0 , ( 4 * ( ( ( 12 ) + ( 16 ) ) + 6 ) ) ] ------^ [asarm] (error #2014) armv7-m/arm_saveusercontext.S 72: expected a register str r1 , [ r0 , ( 4 * ( ( ( 12 ) + ( 16 ) ) + 7 ) ) ] ------------------^ [asarm] (error #2004) armv7-m/arm_saveusercontext.S 75: not within valid register range add r1 , r0 , ( 4 * ( ( ( 12 ) + ( 16 ) ) + 8 ) ) -----------^ [asarm] (error #2071) armv7-m/arm_saveusercontext.S 89: bad parameter stmia r0 ! , { r2 - r11 } --------^ [asarm] (error #2014) armv7-m/arm_saveusercontext.S 93: expected a register mov r1 , - 1 -----------^ Signed-off-by: yanghuatao <[email protected]>

arch/xtensa: Add the optional interrupt stack.

af4bb9e

Signed-off-by: Abdelatif Guettouche <[email protected]>

Ouss4 force-pushed the intstack branch from 3e2a946 to febd53b Compare October 17, 2020 14:31

Ouss4 requested a review from masayuki2009 October 17, 2020 14:45

masayuki2009 mentioned this pull request Oct 20, 2020

Fix interrupt stack handlings for ARM SMP #2042

Merged

Ouss4 force-pushed the intstack branch from febd53b to 06b4a51 Compare October 20, 2020 21:58

arch/xtensa/src/esp32: SMP case of interruptstack.

06b4a51

Signed-off-by: Abdelatif Guettouche <[email protected]>

masayuki2009 reviewed Oct 21, 2020

View reviewed changes

Ouss4 mentioned this pull request Oct 23, 2020

Refactor arm interrupt stack related code #2061

Merged

masayuki2009 merged commit 58655d1 into apache:master Oct 26, 2020

Ouss4 deleted the intstack branch October 27, 2020 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the optional interrupt stack to the Xtensa architecture #2014

Add the optional interrupt stack to the Xtensa architecture #2014

Ouss4 commented Oct 17, 2020

Ouss4 commented Oct 17, 2020

masayuki2009 commented Oct 18, 2020

masayuki2009 commented Oct 18, 2020

Ouss4 commented Oct 18, 2020 •

edited

Loading

masayuki2009 commented Oct 19, 2020

Ouss4 commented Oct 19, 2020

masayuki2009 commented Oct 19, 2020

Ouss4 commented Oct 19, 2020

masayuki2009 commented Oct 19, 2020

masayuki2009 Oct 21, 2020 •

edited

Loading

Ouss4 Oct 21, 2020

masayuki2009 Oct 21, 2020 •

edited

Loading

Ouss4 Oct 21, 2020 •

edited

Loading

Ouss4 Oct 23, 2020

masayuki2009 Oct 23, 2020

Ouss4 Oct 23, 2020

acassis commented Oct 26, 2020

Ouss4 commented Oct 26, 2020

masayuki2009 commented Oct 26, 2020

Add the optional interrupt stack to the Xtensa architecture #2014

Add the optional interrupt stack to the Xtensa architecture #2014

Conversation

Ouss4 commented Oct 17, 2020

Summary

Impact

Testing

Ouss4 commented Oct 17, 2020

masayuki2009 commented Oct 18, 2020

masayuki2009 commented Oct 18, 2020

Ouss4 commented Oct 18, 2020 • edited Loading

masayuki2009 commented Oct 19, 2020

Ouss4 commented Oct 19, 2020

masayuki2009 commented Oct 19, 2020

Ouss4 commented Oct 19, 2020

masayuki2009 commented Oct 19, 2020

masayuki2009 Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Ouss4 Oct 21, 2020

Choose a reason for hiding this comment

masayuki2009 Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Ouss4 Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Ouss4 Oct 23, 2020

Choose a reason for hiding this comment

masayuki2009 Oct 23, 2020

Choose a reason for hiding this comment

Ouss4 Oct 23, 2020

Choose a reason for hiding this comment

acassis commented Oct 26, 2020

Ouss4 commented Oct 26, 2020

masayuki2009 commented Oct 26, 2020

Ouss4 commented Oct 18, 2020 •

edited

Loading

masayuki2009 Oct 21, 2020 •

edited

Loading

masayuki2009 Oct 21, 2020 •

edited

Loading

Ouss4 Oct 21, 2020 •

edited

Loading