[PATCH v2 00/17] boot: Support priority for global bootmeths

[Peter Robinson]

Hi Simon,

Welcome back!

This is quite a large series making it quite hard to review.

> At present global bootmeths always run first, before all other
> bootmeths. Optimisations in the code take advantage of this, putting
> them at the end, so they can be used once and then forgotten.

How often would you expect a device to actually want to enable and run
all boot methods? EG I would expect an organisation who's making a
general purpose devic to maybe want UEFI and extlinux, but likely not
android, rauc and some of the other ones. I would think it's would be
quite unusual for a device to enable everything and for a usecase
specific device they'd probably want just one.

> In some cases it is useful to run global bootmeths later in the boot.
> For example, the EFI-bootmgr bootmeth may itself scan devices and the
> network, so running it first can hold up the boot significantly for
> boards not actually relying on EFI-bootmgr to boot.

Exactly, would boards not relying on UEFI actually enable it at all,
similarly if a device is not ever expecting to boot off a network they
would disable it.

> This series introduces a new field in global bootmeths which indicates
> the priority, using the same scheme as is used with bootdev hunters.
> Thus it is possible to insert the EFI-bootmgr bootmeth just before the
> hunter for network bootdevs is invoked.
>
> Despite the simplicity of the concept and the relatively small series,
> this is a fairly significant enhancement. It is also quite tricky to
> implement, largely due to the way the original code was written, with
> global bootmeths being a small, size-optimised add-on to the original
> bootstd implementation.

So we know it's complex but I'm still unsure what value it provides.

> For now we only allow each global bootmeth to run at most once, but this
> implementation is written in a way that we could relax that if needed.
> Then the bootmeth itself could decide whether to run at any particular
> point in the bootflow iteration.

Why would we want to run them more than once?

> Size growth is about 390 bytes on Thumb2 (e.g. firefly-rk3288) if
> CONFIG_BOOTMETH_GLOBAL is enabled, which it normally is. With that
> disabled (which saves about 4K on the same platform), there is no
> growth.

So what value does adding 4K by default to the firmware provide? All
what makes something a global boot method vs a non global (local?)
boot method?

> Changes in v2:
> - Add a log_err() for an invalid state
> - Update commit message to indicate the bootmeth will always run
> - Document how the priority was chosen
> - Add a bit more detail suggested by Heinrich

I think this series needs to be broken down into a couple of series.

These looks like fixes mostly unrelated to and independent of the new feature:
>   boot: Improve comments related to global bootmeths
>   boot: Move obtaining the label into a common file
>   boot: Add more debugging to iter_incr()
>   boot: Move showing of bootflows out of the command
>   boot: Move preparing bootdev into a function

I think these are the bits that do the global stuff.
>   boot: Add a new test for global bootmeths
>   boot: Update first_glob_method when dropping a bootmeth
>   boot: Add a flag for whether there are global bootmeths
>   boot: Support rescanning the global bootmeths
>   boot: Only run global bootmeths once each
>   boot: Implement a priority for global bootmeths
>   boot: Don't change the method count after global bootmeths
>   boot: Run global bootmeths after all bootdevs are exhausted
>   boot: Run the EFI bootmgr just before network devices
>   boot: doc: Update for new global-bootmeth features

Why do we want this? What does all bootmeths mean in this context?
>   boot: Try all bootmeths on the final partition

As I, and at least Tom, but I think others, have asked in the past
please do smaller patchsets on single topics. If you need to do
cleanups post them as a different dependent patch set.

You have complained in the past that people don't review your series
fast enough, but doing large sets like this with an inter mix of fixes
and enhancements make it hard for people to review, eg I put this in
my back log because of that exactly.

Regards,
Peter

>  boot/bootflow.c                  | 318 ++++++++++++++++++++++++++++---
>  boot/bootmeth-uclass.c           |  15 +-
>  boot/bootmeth_efi_mgr.c          |   9 +
>  cmd/bootflow.c                   |  66 +------
>  doc/develop/bootstd/overview.rst |  40 +++-
>  include/bootflow.h               |  39 +++-
>  include/bootmeth.h               |   4 +
>  test/boot/bootflow.c             |  75 +++++++-
>  8 files changed, 457 insertions(+), 109 deletions(-)
>
> --
> 2.43.0
>
> base-commit: aff68c8514858ddd8d2e508d47bede566511521b
> branch: glob-us2