Re: [PATCH v3 07/52] xen/arm64: prepare for moving MMU related code from head.S

[Penny Zheng <penny.zheng@xxxxxxx>]

Hi Julien

On 2023/7/5 05:35, Julien Grall wrote:
> Hi Penny,
>
> On 26/06/2023 04:33, Penny Zheng 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 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>
> > ---
> > v1 -> v2:
> > 1. New patch.
> > ---
> > v3:
> > 1. fix commit message
> > 2. Rename puts() to asm_puts() for global export
> > ---
> >   xen/arch/arm/arm64/head.S | 42 +++++++++++++++++++--------------------
> >   1 file changed, 21 insertions(+), 21 deletions(-)
> >
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index 4dfbe0bc6f..66347eedcc 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   x0, 98f ;    \
> This was recently changed to adr_l in staging. So you will need to 
> rebase this patch.
Sure, I'll rebase.

> > -        bl    puts    ;    \
> > +        bl    asm_puts ;   \
> >           mov   lr, x3 ;     \
> >           RODATA_STR(98, _s)
> > @@ -136,22 +136,22 @@
> >           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.  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)
> >           /*
> > @@ -520,7 +520,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
> > @@ -928,15 +928,15 @@ ENDPROC(init_uart)
> >    * x0: Nul-terminated string to print.
> >    * x23: Early UART base address
> >    * Clobbers x0-x1 */
> > -puts:
> > +ENTRY(asm_puts)
> Can you mention in the comment that this function is only supposed to be 
> called from assembly?
> While you are at it, can you update the comment coding style to:
>
> /*
>   * Foo
>   * Bar
>   */
Sure,

> >           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.
> > @@ -966,7 +966,7 @@ hex:    .ascii "0123456789abcdef"
> >   ENTRY(early_puts)
> >   init_uart:
> > -puts:
> > +asm_puts:
> I find odd that you add ENTRY() to the asm_puts() above but not here. 
> However... I can't find any use of asm_puts() outside #ifdef 
> CONFIG_EARLY_PRINTK. So maybe it can be dropped?
True. It is only used under #ifdef CONFIG_EARLY_PRINTK.
I'll drop.

> >   putn:   ret
> >   #endif /* !CONFIG_EARLY_PRINTK */
> Cheers,