ARM Cortex-R52 doesn't have SPSR_hyp

tobias-roehmel commented 2 years ago

Describe the bug

I think I have found an issue with the startup code for the Cortex-R52, specifically this line: https://github.com/zephyrproject-rtos/zephyr/blob/fd0767557439d04c4a220a9857e901d8d71498db/arch/arm/core/aarch32/cortex_a_r/reset.S#L68 I think the Cortex-R52 doesn't support spsr access through the spsr_hyp register. I did some research and found the following:

The Arm® Cortex®-R52 Processor Technical Reference Manual Revision r1p3 (https://developer.arm.com/documentation/100026/0103) doesn't mention spsr_hyp at all.
The ARM Architecture Reference Manual Supplement - ARMv8, for the ARMv8-R AArch32 architecture profile (https://developer.arm.com/documentation/ddi0568/latest) references it for example on page H1-274 while describing aarch32/functions/system/SPSRaccessValid, which explains how an SPSR access is determined valid. It indicates that the access to SPSR_hyp is invalid if the accessing mode is not monitor mode.
The Arm Architecture Reference Manual for A-profile architecture (https://developer.arm.com/documentation/ddi0487/latest) says on page G8-9401: "SPSR_hyp is accessible only in Monitor mode". I think this is also valid for the R52 because nothing specific is mentioned in the Reference Suplement which has an entire section on the differences between Armv8-A and Armv8-R.

In conclusion, I think this line should be "msr SPSR, r0" since the Cortex-R52 doesn't support monitor mode.

Additional context

I came across this because I'm trying to implement the Cortex-R52 in Qemu and a bunch of stuff breaks if you try to execute this instruction.
I know that the Zephyr executes on the ARM fast models, but is there a chance it shouldn't?
I don't have any physical hardware to test this so I was wondering if anyone could confirm or deny this.

julien-massot commented 2 years ago

Hi @tobias-roehmel , Thanks for this bug report and your work on enabling Cortex-r52 on Qemu !

This is surprising, this code has been tested on real hardware as well as with FVP.

This spsr_hyp instruction is inspired from the startup example file which comes with arm developer studio. (Bare-metal_examples_Armv8.zip: startup_Cortex-R52/startup.s ) and the AArch64 code is also initializing the same register spsr_el2

@povergoing Any idea on this issue ?

tobias-roehmel commented 2 years ago

Hi @julien-massot ,

Thanks for the quick reply!

I checked the Bare-metal_examples that you mentioned and can confirm that they use the same instruction. Do you know if Zephyr was started in EL2 when it ran on physical hardware? Otherwise, it wouldn't get to that instruction, right?

I guess the case is pretty clear if this runs on physical hardware, I'll have to change Qemu to handle this accordingly.

julien-massot commented 2 years ago

Hi @julien-massot ,

Thanks for the quick reply!

I checked the Bare-metal_examples that you mentioned and can confirm that they use the same instruction. Do you know if Zephyr was started in EL2 when it ran on physical hardware? Otherwise, it wouldn't get to that instruction, right?

The processor always starts in Hypervisor mode. If Zephyr is the first system then it will be started in Hyp mode/EL2. In case we are not started in Hyp mode we jump to the EL1 reset handler: https://github.com/zephyrproject-rtos/zephyr/blob/fd0767557439d04c4a220a9857e901d8d71498db/arch/arm/core/aarch32/cortex_a_r/reset.S#L54

tobias-roehmel commented 2 years ago

Okay, that was also my understanding. Which means that "msr spsr_hyp, r0" will only be executed if Zephyr is run in EL2.

Do you know if this was tested on physical hardware?

julien-massot commented 2 years ago

yes it was, and pretty convenient when restarting zephyr with gdb without doing a cold reset:)

tobias-roehmel commented 2 years ago

Okay, I guess that resolves this. Weird that it isn't described in more detail in the docs.

Thanks for your help!

povergoing commented 2 years ago

In conclusion, I think this line should be "msr SPSR, r0" since the Cortex-R52 doesn't support monitor mode.

Since hyp mode is the highest exception level and no monitor mode for v8r aarch32, EL2(hyp) can assess spsr_hyp seems reasonable.

povergoing commented 2 years ago

Okay, I guess that resolves this. Weird that it isn't described in more detail in the docs.

So the Qemu supports the spsr_hyp? I am sure FVP supports it. But always, some implementations are different between Qemu and FVP and even real hardware, especially for this kind of "new core"

tobias-roehmel commented 2 years ago

Qemu doesn't support the R52 (yet). I'm working on it and basically used A55 as baseline and it dies horribly if you try to access spsr_hyp from EL2. But you are right, this might be a reasonable change between them. It's just weird that it's not documented. (Or I haven't found it)

povergoing commented 2 years ago

Qemu doesn't support the R52 (yet). I'm working on it and basically used A55 as baseline and it dies horribly if you try to access spsr_hyp from EL2. But you are right, this might be a reasonable change between them.

If you want to use A55 to simulate the R52, better to start with EL1, since the most significant change of R52 is about removing EL3 and making EL2 as the highest el. It can be expected that there are many issues when you do something in EL2. Also A55 simulating R52 is not that precise because A55 doesn't have MPU.

It's just weird that it's not documented. (Or I haven't found it)

I try to raise a ticket and let's see if the document guys want to improve the description.

tobias-roehmel commented 2 years ago

Thanks for the suggestions! You are right, EL2 still makes the biggest problems, but I can run some Zephyr examples if I change the line mentioned in this Issue. I already implemented the MPU in Qemu, but there isn't as much code that tests it. I think Zephyr doesn't support it either

povergoing commented 2 years ago

I already implemented the MPU in Qemu, but there isn't as much code that tests it. I think Zephyr doesn't support it either

Zephyr does support MPU both in AArch32 and AArch64, as long as you would like to give up using Qemu, and instead, use FVP. Basically, you could test whatever you want. :)

pm215 commented 2 years ago

Hi; I was pointed to this issue since Tobias has posted his patches to add R52 support to the QEMU mailing list.

The behaviour of "msr spsr_hyp, r0" when executed from anything except Monitor mode is architecturally UNPREDICTABLE, and this doesn't change for v8R. You can see this in the v8R Supplement pseudocode function aarch32/functions/system/SPSRaccessValid:

when '11110' // SPSR_hyp
    if !HaveEL(EL2) || mode != M32_Monitor then UNPREDICTABLE;

The use of this instruction in the Developer Studio sample code is a bug in that sample code, that went unnoticed because the FVP and the Cortex-R52 hardware happen to choose to handle this UNPREDICTABLE case as "allow the access". I've reported this bug and it should be fixed in some future DS version.

The fix is simple: since you know the code is executing in Hyp mode, just use "msr spsr, r0".

povergoing commented 2 years ago

Hi; I was pointed to this issue since Tobias has posted his patches to add R52 support to the QEMU mailing list.

The behaviour of "msr spsr_hyp, r0" when executed from anything except Monitor mode is architecturally UNPREDICTABLE, and this doesn't change for v8R. You can see this in the v8R Supplement pseudocode function aarch32/functions/system/SPSRaccessValid:
when '11110' // SPSR_hyp
    if !HaveEL(EL2) || mode != M32_Monitor then UNPREDICTABLE;
The use of this instruction in the Developer Studio sample code is a bug in that sample code, that went unnoticed because the FVP and the Cortex-R52 hardware happen to choose to handle this UNPREDICTABLE case as "allow the access". I've reported this bug and it should be fixed in some future DS version.

The fix is simple: since you know the code is executing in Hyp mode, just use "msr spsr, r0".

Thanks for your clarification, would you like to mention in your reported ticket that FVP doesn't handle the msr spsr, r0? So far we cannot change it to "msr spsr, r0".

Specifically, I have tried "msr spsr, r0" in FVP_BaseR_Cortex-R52x1 FVP (version 11.17.3 (Jan 23 2022)), after "msr spsr, r0", the SPSR.E bit won't be set to 0, so that the core work as big-endian instead of little-endian and will crash.

pm215 commented 2 years ago

That's weird. If you can provide an FVP command line and all the necessary image files for (a) the working-with-spsr_hyp and (b) the not-working-with-spsr cases, I can have a look at it...

povergoing commented 2 years ago

@pm215 Really appreciate it if you would like to have a look

FVP: FVP_BaseR_Cortex-R52x1 with version 11.17.3 (Jan 23 2022)
command: FVP_BaseR_Cortex-R52x1 -C bp.sram.enable_atomic_ops=1 -C bp.refcounter.non_arch_start_at_default=1 -C bp.pl011_uart0.out_file=- -C bp.pl011_uart0.unbuffered_output=1 -C bp.terminal_0.start_telnet=0 -C bp.pl011_uart1.out_file=- -C bp.pl011_uart1.unbuffered_output=1 -C bp.terminal_1.start_telnet=0 -C bp.pl011_uart2.out_file=- -C bp.pl011_uart2.unbuffered_output=1 -C bp.terminal_2.start_telnet=0 -C bp.pl011_uart3.out_file=- -C bp.pl011_uart3.unbuffered_output=1 -C bp.terminal_3.start_telnet=0 -C bp.vis.disable_visualisation=1 -C bp.vis.rate_limit-enable=0 -a /path/to/zephyr.elf /path/to/zephyr.elf should be the right path of the zephyr image

diff:


diff --git a/arch/arm/core/aarch32/cortex_a_r/reset.S b/arch/arm/core/aarch32/cortex_a_r/reset.S
index d6232cb949..bfa76e4f09 100644
--- a/arch/arm/core/aarch32/cortex_a_r/reset.S
+++ b/arch/arm/core/aarch32/cortex_a_r/reset.S
@@ -65,7 +65,7 @@ SECTION_SUBSEC_FUNC(TEXT, _reset_section, __start)
 mrs r0, cpsr
 bic r0, #MODE_MASK
 orr r0, #MODE_SVC

msr spsr_hyp, r0
msr spsr, r0

ldr r0, =EL1_Reset_Handler msr elr_hyp, r0
This point (line 68) is at the very beginning of the entry, it can be easily reached with DS. And can be easily checked that when we use msr spsr, r0, after this inst executed, the spsr does not equal to r0 (at least the SPSR.E bit is not right) https://github.com/zephyrproject-rtos/zephyr/blob/fd0767557439d04c4a220a9857e901d8d71498db/arch/arm/core/aarch32/cortex_a_r/reset.S#L48-L74
zephyr-spsr-issue.zip includes the two images a)working-with-spsr_hyp and b)not-working-with-spsr

pm215 commented 2 years ago

Ah, I see what's happened. This is my mistake with the instruction I suggested. The right/full form of the instruction is msr spsr_<fields>, r0, where <fields> is a set of letters saying which fields in the SPSR should be written. If you look at an objdump of the non-working binary you can see:

    13d0:       e169f000        msr     SPSR_fc, r0

that since the source file didn't specify fields the assembler has defaulted to fc, which means "update bits 7:0 and 31:24 only". To update the whole 32 bits of SPSR we want:

  msr spsr_cxsf, r0

Apologies for misleading you there.

povergoing commented 2 years ago

Ah, I see what's happened. This is my mistake with the instruction I suggested. The right/full form of the instruction is msr spsr_<fields>, r0, where <fields> is a set of letters saying which fields in the SPSR should be written.

That's it! Thanks for your clear explanation. I also confirmed it works when we use spsr_cxsf :)

zephyrproject-rtos / zephyr

ARM Cortex-R52 doesn't have SPSR_hyp #47330