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



> -----Original Message-----
> From: Karsten Merker <merker@...ian.org>
> Sent: Tuesday, May 28, 2019 1:53 PM
> To: Anup Patel <Anup.Patel@....com>
> Cc: Troy Benjegerdes <troy.benjegerdes@...ive.com>; Karsten Merker
> <merker@...ian.org>; Albert Ou <aou@...s.berkeley.edu>; Jonathan
> Corbet <corbet@....net>; Ard Biesheuvel <ard.biesheuvel@...aro.org>;
> linux-kernel@...r.kernel.org List <linux-kernel@...r.kernel.org>; Zong Li
> <zong@...estech.com>; Atish Patra <Atish.Patra@....com>; Palmer
> Dabbelt <palmer@...ive.com>; paul.walmsley@...ive.com; Nick Kossifidis
> <mick@....forth.gr>; 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 Tue, May 28, 2019 at 03:54:02AM +0000, Anup Patel wrote:
> > > From: Troy Benjegerdes <troy.benjegerdes@...ive.com>
> > > > 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.
> 
> Hello,
> 
> just to avoid misunderstandings: Atish, does your "Nopes, it works perfectly
> fine on QEMU RISC-V" refer to your original header proposal or to Ard's
> modified header proposal?  For your proposal I agree that it works without

Sorry for the confusion. I meant here that adding "MZ" at start in Atish's
proposed header works fine on QEMU.

> problems in all cases that have worked before introduction of the header,
> i.e. adding your proposed header is completely transparent, but as described
> above I have doubts that the same is true for the (different) header format
> that Ard has proposed above.

Yes, Ard's proposed header will break booting on current QEMU and
existing HW. I think Ard's proposed header was to address the case if
"MZ" was not a valid and harmless instruction in RISC-V. Other than
that Ard's proposal is similar to Atish's proposal but organized differently.

For Atish's proposed header, we are certainly relying on the fact that
"MZ" represents a valid and harmless instruction (just like ARM64) but
this approach is allowing us to boot Linux RISC-V kernel without breaking
existing booting methods.

Regards,
Anup

Powered by blists - more mailing lists