[U-Boot] [Patch v2 3/5] armv8/fsl-lsch3: Release secondary cores from boot hold off with Boot Page

[Mark Rutland]

Hi Arnab,

[...]

> >>> Is there any reason to have the SWITCH_TO_EL1 option other than for
> >>> debugging?
> >>
> >> Good question. I will let Arnab to comment here.
> >>
> >>>
> >>> EL2 is the preferred EL to boot at for Linux and Xen (it gives far
> >>> more flexibility), and if dropping to EL1 is necessary I think it
> >>> would make more sense as a run-time option than a compile-time option.
> >>>
> 
> I don't think we plan to boot Linux in EL1. This is primarily here to
> maintain uniformity with "arch/arm/lib/bootm.c". If I remove it from
> here and it was ever defined in the config, then the boot core would
> enter Linux in EL1 while the secondaries entered Linux in EL2. I don't
> know if that breaks anything...

Linux will be very unhappy, we require that CPUs enter the kernel in a
consistent mode. So keeping the CPUs in the same mode is the most
important thing for now.

I don't see a reason other than debugging to boot any CPU at EL1N if EL2
is present (so I'm not keen on the CONFIG_ARMV8_SWITCH_TO_EL1 option at
all). The ideal case would be to always drop to the highest privileged
non-secure mode the CPU supports (i.e. EL2 if present, EL1N otherwise).

If there's an OS that requires EL1N rather than EL2 then I think that
should be the special case (so users get the option of EL2 features by
default, and the OS has more flexibility to fix things up at EL2 if
necessary).

> The run-time option seems interesting and it would definitely work for
> the primary core which could access the u-boot env variables and such
> but the secondaries are executing assembly and the communication between
> cores is fairly primitive (sgi's and sev's etc) so this might require a
> little bit of work.
> 
> If you have any thoughts on how we can go about it, I would be glad to
> do some research, but that seems to be the topic for a separate patchset
> I guess.

I guess if secondaries were first dropped into a spin-table at EL2 you
could boot them into a shim that did something like:

 - Reset the cpu-release-addr for the CPU
 - Configure EL1
 - Drop to EL1
 - Return to the spin-table

So all that would be necessary is the sev and the usual polling to see
that the CPU has responded to the cpu-release-addr being written.

As you say, that's a topic for a separate patchset.

> >>>>
> >>>> -       /*
> >>>> -        * All processors will enter EL2 and optionally EL1.
> >>>> +slave_cpu:
> >>>> +       wfe
> >>>> +#ifdef CONFIG_FSL_SMP_RELEASE_ALL
> >>>> +       /* All cores are released from the address in the 1st spin
> >> table
> >>>> +        * element
> >>>>           */
> >>>> -       bl      armv8_switch_to_el2
> >>>> -#ifdef CONFIG_ARMV8_SWITCH_TO_EL1
> >>>> -       bl      armv8_switch_to_el1
> >>>> +       ldr     x1, =__spin_table
> >>>> +       ldr     x0, [x1]
> >>>> +#else
> >>>> +       ldr     x0, [x11]
> >>>> +#endif
> >>>> +       cbz     x0, slave_cpu
> >>>
> >>> Similarly is there any reason to have the option of a single release
> >>> addr if we can support unique addresses?
> >>
> >> I think it was used by Linux for some ARM parts. I personally not a fun of
> >> using single release. But if it makes everyone happy, I can keep it.
> >
> > We followed the standard ARMv8 foundation model DTS initially which along with others
> > supported a single release address for all the cores. So, we wanted to comply to the same.
> >
> 
> Yes this is left over code which should (and will) be cleaned up.

The foundation model DTS is unfortunately not a good example, and it's
too late to change it due to existing users.

Linux has supported unique cpu release addresses since the start of the
arm64 port, and it's the preferred implementation. The bootwrapper was a
quick hack to get things booting rather than a reference bootloader. It
does plenty of things wrong that I would like to fix.

I realise that this is not made clear at the moment. Putting together a
document with guidance for bootloaders has been on my TODO list, but
unfortunately it is at the far end.

[...]

> >>>> diff --git a/arch/arm/include/asm/macro.h
> >>>> b/arch/arm/include/asm/macro.h index f77e4b8..0009c28 100644
> >>>> --- a/arch/arm/include/asm/macro.h
> >>>> +++ b/arch/arm/include/asm/macro.h
> >>>> @@ -105,6 +105,98 @@ lr .req    x30
> >>>>          cbz     \xreg1, \master_label
> >>>>   .endm
> >>>>
> >>>> +.macro armv8_switch_to_el2_m, xreg1
> >>>> +       mov     \xreg1, #0x5b1  /* Non-secure EL0/EL1 | HVC | 64bit EL2
> >> */
> >>>> +       msr     scr_el3, \xreg1
> >>>
> >>> When dropping to EL1 from EL2 we disable HVC via HCR_EL2; presumably
> >>> due to lack of a handler. Would it make sense to do similarly here and
> >>> disable SMC here until we have a user (e.g. PSCI)?
> >>
> >> I will let Arnab to comment here.
> >
> 
> SMC's are disabled (we are setting bit 7, the SMD bit). The comment does
> not capture this. I'll fix it.

Ah, sorry. I attempted to review the hex values manually but evidently I
got confused here. I've just gone over the value here (0x5b1 for
SCR_EL3) with a piece of paper and an ARM ARM to hand, and it looks sane
to me.

The comment update would be nice regardless.

Thanks,
Mark.