Re: [PATCH v1 1/2] xen/mpu: Map early uart when earlyprintk on

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 08/11/2024 20:00, Ayan Kumar Halder wrote:
> CONFIG_EARLY_UART_SIZE is introduced to let user provide physical size of
> early UART. Unlike MMU where we map a page in the virtual address space,
> here we need to know the exact physical size to be mapped.
> As VA == PA in case of MPU, the memory layout follows exactly the hardware
> configuration. As a consequence, we set  EARLY_UART_VIRTUAL_ADDRESS as physical
> address.
>
> Further, we check whether user-defined EARLY_UART_SIZE is aligned to PAGE_SIZE
> (4KB). This is partly because we intend to map a minimum of 1 page(ie 4KB) and
> the limit address is set as "EARLY_UART_SIZE-1". The limit address needs to end
> with 0x3f (as required by PRLAR register).
Looking at the code below, it is not clear why you need to have 
UART_SIZE 4KB-aligned. Furthermore...
> UART is mapped as nGnRE region (as specified by ATTR=100 , refer G1.3.13,
> MAIR_EL2, "---0100 Device memory nGnRE") and Doc ID - 102670_0101_02_en
> Table 4-3, Armv8 architecture memory types (nGnRE - Corresponds to Device in
> Armv7 architecture). Also, it is mapped as outer shareable, RW at EL2 only
> and execution of instructions from the region is not permitted.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
>   xen/arch/arm/Kconfig.debug              |  7 +++++++
>   xen/arch/arm/arm64/mpu/head.S           |  9 +++++++++
>   xen/arch/arm/include/asm/early_printk.h | 19 +++++++++++++++++++
>   3 files changed, 35 insertions(+)
>
> diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
> index 7660e599c0..84a0616102 100644
> --- a/xen/arch/arm/Kconfig.debug
> +++ b/xen/arch/arm/Kconfig.debug
> @@ -121,6 +121,13 @@ config EARLY_UART_BASE_ADDRESS
>         hex "Early printk, physical base address of debug UART"
>         range 0x0 0xffffffff if ARM_32
>   
> +config EARLY_UART_SIZE
> +       depends on EARLY_PRINTK
> +       depends on MPU
> +       hex "Early printk, physical size of debug UART"
> +       range 0x0 0xffffffff if ARM_32
> +       default 0x1000
> +
>   config EARLY_UART_PL011_BAUD_RATE
>         depends on EARLY_UART_PL011
>         int "Early printk UART baud rate for pl011"
> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
> index 731698aa3b..98422d7ed3 100644
> --- a/xen/arch/arm/arm64/mpu/head.S
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -11,8 +11,10 @@
>   #define REGION_TEXT_PRBAR       0x38    /* SH=11 AP=10 XN=00 */
>   #define REGION_RO_PRBAR         0x3A    /* SH=11 AP=10 XN=10 */
>   #define REGION_DATA_PRBAR       0x32    /* SH=11 AP=00 XN=10 */
> +#define REGION_DEVICE_PRBAR     0x22    /* SH=10 AP=00 XN=10 */
>   
>   #define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
> +#define REGION_DEVICE_PRLAR     0x09    /* NS=0 ATTR=100 EN=1 */
>   
>   /*
>    * Macro to prepare and set a EL2 MPU memory region.
> @@ -137,6 +139,13 @@ FUNC(enable_boot_cpu_mm)
>       ldr   x2, =__bss_end
>       prepare_xen_region x0, x1, x2, x3, x4, x5
>   
> +#ifdef CONFIG_EARLY_PRINTK
> +    /* Xen early UART section. */
> +    ldr   x1, =CONFIG_EARLY_UART_BASE_ADDRESS
> +    ldr   x2, =(CONFIG_EARLY_UART_BASE_ADDRESS + CONFIG_EARLY_UART_SIZE)
> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_DEVICE_PRBAR, 
> attr_prlar=REGION_DEVICE_PRLAR

... the documentation of prepare_xen_region suggests that the base UART 
should be page-aligned. However, there are systems (although not sure 
about MPU ones) where the base UART address is not page-aligned (this is 
because multiple UARTs share the same page).
But I don't see any need to require page alignment here. I think the 
alignment/size should be based on the minumum supported by the MPU. This 
means the documentation in prepare_xen_region likely needs to be updated.

> +#endif
> +
>       b    enable_mpu
>       ret
>   END(enable_boot_cpu_mm)
> diff --git a/xen/arch/arm/include/asm/early_printk.h 
> b/xen/arch/arm/include/asm/early_printk.h
> index 46a5e562dd..98fd52c4db 100644
> --- a/xen/arch/arm/include/asm/early_printk.h
> +++ b/xen/arch/arm/include/asm/early_printk.h
> @@ -15,6 +15,24 @@
>   
>   #ifdef CONFIG_EARLY_PRINTK
>   
> +#ifndef CONFIG_MMU
Shouldn't this be #ifdef CONFIG_MPU followed by...

> +
> +/*
> + * For MPU systems, there is no VMSA support in EL2, so we use VA == PA
> + * for EARLY_UART_VIRTUAL_ADDRESS.
> + */
> +#define EARLY_UART_VIRTUAL_ADDRESS CONFIG_EARLY_UART_BASE_ADDRESS
> +
> +/*
> + * User-defined EARLY_UART_SIZE must be aligned to a PAGE_SIZE, or
> + * we may map more than necessary in MPU system.
> + */
> +#if (EARLY_UART_SIZE % PAGE_SIZE) != 0
> +#error "EARLY_UART_SIZE must be aligned to PAGE_SIZE"
> +#endif
> +
> +#else
#elif CONFIG_MMU

> +
>   /* need to add the uart address offset in page to the fixmap address */
>   #define EARLY_UART_VIRTUAL_ADDRESS \
>       (FIXMAP_ADDR(FIX_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
> @@ -22,6 +40,7 @@
>   #define TEMPORARY_EARLY_UART_VIRTUAL_ADDRESS \
>       (TEMPORARY_FIXMAP_ADDR(FIX_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS & 
> ~PAGE_MASK))
>   
> +#endif /* CONFIG_MMU */
And finally

#else
# error ...
#endif

>   #endif /* !CONFIG_EARLY_PRINTK */
>   >   #endif

--
Julien Grall