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

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 01/08/2023 04:44, Henry Wang wrote:
> From: Wei Chen <wei.chen@xxxxxxx>
>
> 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@xxxxxxx>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
With one remark below:

Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

> ---
> 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,

--
Julien Grall