diff mbox series

xen/riscv: PE/COFF image header for RISC-V target

Message ID 87b5e458498bbff2e54ac011a50ff1f9555c3613.1717354932.git.milan.djokic@rt-rk.com (mailing list archive)
State Superseded
Headers show
Series xen/riscv: PE/COFF image header for RISC-V target | expand

Commit Message

Milan Djokic June 5, 2024, 4:54 p.m. UTC 
From: Nikola Jelic <nikola.jelic@rt-rk.com>

Extended RISC-V xen image with PE/COFF headers,
in order to support xen boot from popular bootloaders like U-boot.
Image header is optionally included (with CONFIG_RISCV_EFI) so
both plain ELF and image with PE/COFF header can now be generated as build artifacts.

Tested on both QEMU and StarFive VisionFive 2 with OpenSBI->U-Boot->xen->dom0 boot chain.

Signed-off-by: Nikola Jelic <nikola.jelic@rt-rk.com>
---
 xen/arch/riscv/Kconfig             |  9 +++++
 xen/arch/riscv/include/asm/image.h | 62 ++++++++++++++++++++++++++++++
 xen/arch/riscv/riscv64/head.S      | 33 +++++++++++++++-
 3 files changed, 103 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/riscv/include/asm/image.h

Comments

Jan Beulich June 6, 2024, 6:52 a.m. UTC | #1 
On 05.06.2024 18:54, milandjokic1995@gmail.com wrote:
> --- a/xen/arch/riscv/Kconfig
> +++ b/xen/arch/riscv/Kconfig
> @@ -9,6 +9,15 @@ config ARCH_DEFCONFIG
>  	string
>  	default "arch/riscv/configs/tiny64_defconfig"
>  
> +config RISCV_EFI
> +	bool "UEFI boot service support"
> +	depends on RISCV_64
> +	default n
> +	help
> +	  This option provides support for boot services through
> +	  UEFI firmware. A UEFI stub is provided to allow Xen to
> +	  be booted as an EFI application.

Hmm, all of this promises quite a bit more than you actually add. If
there are future plans, please clarify in the description. Otherwise
please consider adjusting name, prompt, and help text to actually
cover just what's actually done.

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/image.h

This is pretty generic a name for something pretty specific.

> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +
> +#ifndef _ASM_RISCV_IMAGE_H
> +#define _ASM_RISCV_IMAGE_H
> +
> +#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
> +
> +#define RISCV_IMAGE_FLAG_LE		0
> +#define RISCV_IMAGE_FLAG_BE		1
> +
> +#ifdef CONFIG_CPU_BIG_ENDIAN

I don't think we have such a Kconfig control.

> +#error conversion of header fields to LE not yet implemented
> +#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 2
> +
> +#define RISCV_HEADER_VERSION (RISCV_HEADER_VERSION_MAJOR << 16 | \
> +			      RISCV_HEADER_VERSION_MINOR)

Nit: Indentation of this 2nd line wants to result in the two RISCV_
to be properly aligned.

> +#ifndef __ASSEMBLY__
> +/*
> + * struct riscv_image_header - riscv xen image header
> + *
> + * @code0:		Executable code
> + * @code1:		Executable code
> + * @text_offset:	Image load offset
> + * @image_size:		Effective Image size
> + * @reserved:		reserved
> + * @reserved:		reserved
> + * @reserved:		reserved
> + * @magic:		Magic number
> + * @reserved:		reserved
> + * @reserved:		reserved (will be used for PE COFF offset)
> + */
> +
> +struct riscv_image_header {
> +	u32 code0;
> +	u32 code1;
> +	u64 text_offset;
> +	u64 image_size;
> +	u64 res1;
> +	u64 res2;
> +	u64 res3;
> +	u64 magic;
> +	u32 res4;
> +	u32 res5;

No new uses of u32 / u64 anymore, please. We're in the process of fully
moving to uint<N>_t.

> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -1,14 +1,40 @@
>  #include <asm/asm.h>
>  #include <asm/riscv_encoding.h>
> +#include <asm/image.h>
>  
>          .section .text.header, "ax", %progbits
>  
>          /*
>           * OpenSBI pass to start():
>           *   a0 -> hart_id ( bootcpu_id )
> -         *   a1 -> dtb_base 
> +         *   a1 -> dtb_base
>           */
>  FUNC(start)
> +#ifdef CONFIG_RISCV_EFI
> +        j xen_start

Comparing with what Arm does, shouldn't this similarly resolve to
the MZ pattern in the binary? In which case likely it needs to be
an entirely different insn, if such an insn even exists on RISC-V?
Otherwise the lack of MZ would clearly need explaining in the
description.

> +        /* -----------  Header -------------- */
> +	.word 0

Nit: Please use consistent indentation - either always tabs or
always blanks (matching what existing code uses).

> +	.balign 8
> +#if __riscv_xlen == 64

Wouldn't this better be CONFIG_RISCV_64? We do have #if-s like
this, but in different contexts. Even there I wonder of the
mix - Cc-ing Oleksii to possible comment (you probably should
have Cc-ed him anyway).

> +	/* 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 xen image */
> +	.dword _end - _start
> +	.dword __HEAD_FLAGS
> +	.word RISCV_HEADER_VERSION
> +	.word 0
> +	.dword 0
> +	.ascii RISCV_IMAGE_MAGIC
> +	.balign 4
> +	.ascii RISCV_IMAGE_MAGIC2

There's only one "magic" in the struct further up.

This also isn't quite enough for a PE/COFF image, see again Arm
code. If the other header parts aren't needed, that too would
want mentioning / explaining in the description.

> +FUNC(xen_start)
> +#endif
>          /* Mask all interrupts */
>          csrw    CSR_SIE, zero
>  
> @@ -60,6 +86,11 @@ FUNC(start)
>          mv      a1, s1
>  
>          tail    start_xen
> +
> +#ifdef CONFIG_RISCV_EFI
> +END(xen_start)
> +#endif
> +
>  END(start)

I'm not convinced it is a good idea to have two functions nested
within one another, ELF-annotation-wise.

Jan
diff mbox series

Patch

diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index f382b36f6c..59bf5aa2a6 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -9,6 +9,15 @@  config ARCH_DEFCONFIG
 	string
 	default "arch/riscv/configs/tiny64_defconfig"
 
+config RISCV_EFI
+	bool "UEFI boot service support"
+	depends on RISCV_64
+	default n
+	help
+	  This option provides support for boot services through
+	  UEFI firmware. A UEFI stub is provided to allow Xen to
+	  be booted as an EFI application.
+
 menu "Architecture Features"
 
 source "arch/Kconfig"
diff --git a/xen/arch/riscv/include/asm/image.h b/xen/arch/riscv/include/asm/image.h
new file mode 100644
index 0000000000..b379246290
--- /dev/null
+++ b/xen/arch/riscv/include/asm/image.h
@@ -0,0 +1,62 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+
+
+#ifndef _ASM_RISCV_IMAGE_H
+#define _ASM_RISCV_IMAGE_H
+
+#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
+
+#define RISCV_IMAGE_FLAG_LE		0
+#define RISCV_IMAGE_FLAG_BE		1
+
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#error conversion of header fields to LE not yet implemented
+#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 2
+
+#define RISCV_HEADER_VERSION (RISCV_HEADER_VERSION_MAJOR << 16 | \
+			      RISCV_HEADER_VERSION_MINOR)
+
+#ifndef __ASSEMBLY__
+/*
+ * struct riscv_image_header - riscv xen image header
+ *
+ * @code0:		Executable code
+ * @code1:		Executable code
+ * @text_offset:	Image load offset
+ * @image_size:		Effective Image size
+ * @reserved:		reserved
+ * @reserved:		reserved
+ * @reserved:		reserved
+ * @magic:		Magic number
+ * @reserved:		reserved
+ * @reserved:		reserved (will be used for PE COFF offset)
+ */
+
+struct riscv_image_header {
+	u32 code0;
+	u32 code1;
+	u64 text_offset;
+	u64 image_size;
+	u64 res1;
+	u64 res2;
+	u64 res3;
+	u64 magic;
+	u32 res4;
+	u32 res5;
+};
+#endif /* __ASSEMBLY__ */
+#endif /* __ASM_IMAGE_H */
diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
index 3261e9fce8..0edd35b20f 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -1,14 +1,40 @@ 
 #include <asm/asm.h>
 #include <asm/riscv_encoding.h>
+#include <asm/image.h>
 
         .section .text.header, "ax", %progbits
 
         /*
          * OpenSBI pass to start():
          *   a0 -> hart_id ( bootcpu_id )
-         *   a1 -> dtb_base 
+         *   a1 -> dtb_base
          */
 FUNC(start)
+#ifdef CONFIG_RISCV_EFI
+        j xen_start
+
+        /* -----------  Header -------------- */
+	.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 xen image */
+	.dword _end - _start
+	.dword __HEAD_FLAGS
+	.word RISCV_HEADER_VERSION
+	.word 0
+	.dword 0
+	.ascii RISCV_IMAGE_MAGIC
+	.balign 4
+	.ascii RISCV_IMAGE_MAGIC2
+
+FUNC(xen_start)
+#endif
         /* Mask all interrupts */
         csrw    CSR_SIE, zero
 
@@ -60,6 +86,11 @@  FUNC(start)
         mv      a1, s1
 
         tail    start_xen
+
+#ifdef CONFIG_RISCV_EFI
+END(xen_start)
+#endif
+
 END(start)
 
         .section .text, "ax", %progbits