Re: [v4 PATCH] RISC-V: Add an Image header that boot loader can parse.

[Karsten Merker]

On Mon, May 27, 2019 at 04:34:57PM +0200, Ard Biesheuvel wrote:
> On Fri, 24 May 2019 at 06:18, Atish Patra <atish.patra@xxxxxxx> wrote:
> > Currently, the last stage boot loaders such as U-Boot can accept only
> > uImage which is an unnecessary additional step in automating boot
> > process.
> >
> > Add an image header that boot loader understands and boot Linux from
> > flat Image directly.
> >
> > This header is based on ARM64 boot image header and provides an
> > opportunity to combine both ARM64 & RISC-V image headers in future.
> >
> > Also make sure that PE/COFF header can co-exist in the same image so
> > that EFI stub can be supported for RISC-V in future. EFI specification
> > needs PE/COFF image header in the beginning of the kernel image in order
> > to load it as an EFI application. In order to support EFI stub, code0
> > should be replaced with "MZ" magic string and res4(at offset 0x3c)
> > should point to the rest of the PE/COFF header (which will be added
> > during EFI support).
[...]
> >  Documentation/riscv/boot-image-header.txt | 50 ++++++++++++++++++
> >  arch/riscv/include/asm/image.h            | 64 +++++++++++++++++++++++
> >  arch/riscv/kernel/head.S                  | 32 ++++++++++++
> >  3 files changed, 146 insertions(+)
> >  create mode 100644 Documentation/riscv/boot-image-header.txt
> >  create mode 100644 arch/riscv/include/asm/image.h
> >
> > diff --git a/Documentation/riscv/boot-image-header.txt b/Documentation/riscv/boot-image-header.txt
> > new file mode 100644
> > index 000000000000..68abc2353cec
> > --- /dev/null
> > +++ b/Documentation/riscv/boot-image-header.txt
> > @@ -0,0 +1,50 @@
> > +                               Boot image header in RISC-V Linux
> > +                       =============================================
> > +
> > +Author: Atish Patra <atish.patra@xxxxxxx>
> > +Date  : 20 May 2019
> > +
> > +This document only describes the boot image header details for RISC-V Linux.
> > +The complete booting guide will be available at Documentation/riscv/booting.txt.
> > +
> > +The following 64-byte header is present in decompressed Linux kernel image.
> > +
> > +       u32 code0;                /* Executable code */
> > +       u32 code1;                /* Executable code */
> 
> Apologies for not mentioning this in my previous reply, but given that
> you already know that you will need to put the magic string MZ at
> offset 0x0, it makes more sense to not put any code there at all, but
> educate the bootloader that the first executable instruction is at
> offset 0x20, and put the spare fields right after it in case you ever
> need more than 2 slots. (On arm64, we were lucky to be able to find an
> opcode that happened to contain the MZ bit pattern and act almost like
> a NOP, but it seems silly to rely on that for RISC-V as well)
> 
> So something like
> 
> u16 pe_res1;  /* MZ for EFI bootable images, don't care otherwise */
> u8 magic[6];    /* "RISCV\0"
> 
> u64 text_offset;          /* Image load offset, little endian */
> u64 image_size;           /* Effective Image size, little endian */
> u64 flags;                /* kernel flags, little endian */
> 
> u32 code0;                /* Executable code */
> u32 code1;                /* Executable code */
> 
> u64 reserved[2];     /* reserved for future use */
> 
> u32 version;              /* Version of this header */
> u32 pe_res2;                 /* Reserved for PE COFF offset */

Hello,

wouldn't that immediately break existing systems (including qemu
when loading kernels with the "-kernel" option) that rely on the
fact that the kernel entry point is always at the kernel load
address?  The ARM64 header and Atish's original RISC-V proposal
based on the ARM64 header keep the property that jumping to the
kernel load address always works, regardless of what the
particular header looks like and which potential future
extensions it includes, but the proposed change above wouldn't do
that.

Although I agree that having to integrate the "MZ" string as an
instruction isn't particularly nice, I don't think that this is a
sufficient justification for breaking compatibility with prior
kernel releases and/or existing boot firmware.  On RISC-V, the
"MZ" string is a compressed load immediate to x20/s4, i.e. an
instruction that should be "harmless" as far as the kernel boot
flow is concerned as the x20/s4 register AFAIK doesn't contain any
information that the kernel would use.

Regards,
Karsten
-- 
Ich widerspreche hiermit ausdrÃcklich der Nutzung sowie der
Weitergabe meiner personenbezogenen Daten fÃr Zwecke der Werbung
sowie der Markt- oder Meinungsforschung.