Message-ID: <MN2PR04MB60610CF4829D5251A112CF9C8D1E0@MN2PR04MB6061.namprd04.prod.outlook.com>
Date:   Tue, 28 May 2019 03:54:02 +0000
From:   Anup Patel <Anup.Patel@....com>
To:     Troy Benjegerdes <troy.benjegerdes@...ive.com>,
        Karsten Merker <merker@...ian.org>
CC:     Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Albert Ou <aou@...s.berkeley.edu>,
        Jonathan Corbet <corbet@....net>, Zong Li <zong@...estech.com>,
        Atish Patra <Atish.Patra@....com>,
        Nick Kossifidis <mick@....forth.gr>,
        Palmer Dabbelt <palmer@...ive.com>,
        "paul.walmsley@...ive.com" <paul.walmsley@...ive.com>,
        "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
        "marek.vasut@...il.com" <marek.vasut@...il.com>,
        "linux-kernel@...r.kernel.org List" <linux-kernel@...r.kernel.org>,
        "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>
Subject: RE: [v4 PATCH] RISC-V: Add an Image header that boot loader can
 parse.



> -----Original Message-----
> From: Troy Benjegerdes <troy.benjegerdes@...ive.com>
> Sent: Tuesday, May 28, 2019 5:11 AM
> To: Karsten Merker <merker@...ian.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@...aro.org>; Albert Ou
> <aou@...s.berkeley.edu>; Jonathan Corbet <corbet@....net>; Anup Patel
> <Anup.Patel@....com>; Zong Li <zong@...estech.com>; Atish Patra
> <Atish.Patra@....com>; Nick Kossifidis <mick@....forth.gr>; Palmer Dabbelt
> <palmer@...ive.com>; paul.walmsley@...ive.com; linux-
> riscv@...ts.infradead.org; marek.vasut@...il.com
> Subject: Re: [v4 PATCH] RISC-V: Add an Image header that boot loader can
> parse.
> 
> 
> 
> > On May 27, 2019, at 5:16 PM, Karsten Merker <merker@...ian.org>
> wrote:
> >
> > 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@....com> 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@....com> 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
> >
> 
> Yes, that would break existing systems. Besides, the qemu -kernel option
> uses the vmlinux elf file, and I think a better solution is make ‘loadelf’ work,
> and include a second method for EFI.
> 
> (unfortunately, I had to drop some lists as I’m having trouble sending to
> them via gmail, so the CC list on my response has been limited)

Nopes, it works perfectly fine on QEMU RISC-V.

Just like ARM64, we are lucky for RISC-V as well. The "MZ" string is a
harmless load instruction in RISC-V so we don't need any changes in QEMU.

We should have "MZ" string in Image header only when Linux RISC-V kernel
has EFI support enabled (just like Linux ARM64 kernel) so that bootloader
trying to boot Linux RISC-V kernel as EFI application will certainly fail
when EFI support is disabled/not-available in Linux RISC-V kernel.

Regards,
Anup

Powered by blists - more mailing lists