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

[Luca Fancellu]

Hi Andre,


> On 24 Jun 2024, at 13:22, Luca Fancellu <Luca.Fancellu at arm.com> wrote:
> 
> 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?
> 
> 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
> 
> 

Gentle ping on this...


>> 
>>> #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)
> 2) FEAT_CSV2_1p2 which is implemented if ID_AA64PFR1_EL1.CSV2_frac is 0010 (2)
> 
> Does it sounds ok for you?

And this.

Cheers,
Luca