Re: [PATCH v2 05/40] xen/arm64: prepare for moving MMU related code from head.S

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 13/01/2023 05:28, Penny Zheng wrote:
> From: Wei Chen <wei.chen@xxxxxxx>
>
> We want to reuse head.S for MPU systems, but there are some
> code implemented for MMU systems only. We will move such
> code to another MMU specific file. But before that, we will
> do some preparations in this patch to make them easier
> for reviewing:
Well, I agree that...

> 1. Fix the indentations of code comments.
... changing the indentation is better here. But...

> 2. Export some symbols that will be accessed out of file
>     scope.
... I have no idea which functions are going to be used in a separate 
file. So I think they should belong to the patch moving the code.
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
Your signed-off-by is missing.

> ---
> v1 -> v2:
> 1. New patch.
> ---
>   xen/arch/arm/arm64/head.S | 40 +++++++++++++++++++--------------------
>   1 file changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 93f9b0b9d5..b2214bc5e3 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -136,22 +136,22 @@
>           add \xb, \xb, x20
>   .endm
>   
> -        .section .text.header, "ax", %progbits
> -        /*.aarch64*/
> +.section .text.header, "ax", %progbits
> +/*.aarch64*/
This change is not mentioned.

>   
> -        /*
> -         * 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.  All of text+data+bss must fit in 2MB,
> -         * or the initial pagetable code below will need adjustment.
> -         */
> +/*
> + * 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.  All of text+data+bss must fit in 2MB,
> + * or the initial pagetable code below will need adjustment.
> + */
>   
>   GLOBAL(start)
>           /*
> @@ -586,7 +586,7 @@ ENDPROC(cpu_init)
>    *
>    * Clobbers x0 - x4
>    */
> -create_page_tables:
> +ENTRY(create_page_tables)
I am not sure about keeping this name. Now we have create_page_tables() 
and arch_setup_page_tables().
I would conside to name it create_boot_page_tables().

>           /* Prepare the page-tables for mapping Xen */
>           ldr   x0, =XEN_VIRT_START
>           create_table_entry boot_pgtable, boot_first, x0, 0, x1, x2, x3
> @@ -680,7 +680,7 @@ ENDPROC(create_page_tables)
>    *
>    * Clobbers x0 - x3
>    */
> -enable_mmu:
> +ENTRY(enable_mmu)
>           PRINT("- Turning on paging -\r\n")
>   
>           /*
> @@ -714,7 +714,7 @@ ENDPROC(enable_mmu)
>    *
>    * Clobbers x0 - x1
>    */
> -remove_identity_mapping:
> +ENTRY(remove_identity_mapping)
Patch #14 should be before this patch. So you don't have to export 
remove_identity_mapping temporarily.
This will also avoid (transient) naming confusing with my work (see [1]).

>           /*
>            * Find the zeroeth slot used. Remove the entry from zeroeth
>            * table if the slot is not XEN_ZEROETH_SLOT.
> @@ -775,7 +775,7 @@ ENDPROC(remove_identity_mapping)
>    *
>    * Clobbers x0 - x3
>    */
> -setup_fixmap:
> +ENTRY(setup_fixmap)
>   #ifdef CONFIG_EARLY_PRINTK
>           /* Add UART to the fixmap table */
>           ldr   x0, =EARLY_UART_VIRTUAL_ADDRESS
> @@ -871,7 +871,7 @@ ENDPROC(init_uart)
>    * x0: Nul-terminated string to print.
>    * x23: Early UART base address
>    * Clobbers x0-x1 */
> -puts:
> +ENTRY(puts)
This name is a bit too generic to be globally exported. It is also now 
quite confusing because we have "early_puts" and "puts".
I would consider to name it asm_puts(). It is still not great but 
hopefully it would give a hint that should be call from assembly code.
>           early_uart_ready x23, 1
>           ldrb  w1, [x0], #1           /* Load next char */
>           cbz   w1, 1f                 /* Exit on nul */
Cheers,

[1] https://lore.kernel.org/all/20230113101136.479-13-julien@xxxxxxx/

--
Julien Grall