[PATCH v3 4/4] fs: remove explicit efi configuration dependency

[Simon Glass]

Hi,

On Wed, Dec 20, 2023 at 1:25 PM Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Dec 19, 2023 at 09:46:27PM -0700, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 18 Dec 2023 at 15:16, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > >
> > >
> > >
> > > Am 18. Dezember 2023 16:01:40 MEZ schrieb Simon Glass <sjg at chromium.org>:
> > > >Hi AKASHI,
> > > >
> > > >On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
> > > ><takahiro.akashi at linaro.org> wrote:
> > > >>
> > > >> Now it is clear that the feature actually depends on efi interfaces,
> > > >> not "bootefi" command. efi_set_bootdev() will automatically be nullified
> > > >> if necessary efi component is disabled.
> > > >>
> > > >> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > > >> ---
> > > >>  fs/fs.c | 7 +++----
> > > >>  1 file changed, 3 insertions(+), 4 deletions(-)
> > > >>
> > > >> diff --git a/fs/fs.c b/fs/fs.c
> > > >> index f33b85f92b61..82ee03b160e9 100644
> > > >> --- a/fs/fs.c
> > > >> +++ b/fs/fs.c
> > > >> @@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
> > > >>                 return 1;
> > > >>         }
> > > >>
> > > >> -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI))
> > > >> -               efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> > > >> -                               (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
> > > >> -                               len_read);
> > > >> +       efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> > > >> +                       (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
> > > >> +                       len_read);
> > > >
> > > >As I understand it, this is setting the boot device so that (if it
> > > >happens to be an efi application) it will know which device it came
> > > >from. But this is a hack. For bootstd, the device is known as it loads
> > > >the kernel.
> > >
> > > Please, consider what happens when the user interactively executes the load and bootefi command from the console.

Yes, that case needs to be handled.

> > >
> > > >
> > > >Also it does not deal with memory allocation (nor can it).
> > > >
> > > >Where are we using the 'load' command to load a kernel? The distro
> > > >scripts are deprecated.
> > >
> > > Some users use boot.scr scripts

We will need to figure out that. But we should make bootstd do the right thing.

> > >
> > >
> > > >
> > > >At some point this code should be removed. Is it too early for that?
> > >
> > > Yes, as long as we allow users to invoke the bootefi command with an address pointer.
> >
> > So how about we create the new path, with the info passed in as part
> > of the bootefi call? Then we can remove the call from bootstd, at
> > least.
>
> Why? It's not clear to me we'd have this information available at that
> point unless we stored things to pass along at that point too.

For the bootstd case, we know the device where the kernel came from.
See distro_efi_boot() for this code. It calls efiload_read_file()
which calls set_efi_bootdev().

We should instead pass the device information to efi_binary_run().

With that done, we can perhaps make the 'load' and 'tftp' commands
record the device information somewhere, so that bootm can pick it up
when it starts and put it in 'images'. Then at some point we can clean
up the 'devnr' parameter, which should really be a partition number,
since the 'dev' parameter tells you the device.

Anyway, this series is fine, so long as we can agree to the above.

Reviewed-by: Simon Glass <sjg at chromium.org>

Regards,
Simon