* [PATCH 0/3] xen/arm64: head: misc cleanup
@ 2023-11-21 9:45 Michal Orzel
2023-11-21 9:45 ` [PATCH 1/3] xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str Michal Orzel
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Michal Orzel @ 2023-11-21 9:45 UTC (permalink / raw
To: xen-devel
Cc: Michal Orzel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Some cleanup and improvements for the assembly boot code to make the behavior
more consistent between arm32 and arm64.
Michal Orzel (3):
xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str
xen/arm64: Move print_reg macro to asm/arm64/macros.h
xen/arm64/mmu: head: Replace load_paddr with adr_l where appropriate
xen/arch/arm/arm64/head.S | 37 ++++---------------------
xen/arch/arm/arm64/mmu/head.S | 8 +++---
xen/arch/arm/include/asm/arm64/macros.h | 15 ++++++++++
3 files changed, 25 insertions(+), 35 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/3] xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str
2023-11-21 9:45 [PATCH 0/3] xen/arm64: head: misc cleanup Michal Orzel
@ 2023-11-21 9:45 ` Michal Orzel
2023-11-21 15:48 ` Luca Fancellu
2023-11-21 16:09 ` Julien Grall
2023-11-21 9:45 ` [PATCH 2/3] xen/arm64: Move print_reg macro to asm/arm64/macros.h Michal Orzel
2023-11-21 9:45 ` [PATCH 3/3] xen/arm64/mmu: head: Replace load_paddr with adr_l where appropriate Michal Orzel
2 siblings, 2 replies; 19+ messages in thread
From: Michal Orzel @ 2023-11-21 9:45 UTC (permalink / raw
To: xen-devel
Cc: Michal Orzel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
At the moment, the 'hex' string is placed right after the 'putn'
function in the .text section. This is because of the limited range
(+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use
'adr_l' instead (range extended to +/- 4GB) and move the string to
.rodata.str. This way all the earlyprintk messages will be part of .rodata
and the behavior will be consistent with what we already do on arm32.
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
xen/arch/arm/arm64/head.S | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 8dbd3300d89f..b6111399e766 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -498,7 +498,7 @@ ENDPROC(asm_puts)
* Clobbers x0-x3
*/
putn:
- adr x1, hex
+ adr_l x1, hex
mov x3, #16
1:
early_uart_ready x23, 2
@@ -512,8 +512,7 @@ putn:
ret
ENDPROC(putn)
-hex: .ascii "0123456789abcdef"
- .align 2
+RODATA_STR(hex, "0123456789abcdef")
#else /* CONFIG_EARLY_PRINTK */
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/3] xen/arm64: Move print_reg macro to asm/arm64/macros.h
2023-11-21 9:45 [PATCH 0/3] xen/arm64: head: misc cleanup Michal Orzel
2023-11-21 9:45 ` [PATCH 1/3] xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str Michal Orzel
@ 2023-11-21 9:45 ` Michal Orzel
2023-11-21 16:05 ` Luca Fancellu
2023-11-21 16:16 ` Julien Grall
2023-11-21 9:45 ` [PATCH 3/3] xen/arm64/mmu: head: Replace load_paddr with adr_l where appropriate Michal Orzel
2 siblings, 2 replies; 19+ messages in thread
From: Michal Orzel @ 2023-11-21 9:45 UTC (permalink / raw
To: xen-devel
Cc: Michal Orzel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Macro print_reg is used to print a value of a register passed as an
argument. While today it is only used from within the common head.S,
in the future we might want to make use of it from other files, just
like PRINT(). It also serves as a great aid when debugging.
Expose print_reg macro by moving it to asm/arm64/macros.h and:
- rename putn to asm_putn to denote the usage from assembly only,
- use ENTRY() for asm_putn to make it globally visible,
- get rid of unneeded stubs for early_puts, init_uart and putn since
the calls to them are already protected by #ifdef CONFIG_EARLY_PRINTK.
This way the behavior will be consistent with what we already do on arm32.
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
xen/arch/arm/arm64/head.S | 32 ++++---------------------
xen/arch/arm/include/asm/arm64/macros.h | 15 ++++++++++++
2 files changed, 19 insertions(+), 28 deletions(-)
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index b6111399e766..a3dbf81ab515 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -74,25 +74,6 @@
* x30 - lr
*/
- #ifdef CONFIG_EARLY_PRINTK
-/*
- * Macro to print the value of register \xb
- *
- * Clobbers x0 - x4
- */
-.macro print_reg xb
- mov x0, \xb
- mov x4, lr
- bl putn
- mov lr, x4
-.endm
-
-#else /* CONFIG_EARLY_PRINTK */
-.macro print_reg xb
-.endm
-
-#endif /* !CONFIG_EARLY_PRINTK */
-
.section .text.header, "ax", %progbits
/*.aarch64*/
@@ -493,11 +474,12 @@ ENDPROC(asm_puts)
/*
* Print a 64-bit number in hex.
+ * Note: This function must be called from assembly.
* x0: Number to print.
* x23: Early UART base address
* Clobbers x0-x3
*/
-putn:
+ENTRY(asm_putn)
adr_l x1, hex
mov x3, #16
1:
@@ -510,17 +492,11 @@ putn:
subs x3, x3, #1
b.ne 1b
ret
-ENDPROC(putn)
+ENDPROC(asm_putn)
RODATA_STR(hex, "0123456789abcdef")
-#else /* CONFIG_EARLY_PRINTK */
-
-ENTRY(early_puts)
-init_uart:
-putn: ret
-
-#endif /* !CONFIG_EARLY_PRINTK */
+#endif /* CONFIG_EARLY_PRINTK */
/* This provides a C-API version of __lookup_processor_type
* TODO: For now, the implementation return NULL every time
diff --git a/xen/arch/arm/include/asm/arm64/macros.h b/xen/arch/arm/include/asm/arm64/macros.h
index fb9a0602494d..d108dc3a3a71 100644
--- a/xen/arch/arm/include/asm/arm64/macros.h
+++ b/xen/arch/arm/include/asm/arm64/macros.h
@@ -45,9 +45,24 @@
mov lr, x3 ; \
RODATA_STR(98, _s)
+/*
+ * Macro to print the value of register \xb
+ *
+ * Clobbers x0 - x4
+ */
+.macro print_reg xb
+ mov x0, \xb
+ mov x4, lr
+ bl asm_putn
+ mov lr, x4
+.endm
+
#else /* CONFIG_EARLY_PRINTK */
#define PRINT(s)
+.macro print_reg xb
+.endm
+
#endif /* !CONFIG_EARLY_PRINTK */
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/3] xen/arm64/mmu: head: Replace load_paddr with adr_l where appropriate
2023-11-21 9:45 [PATCH 0/3] xen/arm64: head: misc cleanup Michal Orzel
2023-11-21 9:45 ` [PATCH 1/3] xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str Michal Orzel
2023-11-21 9:45 ` [PATCH 2/3] xen/arm64: Move print_reg macro to asm/arm64/macros.h Michal Orzel
@ 2023-11-21 9:45 ` Michal Orzel
2023-11-21 16:30 ` Julien Grall
2023-11-21 16:31 ` Luca Fancellu
2 siblings, 2 replies; 19+ messages in thread
From: Michal Orzel @ 2023-11-21 9:45 UTC (permalink / raw
To: xen-devel
Cc: Michal Orzel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Macros load_paddr and adr_l are equivalent when used before the MMU is
enabled, resulting in obtaining physical address of a symbol. The former
requires to know the physical offset (PA - VA) and can be used both before
and after the MMU is enabled. In the spirit of using something only when
truly necessary, replace all instances of load_paddr with adr_l, except
in create_table_entry macro. Even though there is currently no use of
load_paddr after MMU is enabled, this macro used to be call in such a
context and we can't rule out that it won't happen again.
This way, the logic behind using load_paddr/adr_l is consistent between
arm32 and arm64, making it easier for developers to determine which one
to use and when.
Take the opportunity to fix a comment with incorrect function name.
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
xen/arch/arm/arm64/mmu/head.S | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S
index 10774f30e40e..41779020eb4d 100644
--- a/xen/arch/arm/arm64/mmu/head.S
+++ b/xen/arch/arm/arm64/mmu/head.S
@@ -146,10 +146,10 @@ create_page_tables:
/*
* We need to use a stash register because
- * create_table_entry_paddr() will clobber the register storing
+ * create_table_entry_from_paddr() will clobber the register storing
* the physical address of the table to point to.
*/
- load_paddr x4, boot_third
+ adr_l x4, boot_third
ldr x1, =XEN_VIRT_START
.rept XEN_NR_ENTRIES(2)
mov x0, x4 /* x0 := paddr(l3 table) */
@@ -311,7 +311,7 @@ ENDPROC(enable_mmu)
ENTRY(enable_secondary_cpu_mm)
mov x6, lr
- load_paddr x0, init_ttbr
+ adr_l x0, init_ttbr
ldr x0, [x0]
mov x1, #SCTLR_Axx_ELx_WXN /* Enable WxN from the start */
@@ -336,7 +336,7 @@ ENTRY(enable_boot_cpu_mm)
mov x6, lr
bl create_page_tables
- load_paddr x0, boot_pgtable
+ adr_l x0, boot_pgtable
mov x1, #0 /* No extra SCTLR flags */
bl enable_mmu
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str
2023-11-21 9:45 ` [PATCH 1/3] xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str Michal Orzel
@ 2023-11-21 15:48 ` Luca Fancellu
2023-11-21 16:09 ` Julien Grall
1 sibling, 0 replies; 19+ messages in thread
From: Luca Fancellu @ 2023-11-21 15:48 UTC (permalink / raw
To: Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
> On 21 Nov 2023, at 09:45, Michal Orzel <michal.orzel@amd.com> wrote:
>
> At the moment, the 'hex' string is placed right after the 'putn'
> function in the .text section. This is because of the limited range
> (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use
> 'adr_l' instead (range extended to +/- 4GB) and move the string to
> .rodata.str. This way all the earlyprintk messages will be part of .rodata
> and the behavior will be consistent with what we already do on arm32.
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Hi Michal,
I’ve also tested on FVP
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-by: Luca Fancellu <luca.fancellu@arm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] xen/arm64: Move print_reg macro to asm/arm64/macros.h
2023-11-21 9:45 ` [PATCH 2/3] xen/arm64: Move print_reg macro to asm/arm64/macros.h Michal Orzel
@ 2023-11-21 16:05 ` Luca Fancellu
2023-11-21 16:16 ` Julien Grall
1 sibling, 0 replies; 19+ messages in thread
From: Luca Fancellu @ 2023-11-21 16:05 UTC (permalink / raw
To: Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
> On 21 Nov 2023, at 09:45, Michal Orzel <michal.orzel@amd.com> wrote:
>
> Macro print_reg is used to print a value of a register passed as an
> argument. While today it is only used from within the common head.S,
> in the future we might want to make use of it from other files, just
> like PRINT(). It also serves as a great aid when debugging.
>
> Expose print_reg macro by moving it to asm/arm64/macros.h and:
> - rename putn to asm_putn to denote the usage from assembly only,
> - use ENTRY() for asm_putn to make it globally visible,
> - get rid of unneeded stubs for early_puts, init_uart and putn since
> the calls to them are already protected by #ifdef CONFIG_EARLY_PRINTK.
>
> This way the behavior will be consistent with what we already do on arm32.
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Hi Michal,
I’ve also tested on FVP
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-by: Luca Fancellu <luca.fancellu@arm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str
2023-11-21 9:45 ` [PATCH 1/3] xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str Michal Orzel
2023-11-21 15:48 ` Luca Fancellu
@ 2023-11-21 16:09 ` Julien Grall
2023-11-21 17:00 ` Michal Orzel
1 sibling, 1 reply; 19+ messages in thread
From: Julien Grall @ 2023-11-21 16:09 UTC (permalink / raw
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Michal,
On 21/11/2023 09:45, Michal Orzel wrote:
> At the moment, the 'hex' string is placed right after the 'putn'
> function in the .text section. This is because of the limited range
> (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use
> 'adr_l' instead (range extended to +/- 4GB) and move the string to
> .rodata.str. This way all the earlyprintk messages will be part of .rodata
> and the behavior will be consistent with what we already do on arm32.
This will be correct today, but I am actually thinking to move 'hex' to
.rodata.idmap so it can be used while we are on the 1:1 mapping and/or
temporary mapping.
So I would consider to drop this patch for now.
Cheers,
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
> xen/arch/arm/arm64/head.S | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 8dbd3300d89f..b6111399e766 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -498,7 +498,7 @@ ENDPROC(asm_puts)
> * Clobbers x0-x3
> */
> putn:
> - adr x1, hex
> + adr_l x1, hex
> mov x3, #16
> 1:
> early_uart_ready x23, 2
> @@ -512,8 +512,7 @@ putn:
> ret
> ENDPROC(putn)
>
> -hex: .ascii "0123456789abcdef"
> - .align 2
> +RODATA_STR(hex, "0123456789abcdef")
>
> #else /* CONFIG_EARLY_PRINTK */
>
[1] https://lore.kernel.org/20231101233011.83098-3-julien@xen.org
--
Julien Grall
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] xen/arm64: Move print_reg macro to asm/arm64/macros.h
2023-11-21 9:45 ` [PATCH 2/3] xen/arm64: Move print_reg macro to asm/arm64/macros.h Michal Orzel
2023-11-21 16:05 ` Luca Fancellu
@ 2023-11-21 16:16 ` Julien Grall
2023-11-21 17:02 ` Michal Orzel
1 sibling, 1 reply; 19+ messages in thread
From: Julien Grall @ 2023-11-21 16:16 UTC (permalink / raw
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
On 21/11/2023 09:45, Michal Orzel wrote:
> Macro print_reg is used to print a value of a register passed as an
> argument. While today it is only used from within the common head.S,
> in the future we might want to make use of it from other files, just
> like PRINT(). It also serves as a great aid when debugging.
>
> Expose print_reg macro by moving it to asm/arm64/macros.h and:
> - rename putn to asm_putn to denote the usage from assembly only,
> - use ENTRY() for asm_putn to make it globally visible,
> - get rid of unneeded stubs for early_puts, init_uart and putn since
> the calls to them are already protected by #ifdef CONFIG_EARLY_PRINTK.
NIT: The last one read as this is necessary to move print_reg() to
asm/arm64/macros. But really, this is just a clean-up. So I would add
something like "Take the opportunity to..." or similar to make it clearer.
Other than that:
Acked-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] xen/arm64/mmu: head: Replace load_paddr with adr_l where appropriate
2023-11-21 9:45 ` [PATCH 3/3] xen/arm64/mmu: head: Replace load_paddr with adr_l where appropriate Michal Orzel
@ 2023-11-21 16:30 ` Julien Grall
2023-11-21 18:13 ` Michal Orzel
2023-11-21 16:31 ` Luca Fancellu
1 sibling, 1 reply; 19+ messages in thread
From: Julien Grall @ 2023-11-21 16:30 UTC (permalink / raw
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Michal,
On 21/11/2023 09:45, Michal Orzel wrote:
> Macros load_paddr and adr_l are equivalent when used before the MMU is
> enabled, resulting in obtaining physical address of a symbol. The former
> requires to know the physical offset (PA - VA) and can be used both before
> and after the MMU is enabled. In the spirit of using something only when
> truly necessary, replace all instances of load_paddr with adr_l, except
I don't buy this argument. The advantage with using "load_paddr" is that
it is pretty clear what you get from the call. With "adr_l" you will
need to check whether the MMU is on or off.
> in create_table_entry macro. Even though there is currently no use of
> load_paddr after MMU is enabled, this macro used to be call in such a
> context and we can't rule out that it won't happen again.
>
> This way, the logic behind using load_paddr/adr_l is consistent between
> arm32 and arm64, making it easier for developers to determine which one
> to use and when.
Not really. See above. But there is also no documentation stating that
"load_paddr" should not be used before the MMU is on. And as I said
above, I find it easier to work with compare to "adr_l".
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] xen/arm64/mmu: head: Replace load_paddr with adr_l where appropriate
2023-11-21 9:45 ` [PATCH 3/3] xen/arm64/mmu: head: Replace load_paddr with adr_l where appropriate Michal Orzel
2023-11-21 16:30 ` Julien Grall
@ 2023-11-21 16:31 ` Luca Fancellu
1 sibling, 0 replies; 19+ messages in thread
From: Luca Fancellu @ 2023-11-21 16:31 UTC (permalink / raw
To: Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
> On 21 Nov 2023, at 09:45, Michal Orzel <michal.orzel@amd.com> wrote:
>
> Macros load_paddr and adr_l are equivalent when used before the MMU is
> enabled, resulting in obtaining physical address of a symbol. The former
> requires to know the physical offset (PA - VA) and can be used both before
> and after the MMU is enabled. In the spirit of using something only when
> truly necessary, replace all instances of load_paddr with adr_l, except
> in create_table_entry macro. Even though there is currently no use of
> load_paddr after MMU is enabled, this macro used to be call in such a
> context and we can't rule out that it won't happen again.
>
> This way, the logic behind using load_paddr/adr_l is consistent between
> arm32 and arm64, making it easier for developers to determine which one
> to use and when.
>
> Take the opportunity to fix a comment with incorrect function name.
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
Hi Michal,
I’ve also tested on FVP
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-by: Luca Fancellu <luca.fancellu@arm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str
2023-11-21 16:09 ` Julien Grall
@ 2023-11-21 17:00 ` Michal Orzel
2023-11-21 17:04 ` Julien Grall
0 siblings, 1 reply; 19+ messages in thread
From: Michal Orzel @ 2023-11-21 17:00 UTC (permalink / raw
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
On 21/11/2023 17:09, Julien Grall wrote:
>
>
> Hi Michal,
>
> On 21/11/2023 09:45, Michal Orzel wrote:
>> At the moment, the 'hex' string is placed right after the 'putn'
>> function in the .text section. This is because of the limited range
>> (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use
>> 'adr_l' instead (range extended to +/- 4GB) and move the string to
>> .rodata.str. This way all the earlyprintk messages will be part of .rodata
>> and the behavior will be consistent with what we already do on arm32.
>
> This will be correct today, but I am actually thinking to move 'hex' to
> .rodata.idmap so it can be used while we are on the 1:1 mapping and/or
> temporary mapping.
So you are planning on extending your series to also cover arm64?
If that is the case, then I'm ok on postponing/dropping this patch.
~Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] xen/arm64: Move print_reg macro to asm/arm64/macros.h
2023-11-21 16:16 ` Julien Grall
@ 2023-11-21 17:02 ` Michal Orzel
0 siblings, 0 replies; 19+ messages in thread
From: Michal Orzel @ 2023-11-21 17:02 UTC (permalink / raw
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
On 21/11/2023 17:16, Julien Grall wrote:
>
>
> On 21/11/2023 09:45, Michal Orzel wrote:
>> Macro print_reg is used to print a value of a register passed as an
>> argument. While today it is only used from within the common head.S,
>> in the future we might want to make use of it from other files, just
>> like PRINT(). It also serves as a great aid when debugging.
>>
>> Expose print_reg macro by moving it to asm/arm64/macros.h and:
>> - rename putn to asm_putn to denote the usage from assembly only,
>> - use ENTRY() for asm_putn to make it globally visible,
>> - get rid of unneeded stubs for early_puts, init_uart and putn since
>> the calls to them are already protected by #ifdef CONFIG_EARLY_PRINTK.
>
> NIT: The last one read as this is necessary to move print_reg() to
> asm/arm64/macros. But really, this is just a clean-up. So I would add
> something like "Take the opportunity to..." or similar to make it clearer.
Ok, I can do that.
~Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str
2023-11-21 17:00 ` Michal Orzel
@ 2023-11-21 17:04 ` Julien Grall
2023-11-21 17:18 ` Michal Orzel
0 siblings, 1 reply; 19+ messages in thread
From: Julien Grall @ 2023-11-21 17:04 UTC (permalink / raw
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
On 21/11/2023 17:00, Michal Orzel wrote:
> Hi Julien,
Hi,
> On 21/11/2023 17:09, Julien Grall wrote:
>>
>>
>> Hi Michal,
>>
>> On 21/11/2023 09:45, Michal Orzel wrote:
>>> At the moment, the 'hex' string is placed right after the 'putn'
>>> function in the .text section. This is because of the limited range
>>> (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use
>>> 'adr_l' instead (range extended to +/- 4GB) and move the string to
>>> .rodata.str. This way all the earlyprintk messages will be part of .rodata
>>> and the behavior will be consistent with what we already do on arm32.
>>
>> This will be correct today, but I am actually thinking to move 'hex' to
>> .rodata.idmap so it can be used while we are on the 1:1 mapping and/or
>> temporary mapping.
> So you are planning on extending your series to also cover arm64?
It is not in my soonish plan. But you are arguing that this patch is for
consistency with arm32. This will not be after my series.
And I would not change arm64 just for consistency because I don't view
it as necessary. The boot code is already not the same.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str
2023-11-21 17:04 ` Julien Grall
@ 2023-11-21 17:18 ` Michal Orzel
2023-11-21 18:31 ` Julien Grall
0 siblings, 1 reply; 19+ messages in thread
From: Michal Orzel @ 2023-11-21 17:18 UTC (permalink / raw
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
On 21/11/2023 18:04, Julien Grall wrote:
>
>
> On 21/11/2023 17:00, Michal Orzel wrote:
>> Hi Julien,
>
> Hi,
>
>> On 21/11/2023 17:09, Julien Grall wrote:
>>>
>>>
>>> Hi Michal,
>>>
>>> On 21/11/2023 09:45, Michal Orzel wrote:
>>>> At the moment, the 'hex' string is placed right after the 'putn'
>>>> function in the .text section. This is because of the limited range
>>>> (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use
>>>> 'adr_l' instead (range extended to +/- 4GB) and move the string to
>>>> .rodata.str. This way all the earlyprintk messages will be part of .rodata
>>>> and the behavior will be consistent with what we already do on arm32.
>>>
>>> This will be correct today, but I am actually thinking to move 'hex' to
>>> .rodata.idmap so it can be used while we are on the 1:1 mapping and/or
>>> temporary mapping.
>> So you are planning on extending your series to also cover arm64?
>
> It is not in my soonish plan. But you are arguing that this patch is for
> consistency with arm32. This will not be after my series.
Hmm, consistency was not the only reason for sending this patch. It's a beneficial side effect
given that putn implementations are very similar on arm32/arm64. Are you saying that
moving a string that is const from .text to .rodata is not a good change?
>
> And I would not change arm64 just for consistency because I don't view
> it as necessary. The boot code is already not the same.
I don't recall trying to make the entire boot code consistent. But if there are parts that
are almost identical, I think symmetry is welcomed. It helps reading the code, at least for me.
~Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] xen/arm64/mmu: head: Replace load_paddr with adr_l where appropriate
2023-11-21 16:30 ` Julien Grall
@ 2023-11-21 18:13 ` Michal Orzel
2023-11-21 18:43 ` Julien Grall
0 siblings, 1 reply; 19+ messages in thread
From: Michal Orzel @ 2023-11-21 18:13 UTC (permalink / raw
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
On 21/11/2023 17:30, Julien Grall wrote:
>
>
> Hi Michal,
>
> On 21/11/2023 09:45, Michal Orzel wrote:
>> Macros load_paddr and adr_l are equivalent when used before the MMU is
>> enabled, resulting in obtaining physical address of a symbol. The former
>> requires to know the physical offset (PA - VA) and can be used both before
>> and after the MMU is enabled. In the spirit of using something only when
>> truly necessary, replace all instances of load_paddr with adr_l, except
>
> I don't buy this argument. The advantage with using "load_paddr" is that
> it is pretty clear what you get from the call. With "adr_l" you will
> need to check whether the MMU is on or off.
>
>> in create_table_entry macro. Even though there is currently no use of
>> load_paddr after MMU is enabled, this macro used to be call in such a
>> context and we can't rule out that it won't happen again.
>>
>> This way, the logic behind using load_paddr/adr_l is consistent between
>> arm32 and arm64, making it easier for developers to determine which one
>> to use and when.
>
> Not really. See above. But there is also no documentation stating that
> "load_paddr" should not be used before the MMU is on. And as I said
> above, I find it easier to work with compare to "adr_l".
I guess it is a matter of taste. load_paddr requires adding a physical offset to
calculate an address, where in fact we have no places in the code where this is truly needed.
Seeing an instance of this macro makes me think that this piece of code runs with MMU enabled.
We could in fact remove it completely and the only reason I did not is because you told me [1] that
one day we might want to use it.
[1] https://lore.kernel.org/xen-devel/2b10267a-514c-4c9b-b715-f65c26d7f757@xen.org/
~Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str
2023-11-21 17:18 ` Michal Orzel
@ 2023-11-21 18:31 ` Julien Grall
2023-11-22 7:54 ` Michal Orzel
0 siblings, 1 reply; 19+ messages in thread
From: Julien Grall @ 2023-11-21 18:31 UTC (permalink / raw
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Michal,
On 21/11/2023 17:18, Michal Orzel wrote:
>
>
> On 21/11/2023 18:04, Julien Grall wrote:
>>
>>
>> On 21/11/2023 17:00, Michal Orzel wrote:
>>> Hi Julien,
>>
>> Hi,
>>
>>> On 21/11/2023 17:09, Julien Grall wrote:
>>>>
>>>>
>>>> Hi Michal,
>>>>
>>>> On 21/11/2023 09:45, Michal Orzel wrote:
>>>>> At the moment, the 'hex' string is placed right after the 'putn'
>>>>> function in the .text section. This is because of the limited range
>>>>> (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use
>>>>> 'adr_l' instead (range extended to +/- 4GB) and move the string to
>>>>> .rodata.str. This way all the earlyprintk messages will be part of .rodata
>>>>> and the behavior will be consistent with what we already do on arm32.
>>>>
>>>> This will be correct today, but I am actually thinking to move 'hex' to
>>>> .rodata.idmap so it can be used while we are on the 1:1 mapping and/or
>>>> temporary mapping.
>>> So you are planning on extending your series to also cover arm64?
>>
>> It is not in my soonish plan. But you are arguing that this patch is for
>> consistency with arm32. This will not be after my series.
> Hmm, consistency was not the only reason for sending this patch. It's a beneficial side effect
> given that putn implementations are very similar on arm32/arm64.
> Are you saying that
> moving a string that is const from .text to .rodata is not a good change?
.rodata is better but I would rather prefer if this is moved to
.rodata.idmap directly.
>
>>
>> And I would not change arm64 just for consistency because I don't view
>> it as necessary. The boot code is already not the same.
> I don't recall trying to make the entire boot code consistent. But if there are parts that
> are almost identical, I think symmetry is welcomed. It helps reading the code, at least for me.
If you want symmetry, then I am happy to switch to .rodata.idmap. But I
would rather not first move to .rodata and then in a month time switch
to .rodata.idmap.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] xen/arm64/mmu: head: Replace load_paddr with adr_l where appropriate
2023-11-21 18:13 ` Michal Orzel
@ 2023-11-21 18:43 ` Julien Grall
2023-11-22 8:12 ` Michal Orzel
0 siblings, 1 reply; 19+ messages in thread
From: Julien Grall @ 2023-11-21 18:43 UTC (permalink / raw
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi,
On 21/11/2023 18:13, Michal Orzel wrote:
> On 21/11/2023 17:30, Julien Grall wrote:
>>
>>
>> Hi Michal,
>>
>> On 21/11/2023 09:45, Michal Orzel wrote:
>>> Macros load_paddr and adr_l are equivalent when used before the MMU is
>>> enabled, resulting in obtaining physical address of a symbol. The former
>>> requires to know the physical offset (PA - VA) and can be used both before
>>> and after the MMU is enabled. In the spirit of using something only when
>>> truly necessary, replace all instances of load_paddr with adr_l, except
>>
>> I don't buy this argument. The advantage with using "load_paddr" is that
>> it is pretty clear what you get from the call. With "adr_l" you will
>> need to check whether the MMU is on or off.
>>
>>> in create_table_entry macro. Even though there is currently no use of
>>> load_paddr after MMU is enabled, this macro used to be call in such a
>>> context and we can't rule out that it won't happen again.
>>>
>>> This way, the logic behind using load_paddr/adr_l is consistent between
>>> arm32 and arm64, making it easier for developers to determine which one
>>> to use and when.
>>
>> Not really. See above. But there is also no documentation stating that
>> "load_paddr" should not be used before the MMU is on. And as I said
>> above, I find it easier to work with compare to "adr_l".
> I guess it is a matter of taste. load_paddr requires adding a physical offset to
I agree this is a matter of taste.
> calculate an address, where in fact we have no places in the code where this is truly needed.
I added adr_l only recently (2019). Before hand, we were using
open-coded adrp and load_paddr (which was introduced in 2017).
> Seeing an instance of this macro makes me think that this piece of code runs with MMU enabled.
Fair enough. But how do you know when 'adr_l' effectively returns a
physical address or virtual address? You could go through the functions
call but that's fairly cumbersome.
This is why I don't particularly like this change and I am afraid, I
will not ack it.
> We could in fact remove it completely and the only reason I did not is because you told me [1] that
> one day we might want to use it.
Yes I still have plan to use it.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str
2023-11-21 18:31 ` Julien Grall
@ 2023-11-22 7:54 ` Michal Orzel
0 siblings, 0 replies; 19+ messages in thread
From: Michal Orzel @ 2023-11-22 7:54 UTC (permalink / raw
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi,
On 21/11/2023 19:31, Julien Grall wrote:
>
>
> Hi Michal,
>
> On 21/11/2023 17:18, Michal Orzel wrote:
>>
>>
>> On 21/11/2023 18:04, Julien Grall wrote:
>>>
>>>
>>> On 21/11/2023 17:00, Michal Orzel wrote:
>>>> Hi Julien,
>>>
>>> Hi,
>>>
>>>> On 21/11/2023 17:09, Julien Grall wrote:
>>>>>
>>>>>
>>>>> Hi Michal,
>>>>>
>>>>> On 21/11/2023 09:45, Michal Orzel wrote:
>>>>>> At the moment, the 'hex' string is placed right after the 'putn'
>>>>>> function in the .text section. This is because of the limited range
>>>>>> (+/- 1MB) of PC relative 'adr' instruction 'putn' uses. Modify it to use
>>>>>> 'adr_l' instead (range extended to +/- 4GB) and move the string to
>>>>>> .rodata.str. This way all the earlyprintk messages will be part of .rodata
>>>>>> and the behavior will be consistent with what we already do on arm32.
>>>>>
>>>>> This will be correct today, but I am actually thinking to move 'hex' to
>>>>> .rodata.idmap so it can be used while we are on the 1:1 mapping and/or
>>>>> temporary mapping.
>>>> So you are planning on extending your series to also cover arm64?
>>>
>>> It is not in my soonish plan. But you are arguing that this patch is for
>>> consistency with arm32. This will not be after my series.
>> Hmm, consistency was not the only reason for sending this patch. It's a beneficial side effect
>> given that putn implementations are very similar on arm32/arm64.
>> Are you saying that
>> moving a string that is const from .text to .rodata is not a good change?
>
> .rodata is better but I would rather prefer if this is moved to
> .rodata.idmap directly.
>
>>
>>>
>>> And I would not change arm64 just for consistency because I don't view
>>> it as necessary. The boot code is already not the same.
>> I don't recall trying to make the entire boot code consistent. But if there are parts that
>> are almost identical, I think symmetry is welcomed. It helps reading the code, at least for me.
>
> If you want symmetry, then I am happy to switch to .rodata.idmap. But I
> would rather not first move to .rodata and then in a month time switch
> to .rodata.idmap.
Ok, fair enough.
~Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] xen/arm64/mmu: head: Replace load_paddr with adr_l where appropriate
2023-11-21 18:43 ` Julien Grall
@ 2023-11-22 8:12 ` Michal Orzel
0 siblings, 0 replies; 19+ messages in thread
From: Michal Orzel @ 2023-11-22 8:12 UTC (permalink / raw
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi,
On 21/11/2023 19:43, Julien Grall wrote:
>
>
> Hi,
>
> On 21/11/2023 18:13, Michal Orzel wrote:
>> On 21/11/2023 17:30, Julien Grall wrote:
>>>
>>>
>>> Hi Michal,
>>>
>>> On 21/11/2023 09:45, Michal Orzel wrote:
>>>> Macros load_paddr and adr_l are equivalent when used before the MMU is
>>>> enabled, resulting in obtaining physical address of a symbol. The former
>>>> requires to know the physical offset (PA - VA) and can be used both before
>>>> and after the MMU is enabled. In the spirit of using something only when
>>>> truly necessary, replace all instances of load_paddr with adr_l, except
>>>
>>> I don't buy this argument. The advantage with using "load_paddr" is that
>>> it is pretty clear what you get from the call. With "adr_l" you will
>>> need to check whether the MMU is on or off.
>>>
>>>> in create_table_entry macro. Even though there is currently no use of
>>>> load_paddr after MMU is enabled, this macro used to be call in such a
>>>> context and we can't rule out that it won't happen again.
>>>>
>>>> This way, the logic behind using load_paddr/adr_l is consistent between
>>>> arm32 and arm64, making it easier for developers to determine which one
>>>> to use and when.
>>>
>>> Not really. See above. But there is also no documentation stating that
>>> "load_paddr" should not be used before the MMU is on. And as I said
>>> above, I find it easier to work with compare to "adr_l".
>> I guess it is a matter of taste. load_paddr requires adding a physical offset to
>
> I agree this is a matter of taste.
>
>> calculate an address, where in fact we have no places in the code where this is truly needed.
>
> I added adr_l only recently (2019). Before hand, we were using
> open-coded adrp and load_paddr (which was introduced in 2017).
>
>> Seeing an instance of this macro makes me think that this piece of code runs with MMU enabled.
>
> Fair enough. But how do you know when 'adr_l' effectively returns a
> physical address or virtual address? You could go through the functions
> call but that's fairly cumbersome.
I see your point but we already use adr_l in MMU code. Also, recently we accepted a patch from Ayan
for arm32 that does exactly the same - load_paddr is used only in one place in MMU head.S where it is required
(I thought we are aligned on this subject + I shared my plan some weeks ago). Ayan's change together with my patch
would make it obvious that we use load_paddr only in MMU enabled context. That is why I struggle to understand why nacking this patch
if you let the other one go. IMO this can create confusion for a future developer \wrt which one to use.
~Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-11-22 8:13 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-21 9:45 [PATCH 0/3] xen/arm64: head: misc cleanup Michal Orzel
2023-11-21 9:45 ` [PATCH 1/3] xen/arm64: head: Move earlyprintk 'hex' string to .rodata.str Michal Orzel
2023-11-21 15:48 ` Luca Fancellu
2023-11-21 16:09 ` Julien Grall
2023-11-21 17:00 ` Michal Orzel
2023-11-21 17:04 ` Julien Grall
2023-11-21 17:18 ` Michal Orzel
2023-11-21 18:31 ` Julien Grall
2023-11-22 7:54 ` Michal Orzel
2023-11-21 9:45 ` [PATCH 2/3] xen/arm64: Move print_reg macro to asm/arm64/macros.h Michal Orzel
2023-11-21 16:05 ` Luca Fancellu
2023-11-21 16:16 ` Julien Grall
2023-11-21 17:02 ` Michal Orzel
2023-11-21 9:45 ` [PATCH 3/3] xen/arm64/mmu: head: Replace load_paddr with adr_l where appropriate Michal Orzel
2023-11-21 16:30 ` Julien Grall
2023-11-21 18:13 ` Michal Orzel
2023-11-21 18:43 ` Julien Grall
2023-11-22 8:12 ` Michal Orzel
2023-11-21 16:31 ` Luca Fancellu
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.