Re: [PATCH v3 00/19] bootstd: Support recording images

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 7197 bytes --]

On Wed, Jan 15, 2025 at 04:14:27PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 15 Jan 2025 at 14:24, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Jan 15, 2025 at 06:55:45AM -0700, Simon Glass wrote:
> > > Hi,
> > >
> > > On Mon, 4 Nov 2024 at 10:51, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > This series provides a way to keep track of the images used in bootstd,
> > > > including the type of each image.
> > > >
> > > > At present this is sort-of handled by struct bootflow but in quite an
> > > > ad-hoc way. The structure has become quite large and is hard to query.
> > > > Future work will be able to reduce its size.
> > > >
> > > > Ultimately the 'bootflow info' command may change to also show images as
> > > > a list, but that is left for later, as this series is already fairly
> > > > long. So for now, just introduce the concept and adjust bootstd to use
> > > > it, with a simple command to list the images.
> > > >
> > > > This series includes various alist enhancements, to make use of this new
> > > > data structure a little easier.
> > > >
> > > > Changes in v3:
> > > > - Add a helper in bootflow
> > > > - Add a new patch supporting ad-hoc bootflows
> > > > - Add new patch to export bootdev_get_from_blk()
> > > > - Add new patch to record loaded files in an ad-hoc bootflow
> > > > - Update bootstd_add_bootflow() to return the element number
> > > > - Use the helper function
> > > >
> > > > Changes in v2:
> > > > - Add an image type extension in bootflow.h
> > > > - Change the tag to bootmeth_efi
> > > > - Drop patches already applied
> > > > - Drop patches which add new image types
> > > > - Update to use a new image-type enum in bootflow.h
> > > > - Use the word 'feature' instead of 'hack'
> > > >
> > > > Simon Glass (19):
> > > >   bootstd: Move bootflow-adding to bootstd
> > > >   bootstd: Move bootflow-clearing to bootstd
> > > >   bootstd: Add a function to get bootstd only if available
> > > >   bootstd: Drop the bootdev-specific list of bootflows
> > > >   bootstd: Move the bootflow list into an alist
> > > >   bootstd: Maintain a list of images
> > > >   bootstd: Update bootmeth_alloc_file() to record images
> > > >   boot: pxe: Drop the duplicate comment on get_pxe_file()
> > > >   bootmeth_efi: Simplify reading files by using the common function
> > > >   bootmeth: Update the read_file() method to include a type
> > > >   bootmeth_efi: Check the filename-allocation in the network path
> > > >   boot: Update extlinux pxe_getfile_func() to include type
> > > >   boot: Update pxe bootmeth to record images
> > > >   Update bootmeth_alloc_other() to record images
> > > >   bootstd: Update cros bootmeth to record images
> > > >   bootstd: Add a simple command to list images
> > > >   bootstd: Export bootdev_get_from_blk()
> > > >   bootstd: Add the concept of an ad-hoc bootflow
> > > >   fs: Record loaded files in an ad-hoc bootflow
> > > >
> > > >  boot/bootdev-uclass.c            |  78 ++++++-------------
> > > >  boot/bootflow.c                  |  77 +++++++++++++++----
> > > >  boot/bootmeth-uclass.c           |  29 +++++--
> > > >  boot/bootmeth_android.c          |   3 +-
> > > >  boot/bootmeth_cros.c             |  17 ++++-
> > > >  boot/bootmeth_efi.c              |  16 ++--
> > > >  boot/bootmeth_efi_mgr.c          |   3 +-
> > > >  boot/bootmeth_extlinux.c         |   7 +-
> > > >  boot/bootmeth_pxe.c              |  10 ++-
> > > >  boot/bootmeth_qfw.c              |   3 +-
> > > >  boot/bootmeth_sandbox.c          |   3 +-
> > > >  boot/bootmeth_script.c           |   7 +-
> > > >  boot/bootstd-uclass.c            | 104 +++++++++++++++++++++++--
> > > >  boot/pxe_utils.c                 |  36 ++++-----
> > > >  boot/vbe_simple.c                |   5 +-
> > > >  cmd/Kconfig                      |   9 +++
> > > >  cmd/Makefile                     |   1 +
> > > >  cmd/bootdev.c                    |   2 +-
> > > >  cmd/bootflow.c                   |  11 +--
> > > >  cmd/bootstd.c                    |  65 ++++++++++++++++
> > > >  cmd/pxe.c                        |   2 +-
> > > >  cmd/sysboot.c                    |   6 +-
> > > >  doc/develop/bootstd/overview.rst |  24 +++++-
> > > >  doc/usage/cmd/bootstd.rst        |  79 +++++++++++++++++++
> > > >  doc/usage/index.rst              |   1 +
> > > >  fs/fs.c                          |  15 ++++
> > > >  include/bootdev.h                |  36 +++------
> > > >  include/bootflow.h               |  88 ++++++++++++++++++---
> > > >  include/bootmeth.h               |  22 ++++--
> > > >  include/bootstd.h                |  69 ++++++++++++++++-
> > > >  include/pxe_utils.h              |  14 +++-
> > > >  test/boot/bootflow.c             | 126 +++++++++++++++++++++++++++++++
> > > >  test/py/tests/test_ut.py         |   3 +-
> > > >  33 files changed, 788 insertions(+), 183 deletions(-)
> > > >  create mode 100644 cmd/bootstd.c
> > > >  create mode 100644 doc/usage/cmd/bootstd.rst
> > > >
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > patchwork link: https://patchwork.ozlabs.org/project/uboot/list/?series=433068
> > >
> > > What is the status of this series please?
> >
> > So, for v4 of this series, most arm64 platforms grow by ~1KiB (a few are
> > 2, a few are a bit less than 1). 32bit ARM is similar but now mx6sabresd
> > doesn't build in CI (there's some embedded path names that now overflow,
> > try make O=/tmp/1/2/3/4/5/6/7/8/9/10/mx6sabresd mx6sabresd_config all).
> > For size changes, really most architectures are like that, so perhaps
> > it's worth stepping back again and asking where this should be default
> > enabled. For the mx6sabresd part, if we drop the recording for ad-hoc
> > (which I think you had suggested at some point since I wasn't sure about
> > that direction), everything builds at least.
> 
> Thanks for looking at it. So what do you think about dropping those
> last two patches and applying the rest? The increase is about 416
> bytes on pinecube (Thumb2). Quite a bit of this seems to be bringing
> alist which is something of a one-off hit:
> 
>                  alist_ensure_ptr                             -      64     +64
>                  alist_calc_index                             -      32     +32
>                  alist_add_ptr                                -      26     +26
>                  alist_update_end                             -      18     +18
>                  alist_empty                                  -       6      +6
>                  alist_add_placeholder                        -       6      +6
> 
> Then I can use the final two patches as the start of the series to
> finish off ad-hoc images.

I was thinking just patch #20, but we can drop 19 and 20 instead, sure.

> As to default-enabled, moving bootflow to an alist isn't really
> something we can disable.

I'm referring to the whole bootmeth/bootflow/etc thing. Is this going to
be put to use by anyone / anywhere on PowerPC? M68K? MIPS? 32bit ARM is
a harder question there, which is also where this particular overflow
was.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]