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

[Cristian Ciocaltea <cristian.ciocaltea@gmail.com>]

On Sun, Dec 29, 2019 at 11:56:01AM +0100, Heinrich Schuchardt wrote:
> On 12/29/19 11:34 AM, Heinrich Schuchardt wrote:
> > On 12/24/19 5:05 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@gmail.com>
> > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > ---
> > >   cmd/Kconfig       |  7 ++++++
> > >   common/bootm_os.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++
> > >   2 files changed, 63 insertions(+)
> > > 
> > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > index 1e4cf146c5..87f2335a3c 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"
> > 
> > Shouldn't this be "Support booting UEFI FIT images"?
> > 
> > > +    depends on CMD_BOOTEFI
> > 
> > depends on BOOTM
> 
> depends on CMD_BOOTM
> 
> The patch series compiles without CONFIG_FIT. But shouldn't this also be
> a dependency?

Indeed, thanks.

> If we place the definition directly after CMD_BOOTM, it will be indented
> so that it is evident that this is a sub-feature of CMD_BOOTM.
> 
> So how about the following?
> 
> config CMD_BOOTM
>         bool "bootm"
>         default y
>         help
>           Boot an application image from the memory.
> 
> config BOOTM_EFI
>         bool "Support booting UEFI FIT images"
>         depends on CMD_BOOTEFI && CMD_BOOTM && FIT
>         default y
>         help
>           Support booting UEFI FIT images via the bootm command.

In this case, we should probably also move CMD_BOOTZ, CMD_BOOTI and
BOOTM_LINUX right after CMD_BOOTEFI_HELLO, since those commands do not
depend on CMD_BOOTM, while all BOOTM_* features, except BOOTM_LINUX,
depend exclusively on CMD_BOOTM.

> Best regards
> 
> Heinrich
> 
> > 
> > is missing here.
> > 
> > > +    default y
> > > +    help
> > > +      Support booting EFI images via the bootm command.
> > 
> > Should we say:
> > 
> > Support booting UEFI FIT images via the bootm command.
> > 
> > Best regards
> > 
> > Heinrich
> > 
> > > +
> > >   config BOOTM_LINUX
> > >       bool "Support booting Linux OS images"
> > >       depends on CMD_BOOTM || CMD_BOOTZ || CMD_BOOTI
> > > diff --git a/common/bootm_os.c b/common/bootm_os.c
> > > index d89ddc32b0..1d58462509 100644
> > > --- a/common/bootm_os.c
> > > +++ b/common/bootm_os.c
> > > @@ -7,10 +7,12 @@
> > >   #include <common.h>
> > >   #include <bootm.h>
> > >   #include <cpu_func.h>
> > > +#include <efi_loader.h>
> > >   #include <env.h>
> > >   #include <fdt_support.h>
> > >   #include <linux/libfdt.h>
> > >   #include <malloc.h>
> > > +#include <mapmem.h>
> > >   #include <vxworks.h>
> > >   #include <tee/optee.h>
> > > 
> > > @@ -498,6 +500,57 @@ 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;
> > > +    efi_status_t efi_ret;
> > > +    void *image_buf;
> > > +
> > > +    if (flag != BOOTM_STATE_OS_GO)
> > > +        return 0;
> > > +
> > > +    /* Locate FDT, if provided */
> > > +    ret = bootm_find_images(flag, argc, argv);
> > > +    if (ret)
> > > +        return ret;
> > > +
> > > +    /* Initialize EFI drivers */
> > > +    efi_ret = efi_init_obj_list();
> > > +    if (efi_ret != EFI_SUCCESS) {
> > > +        printf("## Failed to initialize UEFI sub-system: r = %lu\n",
> > > +               efi_ret & ~EFI_ERROR_MASK);
> > > +        return 1;
> > > +    }
> > > +
> > > +    /* Install device tree */
> > > +    efi_ret = efi_install_fdt(images->ft_len
> > > +                  ? images->ft_addr : EFI_FDT_USE_INTERNAL);
> > > +    if (efi_ret != EFI_SUCCESS) {
> > > +        printf("## Failed to install device tree: r = %lu\n",
> > > +               efi_ret & ~EFI_ERROR_MASK);
> > > +        return 1;
> > > +    }
> > > +
> > > +    /* Run EFI image */
> > > +    printf("## Transferring control to EFI (at address %08lx) ...\n",
> > > +           images->ep);
> > > +    bootstage_mark(BOOTSTAGE_ID_RUN_OS);
> > > +
> > > +    image_buf = map_sysmem(images->ep, images->os.image_len);
> > > +
> > > +    efi_ret = efi_run_image(image_buf, images->os.image_len);
> > > +    if (efi_ret != EFI_SUCCESS) {
> > > +        printf("## Failed to run EFI image: r = %lu\n",
> > > +               efi_ret & ~EFI_ERROR_MASK);
> > > +        return 1;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +#endif
> > > +
> > >   static boot_os_fn *boot_os[] = {
> > >       [IH_OS_U_BOOT] = do_bootm_standalone,
> > >   #ifdef CONFIG_BOOTM_LINUX
> > > @@ -534,6 +587,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 */
> > > 
> > 
> > 
>