diff mbox series

[BOOT-WRAPPER,v2,08/10] Simplify spin logic

Message ID 20240812101555.3558589-9-mark.rutland@arm.com (mailing list archive)
State New, archived
Headers show
Series Cleanup initialization | expand

Commit Message

Mark Rutland Aug. 12, 2024, 10:15 a.m. UTC
The logic for initial boot is more complicated than it needs to be,
with both first_spin() having a special case for CPU0 that requires an
additional argument to be passed to spin().

Simplify this by moving the special-case logic for CPU0 into
first_spin(). This removes the need to provide a dummy mailbox for CPU0
to spin on, simplfiies callers of first_spin() and spin(), which no
longer need to pass a dummy mailbox or 'is_entry' for CPU0.

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/aarch64/spin.S | 11 +----------
 common/boot.c       | 20 ++++++++------------
 common/psci.c       |  2 +-
 include/boot.h      |  2 +-
 4 files changed, 11 insertions(+), 24 deletions(-)

Comments

Andre Przywara Aug. 21, 2024, 4:55 p.m. UTC | #1
On Mon, 12 Aug 2024 11:15:53 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> The logic for initial boot is more complicated than it needs to be,
> with both first_spin() having a special case for CPU0 that requires an
> additional argument to be passed to spin().
> 
> Simplify this by moving the special-case logic for CPU0 into
> first_spin(). This removes the need to provide a dummy mailbox for CPU0
> to spin on, simplfiies callers of first_spin() and spin(), which no
> longer need to pass a dummy mailbox or 'is_entry' for CPU0.

Ah, that's a nice one! Indeed this looks much cleaner now. The change
looks good to me, just some small thing below:

> 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/aarch64/spin.S | 11 +----------
>  common/boot.c       | 20 ++++++++------------
>  common/psci.c       |  2 +-
>  include/boot.h      |  2 +-
>  4 files changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/aarch64/spin.S b/arch/aarch64/spin.S
> index 375f732..a7879d4 100644
> --- a/arch/aarch64/spin.S
> +++ b/arch/aarch64/spin.S
> @@ -23,15 +23,6 @@ ASM_FUNC(start_bootmethod)
>  	 * Primary CPU (x0 = 0) jumps to kernel, the other ones wait for an
>  	 * address to appear in mbox

I think this comment is a bit out of place now, either remove it entirely,
or hint that first_spin will take care of the difference between mbox and
kernel entrypoint.

Regardless:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

>  	 */
> -	adr	x3, mbox
> -	adr	x4, kernel_address
> -	cmp	x0, #0
> -	csel	x1, x3, x4, ne
> +	adr	x1, mbox
>  	mov	x2, #0
>  	bl	first_spin
> -
> -	.align 3
> -kernel_address:
> -	.long 0
> -
> -	.ltorg
> diff --git a/common/boot.c b/common/boot.c
> index 29d53a4..4417649 100644
> --- a/common/boot.c
> +++ b/common/boot.c
> @@ -27,7 +27,7 @@ const unsigned long id_table[] = { CPU_IDS };
>   * @invalid: value of an invalid address, 0 or -1 depending on the boot method
>   * @is_entry: when true, pass boot parameters to the kernel, instead of 0
>   */
> -void __noreturn spin(unsigned long *mbox, unsigned long invalid, int is_entry)
> +void __noreturn spin(unsigned long *mbox, unsigned long invalid)
>  {
>  	unsigned long addr = invalid;
>  
> @@ -36,13 +36,6 @@ void __noreturn spin(unsigned long *mbox, unsigned long invalid, int is_entry)
>  		addr = *mbox;
>  	}
>  
> -	if (is_entry)
> -#ifdef KERNEL_32
> -		jump_kernel(addr, 0, ~0, (unsigned long)&dtb, 0);
> -#else
> -		jump_kernel(addr, (unsigned long)&dtb, 0, 0, 0);
> -#endif
> -
>  	jump_kernel(addr, 0, 0, 0, 0);
>  
>  	unreachable();
> @@ -60,12 +53,15 @@ void __noreturn first_spin(unsigned int cpu, unsigned long *mbox,
>  			   unsigned long invalid)
>  {
>  	if (cpu == 0) {
> -		*mbox = (unsigned long)&entrypoint;
> -		sevl();
> -		spin(mbox, invalid, 1);
> +		unsigned long addr = (unsigned long)&entrypoint;
> +#ifdef KERNEL_32
> +		jump_kernel(addr, 0, ~0, (unsigned long)&dtb, 0);
> +#else
> +		jump_kernel(addr, (unsigned long)&dtb, 0, 0, 0);
> +#endif
>  	} else {
>  		*mbox = invalid;
> -		spin(mbox, invalid, 0);
> +		spin(mbox, invalid);
>  	}
>  
>  	unreachable();
> diff --git a/common/psci.c b/common/psci.c
> index 5ae4255..19cc315 100644
> --- a/common/psci.c
> +++ b/common/psci.c
> @@ -57,7 +57,7 @@ static int psci_cpu_off(void)
>  
>  	branch_table[cpu] = PSCI_ADDR_INVALID;
>  
> -	spin(branch_table + cpu, PSCI_ADDR_INVALID, 0);
> +	spin(branch_table + cpu, PSCI_ADDR_INVALID);
>  
>  	unreachable();
>  }
> diff --git a/include/boot.h b/include/boot.h
> index 459d1d5..18b805d 100644
> --- a/include/boot.h
> +++ b/include/boot.h
> @@ -12,7 +12,7 @@
>  #include <compiler.h>
>  #include <stdbool.h>
>  
> -void __noreturn spin(unsigned long *mbox, unsigned long invalid, int is_entry);
> +void __noreturn spin(unsigned long *mbox, unsigned long invalid);
>  
>  void __noreturn first_spin(unsigned int cpu, unsigned long *mbox,
>  			   unsigned long invalid_addr);
Mark Rutland Aug. 22, 2024, 9:54 a.m. UTC | #2
On Wed, Aug 21, 2024 at 05:55:25PM +0100, Andre Przywara wrote:
> On Mon, 12 Aug 2024 11:15:53 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > The logic for initial boot is more complicated than it needs to be,
> > with both first_spin() having a special case for CPU0 that requires an
> > additional argument to be passed to spin().
> > 
> > Simplify this by moving the special-case logic for CPU0 into
> > first_spin(). This removes the need to provide a dummy mailbox for CPU0
> > to spin on, simplfiies callers of first_spin() and spin(), which no
> > longer need to pass a dummy mailbox or 'is_entry' for CPU0.
> 
> Ah, that's a nice one! Indeed this looks much cleaner now. The change
> looks good to me, just some small thing below:
> 
> > 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/aarch64/spin.S | 11 +----------
> >  common/boot.c       | 20 ++++++++------------
> >  common/psci.c       |  2 +-
> >  include/boot.h      |  2 +-
> >  4 files changed, 11 insertions(+), 24 deletions(-)
> > 
> > diff --git a/arch/aarch64/spin.S b/arch/aarch64/spin.S
> > index 375f732..a7879d4 100644
> > --- a/arch/aarch64/spin.S
> > +++ b/arch/aarch64/spin.S
> > @@ -23,15 +23,6 @@ ASM_FUNC(start_bootmethod)
> >  	 * Primary CPU (x0 = 0) jumps to kernel, the other ones wait for an
> >  	 * address to appear in mbox
> 
> I think this comment is a bit out of place now, either remove it entirely,
> or hint that first_spin will take care of the difference between mbox and
> kernel entrypoint.

I'll delete it.

> Regardless:
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thanks!

Mark.
diff mbox series

Patch

diff --git a/arch/aarch64/spin.S b/arch/aarch64/spin.S
index 375f732..a7879d4 100644
--- a/arch/aarch64/spin.S
+++ b/arch/aarch64/spin.S
@@ -23,15 +23,6 @@  ASM_FUNC(start_bootmethod)
 	 * Primary CPU (x0 = 0) jumps to kernel, the other ones wait for an
 	 * address to appear in mbox
 	 */
-	adr	x3, mbox
-	adr	x4, kernel_address
-	cmp	x0, #0
-	csel	x1, x3, x4, ne
+	adr	x1, mbox
 	mov	x2, #0
 	bl	first_spin
-
-	.align 3
-kernel_address:
-	.long 0
-
-	.ltorg
diff --git a/common/boot.c b/common/boot.c
index 29d53a4..4417649 100644
--- a/common/boot.c
+++ b/common/boot.c
@@ -27,7 +27,7 @@  const unsigned long id_table[] = { CPU_IDS };
  * @invalid: value of an invalid address, 0 or -1 depending on the boot method
  * @is_entry: when true, pass boot parameters to the kernel, instead of 0
  */
-void __noreturn spin(unsigned long *mbox, unsigned long invalid, int is_entry)
+void __noreturn spin(unsigned long *mbox, unsigned long invalid)
 {
 	unsigned long addr = invalid;
 
@@ -36,13 +36,6 @@  void __noreturn spin(unsigned long *mbox, unsigned long invalid, int is_entry)
 		addr = *mbox;
 	}
 
-	if (is_entry)
-#ifdef KERNEL_32
-		jump_kernel(addr, 0, ~0, (unsigned long)&dtb, 0);
-#else
-		jump_kernel(addr, (unsigned long)&dtb, 0, 0, 0);
-#endif
-
 	jump_kernel(addr, 0, 0, 0, 0);
 
 	unreachable();
@@ -60,12 +53,15 @@  void __noreturn first_spin(unsigned int cpu, unsigned long *mbox,
 			   unsigned long invalid)
 {
 	if (cpu == 0) {
-		*mbox = (unsigned long)&entrypoint;
-		sevl();
-		spin(mbox, invalid, 1);
+		unsigned long addr = (unsigned long)&entrypoint;
+#ifdef KERNEL_32
+		jump_kernel(addr, 0, ~0, (unsigned long)&dtb, 0);
+#else
+		jump_kernel(addr, (unsigned long)&dtb, 0, 0, 0);
+#endif
 	} else {
 		*mbox = invalid;
-		spin(mbox, invalid, 0);
+		spin(mbox, invalid);
 	}
 
 	unreachable();
diff --git a/common/psci.c b/common/psci.c
index 5ae4255..19cc315 100644
--- a/common/psci.c
+++ b/common/psci.c
@@ -57,7 +57,7 @@  static int psci_cpu_off(void)
 
 	branch_table[cpu] = PSCI_ADDR_INVALID;
 
-	spin(branch_table + cpu, PSCI_ADDR_INVALID, 0);
+	spin(branch_table + cpu, PSCI_ADDR_INVALID);
 
 	unreachable();
 }
diff --git a/include/boot.h b/include/boot.h
index 459d1d5..18b805d 100644
--- a/include/boot.h
+++ b/include/boot.h
@@ -12,7 +12,7 @@ 
 #include <compiler.h>
 #include <stdbool.h>
 
-void __noreturn spin(unsigned long *mbox, unsigned long invalid, int is_entry);
+void __noreturn spin(unsigned long *mbox, unsigned long invalid);
 
 void __noreturn first_spin(unsigned int cpu, unsigned long *mbox,
 			   unsigned long invalid_addr);