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

[Andre Przywara <andre.przywara@arm.com>]

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
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel