[PATCH 15/34] image: Add a new type for extlinux

[Simon Glass]

Hi Tom,

On Fri, 18 Oct 2024 at 11:54, Tom Rini <trini at konsulko.com> wrote:
>
> On Fri, Oct 18, 2024 at 11:20:45AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 18 Oct 2024 at 09:17, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Fri, Oct 18, 2024 at 08:57:02AM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 17 Oct 2024 at 21:14, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Thu, Oct 17, 2024 at 05:23:54PM -0600, Simon Glass wrote:
> > > > >
> > > > > > Add an image type for the extlinux.cfg file, which U-Boot supports
> > > > > > reading.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > ---
> > > > > >
> > > > > >  boot/image.c    | 1 +
> > > > > >  include/image.h | 1 +
> > > > > >  2 files changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/boot/image.c b/boot/image.c
> > > > > > index abac254e026..cf16fb7fbf7 100644
> > > > > > --- a/boot/image.c
> > > > > > +++ b/boot/image.c
> > > > > > @@ -183,6 +183,7 @@ static const table_entry_t uimage_type[] = {
> > > > > >       {       IH_TYPE_FDT_LEGACY, "fdt_legacy", "legacy Image with Flat Device Tree ", },
> > > > > >       {       IH_TYPE_RENESAS_SPKG, "spkgimage", "Renesas SPKG Image" },
> > > > > >       {       IH_TYPE_STARFIVE_SPL, "sfspl", "StarFive SPL Image" },
> > > > > > +     {       IH_TYPE_EXTLINUX_CFG, "extlinux_cfg", "Extlinux configuration" },
> > > > > >       {       -1,                 "",           "",                   },
> > > > > >  };
> > > > >
> > > > > This whole part of the series feels like we're abusing IH_TYPE_ far past
> > > > > what it's used for. Especially the command line string one. It seems
> > > > > like the main use is to be able to later on print something human
> > > > > friendly. Please try and figure out some other way to do that. Thanks.
> > > >
> > > > Yes I had the same thought, particularly with cmdline, as you say.
> > > >
> > > > The obvious alternative is to just have a string which communicates
> > > > the type. Then I can use IH_TYPE_INVALID, perhaps, with an extra
> > > > string indicating what it really is.
> > > >
> > > > But one advantage of the approach in this patch is that, for bootmeths
> > > > which include a cmdline, it can be identified and added as a file.
> > > > That includes zimage and ChromiumOS, but not EFI. It will allow me to
> > > > get rid of the cmdline in struct bootflow, perhaps.
> > >
> > > Yeah, this all sounds like IH_TYPE is the wrong direction to expand.
> > > Where you're grabbing these things from is where you might get a hint as
> > > to what these things are and that's where to take the store the
> > > knowledge for later.
> >
> > They are grabbed from several places:
> > - extlinux comes from disk, with the extlinux bootmeth, but can also
> > come from 'include' files within that scripts, also pxe
> > - EFI apps from from disk with the EFI bootmeth
> > - kernel, ramdisk and fdt some from most bootmeths
> > - cmdline comes from extlinux (with zImage only) and ChromeOS,
> > probably Android but I haven't looked
> > - logo comes just from script so far, but I suppose UKI and FIT will
> > have it eventually
> >
> > Can you be more specific with your suggestion? I am not seeing an
> > alternative right now.
>
> I'm just saying that this is not what IH_TYPE is. As you said maybe you
> can store this string information elsewhere? And I'm suggesting that
> perhaps when you load a thing you'll then also know what the thing is.

Yes, I agree. 'Type' is really about the actual file that you are
reading, not the data types of the bits within it, for example. That's
the problem with my approach.

The other idea I had was to add a new thing to image.h (IH_DATA_xxx).
But I wasn't sure it was worth it. Also, if you look at
fit_image_load() it reads an image of a certain type out of an
existing (containing) FIT. So we already have images within images.

How about I just put a data type in bootflow.h ? Then it will be
separate from image.h and I'm not blurring the line any further
between an image and its component parts? We can always change it
later if we find a better way.

Regards,
Simon