Re: [PATCH v3 2/2] xen/arm32: head Split and move MMU-specific head.S to mmu/head.S

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 07/11/2023 12:02, Ayan Kumar Halder wrote:
> 
> 
> The MMU specific code in head.S will not be used on MPU systems.
> Instead of introducing more #ifdefs which will bring complexity
> to the code, move MMU related code to mmu/head.S and keep common
> code in head.S. Two notes while moving:
>  - As "fail" in original head.S is very simple and this name is too
>    easy to be conflicted, duplicate it in mmu/head.S instead of
>    exporting it.
>  - Realigned ".macro ret" so that the alignment matches to the other
>    macros.
>  - Rename puts to asm_puts, putn to asm_putn (this denotes that the
>    macros are used within the context of assembly only).
>  - Use ENTRY() for enable_secondary_cpu_mm, enable_boot_cpu_mm,
>    setup_fixmap, asm_puts, asm_putn  as they will be used externally.
> 
> Also move the assembly macros shared by head.S and mmu/head.S to
> macros.h.
> 
> This is based on 6734327d76be ("xen/arm64: Split and move MMU-specific head.S 
> to mmu/head.S").
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
For now, I won't provide Rb given that the baseline is still under development 
and not very clear to me.

Just a few remarks:

[...]

> -
>  #ifdef CONFIG_EARLY_PRINTK
>  /*
>   * Initialize the UART. Should only be called on the boot CPU.
> @@ -912,14 +298,14 @@ ENDPROC(init_uart)
>   * r11: Early UART base address
You could follow the arm64 patch and add:
"Note: This function must be called from assembly."
to make the usage of this function clear.
Same for asm_putn.

>   * Clobbers r0-r1
>   */
> -puts:
> +ENTRY(asm_puts)
>          early_uart_ready r11, r1
>          ldrb  r1, [r0], #1           /* Load next char */
>          teq   r1, #0                 /* Exit on nul */
>          moveq pc, lr
>          early_uart_transmit r11, r1
> -        b puts
> -ENDPROC(puts)
> +        b asm_puts
> +ENDPROC(asm_puts)
> 
>  /*
>   * Print a 32-bit number in hex.  Specific to the PL011 UART.
The second sentence can be dropped. I don't see anything PL011 specific here.

> @@ -927,7 +313,7 @@ ENDPROC(puts)
>   * r11: Early UART base address
>   * Clobbers r0-r3
>   */
> -putn:
> +ENTRY(asm_putn)
>          adr_l r1, hex
>          mov   r3, #8
>  1:
> @@ -939,7 +325,7 @@ putn:
>          subs  r3, r3, #1
>          bne   1b
>          mov   pc, lr
> -ENDPROC(putn)
> +ENDPROC(asm_putn)
> 
>  RODATA_STR(hex, "0123456789abcdef")
> 
> @@ -947,8 +333,8 @@ RODATA_STR(hex, "0123456789abcdef")
> 
>  ENTRY(early_puts)
>  init_uart:
> -puts:
> -putn:   mov   pc, lr
> +asm_puts:
> +asm_putn:   mov   pc, lr
Both asm_puts and asm_putn are used only within #ifdef so you can drop the stubs

[...]

> diff --git a/xen/arch/arm/include/asm/arm32/macros.h 
> b/xen/arch/arm/include/asm/arm32/macros.h
> index a4e20aa520..c6e390cc5f 100644
> --- a/xen/arch/arm/include/asm/arm32/macros.h
> +++ b/xen/arch/arm/include/asm/arm32/macros.h
> @@ -1,8 +1,62 @@
>  #ifndef __ASM_ARM_ARM32_MACROS_H
>  #define __ASM_ARM_ARM32_MACROS_H
> 
> -    .macro ret
> +.macro ret
>          mov     pc, lr
> -    .endm
> +.endm
> 
> +/*
> + * Move an immediate constant into a 32-bit register using movw/movt
> + * instructions.
> + */
> +.macro mov_w reg, word
> +        movw  \reg, #:lower16:\word
> +        movt  \reg, #:upper16:\word
> +.endm
> +
> +/*
> + * Pseudo-op for PC relative adr <reg>, <symbol> where <symbol> is
> + * within the range +/- 4GB of the PC.
> + *
> + * @dst: destination register
> + * @sym: name of the symbol
> + */
> +.macro adr_l, dst, sym
> +        mov_w \dst, \sym - .Lpc\@
> +        .set  .Lpc\@, .+ 8          /* PC bias */
> +        add   \dst, \dst, pc
> +.endm
> +
> +#ifdef CONFIG_EARLY_PRINTK
> +/*
> + * Macro to print a string to the UART, if there is one.
> + *
> + * Clobbers r0 - r3
> + */
> +#define PRINT(_s)           \
> +        mov   r3, lr       ;\
> +        adr_l r0, 98f      ;\
> +        bl    puts         ;\
This should be a call to asm_puts

~Michal