Re: [PATCH v1 repost 1/4] arm/mmu: Move init_ttbr to a new section .data.idmap

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 16/01/2024 15:37, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> With the upcoming work to color Xen, the binary will not be anymore
> physically contiguous. This will be a problem during boot as the
> assembly code will need to work out where each piece of Xen reside.
> 
> An easy way to solve the issue is to have all code/data accessed
> by the secondary CPUs while the MMU is off within a single page.
> 
> Right now, init_ttbr is used by secondary CPUs to find there page-tables
> before the MMU is on. Yet it is currently in .data which is unlikely
> to be within the same page as the rest of the idmap.
> 
> Create a new section .data.idmap that will be used for variables
> accessed by the early boot code. The first one is init_ttbr.
> 
> The idmap is currently part of the text section and therefore will
> be mapped read-only executable. This means that we need to temporarily
> remap init_ttbr in order to update it.
> 
> Introduce a new function set_init_ttbr() for this purpose so the code
> is not duplicated between arm64 and arm32.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

with ...

> ---
>  xen/arch/arm/mmu/smpboot.c | 34 +++++++++++++++++++++++++++++-----
>  xen/arch/arm/xen.lds.S     |  1 +
>  2 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
> index b6fc0aae07f1..f1cf9252710c 100644
> --- a/xen/arch/arm/mmu/smpboot.c
> +++ b/xen/arch/arm/mmu/smpboot.c
> @@ -9,6 +9,10 @@
> 
>  #include <asm/setup.h>
> 
> +/* Override macros from asm/page.h to make them work with mfn_t */
> +#undef virt_to_mfn
> +#define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
> +
>  /*
>   * Static start-of-day pagetables that we use before the allocators
>   * are up. These are used by all CPUs during bringup before switching
> @@ -44,7 +48,7 @@ DEFINE_BOOT_PAGE_TABLE(boot_second);
>  DEFINE_BOOT_PAGE_TABLES(boot_third, XEN_NR_ENTRIES(2));
> 
>  /* Non-boot CPUs use this to find the correct pagetables. */
> -uint64_t init_ttbr;
> +uint64_t __section(".data.idmap") init_ttbr;
Do we need to keep the declaration in mmu/mm.h? This variable is only used in 
this file
and in assembly, so maybe better to drop declaration and use asmlinkage instead?

> 
>  /* Clear a translation table and clean & invalidate the cache */
>  static void clear_table(void *table)
> @@ -68,6 +72,27 @@ static void clear_boot_pagetables(void)
>      clear_table(boot_third);
>  }
> 
> +static void set_init_ttbr(lpae_t *root)
> +{
> +    /*
> +     * init_ttbr is part of the identity mapping which is read-only. So
> +     * We need to re-map the region so it can be updated
Would you mind fixing s/So We/So we/ and add a full stop after last sentence?

> +     */
> +    void *ptr = map_domain_page(virt_to_mfn(&init_ttbr));
> +
> +    ptr += PAGE_OFFSET(&init_ttbr);
> +
> +    *(uint64_t *)ptr = virt_to_maddr(root);
> +
> +    /*
> +     * init_ttbr will be accessed with the MMU off, so ensure the update
> +     * is visible by cleaning the cache.
> +     */
> +    clean_dcache(ptr);
> +
> +    unmap_domain_page(ptr);
> +}
> +
>  #ifdef CONFIG_ARM_64
>  int prepare_secondary_mm(int cpu)
>  {
> @@ -77,8 +102,8 @@ int prepare_secondary_mm(int cpu)
>       * Set init_ttbr for this CPU coming up. All CPUs share a single setof
>       * pagetables, but rewrite it each time for consistency with 32 bit.
>       */
> -    init_ttbr = virt_to_maddr(xen_pgtable);
> -    clean_dcache(init_ttbr);
> +    set_init_ttbr(xen_pgtable);
> +
>      return 0;
>  }
>  #else
> @@ -109,8 +134,7 @@ int prepare_secondary_mm(int cpu)
>      clear_boot_pagetables();
> 
>      /* Set init_ttbr for this CPU coming up */
> -    init_ttbr = __pa(first);
> -    clean_dcache(init_ttbr);
> +    set_init_ttbr(first);
> 
>      return 0;
>  }
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 20598c6963ce..470c8f22084f 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -36,6 +36,7 @@ SECTIONS
>         *(.text.header)
>         *(.text.idmap)
>         *(.rodata.idmap)
> +       *(.data.idmap)
>         _idmap_end = .;
> 
>         *(.text.cold)
> --
> 2.40.1
> 

~Michal