Re: [PATCH 23/34] bootstd: Maintain a list of images

[Tom Rini <trini@konsulko.com>]

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

On Tue, Oct 22, 2024 at 02:16:52PM +0200, Simon Glass wrote:
> Hi Tom,
> 
> On Sat, 19 Oct 2024 at 18:30, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sat, Oct 19, 2024 at 05:49:40AM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 18 Oct 2024 at 15:30, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Fri, Oct 18, 2024 at 12:48:31PM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Fri, 18 Oct 2024 at 12:04, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Fri, Oct 18, 2024 at 11:20:52AM -0600, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Fri, 18 Oct 2024 at 10:33, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Fri, Oct 18, 2024 at 09:01:03AM -0600, Simon Glass wrote:
> > > > > > > > > Hi Heinrich,
> > > > > > > > >
> > > > > > > > > On Thu, 17 Oct 2024 at 22:07, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Am 18. Oktober 2024 01:24:02 MESZ schrieb Simon Glass <sjg@chromium.org>:
> > > > > > > > > > >We want to keep track of images which are loaded, or those which could
> > > > > > > > > > >perhaps be loaded. This will make it easier to manage memory allocation,
> > > > > > > > > > >as well as permit removal of the EFI set_efi_bootdev() hack.
> > > > > > > > >
> > > > > > > > > I'll change this 'hack' to 'feature'.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Please, keep in mind that files can be loaded manually, e.g. via the dhcp, the wget, and the loady commands. These are outside bootflows.
> > > > > > > > >
> > > > > > > > > Yes, this series is only going to help if bootstd is used. For ad-hoc
> > > > > > > > > use, EFI will need to rely on the above feature, at least until
> > > > > > > > > someone can think of another solution.
> > > > > > > >
> > > > > > > > Perhaps I need to try and be clearer here than I might have been in the
> > > > > > > > past. The consensus among off the shelf free software operating systems
> > > > > > > > is "just give me an EFI interface". This simplifies things on their end
> > > > > > > > if regardless of architecture it's the same interface. This means that
> > > > > > > > in U-Boot we need to treat EFI as one of the primary interfaces. Not a
> > > > > > > > novelty. Not a "some people might use". It is a frequent and commonly
> > > > > > > > used feature.
> > > > > > >
> > > > > > > Yes, EFI is everywhere and growing. All the more reason to tidy up
> > > > > > > this piece. I would like to see bootmgr use this new API, for example.
> > > > > > >
> > > > > > > But how does this comment affect this patch?
> > > > > >
> > > > > > Because at the very high level, I wonder if I made a mistake a few years
> > > > > > back. As I understand it, the nominal case is "bootefi bootmgr". I was
> > > > > > saying at the time that perhaps bootstd can just fire that off, and move
> > > > > > on. Now it seems like we're going along the path of re-inventing that,
> > > > > > and not integrating well with it either.
> > > > >
> > > > > In what way are we re-inventing that? bootstd supports lots of
> > > > > different ways of booting, not just EFI.
> > > >
> > > > At the high level, bootflow scan is re-implementing "bootefi bootmgr".
> > > > but handling non-EFI payloads.
> > >
> > > bootstd is about replacing the distro scripts, not bootmgr.
> >
> > And the distro scripts are functionally replaced by "bootefi bootmgr",
> > outside of bootstd.
> 
> But that doesn't support anything other than EFI apps, so isn't useful
> for when EFI is not wanted.

Yes, the distro scripts wanted to move more firmly / quickly in this
direction. For all of the reasons they've elaborated before.

> > > > > Also I hope that one day EFI
> > > > > will be implemented more as part of U-Boot than as a bolt-on, so will
> > > > > make use of bootflows, etc.
> > > >
> > > > And stuff like that is why I said what I said in here first. To me it
> > > > sounds like you keep implying it's a hack that's not well integrated.
> > > > When it's honestly at this point gotten more traction than FIT images
> > > > have I think (as much as I wish FIT images had "won", it's like VHS vs
> > > > Betamax, to bring in another technology metaphor).
> > >
> > > The 'hack' I was referring to is efi_set_bootdev(), not EFI_LOADER as a whole!
> >
> > I wasn't clear enough, sorry. I didn't mean just in this series where
> > you referred to storing the needed property as a hack but rather
> > "bolt-on" in what I quoted and "tidy up this" and "tidy up that". I'm
> > just saying what impression your words leave with me, and quite possibly
> > others.
> 
> OK I will try to be more gentle with my language.

Thanks.

> > > > > > So, to try and bring things back together. If U-Boot decides to load
> > > > > > $FOO from device $BAR, at that common point is where we need to:
> > > > > > - Is there an lmb for the location this is supposed to go to (for the if
> > > > > >   we know it, entire size)?
> > > > > > - Note down everything else we know, now.
> > > > >
> > > > > Yes.
> > > > >
> > > > > >
> > > > > > This means that we can note down enough stuff so that EFI can construct
> > > > > > the path it needs. And if we're being told a filesystem, that filename
> > > > > > is good enough for the IH_TYPE thing you're wanting, or at least a good
> > > > > > chunk of it I hope.
> > > > >
> > > > > You want me to ignore the type that I know (kernel, ramdisk, logo,
> > > > > etc.) and infer the type from a filename? Why?
> > > >
> > > > No, I want you to save and display the filename. That's probably much
> > > > more useful when debugging than "kernel". If you actually know some
> > > > other type information (ie extlinux.conf says ...) then yes, it too can
> > > > be stored as that's useful too.
> > >
> > > The filename is already saved in bootflow->filename, and now it is in
> > > struct bootflow_img.
> >
> > OK. But that's not generic enough.
> 
> How about we see how things work out here rather than giving up at the
> start? It is pretty clear in my head, so far.

I'd really rather not since it looks like it's starting in the wrong
direction. I really do not understand why when we load the file / do the
network request / read the flash area / etc is the wrong place to start
recording the information about what we load.

> > > > > For EFI there is only an EFI application. It will always just be a PE
> > > > > file. We don't really know what it is, as someone pointed out earlier.
> > > > > Maybe one day we will check to see if it is a UKI and pull things out
> > > > > of it. But then, it would be component parts (kernel, ramdisk, etc.)
> > > > > so I would want to add them as images.
> > > >
> > > > I don't see why yet, honestly.
> > >
> > > For the cmdline, 'bootflow cmdline' allows editing it, for example.
> > > For a logo we can display it in the menu. The filename doesn't tell us
> > > what it is.
> >
> > There may, or may not, be further context clues about what a blob is,
> > yes, and we should make use of them. But we may not have them. Frankly
> > for a logo we probably need to have dealt with that upon initializing
> > the display, so earlier in the process. But this is all getting away
> > from the point I'm trying to make.
> >
> > > > > > It also means that since it's at the most common point, it doesn't
> > > > > > matter if we were in an EFI application, a boot script, a bootmeth or
> > > > > > someone at the cmdline doing "load mmc 0:1 /boot/Image $kernel_addr_r".
> > > > >
> > > > > For that case (at the cmdline), bootstd is not currently running. Are
> > > > > you suggesting that bootstd could pick up these things and record
> > > > > them? If so, then yes, definitely, I want to do that. This series is
> > > > > the starting point for that. If you are suggesting something else,
> > > > > then I think I have lost you at this point.
> > > >
> > > > Yes, I think I lost you somewhere, but I'm not sure how. What I am
> > > > saying is that since everything at some point calls down to say
> > > > fs_read() to read a file, that is the common point to note what
> > > > we're doing. Not the load command, not the bootmeth, not the EFI_LOADER
> > > > call.
> > >
> > > Perhaps what you are missing is that bootstd is a proper boot
> > > implementation for U-Boot, where U-Boot knows what is going on.
> > > Ignoring that information and just hooking into fs_read() is like
> > > throwing away the plan and trying to recreate it from photographs.
> > >
> > > That said, yes I can hook into fs_read() and store that info, as it is
> > > better than nothing, if bootstd is not being used, or there is a
> > > script.
> >
> > Maybe we need to throw out the plan and start over then? Since you seem
> > to not be accounting for the rest of the common paths and assuming
> > that your design can just be used as-is for them despite not taking them
> > in to account in the design in the first place.  Hooking in at around
> > fs_read() or thereabouts is likely where we need to be at least starting
> > some of this so that we do know information that we want to preserve.
> > Similarly, there's the common path for "grab a file via HTTP(s)" and
> > that is where we should be recording information for network loads. We
> > can then augment that if it was a pxelinux.cfg file we grabbed or
> > something else, but that's the common place to start gathering
> > information.
> 
> Here's what I see as the key point: bootstd knows what is being
> booted. It knows that the kernel is a kernel and where it came from.
> If there is a logo or cmdline, it knows about that too. It is sort-of
> the point of bootstd to make booting more automatic and deterministic.
> Where people are using scripts and ad-hoc ways of booting, they just
> aren't going to get the same level of control and visibility as
> bootstd can provide.
> 
> I think I recall having exactly this discussion before?
> 
> I will see if I can construct a small series to show what an ad-hoc
> bootflow might look like. It will at least collect loaded files into
> one place so they can be examined, etc.

Yes, perhaps some series that shows what works / doesn't work and how it
works would be helpful. I'd really like to see where you're starting
your abstractions from.

-- 
Tom

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