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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 17/11/2023 16:31, 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>
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

with a few remarks...

> ---
> 
> Changes from v1 :-
> 
> 1. Added a commit message
> 2. Moved load_paddr to mmu/head.S
> 
> v2 :-
> 
> 1. Renamed puts to asm_puts and putn to asm_putn. Exported asm_putn().
> 2. Moved XEN_TEMPORARY_OFFSET to head.S.
> 3. Some style issues.
> 
> v3 :-
> 
> 1. Updated the comments on top of asm_puts() and asm_putn().
> 2. Removed some stubs.
> 3. PRINT() invokes asm_puts.
> 
>  xen/arch/arm/arm32/head.S               | 630 +-----------------------
>  xen/arch/arm/arm32/mmu/Makefile         |   1 +
>  xen/arch/arm/arm32/mmu/head.S           | 573 +++++++++++++++++++++
>  xen/arch/arm/include/asm/arm32/macros.h |  58 ++-
>  4 files changed, 638 insertions(+), 624 deletions(-)
>  create mode 100644 xen/arch/arm/arm32/mmu/head.S
> 
[...]

>  
> @@ -947,8 +335,6 @@ RODATA_STR(hex, "0123456789abcdef")
>  
>  ENTRY(early_puts)
>  init_uart:
> -puts:
> -putn:   mov   pc, lr
You removed putn, puts and even the return. Looking at the codebase, early_puts 
global makes no sense
and init_uart is only used within #ifdef. So I would expect the entire block to 
be removed.

>  
>  #endif /* !CONFIG_EARLY_PRINTK */
>  
> diff --git a/xen/arch/arm/arm32/mmu/Makefile b/xen/arch/arm/arm32/mmu/Makefile
> index b18cec4836..a8a750a3d0 100644
> --- a/xen/arch/arm/arm32/mmu/Makefile
> +++ b/xen/arch/arm/arm32/mmu/Makefile
> @@ -1 +1,2 @@
> +obj-y += head.o
>  obj-y += mm.o
> diff --git a/xen/arch/arm/arm32/mmu/head.S b/xen/arch/arm/arm32/mmu/head.S
> new file mode 100644
> index 0000000000..6d427328f3
> --- /dev/null
> +++ b/xen/arch/arm/arm32/mmu/head.S
> @@ -0,0 +1,573 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/arm32/mmu/head.S
> + *
> + * Arm32 MMU specific start-of-day code.
> + */
> +
> +#include <asm/page.h>
> +#include <asm/early_printk.h>
> +
> +#define PT_PT     0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
> +#define PT_MEM    0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0 P=1 */
> +#define PT_MEM_L3 0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
> +#define PT_DEV    0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1 */
> +#define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1 */
> +
> +#define PT_UPPER(x) (PT_##x & 0xf00)
> +#define PT_LOWER(x) (PT_##x & 0x0ff)
> +
> +/* Convenience defines to get slot used by Xen mapping. */
> +#define XEN_FIRST_SLOT      first_table_offset(XEN_VIRT_START)
> +#define XEN_SECOND_SLOT     second_table_offset(XEN_VIRT_START)
> +
> +/* Offset between the early boot xen mapping and the runtime xen mapping */
> +#define XEN_TEMPORARY_OFFSET      (TEMPORARY_XEN_VIRT_START - XEN_VIRT_START)
> +
> +.macro load_paddr rb, sym
> +        mov_w \rb, \sym
> +        add   \rb, \rb, r10
> +.endm
> +
> +/*
> + * Flush local TLBs
> + *
> + * @tmp: Scratch register
> + *
> + * See asm/arm32/flushtlb.h for the explanation of the sequence.
> + */
> +.macro flush_xen_tlb_local tmp
> +        dsb   nshst
> +        mcr   CP32(\tmp, TLBIALLH)
> +        dsb   nsh
> +        isb
> +.endm
> +
> +/*
> + * Enforce Xen page-tables do not contain mapping that are both
> + * Writable and eXecutables.
> + *
> + * This should be called on each secondary CPU.
> + */
> +.macro pt_enforce_wxn tmp
> +        mrc   CP32(\tmp, HSCTLR)
> +        orr   \tmp, \tmp, #SCTLR_Axx_ELx_WXN
> +        dsb
> +        mcr   CP32(\tmp, HSCTLR)
> +        /*
> +         * The TLBs may cache SCTLR_EL2.WXN. So ensure it is synchronized
> +         * before flushing the TLBs.
> +         */
> +        isb
> +        flush_xen_tlb_local \tmp
> +.endm
> +
> +/* Macro to find the slot number at a given page-table level
> + *
> + * slot:     slot computed
> + * virt:     virtual address
> + * lvl:      page-table level
> + *
> + * Note that ubxf is unpredictable when the end bit is above 32-bit. So we
> + * can't use it for first level offset.
> + */
> +.macro get_table_slot, slot, virt, lvl
> +    .if \lvl == 1
> +        lsr   \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl)
> +    .else
> +        ubfx  \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl), #XEN_PT_LPAE_SHIFT
> +    .endif
> +.endm
> +
> +/*
> + * Macro to create a page table entry in \ptbl to \tbl (physical
> + * address)
> + *
> + * ptbl:    table symbol where the entry will be created
> + * tbl:     physical address of the table to point to
> + * virt:    virtual address
> + * lvl:     page-table level
> + *
> + * Preserves \virt
> + * Clobbers \tbl, r1 - r3
> + *
> + * Note that \tbl and \virt should be in a register other than r1 - r3
> + */
> +.macro create_table_entry_from_paddr, ptbl, tbl, virt, lvl
> +        get_table_slot r1, \virt, \lvl  /* r1 := slot in \tbl */
> +        lsl   r1, r1, #3                /* r1 := slot offset in \tbl */
> +
> +        movw  r2, #PT_PT             /* r2:r3 := right for linear PT */
> +        orr   r2, r2, \tbl           /*           + \tbl paddr */
> +        mov   r3, #0
> +
> +        adr_l \tbl, \ptbl            /* \tbl := (v,p)addr of \ptbl */
> +
> +        strd  r2, r3, [\tbl, r1]
> +.endm
> +
> +
NIT: too many blank lines

[...]
> diff --git a/xen/arch/arm/include/asm/arm32/macros.h 
> b/xen/arch/arm/include/asm/arm32/macros.h
> index a4e20aa520..c41861efbe 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    asm_puts     ;\
> +        mov   lr, r3       ;\
> +        RODATA_STR(98, _s)
> +
> +/*
> + * Macro to print the value of register \rb
> + *
> + * Clobbers r0 - r4
> + */
> +.macro print_reg rb
> +        mov   r0, \rb
> +        mov   r4, lr
> +        bl    asm_putn
> +        mov   lr, r4
> +.endm
> +
> +#else /* CONFIG_EARLY_PRINTK */
> +#define PRINT(s)
> +
> +.macro print_reg rb
> +.endm
> +
> +#endif /* !CONFIG_EARLY_PRINTK */
please add one blank line here to separate #endif's

>  #endif /* __ASM_ARM_ARM32_MACROS_H */

~Michal