Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

armstub7: Various fixes #85

Closed
wants to merge 4 commits into from
Closed

armstub7: Various fixes #85

wants to merge 4 commits into from

Conversation

@AntonioND
Copy link

@AntonioND AntonioND commented Nov 7, 2017

Various fixes to armstub7.

Original message:

The previous code was enabling both I and D-caches, but keeping the
bit M as the reset value, which is architecturally undetermined. In
practice, in both Cortex-A7 and Cortex-A53, the reset value is 0, so
the D-cache was disabled (but the I-cache was enabled).

For the 32-bit Arm architecture boot, the Linux kernel expects the data
cache and MMU to be disabled when the CPU jumps into it, the I-cache
doesn't matter, as specified in the file Documentation/arm/Booting of
the Linux kernel tree under Calling the kernel image.

This patch disables the D-cache and MMU for correctness while keeping
the I-cache enabled to save a few cycles during the execution of the
stub.

Fixes #84

The previous code was enabling both I and D-caches, but keeping the
bit M as the reset value, which is architecturally undetermined. In
practice, in both Cortex-A7 and Cortex-A53, the reset value is 0, so
the D-cache was disabled (but the I-cache was enabled).

For the 32-bit Arm architecture boot, the Linux kernel expects the data
cache and MMU to be disabled when the CPU jumps into it, the I-cache
doesn't matter, as specified in the file `Documentation/arm/Booting` of
the Linux kernel tree under `Calling the kernel image`.

This patch disables the D-cache and MMU for correctness while keeping
the I-cache enabled to save a few cycles during the execution of the
stub.

Signed-off-by: Antonio Niño Díaz <antonio_nd@outlook.com>
@pelwell
Copy link
Contributor

@pelwell pelwell commented Nov 7, 2017

For other reviewers, see http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0438i/BABJAHDA.html for details of the System Control Register.
That document says that both the D cache and MMU are disabled by default, which would suggest that the correct fix is to remove that line altogether, but since force_core mode relies on an instruction offset I'm happy for this to stay as it is.

@popcornmix?

@AntonioND
Copy link
Author

@AntonioND AntonioND commented Nov 7, 2017

While it is true that for A7 and A53 both C and M bits are set to 0 on reset (according to their technical reference manuals), the ARM ARM says (at least, the ARMv8 ARM) When this register has an architecturally-defined reset value, this field resets to 0. so I don't think that explicitly disabling it is a bad idea in case it is needed by future models.

AntonioND added 3 commits Nov 22, 2017
According to the Technical Reference Manuals of both Cortex-A7 and
Cortex-A53, SMP must be enabled before enabling any caches.

Signed-off-by: Antonio Niño Díaz <antonio_nd@outlook.com>
In Cortex-A7, some reserved fields of NSACR were set to 1. According to
the Technical Reference Manual, the reserved fields of this register are
RAZ/WI (Read-As-Zero, Writes Ignored).

In Cortex-A53, all reserved fields are RES0. Software should respect
this for future compatibility.

The bits NS_SMP and NS_L2ERR of Cortex-A7 need to be set to 1 in order
for the Non-secure world to be able to modify the value of the SMP and
L2 AXI asynchronous error bits. However, in Cortex-A53 they are RES0.
This means that both CPUs can't share the same reset value.

Signed-off-by: Antonio Niño Díaz <antonio_nd@outlook.com>
This register can be accessed from both user and privileged modes and it
is not used in the bootstrap, there is no reason to initialize it here.

Signed-off-by: Antonio Niño Díaz <antonio_nd@outlook.com>
@AntonioND AntonioND changed the title armstub7: Disable data cache and MMU armstub7: Various fixes Nov 22, 2017
@AntonioND
Copy link
Author

@AntonioND AntonioND commented Nov 22, 2017

I've added a couple of commits to fix other issues with this bootstrap (armstub7: Set SMP before enabling instruction cache and armstub7: Fix reset NSACR values).

I'm not sure about armstub7: Remove useless initialization of CNTV_CTL.

Note that I don't have a Raspberry Pi 2 to test the Cortex-A7 code.

@popcornmix
Copy link
Contributor

@popcornmix popcornmix commented Nov 22, 2017

The changes look quite reasonable. Will need double checking on Pi2/Pi3 and force_core firmware code may need patching, but otherwise seems fine.

@petemoore
Copy link

@petemoore petemoore commented Mar 26, 2019

@popcornmix Is there someone at the Raspberry Pi Foundation that can double check the pi2/pi3? It would be nice to get the PR landed. Many thanks.

pelwell added a commit that referenced this pull request Oct 8, 2020
- The bootstub has been completely rewritten taking advantage of the
  Thumb-2 instruction set, which results in major space gains
- Res0 bits of NSACR are no longer set (supersedes #85)
- CNTVOFF is set to zero, now consistent with armstub8 (supersedes #113)
- SMC instructions are now disabled, now consistent with armstub8
- ACTLR is now configured to allow Non-secure access to several CPU
  configuration registers (CPUACTLR/CPUECTLR/L2CTLR/L2ECTLR/L2ACTLR),
  which makes it possible to e.g. enable Spectre v4 mitigations directly
  in the kernel without needing a separate bootstub variant
  (potentially supersedes #115)

Free space in each affected bootstub after this commit:

  armstub7.bin:        108 bytes
  armstub8-32.bin:     104 bytes
  armstub8-32-gic.bin:  44 bytes (!)
@AntonioND AntonioND closed this Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants