diff mbox series

[5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str

Message ID 20220812192448.43016-6-julien@xen.org (mailing list archive)
State New, archived
Headers show
Series xen/arm: More clean-ups and improvement | expand

Commit Message

Julien Grall Aug. 12, 2022, 7:24 p.m. UTC 
From: Julien Grall <jgrall@amazon.com>

At the moment, the strings are in text right after each use because
the instruction 'adr' has specific requirement on the location
and the compiler will forbid cross section label.

The macro 'adr_l' was recently reworked so the caller doesn't need
to know whether the MMU is on. This makes it easier to use where
instructions can be run in both context.

This also means that the strings don't need to be part of .text
anymore. So move them to .rodata.str.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/arm32/head.S | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

Jiamei Xie Aug. 15, 2022, 1:57 a.m. UTC | #1 
Hi Julien,

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Julien Grall
> Sent: Saturday, August 13, 2022 3:25 AM
> To: xen-devel@lists.xenproject.org
> Cc: julien@xen.org; Julien Grall <jgrall@amazon.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: [PATCH 5/7] xen/arm32: head: Move earlyprintk messages
> to .rodata.str
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, the strings are in text right after each use because
> the instruction 'adr' has specific requirement on the location
> and the compiler will forbid cross section label.
> 
> The macro 'adr_l' was recently reworked so the caller doesn't need
> to know whether the MMU is on. This makes it easier to use where
> instructions can be run in both context.
> 
> This also means that the strings don't need to be part of .text
> anymore. So move them to .rodata.str.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>  xen/arch/arm/arm32/head.S | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 27d02ac8d68f..a558c2a6876e 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -93,13 +93,10 @@
>   */
>  #define PRINT(_s)           \
>          mov   r3, lr       ;\
> -        adr   r0, 98f      ;\
> +        adr_l r0, 98f      ;\
>          bl    puts         ;\
>          mov   lr, r3       ;\
> -        b     99f          ;\
> -98:     .asciz _s          ;\
> -        .align 2           ;\
> -99:
> +        RODATA_STR(98, _s)
> 
>  /*
>   * Macro to print the value of register \rb
> @@ -736,7 +733,7 @@ ENDPROC(puts)
>   * Clobbers r0-r3
>   */
>  putn:
> -        adr   r1, hex
> +        adr_l r1, hex
>          mov   r3, #8
>  1:
>          early_uart_ready r11, r2
> @@ -749,8 +746,7 @@ putn:
>          mov   pc, lr
>  ENDPROC(putn)
> 
> -hex:    .ascii "0123456789abcdef"
> -        .align 2
> +RODATA_STR(hex, "0123456789abcdef")
> 
>  #else  /* CONFIG_EARLY_PRINTK */
> 
> --
> 2.37.1
> 

That looks good to me.
Reviewed-by: Jiamei Xie <jiamei.xie@arm.com>

Best wishes
Jiamei Xie
Wei Chen Aug. 15, 2022, 2:05 a.m. UTC | #2 
Hi Julien,


On 2022/8/13 3:24, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, the strings are in text right after each use because
> the instruction 'adr' has specific requirement on the location
> and the compiler will forbid cross section label.
> 
> The macro 'adr_l' was recently reworked so the caller doesn't need
> to know whether the MMU is on. This makes it easier to use where
> instructions can be run in both context.
> 
> This also means that the strings don't need to be part of .text
> anymore. So move them to .rodata.str.
> 

This sounds very good!

> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>   xen/arch/arm/arm32/head.S | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 27d02ac8d68f..a558c2a6876e 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -93,13 +93,10 @@
>    */
>   #define PRINT(_s)           \
>           mov   r3, lr       ;\
> -        adr   r0, 98f      ;\
> +        adr_l r0, 98f      ;\
>           bl    puts         ;\
>           mov   lr, r3       ;\
> -        b     99f          ;\
> -98:     .asciz _s          ;\
> -        .align 2           ;\
> -99:
> +        RODATA_STR(98, _s)
>   
>   /*
>    * Macro to print the value of register \rb
> @@ -736,7 +733,7 @@ ENDPROC(puts)
>    * Clobbers r0-r3
>    */
>   putn:
> -        adr   r1, hex
> +        adr_l r1, hex
>           mov   r3, #8
>   1:
>           early_uart_ready r11, r2
> @@ -749,8 +746,7 @@ putn:
>           mov   pc, lr
>   ENDPROC(putn)
>   
> -hex:    .ascii "0123456789abcdef"
> -        .align 2
> +RODATA_STR(hex, "0123456789abcdef")
>   
>   #else  /* CONFIG_EARLY_PRINTK */
>   

Reviewed-by: Wei Chen <Wei.Chen@arm.com>
Jan Beulich Aug. 15, 2022, 6:43 a.m. UTC | #3 
On 12.08.2022 21:24, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, the strings are in text right after each use because
> the instruction 'adr' has specific requirement on the location
> and the compiler will forbid cross section label.
> 
> The macro 'adr_l' was recently reworked so the caller doesn't need
> to know whether the MMU is on. This makes it easier to use where
> instructions can be run in both context.
> 
> This also means that the strings don't need to be part of .text
> anymore. So move them to .rodata.str.

Wouldn't they better live somewhere in .init* ?

> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -93,13 +93,10 @@
>   */
>  #define PRINT(_s)           \
>          mov   r3, lr       ;\
> -        adr   r0, 98f      ;\
> +        adr_l r0, 98f      ;\
>          bl    puts         ;\
>          mov   lr, r3       ;\
> -        b     99f          ;\
> -98:     .asciz _s          ;\
> -        .align 2           ;\
> -99:
> +        RODATA_STR(98, _s)

Nit: How about using uniform indentation here  and ...

> @@ -736,7 +733,7 @@ ENDPROC(puts)
>   * Clobbers r0-r3
>   */
>  putn:
> -        adr   r1, hex
> +        adr_l r1, hex
>          mov   r3, #8
>  1:
>          early_uart_ready r11, r2
> @@ -749,8 +746,7 @@ putn:
>          mov   pc, lr
>  ENDPROC(putn)
>  
> -hex:    .ascii "0123456789abcdef"
> -        .align 2
> +RODATA_STR(hex, "0123456789abcdef")

... here?

Jan
Julien Grall Aug. 15, 2022, 8:17 a.m. UTC | #4 
Hi Jan,

On 15/08/2022 07:43, Jan Beulich wrote:
> On 12.08.2022 21:24, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> At the moment, the strings are in text right after each use because
>> the instruction 'adr' has specific requirement on the location
>> and the compiler will forbid cross section label.
>>
>> The macro 'adr_l' was recently reworked so the caller doesn't need
>> to know whether the MMU is on. This makes it easier to use where
>> instructions can be run in both context.
>>
>> This also means that the strings don't need to be part of .text
>> anymore. So move them to .rodata.str.
> 
> Wouldn't they better live somewhere in .init* ?

PRINT() is also used in path for secondary bring-up. So this could be 
used after .init (even though today CPU hotplug doesn't work on Arm).

Furthermore, PRINT() is only used when earlyprintk is enabled. This 
should only be used in a development environment (gated by 
CONFIG_DEBUG). So I think it is better to keep all the strings in 
.rodata.str rather than trying to distinguish whether the caller will 
happen only during init on boot.

Cheers,
Jan Beulich Aug. 15, 2022, 8:21 a.m. UTC | #5 
On 15.08.2022 10:17, Julien Grall wrote:
> On 15/08/2022 07:43, Jan Beulich wrote:
>> On 12.08.2022 21:24, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> At the moment, the strings are in text right after each use because
>>> the instruction 'adr' has specific requirement on the location
>>> and the compiler will forbid cross section label.
>>>
>>> The macro 'adr_l' was recently reworked so the caller doesn't need
>>> to know whether the MMU is on. This makes it easier to use where
>>> instructions can be run in both context.
>>>
>>> This also means that the strings don't need to be part of .text
>>> anymore. So move them to .rodata.str.
>>
>> Wouldn't they better live somewhere in .init* ?
> 
> PRINT() is also used in path for secondary bring-up. So this could be 
> used after .init (even though today CPU hotplug doesn't work on Arm).

Then the term "earlyprintk" looks to be misleading?

> Furthermore, PRINT() is only used when earlyprintk is enabled. This 
> should only be used in a development environment (gated by 
> CONFIG_DEBUG). So I think it is better to keep all the strings in 
> .rodata.str rather than trying to distinguish whether the caller will 
> happen only during init on boot.

Fair enough.

Jan
Bertrand Marquis Aug. 15, 2022, 4:26 p.m. UTC | #6 
Hi Julien,

> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, the strings are in text right after each use because
> the instruction 'adr' has specific requirement on the location
> and the compiler will forbid cross section label.
> 
> The macro 'adr_l' was recently reworked so the caller doesn't need
> to know whether the MMU is on. This makes it easier to use where
> instructions can be run in both context.
> 
> This also means that the strings don't need to be part of .text
> anymore. So move them to .rodata.str.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Tested by enabling early printk on qemu arm32 and it works so:

Tested-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/arch/arm/arm32/head.S | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 27d02ac8d68f..a558c2a6876e 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -93,13 +93,10 @@
>  */
> #define PRINT(_s)           \
>         mov   r3, lr       ;\
> -        adr   r0, 98f      ;\
> +        adr_l r0, 98f      ;\
>         bl    puts         ;\
>         mov   lr, r3       ;\
> -        b     99f          ;\
> -98:     .asciz _s          ;\
> -        .align 2           ;\
> -99:
> +        RODATA_STR(98, _s)
> 
> /*
>  * Macro to print the value of register \rb
> @@ -736,7 +733,7 @@ ENDPROC(puts)
>  * Clobbers r0-r3
>  */
> putn:
> -        adr   r1, hex
> +        adr_l r1, hex
>         mov   r3, #8
> 1:
>         early_uart_ready r11, r2
> @@ -749,8 +746,7 @@ putn:
>         mov   pc, lr
> ENDPROC(putn)
> 
> -hex:    .ascii "0123456789abcdef"
> -        .align 2
> +RODATA_STR(hex, "0123456789abcdef")
> 
> #else  /* CONFIG_EARLY_PRINTK */
> 
> -- 
> 2.37.1
>
Julien Grall Aug. 15, 2022, 4:46 p.m. UTC | #7 
Hi Jan,

On 15/08/2022 09:21, Jan Beulich wrote:
> On 15.08.2022 10:17, Julien Grall wrote:
>> On 15/08/2022 07:43, Jan Beulich wrote:
>>> On 12.08.2022 21:24, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> At the moment, the strings are in text right after each use because
>>>> the instruction 'adr' has specific requirement on the location
>>>> and the compiler will forbid cross section label.
>>>>
>>>> The macro 'adr_l' was recently reworked so the caller doesn't need
>>>> to know whether the MMU is on. This makes it easier to use where
>>>> instructions can be run in both context.
>>>>
>>>> This also means that the strings don't need to be part of .text
>>>> anymore. So move them to .rodata.str.
>>>
>>> Wouldn't they better live somewhere in .init* ?
>>
>> PRINT() is also used in path for secondary bring-up. So this could be
>> used after .init (even though today CPU hotplug doesn't work on Arm).
> 
> Then the term "earlyprintk" looks to be misleading?
I can't think of a better name. "early" could also be interpreted as 
"early CPU bring-up".

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 27d02ac8d68f..a558c2a6876e 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -93,13 +93,10 @@ 
  */
 #define PRINT(_s)           \
         mov   r3, lr       ;\
-        adr   r0, 98f      ;\
+        adr_l r0, 98f      ;\
         bl    puts         ;\
         mov   lr, r3       ;\
-        b     99f          ;\
-98:     .asciz _s          ;\
-        .align 2           ;\
-99:
+        RODATA_STR(98, _s)
 
 /*
  * Macro to print the value of register \rb
@@ -736,7 +733,7 @@  ENDPROC(puts)
  * Clobbers r0-r3
  */
 putn:
-        adr   r1, hex
+        adr_l r1, hex
         mov   r3, #8
 1:
         early_uart_ready r11, r2
@@ -749,8 +746,7 @@  putn:
         mov   pc, lr
 ENDPROC(putn)
 
-hex:    .ascii "0123456789abcdef"
-        .align 2
+RODATA_STR(hex, "0123456789abcdef")
 
 #else  /* CONFIG_EARLY_PRINTK */