RISC-V: Add an Image header that boot loader can parse.

Commit Message

Atish Patra April 23, 2019, 11:25 p.m. UTC

Currently, last stage boot loaders such as U-Boot can accept only
uImage which is an unnecessary additional step in automating boot flows.

Add a simple image header that boot loaders can parse and directly
load kernel flat Image. The existing booting methods will continue to
work as it is.

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

Signed-off-by: Atish Patra <[e-mail]>
---
 arch/riscv/include/asm/image.h | 32 ++++++++++++++++++++++++++++++++
 arch/riscv/kernel/head.S       | 28 ++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 arch/riscv/include/asm/image.h

Comments

Palmer Dabbelt April 29, 2019, 11:40 p.m. UTC | #1

On Tue, 23 Apr 2019 16:25:06 PDT (-0700), [e-mail] wrote:
> Currently, last stage boot loaders such as U-Boot can accept only
> uImage which is an unnecessary additional step in automating boot flows.
>
> Add a simple image header that boot loaders can parse and directly
> load kernel flat Image. The existing booting methods will continue to
> work as it is.
>
> Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.
>
> Signed-off-by: Atish Patra <[e-mail]>
> ---
>  arch/riscv/include/asm/image.h | 32 ++++++++++++++++++++++++++++++++
>  arch/riscv/kernel/head.S       | 28 ++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
>  create mode 100644 arch/riscv/include/asm/image.h
>
> diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
> new file mode 100644
> index 000000000000..76a7e0d4068a
> --- /dev/null
> +++ b/arch/riscv/include/asm/image.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __ASM_IMAGE_H
> +#define __ASM_IMAGE_H
> +
> +#define RISCV_IMAGE_MAGIC	"RISCV"
> +
> +#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
> + * @reserved:		reserved
> + * @magic:		Magic number
> + * @reserved:		reserved
> + */
> +
> +struct riscv_image_header {
> +	u32 code0;
> +	u32 code1;
> +	u64 text_offset;
> +	u64 image_size;
> +	u64 res1;
> +	u64 magic;
> +	u32 res2;
> +	u32 res3;
> +};

I don't want to invent our own file format.  Is there a reason we can't just
use something standard?  Off the top of my head I can think of ELF files and
multiboot.

> +#endif /* __ASSEMBLY__ */
> +#endif /* __ASM_IMAGE_H */
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index fe884cd69abd..154647395601 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -19,9 +19,37 @@
>  #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 0
> +	.asciz RISCV_IMAGE_MAGIC
> +	.word 0
> +	.word 0
> +
> +.global _start_kernel
> +_start_kernel:
>  	/* Mask all interrupts */
>  	csrw sie, zero

Atish Patra April 30, 2019, 5:42 a.m. UTC | #2

On 4/29/19 4:40 PM, Palmer Dabbelt wrote:
> On Tue, 23 Apr 2019 16:25:06 PDT (-0700), [e-mail] wrote:
>> Currently, last stage boot loaders such as U-Boot can accept only
>> uImage which is an unnecessary additional step in automating boot flows.
>>
>> Add a simple image header that boot loaders can parse and directly
>> load kernel flat Image. The existing booting methods will continue to
>> work as it is.
>>
>> Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.
>>
>> Signed-off-by: Atish Patra <[e-mail]>
>> ---
>>   arch/riscv/include/asm/image.h | 32 ++++++++++++++++++++++++++++++++
>>   arch/riscv/kernel/head.S       | 28 ++++++++++++++++++++++++++++
>>   2 files changed, 60 insertions(+)
>>   create mode 100644 arch/riscv/include/asm/image.h
>>
>> diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
>> new file mode 100644
>> index 000000000000..76a7e0d4068a
>> --- /dev/null
>> +++ b/arch/riscv/include/asm/image.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef __ASM_IMAGE_H
>> +#define __ASM_IMAGE_H
>> +
>> +#define RISCV_IMAGE_MAGIC	"RISCV"
>> +
>> +#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
>> + * @reserved:		reserved
>> + * @magic:		Magic number
>> + * @reserved:		reserved
>> + */
>> +
>> +struct riscv_image_header {
>> +	u32 code0;
>> +	u32 code1;
>> +	u64 text_offset;
>> +	u64 image_size;
>> +	u64 res1;
>> +	u64 magic;
>> +	u32 res2;
>> +	u32 res3;
>> +};
> 
> I don't want to invent our own file format.  Is there a reason we can't just
> use something standard?  Off the top of my head I can think of ELF files and
> multiboot.
> 

Additional header is required to accommodate PE header format. 
Currently, this is only used for booti command but it will be reused for 
EFI headers as well. Linux kernel Image can pretend as an EFI 
application if PE/COFF header is present. This removes the need of an 
explicit EFI boot loader and EFI firmware can directly load Linux 
(obviously after EFI stub implementation for RISC-V).

ARM64 follows the similar header format as well.
https://www.kernel.org/doc/Documentation/arm64/booting.txt

Regards,
Atish

>> +#endif /* __ASSEMBLY__ */
>> +#endif /* __ASM_IMAGE_H */
>> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
>> index fe884cd69abd..154647395601 100644
>> --- a/arch/riscv/kernel/head.S
>> +++ b/arch/riscv/kernel/head.S
>> @@ -19,9 +19,37 @@
>>   #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 0
>> +	.asciz RISCV_IMAGE_MAGIC
>> +	.word 0
>> +	.word 0
>> +
>> +.global _start_kernel
>> +_start_kernel:
>>   	/* Mask all interrupts */
>>   	csrw sie, zero
> 
> _______________________________________________
> linux-riscv mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>

Karsten Merker May 1, 2019, 4:43 p.m. UTC | #3

On Mon, Apr 29, 2019 at 10:42:40PM -0700, Atish Patra wrote:
> On 4/29/19 4:40 PM, Palmer Dabbelt wrote:
> > On Tue, 23 Apr 2019 16:25:06 PDT (-0700), [e-mail] wrote:
> > > Currently, last stage boot loaders such as U-Boot can accept only
> > > uImage which is an unnecessary additional step in automating boot flows.
> > > 
> > > Add a simple image header that boot loaders can parse and directly
> > > load kernel flat Image. The existing booting methods will continue to
> > > work as it is.
> > > 
> > > Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.
> > > 
> > > Signed-off-by: Atish Patra <[e-mail]>
> > > ---
> > >   arch/riscv/include/asm/image.h | 32 ++++++++++++++++++++++++++++++++
> > >   arch/riscv/kernel/head.S       | 28 ++++++++++++++++++++++++++++
> > >   2 files changed, 60 insertions(+)
> > >   create mode 100644 arch/riscv/include/asm/image.h
> > > 
> > > diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
> > > new file mode 100644
> > > index 000000000000..76a7e0d4068a
> > > --- /dev/null
> > > +++ b/arch/riscv/include/asm/image.h
> > > @@ -0,0 +1,32 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +
> > > +#ifndef __ASM_IMAGE_H
> > > +#define __ASM_IMAGE_H
> > > +
> > > +#define RISCV_IMAGE_MAGIC	"RISCV"
> > > +
> > > +#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
> > > + * @reserved:		reserved
> > > + * @magic:		Magic number
> > > + * @reserved:		reserved
> > > + */
> > > +
> > > +struct riscv_image_header {
> > > +	u32 code0;
> > > +	u32 code1;
> > > +	u64 text_offset;
> > > +	u64 image_size;
> > > +	u64 res1;
> > > +	u64 magic;
> > > +	u32 res2;
> > > +	u32 res3;
> > > +};
> > 
> > I don't want to invent our own file format.  Is there a reason we can't just
> > use something standard?  Off the top of my head I can think of ELF files and
> > multiboot.
> 
> Additional header is required to accommodate PE header format. Currently,
> this is only used for booti command but it will be reused for EFI headers as
> well. Linux kernel Image can pretend as an EFI application if PE/COFF header
> is present. This removes the need of an explicit EFI boot loader and EFI
> firmware can directly load Linux (obviously after EFI stub implementation
> for RISC-V).
> 
> ARM64 follows the similar header format as well.
> https://www.kernel.org/doc/Documentation/arm64/booting.txt

Hello Atish,

the arm64 header looks a bit different (quoted from the
aforementioned URL):

  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 */
  u64 res2      = 0;            /* reserved */
  u64 res3      = 0;            /* reserved */
  u64 res4      = 0;            /* reserved */
  u32 magic     = 0x644d5241;   /* Magic number, little endian, "ARM\x64" */
  u32 res5;                     /* reserved (used for PE COFF offset) */

What I am unclear about is in which ways a RISC-V PE/COFF header
differs from an arm64 one as the arm64 struct is longer than your
RISC-V header and for arm64 the PE offset field is in the last
field, i.e. outside of the area covered by your RISC-V structure
definition.  Can you perhaps explain this part in a bit more
detail or does anybody else have a pointer to a specification of
the RISC-V PE/COFF header format (I have found a lot of documents
about COFF in general, but nothing specific to RISC-V).

Regards,
Karsten

Mark Rutland May 1, 2019, 5 p.m. UTC | #4

On Mon, Apr 29, 2019 at 10:42:40PM -0700, Atish Patra wrote:
> On 4/29/19 4:40 PM, Palmer Dabbelt wrote:
> > On Tue, 23 Apr 2019 16:25:06 PDT (-0700), [e-mail] wrote:
> > > Currently, last stage boot loaders such as U-Boot can accept only
> > > uImage which is an unnecessary additional step in automating boot flows.
> > > 
> > > Add a simple image header that boot loaders can parse and directly
> > > load kernel flat Image. The existing booting methods will continue to
> > > work as it is.
> > > 
> > > Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.
> > > 
> > > Signed-off-by: Atish Patra <[e-mail]>
> > > ---
> > >   arch/riscv/include/asm/image.h | 32 ++++++++++++++++++++++++++++++++
> > >   arch/riscv/kernel/head.S       | 28 ++++++++++++++++++++++++++++
> > >   2 files changed, 60 insertions(+)
> > >   create mode 100644 arch/riscv/include/asm/image.h
> > > 
> > > diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
> > > new file mode 100644
> > > index 000000000000..76a7e0d4068a
> > > --- /dev/null
> > > +++ b/arch/riscv/include/asm/image.h
> > > @@ -0,0 +1,32 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +
> > > +#ifndef __ASM_IMAGE_H
> > > +#define __ASM_IMAGE_H
> > > +
> > > +#define RISCV_IMAGE_MAGIC	"RISCV"
> > > +
> > > +#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
> > > + * @reserved:		reserved
> > > + * @magic:		Magic number
> > > + * @reserved:		reserved
> > > + */
> > > +
> > > +struct riscv_image_header {
> > > +	u32 code0;
> > > +	u32 code1;
> > > +	u64 text_offset;
> > > +	u64 image_size;
> > > +	u64 res1;
> > > +	u64 magic;
> > > +	u32 res2;
> > > +	u32 res3;
> > > +};
> > 
> > I don't want to invent our own file format.  Is there a reason we can't just
> > use something standard?  Off the top of my head I can think of ELF files and
> > multiboot.
> 
> Additional header is required to accommodate PE header format. Currently,
> this is only used for booti command but it will be reused for EFI headers as
> well. Linux kernel Image can pretend as an EFI application if PE/COFF header
> is present. This removes the need of an explicit EFI boot loader and EFI
> firmware can directly load Linux (obviously after EFI stub implementation
> for RISC-V).

Adding the EFI stub on arm64 required very careful consideration of our
Image header and the EFI spec, along with the PE/COFF spec.

For example, to be a compliant PE/COFF header, the first two bytes of
your kernel image need to be "MZ" in ASCII. On arm64 we happened to find
a valid instruction that we could rely upon that met this requirement...

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

... but it's not clear to me if this instruction meets that requriement.

I would strongly encourage you to consider what you actually need for a
compliant EFI header before you set the rest of this ABI in stone.

On arm64 we also had issues with endianness, and I would strongly
recommend that you define how big/little endian will work ahead of time.
e.g. whether fields are always in a fixed endianness.

Thanks,
Mark.

Anup Patel May 1, 2019, 5:02 p.m. UTC | #5

On Wed, May 1, 2019 at 10:14 PM Karsten Merker <[e-mail]> wrote:
>
> On Mon, Apr 29, 2019 at 10:42:40PM -0700, Atish Patra wrote:
> > On 4/29/19 4:40 PM, Palmer Dabbelt wrote:
> > > On Tue, 23 Apr 2019 16:25:06 PDT (-0700), [e-mail] wrote:
> > > > Currently, last stage boot loaders such as U-Boot can accept only
> > > > uImage which is an unnecessary additional step in automating boot flows.
> > > >
> > > > Add a simple image header that boot loaders can parse and directly
> > > > load kernel flat Image. The existing booting methods will continue to
> > > > work as it is.
> > > >
> > > > Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.
> > > >
> > > > Signed-off-by: Atish Patra <[e-mail]>
> > > > ---
> > > >   arch/riscv/include/asm/image.h | 32 ++++++++++++++++++++++++++++++++
> > > >   arch/riscv/kernel/head.S       | 28 ++++++++++++++++++++++++++++
> > > >   2 files changed, 60 insertions(+)
> > > >   create mode 100644 arch/riscv/include/asm/image.h
> > > >
> > > > diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
> > > > new file mode 100644
> > > > index 000000000000..76a7e0d4068a
> > > > --- /dev/null
> > > > +++ b/arch/riscv/include/asm/image.h
> > > > @@ -0,0 +1,32 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +
> > > > +#ifndef __ASM_IMAGE_H
> > > > +#define __ASM_IMAGE_H
> > > > +
> > > > +#define RISCV_IMAGE_MAGIC        "RISCV"
> > > > +
> > > > +#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
> > > > + * @reserved:            reserved
> > > > + * @magic:               Magic number
> > > > + * @reserved:            reserved
> > > > + */
> > > > +
> > > > +struct riscv_image_header {
> > > > + u32 code0;
> > > > + u32 code1;
> > > > + u64 text_offset;
> > > > + u64 image_size;
> > > > + u64 res1;
> > > > + u64 magic;
> > > > + u32 res2;
> > > > + u32 res3;
> > > > +};
> > >
> > > I don't want to invent our own file format.  Is there a reason we can't just
> > > use something standard?  Off the top of my head I can think of ELF files and
> > > multiboot.
> >
> > Additional header is required to accommodate PE header format. Currently,
> > this is only used for booti command but it will be reused for EFI headers as
> > well. Linux kernel Image can pretend as an EFI application if PE/COFF header
> > is present. This removes the need of an explicit EFI boot loader and EFI
> > firmware can directly load Linux (obviously after EFI stub implementation
> > for RISC-V).
> >
> > ARM64 follows the similar header format as well.
> > https://www.kernel.org/doc/Documentation/arm64/booting.txt
>
> Hello Atish,
>
> the arm64 header looks a bit different (quoted from the
> aforementioned URL):
>
>   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 */
>   u64 res2      = 0;            /* reserved */
>   u64 res3      = 0;            /* reserved */
>   u64 res4      = 0;            /* reserved */
>   u32 magic     = 0x644d5241;   /* Magic number, little endian, "ARM\x64" */
>   u32 res5;                     /* reserved (used for PE COFF offset) */
>
> What I am unclear about is in which ways a RISC-V PE/COFF header
> differs from an arm64 one as the arm64 struct is longer than your
> RISC-V header and for arm64 the PE offset field is in the last
> field, i.e. outside of the area covered by your RISC-V structure
> definition.  Can you perhaps explain this part in a bit more
> detail or does anybody else have a pointer to a specification of
> the RISC-V PE/COFF header format (I have found a lot of documents
> about COFF in general, but nothing specific to RISC-V).

The only difference compared to ARM64 is the values of code0, code1
and res5 fields.

As-per PE/COFF, the 32bit value at offset 0x3c tells us offset of PE/COFF
header in image.

For more details refer,
https://en.wikipedia.org/wiki/Portable_Executable
https://en.wikipedia.org/wiki/Portable_Executable#/media/File:Portable_Executable_32_bit_Structure_in_SVG_fixed.svg

For both ARM64 header and RISC-V image header, is actually the
"DOS header" part of PE/COFF format.

This patch only adds "DOS header" part of PE/COFF format. Rest of
the PE/COFF header will be added when add EFI support to Linux
RISC-V kernel.

Regards,
Anup

Anup Patel May 1, 2019, 5:11 p.m. UTC | #6

On Wed, May 1, 2019 at 10:30 PM Mark Rutland <[e-mail]> wrote:
>
> On Mon, Apr 29, 2019 at 10:42:40PM -0700, Atish Patra wrote:
> > On 4/29/19 4:40 PM, Palmer Dabbelt wrote:
> > > On Tue, 23 Apr 2019 16:25:06 PDT (-0700), [e-mail] wrote:
> > > > Currently, last stage boot loaders such as U-Boot can accept only
> > > > uImage which is an unnecessary additional step in automating boot flows.
> > > >
> > > > Add a simple image header that boot loaders can parse and directly
> > > > load kernel flat Image. The existing booting methods will continue to
> > > > work as it is.
> > > >
> > > > Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.
> > > >
> > > > Signed-off-by: Atish Patra <[e-mail]>
> > > > ---
> > > >   arch/riscv/include/asm/image.h | 32 ++++++++++++++++++++++++++++++++
> > > >   arch/riscv/kernel/head.S       | 28 ++++++++++++++++++++++++++++
> > > >   2 files changed, 60 insertions(+)
> > > >   create mode 100644 arch/riscv/include/asm/image.h
> > > >
> > > > diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
> > > > new file mode 100644
> > > > index 000000000000..76a7e0d4068a
> > > > --- /dev/null
> > > > +++ b/arch/riscv/include/asm/image.h
> > > > @@ -0,0 +1,32 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +
> > > > +#ifndef __ASM_IMAGE_H
> > > > +#define __ASM_IMAGE_H
> > > > +
> > > > +#define RISCV_IMAGE_MAGIC        "RISCV"
> > > > +
> > > > +#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
> > > > + * @reserved:            reserved
> > > > + * @magic:               Magic number
> > > > + * @reserved:            reserved
> > > > + */
> > > > +
> > > > +struct riscv_image_header {
> > > > + u32 code0;
> > > > + u32 code1;
> > > > + u64 text_offset;
> > > > + u64 image_size;
> > > > + u64 res1;
> > > > + u64 magic;
> > > > + u32 res2;
> > > > + u32 res3;
> > > > +};
> > >
> > > I don't want to invent our own file format.  Is there a reason we can't just
> > > use something standard?  Off the top of my head I can think of ELF files and
> > > multiboot.
> >
> > Additional header is required to accommodate PE header format. Currently,
> > this is only used for booti command but it will be reused for EFI headers as
> > well. Linux kernel Image can pretend as an EFI application if PE/COFF header
> > is present. This removes the need of an explicit EFI boot loader and EFI
> > firmware can directly load Linux (obviously after EFI stub implementation
> > for RISC-V).
>
> Adding the EFI stub on arm64 required very careful consideration of our
> Image header and the EFI spec, along with the PE/COFF spec.
>
> For example, to be a compliant PE/COFF header, the first two bytes of
> your kernel image need to be "MZ" in ASCII. On arm64 we happened to find
> a valid instruction that we could rely upon that met this requirement...

The "MZ" ASCII (i.e. 0x5a4d) is "li s4,-13" instruction in RISC-V so this
modifies "s4" register which is pretty harmless from Linux RISC-V booting
perspective.

Of course, we should only add "MZ" ASCII in Linux RISC-V image header
when CONFIG_EFI is enabled (just like Linux ARM64).

>
> > > >   __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
>
> ... but it's not clear to me if this instruction meets that requriement.
>
> I would strongly encourage you to consider what you actually need for a
> compliant EFI header before you set the rest of this ABI in stone.
>
> On arm64 we also had issues with endianness, and I would strongly
> recommend that you define how big/little endian will work ahead of time.
> e.g. whether fields are always in a fixed endianness.

As of now RISC-V is little-endian but if big-endian show-up in-future
then we should consider endianness issue.

Regards,
Anup

Atish Patra May 1, 2019, 5:32 p.m. UTC | #7

On 5/1/19 10:02 AM, Anup Patel wrote:
> On Wed, May 1, 2019 at 10:14 PM Karsten Merker <[e-mail]> wrote:
>>
>> On Mon, Apr 29, 2019 at 10:42:40PM -0700, Atish Patra wrote:
>>> On 4/29/19 4:40 PM, Palmer Dabbelt wrote:
>>>> On Tue, 23 Apr 2019 16:25:06 PDT (-0700), [e-mail] wrote:
>>>>> Currently, last stage boot loaders such as U-Boot can accept only
>>>>> uImage which is an unnecessary additional step in automating boot flows.
>>>>>
>>>>> Add a simple image header that boot loaders can parse and directly
>>>>> load kernel flat Image. The existing booting methods will continue to
>>>>> work as it is.
>>>>>
>>>>> Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.
>>>>>
>>>>> Signed-off-by: Atish Patra <[e-mail]>
>>>>> ---
>>>>>    arch/riscv/include/asm/image.h | 32 ++++++++++++++++++++++++++++++++
>>>>>    arch/riscv/kernel/head.S       | 28 ++++++++++++++++++++++++++++
>>>>>    2 files changed, 60 insertions(+)
>>>>>    create mode 100644 arch/riscv/include/asm/image.h
>>>>>
>>>>> diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
>>>>> new file mode 100644
>>>>> index 000000000000..76a7e0d4068a
>>>>> --- /dev/null
>>>>> +++ b/arch/riscv/include/asm/image.h
>>>>> @@ -0,0 +1,32 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>> +
>>>>> +#ifndef __ASM_IMAGE_H
>>>>> +#define __ASM_IMAGE_H
>>>>> +
>>>>> +#define RISCV_IMAGE_MAGIC        "RISCV"
>>>>> +
>>>>> +#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
>>>>> + * @reserved:            reserved
>>>>> + * @magic:               Magic number
>>>>> + * @reserved:            reserved
>>>>> + */
>>>>> +
>>>>> +struct riscv_image_header {
>>>>> + u32 code0;
>>>>> + u32 code1;
>>>>> + u64 text_offset;
>>>>> + u64 image_size;
>>>>> + u64 res1;
>>>>> + u64 magic;
>>>>> + u32 res2;
>>>>> + u32 res3;
>>>>> +};
>>>>
>>>> I don't want to invent our own file format.  Is there a reason we can't just
>>>> use something standard?  Off the top of my head I can think of ELF files and
>>>> multiboot.
>>>
>>> Additional header is required to accommodate PE header format. Currently,
>>> this is only used for booti command but it will be reused for EFI headers as
>>> well. Linux kernel Image can pretend as an EFI application if PE/COFF header
>>> is present. This removes the need of an explicit EFI boot loader and EFI
>>> firmware can directly load Linux (obviously after EFI stub implementation
>>> for RISC-V).
>>>
>>> ARM64 follows the similar header format as well.
>>> https://www.kernel.org/doc/Documentation/arm64/booting.txt
>>
>> Hello Atish,
>>
>> the arm64 header looks a bit different (quoted from the
>> aforementioned URL):
>>
>>    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 */
>>    u64 res2      = 0;            /* reserved */
>>    u64 res3      = 0;            /* reserved */
>>    u64 res4      = 0;            /* reserved */
>>    u32 magic     = 0x644d5241;   /* Magic number, little endian, "ARM\x64" */
>>    u32 res5;                     /* reserved (used for PE COFF offset) */
>>
>> What I am unclear about is in which ways a RISC-V PE/COFF header
>> differs from an arm64 one as the arm64 struct is longer than your
>> RISC-V header and for arm64 the PE offset field is in the last
>> field, i.e. outside of the area covered by your RISC-V structure
>> definition.  Can you perhaps explain this part in a bit more
>> detail or does anybody else have a pointer to a specification of
>> the RISC-V PE/COFF header format (I have found a lot of documents
>> about COFF in general, but nothing specific to RISC-V).
> 

Karsten,

> The only difference compared to ARM64 is the values of code0, code1
> and res5 fields.
> 
> As-per PE/COFF, the 32bit value at offset 0x3c tells us offset of PE/COFF
> header in image.
> 
> For more details refer,
> https://en.wikipedia.org/wiki/Portable_Executable
> https://en.wikipedia.org/wiki/Portable_Executable#/media/File:Portable_Executable_32_bit_Structure_in_SVG_fixed.svg
> 
> For both ARM64 header and RISC-V image header, is actually the
> "DOS header" part of PE/COFF format.
> 
> This patch only adds "DOS header" part of PE/COFF format. Rest of
> the PE/COFF header will be added when add EFI support to Linux
> RISC-V kernel.
> I think Anup answered your question. The original plan was to add EFI 
specific stuff in EFI support patch. That includes adjusting the PE/COFF 
offset at 0x3c and adding the "MZ" value for code0 if EFI is enabled.

In hindsight, I think it created more confusion. I will update the 
"riscv_image_header" structure to put PE/COFF offset(0x3c) at right 
place in v2 patch to avoid further confusion.

"MZ" value part should be added once EFI is enabled.

I will update the comments on riscv/include/asm/image.h as well to 
clarify more.

Regards,
Atish
> Regards,
> Anup
>

Mark Rutland May 1, 2019, 5:35 p.m. UTC | #8

On Wed, May 01, 2019 at 10:41:52PM +0530, Anup Patel wrote:
> On Wed, May 1, 2019 at 10:30 PM Mark Rutland <[e-mail]> wrote:
> >
> > On Mon, Apr 29, 2019 at 10:42:40PM -0700, Atish Patra wrote:
> > > On 4/29/19 4:40 PM, Palmer Dabbelt wrote:
> > > > On Tue, 23 Apr 2019 16:25:06 PDT (-0700), [e-mail] wrote:
> > > > > Currently, last stage boot loaders such as U-Boot can accept only
> > > > > uImage which is an unnecessary additional step in automating boot flows.
> > > > >
> > > > > Add a simple image header that boot loaders can parse and directly
> > > > > load kernel flat Image. The existing booting methods will continue to
> > > > > work as it is.
> > > > >
> > > > > Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.
> > > > >
> > > > > Signed-off-by: Atish Patra <[e-mail]>
> > > > > ---
> > > > >   arch/riscv/include/asm/image.h | 32 ++++++++++++++++++++++++++++++++
> > > > >   arch/riscv/kernel/head.S       | 28 ++++++++++++++++++++++++++++
> > > > >   2 files changed, 60 insertions(+)
> > > > >   create mode 100644 arch/riscv/include/asm/image.h
> > > > >
> > > > > diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
> > > > > new file mode 100644
> > > > > index 000000000000..76a7e0d4068a
> > > > > --- /dev/null
> > > > > +++ b/arch/riscv/include/asm/image.h
> > > > > @@ -0,0 +1,32 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > +
> > > > > +#ifndef __ASM_IMAGE_H
> > > > > +#define __ASM_IMAGE_H
> > > > > +
> > > > > +#define RISCV_IMAGE_MAGIC        "RISCV"
> > > > > +
> > > > > +#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
> > > > > + * @reserved:            reserved
> > > > > + * @magic:               Magic number
> > > > > + * @reserved:            reserved
> > > > > + */
> > > > > +
> > > > > +struct riscv_image_header {
> > > > > + u32 code0;
> > > > > + u32 code1;
> > > > > + u64 text_offset;
> > > > > + u64 image_size;
> > > > > + u64 res1;
> > > > > + u64 magic;
> > > > > + u32 res2;
> > > > > + u32 res3;
> > > > > +};
> > > >
> > > > I don't want to invent our own file format.  Is there a reason we can't just
> > > > use something standard?  Off the top of my head I can think of ELF files and
> > > > multiboot.
> > >
> > > Additional header is required to accommodate PE header format. Currently,
> > > this is only used for booti command but it will be reused for EFI headers as
> > > well. Linux kernel Image can pretend as an EFI application if PE/COFF header
> > > is present. This removes the need of an explicit EFI boot loader and EFI
> > > firmware can directly load Linux (obviously after EFI stub implementation
> > > for RISC-V).
> >
> > Adding the EFI stub on arm64 required very careful consideration of our
> > Image header and the EFI spec, along with the PE/COFF spec.
> >
> > For example, to be a compliant PE/COFF header, the first two bytes of
> > your kernel image need to be "MZ" in ASCII. On arm64 we happened to find
> > a valid instruction that we could rely upon that met this requirement...
> 
> The "MZ" ASCII (i.e. 0x5a4d) is "li s4,-13" instruction in RISC-V so this
> modifies "s4" register which is pretty harmless from Linux RISC-V booting
> perspective.
> 
> Of course, we should only add "MZ" ASCII in Linux RISC-V image header
> when CONFIG_EFI is enabled (just like Linux ARM64).

Great. It would probably be worth just mentioning that in the commit
message, so that it's clear that has been considered.

Thanks,
Mark.

Karsten Merker May 1, 2019, 7:54 p.m. UTC | #9

On Wed, May 01, 2019 at 10:41:52PM +0530, Anup Patel wrote:
> On Wed, May 1, 2019 at 10:30 PM Mark Rutland <[e-mail]> wrote:
> > On Mon, Apr 29, 2019 at 10:42:40PM -0700, Atish Patra wrote:
> > > On 4/29/19 4:40 PM, Palmer Dabbelt wrote:
> > > > On Tue, 23 Apr 2019 16:25:06 PDT (-0700), [e-mail] wrote:
> > > > > Currently, last stage boot loaders such as U-Boot can accept only
> > > > > uImage which is an unnecessary additional step in automating boot flows.
> > > > >
> > > > > Add a simple image header that boot loaders can parse and directly
> > > > > load kernel flat Image. The existing booting methods will continue to
> > > > > work as it is.
> > > > >
> > > > > Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.
> > > > >
> > > > > Signed-off-by: Atish Patra <[e-mail]>
> > > > > ---
> > > > >   arch/riscv/include/asm/image.h | 32 ++++++++++++++++++++++++++++++++
> > > > >   arch/riscv/kernel/head.S       | 28 ++++++++++++++++++++++++++++
> > > > >   2 files changed, 60 insertions(+)
> > > > >   create mode 100644 arch/riscv/include/asm/image.h
> > > > >
> > > > > diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
> > > > > new file mode 100644
> > > > > index 000000000000..76a7e0d4068a
> > > > > --- /dev/null
> > > > > +++ b/arch/riscv/include/asm/image.h
> > > > > @@ -0,0 +1,32 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > +
> > > > > +#ifndef __ASM_IMAGE_H
> > > > > +#define __ASM_IMAGE_H
> > > > > +
> > > > > +#define RISCV_IMAGE_MAGIC        "RISCV"
> > > > > +
> > > > > +#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
> > > > > + * @reserved:            reserved
> > > > > + * @magic:               Magic number
> > > > > + * @reserved:            reserved
> > > > > + */
> > > > > +
> > > > > +struct riscv_image_header {
> > > > > + u32 code0;
> > > > > + u32 code1;
> > > > > + u64 text_offset;
> > > > > + u64 image_size;
> > > > > + u64 res1;
> > > > > + u64 magic;
> > > > > + u32 res2;
> > > > > + u32 res3;
> > > > > +};
> > > >
> > > > I don't want to invent our own file format.  Is there a reason we can't just
> > > > use something standard?  Off the top of my head I can think of ELF files and
> > > > multiboot.
> > >
> > > Additional header is required to accommodate PE header format. Currently,
> > > this is only used for booti command but it will be reused for EFI headers as
> > > well. Linux kernel Image can pretend as an EFI application if PE/COFF header
> > > is present. This removes the need of an explicit EFI boot loader and EFI
> > > firmware can directly load Linux (obviously after EFI stub implementation
> > > for RISC-V).
> >
> > Adding the EFI stub on arm64 required very careful consideration of our
> > Image header and the EFI spec, along with the PE/COFF spec.
> >
> > For example, to be a compliant PE/COFF header, the first two bytes of
> > your kernel image need to be "MZ" in ASCII. On arm64 we happened to find
> > a valid instruction that we could rely upon that met this requirement...
> 
> The "MZ" ASCII (i.e. 0x5a4d) is "li s4,-13" instruction in RISC-V so this
> modifies "s4" register which is pretty harmless from Linux RISC-V booting
> perspective.
> 
> Of course, we should only add "MZ" ASCII in Linux RISC-V image header
> when CONFIG_EFI is enabled (just like Linux ARM64).

Probably I'm missing something obvious, but I cannot completely
follow you here. My understanding is as follows:

The kernel gets executed by jumping to the _start label, where it
currently immediately starts with setting up everything (mask
interrupts, disable FPU, etc).  Now we insert a structure before
the actual init code where the first 64 bits of the structure
(the code0 and code1 fields) are filled with values that
constitute a valid jump opcode to the actual init code behind the
end of the new structure.

If the first byte in a PE/COFF header has to be an ASCII "M",
that is 01001101 in binary.  RISC-V is little-endian and the last
two bits of the lowest-value byte define the type of instruction. 
According to the chapter "Base Instruction-Length Encoding" in
the RISC-V ISA spec everything except 11 as the lowest bits
denotes a compressed instruction and if I have puzzeled together
the the various instruction bits correctly, ASCII "MZ" would be
excuted as a compressed load immediate to x9/s1, wouldn't it?

If my previous thoughts are correct, this would also mean that
having a PE/COFF-compatible header for EFI boot would make a
kernel image inherently incompatible with performing a non-EFI
boot of the same image on systems that don't implement the C
extension.  As the work-in-progress RISC-V Unix platform spec
will probably mandate C, that probably won't be much of a
practical problem, though.

Regards,
Karsten

Karsten Merker May 1, 2019, 8:32 p.m. UTC | #10

On Wed, May 01, 2019 at 09:54:43PM +0200, Karsten Merker wrote:
> On Wed, May 01, 2019 at 10:41:52PM +0530, Anup Patel wrote:
> > On Wed, May 1, 2019 at 10:30 PM Mark Rutland <[e-mail]> wrote:
> > > On Mon, Apr 29, 2019 at 10:42:40PM -0700, Atish Patra wrote:
> > > > On 4/29/19 4:40 PM, Palmer Dabbelt wrote:
> > > > > On Tue, 23 Apr 2019 16:25:06 PDT (-0700), [e-mail] wrote:

> Probably I'm missing something obvious, but I cannot completely
> follow you here. My understanding is as follows:
[...]
> If the first byte in a PE/COFF header has to be an ASCII "M",
> that is 01001101 in binary.  RISC-V is little-endian and the last
> two bits of the lowest-value byte define the type of instruction. 
> According to the chapter "Base Instruction-Length Encoding" in
> the RISC-V ISA spec everything except 11 as the lowest bits
> denotes a compressed instruction and if I have puzzeled together
> the the various instruction bits correctly, ASCII "MZ" would be
> excuted as a compressed load immediate to x9/s1, wouldn't it?

Sorry, I have misinterpreted a bitfield in the spec, it's indeed
a compressed load immediate to x20/s4.

Regards,
Karsten

Patch

diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
new file mode 100644
index 000000000000..76a7e0d4068a
--- /dev/null
+++ b/arch/riscv/include/asm/image.h
@@ -0,0 +1,32 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __ASM_IMAGE_H
+#define __ASM_IMAGE_H
+
+#define RISCV_IMAGE_MAGIC	"RISCV"
+
+#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
+ * @reserved:		reserved
+ * @magic:		Magic number
+ * @reserved:		reserved
+ */
+
+struct riscv_image_header {
+	u32 code0;
+	u32 code1;
+	u64 text_offset;
+	u64 image_size;
+	u64 res1;
+	u64 magic;
+	u32 res2;
+	u32 res3;
+};
+#endif /* __ASSEMBLY__ */
+#endif /* __ASM_IMAGE_H */
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index fe884cd69abd..154647395601 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -19,9 +19,37 @@ 
 #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 0
+	.asciz RISCV_IMAGE_MAGIC
+	.word 0
+	.word 0
+
+.global _start_kernel
+_start_kernel:
 	/* Mask all interrupts */
 	csrw sie, zero