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

[Jonathan Marek]

On 10/12/24 9:36 AM, Ard Biesheuvel wrote:
> 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

(not relevant to this commit, but I need to say that concatenating dtb 
files makes no sense, only the first one will be used by the kernel)

> > 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.

Its not the expected/documented behavior, that should be enough to make 
it a problem. Nowhere is it documented that these options would be 
ignored if provided through CONFIG_CMDLINE.

> > 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?

I boot linux as the "EFI/Boot/bootaa64.efi" on my EFI partition. The 
firmware does not provide any load options. This system needs a dtb, so 
I add the dtb to my EFI partition and configure it using the dtb= option 
(using CONFIG_CMDLINE).