Hi Stefano,
On 13/12/2022 22:56, Stefano Stabellini wrote:
On Tue, 13 Dec 2022, Julien Grall wrote:
Hi Stefano,
On 13/12/2022 02:00, Stefano Stabellini wrote:
On Mon, 12 Dec 2022, Julien Grall wrote:
From: Julien Grall <jgr...@amazon.com>
At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
still on.
Switching TTBR is like replacing existing mappings with new ones. So
we need to follow the break-before-make sequence.
In this case, it means the MMU needs to be switched off while the
TTBR is updated. In order to disable the MMU, we need to first
jump to an identity mapping.
Rename switch_ttbr() to switch_ttbr_id() and create an helper on
top to temporary map the identity mapping and call switch_ttbr()
via the identity address.
switch_ttbr_id() is now reworked to temporarily turn off the MMU
before updating the TTBR.
We also need to make sure the helper switch_ttbr() is part of the
identity mapping. So move _end_boot past it.
The arm32 code will use a different approach. So this issue is for now
only resolved on arm64.
Signed-off-by: Julien Grall <jgr...@amazon.com>
This patch looks overall good to me, aside from the few minor comments
below. I would love for someone else, maybe from ARM, reviewing steps
1-6 making sure they are the right sequence.
---
Changes in v2:
- Remove the arm32 changes. This will be addressed differently
- Re-instate the instruct cache flush. This is not strictly
necessary but kept it for safety.
- Use "dsb ish" rather than "dsb sy".
TODO:
* Handle the case where the runtime Xen is loaded at a different
position for cache coloring. This will be dealt separately.
---
xen/arch/arm/arm64/head.S | 50 +++++++++++++++++++++++------------
xen/arch/arm/arm64/mm.c | 39 +++++++++++++++++++++++++++
xen/arch/arm/include/asm/mm.h | 2 ++
xen/arch/arm/mm.c | 14 +++++-----
4 files changed, 82 insertions(+), 23 deletions(-)
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 663f5813b12e..1f69864492b6 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -816,30 +816,46 @@ ENDPROC(fail)
* Switch TTBR
*
* x0 ttbr
- *
- * TODO: This code does not comply with break-before-make.
*/
-ENTRY(switch_ttbr)
- dsb sy /* Ensure the flushes happen before
- * continuing */
- isb /* Ensure synchronization with
previous
- * changes to text */
- tlbi alle2 /* Flush hypervisor TLB */
- ic iallu /* Flush I-cache */
- dsb sy /* Ensure completion of TLB flush */
+ENTRY(switch_ttbr_id)
+ /* 1) Ensure any previous read/write have completed */
+ dsb ish
+ isb
+
+ /* 2) Turn off MMU */
+ mrs x1, SCTLR_EL2
+ bic x1, x1, #SCTLR_Axx_ELx_M
do we need a "dsb sy" here? we have in enable_mmu
Hmmm... The explanation of the dsb + isb in enable_mmu makes no sense. The isb
doesn't flush the I-cache, it just flushes the pipeline.
For the dsb, I am not convinced it is necessary because we already have the
'dsb nsh' above and in any case the barrier seems to be too strong.
I guess that will be another patch... (probably at a lower priority).
Now back to your question of the 'dsb' here. There is already a 'dsb ish'
above. So memory access before turning off the MMU should be completed.
Also...
+ msr SCTLR_EL2, x1
+ isb
... this isb will ensure the completion of SCTLR before the TLBs are flushed
before. And there should be no memory access (or than instructions here). So I
don't think the a dsb is needed.
Would you mind to explain why you think there is one needed?
I am not at all sure whether it is needed or not, I was just noticing
that we have the "dsb sy" in enable_mmu and here we don't.
Thinking about it, the only reason for the additional dsb would be to
make sure that the two operations:
mrs x1, SCTLR_EL2
bic x1, x1, #SCTLR_Axx_ELx_M
are completed before disabling the MMU:
That's not what a 'dsb' is for. It is used for memory ordering there are
are no memory access involved here.
If you want the operations to be completed, then this would be an 'isb'.
Yet, this would not be necessary here as the next instruction cannot be
re-ordered because of the register dependency.
Cheers,
--
Julien Grall
