| Message ID | 20240116143709.86584-2-julien@xen.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm64: Rework the MMU-off code (idmap) so it is self-contained | expand |
Hi Julien, On 16/01/2024 15:37, Julien Grall wrote: > > > From: Julien Grall <jgrall@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 <jgrall@amazon.com> Reviewed-by: Michal Orzel <michal.orzel@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
On 17/01/2024 08:30, Michal Orzel wrote: > Hi Julien, Hi Michal, > On 16/01/2024 15:37, Julien Grall wrote: >> >> >> From: Julien Grall <jgrall@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 <jgrall@amazon.com> > Reviewed-by: Michal Orzel <michal.orzel@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? I don't see the problem of keeping the declaration in mmu/mm.h. In any case, this seems to be unrelated to this patch. > >> >> /* 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? I can do that. Cheers,
On 17/01/2024 13:10, Julien Grall wrote: > > > On 17/01/2024 08:30, Michal Orzel wrote: >> Hi Julien, > > Hi Michal, > >> On 16/01/2024 15:37, Julien Grall wrote: >>> >>> >>> From: Julien Grall <jgrall@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 <jgrall@amazon.com> >> Reviewed-by: Michal Orzel <michal.orzel@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? > > I don't see the problem of keeping the declaration in mmu/mm.h. In any > case, this seems to be unrelated to this patch. This was just a question about the sense of a declaration that is not used/needed at all. If you also thought so, it could be done in this patch given that it touches definition. Since you don't, no problem whatsoever. ~Michal
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; /* 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 + */ + 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)