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: 6752 bytes --]

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.

-- 
Tom

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