> On 5/28/19 3:47 AM, Ard Biesheuvel wrote: >> On Tue, 28 May 2019 at 12:34, Anup Patel <anup.pa...@wdc.com> wrote: >> >> >> >>> -----Original Message----- >>> From: Karsten Merker <mer...@debian.org> >>> Sent: Tuesday, May 28, 2019 1:53 PM >>> To: Anup Patel <anup.pa...@wdc.com> >>> Cc: Troy Benjegerdes <troy.benjeger...@sifive.com>; Karsten Merker >>> <mer...@debian.org>; Albert Ou <a...@eecs.berkeley.edu>; Jonathan >>> Corbet <cor...@lwn.net>; Ard Biesheuvel <ard.biesheu...@linaro.org>; >>> linux-kernel@vger.kernel.org List <linux-kernel@vger.kernel.org>; Zong Li >>> <z...@andestech.com>; Atish Patra <atish.pa...@wdc.com>; Palmer >>> Dabbelt <pal...@sifive.com>; paul.walms...@sifive.com; Nick Kossifidis >>> <m...@ics.forth.gr>; linux-ri...@lists.infradead.org; >>> marek.va...@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.benjeger...@sifive.com> >>>>>>> On May 27, 2019, at 5:16 PM, Karsten Merker <mer...@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.pa...@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.pa...@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
