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

[Atish Patra <Atish.Patra@wdc.com>]

> On 5/28/19 3:47 AM, Ard Biesheuvel wrote:
>> On Tue, 28 May 2019 at 12:34, Anup Patel <Anup.Patel@wdc.com> wrote:
>> 
>> 
>> 
>>> -----Original Message-----
>>> From: Karsten Merker <merker@debian.org>
>>> Sent: Tuesday, May 28, 2019 1:53 PM
>>> To: Anup Patel <Anup.Patel@wdc.com>
>>> Cc: Troy Benjegerdes <troy.benjegerdes@sifive.com>; Karsten Merker
>>> <merker@debian.org>; Albert Ou <aou@eecs.berkeley.edu>; Jonathan
>>> Corbet <corbet@lwn.net>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
>>> linux-kernel@vger.kernel.org List <linux-kernel@vger.kernel.org>; Zong Li
>>> <zong@andestech.com>; Atish Patra <Atish.Patra@wdc.com>; Palmer
>>> Dabbelt <palmer@sifive.com>; paul.walmsley@sifive.com; Nick Kossifidis
>>> <mick@ics.forth.gr>; linux-riscv@lists.infradead.org;
>>> marek.vasut@gmail.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@sifive.com>
>>>>>>> On May 27, 2019, at 5:16 PM, Karsten Merker <merker@debian.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@wdc.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@wdc.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.
> Fair enough. If you guys can live with it, then so can I :-)

Thanks for the review & feedback!! It got all resolved before I had a chance to take look :) :).

@Paul: Can you queue this for next PR ?

-- 
Regards,
Atish