linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Jaxson Han <Jaxson.Han@arm.com>
To: Andre Przywara <Andre.Przywara@arm.com>
Cc: Mark Rutland <Mark.Rutland@arm.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	Wei Chen <Wei.Chen@arm.com>
Subject: RE: [boot-wrapper PATCH v2 2/8] aarch64: Rename labels and prepare for lower EL booting
Date: Tue, 25 May 2021 01:54:33 +0000	[thread overview]
Message-ID: <AS8PR08MB6117C4496198003AD6E4733A83259@AS8PR08MB6117.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <20210524103224.564dbfb2@slackpad.fritz.box>

Hi Andre,

> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Monday, May 24, 2021 5:32 PM
> To: Jaxson Han <Jaxson.Han@arm.com>
> Cc: Mark Rutland <Mark.Rutland@arm.com>; linux-arm-
> kernel@lists.infradead.org; Wei Chen <Wei.Chen@arm.com>
> Subject: Re: [boot-wrapper PATCH v2 2/8] aarch64: Rename labels and
> prepare for lower EL booting
> 
> On Fri, 21 May 2021 18:48:01 +0800
> Jaxson Han <jaxson.han@arm.com> wrote:
> 
> > Prepare for booting from lower EL. Rename *_el3 relavant labels with
> > *_el_max and *_no_el3 with *_keep_el. Since the original _no_el3 means
> > "We neither do init sequence at this highest EL nor drop to lower EL
> > when entering to kernel", we rename it with _keep_el to make it more
> > clear for lower EL initialisation.
> 
> Thanks for splitting out this patch, I verified that most of it is indeed just
> renames, apart from the things below:
> 
> >
> > Signed-off-by: Jaxson Han <jaxson.han@arm.com>
> > ---
> >  arch/aarch64/boot.S            | 33 ++++++++++++++++++++++-----------
> >  arch/aarch64/include/asm/cpu.h |  3 +++
> >  arch/aarch64/psci.S            | 13 +++++++------
> >  arch/aarch64/spin.S            |  8 ++++----
> >  4 files changed, 36 insertions(+), 21 deletions(-)
> >
> > diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S index
> > a9264de..e4f5f3d 100644
> > --- a/arch/aarch64/boot.S
> > +++ b/arch/aarch64/boot.S
> > @@ -12,7 +12,7 @@
> >  	.section .init
> >
> >  	.globl	_start
> > -	.globl jump_kernel
> > +	.globl	jump_kernel
> >
> >  _start:
> >  	cpuid	x0, x1
> > @@ -22,20 +22,31 @@ _start:
> >  	bl	setup_stack
> >
> >  	/*
> > -	 * EL3 initialisation
> > +	 * Boot sequence
> > +	 * If CurrentEL == EL3, then goto EL3 initialisation and drop to
> > +	 *   lower EL before entering the kernel.
> > +	 * Else, no initialisation and keep the current EL before
> > +	 *   entering the kernel.
> >  	 */
> >  	mrs	x0, CurrentEL
> >  	cmp	x0, #CURRENTEL_EL3
> > -	b.eq	1f
> > +	beq	el3_init
> >
> > +	/*
> > +	 * We stay in the current EL for entering the kernel
> > +	 */
> >  	mov	w0, #1
> > -	ldr	x1, =flag_no_el3
> > +	ldr	x1, =flag_keep_el
> >  	str	w0, [x1]
> >
> >  	bl	setup_stack
> > -	b	start_no_el3
> > +	b	start_keep_el
> >
> > -1:	mov	x0, #0x30			// RES1
> > +	/*
> > +	 * EL3 initialisation
> > +	 */
> > +el3_init:
> > +	mov	x0, #0x30			// RES1
> >  	orr	x0, x0, #(1 << 0)		// Non-secure EL1
> >  	orr	x0, x0, #(1 << 8)		// HVC enable
> >
> > @@ -114,7 +125,7 @@ _start:
> >
> >  	bl	gic_secure_init
> >
> > -	b	start_el3
> > +	b	start_el_max
> >
> >  err_invalid_id:
> >  	b	.
> > @@ -141,7 +152,7 @@ jump_kernel:
> >  	bl	find_logical_id
> >  	bl	setup_stack		// Reset stack pointer
> >
> > -	ldr	w0, flag_no_el3
> > +	ldr	w0, flag_keep_el
> >  	cmp	w0, #0			// Prepare Z flag
> >
> >  	mov	x0, x20
> > @@ -150,9 +161,9 @@ jump_kernel:
> >  	mov	x3, x23
> >
> >  	b.eq	1f
> > -	br	x19			// No EL3
> > +	br	x19			// Keep current EL
> >
> > -1:	mov	x4, #SPSR_KERNEL
> > +1:	ldr	w4, #SPSR_KERNEL
> 
> This looks both premature and wrong: The value of SPSR_KERNEL doesn't
> change here, so doesn't need any change in this instruction. Plus: ldr without
> '=' requires an address, not a value, and this in fact already breaks
> compilation:
> arch/aarch64/boot.S:166: Error: immediate value must be a multiple of 4 at
> operand 2 -- `ldr w4,#((1<<8)|(1<<9)|(1<<7)|(1<<6)|(9<<0))'
> 
> So please remove this change, and also the changes in cpu.h below, and
> introduce them only when they are actually needed.
> 
> Apart from the rest is then really just renames.

Right, I made a mistake. I will fix this and move them into patch 5.

Thanks,
Jaxson

> 
> Cheers,
> Andre
> 
> 
> >
> >  	/*
> >  	 * If bit 0 of the kernel address is set, we're entering in AArch32
> > @@ -168,5 +179,5 @@ jump_kernel:
> >
> >  	.data
> >  	.align 3
> > -flag_no_el3:
> > +flag_keep_el:
> >  	.long 0
> > diff --git a/arch/aarch64/include/asm/cpu.h
> > b/arch/aarch64/include/asm/cpu.h index ccb5397..2b3a0a4 100644
> > --- a/arch/aarch64/include/asm/cpu.h
> > +++ b/arch/aarch64/include/asm/cpu.h
> > @@ -11,6 +11,7 @@
> >
> >  #define MPIDR_ID_BITS		0xff00ffffff
> >
> > +#define CURRENTEL_EL2		(2 << 2)
> >  #define CURRENTEL_EL3		(3 << 2)
> >
> >  /*
> > @@ -24,6 +25,7 @@
> >  #define SPSR_I			(1 << 7)	/* IRQ masked */
> >  #define SPSR_F			(1 << 6)	/* FIQ masked */
> >  #define SPSR_T			(1 << 5)	/* Thumb */
> > +#define SPSR_EL1H		(5 << 0)	/* EL1 Handler mode */
> >  #define SPSR_EL2H		(9 << 0)	/* EL2 Handler mode */
> >  #define SPSR_HYP		(0x1a << 0)	/* M[3:0] = hyp, M[4] =
> AArch32 */
> >
> > @@ -42,6 +44,7 @@
> >  #else
> >  #define SCTLR_EL1_RESET		SCTLR_EL1_RES1
> >  #define SPSR_KERNEL		(SPSR_A | SPSR_D | SPSR_I | SPSR_F |
> SPSR_EL2H)
> > +#define SPSR_KERNEL_EL1		(SPSR_A | SPSR_D | SPSR_I | SPSR_F |
> SPSR_EL1H)
> >  #endif
> >
> >  #ifndef __ASSEMBLY__
> > diff --git a/arch/aarch64/psci.S b/arch/aarch64/psci.S index
> > 01ebe7d..ae02fd6 100644
> > --- a/arch/aarch64/psci.S
> > +++ b/arch/aarch64/psci.S
> > @@ -45,8 +45,8 @@ vector:
> >
> >  	.text
> >
> > -	.globl start_no_el3
> > -	.globl start_el3
> > +	.globl start_keep_el
> > +	.globl start_el_max
> >
> >  err_exception:
> >  	b err_exception
> > @@ -101,7 +101,7 @@ smc_exit:
> >  	eret
> >
> >
> > -start_el3:
> > +start_el_max:
> >  	ldr	x0, =vector
> >  	bl	setup_vector
> >
> > @@ -111,10 +111,11 @@ start_el3:
> >  	b	psci_first_spin
> >
> >  /*
> > - * This PSCI implementation requires EL3. Without EL3 we'll only boot
> > the
> > - * primary cpu, all others will be trapped in an infinite loop.
> > + * This PSCI implementation requires the highest EL(EL3 or Armv8-R EL2).
> > + * Without the highest EL, we'll only boot the primary cpu, all
> > + others
> > + * will be trapped in an infinite loop.
> >   */
> > -start_no_el3:
> > +start_keep_el:
> >  	cpuid	x0, x1
> >  	bl	find_logical_id
> >  	cbz	x0, psci_first_spin
> > diff --git a/arch/aarch64/spin.S b/arch/aarch64/spin.S index
> > 72603cf..533177c 100644
> > --- a/arch/aarch64/spin.S
> > +++ b/arch/aarch64/spin.S
> > @@ -11,11 +11,11 @@
> >
> >  	.text
> >
> > -	.globl start_no_el3
> > -	.globl start_el3
> > +	.globl start_keep_el
> > +	.globl start_el_max
> >
> > -start_el3:
> > -start_no_el3:
> > +start_el_max:
> > +start_keep_el:
> >  	cpuid	x0, x1
> >  	bl	find_logical_id
> >


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-05-25  3:30 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-21 10:47 [boot-wrapper PATCH v2 0/8] Add Armv8-R AArch64 support Jaxson Han
2021-05-21 10:48 ` [boot-wrapper PATCH v2 1/8] Decouple V2M_SYS config by auto-detect dtb node Jaxson Han
2021-05-21 10:48 ` [boot-wrapper PATCH v2 2/8] aarch64: Rename labels and prepare for lower EL booting Jaxson Han
2021-05-24  9:32   ` Andre Przywara
2021-05-25  1:54     ` Jaxson Han [this message]
2021-05-21 10:48 ` [boot-wrapper PATCH v2 3/8] aarch64: Remove the redundant setup_stack Jaxson Han
2021-05-24  9:32   ` Andre Przywara
2021-05-21 10:48 ` [boot-wrapper PATCH v2 4/8] aarch64: Prepare for EL1 booting Jaxson Han
2021-05-24  9:32   ` Andre Przywara
2021-05-21 10:48 ` [boot-wrapper PATCH v2 5/8] aarch64: Prepare for lower EL booting Jaxson Han
2021-05-24  9:33   ` Andre Przywara
2021-05-21 10:48 ` [boot-wrapper PATCH v2 6/8] gic-v3: Prepare for gicv3 with EL2 Jaxson Han
2021-05-24  9:33   ` Andre Przywara
2021-05-21 10:48 ` [boot-wrapper PATCH v2 7/8] aarch64: Prepare for booting " Jaxson Han
2021-05-24  9:33   ` Andre Przywara
2021-05-21 10:48 ` [boot-wrapper PATCH v2 8/8] aarch64: Introduce EL2 boot code for Armv8-R AArch64 Jaxson Han
2021-05-24 10:19   ` Andre Przywara
2021-05-25  1:59     ` Jaxson Han

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AS8PR08MB6117C4496198003AD6E4733A83259@AS8PR08MB6117.eurprd08.prod.outlook.com \
    --to=jaxson.han@arm.com \
    --cc=Andre.Przywara@arm.com \
    --cc=Mark.Rutland@arm.com \
    --cc=Wei.Chen@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --subject='RE: [boot-wrapper PATCH v2 2/8] aarch64: Rename labels and prepare for lower EL booting' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).