Hi Julien
On 2023/7/5 05:24, Julien Grall wrote:
Hi Penny,
On 26/06/2023 04:33, Penny Zheng wrote:
From: Wei Chen <wei.c...@arm.com>
At the moment, on MMU system, enable_mmu() will return to an
address in the 1:1 mapping, then each path is responsible to
switch to virtual runtime mapping. Then remove_identity_mapping()
is called to remove all 1:1 mapping.
The identity mapping is only removed for the boot CPU. You mention it
correctly below but here it lead to think it may be called on the
secondary CPU. So I would add 'on the boot CPU'.
Since remove_identity_mapping() is not necessary on Non-MMU system,
and we also avoid creating empty function for Non-MMU system, trying
to keep only one codeflow in arm64/head.S, we move path switch and
remove_identity_mapping() in enable_mmu() on MMU system.
As the remove_identity_mapping should only be called for the boot
CPU only, so we introduce enable_boot_mmu for boot CPU and
enable_runtime_mmu for secondary CPUs in this patch.
NIT: Add () after enable_runtime_mmu to be consistent with the rest of
commit message. Same for the title.
sure.
Also, I would prefer if we name the functions properly from the start.
So we don't have to rename them when they are moved in patch 7.
ok. change them to enable_runtime_mm() and enable_boot_mm() at the beginning
Signed-off-by: Wei Chen <wei.c...@arm.com>
Signed-off-by: Penny Zheng <penny.zh...@arm.com>
---
v3:
- new patch
---
xen/arch/arm/arm64/head.S | 87 +++++++++++++++++++++++++++++++--------
1 file changed, 70 insertions(+), 17 deletions(-)
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 10a07db428..4dfbe0bc6f 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -314,21 +314,12 @@ real_start_efi:
bl check_cpu_mode
bl cpu_init
- bl create_page_tables
- load_paddr x0, boot_pgtable
- bl enable_mmu
/* We are still in the 1:1 mapping. Jump to the runtime
Virtual Address. */
This comment is not accurate anymore given that the MMU is off.
I'll remove.
- ldr x0, =primary_switched
- br x0
+ ldr lr, =primary_switched
+ b enable_boot_mmu
+
primary_switched:
- /*
- * The 1:1 map may clash with other parts of the Xen virtual
memory
- * layout. As it is not used anymore, remove it completely to
- * avoid having to worry about replacing existing mapping
- * afterwards.
- */
- bl remove_identity_mapping
bl setup_fixmap
#ifdef CONFIG_EARLY_PRINTK
/* Use a virtual address to access the UART. */
@@ -373,13 +364,11 @@ GLOBAL(init_secondary)
#endif
bl check_cpu_mode
bl cpu_init
- load_paddr x0, init_ttbr
- ldr x0, [x0]
- bl enable_mmu
/* We are still in the 1:1 mapping. Jump to the runtime
Virtual Address. */
Note: Remove this too.
- ldr x0, =secondary_switched
- br x0
+ ldr lr, =secondary_switched
+ b enable_runtime_mmu
+
secondary_switched:
#ifdef CONFIG_EARLY_PRINTK
/* Use a virtual address to access the UART. */
@@ -694,6 +683,70 @@ enable_mmu:
ret
ENDPROC(enable_mmu)
+/*
+ * Turn on the Data Cache and the MMU. The function will return
+ * to the virtual address provided in LR (e.g. the runtime mapping).
The description here is exactly the same as below. However, there is a
different between the two functions. One is to deal with the secondary
CPUs whilst the second is for the boot CPUs.
I'll add-on: This function deals with the secondary CPUs.
+ *
+ * Inputs:
+ * lr : Virtual address to return to.
+ *
+ * Clobbers x0 - x5
+ */
+enable_runtime_mmu:
I find "runtime" confusing in this case. How about
"enable_secondary_cpu_mm"?
Sure, will do.
+ mov x5, lr
+
+ load_paddr x0, init_ttbr
+ ldr x0, [x0]
+
+ bl enable_mmu
+ mov lr, x5
+
+ /* return to secondary_switched */
+ ret
+ENDPROC(enable_runtime_mmu)
+
+/*
+ * Turn on the Data Cache and the MMU. The function will return
+ * to the virtual address provided in LR (e.g. the runtime mapping).
Similar remark as for the comment above.
I'll add-on: This function deals with the boot CPUs.
+ *
+ * Inputs:
+ * lr : Virtual address to return to.
+ *
+ * Clobbers x0 - x5
+ */
+enable_boot_mmu:
Based on my comment above, I would sugesst to call it "enable_boot_cpu_mm"
sure,
+ mov x5, lr
+
+ bl create_page_tables
+ load_paddr x0, boot_pgtable
+
+ bl enable_mmu
+ mov lr, x5
+
+ /*
+ * The MMU is turned on and we are in the 1:1 mapping. Switch
+ * to the runtime mapping.
+ */
+ ldr x0, =1f
+ br x0
+1:
+ /*
+ * The 1:1 map may clash with other parts of the Xen virtual
memory
+ * layout. As it is not used anymore, remove it completely to
+ * avoid having to worry about replacing existing mapping
+ * afterwards. Function will return to primary_switched.
+ */
+ b remove_identity_mapping
+
+ /*
+ * Here might not be reached, as "ret" in
remove_identity_mapping
+ * will use the return address in LR in advance. But keep ret
here
+ * might be more safe if "ret" in remove_identity_mapping is
removed
+ * in future.
+ */
+ ret
Given this path is meant to be unreachable, I would prefer if we call
"fail".
sure,
+ENDPROC(enable_boot_mmu)
+
/*
* Remove the 1:1 map from the page-tables. It is not easy to keep
track
* where the 1:1 map was mapped, so we will look for the top-level
entry
Cheers,
