[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/2] xen/arm32: head: Improve logging in head.S
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@xxxxxxxxxx>
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@xxxxxxxxxx>
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
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
|
