Hi Julien,
On 16/01/2024 15:37, Julien Grall wrote:
>
>
> From: Julien Grall <jgr...@amazon.com>
>
> 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 <jgr...@amazon.com>
Reviewed-by: Michal Orzel <michal.or...@amd.com>
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
