arch: armv7-a: Fix memory corruption in SMP mode #3265

masayuki2009 · 2021-04-01T04:18:00Z

Summary

During the stress test with sabre-6quad:smp (QEMU), I noticed
that sometimes prefetch abort happens.
Finally, I found that g_irqtmp is corrupted.
This commit fixes this issue with the following changes
- Allocate g_irqtmp for all CPUs
- Load g_irqtmp only once in arm_vectorirq()
- In SMP mode, the IRQ mode stack is set in arm_start_handler()
- Also, access to the g_irqtmp is adjusted in arm_vectorirq()

Impact

IRQ handling for armv7-a

Testing

Tested with the following configs
- sabre-6quad:smp (QEMU, dev board)
- sabre-6quad:netnsh (QEMU)

Donny9 · 2021-04-01T06:54:19Z

armv7-a/arm_vectors.S: Assembler messages:
armv7-a/arm_vectors.S:204: Error: bad instruction `cpuindex r1'
Makefile:151: recipe for target 'armv7-a/arm_vectors.o' failed

armv7-a/arm_vectors.S: Line 108, This marco "cpuindex" don't used when enable CONFIG_SMP
#if !defined(CONFIG_SMP) && CONFIG_ARCH_INTERRUPTSTACK > 7
.macro cpuindex, index
.mov \index, #0
.endm
#endif

masayuki2009 · 2021-04-01T07:06:35Z

@Donny9

Which configuration are you using?

Donny9 · 2021-04-01T07:23:21Z

@Donny9

Which configuration are you using?

In our own project, it used A7 SMP. and apply this patch, the compile is broken.

masayuki2009 · 2021-04-01T07:25:59Z

In our own project, it used A7 SMP. and apply this patch, the compile is broken.

@Donny9

Perhaps, your project does not set CONFIG_ARCH_INTERRUPTSTACK.
Is my understanding correct?

Donny9 · 2021-04-01T07:36:07Z

In our own project, it used A7 SMP. and apply this patch, the compile is broken.

@Donny9

Perhaps, your project does not set CONFIG_ARCH_INTERRUPTSTACK.
Is my understanding correct?

Yes, we set CONFIG_ARCH_INTERRUPTSTACK=0, and CONFIG_SMP=y, and this cpuindex don't find.

masayuki2009 · 2021-04-01T07:40:22Z

Yes, we set CONFIG_ARCH_INTERRUPTSTACK=0, and CONFIG_SMP=y, and this cpuindex don't find.

@Donny9

OK, I've just fixed for imx6 case.
Please apply the patch to your board.

Donny9 · 2021-04-01T07:41:08Z

Yes, we set CONFIG_ARCH_INTERRUPTSTACK=0, and CONFIG_SMP=y, and this cpuindex don't find.

@Donny9

OK, I've just fixed for imx6 case.
Please apply the patch to your board.

Yes， i try it. Thank you.

jerpelea

LGTM

xiaoxiang781216 · 2021-04-01T11:48:32Z

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


+  /* Set IRQ mode stack for this CPU */
+
+  set_irqstack((uintptr_t)&g_irqtmp + (8  * this_cpu()));


why not move to arm_vectors.S?

@xiaoxiang781216

Because it's difficult to write the same thing in assembler at the beginning of arm_vectorirq().

I will study the code tonight to find wether can fix the bug in assemble code, will give the feedback tomorrow.

Donny9 · 2021-04-01T12:49:06Z

@masayuki2009 I enable ARM_THUMB， complie break:

{standard input}: Assembler messages:
{standard input}:69: Error: Thumb encoding does not support an immediate here -- `msr cpsr_c,#18'
Makefile:154: recipe for target 'armv7-a/arm_cpustart.o' failed

Donny9 · 2021-04-01T12:52:21Z

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

 	ldr		r0, .Lirqtmp			/* Points to temp storage */
+
+#ifdef CONFIG_SMP
+	cpuindex r1						/* r1 = cpuindex */


Is it possible directly get cpuindex?
mrc CP15_MPIDR(r1) /* Get R1=MPIDR /
and r1, r1, #MPIDR_CPUID_MASK
mov r1, r1, lsl #MPIDR_CPUID_SHIFT / r1 = cpuindex */

@Donny9

Why do you want not to use the macro?

acassis · 2021-04-01T18:08:22Z

Converted to Draft, waiting until @xiaoxiang781216 verify if it can be fixed in assembly code

masayuki2009 · 2021-04-01T21:43:16Z

{standard input}:69: Error: Thumb encoding does not support an immediate here -- `msr cpsr_c,#18'

@Donny9

I've just fixed the compile error in thumb2 mode.

masayuki2009 · 2021-04-01T22:07:55Z

Hmmm, Lint failed...

error pulling image configuration: received unexpected HTTP status: 503 No healthy IP available for the backend
Warning: Docker pull failed with exit code 1, back off 1.046 seconds before retry.
/usr/bin/docker pull ghcr.io/github/super-linter:v3.15.5

masayuki2009 · 2021-04-01T22:32:44Z

Hmm,

Download action repository 'actions/download-artifact@v1'
Warning: Failed to download action 'https://api.github.com/repos/actions/download-artifact/tarball/18f0f591fbc635562c815484d73b6e8e3980482e'. Error: Resource temporarily unavailable
Warning: Back off 23.69 seconds before retry.
Warning: Failed to download action 'https://api.github.com/repos/actions/download-artifact/tarball/18f0f591fbc635562c815484d73b6e8e3980482e'. Error: Resource temporarily unavailable
Warning: Back off 13.689 seconds before retry.
Error: Resource temporarily unavailable

acassis · 2021-04-04T14:54:00Z

@xiaoxiang781216 @Donny9 is it working now? May we merge it?

xiaoxiang781216 · 2021-04-04T15:06:48Z

I am working on this, and will provide a update tonight or tomorrow morning.

masayuki2009 · 2021-04-04T22:23:49Z

I rebased the branch and fixed conflicts.

masayuki2009 · 2021-04-05T11:51:40Z

Refactor the inline assembly code in the set_irqstack()

xiaoxiang781216 · 2021-04-05T13:26:16Z

@masayuki2009 , I will take a look tonight.

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

xiaoxiang781216 · 2021-04-05T18:54:31Z

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

+
+	.globl	g_irqtmp
+
 g_irqtmp:


can we avoid use the temporary storage? so SMP get resolved automatically.

@xiaoxiang781216

Hmm, I also tried to remove the temporary storage but I was not able to find how to do that.

I have an idea that:

Switch to SVC mode directly in the entry point

Save some registers to stack

Switch back IRQ/FIQ/ABT/UND mode

Read LR/SPRS register

Switch to SVC mode again

Step 4 should has enough free registers saved at step 2.

I have an idea that:

@xiaoxiang781216

Can you create a new PR for it?

Ok, I will try this afternoon. We hit the same issue on dual Cortex-A7 SoC.

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

Summary: - During the stress test with sabre-6quad:smp (QEMU), I noticed that sometimes prefetch abort happens. - Finally, I found that g_irqtmp is corrupted. - This commit fixes this issue with the following changes - Allocate g_irqtmp for all CPUs - Load g_irqtmp only once in arm_vectorirq() - In SMP mode, the IRQ mode stack is set in arm_start_handler() - Also, access to the g_irqtmp is adjusted in arm_vectorirq() - Apply the same logic to FIQ/ABORT/UNDEF stacks Impact: - IRQ/FIQ/ABORT/UNDEF handling for armv7-a Testing: - Tested with the following configs - sabre-6quad:smp (QEMU, dev board) - sabre-6quad:netnsh (QEMU) Signed-off-by: Masayuki Ishikawa <Masayuki.Ishikawa@jp.sony.com>

…ACK=0 Signed-off-by: Masayuki Ishikawa <Masayuki.Ishikawa@jp.sony.com>

xiaoxiang781216 · 2021-04-06T12:37:18Z

@masayuki2009 could you try this patch #3451?

masayuki2009 · 2021-04-06T13:46:49Z

#3451 resolved this issue.
So close this PR.

jerpelea self-requested a review April 1, 2021 11:17

jerpelea approved these changes Apr 1, 2021

View reviewed changes

xiaoxiang781216 reviewed Apr 1, 2021

View reviewed changes

Donny9 reviewed Apr 1, 2021

View reviewed changes

acassis marked this pull request as draft April 1, 2021 18:07

masayuki2009 force-pushed the armv7-a branch from 684be24 to ba309aa Compare April 1, 2021 21:41

masayuki2009 marked this pull request as ready for review April 1, 2021 21:49

masayuki2009 force-pushed the armv7-a branch from ba309aa to 702e605 Compare April 4, 2021 22:22

masayuki2009 force-pushed the armv7-a branch from 702e605 to bed9c5a Compare April 5, 2021 11:50

xiaoxiang781216 reviewed Apr 5, 2021

View reviewed changes

masayuki2009 added 2 commits April 6, 2021 09:13

arch: imx6: Fix compile with CONFIG_SMP=y and CONFIG_ARCH_INTERRUPTST…

a7cd9b2

…ACK=0 Signed-off-by: Masayuki Ishikawa <Masayuki.Ishikawa@jp.sony.com>

masayuki2009 force-pushed the armv7-a branch from bed9c5a to a7cd9b2 Compare April 6, 2021 00:15

xiaoxiang781216 mentioned this pull request Apr 6, 2021

arch/arm: Remove g_irqtmp, g_undeftmp and g_aborttmp #3451

Merged

masayuki2009 closed this Apr 6, 2021

masayuki2009 deleted the armv7-a branch April 8, 2021 07:51


		/* Set IRQ mode stack for this CPU */

		set_irqstack((uintptr_t)&g_irqtmp + (8 * this_cpu()));

arch: armv7-a: Fix memory corruption in SMP mode #3265

arch: armv7-a: Fix memory corruption in SMP mode #3265

Uh oh!

Conversation

masayuki2009 commented Apr 1, 2021

Summary

Impact

Testing

Uh oh!

Donny9 commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

masayuki2009 commented Apr 1, 2021

Uh oh!

Donny9 commented Apr 1, 2021

Uh oh!

masayuki2009 commented Apr 1, 2021

Uh oh!

Donny9 commented Apr 1, 2021

Uh oh!

masayuki2009 commented Apr 1, 2021

Uh oh!

Donny9 commented Apr 1, 2021

Uh oh!

jerpelea left a comment

Choose a reason for hiding this comment

Uh oh!

xiaoxiang781216 Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Donny9 commented Apr 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acassis commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

masayuki2009 commented Apr 1, 2021

Uh oh!

masayuki2009 commented Apr 1, 2021

Uh oh!

masayuki2009 commented Apr 1, 2021

Uh oh!

acassis commented Apr 4, 2021

Uh oh!

xiaoxiang781216 commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

masayuki2009 commented Apr 4, 2021

Uh oh!

masayuki2009 commented Apr 5, 2021

Uh oh!

xiaoxiang781216 commented Apr 5, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiaoxiang781216 commented Apr 6, 2021

Uh oh!

masayuki2009 commented Apr 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Donny9 commented Apr 1, 2021 •

edited

Loading

xiaoxiang781216 Apr 1, 2021 •

edited

Loading

acassis commented Apr 1, 2021 •

edited

Loading

xiaoxiang781216 commented Apr 4, 2021 •

edited

Loading