ARM: v7: get rid of boot time mini stack

arch/arm/mm/proc-v7.S +0 -1

@@ -549,16 +555,6 @@ __errata_finish:
  THUMB(	orr	r0, r0, #1 << 30	)	@ Thumb exceptions
 	ret	lr				@ return to head.S:__ret
 
-	.align	2

Quoted 4 lines · Ard Biesheuvel

arch/arm/mm/proc-v7.S +0 -0

@@ -549,16 +555,6 @@ __errata_finish:
  THUMB(      orr     r0, r0, #1 << 30        )       @ Thumb exceptions
      ret     lr                              @ return to head.S:__ret
 

arch/arm/mm/proc-v7.S __errata_finish: · line 558

Quoted 11 lines

arch/arm/mm/proc-v7.S +0 -10

-     .align  2
-__v7_setup_stack_ptr:
-     .word   PHYS_RELATIVE(__v7_setup_stack, .)
-ENDPROC(__v7_setup)
-
-     .bss
-     .align  2
-__v7_setup_stack:
-     .space  4 * 7                           @ 7 registers
-
      __INITDATA

You should have preserved the ENDPROC here.

The v7 boot code uses a small chunk of BSS to preserve some register contents across a call to v7_invalidate_l1 that occurs with the MMU and caches disabled. Memory accesses in such cases are tricky on v7+, given that the architecture permits some unintuitive behaviors (it is implementation defined whether accesses done with the MMU and caches off may hit in the caches, and on SoCs that incorporate off-core system caches, this behavior appears to be different even between cache levels). Also, cache invalidation is not safe under virtualization if the intent is to retain stores issued directly to DRAM, given that the hypervisor may upgrade invalidate operations to clean+invalidate, resulting in DRAM contents to be overwritte by the dirty cachelines that we were trying to evict in the first place.

So let's address this issue, by removing the need for this stack to exist in the first place: v7_invalidate_l1 can be rewritten to use fewer registers, which means fewer registers need to be preserved, and we have enough spare registers available.

Quoted 13 lines · Ard Biesheuvel

The v7 boot code uses a small chunk of BSS to preserve some register contents across a call to v7_invalidate_l1 that occurs with the MMU and caches disabled. Memory accesses in such cases are tricky on v7+, given that the architecture permits some unintuitive behaviors (it is implementation defined whether accesses done with the MMU and caches off may hit in the caches, and on SoCs that incorporate off-core system caches, this behavior appears to be different even between cache levels). Also, cache invalidation is not safe under virtualization if the intent is to retain stores issued directly to DRAM, given that the hypervisor may upgrade invalidate operations to clean+invalidate, resulting in DRAM contents to be overwritte by the dirty cachelines that we were trying to evict in the first place.

Quoted 4 lines

So let's address this issue, by removing the need for this stack to exist in the first place: v7_invalidate_l1 can be rewritten to use fewer registers, which means fewer registers need to be preserved, and we have enough spare registers available.

That is excellent.

I wonder why r1-r3 were preserved though.

Quoted 16 lines · Nicolas Pitre

Quoted 13 lines · Ard Biesheuvel

The v7 boot code uses a small chunk of BSS to preserve some register contents across a call to v7_invalidate_l1 that occurs with the MMU and caches disabled. Memory accesses in such cases are tricky on v7+, given that the architecture permits some unintuitive behaviors (it is implementation defined whether accesses done with the MMU and caches off may hit in the caches, and on SoCs that incorporate off-core system caches, this behavior appears to be different even between cache levels). Also, cache invalidation is not safe under virtualization if the intent is to retain stores issued directly to DRAM, given that the hypervisor may upgrade invalidate operations to clean+invalidate, resulting in DRAM contents to be overwritte by the dirty cachelines that we were trying to evict in the first place.

Quoted 9 lines

Quoted 4 lines

So let's address this issue, by removing the need for this stack to exist in the first place: v7_invalidate_l1 can be rewritten to use fewer registers, which means fewer registers need to be preserved, and we have enough spare registers available.

That is excellent.

I wonder why r1-r3 were preserved though.

r1 and r2 are documented in head.S as

* The processor init function will be called with:
* r1 - machine type
* r2 - boot data (atags/dt) pointer

but preserving the value of r3 does not seem necessary. Perhaps this is a leftover from old code?

Quoted 27 lines · Ard Biesheuvel

Quoted 16 lines · Nicolas Pitre

Quoted 13 lines · Ard Biesheuvel

The v7 boot code uses a small chunk of BSS to preserve some register contents across a call to v7_invalidate_l1 that occurs with the MMU and caches disabled. Memory accesses in such cases are tricky on v7+, given that the architecture permits some unintuitive behaviors (it is implementation defined whether accesses done with the MMU and caches off may hit in the caches, and on SoCs that incorporate off-core system caches, this behavior appears to be different even between cache levels). Also, cache invalidation is not safe under virtualization if the intent is to retain stores issued directly to DRAM, given that the hypervisor may upgrade invalidate operations to clean+invalidate, resulting in DRAM contents to be overwritte by the dirty cachelines that we were trying to evict in the first place.

Quoted 9 lines

Quoted 4 lines

So let's address this issue, by removing the need for this stack to exist in the first place: v7_invalidate_l1 can be rewritten to use fewer registers, which means fewer registers need to be preserved, and we have enough spare registers available.

That is excellent.

I wonder why r1-r3 were preserved though.

Quoted 8 lines

r1 and r2 are documented in head.S as

* The processor init function will be called with:
* r1 - machine type
* r2 - boot data (atags/dt) pointer

but preserving the value of r3 does not seem necessary. Perhaps this is a leftover from old code?

Still, further down in the same comment it is said:

* On return, the CPU will be ready for the MMU to be turned on,
* r0 will hold the CPU control register value, r1, r2, r4, and
* r9 will be preserved. r5 will also be preserved if LPAE.

But you're now clobbering r1 and r2. And when this returns, code execution goes to __enable_mmu whose comment also mentions the expected r1 and r2 content. That would need adjusting too.

Quoted 45 lines · Nicolas Pitre

Quoted 27 lines · Ard Biesheuvel

Quoted 16 lines · Nicolas Pitre

Quoted 13 lines · Ard Biesheuvel

The v7 boot code uses a small chunk of BSS to preserve some register contents across a call to v7_invalidate_l1 that occurs with the MMU and caches disabled. Memory accesses in such cases are tricky on v7+, given that the architecture permits some unintuitive behaviors (it is implementation defined whether accesses done with the MMU and caches off may hit in the caches, and on SoCs that incorporate off-core system caches, this behavior appears to be different even between cache levels). Also, cache invalidation is not safe under virtualization if the intent is to retain stores issued directly to DRAM, given that the hypervisor may upgrade invalidate operations to clean+invalidate, resulting in DRAM contents to be overwritte by the dirty cachelines that we were trying to evict in the first place.

Quoted 9 lines

Quoted 4 lines

So let's address this issue, by removing the need for this stack to exist in the first place: v7_invalidate_l1 can be rewritten to use fewer registers, which means fewer registers need to be preserved, and we have enough spare registers available.

That is excellent.

I wonder why r1-r3 were preserved though.

Quoted 8 lines

r1 and r2 are documented in head.S as

* The processor init function will be called with:
* r1 - machine type
* r2 - boot data (atags/dt) pointer

but preserving the value of r3 does not seem necessary. Perhaps this is a leftover from old code?

Still, further down in the same comment it is said:

* On return, the CPU will be ready for the MMU to be turned on,
* r0 will hold the CPU control register value, r1, r2, r4, and
* r9 will be preserved. r5 will also be preserved if LPAE.

Quoted 4 lines

But you're now clobbering r1 and r2. And when this returns, code execution goes to __enable_mmu whose comment also mentions the expected r1 and r2 content. That would need adjusting too.

No, r1 and r2 are preserved - they are stashed in r6/r7 across the call to v7_invalidate_l1. r0, r3 and r12 are clobbered as well, but those do not contain anything that needs to be preserved.

Quoted 51 lines · Ard Biesheuvel

Quoted 49 lines · Nicolas Pitre

Quoted 27 lines · Ard Biesheuvel

Quoted 16 lines · Nicolas Pitre

Quoted 13 lines · Ard Biesheuvel

The v7 boot code uses a small chunk of BSS to preserve some register contents across a call to v7_invalidate_l1 that occurs with the MMU and caches disabled. Memory accesses in such cases are tricky on v7+, given that the architecture permits some unintuitive behaviors (it is implementation defined whether accesses done with the MMU and caches off may hit in the caches, and on SoCs that incorporate off-core system caches, this behavior appears to be different even between cache levels). Also, cache invalidation is not safe under virtualization if the intent is to retain stores issued directly to DRAM, given that the hypervisor may upgrade invalidate operations to clean+invalidate, resulting in DRAM contents to be overwritte by the dirty cachelines that we were trying to evict in the first place.

Quoted 9 lines

Quoted 4 lines

So let's address this issue, by removing the need for this stack to exist in the first place: v7_invalidate_l1 can be rewritten to use fewer registers, which means fewer registers need to be preserved, and we have enough spare registers available.

That is excellent.

I wonder why r1-r3 were preserved though.

Quoted 8 lines

r1 and r2 are documented in head.S as

* The processor init function will be called with:
* r1 - machine type
* r2 - boot data (atags/dt) pointer

but preserving the value of r3 does not seem necessary. Perhaps this is a leftover from old code?

Still, further down in the same comment it is said:

* On return, the CPU will be ready for the MMU to be turned on,
* r0 will hold the CPU control register value, r1, r2, r4, and
* r9 will be preserved. r5 will also be preserved if LPAE.

But you're now clobbering r1 and r2. And when this returns, code execution goes to __enable_mmu whose comment also mentions the expected r1 and r2 content. That would need adjusting too.

Quoted 3 lines

No, r1 and r2 are preserved - they are stashed in r6/r7 across the call to v7_invalidate_l1. r0, r3 and r12 are clobbered as well, but those do not contain anything that needs to be preserved.

OK, you're right. I didn't look at the whole code.

Acked-by Nicolas Pitre <[email protected]>