diff mbox series

[v4,03/13] xen/arm64: prepare for moving MMU related code from head.S

Message ID 20230801034419.2047541-4-Henry.Wang@arm.com (mailing list archive)
State Superseded
Headers show
Series xen/arm: Split MMU code as the prepration of MPU work | expand

Commit Message

Henry Wang Aug. 1, 2023, 3:44 a.m. UTC 
From: Wei Chen <wei.chen@arm.com>

We want to reuse head.S for MPU systems, but there are some
code are implemented for MMU systems only. We will move such
code to another MMU specific file. But before that we will
do some indentations fix in this patch to make them be easier
for reviewing:
1. Fix the indentations and incorrect style of code comments.
2. Fix the indentations for .text.header section.
3. Rename puts() to asm_puts() for global export

Signed-off-by: Wei Chen <wei.chen@arm.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
v4:
- Rebase to pick the adr -> adr_l change in PRINT(_s).
- Correct in-code comment for asm_puts() and add a note to
  mention that asm_puts() should be only called from assembly.
- Drop redundant puts (now asm_puts) under CONFIG_EARLY_PRINTK.
v3:
-  fix commit message
-  Rename puts() to asm_puts() for global export
v2:
-  New patch.
---
 xen/arch/arm/arm64/head.S | 46 ++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

Comments

Ayan Kumar Halder Aug. 7, 2023, 1:22 p.m. UTC | #1 
On 01/08/2023 04:44, Henry Wang wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> From: Wei Chen <wei.chen@arm.com>
>
> We want to reuse head.S for MPU systems, but there are some
> code are implemented for MMU systems only. We will move such
> code to another MMU specific file. But before that we will
> do some indentations fix in this patch to make them be easier
> for reviewing:
> 1. Fix the indentations and incorrect style of code comments.
> 2. Fix the indentations for .text.header section.
> 3. Rename puts() to asm_puts() for global export
>
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> v4:
> - Rebase to pick the adr -> adr_l change in PRINT(_s).
> - Correct in-code comment for asm_puts() and add a note to
>    mention that asm_puts() should be only called from assembly.
> - Drop redundant puts (now asm_puts) under CONFIG_EARLY_PRINTK.
> v3:
> -  fix commit message
> -  Rename puts() to asm_puts() for global export
> v2:
> -  New patch.
> ---
>   xen/arch/arm/arm64/head.S | 46 ++++++++++++++++++++-------------------
>   1 file changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 2af9f974d5..b29bffce5b 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -94,7 +94,7 @@
>   #define PRINT(_s)          \
>           mov   x3, lr ;     \
>           adr_l x0, 98f ;    \
> -        bl    puts    ;    \
> +        bl    asm_puts ;   \
>           mov   lr, x3 ;     \
>           RODATA_STR(98, _s)
>
> @@ -136,21 +136,21 @@
>           add \xb, \xb, x20
>   .endm
>
> -        .section .text.header, "ax", %progbits
> -        /*.aarch64*/
> +.section .text.header, "ax", %progbits
> +/*.aarch64*/
>
> -        /*
> -         * Kernel startup entry point.
> -         * ---------------------------
> -         *
> -         * The requirements are:
> -         *   MMU = off, D-cache = off, I-cache = on or off,
> -         *   x0 = physical address to the FDT blob.
> -         *
> -         * This must be the very first address in the loaded image.
> -         * It should be linked at XEN_VIRT_START, and loaded at any
> -         * 4K-aligned address.
> -         */
> +/*
> + * Kernel startup entry point.
> + * ---------------------------
> + *
> + * The requirements are:
> + *   MMU = off, D-cache = off, I-cache = on or off,
> + *   x0 = physical address to the FDT blob.
> + *
> + * This must be the very first address in the loaded image.
> + * It should be linked at XEN_VIRT_START, and loaded at any
> + * 4K-aligned address.
> + */
>
>   GLOBAL(start)
>           /*
> @@ -535,7 +535,7 @@ ENDPROC(cpu_init)
>    * Macro to create a mapping entry in \tbl to \phys. Only mapping in 3rd
>    * level table (i.e page granularity) is supported.
>    *
> - * ptbl:     table symbol where the entry will be created
> + * ptbl:    table symbol where the entry will be created
>    * virt:    virtual address
>    * phys:    physical address (should be page aligned)
>    * tmp1:    scratch register
> @@ -970,19 +970,22 @@ init_uart:
>           ret
>   ENDPROC(init_uart)
>
> -/* Print early debug messages.
> +/*
> + * Print early debug messages.
> + * Note: This function is only supposed to be called from assembly.
>    * x0: Nul-terminated string to print.
>    * x23: Early UART base address
> - * Clobbers x0-x1 */
> -puts:
> + * Clobbers x0-x1
> + */
> +ENTRY(asm_puts)
>           early_uart_ready x23, 1
>           ldrb  w1, [x0], #1           /* Load next char */
>           cbz   w1, 1f                 /* Exit on nul */
>           early_uart_transmit x23, w1
> -        b     puts
> +        b     asm_puts
>   1:
>           ret
> -ENDPROC(puts)
> +ENDPROC(asm_puts)
>
>   /*
>    * Print a 64-bit number in hex.
> @@ -1012,7 +1015,6 @@ hex:    .ascii "0123456789abcdef"
>
>   ENTRY(early_puts)
>   init_uart:
> -puts:
>   putn:   ret
>
>   #endif /* !CONFIG_EARLY_PRINTK */
> --
> 2.25.1
>
>
Julien Grall Aug. 9, 2023, 12:12 p.m. UTC | #2 
Hi Henry,

On 01/08/2023 04:44, Henry Wang wrote:
> From: Wei Chen <wei.chen@arm.com>
> 
> We want to reuse head.S for MPU systems, but there are some
> code are implemented for MMU systems only. We will move such
> code to another MMU specific file. But before that we will
> do some indentations fix in this patch to make them be easier
> for reviewing:
> 1. Fix the indentations and incorrect style of code comments.
> 2. Fix the indentations for .text.header section.
> 3. Rename puts() to asm_puts() for global export
> 
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>

With one remark below:

Reviewed-by: Julien Grall <jgrall@amazon.com>

> ---
> v4:
> - Rebase to pick the adr -> adr_l change in PRINT(_s).
> - Correct in-code comment for asm_puts() and add a note to
>    mention that asm_puts() should be only called from assembly.
> - Drop redundant puts (now asm_puts) under CONFIG_EARLY_PRINTK.
> v3:
> -  fix commit message
> -  Rename puts() to asm_puts() for global export
> v2:
> -  New patch.
> ---
>   xen/arch/arm/arm64/head.S | 46 ++++++++++++++++++++-------------------
>   1 file changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 2af9f974d5..b29bffce5b 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -94,7 +94,7 @@
>   #define PRINT(_s)          \
>           mov   x3, lr ;     \
>           adr_l x0, 98f ;    \
> -        bl    puts    ;    \
> +        bl    asm_puts ;   \
>           mov   lr, x3 ;     \
>           RODATA_STR(98, _s)
>   
> @@ -136,21 +136,21 @@
>           add \xb, \xb, x20
>   .endm
>   
> -        .section .text.header, "ax", %progbits
> -        /*.aarch64*/
> +.section .text.header, "ax", %progbits
> +/*.aarch64*/
>   
> -        /*
> -         * Kernel startup entry point.
> -         * ---------------------------
> -         *
> -         * The requirements are:
> -         *   MMU = off, D-cache = off, I-cache = on or off,
> -         *   x0 = physical address to the FDT blob.
> -         *
> -         * This must be the very first address in the loaded image.
> -         * It should be linked at XEN_VIRT_START, and loaded at any
> -         * 4K-aligned address.
> -         */
> +/*
> + * Kernel startup entry point.
> + * ---------------------------
> + *
> + * The requirements are:
> + *   MMU = off, D-cache = off, I-cache = on or off,
> + *   x0 = physical address to the FDT blob.
> + *
> + * This must be the very first address in the loaded image.
> + * It should be linked at XEN_VIRT_START, and loaded at any
> + * 4K-aligned address.
> + */
>   
>   GLOBAL(start)
>           /*
> @@ -535,7 +535,7 @@ ENDPROC(cpu_init)
>    * Macro to create a mapping entry in \tbl to \phys. Only mapping in 3rd
>    * level table (i.e page granularity) is supported.
>    *
> - * ptbl:     table symbol where the entry will be created
> + * ptbl:    table symbol where the entry will be created
>    * virt:    virtual address
>    * phys:    physical address (should be page aligned)
>    * tmp1:    scratch register
> @@ -970,19 +970,22 @@ init_uart:
>           ret
>   ENDPROC(init_uart)
>   
> -/* Print early debug messages.
> +/*
> + * Print early debug messages.
> + * Note: This function is only supposed to be called from assembly.

I realize the wording is just a copy of what I wrote earlier on. But I 
would use 'must' as this is a stronger than 'is supposed to'.

This can be dealt on commit if there is nothing else to change.

Cheers,
Henry Wang Aug. 10, 2023, 7:03 a.m. UTC | #3 
Hi Julien,

> On Aug 9, 2023, at 20:12, Julien Grall <julien@xen.org> wrote:
> 
> Hi Henry,
> 
> On 01/08/2023 04:44, Henry Wang wrote:
>> From: Wei Chen <wei.chen@arm.com>
>> We want to reuse head.S for MPU systems, but there are some
>> code are implemented for MMU systems only. We will move such
>> code to another MMU specific file. But before that we will
>> do some indentations fix in this patch to make them be easier
>> for reviewing:
>> 1. Fix the indentations and incorrect style of code comments.
>> 2. Fix the indentations for .text.header section.
>> 3. Rename puts() to asm_puts() for global export
>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> 
> With one remark below:
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Thanks! I have taken this tag with...

> 
>> 
>>  -/* Print early debug messages.
>> +/*
>> + * Print early debug messages.
>> + * Note: This function is only supposed to be called from assembly.
> 
> I realize the wording is just a copy of what I wrote earlier on. But I would use 'must' as this is a stronger than 'is supposed to'.

…this fixed locally.

> 
> This can be dealt on commit if there is nothing else to change.

Either way works for me, maybe you can add this patch to your queue
for your next round of committing. If I don’t see this patch being committed
by the time when I send v5, I will keep this patch in the v5 series and send
it with your Reviewed-by tag.

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall
diff mbox series

Patch

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 2af9f974d5..b29bffce5b 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -94,7 +94,7 @@ 
 #define PRINT(_s)          \
         mov   x3, lr ;     \
         adr_l x0, 98f ;    \
-        bl    puts    ;    \
+        bl    asm_puts ;   \
         mov   lr, x3 ;     \
         RODATA_STR(98, _s)
 
@@ -136,21 +136,21 @@ 
         add \xb, \xb, x20
 .endm
 
-        .section .text.header, "ax", %progbits
-        /*.aarch64*/
+.section .text.header, "ax", %progbits
+/*.aarch64*/
 
-        /*
-         * Kernel startup entry point.
-         * ---------------------------
-         *
-         * The requirements are:
-         *   MMU = off, D-cache = off, I-cache = on or off,
-         *   x0 = physical address to the FDT blob.
-         *
-         * This must be the very first address in the loaded image.
-         * It should be linked at XEN_VIRT_START, and loaded at any
-         * 4K-aligned address.
-         */
+/*
+ * Kernel startup entry point.
+ * ---------------------------
+ *
+ * The requirements are:
+ *   MMU = off, D-cache = off, I-cache = on or off,
+ *   x0 = physical address to the FDT blob.
+ *
+ * This must be the very first address in the loaded image.
+ * It should be linked at XEN_VIRT_START, and loaded at any
+ * 4K-aligned address.
+ */
 
 GLOBAL(start)
         /*
@@ -535,7 +535,7 @@  ENDPROC(cpu_init)
  * Macro to create a mapping entry in \tbl to \phys. Only mapping in 3rd
  * level table (i.e page granularity) is supported.
  *
- * ptbl:     table symbol where the entry will be created
+ * ptbl:    table symbol where the entry will be created
  * virt:    virtual address
  * phys:    physical address (should be page aligned)
  * tmp1:    scratch register
@@ -970,19 +970,22 @@  init_uart:
         ret
 ENDPROC(init_uart)
 
-/* Print early debug messages.
+/*
+ * Print early debug messages.
+ * Note: This function is only supposed to be called from assembly.
  * x0: Nul-terminated string to print.
  * x23: Early UART base address
- * Clobbers x0-x1 */
-puts:
+ * Clobbers x0-x1
+ */
+ENTRY(asm_puts)
         early_uart_ready x23, 1
         ldrb  w1, [x0], #1           /* Load next char */
         cbz   w1, 1f                 /* Exit on nul */
         early_uart_transmit x23, w1
-        b     puts
+        b     asm_puts
 1:
         ret
-ENDPROC(puts)
+ENDPROC(asm_puts)
 
 /*
  * Print a 64-bit number in hex.
@@ -1012,7 +1015,6 @@  hex:    .ascii "0123456789abcdef"
 
 ENTRY(early_puts)
 init_uart:
-puts:
 putn:   ret
 
 #endif /* !CONFIG_EARLY_PRINTK */