Re: [PATCH v6 08/11] xen/arm64: move MMU related code from head.S to head_mmu.S

[Julien Grall <julien@xxxxxxx>]

Hi Wei,

On 04/11/2022 10:07, Wei Chen wrote:
> There are lots of MMU specific code in head.S. This code will not
> be used in MPU systems. If we use #ifdef to gate them, the code
> will become messy and hard to maintain. So we move MMU related
> code to head_mmu.S, and keep common code still in head.S.
I am afraid that you can't simply move the MMU code out of head.S 
because this will break Xen when running using the identity map.
This is because we only map the first 4KB of Xen with PA == VA. At the 
moment, we guarantee it by having everything that needs to be used in 
the identity mapping before _end_boot and checking at link time if this 
fits in 4KB.
Now that you moved the MMU code outside of head.S. We need to find a 
different way to guarantee it. One way to do it would be to create a 
section that would be used for everything that needs to be identity mapped.
> As we need to access "fail" and "puts" functions out of assembly
> file, so we have to export them in this patch. And the assembly
> macros: adr_l and load_paddr will be used by MMU and MPU later,
> so we move them to macros.h.
>
> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
In general, the first signed-off should match the author. So who is who 
here?
> ---
>   xen/arch/arm/arm64/Makefile             |   3 +
>   xen/arch/arm/arm64/head.S               | 407 +-----------------------
>   xen/arch/arm/arm64/head_mmu.S           | 364 +++++++++++++++++++++
>   xen/arch/arm/include/asm/arm64/macros.h |  52 ++-
>   4 files changed, 432 insertions(+), 394 deletions(-)
>   create mode 100644 xen/arch/arm/arm64/head_mmu.S
>
> diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
> index 6d507da0d4..22da2f54b5 100644
> --- a/xen/arch/arm/arm64/Makefile
> +++ b/xen/arch/arm/arm64/Makefile
> @@ -8,6 +8,9 @@ obj-y += domctl.o
>   obj-y += domain.o
>   obj-y += entry.o
>   obj-y += head.o
> +ifneq ($(CONFIG_HAS_MPU),y) > +obj-y += head_mmu.o
> +endif
>   obj-y += insn.o
>   obj-$(CONFIG_LIVEPATCH) += livepatch.o
>   obj-y += smc.o
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index ccedf20dc7..d9a8da9120 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -25,17 +25,6 @@
>   #include <efi/efierr.h>
>   #include <asm/arm64/efibind.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 */
> -
> -/* Convenience defines to get slot used by Xen mapping. */
> -#define XEN_ZEROETH_SLOT    zeroeth_table_offset(XEN_VIRT_START)
> -#define XEN_FIRST_SLOT      first_table_offset(XEN_VIRT_START)
> -#define XEN_SECOND_SLOT     second_table_offset(XEN_VIRT_START)
> -
>   #define __HEAD_FLAG_PAGE_SIZE   ((PAGE_SHIFT - 10) / 2)
>   
>   #define __HEAD_FLAG_PHYS_BASE   1
> @@ -82,73 +71,22 @@
>    *  x30 - lr
>    */
>   
> -#ifdef CONFIG_EARLY_PRINTK
> -/*
> - * Macro to print a string to the UART, if there is one.
> - *
> - * Clobbers x0 - x3
> - */
> -#define PRINT(_s)          \
> -        mov   x3, lr ;     \
> -        adr   x0, 98f ;    \
> -        bl    puts    ;    \
> -        mov   lr, x3 ;     \
> -        RODATA_STR(98, _s)
> +.section .text.header, "ax", %progbits
> +/*.aarch64*/
The patch is already quite difficult to read. So I would rather prefer 
if the indentation is changed separately.
Furthermore, I think it would be best if the functions moved in the 
header are done separately to help checking (I would be able to diff the 
source with the destination more easily).
>   
>   /*
> - * Macro to print the value of register \xb
> + * Kernel startup entry point.
> + * ---------------------------
Same here about the indentation. I will not comment everywhere where the 
indentation was changed. So please look at it.
[...]

> -/*
> - * Map the UART in the fixmap (when earlyprintk is used) and hook the
> - * fixmap table in the page tables.
> - *
> - * The fixmap cannot be mapped in create_page_tables because it may
> - * clash with the 1:1 mapping.
> - *
> - * Inputs:
> - *   x20: Physical offset
> - *   x23: Early UART base physical address
> - *
> - * Clobbers x0 - x3
> - */
> -setup_fixmap:
> -#ifdef CONFIG_EARLY_PRINTK
> -        /* Add UART to the fixmap table */
> -        ldr   x0, =EARLY_UART_VIRTUAL_ADDRESS
> -        create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, type=PT_DEV_L3
> -#endif
> -        /* Map fixmap into boot_second */
> -        ldr   x0, =FIXMAP_ADDR(0)
> -        create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3
> -        /* Ensure any page table updates made above have occurred. */
> -        dsb   nshst
> -
> -        ret
> -ENDPROC(setup_fixmap)
> -
>   /*
>    * Setup the initial stack and jump to the C world
>    *
> @@ -810,41 +458,14 @@ launch:
>   ENDPROC(launch)
>   
>   /* Fail-stop */
> -fail:   PRINT("- Boot failed -\r\n")
> +ENTRY(fail)
This name is a bit too generic to be exposed. But it would be better to 
duplicate it.
Cheers,

--
Julien Grall