Re: [PATCH v2 4/6] aarch64: Introduce EL2 boot code for Armv8-R AArch64

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Jul 29, 2024 at 04:27:37PM +0100, Luca Fancellu wrote:
> Hi Mark,
> 
> >> * - EL2 (Non-secure)
> >> - *   Entering at EL2 is partially supported.
> >> + *   Entering at EL2 is partially supported for Armv8-A.
> >> + *   Entering at EL2 is supported for Armv8-R.
> >
> > Nit: IIUC ARMv8-R is Secure-only, so this isn't quite right.
> 
> Ok I’ll drop this change
> 
> >
> >> *   PSCI is not supported when entered in this exception level.
> >> */
> >> ASM_FUNC(_start)
> >> @@ -76,6 +77,39 @@ reset_at_el2:
> >> msr sctlr_el2, x0
> >> isb
> >>
> >> + /* Detect Armv8-R AArch64 */
> >> + mrs x1, id_aa64mmfr0_el1
> >> + /*
> >> + * Check MSA, bits [51:48]:
> >> + * 0xf means Armv8-R AArch64.
> >> + * If not 0xf, proceed in Armv8-A EL2.
> >> + */
> >> + ubfx x0, x1, #48, #4 // MSA
> >> + cmp x0, 0xf
> >> + bne reset_no_el3
> >> +
> >> + /*
> >> + * Armv8-R AArch64 is found, check if Linux can be booted.
> >> + * Check MSA_frac, bits [55:52]:
> >> + * 0x2 means EL1&0 translation regime also supports VMSAv8-64.
> >> + */
> >> + ubfx x0, x1, #52, #4 // MSA_frac
> >> + cmp x0, 0x2
> >> + /*
> >> + * If not 0x2, no VMSA, so cannot boot Linux and dead loop.
> >> + * Also, since the architecture guarantees that those CPUID
> >> + * fields never lose features when the value in a field
> >> + * increases, we use blt to cover it.
> >> + */
> >> + blt err_invalid_arch
> >> +
> >> + /* Start Armv8-R Linux at EL1 */
> >> + mov w0, #SPSR_KERNEL_EL1
> >> + ldr x1, =spsr_to_elx
> >> + str w0, [x1]
> >
> > I'd prefer if we could do this in C code. I'll post a series shortly
> > where we'll have consistent cpu_init_arch() hook that we can do this
> > under.
> 
> Ok are you suggesting to base this serie on the one you’ll push?

Sorry; yes -- I'll send that out shortly, and I'd like to take that as a
base.

> >> +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);
> >
> > I suspect we also need to initialize VSTCR_EL2?
> 
> Ok, I’ve booted Linux and it seems to work fine, is this considered at all when HCR_EL2.VM is off?
> Anyway I’ll initialise it, I noticed it’s not done in TF-A.

I don't know; the ARMv8-R manual (ARM DDI 0600B.a) says in E1.2.3 DSB:

| The ordering requirements of Data Synchronization Barrier instruction is as
| follows:
| * EL1 and EL0 memory accesses are ordered only with respect to memory accesses
|   using the same VMID.
| * EL2 memory accesses are ordered only with respect to other EL2 memory
|   accesses.

... which seems to apply regardless of HCR_EL2.VM?

It's probably worth clarifying with the relevant architects.

> > ... and don't we also need to initialize VSCTLR_EL2 to give all CPUs the
> > same VMID? Otherwise barriers won't work at EL1 and below...
> 
> I can see TF-A is initialising it so I’ll do the same

Great; thanks!


> 
> >
> >> +
> >> + /*
> >> + * HCR_EL2.ENSCXT is written unconditionally even if in some cases it's
> >> + * RES0 (when FEAT_CSV2_2 or FEAT_CSV2_1p2 are not implemented) in order
> >> + * to simplify the code, but it's safe in this case as the write would be
> >> + * ignored when not implemented and would remove the trap otherwise.
> >> + */
> >> + hcr |= HCR_EL2_ENSCXT_NOTRAP;
> >
> > I'd prefer if we can do the necessary checks. IIUC we can do this with a
> > helper, e.g.
> >
> > static bool cpu_has_scxt(void)
> > {
> > unsigned long csv2 = mrs_field(ID_AA64PFR0_EL1, CSV2);
> > if (csv2 >= 2)
> > return true;
> > if (csv2 < 1)
> > return false;
> > return mrs_field(ID_AA64PFR1_EL1, CSV2_frac) >= 2;
> > }
> >
> > ... then here we can have:
> >
> > if (cpu_has_scxt())
> > hcr |= HCR_EL2_ENSCXT_NOTRAP;
> 
> Ok I’ll do
> 
> >
> >> +
> >> + if (mrs_field(ID_AA64PFR0_EL1, RAS) >= 2)
> >> + hcr |= HCR_EL2_FIEN_NOTRAP;
> >> +
> >> + if (cpu_has_pauth())
> >> + hcr |= HCR_EL2_APK_NOTRAP | HCR_EL2_API_NOTRAP;
> >> +
> >> + msr(hcr_el2, hcr);
> >> + isb();
> >> + msr(CNTFRQ_EL0, COUNTER_FREQ);
> >> +}
> >
> > I believe we also need to initialize:
> >
> > * CNTVOFF_EL2 (for timers to work correctly)
> > * CNTHCTL_EL2 (for timers to not trap)
> > * CPTR_EL2 (for FP to not trap)
> > * MDCR_EL2 (for PMU & debug to not trap)
> 
> Sure, I’ll reset them like in TF-A.

Perfect!

Mark.