diff mbox series

riscv: modify the Image header to improve compatibility with the ARM64 header

Message ID alpine.DEB.2.21.9999.1909132005200.24255@viisi.sifive.com (mailing list archive)
State New, archived
Headers show
Series riscv: modify the Image header to improve compatibility with the ARM64 header | expand

Commit Message

Paul Walmsley Sept. 14, 2019, 3:08 a.m. UTC 
Part of the intention during the definition of the RISC-V kernel image
header was to lay the groundwork for a future merge with the ARM64
image header.  One error during my original review was not noticing
that the RISC-V header's "magic" field was at a different size and
position than the ARM64's "magic" field.  If the existing ARM64 Image
header parsing code were to attempt to parse an existing RISC-V kernel
image header format, it would see a magic number 0.  This is
undesirable, since it's our intention to align as closely as possible
with the ARM64 header format.  Another problem was that the original
"res3" field was not being initialized correctly to zero.

Address these issues by creating a 32-bit "magic2" field in the RISC-V
header which matches the ARM64 "magic" field.  RISC-V binaries will
store "RSC\x05" in this field.  The intention is that the use of the
existing 64-bit "magic" field in the RISC-V header will be deprecated
over time.  Increment the minor version number of the file format to
indicate this change, and update the documentation accordingly.  Fix
the assembler directives in head.S to ensure that reserved fields are
properly zero-initialized.

Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
Reported-by: Palmer Dabbelt <palmer@sifive.com>
Cc: Atish Patra <atish.patra@wdc.com>
Cc: Karsten Merker <merker@debian.org>
---
Will try to get this merged before v5.3, to minimize the delta with the 
ARM64 header in the final release.

 Documentation/riscv/boot-image-header.txt | 13 +++++++------
 arch/riscv/include/asm/image.h            | 12 ++++++------
 arch/riscv/kernel/head.S                  |  4 ++--
 3 files changed, 15 insertions(+), 14 deletions(-)

Comments

Atish Patra Sept. 14, 2019, 4:03 a.m. UTC | #1 
On Fri, 2019-09-13 at 20:08 -0700, Paul Walmsley wrote:
> Part of the intention during the definition of the RISC-V kernel
> image
> header was to lay the groundwork for a future merge with the ARM64
> image header.  One error during my original review was not noticing
> that the RISC-V header's "magic" field was at a different size and
> position than the ARM64's "magic" field.  If the existing ARM64 Image
> header parsing code were to attempt to parse an existing RISC-V
> kernel
> image header format, it would see a magic number 0.  This is
> undesirable, since it's our intention to align as closely as possible
> with the ARM64 header format.  Another problem was that the original
> "res3" field was not being initialized correctly to zero.
> 
> Address these issues by creating a 32-bit "magic2" field in the RISC-
> V
> header which matches the ARM64 "magic" field.  RISC-V binaries will
> store "RSC\x05" in this field.  The intention is that the use of the
> existing 64-bit "magic" field in the RISC-V header will be deprecated
> over time.  Increment the minor version number of the file format to
> indicate this change, and update the documentation accordingly.  Fix
> the assembler directives in head.S to ensure that reserved fields are
> properly zero-initialized.
> 

Thanks for the quick fix. Is there a planned timeline when we can
remove the deprecated magic ?

I was planning to send a U-boot header documentation patch to match
Linux one anyways. Do you want me that to rebase based on this patch or
are you planning to send a U-boot patch as well ?

> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> Reported-by: Palmer Dabbelt <palmer@sifive.com>
> Cc: Atish Patra <atish.patra@wdc.com>
> Cc: Karsten Merker <merker@debian.org>
> ---
> Will try to get this merged before v5.3, to minimize the delta with
> the 
> ARM64 header in the final release.
> 
>  Documentation/riscv/boot-image-header.txt | 13 +++++++------
>  arch/riscv/include/asm/image.h            | 12 ++++++------
>  arch/riscv/kernel/head.S                  |  4 ++--
>  3 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/riscv/boot-image-header.txt
> b/Documentation/riscv/boot-image-header.txt
> index 1b73fea23b39..14b1492f689b 100644
> --- a/Documentation/riscv/boot-image-header.txt
> +++ b/Documentation/riscv/boot-image-header.txt

There is a patch already queued that changed the format to ReST. You
need to rebase the patch accordingly

https://lkml.org/lkml/2019/7/26/1321

> @@ -18,7 +18,7 @@ The following 64-byte header is present in
> decompressed Linux kernel image.
>  	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 magic2 = 0x56534905;  /* Magic number 2, little endian,
> "RSC\x05" */
>  	u32 res4;		  /* Reserved for PE COFF offset */
>  
>  This header format is compliant with PE/COFF header and largely
> inspired from
> @@ -37,13 +37,14 @@ Notes:
>  	Bits 16:31 - Major version
>  
>    This preserves compatibility across newer and older version of the
> header.
> -  The current version is defined as 0.1.
> +  The current version is defined as 0.2.
>  
> -- 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.
> +- The "magic" field is deprecated as of version 0.2.  In a future
> +  release, it may be removed.  This originally should have matched
> up
> +  with the ARM64 header "magic" field, but unfortunately does not.
> +  The "magic2" field replaces it, matching up with the ARM64 header.
>  
> -- In current header, the flag field has only one field.
> +- In current header, the flags 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
> diff --git a/arch/riscv/include/asm/image.h
> b/arch/riscv/include/asm/image.h
> index ef28e106f247..344db5244547 100644
> --- a/arch/riscv/include/asm/image.h
> +++ b/arch/riscv/include/asm/image.h
> @@ -3,7 +3,8 @@
>  #ifndef __ASM_IMAGE_H
>  #define __ASM_IMAGE_H
>  
> -#define RISCV_IMAGE_MAGIC	"RISCV"
> +#define RISCV_IMAGE_MAGIC	"RISCV\0\0\0"
> +#define RISCV_IMAGE_MAGIC2	"RSC\x05"
>  
>  #define RISCV_IMAGE_FLAG_BE_SHIFT	0
>  #define RISCV_IMAGE_FLAG_BE_MASK	0x1
> @@ -23,7 +24,7 @@
>  #define __HEAD_FLAGS		(__HEAD_FLAG(BE))
>  
>  #define RISCV_HEADER_VERSION_MAJOR 0
> -#define RISCV_HEADER_VERSION_MINOR 1
> +#define RISCV_HEADER_VERSION_MINOR 2
>  
>  #define RISCV_HEADER_VERSION (RISCV_HEADER_VERSION_MAJOR << 16 | \
>  			      RISCV_HEADER_VERSION_MINOR)
> @@ -39,9 +40,8 @@
>   * @version:		version
>   * @res1:		reserved
>   * @res2:		reserved
> - * @magic:		Magic number
> - * @res3:		reserved (will be used for additional RISC-V
> specific
> - *			header)
> + * @magic:		Magic number (RISC-V specific; deprecated)
> + * @magic2:		Magic number 2 (to match the ARM64 'magic'
> field pos)
>   * @res4:		reserved (will be used for PE COFF offset)
>   *
>   * The intention is for this header format to be shared between
> multiple
> @@ -58,7 +58,7 @@ struct riscv_image_header {
>  	u32 res1;
>  	u64 res2;
>  	u64 magic;
> -	u32 res3;
> +	u32 magic2;
>  	u32 res4;
>  };
>  #endif /* __ASSEMBLY__ */
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index 0f1ba17e476f..52eec0c1bf30 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -39,9 +39,9 @@ ENTRY(_start)
>  	.word RISCV_HEADER_VERSION
>  	.word 0
>  	.dword 0
> -	.asciz RISCV_IMAGE_MAGIC
> -	.word 0
> +	.ascii RISCV_IMAGE_MAGIC
>  	.balign 4
> +	.ascii RISCV_IMAGE_MAGIC2
>  	.word 0
>  
>  .global _start_kernel
Palmer Dabbelt Sept. 14, 2019, 12:18 p.m. UTC | #2 
On Fri, 13 Sep 2019 20:08:14 PDT (-0700), Paul Walmsley wrote:
>
> Part of the intention during the definition of the RISC-V kernel image
> header was to lay the groundwork for a future merge with the ARM64
> image header.  One error during my original review was not noticing
> that the RISC-V header's "magic" field was at a different size and
> position than the ARM64's "magic" field.  If the existing ARM64 Image
> header parsing code were to attempt to parse an existing RISC-V kernel
> image header format, it would see a magic number 0.  This is
> undesirable, since it's our intention to align as closely as possible
> with the ARM64 header format.  Another problem was that the original
> "res3" field was not being initialized correctly to zero.
>
> Address these issues by creating a 32-bit "magic2" field in the RISC-V
> header which matches the ARM64 "magic" field.  RISC-V binaries will
> store "RSC\x05" in this field.  The intention is that the use of the
> existing 64-bit "magic" field in the RISC-V header will be deprecated
> over time.  Increment the minor version number of the file format to
> indicate this change, and update the documentation accordingly.  Fix
> the assembler directives in head.S to ensure that reserved fields are
> properly zero-initialized.
>
> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> Reported-by: Palmer Dabbelt <palmer@sifive.com>
> Cc: Atish Patra <atish.patra@wdc.com>
> Cc: Karsten Merker <merker@debian.org>
> ---
> Will try to get this merged before v5.3, to minimize the delta with the
> ARM64 header in the final release.
>
>  Documentation/riscv/boot-image-header.txt | 13 +++++++------
>  arch/riscv/include/asm/image.h            | 12 ++++++------
>  arch/riscv/kernel/head.S                  |  4 ++--
>  3 files changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/riscv/boot-image-header.txt b/Documentation/riscv/boot-image-header.txt
> index 1b73fea23b39..14b1492f689b 100644
> --- a/Documentation/riscv/boot-image-header.txt
> +++ b/Documentation/riscv/boot-image-header.txt
> @@ -18,7 +18,7 @@ The following 64-byte header is present in decompressed Linux kernel image.
>  	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 magic2 = 0x56534905;  /* Magic number 2, little endian, "RSC\x05" */
>  	u32 res4;		  /* Reserved for PE COFF offset */
>
>  This header format is compliant with PE/COFF header and largely inspired from
> @@ -37,13 +37,14 @@ Notes:
>  	Bits 16:31 - Major version
>
>    This preserves compatibility across newer and older version of the header.
> -  The current version is defined as 0.1.
> +  The current version is defined as 0.2.
>
> -- 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.
> +- The "magic" field is deprecated as of version 0.2.  In a future
> +  release, it may be removed.  This originally should have matched up
> +  with the ARM64 header "magic" field, but unfortunately does not.
> +  The "magic2" field replaces it, matching up with the ARM64 header.
>
> -- In current header, the flag field has only one field.
> +- In current header, the flags 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
> diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
> index ef28e106f247..344db5244547 100644
> --- a/arch/riscv/include/asm/image.h
> +++ b/arch/riscv/include/asm/image.h
> @@ -3,7 +3,8 @@
>  #ifndef __ASM_IMAGE_H
>  #define __ASM_IMAGE_H
>
> -#define RISCV_IMAGE_MAGIC	"RISCV"
> +#define RISCV_IMAGE_MAGIC	"RISCV\0\0\0"
> +#define RISCV_IMAGE_MAGIC2	"RSC\x05"
>
>  #define RISCV_IMAGE_FLAG_BE_SHIFT	0
>  #define RISCV_IMAGE_FLAG_BE_MASK	0x1
> @@ -23,7 +24,7 @@
>  #define __HEAD_FLAGS		(__HEAD_FLAG(BE))
>
>  #define RISCV_HEADER_VERSION_MAJOR 0
> -#define RISCV_HEADER_VERSION_MINOR 1
> +#define RISCV_HEADER_VERSION_MINOR 2
>
>  #define RISCV_HEADER_VERSION (RISCV_HEADER_VERSION_MAJOR << 16 | \
>  			      RISCV_HEADER_VERSION_MINOR)
> @@ -39,9 +40,8 @@
>   * @version:		version
>   * @res1:		reserved
>   * @res2:		reserved
> - * @magic:		Magic number
> - * @res3:		reserved (will be used for additional RISC-V specific
> - *			header)
> + * @magic:		Magic number (RISC-V specific; deprecated)
> + * @magic2:		Magic number 2 (to match the ARM64 'magic' field pos)
>   * @res4:		reserved (will be used for PE COFF offset)
>   *
>   * The intention is for this header format to be shared between multiple
> @@ -58,7 +58,7 @@ struct riscv_image_header {
>  	u32 res1;
>  	u64 res2;
>  	u64 magic;
> -	u32 res3;
> +	u32 magic2;
>  	u32 res4;
>  };
>  #endif /* __ASSEMBLY__ */
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index 0f1ba17e476f..52eec0c1bf30 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -39,9 +39,9 @@ ENTRY(_start)
>  	.word RISCV_HEADER_VERSION
>  	.word 0
>  	.dword 0
> -	.asciz RISCV_IMAGE_MAGIC
> -	.word 0
> +	.ascii RISCV_IMAGE_MAGIC
>  	.balign 4
> +	.ascii RISCV_IMAGE_MAGIC2
>  	.word 0
>
>  .global _start_kernel

Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
Paul Walmsley Sept. 14, 2019, 1:58 p.m. UTC | #3 
On Sat, 14 Sep 2019, Atish Patra wrote:

> Thanks for the quick fix. Is there a planned timeline when we can
> remove the deprecated magic ?

If Linus merges this patch, we should probably start the transition in the 
bootloaders, QEMU, and user tools as quickly as possible.  Probably the 
key element in the timeline is when we remove support for the old 64-bit 
magic number location in the kernel.  I'm told that U-Boot and QEMU have 
already issued releases with support for the v0.1 image header format, so 
dropping the old magic number from the kernel is probably at least a few 
years away.  (This is to increase the likelihood that anyone using the old 
software has had the chance to update them.)

> I was planning to send a U-boot header documentation patch to match
> Linux one anyways. Do you want me that to rebase based on this patch or
> are you planning to send a U-boot patch as well ?

Once v5.3 comes out, please go ahead.


- Paul
diff mbox series

Patch

diff --git a/Documentation/riscv/boot-image-header.txt b/Documentation/riscv/boot-image-header.txt
index 1b73fea23b39..14b1492f689b 100644
--- a/Documentation/riscv/boot-image-header.txt
+++ b/Documentation/riscv/boot-image-header.txt
@@ -18,7 +18,7 @@  The following 64-byte header is present in decompressed Linux kernel image.
 	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 magic2 = 0x56534905;  /* Magic number 2, little endian, "RSC\x05" */
 	u32 res4;		  /* Reserved for PE COFF offset */
 
 This header format is compliant with PE/COFF header and largely inspired from
@@ -37,13 +37,14 @@  Notes:
 	Bits 16:31 - Major version
 
   This preserves compatibility across newer and older version of the header.
-  The current version is defined as 0.1.
+  The current version is defined as 0.2.
 
-- 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.
+- The "magic" field is deprecated as of version 0.2.  In a future
+  release, it may be removed.  This originally should have matched up
+  with the ARM64 header "magic" field, but unfortunately does not.
+  The "magic2" field replaces it, matching up with the ARM64 header.
 
-- In current header, the flag field has only one field.
+- In current header, the flags 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
diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
index ef28e106f247..344db5244547 100644
--- a/arch/riscv/include/asm/image.h
+++ b/arch/riscv/include/asm/image.h
@@ -3,7 +3,8 @@ 
 #ifndef __ASM_IMAGE_H
 #define __ASM_IMAGE_H
 
-#define RISCV_IMAGE_MAGIC	"RISCV"
+#define RISCV_IMAGE_MAGIC	"RISCV\0\0\0"
+#define RISCV_IMAGE_MAGIC2	"RSC\x05"
 
 #define RISCV_IMAGE_FLAG_BE_SHIFT	0
 #define RISCV_IMAGE_FLAG_BE_MASK	0x1
@@ -23,7 +24,7 @@ 
 #define __HEAD_FLAGS		(__HEAD_FLAG(BE))
 
 #define RISCV_HEADER_VERSION_MAJOR 0
-#define RISCV_HEADER_VERSION_MINOR 1
+#define RISCV_HEADER_VERSION_MINOR 2
 
 #define RISCV_HEADER_VERSION (RISCV_HEADER_VERSION_MAJOR << 16 | \
 			      RISCV_HEADER_VERSION_MINOR)
@@ -39,9 +40,8 @@ 
  * @version:		version
  * @res1:		reserved
  * @res2:		reserved
- * @magic:		Magic number
- * @res3:		reserved (will be used for additional RISC-V specific
- *			header)
+ * @magic:		Magic number (RISC-V specific; deprecated)
+ * @magic2:		Magic number 2 (to match the ARM64 'magic' field pos)
  * @res4:		reserved (will be used for PE COFF offset)
  *
  * The intention is for this header format to be shared between multiple
@@ -58,7 +58,7 @@  struct riscv_image_header {
 	u32 res1;
 	u64 res2;
 	u64 magic;
-	u32 res3;
+	u32 magic2;
 	u32 res4;
 };
 #endif /* __ASSEMBLY__ */
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index 0f1ba17e476f..52eec0c1bf30 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -39,9 +39,9 @@  ENTRY(_start)
 	.word RISCV_HEADER_VERSION
 	.word 0
 	.dword 0
-	.asciz RISCV_IMAGE_MAGIC
-	.word 0
+	.ascii RISCV_IMAGE_MAGIC
 	.balign 4
+	.ascii RISCV_IMAGE_MAGIC2
 	.word 0
 
 .global _start_kernel