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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2023年1月18日 7:37
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v2 05/40] xen/arm64: prepare for moving MMU related
> code from head.S
> 
> 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.
> 

Ok, I will move these changes to the moving code patches.

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

I will add the description in commit message.

> >
> > -        /*
> > -         * 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().
> 

Do you need me to rename it in this patch?

> >           /* 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]).
> 

Ok, we will do it.

> >           /*
> >            * 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.
> 

Yes, I had the same concern. I will rename it in next version.

Cheers,
Wei Chen

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