[05/17] xen/arm64: head: Introduce print_reg
diff mbox series

Message ID 20190610193215.23704-6-julien.grall@arm.com
State New
Headers show
Series
  • xen/arm64: Rework head.S to make it more compliant with the Arm Arm
Related show

Commit Message

Julien Grall June 10, 2019, 7:32 p.m. UTC
At the moment, the user should save x30/lr if it cares about it.

Follow-up patches will introduce more use of putn in place where lr
should be preserved.

Furthermore, any user of putn should also move the value to register x0
if it was stored in a different register.

For convenience, a new macro is introduced to print a given register.
The macro will take care for us to move the value to x0 and also
preserve lr.

Lastly the new macro is used to replace all the callsite of putn. This
will simplify rework/review later on.

Note that CurrentEL is now stored in x5 instead of x4 because the latter
will be clobbered by the macro print_reg.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm64/head.S | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

Comments

Stefano Stabellini June 26, 2019, 12:09 a.m. UTC | #1
On Mon, 10 Jun 2019, Julien Grall wrote:
> At the moment, the user should save x30/lr if it cares about it.
> 
> Follow-up patches will introduce more use of putn in place where lr
> should be preserved.
> 
> Furthermore, any user of putn should also move the value to register x0
> if it was stored in a different register.
> 
> For convenience, a new macro is introduced to print a given register.
> The macro will take care for us to move the value to x0 and also
> preserve lr.
> 
> Lastly the new macro is used to replace all the callsite of putn. This
> will simplify rework/review later on.
> 
> Note that CurrentEL is now stored in x5 instead of x4 because the latter
> will be clobbered by the macro print_reg.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 84e26582c4..9142b4a774 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -90,8 +90,25 @@
>          bl    puts    ;     \
>          mov   lr, x3  ;     \
>          RODATA_STR(98, _s)
> +
> +/*
> + * Macro to print the value of register \xb
> + *
> + * Clobbers x0 - x4
> + */
> +.macro print_reg xb
> +        mov   x4, lr
> +        mov   x0, \xb
> +        bl    putn
> +        mov   lr, x4

I have the same weird issues with my compiler as before, replacing 'lr'
with 'x30' solves the problem.

This patch looks OK though.


> +.endm
> +
>  #else /* CONFIG_EARLY_PRINTK */
>  #define PRINT(s)
> +
> +.macro print_reg xb
> +.endm
> +
>  #endif /* !CONFIG_EARLY_PRINTK */
>  
>  /* Load the physical address of a symbol into xb */
> @@ -304,22 +321,20 @@ GLOBAL(init_secondary)
>  #ifdef CONFIG_EARLY_PRINTK
>          ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
>          PRINT("- CPU ")
> -        mov   x0, x24
> -        bl    putn
> +        print_reg x24
>          PRINT(" booting -\r\n")
>  #endif
>  
>  common_start:
>  
>          PRINT("- Current EL ")
> -        mrs   x4, CurrentEL
> -        mov   x0, x4
> -        bl    putn
> +        mrs   x5, CurrentEL
> +        print_reg x5
>          PRINT(" -\r\n")
>  
>          /* Are we in EL2 */
> -        cmp   x4, #PSR_MODE_EL2t
> -        ccmp  x4, #PSR_MODE_EL2h, #0x4, ne
> +        cmp   x5, #PSR_MODE_EL2t
> +        ccmp  x5, #PSR_MODE_EL2h, #0x4, ne
>          b.eq  el2 /* Yes */
>  
>          /* OK, we're boned. */
> -- 
> 2.11.0
>
Julien Grall June 26, 2019, 9:10 a.m. UTC | #2
Hi Stefano,

On 26/06/2019 01:09, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> At the moment, the user should save x30/lr if it cares about it.
>>
>> Follow-up patches will introduce more use of putn in place where lr
>> should be preserved.
>>
>> Furthermore, any user of putn should also move the value to register x0
>> if it was stored in a different register.
>>
>> For convenience, a new macro is introduced to print a given register.
>> The macro will take care for us to move the value to x0 and also
>> preserve lr.
>>
>> Lastly the new macro is used to replace all the callsite of putn. This
>> will simplify rework/review later on.
>>
>> Note that CurrentEL is now stored in x5 instead of x4 because the latter
>> will be clobbered by the macro print_reg.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/arm64/head.S | 29 ++++++++++++++++++++++-------
>>   1 file changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 84e26582c4..9142b4a774 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -90,8 +90,25 @@
>>           bl    puts    ;     \
>>           mov   lr, x3  ;     \
>>           RODATA_STR(98, _s)
>> +
>> +/*
>> + * Macro to print the value of register \xb
>> + *
>> + * Clobbers x0 - x4
>> + */
>> +.macro print_reg xb
>> +        mov   x4, lr
>> +        mov   x0, \xb
>> +        bl    putn
>> +        mov   lr, x4
> 
> I have the same weird issues with my compiler as before, replacing 'lr'
> with 'x30' solves the problem.

Can you have a try with the following line?

lr      .req    x30             // link register

Cheers,
Volodymyr Babchuk July 15, 2019, 6:46 p.m. UTC | #3
Hi Julien,

Julien Grall writes:

> At the moment, the user should save x30/lr if it cares about it.
>
> Follow-up patches will introduce more use of putn in place where lr
> should be preserved.
>
> Furthermore, any user of putn should also move the value to register x0
> if it was stored in a different register.
>
> For convenience, a new macro is introduced to print a given register.
> The macro will take care for us to move the value to x0 and also
> preserve lr.
>
> Lastly the new macro is used to replace all the callsite of putn. This
> will simplify rework/review later on.
>
> Note that CurrentEL is now stored in x5 instead of x4 because the latter
> will be clobbered by the macro print_reg.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 84e26582c4..9142b4a774 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -90,8 +90,25 @@
>          bl    puts    ;     \
>          mov   lr, x3  ;     \
>          RODATA_STR(98, _s)
> +
> +/*
> + * Macro to print the value of register \xb
> + *
> + * Clobbers x0 - x4
> + */

Despite its name, this macro can't print x4. I would recommend adding at
least comment about this. Static assertion would be even better, but
looks like we don't have them for asm code.

> +.macro print_reg xb
> +        mov   x4, lr
> +        mov   x0, \xb
> +        bl    putn
> +        mov   lr, x4
> +.endm
> +
>  #else /* CONFIG_EARLY_PRINTK */
>  #define PRINT(s)
> +
> +.macro print_reg xb
> +.endm
> +
>  #endif /* !CONFIG_EARLY_PRINTK */
>
>  /* Load the physical address of a symbol into xb */
> @@ -304,22 +321,20 @@ GLOBAL(init_secondary)
>  #ifdef CONFIG_EARLY_PRINTK
>          ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
>          PRINT("- CPU ")
> -        mov   x0, x24
> -        bl    putn
> +        print_reg x24
>          PRINT(" booting -\r\n")
>  #endif
>
>  common_start:
>
>          PRINT("- Current EL ")
> -        mrs   x4, CurrentEL
> -        mov   x0, x4
> -        bl    putn
> +        mrs   x5, CurrentEL
> +        print_reg x5
>          PRINT(" -\r\n")
>
>          /* Are we in EL2 */
> -        cmp   x4, #PSR_MODE_EL2t
> -        ccmp  x4, #PSR_MODE_EL2h, #0x4, ne
> +        cmp   x5, #PSR_MODE_EL2t
> +        ccmp  x5, #PSR_MODE_EL2h, #0x4, ne
>          b.eq  el2 /* Yes */
>
>          /* OK, we're boned. */


--
Best regards, Volodymyr Babchuk
Julien Grall July 16, 2019, 9:55 a.m. UTC | #4
On 7/15/19 7:46 PM, Volodymyr Babchuk wrote:
> Hi Julien,

Hi,

> Julien Grall writes:
> 
>> At the moment, the user should save x30/lr if it cares about it.
>>
>> Follow-up patches will introduce more use of putn in place where lr
>> should be preserved.
>>
>> Furthermore, any user of putn should also move the value to register x0
>> if it was stored in a different register.
>>
>> For convenience, a new macro is introduced to print a given register.
>> The macro will take care for us to move the value to x0 and also
>> preserve lr.
>>
>> Lastly the new macro is used to replace all the callsite of putn. This
>> will simplify rework/review later on.
>>
>> Note that CurrentEL is now stored in x5 instead of x4 because the latter
>> will be clobbered by the macro print_reg.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/arm64/head.S | 29 ++++++++++++++++++++++-------
>>   1 file changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 84e26582c4..9142b4a774 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -90,8 +90,25 @@
>>           bl    puts    ;     \
>>           mov   lr, x3  ;     \
>>           RODATA_STR(98, _s)
>> +
>> +/*
>> + * Macro to print the value of register \xb
>> + *
>> + * Clobbers x0 - x4
>> + */
> 
> Despite its name, this macro can't print x4. I would recommend adding at
> least comment about this. Static assertion would be even better, but
> looks like we don't have them for asm code.

I have a new version of the function allowing to print x4. It is 
basically just swapping "mov x4, lr" with "mov x0, \xb".

> 
>> +.macro print_reg xb
>> +        mov   x4, lr
>> +        mov   x0, \xb
>> +        bl    putn
>> +        mov   lr, x4
>> +.endm
>> +
>>   #else /* CONFIG_EARLY_PRINTK */
>>   #define PRINT(s)

Cheers,

Patch
diff mbox series

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 84e26582c4..9142b4a774 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -90,8 +90,25 @@ 
         bl    puts    ;     \
         mov   lr, x3  ;     \
         RODATA_STR(98, _s)
+
+/*
+ * Macro to print the value of register \xb
+ *
+ * Clobbers x0 - x4
+ */
+.macro print_reg xb
+        mov   x4, lr
+        mov   x0, \xb
+        bl    putn
+        mov   lr, x4
+.endm
+
 #else /* CONFIG_EARLY_PRINTK */
 #define PRINT(s)
+
+.macro print_reg xb
+.endm
+
 #endif /* !CONFIG_EARLY_PRINTK */
 
 /* Load the physical address of a symbol into xb */
@@ -304,22 +321,20 @@  GLOBAL(init_secondary)
 #ifdef CONFIG_EARLY_PRINTK
         ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
         PRINT("- CPU ")
-        mov   x0, x24
-        bl    putn
+        print_reg x24
         PRINT(" booting -\r\n")
 #endif
 
 common_start:
 
         PRINT("- Current EL ")
-        mrs   x4, CurrentEL
-        mov   x0, x4
-        bl    putn
+        mrs   x5, CurrentEL
+        print_reg x5
         PRINT(" -\r\n")
 
         /* Are we in EL2 */
-        cmp   x4, #PSR_MODE_EL2t
-        ccmp  x4, #PSR_MODE_EL2h, #0x4, ne
+        cmp   x5, #PSR_MODE_EL2t
+        ccmp  x5, #PSR_MODE_EL2h, #0x4, ne
         b.eq  el2 /* Yes */
 
         /* OK, we're boned. */