[PATCH v2] bootstd: Make efi_mgr bootmeth work for non-sandbox setups

[Simon Glass]

On Tue, 12 Sept 2023 at 14:06, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>
> > From: Simon Glass <sjg at chromium.org>
> > Date: Sun, 10 Sep 2023 13:13:16 -0600
> >
> > Hi Mark,
> >
> > On Sun, 10 Sept 2023 at 08:57, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
> > >
> > > > From: Simon Glass <sjg at chromium.org>
> > > > Date: Thu, 7 Sep 2023 09:57:34 -0600
> > > >
> > > > Hi Mark,
> > > >
> > > > On Thu, 7 Sept 2023 at 07:08, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
> > > > >
> > > > > > From: Simon Glass <sjg at chromium.org>
> > > > > > Date: Thu, 7 Sep 2023 06:23:10 -0600
> > > > > >
> > > > > > Hi Mark,
> > > > > >
> > > > > > On Sun, 3 Sept 2023 at 14:40, Mark Kettenis <kettenis at openbsd.org> wrote:
> > > > > > >
> > > > > > > Enable the bootflow based on this bootmeth if the BootOrder EFI
> > > > > > > variable is set.
> > > > > > >
> > > > > > > Signed-off-by: Mark Kettenis <kettenis at openbsd.org>
> > > > > > > ---
> > > > > > >
> > > > > > > ChangeLog:
> > > > > > >
> > > > > > > v2: - Initialize EFI subsystem to populate EFI variables
> > > > > > >
> > > > > > >
> > > > > > >  boot/bootmeth_efi_mgr.c | 17 ++++++++++++++++-
> > > > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c
> > > > > > > index e9d973429f..e6c42d41fb 100644
> > > > > > > --- a/boot/bootmeth_efi_mgr.c
> > > > > > > +++ b/boot/bootmeth_efi_mgr.c
> > > > > > > @@ -14,6 +14,8 @@
> > > > > > >  #include <bootmeth.h>
> > > > > > >  #include <command.h>
> > > > > > >  #include <dm.h>
> > > > > > > +#include <efi_loader.h>
> > > > > > > +#include <efi_variable.h>
> > > > > > >
> > > > > > >  /**
> > > > > > >   * struct efi_mgr_priv - private info for the efi-mgr driver
> > > > > > > @@ -46,13 +48,26 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter)
> > > > > > >  static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow)
> > > > > > >  {
> > > > > > >         struct efi_mgr_priv *priv = dev_get_priv(dev);
> > > > > > > +       efi_status_t ret;
> > > > > > > +       efi_uintn_t size;
> > > > > > > +       u16 *bootorder;
> > > > > > >
> > > > > > >         if (priv->fake_dev) {
> > > > > > >                 bflow->state = BOOTFLOWST_READY;
> > > > > > >                 return 0;
> > > > > > >         }
> > > > > > >
> > > > > > > -       /* To be implemented */
> > > > > > > +       ret = efi_init_obj_list();
> > > > > >
> > > > > > Doesn't this start up the whole EFI system? Is there a cheaper way to
> > > > > > see if the variable subsystem exists, or can work?
> > > > >
> > > > > A fair bit of it at least.  With the possible exception of launching
> > > > > EFI capsules (which shouldn't happen for a "normal" boot), I don't
> > > > > think this is very expensive though.
> > > > >
> > > > > There currently isn't a way to only initialize the variable subsystem;
> > > > > we can't just call efi_init_variables() here since we have to also
> > > > > call efi_disks_register() and a later efi_init_obj_list() call would
> > > > > call efi_init_variables() again, which would leak memory.
> > > > >
> > > > > Alternatively we could just unconditionally declare the efi_mgr
> > > > > bootmeth as "ready" (like you already do for sandbox).  That would
> > > > > postpone initialization of the EFI subsystem until we actually execute
> > > > > this bootmeth.
> > > > >
> > > > > But we need to do something before we switch more targets to standard
> > > > > boot since otherwise booting through the EFI boot manager is just
> > > > > plain broken.
> > > >
> > > > Yes...
> > > >
> > > > I don't have any great ideas on this. If we could get the EFI code
> > > > better integrated into U-Boot then perhaps we would not need this
> > > > 'monolithm' init?
> > >
> > > You'll have to take that up with the maintainers of the EFI loader
> > > subsystem.  I just want to repair the breakage that was introduced
> > > with bootstd.
> >
> > Yes, understood. Do you know how long it takes to do this step?
>
> It is instant.  But then this is on a fast machine with the EFI
> variables stored on the internal NVMe disk, which is plenty fast as
> well.  And I haven't enabled any of the capsule or TPM stuff hich may
> take more time.
>
> > The control for this feature is CMD_BOOTEFI_BOOTMGR...I think it would
> > be better to have BOOTEFI_BOOTMGR as well as CMD_BOOTEFI_BOOTMGR,
> > since it is strange to have a CMD Kconfig in lib/efi_loader
> >
> > I am OK with this patch as I don't see any alternative. We need to do
> > something and we can always improve it if it causes problems.
> >
> > Heinrich?

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