Hi Cristian,
On Sun, 24 Nov 2019 at 23:22, Heinrich Schuchardt 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
> > ---
> > 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
