[PATCH 2/2] bootm: Add a bootm command for type IH_OS_EFI

[Simon Glass]

Hi Cristian,

On Sun, 24 Nov 2019 at 23:22, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 11/24/19 9:11 PM, Cristian Ciocaltea wrote:
> > Add support for booting EFI binaries contained in FIT images.
> > A typical usage scenario is chain-loading GRUB2 in a verified
> > boot environment.
> >
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea at gmail.com>
> > ---
> >   cmd/Kconfig       |  9 ++++++++-
> >   cmd/bootefi.c     |  2 +-
> >   common/bootm_os.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >   include/bootm.h   |  2 ++
> >   4 files changed, 55 insertions(+), 2 deletions(-)
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index cf982ff65e..1bec840f5a 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -263,6 +263,13 @@ config CMD_BOOTI
> >       help
> >         Boot an AArch64 Linux Kernel image from memory.
> >
> > +config BOOTM_EFI
> > +     bool "Support booting EFI OS images"
> > +     depends on CMD_BOOTEFI
> > +     default y
> > +     help
> > +       Support booting EFI images via the bootm command.
> > +
> >   config BOOTM_LINUX
> >       bool "Support booting Linux OS images"
> >       depends on CMD_BOOTM || CMD_BOOTZ || CMD_BOOTI
> > @@ -316,7 +323,7 @@ config CMD_BOOTEFI
> >       depends on EFI_LOADER
> >       default y
> >       help
> > -       Boot an EFI image from memory.
> > +       Boot an EFI binary from memory.
> >
> >   config CMD_BOOTEFI_HELLO_COMPILE
> >       bool "Compile a standard EFI hello world binary for testing"
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index f613cce7e2..f25d639dfe 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -553,7 +553,7 @@ static int do_efi_selftest(void)
> >    * @argv:   command line arguments
> >    * Return:  status code
> >    */
> > -static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > +int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >   {
> >       efi_status_t ret;
> >
> > diff --git a/common/bootm_os.c b/common/bootm_os.c
> > index 6fb7d658da..706151913a 100644
> > --- a/common/bootm_os.c
> > +++ b/common/bootm_os.c
> > @@ -462,6 +462,47 @@ static int do_bootm_tee(int flag, int argc, char * const argv[],
> >   }
> >   #endif
> >
> > +#ifdef CONFIG_BOOTM_EFI
> > +static int do_bootm_efi(int flag, int argc, char * const argv[],
> > +                     bootm_headers_t *images)
> > +{
> > +     int ret;
> > +     int local_argc = 2;
> > +     char *local_args[3];
> > +     char str_efi_addr[16];
> > +     char str_fdt_addr[16];
> > +
> > +     if (flag != BOOTM_STATE_OS_GO)
> > +             return 0;
> > +
> > +     /* Locate FDT etc */
> > +     ret = bootm_find_images(flag, argc, argv);
> > +     if (ret)
> > +             return ret;
> > +
> > +     printf("## Transferring control to EFI (at address %08lx) ...\n",
> > +            images->ep);
> > +     bootstage_mark(BOOTSTAGE_ID_RUN_OS);
> > +
> > +     local_args[0] = argv[0];
> > +
> > +     /* Write efi addr into string */
> > +     sprintf(str_efi_addr, "%lx", images->ep);
>
> I think we should avoid transferring arguments as strings. It is
> preferable to separate do_bootefi() into an argument parser and an
> execution part.
>
> Let's CC the developers who have contributed to common/bootm_os.c.

Yes I think it would be good to create a function to hold most of the
code from do_bootefi(), call the function with appropriate parameters
and changing do_bootefi() to call that new function.

>
> Best regards
>
> Heinrich
>
> > +     /* and provide it via the arguments */
> > +     local_args[1] = str_efi_addr;
> > +
> > +     if (images->ft_len) {
> > +             /* Write fdt addr into string */
> > +             sprintf(str_fdt_addr, "%lx", (unsigned long)images->ft_addr);
> > +             /* and provide it via the arguments */
> > +             local_args[2] = str_fdt_addr;
> > +             local_argc = 3;
> > +     }
> > +
> > +     return do_bootefi(NULL, 0, local_argc, local_args);
> > +}
> > +#endif
> > +
> >   static boot_os_fn *boot_os[] = {
> >       [IH_OS_U_BOOT] = do_bootm_standalone,
> >   #ifdef CONFIG_BOOTM_LINUX
> > @@ -498,6 +539,9 @@ static boot_os_fn *boot_os[] = {
> >   #ifdef CONFIG_BOOTM_OPTEE
> >       [IH_OS_TEE] = do_bootm_tee,
> >   #endif
> > +#ifdef CONFIG_BOOTM_EFI
> > +     [IH_OS_EFI] = do_bootm_efi,
> > +#endif
> >   };
> >
> >   /* Allow for arch specific config before we boot */
> > diff --git a/include/bootm.h b/include/bootm.h
> > index edeeacb0df..a0da86dc32 100644
> > --- a/include/bootm.h
> > +++ b/include/bootm.h
> > @@ -37,7 +37,9 @@ typedef int boot_os_fn(int flag, int argc, char * const argv[],
> >   extern boot_os_fn do_bootm_linux;
> >   extern boot_os_fn do_bootm_vxworks;
> >
> > +int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);

Please add a function comment.

> >   int do_bootelf(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> > +
> >   void lynxkdi_boot(image_header_t *hdr);
> >
> >   boot_os_fn *bootm_os_get_boot_func(int os);
> >
>

Regards,
Simon