[boot-wrapper 5/7] aarch64: Introduce EL2 boot code for Armv8-R AArch64

Mon Jul 15 06:40:03 PDT 2024

On Mon, 24 Jun 2024 13:22:38 +0100
Luca Fancellu <Luca.Fancellu at arm.com> wrote:

Hi Luca,

> Hi Andre,
> 
> >  
> >> +
> >> + /* Start Armv8-R Linux at EL1 */
> >> + mov w0, #SPSR_KERNEL_EL1
> >> + ldr x1, =spsr_to_elx
> >> + str w0, [x1]
> >> +
> >> + cpuid x0, x1
> >> + bl find_logical_id
> >> + cmp x0, #MPIDR_INVALID
> >> + b.eq err_invalid_id
> >> + bl setup_stack
> >> +
> >> + bl cpu_init_bootwrapper
> >> +
> >> + bl cpu_init_armv8r_el2
> >> +
> >> + bl gic_secure_init  
> >
> > Do we actually need this? v8-R64 is always secure, so there is nothing
> > special that we'd need to do here? Have you tried dropping this?
> >
> > Also this sequence looks like to have quite some overlap with the existing
> > code.
> > Can't we rewrite it like this:
> >
> > ....
> > blt err_invalid_arch
> > (set SPSR_KERNEL_EL1)
> >
> > bl cpu_init_armv8r_el2
> >
> > b reset_no_el3
> >
> > and just find a solution for flag_keep_el?  
> 
> What do you think about the below changes to be applied to this patch?

A patch to a patch is really hard to read, please send a new version with
your proposal.

> diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
> index b2b9863b8d6a..2a8234f7a17d 100644
> --- a/arch/aarch64/boot.S
> +++ b/arch/aarch64/boot.S
> @@ -108,19 +108,9 @@ reset_at_el2:
>         ldr     x1, =spsr_to_elx
>         str     w0, [x1]
> 
> -       cpuid   x0, x1
> -       bl      find_logical_id
> -       cmp     x0, #MPIDR_INVALID
> -       b.eq    err_invalid_id
> -       bl      setup_stack
> -
> -       bl      cpu_init_bootwrapper
> -
>         bl      cpu_init_armv8r_el2
> 
> -       bl      gic_secure_init
> -
> -       b       start_bootmethod
> +       b       reset_no_el3
> 
>         /*
>          * EL1 initialization
> @@ -139,10 +129,16 @@ reset_no_el3:
>         b.eq    err_invalid_id
>         bl      setup_stack
> 
> +       ldr     w1, spsr_to_elx
> +       and w0, w1, 0xf
> +       cmp     w0, #SPSR_EL1H
> +       b.eq drop_el
> +
>         mov     w0, #1
>         ldr     x1, =flag_keep_el
>         str     w0, [x1]
> 
> +drop_el:
>         bl      cpu_init_bootwrapper
> 
>         b       start_bootmethod
> 
> 
> >  
> >> +
> >> + b start_bootmethod
> >>
> >> /*
> >> * EL1 initialization
> >> @@ -104,6 +148,7 @@ reset_no_el3:
> >> b start_bootmethod
> >>
> >> err_invalid_id:
> >> +err_invalid_arch:
> >> b .
> >>
> >> /*
> >> @@ -121,10 +166,14 @@ ASM_FUNC(jump_kernel)
> >> ldr x0, =SCTLR_EL1_KERNEL
> >> msr sctlr_el1, x0
> >>
> >> + mrs x5, CurrentEL
> >> + cmp x5, #CURRENTEL_EL2
> >> + b.eq 1f
> >> +
> >> ldr x0, =SCTLR_EL2_KERNEL
> >> msr sctlr_el2, x0
> >>
> >> - cpuid x0, x1
> >> +1: cpuid x0, x1
> >> bl find_logical_id
> >> bl setup_stack // Reset stack pointer
> >>
> >> @@ -147,10 +196,18 @@ ASM_FUNC(jump_kernel)
> >> */
> >> bfi x4, x19, #5, #1
> >>
> >> + mrs x5, CurrentEL
> >> + cmp x5, #CURRENTEL_EL2
> >> + b.eq 1f
> >> +
> >> msr elr_el3, x19
> >> msr spsr_el3, x4
> >> eret
> >>
> >> +1: msr elr_el2, x19
> >> + msr spsr_el2, x4
> >> + eret
> >> +
> >> .ltorg
> >>
> >> .data
> >> diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
> >> index 846b89f8405d..6b2f5fbe4502 100644
> >> --- a/arch/aarch64/include/asm/cpu.h
> >> +++ b/arch/aarch64/include/asm/cpu.h
> >> @@ -58,7 +58,13 @@
> >> #define SCR_EL3_TCR2EN BIT(43)
> >> #define SCR_EL3_PIEN BIT(45)
> >>
> >> +#define VTCR_EL2_MSA BIT(31)
> >> +
> >> #define HCR_EL2_RES1 BIT(1)
> >> +#define HCR_EL2_APK_NOTRAP BIT(40)
> >> +#define HCR_EL2_API_NOTRAP BIT(41)
> >> +#define HCR_EL2_FIEN_NOTRAP BIT(47)
> >> +#define HCR_EL2_ENSCXT_NOTRAP BIT(53)  
> >
> > That looks misaligned (checked in the file), you'd need two tabs.
> >  
> >> #define ID_AA64DFR0_EL1_PMSVER BITS(35, 32)
> >> #define ID_AA64DFR0_EL1_TRACEBUFFER BITS(47, 44)
> >> @@ -88,7 +94,9 @@
> >>
> >> #define ID_AA64PFR1_EL1_MTE BITS(11, 8)
> >> #define ID_AA64PFR1_EL1_SME BITS(27, 24)
> >> +#define ID_AA64PFR0_EL1_RAS BITS(31, 28)
> >> #define ID_AA64PFR0_EL1_SVE BITS(35, 32)
> >> +#define ID_AA64PFR0_EL1_CSV2 BITS(59, 56)  
> >
> > Same here, two tabs.
> >
> > The bits and registers are correct, though.
> >  
> >> #define ID_AA64SMFR0_EL1 s3_0_c0_c4_5
> >> #define ID_AA64SMFR0_EL1_FA64 BIT(63)
> >> @@ -114,6 +122,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 */
> >>
> >> @@ -153,6 +162,7 @@
> >> #else
> >> #define SCTLR_EL1_KERNEL 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)  
> >
> > (that one is fine, btw, it just looks off in the diff)  
> 
> Thanks, I’ll fix all the alignments, probably some misconfiguration of my editor.
> 
> >  
> >> #endif
> >>
> >> #ifndef __ASSEMBLY__
> >> diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c
> >> index 37cb45fde446..8006f2705193 100644
> >> --- a/arch/aarch64/init.c
> >> +++ b/arch/aarch64/init.c
> >> @@ -145,6 +145,30 @@ void cpu_init_el3(void)
> >> msr(CNTFRQ_EL0, COUNTER_FREQ);
> >> }
> >>
> >> +void cpu_init_armv8r_el2(void)
> >> +{
> >> + unsigned long hcr = mrs(hcr_el2);
> >> +
> >> + msr(vpidr_el2, mrs(midr_el1));
> >> + msr(vmpidr_el2, mrs(mpidr_el1));
> >> +
> >> + /* VTCR_MSA: VMSAv8-64 support */
> >> + msr(vtcr_el2, VTCR_EL2_MSA);
> >> +
> >> + if (mrs_field(ID_AA64PFR0_EL1, CSV2) <= 2)  
> >
> > What is the significance of "<= 2"? Shall this read ">= 2"?  
> 
> I think you are right, also, reading again the manual we have the HCR_EL2.EnSCXT bit implemented only when
> FEAT_CSV2_2 is implemented or FEAT_CSV2_1p2 is implemented, otherwise it’s res0, so I think I should check:
> 
> 1) FEAT_CSV2_2 which is implemented if ID_AA64PFR0_EL1.CSV2 is 0010 (2)

... or larger than 2, since CSV2_3 includes all functionality of CSV2_2.

> 2) FEAT_CSV2_1p2 which is implemented if ID_AA64PFR1_EL1.CSV2_frac is 0010 (2)

... and ID_AA64PFR0_EL1.CSV2 is 0b0001. Also I think CSV2_frac >= 0b0010.

> Does it sounds ok for you?

Before this gets too complicated, please check two things:
1) HCR_EL2.EnSCXT is RES0 otherwise, which means we can always write this
unconditionally, without adverse effects?
2) The Armv8-R64 supplement defines minimum values for PFR0.CSV2, maybe
this simplifies some checks?

Cheers,
Andre

> >> + hcr |= HCR_EL2_ENSCXT_NOTRAP;
> >> +
> >> + if (mrs_field(ID_AA64PFR0_EL1, RAS) <= 2)  
> >
> > Same here, FEAT_RASv1p1 is 0b0010, so it should be ">= 2"?  
> 
> I’ll fix it.
> 
> Cheers,
> Luca
> 
> 