[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 18/18] xen/arm64: mm: Rework switch_ttbr()
Hi Stefano,
On 13/12/2022 02:00, Stefano Stabellini wrote:
On Mon, 12 Dec 2022, Julien Grall wrote:
From: Julien Grall <jgrall@xxxxxxxxxx>
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 <jgrall@xxxxxxxxxx>
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?
+
+ /*
+ * 3) Flush the TLBs.
+ * See asm/arm64/flushtlb.h for the explanation of the sequence.
+ */
+ dsb nshst
+ tlbi alle2
+ dsb nsh
+ isb
+
+ /* 4) Update the TTBR */
+ msr TTBR0_EL2, x0
isb
- msr TTBR0_EL2, x0
+ /*
+ * 5) Flush I-cache
+ * This should not be necessary but it is kept for safety.
+ */
+ ic iallu
+ isb
- 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 */
+ /* 5) Turn on the MMU */
This should be 6)
I will update it.
+ mrs x1, SCTLR_EL2
+ orr x1, x1, #SCTLR_Axx_ELx_M /* Enable MMU */
+ msr SCTLR_EL2, x1
isb
ret
-ENDPROC(switch_ttbr)
+ENDPROC(switch_ttbr_id)
#ifdef CONFIG_EARLY_PRINTK
/*
diff --git a/xen/arch/arm/arm64/mm.c b/xen/arch/arm/arm64/mm.c
index 9eaf545ea9dd..2ede4e75ae33 100644
--- a/xen/arch/arm/arm64/mm.c
+++ b/xen/arch/arm/arm64/mm.c
@@ -31,6 +31,15 @@ static void __init prepare_boot_identity_mapping(void)
lpae_t pte;
DECLARE_OFFSETS(id_offsets, id_addr);
+ /*
+ * We will be re-using the boot ID tables. They may not have been
+ * zeroed but they should be unlinked. So it is fine to use
+ * clear_page().
+ */
+ clear_page(boot_first_id);
+ clear_page(boot_second_id);
+ clear_page(boot_third_id);
Could this code be in patch #15?
Yes, I can't remember why I decided to clear them in patch #18.
if ( id_offsets[0] != 0 )
panic("Cannot handled ID mapping above 512GB\n");
@@ -111,6 +120,36 @@ void update_identity_mapping(bool enable)
BUG_ON(rc);
}
+extern void switch_ttbr_id(uint64_t ttbr);
+
+typedef void (switch_ttbr_fn)(uint64_t ttbr);
+
+void __init switch_ttbr(uint64_t ttbr)
+{
+ vaddr_t id_addr = virt_to_maddr(switch_ttbr_id);
+ switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr;
+ lpae_t pte;
+
+ /* Enable the identity mapping in the boot page tables */
See below...
+ update_identity_mapping(true);
+ /* Enable the identity mapping in the runtime page tables */
+ pte = pte_of_xenaddr((vaddr_t)switch_ttbr_id);
+ pte.pt.table = 1;
+ pte.pt.xn = 0;
+ pte.pt.ro = 1;
+ write_pte(&xen_third_id[third_table_offset(id_addr)], pte);
+
+ /* Switch TTBR */
+ fn(ttbr);
+
+ /*
+ * Disable the identity mapping in the runtime page tables.
+ * Note it is not necessary to disable it in the boot page tables
+ * because they are not going to be used by this CPU anymore.
+ */
...is update_identity_mapping acting on the boot pagetables or the
runtime pagetables? The two comments make me think that
update_identity_mapping is enabling mapping in the boot pagetables and
removing them from the runtime pagetable, which would be strangely
inconsistent.
update_identity_mapping() is acting on the live page-tables (i.e. the
one pointed by TTBR_EL2).
Before switch_ttbr(), this will be the boot page-tables. But after, this
will be the runtime page-tables.
Would the following comment on top of the declaration of
update_identity_mapping() clarifies it:
"Enable/disable the identity mapping in the live page-tables (i.e. the
one pointed by TTBR_EL2)."
Cheers,
--
Julien Grall
|
