| Message ID | 20240812101555.3558589-11-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup initialization | expand |
On Mon, 12 Aug 2024 11:15:55 +0100 Mark Rutland <mark.rutland@arm.com> wrote: > Currently the boot-wrapper initializes all CPUs in parallel. This means > that we cannot log errors as they happen, as this would mean multiple > CPUs concurrently writing to the UART, producing garbled output. To > produce meaningful output we have to special-case errors on the boot CPU > and hope CPUs have been configured consistently. > > To make it easier to handle errors, boot CPUs sequentially so that errors > can be logged as they happen. With this change it's pretty clear that > the cpu_init_bootmethod() abstraction isn't helpful, and so this is > removed with cpu_init_arch() directly initializing PSCI where necessary. Yeah, makes sense, though the change is a bit hard to follow in the diff below. But I convinced myself that it's correct. Just one small comment inside: > When things go well this looks like: > > | Boot-wrapper v0.2 > | Entered at EL3 > | Memory layout: > | [0000000080000000..0000000080001f90] => boot-wrapper > | [000000008000fff8..0000000080010000] => mbox > | [0000000080200000..0000000082cbaa00] => kernel > | [0000000088000000..0000000088002df1] => dtb > | CPU0: (MPIDR 0000000000000000) initializing... > | CPU0: Done > | CPU1: (MPIDR 0000000000000100) initializing... > | CPU1: Done > | CPU2: (MPIDR 0000000000000200) initializing... > | CPU2: Done > | CPU3: (MPIDR 0000000000000300) initializing... > | CPU3: Done > | CPU4: (MPIDR 0000000000010000) initializing... > | CPU4: Done > | CPU5: (MPIDR 0000000000010100) initializing... > | CPU5: Done > | CPU6: (MPIDR 0000000000010200) initializing... > | CPU6: Done > | CPU7: (MPIDR 0000000000010300) initializing... > | CPU7: Done > | All CPUs initialized. Entering kernel... > | > | [ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd0f0] > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Acked-by: Marc Zyngier <maz@kernel.org> > Cc: Akos Denke <akos.denke@arm.com> > Cc: Andre Przywara <andre.przywara@arm.com> > Cc: Luca Fancellu <luca.fancellu@arm.com> > --- > arch/aarch32/boot.S | 2 -- > arch/aarch32/init.c | 17 +++++++++------ > arch/aarch64/boot.S | 2 -- > arch/aarch64/init.c | 17 +++++++++------ > arch/aarch64/spin.S | 3 --- > common/init.c | 50 +++++++++++++++++++++++++++++++++++++-------- > common/psci.c | 14 ------------- > include/boot.h | 6 +----- > 8 files changed, 64 insertions(+), 47 deletions(-) > > diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S > index 8b6ffac..08029ff 100644 > --- a/arch/aarch32/boot.S > +++ b/arch/aarch32/boot.S > @@ -70,8 +70,6 @@ reset_common: > > bl cpu_init_bootwrapper > > - bl cpu_init_arch > - > b start_bootmethod > > err_invalid_id: > diff --git a/arch/aarch32/init.c b/arch/aarch32/init.c > index cb67bf6..d08ac83 100644 > --- a/arch/aarch32/init.c > +++ b/arch/aarch32/init.c > @@ -6,6 +6,7 @@ > * Use of this source code is governed by a BSD-style license that can be > * found in the LICENSE.txt file. > */ > +#include <boot.h> > #include <cpu.h> > #include <gic.h> > #include <platform.h> > @@ -43,24 +44,28 @@ static void cpu_init_monitor(void) > #ifdef PSCI > extern char psci_vectors[]; > > -bool cpu_init_psci_arch(void) > +static void cpu_init_psci_arch(unsigned int cpu) > { > - if (read_cpsr_mode() != PSR_MON) > - return false; > + if (read_cpsr_mode() != PSR_MON) { > + print_cpu_warn(cpu, "PSCI could not be initialized (not booted at PL1).\r\n"); > + return; > + } > > mcr(MVBAR, (unsigned long)psci_vectors); > isb(); > - > - return true; > } > +#else > +static static void cpu_init_psci_arch(unsigned int cpu) { } > #endif > > -void cpu_init_arch(void) > +void cpu_init_arch(unsigned int cpu) > { > if (read_cpsr_mode() == PSR_MON) { > cpu_init_monitor(); > gic_secure_init(); > } > > + cpu_init_psci_arch(cpu); > + > mcr(CNTFRQ, COUNTER_FREQ); > } > diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S > index 98ae77d..6bec836 100644 > --- a/arch/aarch64/boot.S > +++ b/arch/aarch64/boot.S > @@ -69,8 +69,6 @@ reset_common: > > bl cpu_init_bootwrapper > > - bl cpu_init_arch > - > b start_bootmethod > > err_invalid_id: > diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c > index 68c220b..63fa949 100644 > --- a/arch/aarch64/init.c > +++ b/arch/aarch64/init.c > @@ -6,6 +6,7 @@ > * Use of this source code is governed by a BSD-style license that can be > * found in the LICENSE.txt file. > */ > +#include <boot.h> > #include <cpu.h> > #include <gic.h> > #include <platform.h> > @@ -159,24 +160,28 @@ static void cpu_init_el3(void) > #ifdef PSCI > extern char psci_vectors[]; > > -bool cpu_init_psci_arch(void) > +static void cpu_init_psci_arch(unsigned int cpu) > { > - if (mrs(CurrentEL) != CURRENTEL_EL3) > - return false; > + if (mrs(CurrentEL) != CURRENTEL_EL3) { > + print_cpu_warn(cpu, "PSCI could not be initialized (not booted at EL3).\r\n"); > + return; > + } > > msr(VBAR_EL3, (unsigned long)psci_vectors); > isb(); > - > - return true; > } > +#else > +static void cpu_init_psci_arch(unsigned int cpu) { } > #endif > > -void cpu_init_arch(void) > +void cpu_init_arch(unsigned int cpu) > { > if (mrs(CurrentEL) == CURRENTEL_EL3) { > cpu_init_el3(); > gic_secure_init(); > } > > + cpu_init_psci_arch(cpu); > + > msr(CNTFRQ_EL0, COUNTER_FREQ); > } > diff --git a/arch/aarch64/spin.S b/arch/aarch64/spin.S > index a7879d4..81d7ee4 100644 > --- a/arch/aarch64/spin.S > +++ b/arch/aarch64/spin.S > @@ -12,9 +12,6 @@ > > .text > > -ASM_FUNC(cpu_init_bootmethod) > - ret > - > ASM_FUNC(start_bootmethod) > cpuid x0, x1 > bl find_logical_id > diff --git a/common/init.c b/common/init.c > index 3c05ac3..6197ead 100644 > --- a/common/init.c > +++ b/common/init.c > @@ -43,18 +43,50 @@ static void announce_objects(void) > > void announce_arch(void); > > +static void init_bootwrapper(void) > +{ > + init_uart(); > + announce_bootwrapper(); > + announce_arch(); > + announce_objects(); > + init_platform(); > +} > + > +static void cpu_init_self(unsigned int cpu) > +{ > + print_string("CPU"); > + print_uint_dec(cpu); > + print_string(": (MPIDR "); > + print_ulong_hex(read_mpidr()); > + print_string(") initializing...\r\n"); It feels like using two lines per core for something that rarely fails is a bit wasteful. Can we get rid of the EOL here, and just put the "Done" right behind it, in the same line? That reduces the noise on the terminal. Cheers, Andre > + > + cpu_init_arch(cpu); > + > + print_cpu_msg(cpu, "Done\r\n"); > +} > + > void cpu_init_bootwrapper(void) > { > + static volatile unsigned int cpu_next = 0; > unsigned int cpu = this_cpu_logical_id(); > > - if (cpu == 0) { > - init_uart(); > - announce_bootwrapper(); > - announce_arch(); > - announce_objects(); > - print_string("\r\n"); > - init_platform(); > - } > + if (cpu == 0) > + init_bootwrapper(); > + > + while (cpu_next != cpu) > + wfe(); > + > + cpu_init_self(cpu); > + > + cpu_next = cpu + 1; > + dsb(sy); > + sev(); > + > + if (cpu != 0) > + return; > + > + while (cpu_next != NR_CPUS) > + wfe(); > > - cpu_init_bootmethod(cpu); > + print_string("All CPUs initialized. Entering kernel...\r\n\r\n"); > } > diff --git a/common/psci.c b/common/psci.c > index 19cc315..5fe8999 100644 > --- a/common/psci.c > +++ b/common/psci.c > @@ -87,17 +87,3 @@ void __noreturn psci_first_spin(void) > > unreachable(); > } > - > -void cpu_init_bootmethod(unsigned int cpu) > -{ > - if (cpu_init_psci_arch()) > - return; > - > - if (cpu == 0) { > - print_string("WARNING: PSCI could not be initialized. Boot may fail\r\n\r\n"); > - return; > - } > - > - while (1) > - wfe(); > -} > diff --git a/include/boot.h b/include/boot.h > index 18b805d..12c9c5c 100644 > --- a/include/boot.h > +++ b/include/boot.h > @@ -17,10 +17,6 @@ void __noreturn spin(unsigned long *mbox, unsigned long invalid); > void __noreturn first_spin(unsigned int cpu, unsigned long *mbox, > unsigned long invalid_addr); > > -void cpu_init_bootmethod(unsigned int cpu); > - > -#ifdef PSCI > -bool cpu_init_psci_arch(void); > -#endif > +void cpu_init_arch(unsigned int cpu); > > #endif
On Wed, Aug 21, 2024 at 06:49:23PM +0100, Andre Przywara wrote: > On Mon, 12 Aug 2024 11:15:55 +0100 > Mark Rutland <mark.rutland@arm.com> wrote: > > > Currently the boot-wrapper initializes all CPUs in parallel. This means > > that we cannot log errors as they happen, as this would mean multiple > > CPUs concurrently writing to the UART, producing garbled output. To > > produce meaningful output we have to special-case errors on the boot CPU > > and hope CPUs have been configured consistently. > > > > To make it easier to handle errors, boot CPUs sequentially so that errors > > can be logged as they happen. With this change it's pretty clear that > > the cpu_init_bootmethod() abstraction isn't helpful, and so this is > > removed with cpu_init_arch() directly initializing PSCI where necessary. > > Yeah, makes sense, though the change is a bit hard to follow in the diff > below. But I convinced myself that it's correct. Just one small comment > inside: > > > When things go well this looks like: > > > > | Boot-wrapper v0.2 > > | Entered at EL3 > > | Memory layout: > > | [0000000080000000..0000000080001f90] => boot-wrapper > > | [000000008000fff8..0000000080010000] => mbox > > | [0000000080200000..0000000082cbaa00] => kernel > > | [0000000088000000..0000000088002df1] => dtb > > | CPU0: (MPIDR 0000000000000000) initializing... > > | CPU0: Done > > | CPU1: (MPIDR 0000000000000100) initializing... > > | CPU1: Done > > | CPU2: (MPIDR 0000000000000200) initializing... > > | CPU2: Done > > | CPU3: (MPIDR 0000000000000300) initializing... > > | CPU3: Done > > | CPU4: (MPIDR 0000000000010000) initializing... > > | CPU4: Done > > | CPU5: (MPIDR 0000000000010100) initializing... > > | CPU5: Done > > | CPU6: (MPIDR 0000000000010200) initializing... > > | CPU6: Done > > | CPU7: (MPIDR 0000000000010300) initializing... > > | CPU7: Done > > | All CPUs initialized. Entering kernel... > > | > > | [ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd0f0] > > +static void cpu_init_self(unsigned int cpu) > > +{ > > + print_string("CPU"); > > + print_uint_dec(cpu); > > + print_string(": (MPIDR "); > > + print_ulong_hex(read_mpidr()); > > + print_string(") initializing...\r\n"); > > It feels like using two lines per core for something that rarely fails > is a bit wasteful. Can we get rid of the EOL here, and just put the "Done" > right behind it, in the same line? That reduces the noise on the terminal. That'll interact poorly with logging errors, as the first will get appended to the "initializing..." line. Instead, I'll get rid of the "Done" line, so that the output ends up as: | CPU0: (MPIDR 0000000000000000) initializing... | CPU1: (MPIDR 0000000000000100) initializing... | CPU2: (MPIDR 0000000000000200) initializing... | CPU3: (MPIDR 0000000000000300) initializing... | CPU4: (MPIDR 0000000000010000) initializing... | CPU5: (MPIDR 0000000000010100) initializing... | CPU6: (MPIDR 0000000000010200) initializing... | CPU7: (MPIDR 0000000000010300) initializing... | All CPUs initialized. Entering kernel... ... and if any error occurs it'll still be clear which CPU it belongs to given the next line will either be the next CPU being announced or the final "All CPUs initialized." message. Mark.
diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S index 8b6ffac..08029ff 100644 --- a/arch/aarch32/boot.S +++ b/arch/aarch32/boot.S @@ -70,8 +70,6 @@ reset_common: bl cpu_init_bootwrapper - bl cpu_init_arch - b start_bootmethod err_invalid_id: diff --git a/arch/aarch32/init.c b/arch/aarch32/init.c index cb67bf6..d08ac83 100644 --- a/arch/aarch32/init.c +++ b/arch/aarch32/init.c @@ -6,6 +6,7 @@ * Use of this source code is governed by a BSD-style license that can be * found in the LICENSE.txt file. */ +#include <boot.h> #include <cpu.h> #include <gic.h> #include <platform.h> @@ -43,24 +44,28 @@ static void cpu_init_monitor(void) #ifdef PSCI extern char psci_vectors[]; -bool cpu_init_psci_arch(void) +static void cpu_init_psci_arch(unsigned int cpu) { - if (read_cpsr_mode() != PSR_MON) - return false; + if (read_cpsr_mode() != PSR_MON) { + print_cpu_warn(cpu, "PSCI could not be initialized (not booted at PL1).\r\n"); + return; + } mcr(MVBAR, (unsigned long)psci_vectors); isb(); - - return true; } +#else +static static void cpu_init_psci_arch(unsigned int cpu) { } #endif -void cpu_init_arch(void) +void cpu_init_arch(unsigned int cpu) { if (read_cpsr_mode() == PSR_MON) { cpu_init_monitor(); gic_secure_init(); } + cpu_init_psci_arch(cpu); + mcr(CNTFRQ, COUNTER_FREQ); } diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S index 98ae77d..6bec836 100644 --- a/arch/aarch64/boot.S +++ b/arch/aarch64/boot.S @@ -69,8 +69,6 @@ reset_common: bl cpu_init_bootwrapper - bl cpu_init_arch - b start_bootmethod err_invalid_id: diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c index 68c220b..63fa949 100644 --- a/arch/aarch64/init.c +++ b/arch/aarch64/init.c @@ -6,6 +6,7 @@ * Use of this source code is governed by a BSD-style license that can be * found in the LICENSE.txt file. */ +#include <boot.h> #include <cpu.h> #include <gic.h> #include <platform.h> @@ -159,24 +160,28 @@ static void cpu_init_el3(void) #ifdef PSCI extern char psci_vectors[]; -bool cpu_init_psci_arch(void) +static void cpu_init_psci_arch(unsigned int cpu) { - if (mrs(CurrentEL) != CURRENTEL_EL3) - return false; + if (mrs(CurrentEL) != CURRENTEL_EL3) { + print_cpu_warn(cpu, "PSCI could not be initialized (not booted at EL3).\r\n"); + return; + } msr(VBAR_EL3, (unsigned long)psci_vectors); isb(); - - return true; } +#else +static void cpu_init_psci_arch(unsigned int cpu) { } #endif -void cpu_init_arch(void) +void cpu_init_arch(unsigned int cpu) { if (mrs(CurrentEL) == CURRENTEL_EL3) { cpu_init_el3(); gic_secure_init(); } + cpu_init_psci_arch(cpu); + msr(CNTFRQ_EL0, COUNTER_FREQ); } diff --git a/arch/aarch64/spin.S b/arch/aarch64/spin.S index a7879d4..81d7ee4 100644 --- a/arch/aarch64/spin.S +++ b/arch/aarch64/spin.S @@ -12,9 +12,6 @@ .text -ASM_FUNC(cpu_init_bootmethod) - ret - ASM_FUNC(start_bootmethod) cpuid x0, x1 bl find_logical_id diff --git a/common/init.c b/common/init.c index 3c05ac3..6197ead 100644 --- a/common/init.c +++ b/common/init.c @@ -43,18 +43,50 @@ static void announce_objects(void) void announce_arch(void); +static void init_bootwrapper(void) +{ + init_uart(); + announce_bootwrapper(); + announce_arch(); + announce_objects(); + init_platform(); +} + +static void cpu_init_self(unsigned int cpu) +{ + print_string("CPU"); + print_uint_dec(cpu); + print_string(": (MPIDR "); + print_ulong_hex(read_mpidr()); + print_string(") initializing...\r\n"); + + cpu_init_arch(cpu); + + print_cpu_msg(cpu, "Done\r\n"); +} + void cpu_init_bootwrapper(void) { + static volatile unsigned int cpu_next = 0; unsigned int cpu = this_cpu_logical_id(); - if (cpu == 0) { - init_uart(); - announce_bootwrapper(); - announce_arch(); - announce_objects(); - print_string("\r\n"); - init_platform(); - } + if (cpu == 0) + init_bootwrapper(); + + while (cpu_next != cpu) + wfe(); + + cpu_init_self(cpu); + + cpu_next = cpu + 1; + dsb(sy); + sev(); + + if (cpu != 0) + return; + + while (cpu_next != NR_CPUS) + wfe(); - cpu_init_bootmethod(cpu); + print_string("All CPUs initialized. Entering kernel...\r\n\r\n"); } diff --git a/common/psci.c b/common/psci.c index 19cc315..5fe8999 100644 --- a/common/psci.c +++ b/common/psci.c @@ -87,17 +87,3 @@ void __noreturn psci_first_spin(void) unreachable(); } - -void cpu_init_bootmethod(unsigned int cpu) -{ - if (cpu_init_psci_arch()) - return; - - if (cpu == 0) { - print_string("WARNING: PSCI could not be initialized. Boot may fail\r\n\r\n"); - return; - } - - while (1) - wfe(); -} diff --git a/include/boot.h b/include/boot.h index 18b805d..12c9c5c 100644 --- a/include/boot.h +++ b/include/boot.h @@ -17,10 +17,6 @@ void __noreturn spin(unsigned long *mbox, unsigned long invalid); void __noreturn first_spin(unsigned int cpu, unsigned long *mbox, unsigned long invalid_addr); -void cpu_init_bootmethod(unsigned int cpu); - -#ifdef PSCI -bool cpu_init_psci_arch(void); -#endif +void cpu_init_arch(unsigned int cpu); #endif