| Message ID | 20240116143709.86584-3-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| 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, smp_up_cpu is used by secondary CPUs to wait there turn for s/there/their ? > booting 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. > > Move smp_up_cpu to the recently create section .data.idmap. The idmap is s/create/created > currently part of the text section and therefore will be mapped read-onl s/onl/only > executable. This means that we need to temporarily remap > smp_up_cpu in order to update it. > > Introduce a new function set_smp_up_cpu() for this purpose so the code > is not duplicated between when opening and closing the gate. > > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Michal Orzel <michal.orzel@amd.com> > --- > xen/arch/arm/smpboot.c | 36 +++++++++++++++++++++++++++++++----- > 1 file changed, 31 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > index 7110bc11fc05..8d508a1bb258 100644 > --- a/xen/arch/arm/smpboot.c > +++ b/xen/arch/arm/smpboot.c > @@ -29,6 +29,10 @@ > #include <asm/psci.h> > #include <asm/acpi.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)) > + > cpumask_t cpu_online_map; > cpumask_t cpu_present_map; > cpumask_t cpu_possible_map; > @@ -56,7 +60,7 @@ struct init_info init_data = > }; > > /* Shared state for coordinating CPU bringup */ > -unsigned long smp_up_cpu = MPIDR_INVALID; > +unsigned long __section(".data.idmap") smp_up_cpu = MPIDR_INVALID; > /* Shared state for coordinating CPU teardown */ > static bool cpu_is_dead; > > @@ -429,6 +433,28 @@ void stop_cpu(void) > wfi(); > } > > +static void set_smp_up_cpu(unsigned long mpidr) > +{ > + /* > + * smp_up_cpu 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(&smp_up_cpu)); > + > + ptr += PAGE_OFFSET(&smp_up_cpu); > + > + *(unsigned long *)ptr = mpidr; > + > + /* > + * init_ttbr will be accessed with the MMU off, so ensure the update smp_up_cpu instead of init_ttbr > + * is visible by cleaning the cache. > + */ > + clean_dcache(ptr); > + > + unmap_domain_page(ptr); > + > +} > + > int __init cpu_up_send_sgi(int cpu) > { > /* We don't know the GIC ID of the CPU until it has woken up, so just > @@ -460,8 +486,7 @@ int __cpu_up(unsigned int cpu) > init_data.cpuid = cpu; > > /* Open the gate for this CPU */ > - smp_up_cpu = cpu_logical_map(cpu); > - clean_dcache(smp_up_cpu); > + set_smp_up_cpu(cpu_logical_map(cpu)); > > rc = arch_cpu_up(cpu); > > @@ -497,8 +522,9 @@ int __cpu_up(unsigned int cpu) > */ > init_data.stack = NULL; > init_data.cpuid = ~0; > - smp_up_cpu = MPIDR_INVALID; > - clean_dcache(smp_up_cpu); > + > + set_smp_up_cpu(MPIDR_INVALID); > + > arch_cpu_up_finish(); > > if ( !cpu_online(cpu) ) > -- > 2.40.1 > ~Michal
On 17/01/2024 08:44, 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, smp_up_cpu is used by secondary CPUs to wait there turn for > s/there/their ? Yes. I will fix it on commit. > >> booting 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. >> >> Move smp_up_cpu to the recently create section .data.idmap. The idmap is > s/create/created > >> currently part of the text section and therefore will be mapped read-onl > s/onl/only > >> executable. This means that we need to temporarily remap >> smp_up_cpu in order to update it. >> >> Introduce a new function set_smp_up_cpu() for this purpose so the code >> is not duplicated between when opening and closing the gate. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> > Reviewed-by: Michal Orzel <michal.orzel@amd.com> > >> --- >> xen/arch/arm/smpboot.c | 36 +++++++++++++++++++++++++++++++----- >> 1 file changed, 31 insertions(+), 5 deletions(-) >> >> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c >> index 7110bc11fc05..8d508a1bb258 100644 >> --- a/xen/arch/arm/smpboot.c >> +++ b/xen/arch/arm/smpboot.c >> @@ -29,6 +29,10 @@ >> #include <asm/psci.h> >> #include <asm/acpi.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)) >> + >> cpumask_t cpu_online_map; >> cpumask_t cpu_present_map; >> cpumask_t cpu_possible_map; >> @@ -56,7 +60,7 @@ struct init_info init_data = >> }; >> >> /* Shared state for coordinating CPU bringup */ >> -unsigned long smp_up_cpu = MPIDR_INVALID; >> +unsigned long __section(".data.idmap") smp_up_cpu = MPIDR_INVALID; >> /* Shared state for coordinating CPU teardown */ >> static bool cpu_is_dead; >> >> @@ -429,6 +433,28 @@ void stop_cpu(void) >> wfi(); >> } >> >> +static void set_smp_up_cpu(unsigned long mpidr) >> +{ >> + /* >> + * smp_up_cpu 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(&smp_up_cpu)); >> + >> + ptr += PAGE_OFFSET(&smp_up_cpu); >> + >> + *(unsigned long *)ptr = mpidr; >> + >> + /* >> + * init_ttbr will be accessed with the MMU off, so ensure the update > smp_up_cpu instead of init_ttbr Doh. I just copied/pasted the code from init_ttbr. I will update it. BTW, I thought about trying to consolidate the code between set_init_ttbr() and set_smp_up_cpu() but it didn't seem to be worth it. Cheers,
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c index 7110bc11fc05..8d508a1bb258 100644 --- a/xen/arch/arm/smpboot.c +++ b/xen/arch/arm/smpboot.c @@ -29,6 +29,10 @@ #include <asm/psci.h> #include <asm/acpi.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)) + cpumask_t cpu_online_map; cpumask_t cpu_present_map; cpumask_t cpu_possible_map; @@ -56,7 +60,7 @@ struct init_info init_data = }; /* Shared state for coordinating CPU bringup */ -unsigned long smp_up_cpu = MPIDR_INVALID; +unsigned long __section(".data.idmap") smp_up_cpu = MPIDR_INVALID; /* Shared state for coordinating CPU teardown */ static bool cpu_is_dead; @@ -429,6 +433,28 @@ void stop_cpu(void) wfi(); } +static void set_smp_up_cpu(unsigned long mpidr) +{ + /* + * smp_up_cpu 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(&smp_up_cpu)); + + ptr += PAGE_OFFSET(&smp_up_cpu); + + *(unsigned long *)ptr = mpidr; + + /* + * 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); + +} + int __init cpu_up_send_sgi(int cpu) { /* We don't know the GIC ID of the CPU until it has woken up, so just @@ -460,8 +486,7 @@ int __cpu_up(unsigned int cpu) init_data.cpuid = cpu; /* Open the gate for this CPU */ - smp_up_cpu = cpu_logical_map(cpu); - clean_dcache(smp_up_cpu); + set_smp_up_cpu(cpu_logical_map(cpu)); rc = arch_cpu_up(cpu); @@ -497,8 +522,9 @@ int __cpu_up(unsigned int cpu) */ init_data.stack = NULL; init_data.cpuid = ~0; - smp_up_cpu = MPIDR_INVALID; - clean_dcache(smp_up_cpu); + + set_smp_up_cpu(MPIDR_INVALID); + arch_cpu_up_finish(); if ( !cpu_online(cpu) )