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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi Alexey, > > > > > > > > > > > > > > > > On Thu, 25 Jun 2026 at 16:52, Alexey Charkov > > > > > > > > <[email protected]> wrote: > > > > > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > > > > > On Thu, Jun 25, 2026 at 7:28 PM Simon Glass > > > > > > > > > <[email protected]> wrote: > > > > > > > > > > > > > > > > > > > > Hi Alexey, > > > > > > > > > > > > > > > > > > > > On 2026-06-04T15:31:05, Alexey Charkov > > > > > > > > > > <[email protected]> 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
signature.asc
Description: PGP signature
