[PATCH v2 1/4] image: Add IH_OS_EFI for EFI chain-load boot

[Cristian Ciocaltea]

On Tue, Dec 10, 2019 at 10:49:09PM +0000, Peter Robinson wrote:
> On Tue, Dec 10, 2019 at 6:30 PM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >
> > On 12/10/19 9:56 AM, Cristian Ciocaltea wrote:
> > > Add a new OS type to be used for chain-loading an EFI compatible
> > > firmware or boot loader like GRUB2, possibly in a verified boot
> > > scenario.
> > >
> > > Bellow is sample ITS file that generates a FIT image supporting
> > > secure boot. Please note the presence of 'os = "efi";' line, which
> > > identifies the currently introduced OS type:
> > >
> > > / {
> > >      #address-cells = <1>;
> > >
> > >      images {
> > >          efi-grub {
> > >              description = "GRUB EFI";
> > >              data = /incbin/("EFI/BOOT/bootarm.efi");
> >
> > According to UEFI Spec 2.8 the default file name for 32 bit ARM is
> > BOOTARM.EFI. But GRUB calls the file grubarm.efi.
> 
> In Linux the boot<arch>.efi file is provided by shim [1] and used for
> secure boot etc, I believe the default is also the fall back boot
> method. I'm unaware of shim currently being built for armv7.
> 
> [1] https://github.com/rhboot/shim/
> 
> > So shouldn't we use grubarm.efi here as filename?

My build platform relies on buildroot and that is the default path
where the GRUB EFI binary is deployed. I don't know the reasons behind,
but most probably they are related to portability/compatibility,
as Peter already pointed out.

> > You use EFI/BOOT as directory name. I think this path does not add
> > benefit to the example. The other *.its files also come without any
> > specific path.

Totally agree, I will remove the directory path.

> > Best regards
> >
> > Heinrich
> >
> > >              type = "kernel_noload";
> > >              arch = "arm";
> > >              os = "efi";
> > >              compression = "none";
> > >              load = <0x0>;
> > >              entry = <0x0>;
> > >              hash-1 {
> > >                  algo = "sha256";
> > >              };
> > >          };
> > >      };
> > >
> > >      configurations {
> > >          default = "config-grub";
> > >          config-grub {
> > >              kernel = "efi-grub";
> > >              signature-1 {
> > >                  algo = "sha256,rsa2048";
> > >                  sign-images = "kernel";
> > >              };
> > >          };
> > >      };
> > > };
> > >
> > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea at gmail.com>
> > > ---
> > >   common/image-fit.c | 3 ++-
> > >   common/image.c     | 1 +
> > >   include/image.h    | 1 +
> > >   3 files changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/common/image-fit.c b/common/image-fit.c
> > > index 5c63c769de..19e313bf41 100644
> > > --- a/common/image-fit.c
> > > +++ b/common/image-fit.c
> > > @@ -1925,7 +1925,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
> > >               image_type == IH_TYPE_FPGA ||
> > >               fit_image_check_os(fit, noffset, IH_OS_LINUX) ||
> > >               fit_image_check_os(fit, noffset, IH_OS_U_BOOT) ||
> > > -             fit_image_check_os(fit, noffset, IH_OS_OPENRTOS);
> > > +             fit_image_check_os(fit, noffset, IH_OS_OPENRTOS) ||
> > > +             fit_image_check_os(fit, noffset, IH_OS_EFI);
> > >
> > >       /*
> > >        * If either of the checks fail, we should report an error, but
> > > diff --git a/common/image.c b/common/image.c
> > > index f17fa40c49..2e0e2b0e7f 100644
> > > --- a/common/image.c
> > > +++ b/common/image.c
> > > @@ -134,6 +134,7 @@ static const table_entry_t uimage_os[] = {
> > >       {       IH_OS_OPENRTOS, "openrtos",     "OpenRTOS",             },
> > >   #endif
> > >       {       IH_OS_OPENSBI,  "opensbi",      "RISC-V OpenSBI",       },
> > > +     {       IH_OS_EFI,      "efi",          "EFI Firmware" },
> > >
> > >       {       -1,             "",             "",                     },
> > >   };
> > > diff --git a/include/image.h b/include/image.h
> > > index f4d2aaf53e..4a280b78e7 100644
> > > --- a/include/image.h
> > > +++ b/include/image.h
> > > @@ -157,6 +157,7 @@ enum {
> > >       IH_OS_ARM_TRUSTED_FIRMWARE,     /* ARM Trusted Firmware */
> > >       IH_OS_TEE,                      /* Trusted Execution Environment */
> > >       IH_OS_OPENSBI,                  /* RISC-V OpenSBI */
> > > +     IH_OS_EFI,                      /* EFI Firmware (e.g. GRUB2) */
> > >
> > >       IH_OS_COUNT,
> > >   };
> > >
> >