Refactor arm interrupt stack related code #2061

masayuki2009 · 2020-10-22T08:23:00Z

Summary

Background:
- This PR was created based on the discussion at Fix interrupt stack handlings for ARM SMP #2042
This PR refactors interrupt stack related code for all ARM architectures
Also, this PR includes a fix for CPUx Idle stack for Cortex-A SMP
This PR consists of the following commits
commit 1: arch: armv7-a: Fix style warnings in smp.h
commit 2: arch: armv7-a: Fix style warnings in arm_cpuidlestack.c
commit 3: arch: armv7-a: Fix CPUx IDLE stack top for SMP
- This commit fixes CPUx IDLE stack top for SMP
- Also removes SMP_STACK_TOP from smp.h
commit 4: boards: sabre-6quad: Update nsh/defconfig
- Add CONFIG_DEBUG_FULLOPT=y
- Add CONFIG_DEBUG_SYMBOLS=y
- Remove CONFIG_HOST_WINDOWS=y
- Add CONFIG_READLINE_CMD_HISTORY=y
- Add CONFIG_STACK_COLORATION=y
commit 5: armv7-a, imx6: Refactor interrupt stack related code
- Remove -4/-8 offset coding in imx_irq.c and arm_vectors.S
- Instead, add SP adjustment after calling setirqstack/setfiqstack
- Also, fix alignments for g_intstackalloc
- Fix comments on the user stack pointer in arm_vectors.S
- Also, fix up_dumpstate() to extract the user stack pointer
- NOTE: stack pointer alignment is 8-byte
commit 6: arm, c5471: Refactor interrupt stack related code
- Apply the same logic for armv7-a
- NOTE: stack pointer alignment is 4-byte
commit 7: arch: armv7-r: Refactor interrupt stack related code
- Apply the same logic for armv7-a
- NOTE: stack pointer alignment is 8-byte
commit 8: boards: stm32f4discovery: Update wifi/defconfig
- Add CONFIG_ARCH_INTERRUPTSTACK=2048
- Add CONFIG_ARMV7M_LAZYFPU=y
- Add CONFIG_TESTING_OSTEST_FPUSIZE=132
commit 9: armv7-m, cxd56xx, lc823450: Refactor interrupt stack related code
- Remove +4/-8 offset coding
- Instead, add SP adjustment after calling setintstack
- Also, fix alignments for g_intstackalloc
- NOTE: stack pointer alignment is 8-byte
commit 10: arch: armv8-m: Refactor interrupt stack related code
- Apply the same logic for armv7-m
- NOTE: stack pointer alignment is 8-byte
commit 11: arch: armv6-m: Refactor interrupt stack related code
- Apply the same logic for armv7-m
- NOTE: stack pointer alignment is 4-byte
commit 12: arm: armv7-a: Fix kernel stack dump in arm_assert.c
- This commit fixes kernel stack dump information

Impact

All ARM architectures with interrupt stack enabled
Affects armv7-a with kernel build

Testing

Tested with sabre-6quad:smp with QEMU
Tested with sabre-6quad:nsh with QEMU
Built with c5471evm.nsh (CONFIG_ARCH_INTERRUPTSTACK=2048)
Built with ea3131:nsh (CONFIG_ARCH_INTERRUPTSTACK=2048)
Tested with spresense:wifi_smp
Tested with lc823450:smp
Tested with stm32f4discovery:wifi
Not tested for armv7-r
Not tested for armv8-m
Built with freedom-kl25z:nsh (CONFIG_ARCH_INTERRUPTSTACK=2048)
Not tested for armv6-m
Built with sama5d4-ek:knsh but not tested

Signed-off-by: Masayuki Ishikawa <[email protected]>

Ouss4 · 2020-10-22T08:45:41Z

arch/arm/src/armv7-m/gnu/arm_exception.S

 g_intstackalloc:
-	.skip	((CONFIG_ARCH_INTERRUPTSTACK + 4) & ~7)
+	.skip	(CONFIG_ARCH_INTERRUPTSTACK & ~7)


Shouldn't we align up here? ((CONFIG_ARCH_INTERRUPTSTACK + 7) & ~7)

@Ouss4
I agree.

the alignment need sync with the code in stack setup and dump. for example here is line 70:

# if defined(CONFIG_BUILD_PROTECTED) && CONFIG_ARCH_INTERRUPTSTACK < 8

if we select the align up, the above logic is wrong and should change to CONFIG_ARCH_INTERRUPTSTACK < 1.
So, I would like to keep the logic as simple as possible instead bringing up more inconsistentence like this. The round down logic is more suitable choice because I don't think there is much difference betwen 2000 and 2008 if user set CONFIG_ARCH_INTERRUPTSTACK to 2006. The consistent round strategy across all related code is more important.

@xiaoxiang781216 why would that line be wrong? We use CONFIG_ARCH_INTERRUPTSTACK < ALIGN_REQUIREMENT to mean that there is no interrupt stack. We are not changing CONFIG_ARCH_INTERRUPTSTACK.
But yes, when dumping and checking the stack we always use the align down. Not sure why, if we have a consistent INTSTACK_SIZE (whether aligned up or down) wouldn't that suffice?

If the intension is aligned up, CONFIG_ARCH_INTERRUPTSTACK> 0 && CONFIG_ARCH_INTERRUPTSTACK < 7 is a correct stack value(even it's crazy that the stack has 8 bytes, but it correct from logic). Yes, as I mention before the key point is to use the alignup or aligndown consistently in all place, but since algindown is use in many place and there isn't real difference between alignup and aligndown, I would prefer to use the aligndown across the related code.

@Ouss4
Thanks, I keep the commit.

Here is the thing. Nuttx does not support nested interrupt. If a design is super low on memory (it happens) the interrupt stack allocation has to not round down. If know the CONFIG (FPU etc) the size of the int stack is a fixed number . If I ask for That number and get a stack that is less it will over write memory. This will be a debugging nightmare! Please round it UP

This PR doesn't change the round behaviour, please don't do the different thing in one PR.

need keep the old code after disscussion

arch/arm/src/armv7-a/arm_vectors.S

davids5 · 2020-10-22T11:07:10Z

@masayuki2009 Thank you for the PR and all the information on what was done. Would you please add some back ground as to why these changes are needed?

masayuki2009 · 2020-10-22T11:27:04Z

@masayuki2009 Thank you for the PR and all the information on what was done. Would you please add some back ground as to why these changes are needed?

@davids5
Good question!
I forgot to add the most important information on this PR.
This PR was created based on the discussion at #2042.

Anyway, thanks for pointing out.
I will add a comment to the PR summary first.

arch/arm/src/armv7-a/smp.h

arch/arm/src/armv7-a/arm_assert.c

arch/arm/src/armv7-a/arm_vectors.S

arch/arm/src/arm/arm_vectors.S

arch/arm/src/armv7-m/gnu/arm_lazyexception.S

arch/arm/src/armv8-m/arm_lazyexception.S

arch/arm/src/armv8-m/arm_exception.S

arch/arm/src/armv6-m/arm_exception.S

arch/arm/src/armv7-m/gnu/arm_exception.S

Signed-off-by: Masayuki Ishikawa <[email protected]>

masayuki2009 · 2020-10-23T07:07:15Z

@Ouss4 @xiaoxiang781216
I modified the code and pushed it with -f.
Also, I updated the summary.

xiaoxiang781216

LGTM.

xiaoxiang781216 · 2020-10-23T08:29:12Z

@masayuki2009 do you have plan to fix -4/-8 issue for idle thread stack and normal task/thread stack?

xiaoxiang781216 · 2020-10-23T08:50:31Z

@Ouss4 do you have more comment? otherwise, I will merge this patch.

davids5 · 2020-10-23T09:04:18Z

@xiaoxiang781216, @MasayukiIshikawa, @Ouss4

I would like to test this on HW.

I also think we need to define Top, bottom, base in terms of addresses.

Thing like this read wrong

if (sp < istackbase && sp > istackbase - istacksize)

high address
<- top, bound

<-sp
<- bottom, base
low addresses

I need to review this more carefully, but it look to me that were are adding instructions for alignment when the compiler can do it.
We need to all ways allocate larger for alignment.

Update: we are adding instructions for alignment - I was looking not the latest changes @xiaoxiang781216 had asked for.

xiaoxiang781216 · 2020-10-23T09:16:26Z

@xiaoxiang781216, @MasayukiIshikawa, @Ouss4

I would like to test this on HW.

Sure, let's wait your test.

I also think we need to define Top, bottom, base in terms of addresses.

Thing like this read wrong

if (sp < istackbase && sp > istackbase - istacksize)

high address
<- top, bound

<-sp
<- bottom, base
low addresses

Yes, the term usage is strange, but it is an unrelated issue. It's better to create new PR to fix the term issue.

I need to review this more carefully, but it look to me that were are adding instructions for alignment when the compiler can do it.
We need to all ways allocate larger for alignment.

Ouss4 · 2020-10-23T09:52:03Z

LGTM, but let's wait for David's input.

I also think we need to define Top, bottom, base in terms of addresses.

In terms of address, "base" is "top" for a push-down stack, this is what's making things a bit confusing. However, I believe it's used consistently: xx_alloc = bottom and xx_base = top or bottom depending on the stack.

davids5 · 2020-10-23T10:29:19Z

@masayuki2009 - I have tested this. My only 2 concerns are 1) Not rounding UP (see #2061 (comment)), and 2 this change:

Would you help me understand if is it correct?

Are we using Descending Empty or Descending Full?

davids5 · 2020-10-23T10:50:41Z

Yes, the term usage is strange, but it is an unrelated issue. It's better to create new PR to fix the term issue.

@xiaoxiang781216 Agreed it can be changes in another PR.

In terms of address, "base" is "top" for a push-down stack, this is what's making things a bit confusing. However, I believe it's used consistently: xx_alloc = bottom and xx_base = top or bottom depending on the stack.

Yes Memory allocations are base that is understood. The other is a visual thing for me. (I was indoctrinated by my first debugger http://www.pestingers.net/pdfs/s100-computers/vector-graphics/vector-raid-man.pdf - I wish there were pics)

I find it incorrect and confusing on descending stack arch.

Please read this with a smile....it it written with one......

Where do we put the lamp shade on a lamp? On the top or the base.
If we hang fly paper from a pole? On the base it hits the floor. On the top it does not, and it grows downward
What is the base address of a UART?

xiaoxiang781216 · 2020-10-23T11:31:58Z

@masayuki2009 - I have tested this. My only 2 concerns are 1) Not rounding UP (see [#2061 (comment)]

The rounddown exist for a long time and nobody complain this behvaiour. if you think the roundup is better, please create a new PR to fix it.

(#2061 (comment))), and 2 this change:

Would you help me understand if is it correct?

Are we using Descending Empty or Descending Full?

We can't select here, ARM require the descending full stack:
https://static.docs.arm.com/ihi0042/f/IHI0042F_aapcs.pdf

davids5 · 2020-10-23T12:32:49Z

@masayuki2009 - I have tested this. My only 2 concerns are 1) Not rounding UP (see [#2061 (comment)]

The rounddown exist for a long time and nobody complain this behvaiour. if you think the roundup is better, please create a new PR to fix it.

(#2061 (comment))), and 2 this change:

Would you help me understand if is it correct?
Are we using Descending Empty or Descending Full?

We can't select here, ARM require the descending full stack:
https://static.docs.arm.com/ihi0042/f/IHI0042F_aapcs.pdf

Thanks @xiaoxiang781216

So..

Descending-full then implies:
SP ->last write.
push is decrement then store

Correct?

Then isn't the changes with SP-X wrong? The value at [SP-X] get overwritten by any push's in the call chain of arm_decodeirq.

Or am I seeing this incorrectly?

xiaoxiang781216 · 2020-10-23T12:39:59Z

So..

Descending-full then implies:
SP ->last write.
push is decrement then store

Correct?

Yes, it is correct.

Then isn't the changes with SP-X wrong? The value at [SP-X] get overwritten by any push's in the call chain of arm_decodeirq.

The ! at the end will write back the sp - 4 to sp, so the next push will allocate the new location in the next slot.

Or am I seeing this incorrectly?

davids5 · 2020-10-23T12:49:24Z

@masayuki2009 @xiaoxiang781216

On the alignment please see the 86400a2 and the blame. I think in this PR we are reverting a change that fixed a real problem.

davids5 · 2020-10-23T12:53:01Z

So..
Descending-full then implies:
SP ->last write.
push is decrement then store
Correct?

Yes, it is correct.

Then isn't the changes with SP-X wrong? The value at [SP-X] get overwritten by any push's in the call chain of arm_decodeirq.

The ! at the end will write back the sp - 4 to sp, so the next push will allocate the new location in the next slot.

Or am I seeing this incorrectly?

I did not even notice it! Thank you!

xiaoxiang781216 · 2020-10-23T13:06:03Z

@masayuki2009 @xiaoxiang781216

On the alignment please see the 86400a2 and the blame. I think in this PR we are reverting a change that fixed a real problem.

from the change, the stack space allocation round to the near(may roundup or rounddown), but the sp always point to the rounddown location. So I can't understand what problem he want to fix without the detail commit message. At least, the change isn't consistent with other related code(init sp and stack check).

davids5 · 2020-10-23T13:10:29Z

@masayuki2009 @xiaoxiang781216
On the alignment please see the 86400a2 and the blame. I think in this PR we are reverting a change that fixed a real problem.

from the change, the stack space allocation round to the near(may roundup or rounddown), but the sp always point to the rounddown location. So I can't understand what problem he want to fix without the detail commit message.

xiaoxiang781216 · 2020-10-23T13:25:10Z

@davids5 , from the description +4 is good fix to support 4 bytes and 8 bytes alignment for different arm arch.

arch/arm/src/armv7-a/arm_vectors.S

xiaoxiang781216 · 2020-10-23T13:27:54Z

arch/arm/src/armv7-m/gnu/arm_exception.S

 g_intstackalloc:
-	.skip	((CONFIG_ARCH_INTERRUPTSTACK + 4) & ~7)
+	.skip	(CONFIG_ARCH_INTERRUPTSTACK & ~7)


need keep the old code after disscussion

arch/arm/src/armv7-r/arm_vectors.S

arch/arm/src/armv8-m/arm_exception.S

arch/arm/src/armv8-m/arm_lazyexception.S

Summary: - This commit fixes CPUx IDLE stack top for SMP - Also removes SMP_STACK_TOP from smp.h Impact: - Affects armv7-a SMP only Testing: - Tested with sabre-6quad:smp (QEMU) Signed-off-by: Masayuki Ishikawa <[email protected]>

Summary: - Add CONFIG_DEBUG_FULLOPT=y - Add CONFIG_DEBUG_SYMBOLS=y - Remove CONFIG_HOST_WINDOWS=y - Add CONFIG_READLINE_CMD_HISTORY=y - Add CONFIG_STACK_COLORATION=y Impact: - Affects sabre-6quad:nsh only Testing: - Tested with QEMU Signed-off-by: Masayuki Ishikawa <[email protected]>

Summary: - Remove -4/-8 offset coding in imx_irq.c and arm_vectors.S - Instead, add SP adjustment after calling setirqstack/setfiqstack - Fix off-by-one irq/fiq stack allocation in 8-byte aligned arch - Fix comments on the user stack pointer in arm_vectors.S - Also, fix up_dumpstate() to extract the user stack pointer - NOTE: stack pointer alignment is 8-byte Impact: - Affects armv7-a with interrupt stack enabled Testing: - Tested with sabre-6quad:smp with QEMU - Tested with sabre-6quad:nsh with QEMU Signed-off-by: Masayuki Ishikawa <[email protected]>

Summary: - Apply the same logic for armv7-a - NOTE: stack pointer alignment is 4-byte Impact: - Affects arm (arm7/9) and c5471 with interrupt stack enabled Testing: - Built with c5471evm.nsh (CONFIG_ARCH_INTERRUPTSTACK=2048) - Built with ea3131:nsh (CONFIG_ARCH_INTERRUPTSTACK=2048) - Not tested but should work Signed-off-by: Masayuki Ishikawa <[email protected]>

Summary: - Apply the same logic for armv7-a - NOTE: stack pointer alignment is 8-byte Impact: - Affects armv7-r with interrupt stack enabled Testing: - Not tested but should work Signed-off-by: Masayuki Ishikawa <[email protected]>

Summary: - Add CONFIG_ARCH_INTERRUPTSTACK=2048 - Add CONFIG_ARMV7M_LAZYFPU=y - Add CONFIG_TESTING_OSTEST_FPUSIZE=132 Impact: - Affects stm32f4discovery:wifi only Testing: - Tested with ostest, uSD card, Wi-Fi Signed-off-by: Masayuki Ishikawa <[email protected]>

Summary: - Remove +4/-8 offset coding - Also, fix alignments for g_intstackalloc - NOTE: stack pointer alignment is 8-byte Impact: - Affects armv7-m with interrupt stack enabled Testing: - Tested with spresense:wifi_smp - Tested with lc823450:smp - Tested with stm32f4discovery:wifi Signed-off-by: Masayuki Ishikawa <[email protected]>

Summary: - Apply the same logic for armv7-m - NOTE: stack pointer alignment is 8-byte Impact: - Affects armv8-m with interrupt stack enabled Testing: - Not tested but should work Signed-off-by: Masayuki Ishikawa <[email protected]>

Summary: - Apply the same logic for armv7-m - NOTE: stack pointer alignment is 4-byte Impact: - Affects armv6-m with interrupt stack enabled Testing: - Built with freedom-kl25z:nsh (CONFIG_ARCH_INTERRUPTSTACK=2048) - Not tested but should work Signed-off-by: Masayuki Ishikawa <[email protected]>

Summary: - This commit fixes kernel stack dump information Impact: - Affects armv7-a with kernel build Testing: - Built with sama5d4-ek:knsh - Not tested Signed-off-by: Masayuki Ishikawa <[email protected]>

masayuki2009 · 2020-10-24T01:53:25Z

@Ouss4, @davids5, @xiaoxiang781216
Thanks for your comments.
I've just read all discussions and fixed the commits again.

@davids5
Thanks for imforming us the "ARM: Fix off-by-one ..." issue.
I think the PR was posted to the old NuttX repository on bitbucket.
However, the repository had been removed.
So how can we find the PR?

xiaoxiang781216 · 2020-10-24T05:59:26Z

LGTM, @davids5 please help verify and merge the change.

davids5 · 2020-10-24T12:30:24Z

LGTM, @davids5 please help verify and merge the change.

@xiaoxiang781216 I will, but it will Monday.

davids5 · 2020-10-24T12:32:32Z

@Ouss4, @davids5, @xiaoxiang781216
Thanks for your comments.
I've just read all discussions and fixed the commits again.

@davids5
Thanks for imforming us the "ARM: Fix off-by-one ..." issue.
I think the PR was posted to the old NuttX repository on bitbucket.
However, the repository had been removed.
So how can we find the PR?

hmmm, That pic was from my email, maybe we can ask @patacongo if it can be restored.

davids5

@masayuki2009 - I have tested this on an M7 and M3 core. AOK!

masayuki2009 · 2020-10-26T22:34:05Z

@masayuki2009 - I have tested this on an M7 and M3 core. AOK!

@davids5
Thanks!

arch: armv7-a: Fix style warnings in smp.h

d922922

Signed-off-by: Masayuki Ishikawa <[email protected]>

Ouss4 reviewed Oct 22, 2020

View reviewed changes

masayuki2009 force-pushed the refactor_arm_irqstack branch from 91b8f9d to 3a0b9ec Compare October 22, 2020 09:09

xiaoxiang781216 reviewed Oct 22, 2020

View reviewed changes

arch: armv7-a: Fix style warnings in arm_cpuidlestack.c

d427a3e

Signed-off-by: Masayuki Ishikawa <[email protected]>

masayuki2009 force-pushed the refactor_arm_irqstack branch from 3a0b9ec to 920390f Compare October 23, 2020 06:13

xiaoxiang781216 approved these changes Oct 23, 2020

View reviewed changes

xiaoxiang781216 requested changes Oct 23, 2020

View reviewed changes

masayuki2009 added 4 commits October 24, 2020 09:34

arch: armv7-a: Fix CPUx IDLE stack top for SMP

0cdd055

Summary: - This commit fixes CPUx IDLE stack top for SMP - Also removes SMP_STACK_TOP from smp.h Impact: - Affects armv7-a SMP only Testing: - Tested with sabre-6quad:smp (QEMU) Signed-off-by: Masayuki Ishikawa <[email protected]>

masayuki2009 added 6 commits October 24, 2020 10:07

arch: armv7-r: Refactor interrupt stack related code

cffcb79

Summary: - Apply the same logic for armv7-a - NOTE: stack pointer alignment is 8-byte Impact: - Affects armv7-r with interrupt stack enabled Testing: - Not tested but should work Signed-off-by: Masayuki Ishikawa <[email protected]>

arch: armv8-m: Refactor interrupt stack related code

c575a46

Summary: - Apply the same logic for armv7-m - NOTE: stack pointer alignment is 8-byte Impact: - Affects armv8-m with interrupt stack enabled Testing: - Not tested but should work Signed-off-by: Masayuki Ishikawa <[email protected]>

arm: armv7-a: Fix kernel stack dump in arm_assert.c

df8c323

Summary: - This commit fixes kernel stack dump information Impact: - Affects armv7-a with kernel build Testing: - Built with sama5d4-ek:knsh - Not tested Signed-off-by: Masayuki Ishikawa <[email protected]>

masayuki2009 force-pushed the refactor_arm_irqstack branch from 920390f to df8c323 Compare October 24, 2020 01:25

xiaoxiang781216 approved these changes Oct 24, 2020

View reviewed changes

Ouss4 mentioned this pull request Oct 26, 2020

Add the optional interrupt stack to the Xtensa architecture #2014

Merged

davids5 approved these changes Oct 26, 2020

View reviewed changes

davids5 merged commit 904a602 into apache:master Oct 26, 2020

masayuki2009 deleted the refactor_arm_irqstack branch October 26, 2020 23:51

masayuki2009 mentioned this pull request Nov 17, 2020

Fix interrupt stack handlings for ARM SMP #2042

Merged

masayuki2009 added the backport/10.0 label Nov 18, 2020

btashton mentioned this pull request Nov 19, 2020

[BACKPORT #1939 #2042 #2061 #2204] smp/irq fixes and improvements #2340

Merged

Refactor arm interrupt stack related code #2061

Refactor arm interrupt stack related code #2061

Conversation

masayuki2009 commented Oct 22, 2020 • edited Loading

Summary

Impact

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ouss4 Oct 22, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davids5 commented Oct 22, 2020

masayuki2009 commented Oct 22, 2020 • edited Loading

masayuki2009 commented Oct 23, 2020 • edited Loading

xiaoxiang781216 left a comment

Choose a reason for hiding this comment

xiaoxiang781216 commented Oct 23, 2020

xiaoxiang781216 commented Oct 23, 2020

davids5 commented Oct 23, 2020 • edited Loading

xiaoxiang781216 commented Oct 23, 2020

Ouss4 commented Oct 23, 2020

davids5 commented Oct 23, 2020

davids5 commented Oct 23, 2020 • edited Loading

xiaoxiang781216 commented Oct 23, 2020 • edited Loading

davids5 commented Oct 23, 2020

xiaoxiang781216 commented Oct 23, 2020

davids5 commented Oct 23, 2020

davids5 commented Oct 23, 2020

xiaoxiang781216 commented Oct 23, 2020 • edited Loading

davids5 commented Oct 23, 2020

xiaoxiang781216 commented Oct 23, 2020

Choose a reason for hiding this comment

masayuki2009 commented Oct 24, 2020

xiaoxiang781216 commented Oct 24, 2020

davids5 commented Oct 24, 2020

davids5 commented Oct 24, 2020

davids5 left a comment

Choose a reason for hiding this comment

masayuki2009 commented Oct 26, 2020

masayuki2009 commented Oct 22, 2020 •

edited

Loading

Ouss4 Oct 22, 2020 •

edited

Loading

masayuki2009 commented Oct 22, 2020 •

edited

Loading

masayuki2009 commented Oct 23, 2020 •

edited

Loading

davids5 commented Oct 23, 2020 •

edited

Loading

davids5 commented Oct 23, 2020 •

edited

Loading

xiaoxiang781216 commented Oct 23, 2020 •

edited

Loading

xiaoxiang781216 commented Oct 23, 2020 •

edited

Loading