Re: [PATCH 7/7] boot: add a minimal bootmeth for the Boot Loader Specification

[Tom Rini <trini@konsulko.com>]

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

On Mon, Jun 29, 2026 at 06:45:07AM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 26 Jun 2026 at 14:47, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Jun 26, 2026 at 11:45:22AM +0100, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 25 Jun 2026 at 18:33, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Jun 25, 2026 at 06:29:18PM +0100, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Thu, 25 Jun 2026 at 18:24, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Thu, Jun 25, 2026 at 08:57:13PM +0400, Alexey Charkov wrote:
> > > > > > > On Thu, Jun 25, 2026 at 8:24 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Alexey,
> > > > > > > >
> > > > > > > > On Thu, 25 Jun 2026 at 16:52, Alexey Charkov <alchark@flipper.net> wrote:
> > > > > > > > >
> > > > > > > > > Hi Simon,
> > > > > > > > >
> > > > > > > > > On Thu, Jun 25, 2026 at 7:28 PM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Alexey,
> > > > > > > > > >
> > > > > > > > > > On 2026-06-04T15:31:05, Alexey Charkov <alchark@flipper.net> wrote:
> > > > > > > > > > > boot: add a minimal bootmeth for the Boot Loader Specification
> > > > > > > > > > >
> > > > > > > > > > > Add a bootmeth that finds and boots Boot Loader Specification (BLS)
> > > > > > > > > > > type #1 entry files [1]. On each block-device partition it scans, the
> > > > > > > > > > > bootmeth looks for files matching '<prefix>loader/entries/*.conf'
> > > > > > > > > > > (where <prefix> comes from bootstd_get_prefixes(), typically '/' and
> > > > > > > > > > > '/boot/'), picks the highest-sorting filename, parses it, and exposes
> > > > > > > > > > > it as a bootflow.
> > > > > > > > > > >
> > > > > > > > > > > Implementation reuses the existing pxelinux infrastructure.
> > > > > > > > > > >
> > > > > > > > > > > For now the entry chosen on a partition is purely the lexicographic
> > > > > > > > > > > maximum of *.conf filenames; sort-key / version field handling
> > > > > > > > > > > (spec-mandated tiebreakers) and boot-counting (the '+TRIES_LEFT'
> > > > > > > > > > > filename suffix) are left as TODOs. Likewise, only the top-sorted entry
> > > > > > > > > > > is surfaced because the bootstd framework currently allows one bootflow
> > > > > > > > > > > per (bootmeth, partition); exposing every discovered entry will require
> > > > > > > > > > > a framework extension.
> > > > > > > > > > >
> > > > > > > > > > > Type #2 BLS (drop-in directory of EFI binaries) is out of scope here;
> > > > > > > > > > > [...]
> > > > > > > > > > >
> > > > > > > > > > > boot/Kconfig        |  17 +++
> > > > > > > > > > >  boot/Makefile       |   1 +
> > > > > > > > > > >  boot/bootmeth_bls.c | 333 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > >  3 files changed, 351 insertions(+)
> > > > > > > > > >
> > > > > > > > > > > diff --git a/boot/Kconfig b/boot/Kconfig
> > > > > > > > > > > @@ -649,6 +649,23 @@ config BOOTMETH_EXTLINUX_PXE
> > > > > > > > > > > +config BOOTMETH_BLS
> > > > > > > > > > > +     bool "Bootdev support for Boot Loader Specification entries"
> > > > > > > > > > > +     select PXE_UTILS
> > > > > > > > > > > +     default y
> > > > > > > > > >
> > > > > > > > > > Wherever the 'default y' discussion lands, this will be enabled on
> > > > > > > > > > sandbox, so the 'bootmeth list' tests in test/boot/bootmeth.c will
> > > > > > > > > > fail since they check the exact list and count. Please can you run the
> > > > > > > > > > sandbox tests and update them as needed?
> > > > > > > > > >
> > > > > > > > > > Since you are adding a new bootmeth you also need a sandbox test that
> > > > > > > > > > exercises it (see the extlinux tests in test/boot/bootflow.c, with a
> > > > > > > > > > fixture disk image) and a documentation page, e.g.
> > > > > > > > > > doc/develop/bootstd/bls.rst with an entry in the index there.
> > > > > > > > >
> > > > > > > > > Will do, thanks!
> > > > > > > > >
> > > > > > > > > > > diff --git a/boot/bootmeth_bls.c b/boot/bootmeth_bls.c
> > > > > > > > > > > @@ -0,0 +1,333 @@
> > > > > > > > > > > +static int bls_getfile(struct pxe_context *ctx, const char *file_path,
> > > > > > > > > > > +                    char *file_addr, enum bootflow_img_t type, ulong *sizep)
> > > > > > > > > >
> > > > > > > > > > This is a verbatim copy of extlinux_getfile() - please can you export
> > > > > > > > > > that, or move it into a shared helper?
> > > > > > > > >
> > > > > > > > > Ack
> > > > > > > > >
> > > > > > > > > > > diff --git a/boot/bootmeth_bls.c b/boot/bootmeth_bls.c
> > > > > > > > > > > @@ -0,0 +1,333 @@
> > > > > > > > > > > +     ret = bootmeth_alloc_file(bflow, SZ_64K, 1, BFI_EXTLINUX_CFG);
> > > > > > > > > >
> > > > > > > > > > The extlinux bootmeth passes ARCH_DMA_MINALIGN here, since the buffer
> > > > > > > > > > is filled by a block-device read which may use DMA on some platforms.
> > > > > > > > > > An alignment of 1 risks cache-line corruption there, so please use
> > > > > > > > > > ARCH_DMA_MINALIGN.
> > > > > > > > >
> > > > > > > > > Ack
> > > > > > > > >
> > > > > > > > > > > diff --git a/boot/bootmeth_bls.c b/boot/bootmeth_bls.c
> > > > > > > > > > > @@ -0,0 +1,333 @@
> > > > > > > > > > > +     bflow->bootmeth_priv = label;
> > > > > > > > > > > +     free(fpath);
> > > > > > > > > >
> > > > > > > > > > This leaks memory: bootflow_free() releases bootmeth_priv with a plain
> > > > > > > > > > free(), so the label's string members (name, menu, kernel, append,
> > > > > > > > > > initrd, etc.) are never freed - for every bootflow discarded after a
> > > > > > > > > > scan, not just on error paths.
> > > > > > > > > >
> > > > > > > > > > Since get_string() copies tokens out of the buffer rather than
> > > > > > > > > > modifying it in place, you could follow the extlinux approach: keep
> > > > > > > > > > only bflow->buf across the scan (already populated by
> > > > > > > > > > bootmeth_alloc_file() and freed by the framework), use a temporary
> > > > > > > > > > label in bls_read_bootflow() just to extract the title, destroy it
> > > > > > > > > > with label_destroy(), and re-parse the buffer in bls_boot(). Then
> > > > > > > > > > bootmeth_priv is not needed at all. What do you think?
> > > > > > > > >
> > > > > > > > > Will address the leak, thanks for pointing it out!
> > > > > > > > >
> > > > > > > > > The bls_priv structure is needed for my follow-up extension which
> > > > > > > > > allows multiple boot entries to be returned by each (bootdev,
> > > > > > > > > bootmeth, partition) tuple, and I wanted to minimize churn between
> > > > > > > > > those two. I haven't sent that follow-up for review yet, as I wanted
> > > > > > > > > to confirm this simpler version is acceptable first.
> > > > > > > >
> > > > > > > > We should implement this using an generic index rather than something
> > > > > > > > bls-specific...please see some commits at:
> > > > > > > >
> > > > > > > > https://concept.deinde.dev/u-boot/u-boot/-/merge_requests/782
> > > > > > >
> > > > > > > Oh, there's already BLS in your tree :-D
> > > > > > >
> > > > > > > Would you like to merge that instead? Your version looks much more
> > > > > > > full-fledged. If you happen to have one rebased on master or next I'd
> > > > > > > be happy to test!
> > > > > >
> > > > > > Unfortunately Simon's work here is AI-generated and so not particularly
> > > > > > welcome, among other problems right now.
> > > > >
> > > > > No, not AI-generated, but certainly AI-assisted (perhaps that is what
> > > > > you meant?)
> > > >
> > > > I didn't dig back super far to see what all AI-generated (such as your
> > > > dlmalloc upgrade) vs "AI-assisted" (commit from just a few months ago)
> > > > and try and guess based on the quality if it's "generated" or
> > > > "assisted". But it doesn't really matter in the AI context. And it
> > > > certainly doesn't matter until you've donated u-boot.org to the project.
> > >
> > > Yes the dlmalloc upgrade was a lot of work and brings a lot of
> > > benefits to U-Boot. There is also a backtrace feature which is really
> > > handlyfor tracking down memory leaks and hangs. I found a ton of
> > > leaks, some of them very large (e.g. SCMI). There was certainly a lot
> > > of manual work involved, along with AI assistance. I believe AI is
> > > particularly effective when used by someone who knows the project and
> > > codebase well.
> >
> > OK, but did you review that dlmalloc upgrade? Did you think Claude did a
> > good and appropriate job there?
> 
> It's a while ago, but I remember being about 3 bugs deep at the time
> (crashes in CI which turned out to be memory leaks) so I am sure it
> could be better. If you are interested in it for mainline I could take
> another look.

Really, I think that just adds to the list of reasons why these "tools"
are so terrible. The series itself is disrespectful to the community, as
it kept the commit messages *and* review tags of the actual humans that
originally did work while the changes being done almost never[1] had any
clear relationship to what was originally done. And as a "cherry on
top", tracking down what one of the original commits even is was not
easy as Claude rewrote the last third or so of the git hash.

So no, it would not be "take another look", it would be "throw it out
and do it correctly".

[1]: Of what I was cc'd one, which was a reasonable number, in only one
case was the original change, and the new change doing the same thing as
before in a clear manner that the commit message was still certainly
correct, and the remaining change was the same as before, so if a human
did it, it would have been considered reasonable to do with an
explanation below the "---".

-- 
Tom

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