Re: [PATCH v4 06/13] xen/arm64: Move setup_fixmap() to create_page_tables()

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

Title: NIT: It is more a fold rather than move.

On 01/08/2023 04:44, Henry Wang wrote:
>  For the
> future MPU support work, the early UART mapping could then be moved
> in prepare_early_mappings().
I would drop this sentence as this is more related to the future 
implementation of MPU rather than this patch itself.
> No functional change intended.
>
> [1] 
> https://lore.kernel.org/xen-devel/78862bb8-fd7f-5a51-a7ae-3c5b5998ed80@xxxxxxx/
>
> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> ---
> v4:
> - Rework "[v3,12/52] xen/mmu: extract early uart mapping from setup_fixmap"
> ---
>   xen/arch/arm/arm64/head.S     |  1 -
>   xen/arch/arm/arm64/mmu/head.S | 50 ++++++++++++-----------------------
>   2 files changed, 17 insertions(+), 34 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index e4f579a48e..56f68a8e37 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -275,7 +275,6 @@ real_start_efi:
>           b     enable_boot_cpu_mm
>   
>   primary_switched:
> -        bl    setup_fixmap
>   #ifdef CONFIG_EARLY_PRINTK
>           /* Use a virtual address to access the UART. */
>           ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
> diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S
> index b7c3dd423a..6bd94c3a45 100644
> --- a/xen/arch/arm/arm64/mmu/head.S
> +++ b/xen/arch/arm/arm64/mmu/head.S
The second paragraph in create_page_tables() now needs to be dropped.

> @@ -231,6 +231,23 @@ link_from_second_id:
>           create_table_entry boot_second_id, boot_third_id, x19, 2, x0, x1, x2
>   link_from_third_id:
>           create_mapping_entry boot_third_id, x19, x19, x0, x1, x2
> +
> +#ifdef CONFIG_EARLY_PRINTK
> +        /* Add UART to the fixmap table */
> +        ldr   x0, =EARLY_UART_VIRTUAL_ADDRESS
> +        /* x23: Early UART base physical 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
> +        /*
> +         * The fixmap area will be used soon after. So ensure no hardware
> +         * translation happens before the dsb completes.
> +         */
> +        isb
The DSB/ISB is not necessary anymore as you are not modifying live 
page-tables. So I would prefer if this is removed.
Cheers,

--
Julien Grall