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

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

[Atish Patra]

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).

Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
Reviewed-by: Karsten Merker <merker@debian.org>
Tested-by: Karsten Merker <merker@debian.org> (QEMU+OpenSBI+U-Boot)

---
I have not sent out corresponding U-Boot patch as all the changes are
compatible with current u-boot support. Once, the kernel header format
is agreed upon, I will update the U-Boot patch.

Changes from v3->v4
1. Update the commit text to clarify about PE/COFF header.

Changes from v2->v3
1. Modified reserved fields to define a header version.
2. Added header documentation.

Changes from v1-v2:
1. Added additional reserved elements to make it fully PE compatible.
---
 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 */
+	u64 text_offset;	  /* Image load offset, little endian */
+	u64 image_size;		  /* Effective Image size, little endian */
+	u64 flags;		  /* kernel flags, little endian */
+	u32 version;		  /* Version of this header */
+	u32 res1  = 0;		  /* Reserved */
+	u64 res2  = 0;    	  /* Reserved */
+	u64 magic = 0x5643534952; /* Magic number, little endian, "RISCV" */
+	u32 res3;		  /* Reserved for additional RISC-V specific header */
+	u32 res4;		  /* Reserved for PE COFF offset */
+
+This header format is compliant with PE/COFF header and largely inspired from
+ARM64 header. Thus, both ARM64 & RISC-V header can be combined into one common
+header in future.
+
+Notes:
+- This header can also be reused to support EFI stub 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 res5(at offset 0x3c) should
+  point to the rest of the PE/COFF header.
+
+- version field indicate header version number.
+  	Bits 0:15  - Minor version
+	Bits 16:31 - Major version
+
+  This preserves compatibility across newer and older version of the header.
+  The current version is defined as 0.1.
+
+- res3 is reserved for offset to any other additional fields. This makes the
+  header extendible in future. One example would be to accommodate ISA
+  extension for RISC-V in future. For current version, it is set to be zero.
+
+- In current header, the flag field has only one field.
+	Bit 0: Kernel endianness. 1 if BE, 0 if LE.
+
+- Image size is mandatory for boot loader to load kernel image. Booting will
+  fail otherwise.
diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
new file mode 100644
index 000000000000..61c9f20d2f19
--- /dev/null
+++ b/arch/riscv/include/asm/image.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __ASM_IMAGE_H
+#define __ASM_IMAGE_H
+
+#define RISCV_IMAGE_MAGIC	"RISCV"
+
+
+#define RISCV_IMAGE_FLAG_BE_SHIFT	0
+#define RISCV_IMAGE_FLAG_BE_MASK	0x1
+
+#define RISCV_IMAGE_FLAG_LE		0
+#define RISCV_IMAGE_FLAG_BE		1
+
+
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define __HEAD_FLAG_BE		RISCV_IMAGE_FLAG_BE
+#else
+#define __HEAD_FLAG_BE		RISCV_IMAGE_FLAG_LE
+#endif
+
+#define __HEAD_FLAG(field)	(__HEAD_FLAG_##field << \
+				RISCV_IMAGE_FLAG_##field##_SHIFT)
+
+#define __HEAD_FLAGS		(__HEAD_FLAG(BE))
+
+#define RISCV_HEADER_VERSION_MAJOR 0
+#define RISCV_HEADER_VERSION_MINOR 1
+
+#define RISCV_HEADER_VERSION (RISCV_HEADER_VERSION_MAJOR << 16 | \
+			      RISCV_HEADER_VERSION_MINOR)
+
+#ifndef __ASSEMBLY__
+/*
+ * struct riscv_image_header - riscv kernel image header
+ *
+ * @code0:		Executable code
+ * @code1:		Executable code
+ * @text_offset:	Image load offset
+ * @image_size:		Effective Image size
+ * @flags:		kernel flags
+ * @version:		version
+ * @reserved:		reserved
+ * @reserved:		reserved
+ * @magic:		Magic number
+ * @reserved:		reserved (will be used for additional RISC-V specific header)
+ * @reserved:		reserved (will be used for PE COFF offset)
+ */
+
+struct riscv_image_header {
+	u32 code0;
+	u32 code1;
+	u64 text_offset;
+	u64 image_size;
+	u64 flags;
+	u32 version;
+	u32 res1;
+	u64 res2;
+	u64 magic;
+	u32 res3;
+	u32 res4;
+};
+#endif /* __ASSEMBLY__ */
+#endif /* __ASM_IMAGE_H */
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index 370c66ce187a..577893bb150d 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -19,9 +19,41 @@
 #include <asm/thread_info.h>
 #include <asm/page.h>
 #include <asm/csr.h>
+#include <asm/image.h>
 
 __INIT
 ENTRY(_start)
+	/*
+	 * Image header expected by Linux boot-loaders. The image header data
+	 * structure is described in asm/image.h.
+	 * Do not modify it without modifying the structure and all bootloaders
+	 * that expects this header format!!
+	 */
+	/* jump to start kernel */
+	j _start_kernel
+	/* reserved */
+	.word 0
+	.balign 8
+#if __riscv_xlen == 64
+	/* Image load offset(2MB) from start of RAM */
+	.dword 0x200000
+#else
+	/* Image load offset(4MB) from start of RAM */
+	.dword 0x400000
+#endif
+	/* Effective size of kernel image */
+	.dword _end - _start
+	.dword __HEAD_FLAGS
+	.word RISCV_HEADER_VERSION
+	.word 0
+	.dword 0
+	.asciz RISCV_IMAGE_MAGIC
+	.word 0
+	.balign 4
+	.word 0
+
+.global _start_kernel
+_start_kernel:
 	/* Mask all interrupts */
 	csrw CSR_SIE, zero
 	csrw CSR_SIP, zero
-- 
2.21.0

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

[Loys Ollivier]

On Thu 23 May 2019 at 21: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).
>
> Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.

Thanks Atish, happy to have this support that makes the boot process
more straightforward.
Tested on HiFive Unleashed using OpenSBI + U-Boot v2019.07-rc2 + Linux.

>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Reviewed-by: Karsten Merker <merker@debian.org>
> Tested-by: Karsten Merker <merker@debian.org> (QEMU+OpenSBI+U-Boot)
Tested-by: Loys Ollivier <lollivier@baylibre.com>

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

[Ard Biesheuvel]

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).
>
> Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> Reviewed-by: Karsten Merker <merker@debian.org>
> Tested-by: Karsten Merker <merker@debian.org> (QEMU+OpenSBI+U-Boot)
>
> ---
> I have not sent out corresponding U-Boot patch as all the changes are
> compatible with current u-boot support. Once, the kernel header format
> is agreed upon, I will update the U-Boot patch.
>
> Changes from v3->v4
> 1. Update the commit text to clarify about PE/COFF header.
>
> Changes from v2->v3
> 1. Modified reserved fields to define a header version.
> 2. Added header documentation.
>
> Changes from v1-v2:
> 1. Added additional reserved elements to make it fully PE compatible.
> ---
>  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 */



> +This header format is compliant with PE/COFF header and largely inspired from
> +ARM64 header. Thus, both ARM64 & RISC-V header can be combined into one common
> +header in future.
> +
> +Notes:
> +- This header can also be reused to support EFI stub 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 res5(at offset 0x3c) should
> +  point to the rest of the PE/COFF header.
> +
> +- version field indicate header version number.
> +       Bits 0:15  - Minor version
> +       Bits 16:31 - Major version
> +
> +  This preserves compatibility across newer and older version of the header.
> +  The current version is defined as 0.1.
> +
> +- res3 is reserved for offset to any other additional fields. This makes the
> +  header extendible in future. One example would be to accommodate ISA
> +  extension for RISC-V in future. For current version, it is set to be zero.
> +
> +- In current header, the flag field has only one field.
> +       Bit 0: Kernel endianness. 1 if BE, 0 if LE.
> +
> +- Image size is mandatory for boot loader to load kernel image. Booting will
> +  fail otherwise.
> diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
> new file mode 100644
> index 000000000000..61c9f20d2f19
> --- /dev/null
> +++ b/arch/riscv/include/asm/image.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __ASM_IMAGE_H
> +#define __ASM_IMAGE_H
> +
> +#define RISCV_IMAGE_MAGIC      "RISCV"
> +
> +
> +#define RISCV_IMAGE_FLAG_BE_SHIFT      0
> +#define RISCV_IMAGE_FLAG_BE_MASK       0x1
> +
> +#define RISCV_IMAGE_FLAG_LE            0
> +#define RISCV_IMAGE_FLAG_BE            1
> +
> +
> +#ifdef CONFIG_CPU_BIG_ENDIAN
> +#define __HEAD_FLAG_BE         RISCV_IMAGE_FLAG_BE
> +#else
> +#define __HEAD_FLAG_BE         RISCV_IMAGE_FLAG_LE
> +#endif
> +
> +#define __HEAD_FLAG(field)     (__HEAD_FLAG_##field << \
> +                               RISCV_IMAGE_FLAG_##field##_SHIFT)
> +
> +#define __HEAD_FLAGS           (__HEAD_FLAG(BE))
> +
> +#define RISCV_HEADER_VERSION_MAJOR 0
> +#define RISCV_HEADER_VERSION_MINOR 1
> +
> +#define RISCV_HEADER_VERSION (RISCV_HEADER_VERSION_MAJOR << 16 | \
> +                             RISCV_HEADER_VERSION_MINOR)
> +
> +#ifndef __ASSEMBLY__
> +/*
> + * struct riscv_image_header - riscv kernel image header
> + *
> + * @code0:             Executable code
> + * @code1:             Executable code
> + * @text_offset:       Image load offset
> + * @image_size:                Effective Image size
> + * @flags:             kernel flags
> + * @version:           version
> + * @reserved:          reserved
> + * @reserved:          reserved
> + * @magic:             Magic number
> + * @reserved:          reserved (will be used for additional RISC-V specific header)
> + * @reserved:          reserved (will be used for PE COFF offset)
> + */
> +
> +struct riscv_image_header {
> +       u32 code0;
> +       u32 code1;
> +       u64 text_offset;
> +       u64 image_size;
> +       u64 flags;
> +       u32 version;
> +       u32 res1;
> +       u64 res2;
> +       u64 magic;
> +       u32 res3;
> +       u32 res4;
> +};
> +#endif /* __ASSEMBLY__ */
> +#endif /* __ASM_IMAGE_H */
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index 370c66ce187a..577893bb150d 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -19,9 +19,41 @@
>  #include <asm/thread_info.h>
>  #include <asm/page.h>
>  #include <asm/csr.h>
> +#include <asm/image.h>
>
>  __INIT
>  ENTRY(_start)
> +       /*
> +        * Image header expected by Linux boot-loaders. The image header data
> +        * structure is described in asm/image.h.
> +        * Do not modify it without modifying the structure and all bootloaders
> +        * that expects this header format!!
> +        */
> +       /* jump to start kernel */
> +       j _start_kernel
> +       /* reserved */
> +       .word 0
> +       .balign 8
> +#if __riscv_xlen == 64
> +       /* Image load offset(2MB) from start of RAM */
> +       .dword 0x200000
> +#else
> +       /* Image load offset(4MB) from start of RAM */
> +       .dword 0x400000
> +#endif
> +       /* Effective size of kernel image */
> +       .dword _end - _start
> +       .dword __HEAD_FLAGS
> +       .word RISCV_HEADER_VERSION
> +       .word 0
> +       .dword 0
> +       .asciz RISCV_IMAGE_MAGIC
> +       .word 0
> +       .balign 4
> +       .word 0
> +
> +.global _start_kernel
> +_start_kernel:
>         /* Mask all interrupts */
>         csrw CSR_SIE, zero
>         csrw CSR_SIP, zero
> --
> 2.21.0
>

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@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
-- 
Ich widerspreche hiermit ausdrücklich der Nutzung sowie der
Weitergabe meiner personenbezogenen Daten für Zwecke der Werbung
sowie der Markt- oder Meinungsforschung.

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

[Anup Patel]

> -----Original Message-----
> From: Troy Benjegerdes <troy.benjegerdes@sifive.com>
> Sent: Tuesday, May 28, 2019 5:11 AM
> To: Karsten Merker <merker@debian.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Albert Ou
> <aou@eecs.berkeley.edu>; Jonathan Corbet <corbet@lwn.net>; Anup Patel
> <Anup.Patel@wdc.com>; Zong Li <zong@andestech.com>; Atish Patra
> <Atish.Patra@wdc.com>; Nick Kossifidis <mick@ics.forth.gr>; Palmer Dabbelt
> <palmer@sifive.com>; paul.walmsley@sifive.com; 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 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.

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

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

[Karsten Merker]

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 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.

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.

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

[Anup Patel]

> -----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.

Regards,
Anup

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

[Ard Biesheuvel]

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 :-)

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

[Atish Patra]

> 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