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 2/5] aarch64: Rename labels and prepare for lower EL booting
Date: Wed, 28 Apr 2021 03:28:57 +0000 [thread overview]
Message-ID: <AS8PR08MB61176B35E989EAABA676D41283409@AS8PR08MB6117.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <20210426124055.20253825@slackpad.fritz.box>
Hi Andre,
> -----Original Message-----
> From: Andre Przywara <andre.przywara@arm.com>
> Sent: Monday, April 26, 2021 7:41 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 2/5] aarch64: Rename labels and prepare
> for lower EL booting
>
> On Tue, 20 Apr 2021 15:24:35 +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.
>
> So this cleanup and rename is fine, however the patch is hard to read since
> various other changes are mixed in.
> Can you try to isolate this more? If this patch would just rename the labels,
> and wouldn't carry any functional change, it might be easier to reason about.
> At least the SPSR value change should be separate.
Yes, I will.
>
> Some more below:
>
> > Also in jump_kernel, skip sctlr_el2 initialisation when CurrentEL < EL2.
> >
> > Signed-off-by: Jaxson Han <jaxson.han@arm.com>
> > ---
> > arch/aarch64/boot.S | 54 +++++++++++++++++++++++++---------
> > arch/aarch64/include/asm/cpu.h | 3 ++
> > arch/aarch64/psci.S | 13 ++++----
> > arch/aarch64/spin.S | 8 ++---
> > 4 files changed, 54 insertions(+), 24 deletions(-)
> >
> > diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S index
> > e47cf59..f7dbf3f 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,30 @@ _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
>
> I think this is indeed redundant, but can you either make this a separate
> patch or at least mention it in the commit message?
Yes, I will make this separate.
>
> > - 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
> >
> > @@ -93,13 +103,23 @@ _start:
> > mov x0, #ZCR_EL3_LEN_MASK // SVE: Enable full
> vector len
> > msr ZCR_EL3, x0 // for EL2.
> >
> > -1:
> > + /*
> > + * Save SPSR_KERNEL into spsr_to_elx.
> > + * The jump_kernel will load spsr_to_elx into spsr_el3
> > + */
> > +1: mov w0, #SPSR_KERNEL
> > + ldr x1, =spsr_to_elx
> > + str w0, [x1]
> > + b el_max_init
>
> This (and the other SPSR related changes below) should be in a separate
> patch.
Yes, I will
>
> > +
> > +el_max_init:
> > ldr x0, =CNTFRQ
> > msr cntfrq_el0, x0
> > + isb
>
> Why this isb here? I don't see that we would require this?
No need here. I'll remove it.
>
> > bl gic_secure_init
> >
> > - b start_el3
> > + b start_el_max
> >
> > err_invalid_id:
> > b .
> > @@ -119,14 +139,18 @@ jump_kernel:
> > ldr x0, =SCTLR_EL1_RESET
> > msr sctlr_el1, x0
> >
> > + mrs x0, CurrentEL
> > + cmp x0, #CURRENTEL_EL2
> > + b.lt 1f
> > +
>
> This is also a change which might be separate.
Yes
Thanks,
Jaxson
>
> Cheers,
> Andre
>
>
> > ldr x0, =SCTLR_EL2_RESET
> > msr sctlr_el2, x0
> >
> > - cpuid x0, x1
> > +1: cpuid x0, x1
> > 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
> > @@ -135,9 +159,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_to_elx
> >
> > /*
> > * If bit 0 of the kernel address is set, we're entering in AArch32
> > @@ -153,5 +177,7 @@ jump_kernel:
> >
> > .data
> > .align 3
> > -flag_no_el3:
> > +flag_keep_el:
> > + .long 0
> > +spsr_to_elx:
> > .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
next prev parent reply other threads:[~2021-04-28 3:31 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-20 7:24 [boot-wrapper PATCH 0/5] Add Armv8-R AArch64 support Jaxson Han
2021-04-20 7:24 ` [boot-wrapper PATCH 1/5] Decouple V2M_SYS config by auto-detect dtb node Jaxson Han
2021-04-26 11:30 ` Andre Przywara
2021-04-28 3:23 ` Jaxson Han
2021-05-10 8:30 ` Andre Przywara
2021-05-10 8:45 ` Jaxson Han
2021-04-20 7:24 ` [boot-wrapper PATCH 2/5] aarch64: Rename labels and prepare for lower EL booting Jaxson Han
2021-04-26 11:40 ` Andre Przywara
2021-04-28 3:28 ` Jaxson Han [this message]
2021-04-20 7:24 ` [boot-wrapper PATCH 3/5] gic-v3: Prepare for gicv3 with EL2 Jaxson Han
2021-04-26 11:48 ` Andre Przywara
2021-04-28 3:30 ` Jaxson Han
2021-04-20 7:24 ` [boot-wrapper PATCH 4/5] aarch64: Prepare for booting " Jaxson Han
2021-04-26 11:51 ` Andre Przywara
2021-04-20 7:24 ` [boot-wrapper PATCH 5/5] aarch64: Introduce EL2 boot code for Armv8-R AArch64 Jaxson Han
2021-04-26 12:35 ` Andre Przywara
2021-04-28 3:44 ` Jaxson Han
2021-05-10 2:13 ` Jaxson Han
2021-05-10 8:54 ` Andre Przywara
2021-05-11 2:03 ` Jaxson Han
2021-05-11 7:59 ` Andre Przywara
2021-05-11 9:49 ` 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=AS8PR08MB61176B35E989EAABA676D41283409@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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).