loader: add ISB around system register writes#410
loader: add ISB around system register writes#410midnightveil wants to merge 1 commit intoseL4:mainfrom
Conversation
|
Do we need ISBs before |
I think so, because we do an MSR to enable ERET. I think that needs to happen before executing ERET... even if ERET would be a synchronisation barrier itself. I'm not entirely sure on that point, though. |
loader/src/aarch64/util64.S
Outdated
|
|
||
| /* barrier for writes to special register */ | ||
| isb |
There was a problem hiding this comment.
A bit nitpicky, but it are called "system registers" on AArch64 and the comment is vague enough that you can as well not have it, because if you understand the comment and why the ISB is there, you don't need the comment anyway.
However, because of the ERET below, it's much more important to say why an ISB may be needed after all, because usually it can be omitted in such cases.
The best documentation about this so far I found in the bloody glossary of ARM DDI 0487, under "Context Synchronization event". It says, other than returning from an exception being one:
The effects of a Context synchronization event are:
- No instructions appearing in program order after an instruction that causes a Context
synchronization event will have performed any part of their functionality until the Context
synchronization event has occurred. - All direct and indirect writes to System registers that are made before the Context
synchronization event affect any instruction, including a direct read, that appears
in program order after the instruction causing the Context synchronization event.
To me it seems the instruction causing the context synchronisation event itself is excluded, and happens before the barrier so to speak. So changes to system registers spsr_el2 and elr_el2 which affect ERET behaviour do need an ISB.
There was a problem hiding this comment.
A bit nitpicky, but it are called "system registers" on AArch64 and the comment is vague enough that you can as well not have it, because if you understand the comment and why the ISB is there, you don't need the comment anyway.
The MRS instruction on Cortex-A does call it a 'system register', whereas the MRS instruction on Cortex-M does call it a 'special' / 'special-purpose' register, so I do agree with you.... however:
My copy of the manual (DDI0487L.b) under §C5.2 also calls them 'Special-purpose' registers. But these appear to be a subset of 'System registers'.
Which, now that I look at it, shows this:
The full list of Special-purpose registers is in Table C5-8. The characteristic of a Special-purpose register is that all direct and indirect reads and writes to the register appear to occur in program order relative to other instructions, without
the need for any explicit synchronization.
Notably, 'SP_EL1', 'SP_EL2', 'SPSR_EL1', 'SPSR_EL2', 'ELR_EL1' and 'ELR_EL2' are all special purpose registers, and do not need synchronisation. So this solves our question about 'ERET' and the 'ELR' values.
the full list
- ALLINT, that holds the PSTATE.ALLINT bit.
- CurrentEL, that holds PSTATE.EL, and that software can read to determine the current Exception level.
- DAIF, that holds the current PSTATE.{D, A, I, F} interrupt mask bits.
- DIT, that holds the PSTATE.DIT bit.
- ELR_EL1, that holds the address to return to for an exception return from EL1.
- ELR_EL2, that holds the address to return to for an exception return from EL2.
- ELR_EL3, that holds the address to return to for an exception return from EL3.
- FPCR, that provides control of floating-point operation.
- FPMR, that controls behaviors of the FP8 instructions.
- FPSR, that provides floating-point status information.
- NZCV, that holds the PSTATE.{N, Z, C, V} condition flags.
- PAN, that holds the PSTATE.PAN state bit.
- PM, that provides access to the Profiling exception mask bit.
- SP_EL0, that holds the stack pointer for EL0.
- SP_EL1, that holds the stack pointer for EL1.
- SP_EL2, that holds the stack pointer for EL2.
- SP_EL3, that holds the stack pointer for EL3.
- SPSel, that holds PSTATE.SP, that at EL1 or higher selects the current SP.
- SPSR_abt, that holds process state on taking an exception to AArch32 Abort mode.
- SPSR_EL1, that holds process state on taking an exception to AArch64 EL1.
- SPSR_EL2, that holds process state on taking an exception to AArch64 EL2.
- SPSR_EL3, that holds process state on taking an exception to AArch64 EL3.
- SPSR_fiq, that holds process state on taking an exception to AArch32 FIQ mode.
- SPSR_irq, that holds process state on taking an exception to AArch32 IRQ mode.
- SPSR_und, that holds process state on taking an exception to AArch32 Undefined mode.
- SSBS, that holds the PSTATE.SSBS bit.
- SVCR, controls Streaming SVE mode and SME behavior.
- TCO, that holds the PSTATE.TCO bit.
- UAO, that holds the PSTATE.UAO bit.
The following registers are also Special-purpose registers:
- DLR_EL0, that holds the address to return to for a return from Debug state.
- DSPSR_EL0, that holds process state on entry to Debug state
There was a problem hiding this comment.
Good find!
In "D24.1.2.2 Synchronization requirements for AArch64 System registers", they do say:
Unless otherwise stated in its System register definition, a direct write to a System register requires synchronization before software can rely on the effects of that write to affect instructions appearing in program order after the write. This does not apply to Special-purpose registers.
But that's very easy to miss and they don't elaborate about Special-purpose registers there. I guess they assume you read all 7k pages in order. They could at least have given a reference to C5.2 when giving the table of exceptions for normal system registers. :-(
System registers (but not special-purpose registers) require explicit synchronisation, as per D24.1.2.2 'Synchronization requirements for AArch64 System registers' (DDI0487 L.b) Signed-off-by: Julia Vassiliki <julia.vassiliki@unsw.edu.au>
These are required by the spec.