Re: [PATCH 2/2] xen/arm32: head: Improve logging in head.S

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 02/11/2023 00:30, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> The sequence to enable the MMU on arm32 is quite complex as we may need
> to jump to a temporary mapping to map Xen.
> 
> Recently, we had one bug in the logic (see f5a49eb7f8b3 ("xen/arm32:
> head: Add mising isb in switch_to_runtime_mapping()") and it was
> a pain to debug because there are no logging.
> 
> In order to improve the logging in the MMU switch we need to add
> support for early printk while running on the identity mapping
> and also on the temporary mapping.
> 
> For the identity mapping, we have only the first page of Xen mapped.
> So all the strings should reside in the first page. For that purpose
> a new macro PRINT_ID is introduced.
> 
> For the temporary mapping, the fixmap is already linked the temporary
> area (and so does the UART). So we just need to update the register
> storing the UART address (i.e. r11) to point to the UART temporary
> mapping.
> 
> Take the opportunity to introduce mov_w_on_cond in order to
> conditionally execute mov_w and avoid branches.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> ---
>  xen/arch/arm/arm32/head.S               | 68 ++++++++++++++++++++-----
>  xen/arch/arm/include/asm/asm_defns.h    |  6 ++-
>  xen/arch/arm/include/asm/early_printk.h |  3 ++
>  xen/arch/arm/include/asm/mmu/layout.h   |  4 ++
>  xen/arch/arm/mm.c                       |  5 ++
>  xen/arch/arm/xen.lds.S                  |  1 +
>  6 files changed, 71 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index fec2433e568f..bd61521a9dea 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -46,9 +46,13 @@
>   * Move an immediate constant into a 32-bit register using movw/movt
>   * instructions.
>   */
> +.macro mov_w_on_cond cond, reg, word
> +        movw\cond  \reg, #:lower16:\word
> +        movt\cond  \reg, #:upper16:\word
> +.endm
> +
>  .macro mov_w reg, word
> -        movw  \reg, #:lower16:\word
> -        movt  \reg, #:upper16:\word
> +        mov_w_on_cond al, \reg, \word
>  .endm
> 
>  /*
> @@ -104,16 +108,26 @@
>   */
>  #ifdef CONFIG_EARLY_PRINTK
>  /*
> - * Macro to print a string to the UART, if there is one.
> + * Macros to print a string to the UART, if there is one.
> + *
> + * There are multiple flavors:
> + *  - PRINT_SECT(section, string): The @string will be located in @section
> + *  - PRINT(): The string will be located in .rodata.str.
> + *  - PRINT_ID(): When Xen is running on the Identity Mapping, it is
> + *    only possible to have a limited amount of Xen. This will create
> + *    the string in .rodata.idmap which will always be mapped.
>   *
>   * Clobbers r0 - r3
>   */
> -#define PRINT(_s)           \
> -        mov   r3, lr       ;\
> -        adr_l r0, 98f      ;\
> -        bl    puts         ;\
> -        mov   lr, r3       ;\
> -        RODATA_STR(98, _s)
> +#define PRINT_SECT(section, string)         \
> +        mov   r3, lr                       ;\
> +        adr_l r0, 98f                      ;\
> +        bl    puts                         ;\
> +        mov   lr, r3                       ;\
> +        RODATA_SECT(section, 98, string)
> +
> +#define PRINT(string) PRINT_SECT(.rodata.str, string)
> +#define PRINT_ID(string) PRINT_SECT(.rodata.idmap, string)
> 
>  /*
>   * Macro to print the value of register \rb
> @@ -129,6 +143,7 @@
> 
>  #else /* CONFIG_EARLY_PRINTK */
>  #define PRINT(s)
> +#define PRINT_ID(s)
> 
>  .macro print_reg rb
>  .endm
> @@ -183,11 +198,6 @@ past_zImage:
>          bl    check_cpu_mode
>          bl    cpu_init
>          bl    create_page_tables
> -        /* Add the UART mapping if requested */
> -#ifdef CONFIG_EARLY_PRINTK
> -        mov_w r0, EARLY_UART_VIRTUAL_ADDRESS
> -        create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3
> -#endif
I think that patch no.1 should move the mapping into init_uart right away. This 
way
the issue I mentioned in that patch will be fixed and there will be no need to 
remove the code
introduced just before.

> 
>          /* Address in the runtime mapping to jump to after the MMU is 
> enabled */
>          mov_w lr, primary_switched
> @@ -593,6 +603,21 @@ enable_mmu:
>          mcr   CP32(r0, HSCTLR)       /* now paging is enabled */
>          isb                          /* Now, flush the icache */
> 
> +        /*
> +         * At this stage, the UART address will depend on whether the
> +         * temporary mapping was created or not.
> +         *
> +         * If it was, then the UART will be mapped in the temporary
> +         * area. Otherwise, it will be mapped at runtime virtual
> +         * mapping.
> +         */
> +#ifdef CONFIG_EARLY_PRINTK
> +        teq   r12, #1               /* Was the temporary mapping created? */
> +        mov_w_on_cond eq, r11, TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS
Shouldn't the clobber list be updated?

> +        mov_w_on_cond ne, r11, EARLY_UART_VIRTUAL_ADDRESS
> +#endif
> +        PRINT_ID("- Paging turned on -\r\n")
> +
>          /*
>           * The MMU is turned on and we are in the 1:1 mapping. Switch
>           * to the runtime mapping.
> @@ -643,12 +668,14 @@ switch_to_runtime_mapping:
>          teq   r12, #0
>          beq   ready_to_switch
> 
> +        PRINT_ID("- Switching to the temporary mapping -\r\n")
>          /* We are still in the 1:1 mapping. Jump to the temporary Virtual 
> address. */
>          mov_w r0, 1f
>          add   r0, r0, #XEN_TEMPORARY_OFFSET /* r0 := address in temporary 
> mapping */
>          mov   pc, r0
> 
>  1:
> +        PRINT("- Running on the temporary mapping  -\r\n")
>          /* Remove boot_second_id */
>          mov   r2, #0
>          mov   r3, #0
> @@ -659,6 +686,8 @@ switch_to_runtime_mapping:
> 
>          flush_xen_tlb_local r0
> 
> +        PRINT("- 1:1 mapping removed -\r\n")
> +
>          /* Map boot_second into boot_pgtable */
>          mov_w r0, XEN_VIRT_START
>          create_table_entry boot_pgtable, boot_second, r0, 1
> @@ -671,7 +700,10 @@ switch_to_runtime_mapping:
>           */
>          isb
> 
> +        PRINT("- Runtime mapping mapped -\r\n")
>  ready_to_switch:
> +        PRINT("- Jumping to runtime address -\r\n")
This is causing a boot hang in a configuration where there is no need for 
temporary mapping.
In that case, we are still in 1:1 map and should use PRINT_ID

> +
>          mov   pc, lr
>  ENDPROC(switch_to_runtime_mapping)
> 
> @@ -795,6 +827,14 @@ init_uart:
>          early_uart_init r11, r1, r2
>  #endif
>          PRINT("- UART enabled -\r\n")
> +
> +        /*
> +         * Add the UART mapping to the fixmap so the UART can be used
> +         * as the MMU is on.
> +         */
> +        mov_w r0, EARLY_UART_VIRTUAL_ADDRESS
> +        create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3
Shouldn't the clobber list be updated to include r4?

> +
>          mov   pc, lr
>  ENDPROC(init_uart)
> 
> diff --git a/xen/arch/arm/include/asm/asm_defns.h 
> b/xen/arch/arm/include/asm/asm_defns.h
> index 29a9dbb002fa..ec803c0a370c 100644
> --- a/xen/arch/arm/include/asm/asm_defns.h
> +++ b/xen/arch/arm/include/asm/asm_defns.h
> @@ -22,11 +22,13 @@
>  # error "unknown ARM variant"
>  #endif
> 
> -#define RODATA_STR(label, msg)                  \
> -.pushsection .rodata.str, "aMS", %progbits, 1 ; \
> +#define RODATA_SECT(section, label, msg)         \
> +.pushsection section, "aMS", %progbits, 1 ;     \
>  label:  .asciz msg;                             \
>  .popsection
> 
> +#define RODATA_STR(label, msg) RODATA_SECT(.rodata.str, label, msg)
> +
>  #define ASM_INT(label, val)                 \
>      .p2align 2;                             \
>  label: .long (val);                         \
> diff --git a/xen/arch/arm/include/asm/early_printk.h 
> b/xen/arch/arm/include/asm/early_printk.h
> index c5149b2976da..c1e84f8b0009 100644
> --- a/xen/arch/arm/include/asm/early_printk.h
> +++ b/xen/arch/arm/include/asm/early_printk.h
> @@ -19,6 +19,9 @@
>  #define EARLY_UART_VIRTUAL_ADDRESS \
>      (FIXMAP_ADDR(FIXMAP_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS & 
> ~PAGE_MASK))
> 
> +#define TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS \
> +    (TEMPORARY_FIXMAP_ADDR(FIXMAP_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS 
> & ~PAGE_MASK))
> +
>  #endif /* !CONFIG_EARLY_PRINTK */
> 
>  #endif
> diff --git a/xen/arch/arm/include/asm/mmu/layout.h 
> b/xen/arch/arm/include/asm/mmu/layout.h
> index da6be276ac5f..68855c4f3184 100644
> --- a/xen/arch/arm/include/asm/mmu/layout.h
> +++ b/xen/arch/arm/include/asm/mmu/layout.h
> @@ -113,6 +113,10 @@
>        (TEMPORARY_AREA_FIRST_SLOT << XEN_PT_LEVEL_SHIFT(1)))
> 
>  #define TEMPORARY_XEN_VIRT_START    TEMPORARY_AREA_ADDR(XEN_VIRT_START)
> +#define TEMPORARY_FIXMAP_VIRT_START TEMPORARY_AREA_ADDR(FIXMAP_VIRT_START)
> +
> +#define TEMPORARY_FIXMAP_ADDR(n)                    \
> +     (TEMPORARY_FIXMAP_VIRT_START + (n) * PAGE_SIZE)
> 
>  #else /* ARM_64 */
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index c34cc94c907a..6ffa45c53be6 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -193,7 +193,12 @@ static void __init __maybe_unused build_assertions(void)
>  #ifdef CONFIG_ARM_32
>      CHECK_SAME_SLOT(first, TEMPORARY_XEN_VIRT_START, DOMHEAP_VIRT_START);
>      CHECK_DIFFERENT_SLOT(first, XEN_VIRT_START, TEMPORARY_XEN_VIRT_START);
> +    CHECK_SAME_SLOT(first, TEMPORARY_XEN_VIRT_START,
> +                    TEMPORARY_FIXMAP_VIRT_START);
> +    CHECK_SAME_SLOT(first, TEMPORARY_XEN_VIRT_START,
> +                    TEMPORARY_FIXMAP_VIRT_START);
duplicated entry?

~Michal