* [PATCH v2 0/2] xen/arm32: Improve logging during early boot
@ 2024-01-11 18:34 Julien Grall
2024-01-11 18:34 ` [PATCH v2 1/2] xen/arm32: head: Rework how the fixmap and early UART mapping are prepared Julien Grall
2024-01-11 18:34 ` [PATCH v2 2/2] xen/arm32: head: Improve logging in head.S Julien Grall
0 siblings, 2 replies; 9+ messages in thread
From: Julien Grall @ 2024-01-11 18:34 UTC (permalink / raw)
To: xen-devel
Cc: julien, Julien Grall, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk
From: Julien Grall <jgrall@amazon.com>
Hi all,
This small series is based on some debugging I added while investigating
f5a49eb7f8b3 ("xen/arm32: head: Add mising isb in
switch_to_runtime_mapping()").
This will make easier to narrow down where the CPU is stuck while
enabling the MMU.
Cheers,
Julien Grall (2):
xen/arm32: head: Rework how the fixmap and early UART mapping are
prepared
xen/arm32: head: Improve logging in head.S
xen/arch/arm/arm32/head.S | 9 ---
xen/arch/arm/arm32/mmu/head.S | 100 +++++++++++++-----------
xen/arch/arm/include/asm/arm32/macros.h | 33 +++++---
xen/arch/arm/include/asm/asm_defns.h | 6 +-
xen/arch/arm/include/asm/early_printk.h | 3 +
xen/arch/arm/include/asm/mmu/layout.h | 4 +
xen/arch/arm/mmu/setup.c | 3 +
xen/arch/arm/xen.lds.S | 1 +
8 files changed, 92 insertions(+), 67 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] xen/arm32: head: Rework how the fixmap and early UART mapping are prepared
2024-01-11 18:34 [PATCH v2 0/2] xen/arm32: Improve logging during early boot Julien Grall
@ 2024-01-11 18:34 ` Julien Grall
2024-01-12 7:46 ` Michal Orzel
2024-01-11 18:34 ` [PATCH v2 2/2] xen/arm32: head: Improve logging in head.S Julien Grall
1 sibling, 1 reply; 9+ messages in thread
From: Julien Grall @ 2024-01-11 18:34 UTC (permalink / raw)
To: xen-devel
Cc: julien, Julien Grall, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk
From: Julien Grall <jgrall@amazon.com>
Since commit 5e213f0f4d2c ("xen/arm32: head: Widen the use of the
temporary mapping"), boot_second (used to cover regions like Xen and
the fixmap) will not be mapped if the identity mapping overlap.
So it is ok to prepare the fixmap table and link it in boot_second
earlier. With that, the fixmap can also be used earlier via the
temporary mapping.
Therefore split setup_fixmap() in two:
* The table is now linked in create_page_tables() because
the boot page tables needs to be recreated for every CPU.
* The early UART mapping is only added for the boot CPU0 as the
fixmap table is not cleared when secondary CPUs boot.
Signed-off-by: Julien Grall <jgrall@amazon.com>
----
Changelog since v1:
* Rebase
* Move the UART mapping enable_mm_boot_cpu()
---
xen/arch/arm/arm32/mmu/head.S | 61 ++++++++---------------------------
1 file changed, 14 insertions(+), 47 deletions(-)
diff --git a/xen/arch/arm/arm32/mmu/head.S b/xen/arch/arm/arm32/mmu/head.S
index 4e6395e7876d..a90799ad5451 100644
--- a/xen/arch/arm/arm32/mmu/head.S
+++ b/xen/arch/arm/arm32/mmu/head.S
@@ -165,11 +165,6 @@
* Rebuild the boot pagetable's first-level entries. The structure
* is described in mm.c.
*
- * After the CPU enables paging it will add the fixmap mapping
- * to these page tables, however this may clash with the 1:1
- * mapping. So each CPU must rebuild the page tables here with
- * the 1:1 in place.
- *
* Inputs:
* r9 : paddr(start)
* r10: phys offset
@@ -197,6 +192,10 @@ create_page_tables:
add r5, r5, #PAGE_SIZE /* r5 := Next table */
.endr
+ /* Map the fixmap into boot_second */
+ mov_w r0, FIXMAP_ADDR(0)
+ create_table_entry boot_second, xen_fixmap, r0, 2
+
/*
* Find the size of Xen in pages and multiply by the size of a
* PTE. This will then be compared in the mapping loop below.
@@ -442,19 +441,20 @@ ENDPROC(enable_secondary_cpu_mm)
ENTRY(enable_boot_cpu_mm)
mov r6, lr
+#ifdef CONFIG_EARLY_PRINTK
+ /*
+ * Add the UART mapping to the fixmap so the UART can be used
+ * as the MMU is on. This only need to do done on the boot CPU.
+ */
+ mov_w r0, EARLY_UART_VIRTUAL_ADDRESS
+ create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3
+#endif
+
bl create_page_tables
/* Address in the runtime mapping to jump to after the MMU is enabled */
- mov_w lr, 1f
- b enable_mmu
-1:
mov lr, r6
-
- /*
- * Prepare the fixmap. The function will return to the virtual address
- * requested by the caller.
- */
- b setup_fixmap
+ b enable_mmu
ENDPROC(enable_boot_cpu_mm)
/*
@@ -503,39 +503,6 @@ remove_temporary_mapping:
mov pc, lr
ENDPROC(remove_temporary_mapping)
-/*
- * Map the UART in the fixmap (when earlyprintk is used) and hook the
- * fixmap table in the page tables.
- *
- * The fixmap cannot be mapped in create_page_tables because it may
- * clash with the 1:1 mapping.
- *
- * Inputs:
- * r10: Physical offset
- * r11: Early UART base physical address
- *
- * Clobbers r0 - r4
- */
-setup_fixmap:
-#if defined(CONFIG_EARLY_PRINTK)
- /* Add UART to the fixmap table */
- mov_w r0, EARLY_UART_VIRTUAL_ADDRESS
- create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3
-#endif
- /* Map fixmap into boot_second */
- mov_w r0, FIXMAP_ADDR(0)
- create_table_entry boot_second, xen_fixmap, r0, 2
- /* Ensure any page table updates made above have occurred. */
- dsb nshst
- /*
- * The fixmap area will be used soon after. So ensure no hardware
- * translation happens before the dsb completes.
- */
- isb
-
- mov pc, lr
-ENDPROC(setup_fixmap)
-
/* Fail-stop */
fail: PRINT("- Boot failed -\r\n")
1: wfe
--
2.40.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] xen/arm32: head: Improve logging in head.S
2024-01-11 18:34 [PATCH v2 0/2] xen/arm32: Improve logging during early boot Julien Grall
2024-01-11 18:34 ` [PATCH v2 1/2] xen/arm32: head: Rework how the fixmap and early UART mapping are prepared Julien Grall
@ 2024-01-11 18:34 ` Julien Grall
2024-01-12 8:49 ` Michal Orzel
1 sibling, 1 reply; 9+ messages in thread
From: Julien Grall @ 2024-01-11 18:34 UTC (permalink / raw)
To: xen-devel
Cc: julien, Julien Grall, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk
From: Julien Grall <jgrall@amazon.com>
The sequence to enable the MMU on arm32 is quite complex as we may need
to jump to a temporary mapping to map Xen.
Recently, we had one bug in the logic (see f5a49eb7f8b3 ("xen/arm32:
head: Add mising isb in switch_to_runtime_mapping()") and it was
a pain to debug because there are no logging.
In order to improve the logging in the MMU switch we need to add
support for early printk while running on the identity mapping
and also on the temporary mapping.
For the identity mapping, we have only the first page of Xen mapped.
So all the strings should reside in the first page. For that purpose
a new macro PRINT_ID is introduced.
For the temporary mapping, the fixmap is already linked in the temporary
area (and so does the UART). So we just need to update the register
storing the UART address (i.e. r11) to point to the UART temporary
mapping.
Take the opportunity to introduce mov_w_on_cond in order to
conditionally execute mov_w and avoid branches.
Signed-off-by: Julien Grall <jgrall@amazon.com>
----
Changelog since v1:
- Rebase
- Move one hunk to the first patch to unbreak compilation
- Add more logging
- Remove duplicated entry
---
xen/arch/arm/arm32/head.S | 9 ------
xen/arch/arm/arm32/mmu/head.S | 39 +++++++++++++++++++++++++
xen/arch/arm/include/asm/arm32/macros.h | 33 +++++++++++++++------
xen/arch/arm/include/asm/asm_defns.h | 6 ++--
xen/arch/arm/include/asm/early_printk.h | 3 ++
xen/arch/arm/include/asm/mmu/layout.h | 4 +++
xen/arch/arm/mmu/setup.c | 3 ++
xen/arch/arm/xen.lds.S | 1 +
8 files changed, 78 insertions(+), 20 deletions(-)
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 34ab14a9e228..99d7d4aa63d1 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -98,10 +98,6 @@ past_zImage:
b enable_boot_cpu_mm
primary_switched:
-#ifdef CONFIG_EARLY_PRINTK
- /* Use a virtual address to access the UART. */
- mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
-#endif
bl zero_bss
PRINT("- Ready -\r\n")
/* Setup the arguments for start_xen and jump to C world */
@@ -142,12 +138,7 @@ GLOBAL(init_secondary)
mov_w lr, secondary_switched
b enable_secondary_cpu_mm
-
secondary_switched:
-#ifdef CONFIG_EARLY_PRINTK
- /* Use a virtual address to access the UART. */
- mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
-#endif
PRINT("- Ready -\r\n")
/* Jump to C world */
mov_w r2, start_secondary
diff --git a/xen/arch/arm/arm32/mmu/head.S b/xen/arch/arm/arm32/mmu/head.S
index a90799ad5451..f4abd690b612 100644
--- a/xen/arch/arm/arm32/mmu/head.S
+++ b/xen/arch/arm/arm32/mmu/head.S
@@ -298,6 +298,21 @@ enable_mmu:
mcr CP32(r0, HSCTLR) /* now paging is enabled */
isb /* Now, flush the icache */
+ /*
+ * At this stage, the UART address will depend on whether the
+ * temporary mapping was created or not.
+ *
+ * If it was, then the UART will be mapped in the temporary
+ * area. Otherwise, it will be mapped at runtime virtual
+ * mapping.
+ */
+#ifdef CONFIG_EARLY_PRINTK
+ teq r12, #1 /* Was the temporary mapping created? */
+ mov_w_on_cond eq, r11, TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS
+ mov_w_on_cond ne, r11, EARLY_UART_VIRTUAL_ADDRESS
+#endif
+ PRINT_ID("- Paging turned on -\r\n")
+
/*
* The MMU is turned on and we are in the 1:1 mapping. Switch
* to the runtime mapping.
@@ -307,6 +322,17 @@ enable_mmu:
b switch_to_runtime_mapping
1:
mov lr, r5 /* Restore LR */
+
+ /*
+ * Now we are running at the runtime address. The UART can
+ * be accessed using its runtime virtual address.
+ */
+#ifdef CONFIG_EARLY_PRINTK
+ mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
+#endif
+
+ PRINT("- Switched to the runtime mapping -\r\n")
+
/*
* At this point, either the 1:1 map or the temporary mapping
* will be present. The former may clash with other parts of the
@@ -348,12 +374,14 @@ switch_to_runtime_mapping:
teq r12, #0
beq ready_to_switch
+ PRINT_ID("- Switching to the temporary mapping -\r\n")
/* We are still in the 1:1 mapping. Jump to the temporary Virtual address. */
mov_w r0, 1f
add r0, r0, #XEN_TEMPORARY_OFFSET /* r0 := address in temporary mapping */
mov pc, r0
1:
+ PRINT("- Running on the temporary mapping -\r\n")
/* Remove boot_second_id */
mov r2, #0
mov r3, #0
@@ -364,6 +392,8 @@ switch_to_runtime_mapping:
flush_xen_tlb_local r0
+ PRINT("- 1:1 mapping removed -\r\n")
+
/* Map boot_second into boot_pgtable */
mov_w r0, XEN_VIRT_START
create_table_entry boot_pgtable, boot_second, r0, 1
@@ -376,7 +406,10 @@ switch_to_runtime_mapping:
*/
isb
+ PRINT("- Runtime mapping mapped -\r\n")
ready_to_switch:
+ PRINT_ID("- Jumping to runtime address -\r\n")
+
mov pc, lr
ENDPROC(switch_to_runtime_mapping)
@@ -404,6 +437,8 @@ ENTRY(enable_secondary_cpu_mm)
mov_w lr, 1f
b enable_mmu
1:
+ PRINT("- Switching to the runtime page-tables -\r\n")
+
/*
* Non-boot CPUs need to move on to the proper pagetables, which were
* setup in prepare_secondary_mm.
@@ -468,6 +503,8 @@ ENDPROC(enable_boot_cpu_mm)
* Clobbers r0 - r3
*/
remove_identity_mapping:
+ PRINT("- Removing the identity mapping -\r\n")
+
/* r2:r3 := invalid page-table entry */
mov r2, #0x0
mov r3, #0x0
@@ -488,6 +525,8 @@ ENDPROC(remove_identity_mapping)
* Clobbers r0 - r3
*/
remove_temporary_mapping:
+ PRINT("- Removing the temporary mapping -\r\n")
+
/* r2:r3 := invalid page-table entry */
mov r2, #0
mov r3, #0
diff --git a/xen/arch/arm/include/asm/arm32/macros.h b/xen/arch/arm/include/asm/arm32/macros.h
index b84666c764d4..db681a7c7eaa 100644
--- a/xen/arch/arm/include/asm/arm32/macros.h
+++ b/xen/arch/arm/include/asm/arm32/macros.h
@@ -9,9 +9,13 @@
* Move an immediate constant into a 32-bit register using movw/movt
* instructions.
*/
+.macro mov_w_on_cond cond, reg, word
+ movw\cond \reg, #:lower16:\word
+ movt\cond \reg, #:upper16:\word
+.endm
+
.macro mov_w reg, word
- movw \reg, #:lower16:\word
- movt \reg, #:upper16:\word
+ mov_w_on_cond al, \reg, \word
.endm
/*
@@ -29,16 +33,26 @@
#ifdef CONFIG_EARLY_PRINTK
/*
- * Macro to print a string to the UART, if there is one.
+ * Macros to print a string to the UART, if there is one.
+ *
+ * There are multiple flavors:
+ * - PRINT_SECT(section, string): The @string will be located in @section
+ * - PRINT(): The string will be located in .rodata.str.
+ * - PRINT_ID(): When Xen is running on the Identity Mapping, it is
+ * only possible to have a limited amount of Xen. This will create
+ * the string in .rodata.idmap which will always be mapped.
*
* Clobbers r0 - r3
*/
-#define PRINT(_s) \
- mov r3, lr ;\
- adr_l r0, 98f ;\
- bl asm_puts ;\
- mov lr, r3 ;\
- RODATA_STR(98, _s)
+#define PRINT_SECT(section, string) \
+ mov r3, lr ;\
+ adr_l r0, 98f ;\
+ bl asm_puts ;\
+ mov lr, r3 ;\
+ RODATA_SECT(section, 98, string)
+
+#define PRINT(string) PRINT_SECT(.rodata.str, string)
+#define PRINT_ID(string) PRINT_SECT(.rodata.idmap, string)
/*
* Macro to print the value of register \rb
@@ -54,6 +68,7 @@
#else /* CONFIG_EARLY_PRINTK */
#define PRINT(s)
+#define PRINT_ID(s)
.macro print_reg rb
.endm
diff --git a/xen/arch/arm/include/asm/asm_defns.h b/xen/arch/arm/include/asm/asm_defns.h
index 29a9dbb002fa..ec803c0a370c 100644
--- a/xen/arch/arm/include/asm/asm_defns.h
+++ b/xen/arch/arm/include/asm/asm_defns.h
@@ -22,11 +22,13 @@
# error "unknown ARM variant"
#endif
-#define RODATA_STR(label, msg) \
-.pushsection .rodata.str, "aMS", %progbits, 1 ; \
+#define RODATA_SECT(section, label, msg) \
+.pushsection section, "aMS", %progbits, 1 ; \
label: .asciz msg; \
.popsection
+#define RODATA_STR(label, msg) RODATA_SECT(.rodata.str, label, msg)
+
#define ASM_INT(label, val) \
.p2align 2; \
label: .long (val); \
diff --git a/xen/arch/arm/include/asm/early_printk.h b/xen/arch/arm/include/asm/early_printk.h
index c5149b2976da..c1e84f8b0009 100644
--- a/xen/arch/arm/include/asm/early_printk.h
+++ b/xen/arch/arm/include/asm/early_printk.h
@@ -19,6 +19,9 @@
#define EARLY_UART_VIRTUAL_ADDRESS \
(FIXMAP_ADDR(FIXMAP_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
+#define TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS \
+ (TEMPORARY_FIXMAP_ADDR(FIXMAP_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
+
#endif /* !CONFIG_EARLY_PRINTK */
#endif
diff --git a/xen/arch/arm/include/asm/mmu/layout.h b/xen/arch/arm/include/asm/mmu/layout.h
index eac7eef885d6..a3b546465b5a 100644
--- a/xen/arch/arm/include/asm/mmu/layout.h
+++ b/xen/arch/arm/include/asm/mmu/layout.h
@@ -116,6 +116,10 @@
(TEMPORARY_AREA_FIRST_SLOT << XEN_PT_LEVEL_SHIFT(1)))
#define TEMPORARY_XEN_VIRT_START TEMPORARY_AREA_ADDR(XEN_VIRT_START)
+#define TEMPORARY_FIXMAP_VIRT_START TEMPORARY_AREA_ADDR(FIXMAP_VIRT_START)
+
+#define TEMPORARY_FIXMAP_ADDR(n) \
+ (TEMPORARY_FIXMAP_VIRT_START + (n) * PAGE_SIZE)
#else /* ARM_64 */
diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
index d5264e51bc44..72725840b6b7 100644
--- a/xen/arch/arm/mmu/setup.c
+++ b/xen/arch/arm/mmu/setup.c
@@ -120,7 +120,10 @@ static void __init __maybe_unused build_assertions(void)
#ifdef CONFIG_ARM_32
CHECK_SAME_SLOT(first, TEMPORARY_XEN_VIRT_START, DOMHEAP_VIRT_START);
CHECK_DIFFERENT_SLOT(first, XEN_VIRT_START, TEMPORARY_XEN_VIRT_START);
+ CHECK_SAME_SLOT(first, TEMPORARY_XEN_VIRT_START,
+ TEMPORARY_FIXMAP_VIRT_START);
CHECK_SAME_SLOT(second, XEN_VIRT_START, TEMPORARY_XEN_VIRT_START);
+ CHECK_SAME_SLOT(second, FIXMAP_VIRT_START, TEMPORARY_FIXMAP_VIRT_START);
#endif
#undef CHECK_SAME_SLOT
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 59b80d122fd0..20598c6963ce 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -35,6 +35,7 @@ SECTIONS
_idmap_start = .;
*(.text.header)
*(.text.idmap)
+ *(.rodata.idmap)
_idmap_end = .;
*(.text.cold)
--
2.40.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] xen/arm32: head: Rework how the fixmap and early UART mapping are prepared
2024-01-11 18:34 ` [PATCH v2 1/2] xen/arm32: head: Rework how the fixmap and early UART mapping are prepared Julien Grall
@ 2024-01-12 7:46 ` Michal Orzel
2024-01-12 10:44 ` Julien Grall
0 siblings, 1 reply; 9+ messages in thread
From: Michal Orzel @ 2024-01-12 7:46 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
On 11/01/2024 19:34, Julien Grall wrote:
>
>
> From: Julien Grall <jgrall@amazon.com>
>
> Since commit 5e213f0f4d2c ("xen/arm32: head: Widen the use of the
> temporary mapping"), boot_second (used to cover regions like Xen and
> the fixmap) will not be mapped if the identity mapping overlap.
>
> So it is ok to prepare the fixmap table and link it in boot_second
> earlier. With that, the fixmap can also be used earlier via the
> temporary mapping.
>
> Therefore split setup_fixmap() in two:
> * The table is now linked in create_page_tables() because
> the boot page tables needs to be recreated for every CPU.
> * The early UART mapping is only added for the boot CPU0 as the
> fixmap table is not cleared when secondary CPUs boot.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
with below 2 adjustments:
>
> ----
>
> Changelog since v1:
> * Rebase
> * Move the UART mapping enable_mm_boot_cpu()
> ---
> xen/arch/arm/arm32/mmu/head.S | 61 ++++++++---------------------------
> 1 file changed, 14 insertions(+), 47 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/mmu/head.S b/xen/arch/arm/arm32/mmu/head.S
> index 4e6395e7876d..a90799ad5451 100644
> --- a/xen/arch/arm/arm32/mmu/head.S
> +++ b/xen/arch/arm/arm32/mmu/head.S
> @@ -165,11 +165,6 @@
> * Rebuild the boot pagetable's first-level entries. The structure
> * is described in mm.c.
> *
> - * After the CPU enables paging it will add the fixmap mapping
> - * to these page tables, however this may clash with the 1:1
> - * mapping. So each CPU must rebuild the page tables here with
> - * the 1:1 in place.
> - *
> * Inputs:
> * r9 : paddr(start)
> * r10: phys offset
> @@ -197,6 +192,10 @@ create_page_tables:
> add r5, r5, #PAGE_SIZE /* r5 := Next table */
> .endr
>
> + /* Map the fixmap into boot_second */
> + mov_w r0, FIXMAP_ADDR(0)
> + create_table_entry boot_second, xen_fixmap, r0, 2
> +
> /*
> * Find the size of Xen in pages and multiply by the size of a
> * PTE. This will then be compared in the mapping loop below.
> @@ -442,19 +441,20 @@ ENDPROC(enable_secondary_cpu_mm)
> ENTRY(enable_boot_cpu_mm)
> mov r6, lr
>
> +#ifdef CONFIG_EARLY_PRINTK
> + /*
> + * Add the UART mapping to the fixmap so the UART can be used
> + * as the MMU is on. This only need to do done on the boot CPU.
s/need to do done/needs to be done/
> + */
> + mov_w r0, EARLY_UART_VIRTUAL_ADDRESS
> + create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3
Would you mind listing r11 in the Input section of a comment?
~Michal
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] xen/arm32: head: Improve logging in head.S
2024-01-11 18:34 ` [PATCH v2 2/2] xen/arm32: head: Improve logging in head.S Julien Grall
@ 2024-01-12 8:49 ` Michal Orzel
2024-01-12 10:58 ` Julien Grall
0 siblings, 1 reply; 9+ messages in thread
From: Michal Orzel @ 2024-01-12 8:49 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
On 11/01/2024 19:34, Julien Grall wrote:
>
>
> From: Julien Grall <jgrall@amazon.com>
>
> The sequence to enable the MMU on arm32 is quite complex as we may need
> to jump to a temporary mapping to map Xen.
>
> Recently, we had one bug in the logic (see f5a49eb7f8b3 ("xen/arm32:
> head: Add mising isb in switch_to_runtime_mapping()") and it was
> a pain to debug because there are no logging.
>
> In order to improve the logging in the MMU switch we need to add
> support for early printk while running on the identity mapping
> and also on the temporary mapping.
>
> For the identity mapping, we have only the first page of Xen mapped.
> So all the strings should reside in the first page. For that purpose
> a new macro PRINT_ID is introduced.
>
> For the temporary mapping, the fixmap is already linked in the temporary
> area (and so does the UART). So we just need to update the register
> storing the UART address (i.e. r11) to point to the UART temporary
> mapping.
>
> Take the opportunity to introduce mov_w_on_cond in order to
> conditionally execute mov_w and avoid branches.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
with some questions below:
>
> ----
> Changelog since v1:
> - Rebase
> - Move one hunk to the first patch to unbreak compilation
> - Add more logging
> - Remove duplicated entry
> ---
> xen/arch/arm/arm32/head.S | 9 ------
> xen/arch/arm/arm32/mmu/head.S | 39 +++++++++++++++++++++++++
> xen/arch/arm/include/asm/arm32/macros.h | 33 +++++++++++++++------
> xen/arch/arm/include/asm/asm_defns.h | 6 ++--
> xen/arch/arm/include/asm/early_printk.h | 3 ++
> xen/arch/arm/include/asm/mmu/layout.h | 4 +++
> xen/arch/arm/mmu/setup.c | 3 ++
> xen/arch/arm/xen.lds.S | 1 +
> 8 files changed, 78 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 34ab14a9e228..99d7d4aa63d1 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -98,10 +98,6 @@ past_zImage:
> b enable_boot_cpu_mm
>
> primary_switched:
> -#ifdef CONFIG_EARLY_PRINTK
> - /* Use a virtual address to access the UART. */
> - mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
> -#endif
> bl zero_bss
> PRINT("- Ready -\r\n")
> /* Setup the arguments for start_xen and jump to C world */
> @@ -142,12 +138,7 @@ GLOBAL(init_secondary)
>
> mov_w lr, secondary_switched
> b enable_secondary_cpu_mm
> -
> secondary_switched:
> -#ifdef CONFIG_EARLY_PRINTK
> - /* Use a virtual address to access the UART. */
> - mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
> -#endif
> PRINT("- Ready -\r\n")
> /* Jump to C world */
> mov_w r2, start_secondary
> diff --git a/xen/arch/arm/arm32/mmu/head.S b/xen/arch/arm/arm32/mmu/head.S
> index a90799ad5451..f4abd690b612 100644
> --- a/xen/arch/arm/arm32/mmu/head.S
> +++ b/xen/arch/arm/arm32/mmu/head.S
> @@ -298,6 +298,21 @@ enable_mmu:
> mcr CP32(r0, HSCTLR) /* now paging is enabled */
> isb /* Now, flush the icache */
>
> + /*
> + * At this stage, the UART address will depend on whether the
> + * temporary mapping was created or not.
> + *
> + * If it was, then the UART will be mapped in the temporary
> + * area. Otherwise, it will be mapped at runtime virtual
> + * mapping.
> + */
> +#ifdef CONFIG_EARLY_PRINTK
> + teq r12, #1 /* Was the temporary mapping created? */
> + mov_w_on_cond eq, r11, TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS
> + mov_w_on_cond ne, r11, EARLY_UART_VIRTUAL_ADDRESS
> +#endif
> + PRINT_ID("- Paging turned on -\r\n")
> +
> /*
> * The MMU is turned on and we are in the 1:1 mapping. Switch
> * to the runtime mapping.
> @@ -307,6 +322,17 @@ enable_mmu:
> b switch_to_runtime_mapping
> 1:
> mov lr, r5 /* Restore LR */
> +
> + /*
> + * Now we are running at the runtime address. The UART can
> + * be accessed using its runtime virtual address.
> + */
> +#ifdef CONFIG_EARLY_PRINTK
> + mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
> +#endif
> +
> + PRINT("- Switched to the runtime mapping -\r\n")
> +
> /*
> * At this point, either the 1:1 map or the temporary mapping
> * will be present. The former may clash with other parts of the
> @@ -348,12 +374,14 @@ switch_to_runtime_mapping:
> teq r12, #0
> beq ready_to_switch
>
> + PRINT_ID("- Switching to the temporary mapping -\r\n")
> /* We are still in the 1:1 mapping. Jump to the temporary Virtual address. */
> mov_w r0, 1f
> add r0, r0, #XEN_TEMPORARY_OFFSET /* r0 := address in temporary mapping */
> mov pc, r0
>
> 1:
> + PRINT("- Running on the temporary mapping -\r\n")
> /* Remove boot_second_id */
> mov r2, #0
> mov r3, #0
> @@ -364,6 +392,8 @@ switch_to_runtime_mapping:
>
> flush_xen_tlb_local r0
>
> + PRINT("- 1:1 mapping removed -\r\n")
Do I understand it right that we cannot call remove_identity_mapping due to adr_l/mov_w difference?
> +
> /* Map boot_second into boot_pgtable */
> mov_w r0, XEN_VIRT_START
> create_table_entry boot_pgtable, boot_second, r0, 1
> @@ -376,7 +406,10 @@ switch_to_runtime_mapping:
> */
> isb
>
> + PRINT("- Runtime mapping mapped -\r\n")
> ready_to_switch:
> + PRINT_ID("- Jumping to runtime address -\r\n")
> +
> mov pc, lr
> ENDPROC(switch_to_runtime_mapping)
>
> @@ -404,6 +437,8 @@ ENTRY(enable_secondary_cpu_mm)
> mov_w lr, 1f
> b enable_mmu
> 1:
> + PRINT("- Switching to the runtime page-tables -\r\n")
> +
> /*
> * Non-boot CPUs need to move on to the proper pagetables, which were
> * setup in prepare_secondary_mm.
> @@ -468,6 +503,8 @@ ENDPROC(enable_boot_cpu_mm)
> * Clobbers r0 - r3
> */
> remove_identity_mapping:
> + PRINT("- Removing the identity mapping -\r\n")
> +
> /* r2:r3 := invalid page-table entry */
> mov r2, #0x0
> mov r3, #0x0
> @@ -488,6 +525,8 @@ ENDPROC(remove_identity_mapping)
> * Clobbers r0 - r3
> */
> remove_temporary_mapping:
> + PRINT("- Removing the temporary mapping -\r\n")
> +
> /* r2:r3 := invalid page-table entry */
> mov r2, #0
> mov r3, #0
> diff --git a/xen/arch/arm/include/asm/arm32/macros.h b/xen/arch/arm/include/asm/arm32/macros.h
> index b84666c764d4..db681a7c7eaa 100644
> --- a/xen/arch/arm/include/asm/arm32/macros.h
> +++ b/xen/arch/arm/include/asm/arm32/macros.h
> @@ -9,9 +9,13 @@
> * Move an immediate constant into a 32-bit register using movw/movt
> * instructions.
> */
> +.macro mov_w_on_cond cond, reg, word
> + movw\cond \reg, #:lower16:\word
> + movt\cond \reg, #:upper16:\word
> +.endm
> +
> .macro mov_w reg, word
> - movw \reg, #:lower16:\word
> - movt \reg, #:upper16:\word
> + mov_w_on_cond al, \reg, \word
> .endm
>
> /*
> @@ -29,16 +33,26 @@
>
> #ifdef CONFIG_EARLY_PRINTK
> /*
> - * Macro to print a string to the UART, if there is one.
> + * Macros to print a string to the UART, if there is one.
> + *
> + * There are multiple flavors:
> + * - PRINT_SECT(section, string): The @string will be located in @section
> + * - PRINT(): The string will be located in .rodata.str.
> + * - PRINT_ID(): When Xen is running on the Identity Mapping, it is
> + * only possible to have a limited amount of Xen. This will create
> + * the string in .rodata.idmap which will always be mapped.
> *
> * Clobbers r0 - r3
> */
> -#define PRINT(_s) \
> - mov r3, lr ;\
> - adr_l r0, 98f ;\
> - bl asm_puts ;\
> - mov lr, r3 ;\
> - RODATA_STR(98, _s)
> +#define PRINT_SECT(section, string) \
> + mov r3, lr ;\
> + adr_l r0, 98f ;\
> + bl asm_puts ;\
> + mov lr, r3 ;\
> + RODATA_SECT(section, 98, string)
> +
> +#define PRINT(string) PRINT_SECT(.rodata.str, string)
> +#define PRINT_ID(string) PRINT_SECT(.rodata.idmap, string)
I know this is just a macro but does it make sense to have something MMU specific in common header?
I don't expect MPU to use it.
>
> /*
> * Macro to print the value of register \rb
> @@ -54,6 +68,7 @@
>
> #else /* CONFIG_EARLY_PRINTK */
> #define PRINT(s)
> +#define PRINT_ID(s)
>
> .macro print_reg rb
> .endm
> diff --git a/xen/arch/arm/include/asm/asm_defns.h b/xen/arch/arm/include/asm/asm_defns.h
> index 29a9dbb002fa..ec803c0a370c 100644
> --- a/xen/arch/arm/include/asm/asm_defns.h
> +++ b/xen/arch/arm/include/asm/asm_defns.h
> @@ -22,11 +22,13 @@
> # error "unknown ARM variant"
> #endif
>
> -#define RODATA_STR(label, msg) \
> -.pushsection .rodata.str, "aMS", %progbits, 1 ; \
> +#define RODATA_SECT(section, label, msg) \
> +.pushsection section, "aMS", %progbits, 1 ; \
> label: .asciz msg; \
> .popsection
>
> +#define RODATA_STR(label, msg) RODATA_SECT(.rodata.str, label, msg)
> +
> #define ASM_INT(label, val) \
> .p2align 2; \
> label: .long (val); \
> diff --git a/xen/arch/arm/include/asm/early_printk.h b/xen/arch/arm/include/asm/early_printk.h
> index c5149b2976da..c1e84f8b0009 100644
> --- a/xen/arch/arm/include/asm/early_printk.h
> +++ b/xen/arch/arm/include/asm/early_printk.h
> @@ -19,6 +19,9 @@
> #define EARLY_UART_VIRTUAL_ADDRESS \
> (FIXMAP_ADDR(FIXMAP_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
>
> +#define TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS \
> + (TEMPORARY_FIXMAP_ADDR(FIXMAP_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
> +
> #endif /* !CONFIG_EARLY_PRINTK */
>
> #endif
> diff --git a/xen/arch/arm/include/asm/mmu/layout.h b/xen/arch/arm/include/asm/mmu/layout.h
> index eac7eef885d6..a3b546465b5a 100644
> --- a/xen/arch/arm/include/asm/mmu/layout.h
> +++ b/xen/arch/arm/include/asm/mmu/layout.h
> @@ -116,6 +116,10 @@
> (TEMPORARY_AREA_FIRST_SLOT << XEN_PT_LEVEL_SHIFT(1)))
>
> #define TEMPORARY_XEN_VIRT_START TEMPORARY_AREA_ADDR(XEN_VIRT_START)
> +#define TEMPORARY_FIXMAP_VIRT_START TEMPORARY_AREA_ADDR(FIXMAP_VIRT_START)
> +
> +#define TEMPORARY_FIXMAP_ADDR(n) \
> + (TEMPORARY_FIXMAP_VIRT_START + (n) * PAGE_SIZE)
NIT: this could fit in one line
>
> #else /* ARM_64 */
>
> diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
> index d5264e51bc44..72725840b6b7 100644
> --- a/xen/arch/arm/mmu/setup.c
> +++ b/xen/arch/arm/mmu/setup.c
> @@ -120,7 +120,10 @@ static void __init __maybe_unused build_assertions(void)
> #ifdef CONFIG_ARM_32
> CHECK_SAME_SLOT(first, TEMPORARY_XEN_VIRT_START, DOMHEAP_VIRT_START);
> CHECK_DIFFERENT_SLOT(first, XEN_VIRT_START, TEMPORARY_XEN_VIRT_START);
> + CHECK_SAME_SLOT(first, TEMPORARY_XEN_VIRT_START,
> + TEMPORARY_FIXMAP_VIRT_START);
> CHECK_SAME_SLOT(second, XEN_VIRT_START, TEMPORARY_XEN_VIRT_START);
> + CHECK_SAME_SLOT(second, FIXMAP_VIRT_START, TEMPORARY_FIXMAP_VIRT_START);
> #endif
>
> #undef CHECK_SAME_SLOT
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 59b80d122fd0..20598c6963ce 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -35,6 +35,7 @@ SECTIONS
> _idmap_start = .;
> *(.text.header)
> *(.text.idmap)
> + *(.rodata.idmap)
> _idmap_end = .;
>
> *(.text.cold)
> --
> 2.40.1
>
~Michal
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] xen/arm32: head: Rework how the fixmap and early UART mapping are prepared
2024-01-12 7:46 ` Michal Orzel
@ 2024-01-12 10:44 ` Julien Grall
0 siblings, 0 replies; 9+ messages in thread
From: Julien Grall @ 2024-01-12 10:44 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Michal,
On 12/01/2024 07:46, Michal Orzel wrote:
> On 11/01/2024 19:34, Julien Grall wrote:
>>
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Since commit 5e213f0f4d2c ("xen/arm32: head: Widen the use of the
>> temporary mapping"), boot_second (used to cover regions like Xen and
>> the fixmap) will not be mapped if the identity mapping overlap.
>>
>> So it is ok to prepare the fixmap table and link it in boot_second
>> earlier. With that, the fixmap can also be used earlier via the
>> temporary mapping.
>>
>> Therefore split setup_fixmap() in two:
>> * The table is now linked in create_page_tables() because
>> the boot page tables needs to be recreated for every CPU.
>> * The early UART mapping is only added for the boot CPU0 as the
>> fixmap table is not cleared when secondary CPUs boot.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Thanks.
>
> with below 2 adjustments:
I will address them on commit.
>> + */
>> + mov_w r0, EARLY_UART_VIRTUAL_ADDRESS
>> + create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3
> Would you mind listing r11 in the Input section of a comment?
I have added:
r11: UART physical address
>
> ~Michal
--
Julien Grall
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] xen/arm32: head: Improve logging in head.S
2024-01-12 8:49 ` Michal Orzel
@ 2024-01-12 10:58 ` Julien Grall
2024-01-12 11:25 ` Michal Orzel
0 siblings, 1 reply; 9+ messages in thread
From: Julien Grall @ 2024-01-12 10:58 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
On 12/01/2024 08:49, Michal Orzel wrote:
> Hi Julien,
Hi Michal,
> On 11/01/2024 19:34, Julien Grall wrote:
>>
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> The sequence to enable the MMU on arm32 is quite complex as we may need
>> to jump to a temporary mapping to map Xen.
>>
>> Recently, we had one bug in the logic (see f5a49eb7f8b3 ("xen/arm32:
>> head: Add mising isb in switch_to_runtime_mapping()") and it was
>> a pain to debug because there are no logging.
>>
>> In order to improve the logging in the MMU switch we need to add
>> support for early printk while running on the identity mapping
>> and also on the temporary mapping.
>>
>> For the identity mapping, we have only the first page of Xen mapped.
>> So all the strings should reside in the first page. For that purpose
>> a new macro PRINT_ID is introduced.
>>
>> For the temporary mapping, the fixmap is already linked in the temporary
>> area (and so does the UART). So we just need to update the register
>> storing the UART address (i.e. r11) to point to the UART temporary
>> mapping.
>>
>> Take the opportunity to introduce mov_w_on_cond in order to
>> conditionally execute mov_w and avoid branches.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Thanks!
>> /*
>> @@ -29,16 +33,26 @@
>>
>> #ifdef CONFIG_EARLY_PRINTK
>> /*
>> - * Macro to print a string to the UART, if there is one.
>> + * Macros to print a string to the UART, if there is one.
>> + *
>> + * There are multiple flavors:
>> + * - PRINT_SECT(section, string): The @string will be located in @section
>> + * - PRINT(): The string will be located in .rodata.str.
>> + * - PRINT_ID(): When Xen is running on the Identity Mapping, it is
>> + * only possible to have a limited amount of Xen. This will create
>> + * the string in .rodata.idmap which will always be mapped.
>> *
>> * Clobbers r0 - r3
>> */
>> -#define PRINT(_s) \
>> - mov r3, lr ;\
>> - adr_l r0, 98f ;\
>> - bl asm_puts ;\
>> - mov lr, r3 ;\
>> - RODATA_STR(98, _s)
>> +#define PRINT_SECT(section, string) \
>> + mov r3, lr ;\
>> + adr_l r0, 98f ;\
>> + bl asm_puts ;\
>> + mov lr, r3 ;\
>> + RODATA_SECT(section, 98, string)
>> +
>> +#define PRINT(string) PRINT_SECT(.rodata.str, string)
>> +#define PRINT_ID(string) PRINT_SECT(.rodata.idmap, string)
> I know this is just a macro but does it make sense to have something MMU specific in common header?
> I don't expect MPU to use it.
For cache coloring, I would like secondary boot CPUs to start directly
on the colored Xen. This means that any message used before enabling the
MMU will need to be part of the .rodata.idmap.
I know that 32-bit is not in scope for the cache coloring series. But I
would like to keep 32-bit and 64-bit boot logic fairly similar.
With that in mind, would you be happy if I keep PRINT_ID() in macros.h?
Note that I would be ok to move in mmu/head.S and move back in macros.h
later on. I just wanted to avoid code movement :).
>
>>
>> /*
>> * Macro to print the value of register \rb
>> @@ -54,6 +68,7 @@
>>
>> #else /* CONFIG_EARLY_PRINTK */
>> #define PRINT(s)
>> +#define PRINT_ID(s)
>>
>> .macro print_reg rb
>> .endm
>> diff --git a/xen/arch/arm/include/asm/asm_defns.h b/xen/arch/arm/include/asm/asm_defns.h
>> index 29a9dbb002fa..ec803c0a370c 100644
>> --- a/xen/arch/arm/include/asm/asm_defns.h
>> +++ b/xen/arch/arm/include/asm/asm_defns.h
>> @@ -22,11 +22,13 @@
>> # error "unknown ARM variant"
>> #endif
>>
>> -#define RODATA_STR(label, msg) \
>> -.pushsection .rodata.str, "aMS", %progbits, 1 ; \
>> +#define RODATA_SECT(section, label, msg) \
>> +.pushsection section, "aMS", %progbits, 1 ; \
>> label: .asciz msg; \
>> .popsection
>>
>> +#define RODATA_STR(label, msg) RODATA_SECT(.rodata.str, label, msg)
>> +
>> #define ASM_INT(label, val) \
>> .p2align 2; \
>> label: .long (val); \
>> diff --git a/xen/arch/arm/include/asm/early_printk.h b/xen/arch/arm/include/asm/early_printk.h
>> index c5149b2976da..c1e84f8b0009 100644
>> --- a/xen/arch/arm/include/asm/early_printk.h
>> +++ b/xen/arch/arm/include/asm/early_printk.h
>> @@ -19,6 +19,9 @@
>> #define EARLY_UART_VIRTUAL_ADDRESS \
>> (FIXMAP_ADDR(FIXMAP_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
>>
>> +#define TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS \
>> + (TEMPORARY_FIXMAP_ADDR(FIXMAP_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
>> +
>> #endif /* !CONFIG_EARLY_PRINTK */
>>
>> #endif
>> diff --git a/xen/arch/arm/include/asm/mmu/layout.h b/xen/arch/arm/include/asm/mmu/layout.h
>> index eac7eef885d6..a3b546465b5a 100644
>> --- a/xen/arch/arm/include/asm/mmu/layout.h
>> +++ b/xen/arch/arm/include/asm/mmu/layout.h
>> @@ -116,6 +116,10 @@
>> (TEMPORARY_AREA_FIRST_SLOT << XEN_PT_LEVEL_SHIFT(1)))
>>
>> #define TEMPORARY_XEN_VIRT_START TEMPORARY_AREA_ADDR(XEN_VIRT_START)
>> +#define TEMPORARY_FIXMAP_VIRT_START TEMPORARY_AREA_ADDR(FIXMAP_VIRT_START)
>> +
>> +#define TEMPORARY_FIXMAP_ADDR(n) \
>> + (TEMPORARY_FIXMAP_VIRT_START + (n) * PAGE_SIZE)
> NIT: this could fit in one line
It actually doesn't. With the newline, it will be 81 characters.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] xen/arm32: head: Improve logging in head.S
2024-01-12 10:58 ` Julien Grall
@ 2024-01-12 11:25 ` Michal Orzel
2024-01-12 11:55 ` Julien Grall
0 siblings, 1 reply; 9+ messages in thread
From: Michal Orzel @ 2024-01-12 11:25 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
On 12/01/2024 11:58, Julien Grall wrote:
>
>
> On 12/01/2024 08:49, Michal Orzel wrote:
>> Hi Julien,
>
> Hi Michal,
>
>> On 11/01/2024 19:34, Julien Grall wrote:
>>>
>>>
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> The sequence to enable the MMU on arm32 is quite complex as we may need
>>> to jump to a temporary mapping to map Xen.
>>>
>>> Recently, we had one bug in the logic (see f5a49eb7f8b3 ("xen/arm32:
>>> head: Add mising isb in switch_to_runtime_mapping()") and it was
>>> a pain to debug because there are no logging.
>>>
>>> In order to improve the logging in the MMU switch we need to add
>>> support for early printk while running on the identity mapping
>>> and also on the temporary mapping.
>>>
>>> For the identity mapping, we have only the first page of Xen mapped.
>>> So all the strings should reside in the first page. For that purpose
>>> a new macro PRINT_ID is introduced.
>>>
>>> For the temporary mapping, the fixmap is already linked in the temporary
>>> area (and so does the UART). So we just need to update the register
>>> storing the UART address (i.e. r11) to point to the UART temporary
>>> mapping.
>>>
>>> Take the opportunity to introduce mov_w_on_cond in order to
>>> conditionally execute mov_w and avoid branches.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>
> Thanks!
>
>>> /*
>>> @@ -29,16 +33,26 @@
>>>
>>> #ifdef CONFIG_EARLY_PRINTK
>>> /*
>>> - * Macro to print a string to the UART, if there is one.
>>> + * Macros to print a string to the UART, if there is one.
>>> + *
>>> + * There are multiple flavors:
>>> + * - PRINT_SECT(section, string): The @string will be located in @section
>>> + * - PRINT(): The string will be located in .rodata.str.
>>> + * - PRINT_ID(): When Xen is running on the Identity Mapping, it is
>>> + * only possible to have a limited amount of Xen. This will create
>>> + * the string in .rodata.idmap which will always be mapped.
>>> *
>>> * Clobbers r0 - r3
>>> */
>>> -#define PRINT(_s) \
>>> - mov r3, lr ;\
>>> - adr_l r0, 98f ;\
>>> - bl asm_puts ;\
>>> - mov lr, r3 ;\
>>> - RODATA_STR(98, _s)
>>> +#define PRINT_SECT(section, string) \
>>> + mov r3, lr ;\
>>> + adr_l r0, 98f ;\
>>> + bl asm_puts ;\
>>> + mov lr, r3 ;\
>>> + RODATA_SECT(section, 98, string)
>>> +
>>> +#define PRINT(string) PRINT_SECT(.rodata.str, string)
>>> +#define PRINT_ID(string) PRINT_SECT(.rodata.idmap, string)
>> I know this is just a macro but does it make sense to have something MMU specific in common header?
>> I don't expect MPU to use it.
> For cache coloring, I would like secondary boot CPUs to start directly
> on the colored Xen. This means that any message used before enabling the
> MMU will need to be part of the .rodata.idmap.
>
> I know that 32-bit is not in scope for the cache coloring series. But I
> would like to keep 32-bit and 64-bit boot logic fairly similar.
>
> With that in mind, would you be happy if I keep PRINT_ID() in macros.h?
> Note that I would be ok to move in mmu/head.S and move back in macros.h
> later on. I just wanted to avoid code movement :).
With the above explanation it does not make sense to move it back and forth, so let's keep it as is.
~Michal
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] xen/arm32: head: Improve logging in head.S
2024-01-12 11:25 ` Michal Orzel
@ 2024-01-12 11:55 ` Julien Grall
0 siblings, 0 replies; 9+ messages in thread
From: Julien Grall @ 2024-01-12 11:55 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Michal,
On 12/01/2024 11:25, Michal Orzel wrote:
>
>
> On 12/01/2024 11:58, Julien Grall wrote:
>>
>>
>> On 12/01/2024 08:49, Michal Orzel wrote:
>>> Hi Julien,
>>
>> Hi Michal,
>>
>>> On 11/01/2024 19:34, Julien Grall wrote:
>>>>
>>>>
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> The sequence to enable the MMU on arm32 is quite complex as we may need
>>>> to jump to a temporary mapping to map Xen.
>>>>
>>>> Recently, we had one bug in the logic (see f5a49eb7f8b3 ("xen/arm32:
>>>> head: Add mising isb in switch_to_runtime_mapping()") and it was
>>>> a pain to debug because there are no logging.
>>>>
>>>> In order to improve the logging in the MMU switch we need to add
>>>> support for early printk while running on the identity mapping
>>>> and also on the temporary mapping.
>>>>
>>>> For the identity mapping, we have only the first page of Xen mapped.
>>>> So all the strings should reside in the first page. For that purpose
>>>> a new macro PRINT_ID is introduced.
>>>>
>>>> For the temporary mapping, the fixmap is already linked in the temporary
>>>> area (and so does the UART). So we just need to update the register
>>>> storing the UART address (i.e. r11) to point to the UART temporary
>>>> mapping.
>>>>
>>>> Take the opportunity to introduce mov_w_on_cond in order to
>>>> conditionally execute mov_w and avoid branches.
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>
>> Thanks!
>>
>>>> /*
>>>> @@ -29,16 +33,26 @@
>>>>
>>>> #ifdef CONFIG_EARLY_PRINTK
>>>> /*
>>>> - * Macro to print a string to the UART, if there is one.
>>>> + * Macros to print a string to the UART, if there is one.
>>>> + *
>>>> + * There are multiple flavors:
>>>> + * - PRINT_SECT(section, string): The @string will be located in @section
>>>> + * - PRINT(): The string will be located in .rodata.str.
>>>> + * - PRINT_ID(): When Xen is running on the Identity Mapping, it is
>>>> + * only possible to have a limited amount of Xen. This will create
>>>> + * the string in .rodata.idmap which will always be mapped.
>>>> *
>>>> * Clobbers r0 - r3
>>>> */
>>>> -#define PRINT(_s) \
>>>> - mov r3, lr ;\
>>>> - adr_l r0, 98f ;\
>>>> - bl asm_puts ;\
>>>> - mov lr, r3 ;\
>>>> - RODATA_STR(98, _s)
>>>> +#define PRINT_SECT(section, string) \
>>>> + mov r3, lr ;\
>>>> + adr_l r0, 98f ;\
>>>> + bl asm_puts ;\
>>>> + mov lr, r3 ;\
>>>> + RODATA_SECT(section, 98, string)
>>>> +
>>>> +#define PRINT(string) PRINT_SECT(.rodata.str, string)
>>>> +#define PRINT_ID(string) PRINT_SECT(.rodata.idmap, string)
>>> I know this is just a macro but does it make sense to have something MMU specific in common header?
>>> I don't expect MPU to use it.
>> For cache coloring, I would like secondary boot CPUs to start directly
>> on the colored Xen. This means that any message used before enabling the
>> MMU will need to be part of the .rodata.idmap.
>>
>> I know that 32-bit is not in scope for the cache coloring series. But I
>> would like to keep 32-bit and 64-bit boot logic fairly similar.
>>
>> With that in mind, would you be happy if I keep PRINT_ID() in macros.h?
>> Note that I would be ok to move in mmu/head.S and move back in macros.h
>> later on. I just wanted to avoid code movement :).
> With the above explanation it does not make sense to move it back and forth, so let's keep it as is.
Thanks! If that's change, we will move PRINT_ID() to mmu/head.S
I have committed the patch.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-01-12 11:55 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-11 18:34 [PATCH v2 0/2] xen/arm32: Improve logging during early boot Julien Grall
2024-01-11 18:34 ` [PATCH v2 1/2] xen/arm32: head: Rework how the fixmap and early UART mapping are prepared Julien Grall
2024-01-12 7:46 ` Michal Orzel
2024-01-12 10:44 ` Julien Grall
2024-01-11 18:34 ` [PATCH v2 2/2] xen/arm32: head: Improve logging in head.S Julien Grall
2024-01-12 8:49 ` Michal Orzel
2024-01-12 10:58 ` Julien Grall
2024-01-12 11:25 ` Michal Orzel
2024-01-12 11:55 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.