Re: [bootwrapper PATCH v2 00/13] Cleanups and improvements

Andre Przywara
2022-01-14T15:11:20Z

On Fri, 14 Jan 2022 10:56:40 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

Hi Mark,

> This series reworks the aarch64 boot-wrapper, moving a fair amount of
> initialization code to C. This has a few benefits:
>
> 1) This makes the code more legible and easier to maintain.
>
> Current feature detection and system register configuration is
> written in assembly, requiring runs of *almost* identical blocks of
> assembly to read ID registers and conditionally initialize register
> values. This requires a bunch of labels (which are all named
> numerically), and all the magic numbers are hard coded, so this gets
> pretty painful to read:
>
> | mrs x1, id_aa64isar0_el1
> | ubfx x1, x1, #24, #4
> | cbz x1, 1f
> |
> | orr x0, x0, #(1 << 34) // TME enable
> |
> | 1:
>
> In C, it's much easier to add helpers which use mnemonics, which
> makes the code much easier to read, and avoids the need to manually
> allocate registers, etc:
>
> | if (mrs_field(ID_AA64ISAR0_EL1, TME))
> | scr |= SCR_EL3_TME;
>
> This should make it easier to handle new architectural extensions
> (and/or architecture variants such as ARMv8-R) in future.
>
> 2) This allows for better diagnostics.
>
> Currently a mis-configured boot-wrapper is rather unforgiving, and
> provides no indication as to what might have gone wrong. By moving
> initialization to C, we can make use to the UART code to log
> diagnostic information, and we can more easily add additional error
> handling and conditional logic.
>
> This series adds diagnostic information and error handling that can
> be used to identify problems such as the boot-wrapper being launched
> at the wrong exception level:
>
> | Boot-wrapper v0.2
> | Entered at EL2
> | Memory layout:
> | [0000000080000000..0000000080001f90] => boot-wrapper
> | [000000008000fff8..0000000080010000] => mbox
> | [0000000080200000..00000000822af200] => kernel
> | [0000000088000000..0000000088002857] => dtb
> |
> | WARNING: PSCI could not be initialized. Boot may fail
>
> Originally I had planned for this to be a more expansive set of changes,
> unifying some early control-flow, fixing some latent bugs, and making
> the boot-wrapper dynamically handle being entered at any of EL{3,2,1}
> with automated selection of a suitable DT. As the series has already
> become pretty long, I'd like to get this preparatory cleanup out of the
> way first, and handle those cases with subsequent patches.
>
> If there are no objections, I intend to apply these by the end of the
> day.

Can you hold back with that till the beginning of next week? I got stuck
halfway in the series with my review, but am on it now as we speak.

Cheers,
Andre

>
> I've pushed the series to the `cleanup` branch in the boot-wrapper repo:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mark/boot-wrapper-aarch64.git/
> git://git.kernel.org/pub/scm/linux/kernel/git/mark/boot-wrapper-aarch64.git
>
> ... and it should apply cleanup atop the `master` branch.
>
> Since v1 [1]:
> * Add comments for bit-field macros
> * Use BIT() for *_RES1 definitions
> * Complete start_el3/start_no_el3 cleanup
> * Remove extraneous macro definition
>
> [1] https://lore.kernel.org/r/20220111130653.2331827-1-mark.rutland@arm.com/
>
> Thanks,
> Mark.
>
> Mark Rutland (13):
> Document entry requirements
> Add bit-field macros
> aarch64: add system register accessors
> aarch32: add coprocessor accessors
> aarch64: add mov_64 macro
> aarch64: initialize SCTLR_ELx for the boot-wrapper
> Rework common init C code
> Announce boot-wrapper mode / exception level
> aarch64: move the bulk of EL3 initialization to C
> aarch32: move the bulk of Secure PL1 initialization to C
> Announce locations of memory objects
> Rework bootmethod initialization
> Unify start_el3 & start_no_el3
>
> Makefile.am | 6 +-
> arch/aarch32/boot.S | 33 +++---
> arch/aarch32/include/asm/cpu.h | 62 ++++++++---
> arch/aarch32/include/asm/gic-v3.h | 6 +-
> arch/aarch32/include/asm/psci.h | 28 +++++
> arch/aarch32/init.c | 42 ++++++++
> arch/aarch32/psci.S | 16 +--
> arch/aarch32/utils.S | 9 --
> arch/aarch64/boot.S | 169 +++++++++++++-----------------
> arch/aarch64/common.S | 10 +-
> arch/aarch64/include/asm/cpu.h | 102 +++++++++++++++---
> arch/aarch64/include/asm/gic-v3.h | 10 +-
> arch/aarch64/include/asm/psci.h | 28 +++++
> arch/aarch64/init.c | 88 ++++++++++++++++
> arch/aarch64/psci.S | 22 +---
> arch/aarch64/spin.S | 6 +-
> arch/aarch64/utils.S | 9 --
> common/boot.c | 4 -
> common/init.c | 60 +++++++++++
> common/platform.c | 45 +++++---
> common/psci.c | 22 +++-
> include/bits.h | 79 ++++++++++++++
> include/boot.h | 2 +
> include/platform.h | 20 ++++
> model.lds.S | 20 ++--
> 25 files changed, 668 insertions(+), 230 deletions(-)
> create mode 100644 arch/aarch32/include/asm/psci.h
> create mode 100644 arch/aarch32/init.c
> create mode 100644 arch/aarch64/include/asm/psci.h
> create mode 100644 arch/aarch64/init.c
> create mode 100644 common/init.c
> create mode 100644 include/bits.h
> create mode 100644 include/platform.h