Re: [PATCH 3/3] efi/libstub: consider CONFIG_CMDLINE for initrd= and dtb= options

[Ard Biesheuvel]

On Sat, 12 Oct 2024 at 14:04, Jonathan Marek <jonathan@xxxxxxxx> wrote:
>
>
>
> On 10/12/24 3:54 AM, Ard Biesheuvel wrote:
> > On Sat, 12 Oct 2024 at 00:52, Jonathan Marek <jonathan@xxxxxxxx> wrote:
> >>
> >> Replace cmdline with CONFIG_CMDLINE when it should be used instead of
> >> load_options.
> >>
> >> In the EXTEND case, it may be necessary to combine both CONFIG_CMDLINE and
> >> load_options. In that case, keep the old behavior and print a warning about
> >> the incorrect behavior.
> >>
> >
> > The core kernel has its own handling for EXTEND/FORCE, so while we
> > should parse it in the EFI stub to look for options that affect the
> > stub's own behavior, we should not copy it into the command line that
> > the stub provides to the core kernel.
> >
> > E.g., drivers/of/fdt.c takes the bootargs from the DT and combines
> > them with CONFIG_CMDLINE.
> >
> >
>
> I'm aware of that - the replacement the commit message is referring to
> is specifically for handle_cmdline_files() which this commit is modifying.
>

Ah ok - I missed that.

This is the kind of context that I'd expect in a cover letter, i.e.,
that the command line handling is inconsistent, and that we obtain the
command line from the loaded image twice.

Also, the fact the initrd= handling and dtb= are special, because
a) multiple initrd=  arguments are processed in order, and the files
concatenated,
b) the filenames are consumed as UTF-16 as they are plugged into the
file I/O protocols

> Currently efistub completely ignores initrd= and dtb= options provided
> through CONFIG_CMDLINE (handle_cmdline_files() only parses the EFI options)
>

Indeed. You haven't explained why this is a problem. initrd= and dtb=
contain references to files in the file system, and this does not seem
like something CONFIG_EXTEND was intended for.

> For the EXTEND case, I didn't implement the full solution because its
> more complex and EXTEND is not available on arm64 anyway, so I went with
> just printing a warning instead.

This code is shared between all architectures, so what arm64 does or
does not support is irrelevant.

Can you explain your use case please?