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
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
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
--
2.25.1On 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
Added riscv image header to enable boot from second stage bootloaders (e.g. uboot. Image header defined in riscv-image-header.h)
Additionally, RISC-V xen image is extended with PE/COFF headers, introducing EFI application format.
PE/COFF 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.
Note that this patch represents initial EFI application format support (image
contains EFI application header embeded into binary when CONFIG_RISCV_EFI
is enabled). For full EFI application Xen support, boot/runtime services
and system table handling are yet to be implemented.
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>
Signed-off-by: Milan Djokic <milan.djokic@rt-rk.com>
---
Changes since v2:
* Restructured EFI image headers - generic PE/COFF headers moved to xen/include/efi/pe.h, riscv image header left in arch specific dir (riscv/include/asm/riscv-image-header.h)
* EFI PE/COFF header section moved to separate file (efi-header.S) and optionally included into image (head.S) if CONFIG_RISCV_EFI is enabled
* Removed explicit usage of compressed (c.*) instructions where not necessary to avoid dependency on C extension.
* Removed redundant parts of code which were originally taken over from linux kernel, but not used in this case
(data directories definition in optional PE header, _init_end_efi in linker script)
* Removed nested code, explained pe header fields mapping and current EFI support status
* Clarified riscv image header which is inserted into image regardless EFI option value (in order to support boot from second stage bootloaders for both ELF and EFI image format)
---
Changes since v1 (following review comments from Jan Beulich):
* Fix coding style
* Extended image header with all the necessary
PE/COFF (EFI) fields (instead of only those used by U-boot)
* Removed usage of deprecated types
---
xen/arch/riscv/Kconfig | 10 +
.../riscv/include/asm/riscv-image-header.h | 57 +++
xen/arch/riscv/riscv64/efi-header.S | 99 ++++
xen/arch/riscv/riscv64/head.S | 45 +-
xen/arch/riscv/xen.lds.S | 3 +
xen/include/efi/pe.h | 460 ++++++++++++++++++
6 files changed, 672 insertions(+), 2 deletions(-)
create mode 100644 xen/arch/riscv/include/asm/riscv-image-header.h
create mode 100644 xen/arch/riscv/riscv64/efi-header.S
create mode 100644 xen/include/efi/pe.h
diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index f382b36f6c..ec1e2b1386 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -9,6 +9,16 @@ config ARCH_DEFCONFIG
string
default "arch/riscv/configs/tiny64_defconfig"
+config RISCV_EFI
+ bool "UEFI boot service support"
+ depends on RISCV_64 && RISCV_ISA_C
+ 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. Currently, only EFI PE/COFF application
+ header is included into RISC-V image. Boot/Runtime services as part
+ of EFI application stub are yet to be implemented.
+
menu "Architecture Features"
source "arch/Kconfig"
diff --git a/xen/arch/riscv/include/asm/riscv-image-header.h b/xen/arch/riscv/include/asm/riscv-image-header.h
new file mode 100644
index 0000000000..eebca47620
--- /dev/null
+++ b/xen/arch/riscv/include/asm/riscv-image-header.h
@@ -0,0 +1,57 @@
+#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_LE 0
+#define RISCV_IMAGE_FLAG_BE 1
+
+#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_LE
+
+#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 image header
+ * @code0: Executable code
+ * @code1: Executable code
+ * @text_offset: Image load offset (little endian)
+ * @image_size: Effective Image size (little endian)
+ * @flags: kernel flags (little endian)
+ * @version: version
+ * @res1: reserved
+ * @res2: reserved
+ * @magic: Magic number (RISC-V specific; deprecated)
+ * @magic2: Magic number 2 (to match the ARM64 'magic' field pos)
+ * @res3: reserved (will be used for PE COFF offset)
+ *
+ * The intention is for this header format to be shared between multiple
+ * architectures to avoid a proliferation of image header formats.
+ */
+struct riscv_image_header {
+ uint32_t code0;
+ uint32_t code1;
+ uint64_t text_offset;
+ uint64_t image_size;
+ uint64_t flags;
+ uint64_t version;
+ uint32_t res1;
+ uint64_t res2;
+ uint64_t magic;
+ uint32_t magic2;
+ uint32_t res3;
+};
+#endif /* __ASSEMBLY__ */
+#endif /* _ASM_RISCV_IMAGE_H */
diff --git a/xen/arch/riscv/riscv64/efi-header.S b/xen/arch/riscv/riscv64/efi-header.S
new file mode 100644
index 0000000000..87cd591a96
--- /dev/null
+++ b/xen/arch/riscv/riscv64/efi-header.S
@@ -0,0 +1,99 @@
+#include <efi/pe.h>
+#include <xen/sizes.h>
+
+ .macro __EFI_PE_HEADER
+ .long PE_MAGIC
+coff_header:
+#ifdef CONFIG_RISCV_64
+ .short IMAGE_FILE_MACHINE_RISCV64 /* Machine */
+#else
+ .short IMAGE_FILE_MACHINE_RISCV32 /* Machine */
+#endif
+ .short section_count /* NumberOfSections */
+ .long 0 /* TimeDateStamp */
+ .long 0 /* PointerToSymbolTable */
+ .long 0 /* NumberOfSymbols */
+ .short section_table - optional_header /* SizeOfOptionalHeader */
+ .short IMAGE_FILE_DEBUG_STRIPPED | \
+ IMAGE_FILE_EXECUTABLE_IMAGE | \
+ IMAGE_FILE_LINE_NUMS_STRIPPED /* Characteristics */
+
+optional_header:
+#ifdef CONFIG_RISCV_64
+ .short PE_OPT_MAGIC_PE32PLUS /* PE32+ format */
+#else
+ .short PE_OPT_MAGIC_PE32 /* PE32 format */
+#endif
+ .byte 0x02 /* MajorLinkerVersion */
+ .byte 0x14 /* MinorLinkerVersion */
+ .long _end - xen_start /* SizeOfCode */
+ .long 0 /* SizeOfInitializedData */
+ .long 0 /* SizeOfUninitializedData */
+ .long 0 /* AddressOfEntryPoint */
+ .long xen_start - _start /* BaseOfCode */
+#ifdef CONFIG_RISCV_32
+ .long _end - _start /* BaseOfData */
+#endif
+
+extra_header_fields:
+ .quad 0 /* ImageBase */
+ .long PECOFF_SECTION_ALIGNMENT /* SectionAlignment */
+ .long PECOFF_FILE_ALIGNMENT /* FileAlignment */
+ .short 0 /* MajorOperatingSystemVersion */
+ .short 0 /* MinorOperatingSystemVersion */
+ .short LINUX_EFISTUB_MAJOR_VERSION /* MajorImageVersion */
+ .short LINUX_EFISTUB_MINOR_VERSION /* MinorImageVersion */
+ .short 0 /* MajorSubsystemVersion */
+ .short 0 /* MinorSubsystemVersion */
+ .long 0 /* Win32VersionValue */
+ .long _end - _start /* SizeOfImage */
+
+ /* Everything before the xen image is considered part of the header */
+ .long xen_start - _start /* SizeOfHeaders */
+ .long 0 /* CheckSum */
+ .short IMAGE_SUBSYSTEM_EFI_APPLICATION /* Subsystem */
+ .short 0 /* DllCharacteristics */
+ .quad 0 /* SizeOfStackReserve */
+ .quad 0 /* SizeOfStackCommit */
+ .quad 0 /* SizeOfHeapReserve */
+ .quad 0 /* SizeOfHeapCommit */
+ .long 0 /* LoaderFlags */
+ .long (section_table - .) / 8 /* NumberOfRvaAndSizes */
+ /*
+ * Data directories are not used in this case, therefore not defined to reduce header size.
+ */
+
+ /* Section table */
+section_table:
+ /* Currently code/data sections are not used since EFI stub implementation is not yet finalized */
+ .ascii ".text\0\0\0"
+ .long 0 /* VirtualSize */
+ .long 0 /* VirtualAddress */
+ .long 0 /* SizeOfRawData */
+ .long 0 /* PointerToRawData */
+ .long 0 /* PointerToRelocations */
+ .long 0 /* PointerToLineNumbers */
+ .short 0 /* NumberOfRelocations */
+ .short 0 /* NumberOfLineNumbers */
+ .long IMAGE_SCN_CNT_CODE | \
+ IMAGE_SCN_MEM_READ | \
+ IMAGE_SCN_MEM_EXECUTE /* Characteristics */
+
+ .ascii ".data\0\0\0"
+ .long 0 /* VirtualSize */
+ .long 0 /* VirtualAddress */
+ .long 0 /* SizeOfRawData */
+ .long 0 /* PointerToRawData */
+ .long 0 /* PointerToRelocations */
+ .long 0 /* PointerToLineNumbers */
+ .short 0 /* NumberOfRelocations */
+ .short 0 /* NumberOfLineNumbers */
+ .long IMAGE_SCN_CNT_INITIALIZED_DATA | \
+ IMAGE_SCN_MEM_READ | \
+ IMAGE_SCN_MEM_WRITE /* Characteristics */
+
+ .set section_count, (. - section_table) / 40
+
+ .balign 0x1000
+efi_header_end:
+ .endm
\ No newline at end of file
diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
index 3261e9fce8..5669e5df20 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -1,14 +1,54 @@
#include <asm/asm.h>
#include <asm/riscv_encoding.h>
+#include <asm/riscv-image-header.h>
+#ifdef CONFIG_RISCV_EFI
+#include "efi-header.S"
+#endif
.section .text.header, "ax", %progbits
-
/*
* OpenSBI pass to start():
* a0 -> hart_id ( bootcpu_id )
- * a1 -> dtb_base
+ * a1 -> dtb_base
*/
FUNC(start)
+/* Image header expected by second stage bootloaders (format defined in asm/riscv-image-header.h) */
+#ifdef CONFIG_RISCV_EFI
+ /*
+ * This instruction decodes to "MZ" ASCII required by UEFI.
+ */
+ c.li s4,-13
+ c.j xen_start
+#else
+ /* jump to start kernel */
+ jal xen_start
+#endif
+ .balign 8
+#ifdef CONFIG_RISCV_64
+ /* Image load offset(2MB) from start of RAM */
+ .quad 0x200000
+#else
+ /* Image load offset(4MB) from start of RAM */
+ .quad 0x400000
+#endif
+ .quad _end - _start /* Effective Image size */
+ .quad __HEAD_FLAGS
+ .long RISCV_HEADER_VERSION
+ .long 0 /* reserved */
+ .quad 0 /* reserved */
+ .ascii RISCV_IMAGE_MAGIC /* Magic number (RISC-V specific; deprecated) */
+ .balign 4
+ .ascii RISCV_IMAGE_MAGIC2 /* Magic number 2 (to match the ARM64 'magic' field pos) */
+#ifdef CONFIG_RISCV_EFI
+ .long pe_head_start - _start /* reserved (PE COFF offset) */
+pe_head_start:
+
+ __EFI_PE_HEADER
+#else
+ .long 0 /* 0 means no PE header. */
+#endif
+
+xen_start:
/* Mask all interrupts */
csrw CSR_SIE, zero
@@ -60,6 +100,7 @@ FUNC(start)
mv a1, s1
tail start_xen
+
END(start)
.section .text, "ax", %progbits
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index 8510a87c4d..c36d76baf3 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -12,6 +12,9 @@ PHDRS
#endif
}
+PECOFF_SECTION_ALIGNMENT = 0x1000;
+PECOFF_FILE_ALIGNMENT = 0x200;
+
SECTIONS
{
. = XEN_VIRT_START;
diff --git a/xen/include/efi/pe.h b/xen/include/efi/pe.h
new file mode 100644
index 0000000000..71fd7e3126
--- /dev/null
+++ b/xen/include/efi/pe.h
@@ -0,0 +1,460 @@
+#ifndef __PE_H
+#define __PE_H
+
+/*
+ * Linux EFI stub v1.0 adds the following functionality:
+ * - Loading initrd from the LINUX_EFI_INITRD_MEDIA_GUID device path,
+ * - Loading/starting the kernel from firmware that targets a different
+ * machine type, via the entrypoint exposed in the .compat PE/COFF section.
+ *
+ * The recommended way of loading and starting v1.0 or later kernels is to use
+ * the LoadImage() and StartImage() EFI boot services, and expose the initrd
+ * via the LINUX_EFI_INITRD_MEDIA_GUID device path.
+ *
+ * Versions older than v1.0 support initrd loading via the image load options
+ * (using initrd=, limited to the volume from which the kernel itself was
+ * loaded), or via arch specific means (bootparams, DT, etc).
+ *
+ * On x86, LoadImage() and StartImage() can be omitted if the EFI handover
+ * protocol is implemented, which can be inferred from the version,
+ * handover_offset and xloadflags fields in the bootparams structure.
+ */
+#define LINUX_EFISTUB_MAJOR_VERSION 0x1
+#define LINUX_EFISTUB_MINOR_VERSION 0x0
+
+#define MZ_MAGIC 0x5a4d /* "MZ" */
+
+#define PE_MAGIC 0x00004550 /* "PE\0\0" */
+#define PE_OPT_MAGIC_PE32 0x010b
+#define PE_OPT_MAGIC_PE32_ROM 0x0107
+#define PE_OPT_MAGIC_PE32PLUS 0x020b
+
+/* machine type */
+#define IMAGE_FILE_MACHINE_UNKNOWN 0x0000
+#define IMAGE_FILE_MACHINE_AM33 0x01d3
+#define IMAGE_FILE_MACHINE_AMD64 0x8664
+#define IMAGE_FILE_MACHINE_ARM 0x01c0
+#define IMAGE_FILE_MACHINE_ARMV7 0x01c4
+#define IMAGE_FILE_MACHINE_ARM64 0xaa64
+#define IMAGE_FILE_MACHINE_EBC 0x0ebc
+#define IMAGE_FILE_MACHINE_I386 0x014c
+#define IMAGE_FILE_MACHINE_IA64 0x0200
+#define IMAGE_FILE_MACHINE_M32R 0x9041
+#define IMAGE_FILE_MACHINE_MIPS16 0x0266
+#define IMAGE_FILE_MACHINE_MIPSFPU 0x0366
+#define IMAGE_FILE_MACHINE_MIPSFPU16 0x0466
+#define IMAGE_FILE_MACHINE_POWERPC 0x01f0
+#define IMAGE_FILE_MACHINE_POWERPCFP 0x01f1
+#define IMAGE_FILE_MACHINE_R4000 0x0166
+#define IMAGE_FILE_MACHINE_RISCV32 0x5032
+#define IMAGE_FILE_MACHINE_RISCV64 0x5064
+#define IMAGE_FILE_MACHINE_RISCV128 0x5128
+#define IMAGE_FILE_MACHINE_SH3 0x01a2
+#define IMAGE_FILE_MACHINE_SH3DSP 0x01a3
+#define IMAGE_FILE_MACHINE_SH3E 0x01a4
+#define IMAGE_FILE_MACHINE_SH4 0x01a6
+#define IMAGE_FILE_MACHINE_SH5 0x01a8
+#define IMAGE_FILE_MACHINE_THUMB 0x01c2
+#define IMAGE_FILE_MACHINE_WCEMIPSV2 0x0169
+
+/* flags */
+#define IMAGE_FILE_RELOCS_STRIPPED 0x0001
+#define IMAGE_FILE_EXECUTABLE_IMAGE 0x0002
+#define IMAGE_FILE_LINE_NUMS_STRIPPED 0x0004
+#define IMAGE_FILE_LOCAL_SYMS_STRIPPED 0x0008
+#define IMAGE_FILE_AGGRESSIVE_WS_TRIM 0x0010
+#define IMAGE_FILE_LARGE_ADDRESS_AWARE 0x0020
+#define IMAGE_FILE_16BIT_MACHINE 0x0040
+#define IMAGE_FILE_BYTES_REVERSED_LO 0x0080
+#define IMAGE_FILE_32BIT_MACHINE 0x0100
+#define IMAGE_FILE_DEBUG_STRIPPED 0x0200
+#define IMAGE_FILE_REMOVABLE_RUN_FROM_SWAP 0x0400
+#define IMAGE_FILE_NET_RUN_FROM_SWAP 0x0800
+#define IMAGE_FILE_SYSTEM 0x1000
+#define IMAGE_FILE_DLL 0x2000
+#define IMAGE_FILE_UP_SYSTEM_ONLY 0x4000
+#define IMAGE_FILE_BYTES_REVERSED_HI 0x8000
+
+#define IMAGE_FILE_OPT_ROM_MAGIC 0x107
+#define IMAGE_FILE_OPT_PE32_MAGIC 0x10b
+#define IMAGE_FILE_OPT_PE32_PLUS_MAGIC 0x20b
+
+#define IMAGE_SUBSYSTEM_UNKNOWN 0
+#define IMAGE_SUBSYSTEM_NATIVE 1
+#define IMAGE_SUBSYSTEM_WINDOWS_GUI 2
+#define IMAGE_SUBSYSTEM_WINDOWS_CUI 3
+#define IMAGE_SUBSYSTEM_POSIX_CUI 7
+#define IMAGE_SUBSYSTEM_WINDOWS_CE_GUI 9
+#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10
+#define IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER 11
+#define IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER 12
+#define IMAGE_SUBSYSTEM_EFI_ROM_IMAGE 13
+#define IMAGE_SUBSYSTEM_XBOX 14
+
+#define IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE 0x0040
+#define IMAGE_DLL_CHARACTERISTICS_FORCE_INTEGRITY 0x0080
+#define IMAGE_DLL_CHARACTERISTICS_NX_COMPAT 0x0100
+#define IMAGE_DLLCHARACTERISTICS_NO_ISOLATION 0x0200
+#define IMAGE_DLLCHARACTERISTICS_NO_SEH 0x0400
+#define IMAGE_DLLCHARACTERISTICS_NO_BIND 0x0800
+#define IMAGE_DLLCHARACTERISTICS_WDM_DRIVER 0x2000
+#define IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE 0x8000
+
+/* they actually defined 0x00000000 as well, but I think we'll skip that one. */
+#define IMAGE_SCN_RESERVED_0 0x00000001
+#define IMAGE_SCN_RESERVED_1 0x00000002
+#define IMAGE_SCN_RESERVED_2 0x00000004
+#define IMAGE_SCN_TYPE_NO_PAD 0x00000008 /* don't pad - obsolete */
+#define IMAGE_SCN_RESERVED_3 0x00000010
+#define IMAGE_SCN_CNT_CODE 0x00000020 /* .text */
+#define IMAGE_SCN_CNT_INITIALIZED_DATA 0x00000040 /* .data */
+#define IMAGE_SCN_CNT_UNINITIALIZED_DATA 0x00000080 /* .bss */
+#define IMAGE_SCN_LNK_OTHER 0x00000100 /* reserved */
+#define IMAGE_SCN_LNK_INFO 0x00000200 /* .drectve comments */
+#define IMAGE_SCN_RESERVED_4 0x00000400
+#define IMAGE_SCN_LNK_REMOVE 0x00000800 /* .o only - scn to be rm'd*/
+#define IMAGE_SCN_LNK_COMDAT 0x00001000 /* .o only - COMDAT data */
+#define IMAGE_SCN_RESERVED_5 0x00002000 /* spec omits this */
+#define IMAGE_SCN_RESERVED_6 0x00004000 /* spec omits this */
+#define IMAGE_SCN_GPREL 0x00008000 /* global pointer referenced data */
+/* spec lists 0x20000 twice, I suspect they meant 0x10000 for one of them */
+#define IMAGE_SCN_MEM_PURGEABLE 0x00010000 /* reserved for "future" use */
+#define IMAGE_SCN_16BIT 0x00020000 /* reserved for "future" use */
+#define IMAGE_SCN_LOCKED 0x00040000 /* reserved for "future" use */
+#define IMAGE_SCN_PRELOAD 0x00080000 /* reserved for "future" use */
+/* and here they just stuck a 1-byte integer in the middle of a bitfield */
+#define IMAGE_SCN_ALIGN_1BYTES 0x00100000 /* it does what it says on the box */
+#define IMAGE_SCN_ALIGN_2BYTES 0x00200000
+#define IMAGE_SCN_ALIGN_4BYTES 0x00300000
+#define IMAGE_SCN_ALIGN_8BYTES 0x00400000
+#define IMAGE_SCN_ALIGN_16BYTES 0x00500000
+#define IMAGE_SCN_ALIGN_32BYTES 0x00600000
+#define IMAGE_SCN_ALIGN_64BYTES 0x00700000
+#define IMAGE_SCN_ALIGN_128BYTES 0x00800000
+#define IMAGE_SCN_ALIGN_256BYTES 0x00900000
+#define IMAGE_SCN_ALIGN_512BYTES 0x00a00000
+#define IMAGE_SCN_ALIGN_1024BYTES 0x00b00000
+#define IMAGE_SCN_ALIGN_2048BYTES 0x00c00000
+#define IMAGE_SCN_ALIGN_4096BYTES 0x00d00000
+#define IMAGE_SCN_ALIGN_8192BYTES 0x00e00000
+#define IMAGE_SCN_LNK_NRELOC_OVFL 0x01000000 /* extended relocations */
+#define IMAGE_SCN_MEM_DISCARDABLE 0x02000000 /* scn can be discarded */
+#define IMAGE_SCN_MEM_NOT_CACHED 0x04000000 /* cannot be cached */
+#define IMAGE_SCN_MEM_NOT_PAGED 0x08000000 /* not pageable */
+#define IMAGE_SCN_MEM_SHARED 0x10000000 /* can be shared */
+#define IMAGE_SCN_MEM_EXECUTE 0x20000000 /* can be executed as code */
+#define IMAGE_SCN_MEM_READ 0x40000000 /* readable */
+#define IMAGE_SCN_MEM_WRITE 0x80000000 /* writeable */
+
+#define IMAGE_DEBUG_TYPE_CODEVIEW 2
+
+#ifndef __ASSEMBLY__
+
+struct mz_hdr {
+ uint16_t magic; /* MZ_MAGIC */
+ uint16_t lbsize; /* size of last used block */
+ uint16_t blocks; /* pages in file, 0x3 */
+ uint16_t relocs; /* relocations */
+ uint16_t hdrsize; /* header size in "paragraphs" */
+ uint16_t min_extra_pps; /* .bss */
+ uint16_t max_extra_pps; /* runtime limit for the arena size */
+ uint16_t ss; /* relative stack segment */
+ uint16_t sp; /* initial %sp register */
+ uint16_t checksum; /* word checksum */
+ uint16_t ip; /* initial %ip register */
+ uint16_t cs; /* initial %cs relative to load segment */
+ uint16_t reloc_table_offset; /* offset of the first relocation */
+ uint16_t overlay_num; /* overlay number. set to 0. */
+ uint16_t reserved0[4]; /* reserved */
+ uint16_t oem_id; /* oem identifier */
+ uint16_t oem_info; /* oem specific */
+ uint16_t reserved1[10]; /* reserved */
+ uint32_t peaddr; /* address of pe header */
+ char message[]; /* message to print */
+};
+
+struct mz_reloc {
+ uint16_t offset;
+ uint16_t segment;
+};
+
+struct pe_hdr {
+ uint32_t magic; /* PE magic */
+ uint16_t machine; /* machine type */
+ uint16_t sections; /* number of sections */
+ uint32_t timestamp; /* time_t */
+ uint32_t symbol_table; /* symbol table offset */
+ uint32_t symbols; /* number of symbols */
+ uint16_t opt_hdr_size; /* size of optional header */
+ uint16_t flags; /* flags */
+};
+
+/* the fact that pe32 isn't padded where pe32+ is 64-bit means union won't
+ * work right. vomit. */
+struct pe32_opt_hdr {
+ /* "standard" header */
+ uint16_t magic; /* file type */
+ uint8_t ld_major; /* linker major version */
+ uint8_t ld_minor; /* linker minor version */
+ uint32_t text_size; /* size of text section(s) */
+ uint32_t data_size; /* size of data section(s) */
+ uint32_t bss_size; /* size of bss section(s) */
+ uint32_t entry_point; /* file offset of entry point */
+ uint32_t code_base; /* relative code addr in ram */
+ uint32_t data_base; /* relative data addr in ram */
+ /* "windows" header */
+ uint32_t image_base; /* preferred load address */
+ uint32_t section_align; /* alignment in bytes */
+ uint32_t file_align; /* file alignment in bytes */
+ uint16_t os_major; /* major OS version */
+ uint16_t os_minor; /* minor OS version */
+ uint16_t image_major; /* major image version */
+ uint16_t image_minor; /* minor image version */
+ uint16_t subsys_major; /* major subsystem version */
+ uint16_t subsys_minor; /* minor subsystem version */
+ uint32_t win32_version; /* reserved, must be 0 */
+ uint32_t image_size; /* image size */
+ uint32_t header_size; /* header size rounded up to
+ file_align */
+ uint32_t csum; /* checksum */
+ uint16_t subsys; /* subsystem */
+ uint16_t dll_flags; /* more flags! */
+ uint32_t stack_size_req; /* amt of stack requested */
+ uint32_t stack_size; /* amt of stack required */
+ uint32_t heap_size_req; /* amt of heap requested */
+ uint32_t heap_size; /* amt of heap required */
+ uint32_t loader_flags; /* reserved, must be 0 */
+ uint32_t data_dirs; /* number of data dir entries */
+};
+
+struct pe32plus_opt_hdr {
+ uint16_t magic; /* file type */
+ uint8_t ld_major; /* linker major version */
+ uint8_t ld_minor; /* linker minor version */
+ uint32_t text_size; /* size of text section(s) */
+ uint32_t data_size; /* size of data section(s) */
+ uint32_t bss_size; /* size of bss section(s) */
+ uint32_t entry_point; /* file offset of entry point */
+ uint32_t code_base; /* relative code addr in ram */
+ /* "windows" header */
+ uint64_t image_base; /* preferred load address */
+ uint32_t section_align; /* alignment in bytes */
+ uint32_t file_align; /* file alignment in bytes */
+ uint16_t os_major; /* major OS version */
+ uint16_t os_minor; /* minor OS version */
+ uint16_t image_major; /* major image version */
+ uint16_t image_minor; /* minor image version */
+ uint16_t subsys_major; /* major subsystem version */
+ uint16_t subsys_minor; /* minor subsystem version */
+ uint32_t win32_version; /* reserved, must be 0 */
+ uint32_t image_size; /* image size */
+ uint32_t header_size; /* header size rounded up to
+ file_align */
+ uint32_t csum; /* checksum */
+ uint16_t subsys; /* subsystem */
+ uint16_t dll_flags; /* more flags! */
+ uint64_t stack_size_req; /* amt of stack requested */
+ uint64_t stack_size; /* amt of stack required */
+ uint64_t heap_size_req; /* amt of heap requested */
+ uint64_t heap_size; /* amt of heap required */
+ uint32_t loader_flags; /* reserved, must be 0 */
+ uint32_t data_dirs; /* number of data dir entries */
+};
+
+struct data_dirent {
+ uint32_t virtual_address; /* relative to load address */
+ uint32_t size;
+};
+
+struct data_directory {
+ struct data_dirent exports; /* .edata */
+ struct data_dirent imports; /* .idata */
+ struct data_dirent resources; /* .rsrc */
+ struct data_dirent exceptions; /* .pdata */
+ struct data_dirent certs; /* certs */
+ struct data_dirent base_relocations; /* .reloc */
+ struct data_dirent debug; /* .debug */
+ struct data_dirent arch; /* reservered */
+ struct data_dirent global_ptr; /* global pointer reg. Size=0 */
+ struct data_dirent tls; /* .tls */
+ struct data_dirent load_config; /* load configuration structure */
+ struct data_dirent bound_imports; /* no idea */
+ struct data_dirent import_addrs; /* import address table */
+ struct data_dirent delay_imports; /* delay-load import table */
+ struct data_dirent clr_runtime_hdr; /* .cor (object only) */
+ struct data_dirent reserved;
+};
+
+struct section_header {
+ char name[8]; /* name or "/12\0" string tbl offset */
+ uint32_t virtual_size; /* size of loaded section in ram */
+ uint32_t virtual_address; /* relative virtual address */
+ uint32_t raw_data_size; /* size of the section */
+ uint32_t data_addr; /* file pointer to first page of sec */
+ uint32_t relocs; /* file pointer to relocation entries */
+ uint32_t line_numbers; /* line numbers! */
+ uint16_t num_relocs; /* number of relocations */
+ uint16_t num_lin_numbers; /* srsly. */
+ uint32_t flags;
+};
+
+enum x64_coff_reloc_type {
+ IMAGE_REL_AMD64_ABSOLUTE = 0,
+ IMAGE_REL_AMD64_ADDR64,
+ IMAGE_REL_AMD64_ADDR32,
+ IMAGE_REL_AMD64_ADDR32N,
+ IMAGE_REL_AMD64_REL32,
+ IMAGE_REL_AMD64_REL32_1,
+ IMAGE_REL_AMD64_REL32_2,
+ IMAGE_REL_AMD64_REL32_3,
+ IMAGE_REL_AMD64_REL32_4,
+ IMAGE_REL_AMD64_REL32_5,
+ IMAGE_REL_AMD64_SECTION,
+ IMAGE_REL_AMD64_SECREL,
+ IMAGE_REL_AMD64_SECREL7,
+ IMAGE_REL_AMD64_TOKEN,
+ IMAGE_REL_AMD64_SREL32,
+ IMAGE_REL_AMD64_PAIR,
+ IMAGE_REL_AMD64_SSPAN32,
+};
+
+enum arm_coff_reloc_type {
+ IMAGE_REL_ARM_ABSOLUTE,
+ IMAGE_REL_ARM_ADDR32,
+ IMAGE_REL_ARM_ADDR32N,
+ IMAGE_REL_ARM_BRANCH2,
+ IMAGE_REL_ARM_BRANCH1,
+ IMAGE_REL_ARM_SECTION,
+ IMAGE_REL_ARM_SECREL,
+};
+
+enum sh_coff_reloc_type {
+ IMAGE_REL_SH3_ABSOLUTE,
+ IMAGE_REL_SH3_DIRECT16,
+ IMAGE_REL_SH3_DIRECT32,
+ IMAGE_REL_SH3_DIRECT8,
+ IMAGE_REL_SH3_DIRECT8_WORD,
+ IMAGE_REL_SH3_DIRECT8_LONG,
+ IMAGE_REL_SH3_DIRECT4,
+ IMAGE_REL_SH3_DIRECT4_WORD,
+ IMAGE_REL_SH3_DIRECT4_LONG,
+ IMAGE_REL_SH3_PCREL8_WORD,
+ IMAGE_REL_SH3_PCREL8_LONG,
+ IMAGE_REL_SH3_PCREL12_WORD,
+ IMAGE_REL_SH3_STARTOF_SECTION,
+ IMAGE_REL_SH3_SIZEOF_SECTION,
+ IMAGE_REL_SH3_SECTION,
+ IMAGE_REL_SH3_SECREL,
+ IMAGE_REL_SH3_DIRECT32_NB,
+ IMAGE_REL_SH3_GPREL4_LONG,
+ IMAGE_REL_SH3_TOKEN,
+ IMAGE_REL_SHM_PCRELPT,
+ IMAGE_REL_SHM_REFLO,
+ IMAGE_REL_SHM_REFHALF,
+ IMAGE_REL_SHM_RELLO,
+ IMAGE_REL_SHM_RELHALF,
+ IMAGE_REL_SHM_PAIR,
+ IMAGE_REL_SHM_NOMODE,
+};
+
+enum ppc_coff_reloc_type {
+ IMAGE_REL_PPC_ABSOLUTE,
+ IMAGE_REL_PPC_ADDR64,
+ IMAGE_REL_PPC_ADDR32,
+ IMAGE_REL_PPC_ADDR24,
+ IMAGE_REL_PPC_ADDR16,
+ IMAGE_REL_PPC_ADDR14,
+ IMAGE_REL_PPC_REL24,
+ IMAGE_REL_PPC_REL14,
+ IMAGE_REL_PPC_ADDR32N,
+ IMAGE_REL_PPC_SECREL,
+ IMAGE_REL_PPC_SECTION,
+ IMAGE_REL_PPC_SECREL16,
+ IMAGE_REL_PPC_REFHI,
+ IMAGE_REL_PPC_REFLO,
+ IMAGE_REL_PPC_PAIR,
+ IMAGE_REL_PPC_SECRELLO,
+ IMAGE_REL_PPC_GPREL,
+ IMAGE_REL_PPC_TOKEN,
+};
+
+enum x86_coff_reloc_type {
+ IMAGE_REL_I386_ABSOLUTE,
+ IMAGE_REL_I386_DIR16,
+ IMAGE_REL_I386_REL16,
+ IMAGE_REL_I386_DIR32,
+ IMAGE_REL_I386_DIR32NB,
+ IMAGE_REL_I386_SEG12,
+ IMAGE_REL_I386_SECTION,
+ IMAGE_REL_I386_SECREL,
+ IMAGE_REL_I386_TOKEN,
+ IMAGE_REL_I386_SECREL7,
+ IMAGE_REL_I386_REL32,
+};
+
+enum ia64_coff_reloc_type {
+ IMAGE_REL_IA64_ABSOLUTE,
+ IMAGE_REL_IA64_IMM14,
+ IMAGE_REL_IA64_IMM22,
+ IMAGE_REL_IA64_IMM64,
+ IMAGE_REL_IA64_DIR32,
+ IMAGE_REL_IA64_DIR64,
+ IMAGE_REL_IA64_PCREL21B,
+ IMAGE_REL_IA64_PCREL21M,
+ IMAGE_REL_IA64_PCREL21F,
+ IMAGE_REL_IA64_GPREL22,
+ IMAGE_REL_IA64_LTOFF22,
+ IMAGE_REL_IA64_SECTION,
+ IMAGE_REL_IA64_SECREL22,
+ IMAGE_REL_IA64_SECREL64I,
+ IMAGE_REL_IA64_SECREL32,
+ IMAGE_REL_IA64_DIR32NB,
+ IMAGE_REL_IA64_SREL14,
+ IMAGE_REL_IA64_SREL22,
+ IMAGE_REL_IA64_SREL32,
+ IMAGE_REL_IA64_UREL32,
+ IMAGE_REL_IA64_PCREL60X,
+ IMAGE_REL_IA64_PCREL60B,
+ IMAGE_REL_IA64_PCREL60F,
+ IMAGE_REL_IA64_PCREL60I,
+ IMAGE_REL_IA64_PCREL60M,
+ IMAGE_REL_IA64_IMMGPREL6,
+ IMAGE_REL_IA64_TOKEN,
+ IMAGE_REL_IA64_GPREL32,
+ IMAGE_REL_IA64_ADDEND,
+};
+
+struct coff_reloc {
+ uint32_t virtual_address;
+ uint32_t symbol_table_index;
+
+ union {
+ enum x64_coff_reloc_type x64_type;
+ enum arm_coff_reloc_type arm_type;
+ enum sh_coff_reloc_type sh_type;
+ enum ppc_coff_reloc_type ppc_type;
+ enum x86_coff_reloc_type x86_type;
+ enum ia64_coff_reloc_type ia64_type;
+ uint16_t data;
+ };
+};
+
+/*
+ * Definitions for the contents of the certs data block
+ */
+#define WIN_CERT_TYPE_PKCS_SIGNED_DATA 0x0002
+#define WIN_CERT_TYPE_EFI_OKCS115 0x0EF0
+#define WIN_CERT_TYPE_EFI_GUID 0x0EF1
+
+#define WIN_CERT_REVISION_1_0 0x0100
+#define WIN_CERT_REVISION_2_0 0x0200
+
+struct win_certificate {
+ uint32_t length;
+ uint16_t revision;
+ uint16_t cert_type;
+};
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __PE_H */
--
2.25.1Added riscv image header to enable boot from second stage bootloaders (e.g. uboot. Image header defined in riscv-image-header.h)
Additionally, RISC-V xen image is extended with PE/COFF headers, introducing EFI application format.
PE/COFF 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.
Note that this patch represents initial EFI application format support (image
contains EFI application header embeded into binary when CONFIG_RISCV_EFI
is enabled). For full EFI application Xen support, boot/runtime services
and system table handling are yet to be implemented.
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>
Signed-off-by: Milan Djokic <milan.djokic@rt-rk.com>
---
Changes since v3:
* pe.h structures clarification
---
Changes since v2:
* Restructured EFI image headers - generic PE/COFF headers moved to xen/include/efi/pe.h, riscv image header left in arch specific dir (riscv/include/asm/riscv-image-header.h)
* EFI PE/COFF header section moved to separate file (efi-header.S) and optionally included into image (head.S) if CONFIG_RISCV_EFI is enabled
* Removed explicit usage of compressed (c.*) instructions where not necessary to avoid dependency on C extension.
* Removed redundant parts of code which were originally taken over from linux kernel, but not used in this case
(data directories definition in optional PE header, _init_end_efi in linker script)
* Removed nested code, explained pe header fields mapping and current EFI support status
* Clarified riscv image header which is inserted into image regardless EFI option value (in order to support boot from second stage bootloaders for both ELF and EFI image format)
---
Changes since v1 (following review comments from Jan Beulich):
* Fix coding style
* Extended image header with all the necessary
PE/COFF (EFI) fields (instead of only those used by U-boot)
* Removed usage of deprecated types
---
xen/arch/riscv/Kconfig | 10 +
.../riscv/include/asm/riscv-image-header.h | 57 +++
xen/arch/riscv/riscv64/efi-header.S | 99 ++++
xen/arch/riscv/riscv64/head.S | 45 +-
xen/arch/riscv/xen.lds.S | 3 +
xen/include/efi/pe.h | 458 ++++++++++++++++++
6 files changed, 670 insertions(+), 2 deletions(-)
create mode 100644 xen/arch/riscv/include/asm/riscv-image-header.h
create mode 100644 xen/arch/riscv/riscv64/efi-header.S
create mode 100644 xen/include/efi/pe.h
diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index f382b36f6c..ec1e2b1386 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -9,6 +9,16 @@ config ARCH_DEFCONFIG
string
default "arch/riscv/configs/tiny64_defconfig"
+config RISCV_EFI
+ bool "UEFI boot service support"
+ depends on RISCV_64 && RISCV_ISA_C
+ 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. Currently, only EFI PE/COFF application
+ header is included into RISC-V image. Boot/Runtime services as part
+ of EFI application stub are yet to be implemented.
+
menu "Architecture Features"
source "arch/Kconfig"
diff --git a/xen/arch/riscv/include/asm/riscv-image-header.h b/xen/arch/riscv/include/asm/riscv-image-header.h
new file mode 100644
index 0000000000..eebca47620
--- /dev/null
+++ b/xen/arch/riscv/include/asm/riscv-image-header.h
@@ -0,0 +1,57 @@
+#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_LE 0
+#define RISCV_IMAGE_FLAG_BE 1
+
+#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_LE
+
+#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 image header
+ * @code0: Executable code
+ * @code1: Executable code
+ * @text_offset: Image load offset (little endian)
+ * @image_size: Effective Image size (little endian)
+ * @flags: kernel flags (little endian)
+ * @version: version
+ * @res1: reserved
+ * @res2: reserved
+ * @magic: Magic number (RISC-V specific; deprecated)
+ * @magic2: Magic number 2 (to match the ARM64 'magic' field pos)
+ * @res3: reserved (will be used for PE COFF offset)
+ *
+ * The intention is for this header format to be shared between multiple
+ * architectures to avoid a proliferation of image header formats.
+ */
+struct riscv_image_header {
+ uint32_t code0;
+ uint32_t code1;
+ uint64_t text_offset;
+ uint64_t image_size;
+ uint64_t flags;
+ uint64_t version;
+ uint32_t res1;
+ uint64_t res2;
+ uint64_t magic;
+ uint32_t magic2;
+ uint32_t res3;
+};
+#endif /* __ASSEMBLY__ */
+#endif /* _ASM_RISCV_IMAGE_H */
diff --git a/xen/arch/riscv/riscv64/efi-header.S b/xen/arch/riscv/riscv64/efi-header.S
new file mode 100644
index 0000000000..87cd591a96
--- /dev/null
+++ b/xen/arch/riscv/riscv64/efi-header.S
@@ -0,0 +1,99 @@
+#include <efi/pe.h>
+#include <xen/sizes.h>
+
+ .macro __EFI_PE_HEADER
+ .long PE_MAGIC
+coff_header:
+#ifdef CONFIG_RISCV_64
+ .short IMAGE_FILE_MACHINE_RISCV64 /* Machine */
+#else
+ .short IMAGE_FILE_MACHINE_RISCV32 /* Machine */
+#endif
+ .short section_count /* NumberOfSections */
+ .long 0 /* TimeDateStamp */
+ .long 0 /* PointerToSymbolTable */
+ .long 0 /* NumberOfSymbols */
+ .short section_table - optional_header /* SizeOfOptionalHeader */
+ .short IMAGE_FILE_DEBUG_STRIPPED | \
+ IMAGE_FILE_EXECUTABLE_IMAGE | \
+ IMAGE_FILE_LINE_NUMS_STRIPPED /* Characteristics */
+
+optional_header:
+#ifdef CONFIG_RISCV_64
+ .short PE_OPT_MAGIC_PE32PLUS /* PE32+ format */
+#else
+ .short PE_OPT_MAGIC_PE32 /* PE32 format */
+#endif
+ .byte 0x02 /* MajorLinkerVersion */
+ .byte 0x14 /* MinorLinkerVersion */
+ .long _end - xen_start /* SizeOfCode */
+ .long 0 /* SizeOfInitializedData */
+ .long 0 /* SizeOfUninitializedData */
+ .long 0 /* AddressOfEntryPoint */
+ .long xen_start - _start /* BaseOfCode */
+#ifdef CONFIG_RISCV_32
+ .long _end - _start /* BaseOfData */
+#endif
+
+extra_header_fields:
+ .quad 0 /* ImageBase */
+ .long PECOFF_SECTION_ALIGNMENT /* SectionAlignment */
+ .long PECOFF_FILE_ALIGNMENT /* FileAlignment */
+ .short 0 /* MajorOperatingSystemVersion */
+ .short 0 /* MinorOperatingSystemVersion */
+ .short LINUX_EFISTUB_MAJOR_VERSION /* MajorImageVersion */
+ .short LINUX_EFISTUB_MINOR_VERSION /* MinorImageVersion */
+ .short 0 /* MajorSubsystemVersion */
+ .short 0 /* MinorSubsystemVersion */
+ .long 0 /* Win32VersionValue */
+ .long _end - _start /* SizeOfImage */
+
+ /* Everything before the xen image is considered part of the header */
+ .long xen_start - _start /* SizeOfHeaders */
+ .long 0 /* CheckSum */
+ .short IMAGE_SUBSYSTEM_EFI_APPLICATION /* Subsystem */
+ .short 0 /* DllCharacteristics */
+ .quad 0 /* SizeOfStackReserve */
+ .quad 0 /* SizeOfStackCommit */
+ .quad 0 /* SizeOfHeapReserve */
+ .quad 0 /* SizeOfHeapCommit */
+ .long 0 /* LoaderFlags */
+ .long (section_table - .) / 8 /* NumberOfRvaAndSizes */
+ /*
+ * Data directories are not used in this case, therefore not defined to reduce header size.
+ */
+
+ /* Section table */
+section_table:
+ /* Currently code/data sections are not used since EFI stub implementation is not yet finalized */
+ .ascii ".text\0\0\0"
+ .long 0 /* VirtualSize */
+ .long 0 /* VirtualAddress */
+ .long 0 /* SizeOfRawData */
+ .long 0 /* PointerToRawData */
+ .long 0 /* PointerToRelocations */
+ .long 0 /* PointerToLineNumbers */
+ .short 0 /* NumberOfRelocations */
+ .short 0 /* NumberOfLineNumbers */
+ .long IMAGE_SCN_CNT_CODE | \
+ IMAGE_SCN_MEM_READ | \
+ IMAGE_SCN_MEM_EXECUTE /* Characteristics */
+
+ .ascii ".data\0\0\0"
+ .long 0 /* VirtualSize */
+ .long 0 /* VirtualAddress */
+ .long 0 /* SizeOfRawData */
+ .long 0 /* PointerToRawData */
+ .long 0 /* PointerToRelocations */
+ .long 0 /* PointerToLineNumbers */
+ .short 0 /* NumberOfRelocations */
+ .short 0 /* NumberOfLineNumbers */
+ .long IMAGE_SCN_CNT_INITIALIZED_DATA | \
+ IMAGE_SCN_MEM_READ | \
+ IMAGE_SCN_MEM_WRITE /* Characteristics */
+
+ .set section_count, (. - section_table) / 40
+
+ .balign 0x1000
+efi_header_end:
+ .endm
\ No newline at end of file
diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
index 3261e9fce8..5669e5df20 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -1,14 +1,54 @@
#include <asm/asm.h>
#include <asm/riscv_encoding.h>
+#include <asm/riscv-image-header.h>
+#ifdef CONFIG_RISCV_EFI
+#include "efi-header.S"
+#endif
.section .text.header, "ax", %progbits
-
/*
* OpenSBI pass to start():
* a0 -> hart_id ( bootcpu_id )
- * a1 -> dtb_base
+ * a1 -> dtb_base
*/
FUNC(start)
+/* Image header expected by second stage bootloaders (format defined in asm/riscv-image-header.h) */
+#ifdef CONFIG_RISCV_EFI
+ /*
+ * This instruction decodes to "MZ" ASCII required by UEFI.
+ */
+ c.li s4,-13
+ c.j xen_start
+#else
+ /* jump to start kernel */
+ jal xen_start
+#endif
+ .balign 8
+#ifdef CONFIG_RISCV_64
+ /* Image load offset(2MB) from start of RAM */
+ .quad 0x200000
+#else
+ /* Image load offset(4MB) from start of RAM */
+ .quad 0x400000
+#endif
+ .quad _end - _start /* Effective Image size */
+ .quad __HEAD_FLAGS
+ .long RISCV_HEADER_VERSION
+ .long 0 /* reserved */
+ .quad 0 /* reserved */
+ .ascii RISCV_IMAGE_MAGIC /* Magic number (RISC-V specific; deprecated) */
+ .balign 4
+ .ascii RISCV_IMAGE_MAGIC2 /* Magic number 2 (to match the ARM64 'magic' field pos) */
+#ifdef CONFIG_RISCV_EFI
+ .long pe_head_start - _start /* reserved (PE COFF offset) */
+pe_head_start:
+
+ __EFI_PE_HEADER
+#else
+ .long 0 /* 0 means no PE header. */
+#endif
+
+xen_start:
/* Mask all interrupts */
csrw CSR_SIE, zero
@@ -60,6 +100,7 @@ FUNC(start)
mv a1, s1
tail start_xen
+
END(start)
.section .text, "ax", %progbits
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index 8510a87c4d..c36d76baf3 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -12,6 +12,9 @@ PHDRS
#endif
}
+PECOFF_SECTION_ALIGNMENT = 0x1000;
+PECOFF_FILE_ALIGNMENT = 0x200;
+
SECTIONS
{
. = XEN_VIRT_START;
diff --git a/xen/include/efi/pe.h b/xen/include/efi/pe.h
new file mode 100644
index 0000000000..bc42c3d85e
--- /dev/null
+++ b/xen/include/efi/pe.h
@@ -0,0 +1,458 @@
+#ifndef __PE_H
+#define __PE_H
+
+/*
+ * Linux EFI stub v1.0 adds the following functionality:
+ * - Loading initrd from the LINUX_EFI_INITRD_MEDIA_GUID device path,
+ * - Loading/starting the kernel from firmware that targets a different
+ * machine type, via the entrypoint exposed in the .compat PE/COFF section.
+ *
+ * The recommended way of loading and starting v1.0 or later kernels is to use
+ * the LoadImage() and StartImage() EFI boot services, and expose the initrd
+ * via the LINUX_EFI_INITRD_MEDIA_GUID device path.
+ *
+ * Versions older than v1.0 support initrd loading via the image load options
+ * (using initrd=, limited to the volume from which the kernel itself was
+ * loaded), or via arch specific means (bootparams, DT, etc).
+ *
+ * On x86, LoadImage() and StartImage() can be omitted if the EFI handover
+ * protocol is implemented, which can be inferred from the version,
+ * handover_offset and xloadflags fields in the bootparams structure.
+ */
+#define LINUX_EFISTUB_MAJOR_VERSION 0x1
+#define LINUX_EFISTUB_MINOR_VERSION 0x0
+
+#define MZ_MAGIC 0x5a4d /* "MZ" */
+
+#define PE_MAGIC 0x00004550 /* "PE\0\0" */
+#define PE_OPT_MAGIC_PE32 0x010b
+#define PE_OPT_MAGIC_PE32_ROM 0x0107
+#define PE_OPT_MAGIC_PE32PLUS 0x020b
+
+/* machine type */
+#define IMAGE_FILE_MACHINE_UNKNOWN 0x0000
+#define IMAGE_FILE_MACHINE_AM33 0x01d3
+#define IMAGE_FILE_MACHINE_AMD64 0x8664
+#define IMAGE_FILE_MACHINE_ARM 0x01c0
+#define IMAGE_FILE_MACHINE_ARMV7 0x01c4
+#define IMAGE_FILE_MACHINE_ARM64 0xaa64
+#define IMAGE_FILE_MACHINE_EBC 0x0ebc
+#define IMAGE_FILE_MACHINE_I386 0x014c
+#define IMAGE_FILE_MACHINE_IA64 0x0200
+#define IMAGE_FILE_MACHINE_M32R 0x9041
+#define IMAGE_FILE_MACHINE_MIPS16 0x0266
+#define IMAGE_FILE_MACHINE_MIPSFPU 0x0366
+#define IMAGE_FILE_MACHINE_MIPSFPU16 0x0466
+#define IMAGE_FILE_MACHINE_POWERPC 0x01f0
+#define IMAGE_FILE_MACHINE_POWERPCFP 0x01f1
+#define IMAGE_FILE_MACHINE_R4000 0x0166
+#define IMAGE_FILE_MACHINE_RISCV32 0x5032
+#define IMAGE_FILE_MACHINE_RISCV64 0x5064
+#define IMAGE_FILE_MACHINE_RISCV128 0x5128
+#define IMAGE_FILE_MACHINE_SH3 0x01a2
+#define IMAGE_FILE_MACHINE_SH3DSP 0x01a3
+#define IMAGE_FILE_MACHINE_SH3E 0x01a4
+#define IMAGE_FILE_MACHINE_SH4 0x01a6
+#define IMAGE_FILE_MACHINE_SH5 0x01a8
+#define IMAGE_FILE_MACHINE_THUMB 0x01c2
+#define IMAGE_FILE_MACHINE_WCEMIPSV2 0x0169
+
+/* flags */
+#define IMAGE_FILE_RELOCS_STRIPPED 0x0001
+#define IMAGE_FILE_EXECUTABLE_IMAGE 0x0002
+#define IMAGE_FILE_LINE_NUMS_STRIPPED 0x0004
+#define IMAGE_FILE_LOCAL_SYMS_STRIPPED 0x0008
+#define IMAGE_FILE_AGGRESSIVE_WS_TRIM 0x0010
+#define IMAGE_FILE_LARGE_ADDRESS_AWARE 0x0020
+#define IMAGE_FILE_16BIT_MACHINE 0x0040
+#define IMAGE_FILE_BYTES_REVERSED_LO 0x0080
+#define IMAGE_FILE_32BIT_MACHINE 0x0100
+#define IMAGE_FILE_DEBUG_STRIPPED 0x0200
+#define IMAGE_FILE_REMOVABLE_RUN_FROM_SWAP 0x0400
+#define IMAGE_FILE_NET_RUN_FROM_SWAP 0x0800
+#define IMAGE_FILE_SYSTEM 0x1000
+#define IMAGE_FILE_DLL 0x2000
+#define IMAGE_FILE_UP_SYSTEM_ONLY 0x4000
+#define IMAGE_FILE_BYTES_REVERSED_HI 0x8000
+
+#define IMAGE_FILE_OPT_ROM_MAGIC 0x107
+#define IMAGE_FILE_OPT_PE32_MAGIC 0x10b
+#define IMAGE_FILE_OPT_PE32_PLUS_MAGIC 0x20b
+
+#define IMAGE_SUBSYSTEM_UNKNOWN 0
+#define IMAGE_SUBSYSTEM_NATIVE 1
+#define IMAGE_SUBSYSTEM_WINDOWS_GUI 2
+#define IMAGE_SUBSYSTEM_WINDOWS_CUI 3
+#define IMAGE_SUBSYSTEM_POSIX_CUI 7
+#define IMAGE_SUBSYSTEM_WINDOWS_CE_GUI 9
+#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10
+#define IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER 11
+#define IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER 12
+#define IMAGE_SUBSYSTEM_EFI_ROM_IMAGE 13
+#define IMAGE_SUBSYSTEM_XBOX 14
+
+#define IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE 0x0040
+#define IMAGE_DLL_CHARACTERISTICS_FORCE_INTEGRITY 0x0080
+#define IMAGE_DLL_CHARACTERISTICS_NX_COMPAT 0x0100
+#define IMAGE_DLLCHARACTERISTICS_NO_ISOLATION 0x0200
+#define IMAGE_DLLCHARACTERISTICS_NO_SEH 0x0400
+#define IMAGE_DLLCHARACTERISTICS_NO_BIND 0x0800
+#define IMAGE_DLLCHARACTERISTICS_WDM_DRIVER 0x2000
+#define IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE 0x8000
+
+/* they actually defined 0x00000000 as well, but I think we'll skip that one. */
+#define IMAGE_SCN_RESERVED_0 0x00000001
+#define IMAGE_SCN_RESERVED_1 0x00000002
+#define IMAGE_SCN_RESERVED_2 0x00000004
+#define IMAGE_SCN_TYPE_NO_PAD 0x00000008 /* don't pad - obsolete */
+#define IMAGE_SCN_RESERVED_3 0x00000010
+#define IMAGE_SCN_CNT_CODE 0x00000020 /* .text */
+#define IMAGE_SCN_CNT_INITIALIZED_DATA 0x00000040 /* .data */
+#define IMAGE_SCN_CNT_UNINITIALIZED_DATA 0x00000080 /* .bss */
+#define IMAGE_SCN_LNK_OTHER 0x00000100 /* reserved */
+#define IMAGE_SCN_LNK_INFO 0x00000200 /* .drectve comments */
+#define IMAGE_SCN_RESERVED_4 0x00000400
+#define IMAGE_SCN_LNK_REMOVE 0x00000800 /* .o only - scn to be rm'd*/
+#define IMAGE_SCN_LNK_COMDAT 0x00001000 /* .o only - COMDAT data */
+#define IMAGE_SCN_RESERVED_5 0x00002000 /* spec omits this */
+#define IMAGE_SCN_RESERVED_6 0x00004000 /* spec omits this */
+#define IMAGE_SCN_GPREL 0x00008000 /* global pointer referenced data */
+/* spec lists 0x20000 twice, I suspect they meant 0x10000 for one of them */
+#define IMAGE_SCN_MEM_PURGEABLE 0x00010000 /* reserved for "future" use */
+#define IMAGE_SCN_16BIT 0x00020000 /* reserved for "future" use */
+#define IMAGE_SCN_LOCKED 0x00040000 /* reserved for "future" use */
+#define IMAGE_SCN_PRELOAD 0x00080000 /* reserved for "future" use */
+/* and here they just stuck a 1-byte integer in the middle of a bitfield */
+#define IMAGE_SCN_ALIGN_1BYTES 0x00100000 /* it does what it says on the box */
+#define IMAGE_SCN_ALIGN_2BYTES 0x00200000
+#define IMAGE_SCN_ALIGN_4BYTES 0x00300000
+#define IMAGE_SCN_ALIGN_8BYTES 0x00400000
+#define IMAGE_SCN_ALIGN_16BYTES 0x00500000
+#define IMAGE_SCN_ALIGN_32BYTES 0x00600000
+#define IMAGE_SCN_ALIGN_64BYTES 0x00700000
+#define IMAGE_SCN_ALIGN_128BYTES 0x00800000
+#define IMAGE_SCN_ALIGN_256BYTES 0x00900000
+#define IMAGE_SCN_ALIGN_512BYTES 0x00a00000
+#define IMAGE_SCN_ALIGN_1024BYTES 0x00b00000
+#define IMAGE_SCN_ALIGN_2048BYTES 0x00c00000
+#define IMAGE_SCN_ALIGN_4096BYTES 0x00d00000
+#define IMAGE_SCN_ALIGN_8192BYTES 0x00e00000
+#define IMAGE_SCN_LNK_NRELOC_OVFL 0x01000000 /* extended relocations */
+#define IMAGE_SCN_MEM_DISCARDABLE 0x02000000 /* scn can be discarded */
+#define IMAGE_SCN_MEM_NOT_CACHED 0x04000000 /* cannot be cached */
+#define IMAGE_SCN_MEM_NOT_PAGED 0x08000000 /* not pageable */
+#define IMAGE_SCN_MEM_SHARED 0x10000000 /* can be shared */
+#define IMAGE_SCN_MEM_EXECUTE 0x20000000 /* can be executed as code */
+#define IMAGE_SCN_MEM_READ 0x40000000 /* readable */
+#define IMAGE_SCN_MEM_WRITE 0x80000000 /* writeable */
+
+#define IMAGE_DEBUG_TYPE_CODEVIEW 2
+
+#ifndef __ASSEMBLY__
+
+struct mz_hdr {
+ uint16_t magic; /* MZ_MAGIC */
+ uint16_t lbsize; /* size of last used block */
+ uint16_t blocks; /* pages in file, 0x3 */
+ uint16_t relocs; /* relocations */
+ uint16_t hdrsize; /* header size in "paragraphs" */
+ uint16_t min_extra_pps; /* .bss */
+ uint16_t max_extra_pps; /* runtime limit for the arena size */
+ uint16_t ss; /* relative stack segment */
+ uint16_t sp; /* initial %sp register */
+ uint16_t checksum; /* word checksum */
+ uint16_t ip; /* initial %ip register */
+ uint16_t cs; /* initial %cs relative to load segment */
+ uint16_t reloc_table_offset; /* offset of the first relocation */
+ uint16_t overlay_num; /* overlay number. set to 0. */
+ uint16_t reserved0[4]; /* reserved */
+ uint16_t oem_id; /* oem identifier */
+ uint16_t oem_info; /* oem specific */
+ uint16_t reserved1[10]; /* reserved */
+ uint32_t peaddr; /* address of pe header */
+ char message[]; /* message to print */
+};
+
+struct mz_reloc {
+ uint16_t offset;
+ uint16_t segment;
+};
+
+struct pe_hdr {
+ uint32_t magic; /* PE magic */
+ uint16_t machine; /* machine type */
+ uint16_t sections; /* number of sections */
+ uint32_t timestamp; /* time_t */
+ uint32_t symbol_table; /* symbol table offset */
+ uint32_t symbols; /* number of symbols */
+ uint16_t opt_hdr_size; /* size of optional header */
+ uint16_t flags; /* flags */
+};
+
+/* the fact that pe32 isn't padded where pe32+ is 64-bit means union won't
+ * work right. vomit. */
+struct pe32_opt_hdr {
+ /* "standard" header */
+ uint16_t magic; /* file type */
+ uint8_t ld_major; /* linker major version */
+ uint8_t ld_minor; /* linker minor version */
+ uint32_t text_size; /* size of text section(s) */
+ uint32_t data_size; /* size of data section(s) */
+ uint32_t bss_size; /* size of bss section(s) */
+ uint32_t entry_point; /* file offset of entry point */
+ uint32_t code_base; /* relative code addr in ram */
+ uint32_t data_base; /* relative data addr in ram */
+ /* "windows" header */
+ uint32_t image_base; /* preferred load address */
+ uint32_t section_align; /* alignment in bytes */
+ uint32_t file_align; /* file alignment in bytes */
+ uint16_t os_major; /* major OS version */
+ uint16_t os_minor; /* minor OS version */
+ uint16_t image_major; /* major image version */
+ uint16_t image_minor; /* minor image version */
+ uint16_t subsys_major; /* major subsystem version */
+ uint16_t subsys_minor; /* minor subsystem version */
+ uint32_t win32_version; /* reserved, must be 0 */
+ uint32_t image_size; /* image size */
+ uint32_t header_size; /* header size rounded up to file_align */
+ uint32_t csum; /* checksum */
+ uint16_t subsys; /* subsystem */
+ uint16_t dll_flags; /* executable characteristics */
+ uint32_t stack_size_req; /* amt of stack requested */
+ uint32_t stack_size; /* amt of stack required */
+ uint32_t heap_size_req; /* amt of heap requested */
+ uint32_t heap_size; /* amt of heap required */
+ uint32_t loader_flags; /* reserved, must be 0 */
+ uint32_t data_dirs; /* number of data dir entries */
+};
+
+struct pe32plus_opt_hdr {
+ uint16_t magic; /* file type */
+ uint8_t ld_major; /* linker major version */
+ uint8_t ld_minor; /* linker minor version */
+ uint32_t text_size; /* size of text section(s) */
+ uint32_t data_size; /* size of data section(s) */
+ uint32_t bss_size; /* size of bss section(s) */
+ uint32_t entry_point; /* file offset of entry point */
+ uint32_t code_base; /* relative code addr in ram */
+ /* "windows" header */
+ uint64_t image_base; /* preferred load address */
+ uint32_t section_align; /* alignment in bytes */
+ uint32_t file_align; /* file alignment in bytes */
+ uint16_t os_major; /* major OS version */
+ uint16_t os_minor; /* minor OS version */
+ uint16_t image_major; /* major image version */
+ uint16_t image_minor; /* minor image version */
+ uint16_t subsys_major; /* major subsystem version */
+ uint16_t subsys_minor; /* minor subsystem version */
+ uint32_t win32_version; /* reserved, must be 0 */
+ uint32_t image_size; /* image size */
+ uint32_t header_size; /* header size rounded up to file_align */
+ uint32_t csum; /* checksum */
+ uint16_t subsys; /* subsystem */
+ uint16_t dll_flags; /* executable characteristics */
+ uint64_t stack_size_req; /* amt of stack requested */
+ uint64_t stack_size; /* amt of stack required */
+ uint64_t heap_size_req; /* amt of heap requested */
+ uint64_t heap_size; /* amt of heap required */
+ uint32_t loader_flags; /* reserved, must be 0 */
+ uint32_t data_dirs; /* number of data dir entries */
+};
+
+struct data_dirent {
+ uint32_t virtual_address; /* relative to load address */
+ uint32_t size;
+};
+
+struct data_directory {
+ struct data_dirent exports; /* .edata */
+ struct data_dirent imports; /* .idata */
+ struct data_dirent resources; /* .rsrc */
+ struct data_dirent exceptions; /* .pdata */
+ struct data_dirent certs; /* certs */
+ struct data_dirent base_relocations; /* .reloc */
+ struct data_dirent debug; /* .debug */
+ struct data_dirent arch; /* reservered */
+ struct data_dirent global_ptr; /* global pointer reg. Size=0 */
+ struct data_dirent tls; /* .tls */
+ struct data_dirent load_config; /* load configuration structure */
+ struct data_dirent bound_imports; /* no idea */
+ struct data_dirent import_addrs; /* import address table */
+ struct data_dirent delay_imports; /* delay-load import table */
+ struct data_dirent clr_runtime_hdr; /* .cor (object only) */
+ struct data_dirent reserved;
+};
+
+struct section_header {
+ char name[8]; /* name or string tbl offset */
+ uint32_t virtual_size; /* size of loaded section in ram */
+ uint32_t virtual_address; /* relative virtual address */
+ uint32_t raw_data_size; /* size of the section */
+ uint32_t data_addr; /* file pointer to first page of sec */
+ uint32_t relocs; /* file pointer to relocation entries */
+ uint32_t line_numbers; /* line numbers */
+ uint16_t num_relocs; /* number of relocations */
+ uint16_t num_lin_numbers; /* COFF line count. */
+ uint32_t flags;
+};
+
+enum x64_coff_reloc_type {
+ IMAGE_REL_AMD64_ABSOLUTE = 0,
+ IMAGE_REL_AMD64_ADDR64,
+ IMAGE_REL_AMD64_ADDR32,
+ IMAGE_REL_AMD64_ADDR32N,
+ IMAGE_REL_AMD64_REL32,
+ IMAGE_REL_AMD64_REL32_1,
+ IMAGE_REL_AMD64_REL32_2,
+ IMAGE_REL_AMD64_REL32_3,
+ IMAGE_REL_AMD64_REL32_4,
+ IMAGE_REL_AMD64_REL32_5,
+ IMAGE_REL_AMD64_SECTION,
+ IMAGE_REL_AMD64_SECREL,
+ IMAGE_REL_AMD64_SECREL7,
+ IMAGE_REL_AMD64_TOKEN,
+ IMAGE_REL_AMD64_SREL32,
+ IMAGE_REL_AMD64_PAIR,
+ IMAGE_REL_AMD64_SSPAN32,
+};
+
+enum arm_coff_reloc_type {
+ IMAGE_REL_ARM_ABSOLUTE,
+ IMAGE_REL_ARM_ADDR32,
+ IMAGE_REL_ARM_ADDR32N,
+ IMAGE_REL_ARM_BRANCH2,
+ IMAGE_REL_ARM_BRANCH1,
+ IMAGE_REL_ARM_SECTION,
+ IMAGE_REL_ARM_SECREL,
+};
+
+enum sh_coff_reloc_type {
+ IMAGE_REL_SH3_ABSOLUTE,
+ IMAGE_REL_SH3_DIRECT16,
+ IMAGE_REL_SH3_DIRECT32,
+ IMAGE_REL_SH3_DIRECT8,
+ IMAGE_REL_SH3_DIRECT8_WORD,
+ IMAGE_REL_SH3_DIRECT8_LONG,
+ IMAGE_REL_SH3_DIRECT4,
+ IMAGE_REL_SH3_DIRECT4_WORD,
+ IMAGE_REL_SH3_DIRECT4_LONG,
+ IMAGE_REL_SH3_PCREL8_WORD,
+ IMAGE_REL_SH3_PCREL8_LONG,
+ IMAGE_REL_SH3_PCREL12_WORD,
+ IMAGE_REL_SH3_STARTOF_SECTION,
+ IMAGE_REL_SH3_SIZEOF_SECTION,
+ IMAGE_REL_SH3_SECTION,
+ IMAGE_REL_SH3_SECREL,
+ IMAGE_REL_SH3_DIRECT32_NB,
+ IMAGE_REL_SH3_GPREL4_LONG,
+ IMAGE_REL_SH3_TOKEN,
+ IMAGE_REL_SHM_PCRELPT,
+ IMAGE_REL_SHM_REFLO,
+ IMAGE_REL_SHM_REFHALF,
+ IMAGE_REL_SHM_RELLO,
+ IMAGE_REL_SHM_RELHALF,
+ IMAGE_REL_SHM_PAIR,
+ IMAGE_REL_SHM_NOMODE,
+};
+
+enum ppc_coff_reloc_type {
+ IMAGE_REL_PPC_ABSOLUTE,
+ IMAGE_REL_PPC_ADDR64,
+ IMAGE_REL_PPC_ADDR32,
+ IMAGE_REL_PPC_ADDR24,
+ IMAGE_REL_PPC_ADDR16,
+ IMAGE_REL_PPC_ADDR14,
+ IMAGE_REL_PPC_REL24,
+ IMAGE_REL_PPC_REL14,
+ IMAGE_REL_PPC_ADDR32N,
+ IMAGE_REL_PPC_SECREL,
+ IMAGE_REL_PPC_SECTION,
+ IMAGE_REL_PPC_SECREL16,
+ IMAGE_REL_PPC_REFHI,
+ IMAGE_REL_PPC_REFLO,
+ IMAGE_REL_PPC_PAIR,
+ IMAGE_REL_PPC_SECRELLO,
+ IMAGE_REL_PPC_GPREL,
+ IMAGE_REL_PPC_TOKEN,
+};
+
+enum x86_coff_reloc_type {
+ IMAGE_REL_I386_ABSOLUTE,
+ IMAGE_REL_I386_DIR16,
+ IMAGE_REL_I386_REL16,
+ IMAGE_REL_I386_DIR32,
+ IMAGE_REL_I386_DIR32NB,
+ IMAGE_REL_I386_SEG12,
+ IMAGE_REL_I386_SECTION,
+ IMAGE_REL_I386_SECREL,
+ IMAGE_REL_I386_TOKEN,
+ IMAGE_REL_I386_SECREL7,
+ IMAGE_REL_I386_REL32,
+};
+
+enum ia64_coff_reloc_type {
+ IMAGE_REL_IA64_ABSOLUTE,
+ IMAGE_REL_IA64_IMM14,
+ IMAGE_REL_IA64_IMM22,
+ IMAGE_REL_IA64_IMM64,
+ IMAGE_REL_IA64_DIR32,
+ IMAGE_REL_IA64_DIR64,
+ IMAGE_REL_IA64_PCREL21B,
+ IMAGE_REL_IA64_PCREL21M,
+ IMAGE_REL_IA64_PCREL21F,
+ IMAGE_REL_IA64_GPREL22,
+ IMAGE_REL_IA64_LTOFF22,
+ IMAGE_REL_IA64_SECTION,
+ IMAGE_REL_IA64_SECREL22,
+ IMAGE_REL_IA64_SECREL64I,
+ IMAGE_REL_IA64_SECREL32,
+ IMAGE_REL_IA64_DIR32NB,
+ IMAGE_REL_IA64_SREL14,
+ IMAGE_REL_IA64_SREL22,
+ IMAGE_REL_IA64_SREL32,
+ IMAGE_REL_IA64_UREL32,
+ IMAGE_REL_IA64_PCREL60X,
+ IMAGE_REL_IA64_PCREL60B,
+ IMAGE_REL_IA64_PCREL60F,
+ IMAGE_REL_IA64_PCREL60I,
+ IMAGE_REL_IA64_PCREL60M,
+ IMAGE_REL_IA64_IMMGPREL6,
+ IMAGE_REL_IA64_TOKEN,
+ IMAGE_REL_IA64_GPREL32,
+ IMAGE_REL_IA64_ADDEND,
+};
+
+struct coff_reloc {
+ uint32_t virtual_address;
+ uint32_t symbol_table_index;
+
+ union {
+ enum x64_coff_reloc_type x64_type;
+ enum arm_coff_reloc_type arm_type;
+ enum sh_coff_reloc_type sh_type;
+ enum ppc_coff_reloc_type ppc_type;
+ enum x86_coff_reloc_type x86_type;
+ enum ia64_coff_reloc_type ia64_type;
+ uint16_t data;
+ };
+};
+
+/*
+ * Definitions for the contents of the certs data block
+ */
+#define WIN_CERT_TYPE_PKCS_SIGNED_DATA 0x0002
+#define WIN_CERT_TYPE_EFI_OKCS115 0x0EF0
+#define WIN_CERT_TYPE_EFI_GUID 0x0EF1
+
+#define WIN_CERT_REVISION_1_0 0x0100
+#define WIN_CERT_REVISION_2_0 0x0200
+
+struct win_certificate {
+ uint32_t length;
+ uint16_t revision;
+ uint16_t cert_type;
+};
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __PE_H */
--
2.25.1On 03.07.2024 02:04, Milan Djokic wrote:
> Added riscv image header to enable boot from second stage bootloaders (e.g. uboot. Image header defined in riscv-image-header.h)
> Additionally, RISC-V xen image is extended with PE/COFF headers, introducing EFI application format.
> PE/COFF 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.
> Note that this patch represents initial EFI application format support (image
> contains EFI application header embeded into binary when CONFIG_RISCV_EFI
> is enabled). For full EFI application Xen support, boot/runtime services
> and system table handling are yet to be implemented.
So, first: Please Cc all applicable maintainers. It would probably be prudent
to also Cc Oleksii, who's doing most of the RISC-V work now (but Oleksii,
please correct me if you don't want to be Cc-ed).
With Oleksii in the audience, second: I tink I've seen that in binutils work
is being done to actually allow to create EFI applications "properly" for
RISC-V. Was it firmly determined that you/we do not want to go that route?
> --- a/xen/arch/riscv/Kconfig
> +++ b/xen/arch/riscv/Kconfig
> @@ -9,6 +9,16 @@ config ARCH_DEFCONFIG
> string
> default "arch/riscv/configs/tiny64_defconfig"
>
> +config RISCV_EFI
> + bool "UEFI boot service support"
> + depends on RISCV_64 && RISCV_ISA_C
> + 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. Currently, only EFI PE/COFF application
> + header is included into RISC-V image. Boot/Runtime services as part
> + of EFI application stub are yet to be implemented.
The first sentence in particular worries me. What it says is basically all
taken back by what follows. I think this help text wants reducing to a
minimum, andthen wants replacing once proper EFI support is in place.
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/riscv-image-header.h
> @@ -0,0 +1,57 @@
> +#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_LE 0
> +#define RISCV_IMAGE_FLAG_BE 1
> +
> +#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_LE
> +
> +#define __HEAD_FLAG(field) \
> + (__HEAD_FLAG_##field << RISCV_IMAGE_FLAG_##field##_SHIFT)
Nit: Generally we treat ## as a binary operator, too. Which means it also
wants to be surrounded by blanks.
> +#define __HEAD_FLAGS (__HEAD_FLAG(BE))
Nit: The outer pair of parentheses isn't needed here, is it?
> +#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: Please parenthesize the << against the |.
> --- /dev/null
> +++ b/xen/arch/riscv/riscv64/efi-header.S
> @@ -0,0 +1,99 @@
> +#include <efi/pe.h>
> +#include <xen/sizes.h>
> +
> + .macro __EFI_PE_HEADER
> + .long PE_MAGIC
> +coff_header:
> +#ifdef CONFIG_RISCV_64
> + .short IMAGE_FILE_MACHINE_RISCV64 /* Machine */
> +#else
> + .short IMAGE_FILE_MACHINE_RISCV32 /* Machine */
> +#endif
> + .short section_count /* NumberOfSections */
> + .long 0 /* TimeDateStamp */
> + .long 0 /* PointerToSymbolTable */
> + .long 0 /* NumberOfSymbols */
> + .short section_table - optional_header /* SizeOfOptionalHeader */
> + .short IMAGE_FILE_DEBUG_STRIPPED | \
> + IMAGE_FILE_EXECUTABLE_IMAGE | \
> + IMAGE_FILE_LINE_NUMS_STRIPPED /* Characteristics */
> +
> +optional_header:
> +#ifdef CONFIG_RISCV_64
> + .short PE_OPT_MAGIC_PE32PLUS /* PE32+ format */
> +#else
> + .short PE_OPT_MAGIC_PE32 /* PE32 format */
> +#endif
> + .byte 0x02 /* MajorLinkerVersion */
> + .byte 0x14 /* MinorLinkerVersion */
> + .long _end - xen_start /* SizeOfCode */
> + .long 0 /* SizeOfInitializedData */
> + .long 0 /* SizeOfUninitializedData */
> + .long 0 /* AddressOfEntryPoint */
> + .long xen_start - _start /* BaseOfCode */
> +#ifdef CONFIG_RISCV_32
> + .long _end - _start /* BaseOfData */
> +#endif
As requested before, the decision to represent all of .text/.data/.bss
as code (with no data at all) wants explaining in a (possibly brief)
comment.
> +extra_header_fields:
> + .quad 0 /* ImageBase */
> + .long PECOFF_SECTION_ALIGNMENT /* SectionAlignment */
> + .long PECOFF_FILE_ALIGNMENT /* FileAlignment */
> + .short 0 /* MajorOperatingSystemVersion */
> + .short 0 /* MinorOperatingSystemVersion */
> + .short LINUX_EFISTUB_MAJOR_VERSION /* MajorImageVersion */
> + .short LINUX_EFISTUB_MINOR_VERSION /* MinorImageVersion */
> + .short 0 /* MajorSubsystemVersion */
> + .short 0 /* MinorSubsystemVersion */
> + .long 0 /* Win32VersionValue */
> + .long _end - _start /* SizeOfImage */
> +
> + /* Everything before the xen image is considered part of the header */
> + .long xen_start - _start /* SizeOfHeaders */
> + .long 0 /* CheckSum */
> + .short IMAGE_SUBSYSTEM_EFI_APPLICATION /* Subsystem */
> + .short 0 /* DllCharacteristics */
> + .quad 0 /* SizeOfStackReserve */
> + .quad 0 /* SizeOfStackCommit */
> + .quad 0 /* SizeOfHeapReserve */
> + .quad 0 /* SizeOfHeapCommit */
> + .long 0 /* LoaderFlags */
> + .long (section_table - .) / 8 /* NumberOfRvaAndSizes */
> + /*
> + * Data directories are not used in this case, therefore not defined to reduce header size.
> + */
> +
> + /* Section table */
> +section_table:
> + /* Currently code/data sections are not used since EFI stub implementation is not yet finalized */
> + .ascii ".text\0\0\0"
> + .long 0 /* VirtualSize */
> + .long 0 /* VirtualAddress */
> + .long 0 /* SizeOfRawData */
> + .long 0 /* PointerToRawData */
> + .long 0 /* PointerToRelocations */
> + .long 0 /* PointerToLineNumbers */
> + .short 0 /* NumberOfRelocations */
> + .short 0 /* NumberOfLineNumbers */
> + .long IMAGE_SCN_CNT_CODE | \
> + IMAGE_SCN_MEM_READ | \
> + IMAGE_SCN_MEM_EXECUTE /* Characteristics */
> +
> + .ascii ".data\0\0\0"
> + .long 0 /* VirtualSize */
> + .long 0 /* VirtualAddress */
> + .long 0 /* SizeOfRawData */
> + .long 0 /* PointerToRawData */
> + .long 0 /* PointerToRelocations */
> + .long 0 /* PointerToLineNumbers */
> + .short 0 /* NumberOfRelocations */
> + .short 0 /* NumberOfLineNumbers */
> + .long IMAGE_SCN_CNT_INITIALIZED_DATA | \
> + IMAGE_SCN_MEM_READ | \
> + IMAGE_SCN_MEM_WRITE /* Characteristics */
> +
> + .set section_count, (. - section_table) / 40
> +
> + .balign 0x1000
> +efi_header_end:
> + .endm
> \ No newline at end of file
Please take care of this.
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -1,14 +1,54 @@
> #include <asm/asm.h>
> #include <asm/riscv_encoding.h>
> +#include <asm/riscv-image-header.h>
> +#ifdef CONFIG_RISCV_EFI
> +#include "efi-header.S"
> +#endif
>
> .section .text.header, "ax", %progbits
> -
> /*
I see no reason for removing this blank line.
> * OpenSBI pass to start():
> * a0 -> hart_id ( bootcpu_id )
> - * a1 -> dtb_base
> + * a1 -> dtb_base
> */
> FUNC(start)
> +/* Image header expected by second stage bootloaders (format defined in asm/riscv-image-header.h) */
Nit: Overlong line (and not the only one).
> +#ifdef CONFIG_RISCV_EFI
> + /*
> + * This instruction decodes to "MZ" ASCII required by UEFI.
> + */
> + c.li s4,-13
> + c.j xen_start
> +#else
> + /* jump to start kernel */
> + jal xen_start
JAL, not J? Why?
> +#endif
> + .balign 8
This won't do what you want unless "start" itself is also suitably aligned.
It'll be as long as it's first in the section, but better make such explicit.
> +#ifdef CONFIG_RISCV_64
> + /* Image load offset(2MB) from start of RAM */
> + .quad 0x200000
> +#else
> + /* Image load offset(4MB) from start of RAM */
> + .quad 0x400000
> +#endif
What these constants derive from? I expect they aren't really "magic".
> + .quad _end - _start /* Effective Image size */
> + .quad __HEAD_FLAGS
> + .long RISCV_HEADER_VERSION
In the C struct this is a 64-bit field. Why .long here? Or perhaps the C
struct is wrong, actually also leaving unspecified padding there after
...
> + .long 0 /* reserved */
... this field then.
> + .quad 0 /* reserved */
> + .ascii RISCV_IMAGE_MAGIC /* Magic number (RISC-V specific; deprecated) */
> + .balign 4
RISCV_IMAGE_MAGIC is already nul-padded to 8 bytes. I therefore find
the .balign here somewhat confusing.
> + .ascii RISCV_IMAGE_MAGIC2 /* Magic number 2 (to match the ARM64 'magic' field pos) */
> +#ifdef CONFIG_RISCV_EFI
> + .long pe_head_start - _start /* reserved (PE COFF offset) */
> +pe_head_start:
> +
> + __EFI_PE_HEADER
Using a macro for a single, simple purpose is somewhat unexpected.
Can't you simply
#include "efi-header.S"
here? That would also make it more straightforward to find the use /
purpose of that file.
> +#else
> + .long 0 /* 0 means no PE header. */
> +#endif
> +
> +xen_start:
> /* Mask all interrupts */
> csrw CSR_SIE, zero
>
> @@ -60,6 +100,7 @@ FUNC(start)
> mv a1, s1
>
> tail start_xen
> +
> END(start)
>
> .section .text, "ax", %progbits
What is this hunk about?
> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -12,6 +12,9 @@ PHDRS
> #endif
> }
>
> +PECOFF_SECTION_ALIGNMENT = 0x1000;
> +PECOFF_FILE_ALIGNMENT = 0x200;
What are these about? I see you use them in efi-header.S, but why do
they need supplying from the linker script?
> --- /dev/null
> +++ b/xen/include/efi/pe.h
> @@ -0,0 +1,458 @@
> +#ifndef __PE_H
This probably wants (needs?) to gain an SPDX line.
> +#define __PE_H
> +
> +/*
> + * Linux EFI stub v1.0 adds the following functionality:
> + * - Loading initrd from the LINUX_EFI_INITRD_MEDIA_GUID device path,
> + * - Loading/starting the kernel from firmware that targets a different
> + * machine type, via the entrypoint exposed in the .compat PE/COFF section.
> + *
> + * The recommended way of loading and starting v1.0 or later kernels is to use
> + * the LoadImage() and StartImage() EFI boot services, and expose the initrd
> + * via the LINUX_EFI_INITRD_MEDIA_GUID device path.
> + *
> + * Versions older than v1.0 support initrd loading via the image load options
> + * (using initrd=, limited to the volume from which the kernel itself was
> + * loaded), or via arch specific means (bootparams, DT, etc).
> + *
> + * On x86, LoadImage() and StartImage() can be omitted if the EFI handover
> + * protocol is implemented, which can be inferred from the version,
> + * handover_offset and xloadflags fields in the bootparams structure.
> + */
> +#define LINUX_EFISTUB_MAJOR_VERSION 0x1
> +#define LINUX_EFISTUB_MINOR_VERSION 0x0
> +
> +#define MZ_MAGIC 0x5a4d /* "MZ" */
> +
> +#define PE_MAGIC 0x00004550 /* "PE\0\0" */
> +#define PE_OPT_MAGIC_PE32 0x010b
> +#define PE_OPT_MAGIC_PE32_ROM 0x0107
> +#define PE_OPT_MAGIC_PE32PLUS 0x020b
> +
> +/* machine type */
> +#define IMAGE_FILE_MACHINE_UNKNOWN 0x0000
> +#define IMAGE_FILE_MACHINE_AM33 0x01d3
> +#define IMAGE_FILE_MACHINE_AMD64 0x8664
> +#define IMAGE_FILE_MACHINE_ARM 0x01c0
> +#define IMAGE_FILE_MACHINE_ARMV7 0x01c4
> +#define IMAGE_FILE_MACHINE_ARM64 0xaa64
> +#define IMAGE_FILE_MACHINE_EBC 0x0ebc
> +#define IMAGE_FILE_MACHINE_I386 0x014c
> +#define IMAGE_FILE_MACHINE_IA64 0x0200
> +#define IMAGE_FILE_MACHINE_M32R 0x9041
> +#define IMAGE_FILE_MACHINE_MIPS16 0x0266
> +#define IMAGE_FILE_MACHINE_MIPSFPU 0x0366
> +#define IMAGE_FILE_MACHINE_MIPSFPU16 0x0466
> +#define IMAGE_FILE_MACHINE_POWERPC 0x01f0
> +#define IMAGE_FILE_MACHINE_POWERPCFP 0x01f1
> +#define IMAGE_FILE_MACHINE_R4000 0x0166
> +#define IMAGE_FILE_MACHINE_RISCV32 0x5032
> +#define IMAGE_FILE_MACHINE_RISCV64 0x5064
> +#define IMAGE_FILE_MACHINE_RISCV128 0x5128
> +#define IMAGE_FILE_MACHINE_SH3 0x01a2
> +#define IMAGE_FILE_MACHINE_SH3DSP 0x01a3
> +#define IMAGE_FILE_MACHINE_SH3E 0x01a4
> +#define IMAGE_FILE_MACHINE_SH4 0x01a6
> +#define IMAGE_FILE_MACHINE_SH5 0x01a8
> +#define IMAGE_FILE_MACHINE_THUMB 0x01c2
> +#define IMAGE_FILE_MACHINE_WCEMIPSV2 0x0169
> +
> +/* flags */
> +#define IMAGE_FILE_RELOCS_STRIPPED 0x0001
> +#define IMAGE_FILE_EXECUTABLE_IMAGE 0x0002
> +#define IMAGE_FILE_LINE_NUMS_STRIPPED 0x0004
> +#define IMAGE_FILE_LOCAL_SYMS_STRIPPED 0x0008
> +#define IMAGE_FILE_AGGRESSIVE_WS_TRIM 0x0010
> +#define IMAGE_FILE_LARGE_ADDRESS_AWARE 0x0020
> +#define IMAGE_FILE_16BIT_MACHINE 0x0040
> +#define IMAGE_FILE_BYTES_REVERSED_LO 0x0080
> +#define IMAGE_FILE_32BIT_MACHINE 0x0100
> +#define IMAGE_FILE_DEBUG_STRIPPED 0x0200
> +#define IMAGE_FILE_REMOVABLE_RUN_FROM_SWAP 0x0400
> +#define IMAGE_FILE_NET_RUN_FROM_SWAP 0x0800
> +#define IMAGE_FILE_SYSTEM 0x1000
> +#define IMAGE_FILE_DLL 0x2000
> +#define IMAGE_FILE_UP_SYSTEM_ONLY 0x4000
> +#define IMAGE_FILE_BYTES_REVERSED_HI 0x8000
> +
> +#define IMAGE_FILE_OPT_ROM_MAGIC 0x107
> +#define IMAGE_FILE_OPT_PE32_MAGIC 0x10b
> +#define IMAGE_FILE_OPT_PE32_PLUS_MAGIC 0x20b
> +
> +#define IMAGE_SUBSYSTEM_UNKNOWN 0
> +#define IMAGE_SUBSYSTEM_NATIVE 1
> +#define IMAGE_SUBSYSTEM_WINDOWS_GUI 2
> +#define IMAGE_SUBSYSTEM_WINDOWS_CUI 3
> +#define IMAGE_SUBSYSTEM_POSIX_CUI 7
> +#define IMAGE_SUBSYSTEM_WINDOWS_CE_GUI 9
> +#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10
> +#define IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER 11
> +#define IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER 12
> +#define IMAGE_SUBSYSTEM_EFI_ROM_IMAGE 13
> +#define IMAGE_SUBSYSTEM_XBOX 14
> +
> +#define IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE 0x0040
> +#define IMAGE_DLL_CHARACTERISTICS_FORCE_INTEGRITY 0x0080
> +#define IMAGE_DLL_CHARACTERISTICS_NX_COMPAT 0x0100
> +#define IMAGE_DLLCHARACTERISTICS_NO_ISOLATION 0x0200
> +#define IMAGE_DLLCHARACTERISTICS_NO_SEH 0x0400
> +#define IMAGE_DLLCHARACTERISTICS_NO_BIND 0x0800
> +#define IMAGE_DLLCHARACTERISTICS_WDM_DRIVER 0x2000
> +#define IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE 0x8000
> +
> +/* they actually defined 0x00000000 as well, but I think we'll skip that one. */
> +#define IMAGE_SCN_RESERVED_0 0x00000001
> +#define IMAGE_SCN_RESERVED_1 0x00000002
> +#define IMAGE_SCN_RESERVED_2 0x00000004
> +#define IMAGE_SCN_TYPE_NO_PAD 0x00000008 /* don't pad - obsolete */
> +#define IMAGE_SCN_RESERVED_3 0x00000010
> +#define IMAGE_SCN_CNT_CODE 0x00000020 /* .text */
> +#define IMAGE_SCN_CNT_INITIALIZED_DATA 0x00000040 /* .data */
> +#define IMAGE_SCN_CNT_UNINITIALIZED_DATA 0x00000080 /* .bss */
> +#define IMAGE_SCN_LNK_OTHER 0x00000100 /* reserved */
> +#define IMAGE_SCN_LNK_INFO 0x00000200 /* .drectve comments */
> +#define IMAGE_SCN_RESERVED_4 0x00000400
> +#define IMAGE_SCN_LNK_REMOVE 0x00000800 /* .o only - scn to be rm'd*/
> +#define IMAGE_SCN_LNK_COMDAT 0x00001000 /* .o only - COMDAT data */
> +#define IMAGE_SCN_RESERVED_5 0x00002000 /* spec omits this */
> +#define IMAGE_SCN_RESERVED_6 0x00004000 /* spec omits this */
> +#define IMAGE_SCN_GPREL 0x00008000 /* global pointer referenced data */
> +/* spec lists 0x20000 twice, I suspect they meant 0x10000 for one of them */
> +#define IMAGE_SCN_MEM_PURGEABLE 0x00010000 /* reserved for "future" use */
> +#define IMAGE_SCN_16BIT 0x00020000 /* reserved for "future" use */
> +#define IMAGE_SCN_LOCKED 0x00040000 /* reserved for "future" use */
> +#define IMAGE_SCN_PRELOAD 0x00080000 /* reserved for "future" use */
> +/* and here they just stuck a 1-byte integer in the middle of a bitfield */
> +#define IMAGE_SCN_ALIGN_1BYTES 0x00100000 /* it does what it says on the box */
> +#define IMAGE_SCN_ALIGN_2BYTES 0x00200000
> +#define IMAGE_SCN_ALIGN_4BYTES 0x00300000
> +#define IMAGE_SCN_ALIGN_8BYTES 0x00400000
> +#define IMAGE_SCN_ALIGN_16BYTES 0x00500000
> +#define IMAGE_SCN_ALIGN_32BYTES 0x00600000
> +#define IMAGE_SCN_ALIGN_64BYTES 0x00700000
> +#define IMAGE_SCN_ALIGN_128BYTES 0x00800000
> +#define IMAGE_SCN_ALIGN_256BYTES 0x00900000
> +#define IMAGE_SCN_ALIGN_512BYTES 0x00a00000
> +#define IMAGE_SCN_ALIGN_1024BYTES 0x00b00000
> +#define IMAGE_SCN_ALIGN_2048BYTES 0x00c00000
> +#define IMAGE_SCN_ALIGN_4096BYTES 0x00d00000
> +#define IMAGE_SCN_ALIGN_8192BYTES 0x00e00000
> +#define IMAGE_SCN_LNK_NRELOC_OVFL 0x01000000 /* extended relocations */
> +#define IMAGE_SCN_MEM_DISCARDABLE 0x02000000 /* scn can be discarded */
> +#define IMAGE_SCN_MEM_NOT_CACHED 0x04000000 /* cannot be cached */
> +#define IMAGE_SCN_MEM_NOT_PAGED 0x08000000 /* not pageable */
> +#define IMAGE_SCN_MEM_SHARED 0x10000000 /* can be shared */
> +#define IMAGE_SCN_MEM_EXECUTE 0x20000000 /* can be executed as code */
> +#define IMAGE_SCN_MEM_READ 0x40000000 /* readable */
> +#define IMAGE_SCN_MEM_WRITE 0x80000000 /* writeable */
> +
> +#define IMAGE_DEBUG_TYPE_CODEVIEW 2
> +
> +#ifndef __ASSEMBLY__
> +
> +struct mz_hdr {
> + uint16_t magic; /* MZ_MAGIC */
> + uint16_t lbsize; /* size of last used block */
> + uint16_t blocks; /* pages in file, 0x3 */
> + uint16_t relocs; /* relocations */
> + uint16_t hdrsize; /* header size in "paragraphs" */
> + uint16_t min_extra_pps; /* .bss */
> + uint16_t max_extra_pps; /* runtime limit for the arena size */
> + uint16_t ss; /* relative stack segment */
> + uint16_t sp; /* initial %sp register */
> + uint16_t checksum; /* word checksum */
> + uint16_t ip; /* initial %ip register */
> + uint16_t cs; /* initial %cs relative to load segment */
> + uint16_t reloc_table_offset; /* offset of the first relocation */
> + uint16_t overlay_num; /* overlay number. set to 0. */
> + uint16_t reserved0[4]; /* reserved */
> + uint16_t oem_id; /* oem identifier */
> + uint16_t oem_info; /* oem specific */
> + uint16_t reserved1[10]; /* reserved */
> + uint32_t peaddr; /* address of pe header */
> + char message[]; /* message to print */
> +};
We already have an instance of this struct in common/efi/pe.c. I think
it wouldn't be very desirable to have two (different) instances.
> +struct mz_reloc {
> + uint16_t offset;
> + uint16_t segment;
> +};
We aren't going to need this anywhere, are we?
> +struct pe_hdr {
> + uint32_t magic; /* PE magic */
> + uint16_t machine; /* machine type */
> + uint16_t sections; /* number of sections */
> + uint32_t timestamp; /* time_t */
> + uint32_t symbol_table; /* symbol table offset */
> + uint32_t symbols; /* number of symbols */
> + uint16_t opt_hdr_size; /* size of optional header */
> + uint16_t flags; /* flags */
> +};
And again another (different) instance of this and further struct-s
already exists. Same for the section header further down.
> +/* the fact that pe32 isn't padded where pe32+ is 64-bit means union won't
> + * work right. vomit. */
Noticing here in particular, but being an issue elsewhere as well:
Unless this file is to be a verbatim copy taken from somewhere (in
which case it should probably be introduced in a separate commit
with an Origin: tag), comments want to adhere of ./CODING_STYLE.
> +struct pe32_opt_hdr {
> + /* "standard" header */
> + uint16_t magic; /* file type */
> + uint8_t ld_major; /* linker major version */
> + uint8_t ld_minor; /* linker minor version */
> + uint32_t text_size; /* size of text section(s) */
> + uint32_t data_size; /* size of data section(s) */
> + uint32_t bss_size; /* size of bss section(s) */
> + uint32_t entry_point; /* file offset of entry point */
> + uint32_t code_base; /* relative code addr in ram */
> + uint32_t data_base; /* relative data addr in ram */
> + /* "windows" header */
> + uint32_t image_base; /* preferred load address */
> + uint32_t section_align; /* alignment in bytes */
> + uint32_t file_align; /* file alignment in bytes */
> + uint16_t os_major; /* major OS version */
> + uint16_t os_minor; /* minor OS version */
> + uint16_t image_major; /* major image version */
> + uint16_t image_minor; /* minor image version */
> + uint16_t subsys_major; /* major subsystem version */
> + uint16_t subsys_minor; /* minor subsystem version */
> + uint32_t win32_version; /* reserved, must be 0 */
> + uint32_t image_size; /* image size */
> + uint32_t header_size; /* header size rounded up to file_align */
> + uint32_t csum; /* checksum */
> + uint16_t subsys; /* subsystem */
> + uint16_t dll_flags; /* executable characteristics */
> + uint32_t stack_size_req; /* amt of stack requested */
> + uint32_t stack_size; /* amt of stack required */
> + uint32_t heap_size_req; /* amt of heap requested */
> + uint32_t heap_size; /* amt of heap required */
> + uint32_t loader_flags; /* reserved, must be 0 */
> + uint32_t data_dirs; /* number of data dir entries */
> +};
> +
> +struct pe32plus_opt_hdr {
> + uint16_t magic; /* file type */
> + uint8_t ld_major; /* linker major version */
> + uint8_t ld_minor; /* linker minor version */
> + uint32_t text_size; /* size of text section(s) */
> + uint32_t data_size; /* size of data section(s) */
> + uint32_t bss_size; /* size of bss section(s) */
> + uint32_t entry_point; /* file offset of entry point */
> + uint32_t code_base; /* relative code addr in ram */
> + /* "windows" header */
> + uint64_t image_base; /* preferred load address */
> + uint32_t section_align; /* alignment in bytes */
> + uint32_t file_align; /* file alignment in bytes */
> + uint16_t os_major; /* major OS version */
> + uint16_t os_minor; /* minor OS version */
> + uint16_t image_major; /* major image version */
> + uint16_t image_minor; /* minor image version */
> + uint16_t subsys_major; /* major subsystem version */
> + uint16_t subsys_minor; /* minor subsystem version */
> + uint32_t win32_version; /* reserved, must be 0 */
> + uint32_t image_size; /* image size */
> + uint32_t header_size; /* header size rounded up to file_align */
> + uint32_t csum; /* checksum */
> + uint16_t subsys; /* subsystem */
> + uint16_t dll_flags; /* executable characteristics */
> + uint64_t stack_size_req; /* amt of stack requested */
> + uint64_t stack_size; /* amt of stack required */
> + uint64_t heap_size_req; /* amt of heap requested */
> + uint64_t heap_size; /* amt of heap required */
> + uint32_t loader_flags; /* reserved, must be 0 */
> + uint32_t data_dirs; /* number of data dir entries */
> +};
> +
> +struct data_dirent {
> + uint32_t virtual_address; /* relative to load address */
> + uint32_t size;
> +};
Will we need this and ...
> +struct data_directory {
> + struct data_dirent exports; /* .edata */
> + struct data_dirent imports; /* .idata */
> + struct data_dirent resources; /* .rsrc */
> + struct data_dirent exceptions; /* .pdata */
> + struct data_dirent certs; /* certs */
> + struct data_dirent base_relocations; /* .reloc */
> + struct data_dirent debug; /* .debug */
> + struct data_dirent arch; /* reservered */
> + struct data_dirent global_ptr; /* global pointer reg. Size=0 */
> + struct data_dirent tls; /* .tls */
> + struct data_dirent load_config; /* load configuration structure */
> + struct data_dirent bound_imports; /* no idea */
> + struct data_dirent import_addrs; /* import address table */
> + struct data_dirent delay_imports; /* delay-load import table */
> + struct data_dirent clr_runtime_hdr; /* .cor (object only) */
> + struct data_dirent reserved;
> +};
... this?
> +struct section_header {
> + char name[8]; /* name or string tbl offset */
> + uint32_t virtual_size; /* size of loaded section in ram */
> + uint32_t virtual_address; /* relative virtual address */
> + uint32_t raw_data_size; /* size of the section */
> + uint32_t data_addr; /* file pointer to first page of sec */
> + uint32_t relocs; /* file pointer to relocation entries */
> + uint32_t line_numbers; /* line numbers */
> + uint16_t num_relocs; /* number of relocations */
> + uint16_t num_lin_numbers; /* COFF line count. */
> + uint32_t flags;
> +};
> +
> +enum x64_coff_reloc_type {
> + IMAGE_REL_AMD64_ABSOLUTE = 0,
> + IMAGE_REL_AMD64_ADDR64,
> + IMAGE_REL_AMD64_ADDR32,
> + IMAGE_REL_AMD64_ADDR32N,
> + IMAGE_REL_AMD64_REL32,
> + IMAGE_REL_AMD64_REL32_1,
> + IMAGE_REL_AMD64_REL32_2,
> + IMAGE_REL_AMD64_REL32_3,
> + IMAGE_REL_AMD64_REL32_4,
> + IMAGE_REL_AMD64_REL32_5,
> + IMAGE_REL_AMD64_SECTION,
> + IMAGE_REL_AMD64_SECREL,
> + IMAGE_REL_AMD64_SECREL7,
> + IMAGE_REL_AMD64_TOKEN,
> + IMAGE_REL_AMD64_SREL32,
> + IMAGE_REL_AMD64_PAIR,
> + IMAGE_REL_AMD64_SSPAN32,
> +};
> +
> +enum arm_coff_reloc_type {
> + IMAGE_REL_ARM_ABSOLUTE,
> + IMAGE_REL_ARM_ADDR32,
> + IMAGE_REL_ARM_ADDR32N,
> + IMAGE_REL_ARM_BRANCH2,
> + IMAGE_REL_ARM_BRANCH1,
> + IMAGE_REL_ARM_SECTION,
> + IMAGE_REL_ARM_SECREL,
> +};
> +
> +enum sh_coff_reloc_type {
> + IMAGE_REL_SH3_ABSOLUTE,
> + IMAGE_REL_SH3_DIRECT16,
> + IMAGE_REL_SH3_DIRECT32,
> + IMAGE_REL_SH3_DIRECT8,
> + IMAGE_REL_SH3_DIRECT8_WORD,
> + IMAGE_REL_SH3_DIRECT8_LONG,
> + IMAGE_REL_SH3_DIRECT4,
> + IMAGE_REL_SH3_DIRECT4_WORD,
> + IMAGE_REL_SH3_DIRECT4_LONG,
> + IMAGE_REL_SH3_PCREL8_WORD,
> + IMAGE_REL_SH3_PCREL8_LONG,
> + IMAGE_REL_SH3_PCREL12_WORD,
> + IMAGE_REL_SH3_STARTOF_SECTION,
> + IMAGE_REL_SH3_SIZEOF_SECTION,
> + IMAGE_REL_SH3_SECTION,
> + IMAGE_REL_SH3_SECREL,
> + IMAGE_REL_SH3_DIRECT32_NB,
> + IMAGE_REL_SH3_GPREL4_LONG,
> + IMAGE_REL_SH3_TOKEN,
> + IMAGE_REL_SHM_PCRELPT,
> + IMAGE_REL_SHM_REFLO,
> + IMAGE_REL_SHM_REFHALF,
> + IMAGE_REL_SHM_RELLO,
> + IMAGE_REL_SHM_RELHALF,
> + IMAGE_REL_SHM_PAIR,
> + IMAGE_REL_SHM_NOMODE,
> +};
> +
> +enum ppc_coff_reloc_type {
> + IMAGE_REL_PPC_ABSOLUTE,
> + IMAGE_REL_PPC_ADDR64,
> + IMAGE_REL_PPC_ADDR32,
> + IMAGE_REL_PPC_ADDR24,
> + IMAGE_REL_PPC_ADDR16,
> + IMAGE_REL_PPC_ADDR14,
> + IMAGE_REL_PPC_REL24,
> + IMAGE_REL_PPC_REL14,
> + IMAGE_REL_PPC_ADDR32N,
> + IMAGE_REL_PPC_SECREL,
> + IMAGE_REL_PPC_SECTION,
> + IMAGE_REL_PPC_SECREL16,
> + IMAGE_REL_PPC_REFHI,
> + IMAGE_REL_PPC_REFLO,
> + IMAGE_REL_PPC_PAIR,
> + IMAGE_REL_PPC_SECRELLO,
> + IMAGE_REL_PPC_GPREL,
> + IMAGE_REL_PPC_TOKEN,
> +};
> +
> +enum x86_coff_reloc_type {
> + IMAGE_REL_I386_ABSOLUTE,
> + IMAGE_REL_I386_DIR16,
> + IMAGE_REL_I386_REL16,
> + IMAGE_REL_I386_DIR32,
> + IMAGE_REL_I386_DIR32NB,
> + IMAGE_REL_I386_SEG12,
> + IMAGE_REL_I386_SECTION,
> + IMAGE_REL_I386_SECREL,
> + IMAGE_REL_I386_TOKEN,
> + IMAGE_REL_I386_SECREL7,
> + IMAGE_REL_I386_REL32,
> +};
> +
> +enum ia64_coff_reloc_type {
> + IMAGE_REL_IA64_ABSOLUTE,
> + IMAGE_REL_IA64_IMM14,
> + IMAGE_REL_IA64_IMM22,
> + IMAGE_REL_IA64_IMM64,
> + IMAGE_REL_IA64_DIR32,
> + IMAGE_REL_IA64_DIR64,
> + IMAGE_REL_IA64_PCREL21B,
> + IMAGE_REL_IA64_PCREL21M,
> + IMAGE_REL_IA64_PCREL21F,
> + IMAGE_REL_IA64_GPREL22,
> + IMAGE_REL_IA64_LTOFF22,
> + IMAGE_REL_IA64_SECTION,
> + IMAGE_REL_IA64_SECREL22,
> + IMAGE_REL_IA64_SECREL64I,
> + IMAGE_REL_IA64_SECREL32,
> + IMAGE_REL_IA64_DIR32NB,
> + IMAGE_REL_IA64_SREL14,
> + IMAGE_REL_IA64_SREL22,
> + IMAGE_REL_IA64_SREL32,
> + IMAGE_REL_IA64_UREL32,
> + IMAGE_REL_IA64_PCREL60X,
> + IMAGE_REL_IA64_PCREL60B,
> + IMAGE_REL_IA64_PCREL60F,
> + IMAGE_REL_IA64_PCREL60I,
> + IMAGE_REL_IA64_PCREL60M,
> + IMAGE_REL_IA64_IMMGPREL6,
> + IMAGE_REL_IA64_TOKEN,
> + IMAGE_REL_IA64_GPREL32,
> + IMAGE_REL_IA64_ADDEND,
> +};
All sorts of relocation types, but none for RISC-V? Are we going to need
any of this?
> +struct coff_reloc {
> + uint32_t virtual_address;
> + uint32_t symbol_table_index;
> +
> + union {
> + enum x64_coff_reloc_type x64_type;
> + enum arm_coff_reloc_type arm_type;
> + enum sh_coff_reloc_type sh_type;
> + enum ppc_coff_reloc_type ppc_type;
> + enum x86_coff_reloc_type x86_type;
> + enum ia64_coff_reloc_type ia64_type;
> + uint16_t data;
> + };
> +};
> +
> +/*
> + * Definitions for the contents of the certs data block
> + */
> +#define WIN_CERT_TYPE_PKCS_SIGNED_DATA 0x0002
> +#define WIN_CERT_TYPE_EFI_OKCS115 0x0EF0
> +#define WIN_CERT_TYPE_EFI_GUID 0x0EF1
> +
> +#define WIN_CERT_REVISION_1_0 0x0100
> +#define WIN_CERT_REVISION_2_0 0x0200
> +
> +struct win_certificate {
> + uint32_t length;
> + uint16_t revision;
> + uint16_t cert_type;
> +};
Or any of this?
Jan
On Wed, Jul 3, 2024 at 5:55 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 03.07.2024 02:04, Milan Djokic wrote:
> > Added riscv image header to enable boot from second stage bootloaders (e.g. uboot. Image header defined in riscv-image-header.h)
> > Additionally, RISC-V xen image is extended with PE/COFF headers, introducing EFI application format.
> > PE/COFF 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.
> > Note that this patch represents initial EFI application format support (image
> > contains EFI application header embeded into binary when CONFIG_RISCV_EFI
> > is enabled). For full EFI application Xen support, boot/runtime services
> > and system table handling are yet to be implemented.
>
> So, first: Please Cc all applicable maintainers. It would probably be prudent
> to also Cc Oleksii, who's doing most of the RISC-V work now (but Oleksii,
> please correct me if you don't want to be Cc-ed).
>
Sure, we'll make sure to keep Oleksii in CC
> With Oleksii in the audience, second: I tink I've seen that in binutils work
> is being done to actually allow to create EFI applications "properly" for
> RISC-V. Was it firmly determined that you/we do not want to go that route?
>
We'll wait for Oleksii opinion on this one. I assume that there's no
reason to send updated versions
of our patch until it's cleared out if this feature is needed upstream.
> > --- a/xen/arch/riscv/Kconfig
> > +++ b/xen/arch/riscv/Kconfig
> > @@ -9,6 +9,16 @@ config ARCH_DEFCONFIG
> > string
> > default "arch/riscv/configs/tiny64_defconfig"
> >
> > +config RISCV_EFI
> > + bool "UEFI boot service support"
> > + depends on RISCV_64 && RISCV_ISA_C
> > + 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. Currently, only EFI PE/COFF application
> > + header is included into RISC-V image. Boot/Runtime services as part
> > + of EFI application stub are yet to be implemented.
>
> The first sentence in particular worries me. What it says is basically all
> taken back by what follows. I think this help text wants reducing to a
> minimum, andthen wants replacing once proper EFI support is in place.
>
I agree, we'll rephrase this description to only short description of
what's included so far
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/riscv-image-header.h
> > @@ -0,0 +1,57 @@
> > +#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_LE 0
> > +#define RISCV_IMAGE_FLAG_BE 1
> > +
> > +#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_LE
> > +
> > +#define __HEAD_FLAG(field) \
> > + (__HEAD_FLAG_##field << RISCV_IMAGE_FLAG_##field##_SHIFT)
>
> Nit: Generally we treat ## as a binary operator, too. Which means it also
> wants to be surrounded by blanks.
>
> > +#define __HEAD_FLAGS (__HEAD_FLAG(BE))
>
> Nit: The outer pair of parentheses isn't needed here, is it?
>
> > +#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: Please parenthesize the << against the |.
>
We'll fix formatting for this section
> > --- /dev/null
> > +++ b/xen/arch/riscv/riscv64/efi-header.S
> > @@ -0,0 +1,99 @@
> > +#include <efi/pe.h>
> > +#include <xen/sizes.h>
> > +
> > + .macro __EFI_PE_HEADER
> > + .long PE_MAGIC
> > +coff_header:
> > +#ifdef CONFIG_RISCV_64
> > + .short IMAGE_FILE_MACHINE_RISCV64 /* Machine */
> > +#else
> > + .short IMAGE_FILE_MACHINE_RISCV32 /* Machine */
> > +#endif
> > + .short section_count /* NumberOfSections */
> > + .long 0 /* TimeDateStamp */
> > + .long 0 /* PointerToSymbolTable */
> > + .long 0 /* NumberOfSymbols */
> > + .short section_table - optional_header /* SizeOfOptionalHeader */
> > + .short IMAGE_FILE_DEBUG_STRIPPED | \
> > + IMAGE_FILE_EXECUTABLE_IMAGE | \
> > + IMAGE_FILE_LINE_NUMS_STRIPPED /* Characteristics */
> > +
> > +optional_header:
> > +#ifdef CONFIG_RISCV_64
> > + .short PE_OPT_MAGIC_PE32PLUS /* PE32+ format */
> > +#else
> > + .short PE_OPT_MAGIC_PE32 /* PE32 format */
> > +#endif
> > + .byte 0x02 /* MajorLinkerVersion */
> > + .byte 0x14 /* MinorLinkerVersion */
> > + .long _end - xen_start /* SizeOfCode */
> > + .long 0 /* SizeOfInitializedData */
> > + .long 0 /* SizeOfUninitializedData */
> > + .long 0 /* AddressOfEntryPoint */
> > + .long xen_start - _start /* BaseOfCode */
> > +#ifdef CONFIG_RISCV_32
> > + .long _end - _start /* BaseOfData */
> > +#endif
>
> As requested before, the decision to represent all of .text/.data/.bss
> as code (with no data at all) wants explaining in a (possibly brief)
> comment.
>
We're not using .data and .bss for risc-v, since all "data" is part of
the PE header and embedded directly into section.text.header. Of
course, if at some point the program is updated with actual non-PE
header data, initialized/uninitialized data size members should be
updated. We'll clarify this part in code.
> > +extra_header_fields:
> > + .quad 0 /* ImageBase */
> > + .long PECOFF_SECTION_ALIGNMENT /* SectionAlignment */
> > + .long PECOFF_FILE_ALIGNMENT /* FileAlignment */
> > + .short 0 /* MajorOperatingSystemVersion */
> > + .short 0 /* MinorOperatingSystemVersion */
> > + .short LINUX_EFISTUB_MAJOR_VERSION /* MajorImageVersion */
> > + .short LINUX_EFISTUB_MINOR_VERSION /* MinorImageVersion */
> > + .short 0 /* MajorSubsystemVersion */
> > + .short 0 /* MinorSubsystemVersion */
> > + .long 0 /* Win32VersionValue */
> > + .long _end - _start /* SizeOfImage */
> > +
> > + /* Everything before the xen image is considered part of the header */
> > + .long xen_start - _start /* SizeOfHeaders */
> > + .long 0 /* CheckSum */
> > + .short IMAGE_SUBSYSTEM_EFI_APPLICATION /* Subsystem */
> > + .short 0 /* DllCharacteristics */
> > + .quad 0 /* SizeOfStackReserve */
> > + .quad 0 /* SizeOfStackCommit */
> > + .quad 0 /* SizeOfHeapReserve */
> > + .quad 0 /* SizeOfHeapCommit */
> > + .long 0 /* LoaderFlags */
> > + .long (section_table - .) / 8 /* NumberOfRvaAndSizes */
> > + /*
> > + * Data directories are not used in this case, therefore not defined to reduce header size.
> > + */
> > +
> > + /* Section table */
> > +section_table:
> > + /* Currently code/data sections are not used since EFI stub implementation is not yet finalized */
> > + .ascii ".text\0\0\0"
> > + .long 0 /* VirtualSize */
> > + .long 0 /* VirtualAddress */
> > + .long 0 /* SizeOfRawData */
> > + .long 0 /* PointerToRawData */
> > + .long 0 /* PointerToRelocations */
> > + .long 0 /* PointerToLineNumbers */
> > + .short 0 /* NumberOfRelocations */
> > + .short 0 /* NumberOfLineNumbers */
> > + .long IMAGE_SCN_CNT_CODE | \
> > + IMAGE_SCN_MEM_READ | \
> > + IMAGE_SCN_MEM_EXECUTE /* Characteristics */
> > +
> > + .ascii ".data\0\0\0"
> > + .long 0 /* VirtualSize */
> > + .long 0 /* VirtualAddress */
> > + .long 0 /* SizeOfRawData */
> > + .long 0 /* PointerToRawData */
> > + .long 0 /* PointerToRelocations */
> > + .long 0 /* PointerToLineNumbers */
> > + .short 0 /* NumberOfRelocations */
> > + .short 0 /* NumberOfLineNumbers */
> > + .long IMAGE_SCN_CNT_INITIALIZED_DATA | \
> > + IMAGE_SCN_MEM_READ | \
> > + IMAGE_SCN_MEM_WRITE /* Characteristics */
> > +
> > + .set section_count, (. - section_table) / 40
> > +
> > + .balign 0x1000
> > +efi_header_end:
> > + .endm
> > \ No newline at end of file
>
> Please take care of this.
>
> > --- a/xen/arch/riscv/riscv64/head.S
> > +++ b/xen/arch/riscv/riscv64/head.S
> > @@ -1,14 +1,54 @@
> > #include <asm/asm.h>
> > #include <asm/riscv_encoding.h>
> > +#include <asm/riscv-image-header.h>
> > +#ifdef CONFIG_RISCV_EFI
> > +#include "efi-header.S"
> > +#endif
> >
> > .section .text.header, "ax", %progbits
> > -
> > /*
>
> I see no reason for removing this blank line.
>
> > * OpenSBI pass to start():
> > * a0 -> hart_id ( bootcpu_id )
> > - * a1 -> dtb_base
> > + * a1 -> dtb_base
> > */
> > FUNC(start)
> > +/* Image header expected by second stage bootloaders (format defined in asm/riscv-image-header.h) */
>
> Nit: Overlong line (and not the only one).
>
We'll fix formatting for these parts
> > +#ifdef CONFIG_RISCV_EFI
> > + /*
> > + * This instruction decodes to "MZ" ASCII required by UEFI.
> > + */
> > + c.li s4,-13
> > + c.j xen_start
> > +#else
> > + /* jump to start kernel */
> > + jal xen_start
>
> JAL, not J? Why?
>
We use jal explicitly here to highlight that we want to occupy 32 bits
in order to align with header format (and EFI case where we use two
16-bits instructions). Although it will also work by using "j"
directly, it could be implicitly compressed to 16 bits (if C extension
is available) which would make header layout less obvious imo.
> > +#endif
> > + .balign 8
>
> This won't do what you want unless "start" itself is also suitably aligned.
> It'll be as long as it's first in the section, but better make such explicit.
>
I understand, we'll also explicitly align "start"
> > +#ifdef CONFIG_RISCV_64
> > + /* Image load offset(2MB) from start of RAM */
> > + .quad 0x200000
> > +#else
> > + /* Image load offset(4MB) from start of RAM */
> > + .quad 0x400000
> > +#endif
>
> What these constants derive from? I expect they aren't really "magic" .
>
These offsets are commonly used for RISCV SOCs. In general, these can
have different values, depending on memory layout, but in order to be
compatible with
linux bootloaders we just followed the common practice.
> > + .quad _end - _start /* Effective Image size */
> > + .quad __HEAD_FLAGS
> > + .long RISCV_HEADER_VERSION
>
> In the C struct this is a 64-bit field. Why .long here? Or perhaps the C
> struct is wrong, actually also leaving unspecified padding there after
> ...
>
> > + .long 0 /* reserved */
>
> ... this field then.
>
> > + .quad 0 /* reserved */
> > + .ascii RISCV_IMAGE_MAGIC /* Magic number (RISC-V specific; deprecated) */
> > + .balign 4
>
> RISCV_IMAGE_MAGIC is already nul-padded to 8 bytes. I therefore find
> the .balign here somewhat confusing.
>
Yes, regarding RISCV_HEADER_VERSION, the C struct field has the wrong
type (should be uint_32 instead of uint_64). We'll fix this one. We'll
also remove explicit .balign, since it's not needed.
> > + .ascii RISCV_IMAGE_MAGIC2 /* Magic number 2 (to match the ARM64 'magic' field pos) */
> > +#ifdef CONFIG_RISCV_EFI
> > + .long pe_head_start - _start /* reserved (PE COFF offset) */
> > +pe_head_start:
> > +
> > + __EFI_PE_HEADER
>
> Using a macro for a single, simple purpose is somewhat unexpected.
> Can't you simply
>
> #include "efi-header.S"
>
> here? That would also make it more straightforward to find the use /
> purpose of that file.
>
Leftover from the previous version when we had everything embedded in
head.S. We'll remove the macro and use explicit include here.
> > +#else
> > + .long 0 /* 0 means no PE header. */
> > +#endif
> > +
> > +xen_start:
> > /* Mask all interrupts */
> > csrw CSR_SIE, zero
> >
> > @@ -60,6 +100,7 @@ FUNC(start)
> > mv a1, s1
> >
> > tail start_xen
> > +
> > END(start)
> >
> > .section .text, "ax", %progbits
>
> What is this hunk about?
>
Not sure which part of the code you refer to here. This section is
part of the original riscv implementation. We only grouped this
section under the new, xen_start label since we have different
handling for EFI and ELF formats in the beginning of "start" function
before jumping to xen_start.
> > --- a/xen/arch/riscv/xen.lds.S
> > +++ b/xen/arch/riscv/xen.lds.S
> > @@ -12,6 +12,9 @@ PHDRS
> > #endif
> > }
> >
> > +PECOFF_SECTION_ALIGNMENT = 0x1000;
> > +PECOFF_FILE_ALIGNMENT = 0x200;
>
> What are these about? I see you use them in efi-header.S, but why do
> they need supplying from the linker script?
>
Actually there is no specific reason to supply these constants from
linker scripts, we can embed those as macros directly in code.
> > --- /dev/null
> > +++ b/xen/include/efi/pe.h
> > @@ -0,0 +1,458 @@
> > +#ifndef __PE_H
>
> This probably wants (needs?) to gain an SPDX line.
>
Yes, it was originally taken over from the linux kernel (with some
modifications from our side), but we omitted the SPDX line by mistake.
> > +#define __PE_H
> > +
> > +/*
> > + * Linux EFI stub v1.0 adds the following functionality:
> > + * - Loading initrd from the LINUX_EFI_INITRD_MEDIA_GUID device path,
> > + * - Loading/starting the kernel from firmware that targets a different
> > + * machine type, via the entrypoint exposed in the .compat PE/COFF section.
> > + *
> > + * The recommended way of loading and starting v1.0 or later kernels is to use
> > + * the LoadImage() and StartImage() EFI boot services, and expose the initrd
> > + * via the LINUX_EFI_INITRD_MEDIA_GUID device path.
> > + *
> > + * Versions older than v1.0 support initrd loading via the image load options
> > + * (using initrd=, limited to the volume from which the kernel itself was
> > + * loaded), or via arch specific means (bootparams, DT, etc).
> > + *
> > + * On x86, LoadImage() and StartImage() can be omitted if the EFI handover
> > + * protocol is implemented, which can be inferred from the version,
> > + * handover_offset and xloadflags fields in the bootparams structure.
> > + */
> > +#define LINUX_EFISTUB_MAJOR_VERSION 0x1
> > +#define LINUX_EFISTUB_MINOR_VERSION 0x0
> > +
> > +#define MZ_MAGIC 0x5a4d /* "MZ" */
> > +
> > +#define PE_MAGIC 0x00004550 /* "PE\0\0" */
> > +#define PE_OPT_MAGIC_PE32 0x010b
> > +#define PE_OPT_MAGIC_PE32_ROM 0x0107
> > +#define PE_OPT_MAGIC_PE32PLUS 0x020b
> > +
> > +/* machine type */
> > +#define IMAGE_FILE_MACHINE_UNKNOWN 0x0000
> > +#define IMAGE_FILE_MACHINE_AM33 0x01d3
> > +#define IMAGE_FILE_MACHINE_AMD64 0x8664
> > +#define IMAGE_FILE_MACHINE_ARM 0x01c0
> > +#define IMAGE_FILE_MACHINE_ARMV7 0x01c4
> > +#define IMAGE_FILE_MACHINE_ARM64 0xaa64
> > +#define IMAGE_FILE_MACHINE_EBC 0x0ebc
> > +#define IMAGE_FILE_MACHINE_I386 0x014c
> > +#define IMAGE_FILE_MACHINE_IA64 0x0200
> > +#define IMAGE_FILE_MACHINE_M32R 0x9041
> > +#define IMAGE_FILE_MACHINE_MIPS16 0x0266
> > +#define IMAGE_FILE_MACHINE_MIPSFPU 0x0366
> > +#define IMAGE_FILE_MACHINE_MIPSFPU16 0x0466
> > +#define IMAGE_FILE_MACHINE_POWERPC 0x01f0
> > +#define IMAGE_FILE_MACHINE_POWERPCFP 0x01f1
> > +#define IMAGE_FILE_MACHINE_R4000 0x0166
> > +#define IMAGE_FILE_MACHINE_RISCV32 0x5032
> > +#define IMAGE_FILE_MACHINE_RISCV64 0x5064
> > +#define IMAGE_FILE_MACHINE_RISCV128 0x5128
> > +#define IMAGE_FILE_MACHINE_SH3 0x01a2
> > +#define IMAGE_FILE_MACHINE_SH3DSP 0x01a3
> > +#define IMAGE_FILE_MACHINE_SH3E 0x01a4
> > +#define IMAGE_FILE_MACHINE_SH4 0x01a6
> > +#define IMAGE_FILE_MACHINE_SH5 0x01a8
> > +#define IMAGE_FILE_MACHINE_THUMB 0x01c2
> > +#define IMAGE_FILE_MACHINE_WCEMIPSV2 0x0169
> > +
> > +/* flags */
> > +#define IMAGE_FILE_RELOCS_STRIPPED 0x0001
> > +#define IMAGE_FILE_EXECUTABLE_IMAGE 0x0002
> > +#define IMAGE_FILE_LINE_NUMS_STRIPPED 0x0004
> > +#define IMAGE_FILE_LOCAL_SYMS_STRIPPED 0x0008
> > +#define IMAGE_FILE_AGGRESSIVE_WS_TRIM 0x0010
> > +#define IMAGE_FILE_LARGE_ADDRESS_AWARE 0x0020
> > +#define IMAGE_FILE_16BIT_MACHINE 0x0040
> > +#define IMAGE_FILE_BYTES_REVERSED_LO 0x0080
> > +#define IMAGE_FILE_32BIT_MACHINE 0x0100
> > +#define IMAGE_FILE_DEBUG_STRIPPED 0x0200
> > +#define IMAGE_FILE_REMOVABLE_RUN_FROM_SWAP 0x0400
> > +#define IMAGE_FILE_NET_RUN_FROM_SWAP 0x0800
> > +#define IMAGE_FILE_SYSTEM 0x1000
> > +#define IMAGE_FILE_DLL 0x2000
> > +#define IMAGE_FILE_UP_SYSTEM_ONLY 0x4000
> > +#define IMAGE_FILE_BYTES_REVERSED_HI 0x8000
> > +
> > +#define IMAGE_FILE_OPT_ROM_MAGIC 0x107
> > +#define IMAGE_FILE_OPT_PE32_MAGIC 0x10b
> > +#define IMAGE_FILE_OPT_PE32_PLUS_MAGIC 0x20b
> > +
> > +#define IMAGE_SUBSYSTEM_UNKNOWN 0
> > +#define IMAGE_SUBSYSTEM_NATIVE 1
> > +#define IMAGE_SUBSYSTEM_WINDOWS_GUI 2
> > +#define IMAGE_SUBSYSTEM_WINDOWS_CUI 3
> > +#define IMAGE_SUBSYSTEM_POSIX_CUI 7
> > +#define IMAGE_SUBSYSTEM_WINDOWS_CE_GUI 9
> > +#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10
> > +#define IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER 11
> > +#define IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER 12
> > +#define IMAGE_SUBSYSTEM_EFI_ROM_IMAGE 13
> > +#define IMAGE_SUBSYSTEM_XBOX 14
> > +
> > +#define IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE 0x0040
> > +#define IMAGE_DLL_CHARACTERISTICS_FORCE_INTEGRITY 0x0080
> > +#define IMAGE_DLL_CHARACTERISTICS_NX_COMPAT 0x0100
> > +#define IMAGE_DLLCHARACTERISTICS_NO_ISOLATION 0x0200
> > +#define IMAGE_DLLCHARACTERISTICS_NO_SEH 0x0400
> > +#define IMAGE_DLLCHARACTERISTICS_NO_BIND 0x0800
> > +#define IMAGE_DLLCHARACTERISTICS_WDM_DRIVER 0x2000
> > +#define IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE 0x8000
> > +
> > +/* they actually defined 0x00000000 as well, but I think we'll skip that one. */
> > +#define IMAGE_SCN_RESERVED_0 0x00000001
> > +#define IMAGE_SCN_RESERVED_1 0x00000002
> > +#define IMAGE_SCN_RESERVED_2 0x00000004
> > +#define IMAGE_SCN_TYPE_NO_PAD 0x00000008 /* don't pad - obsolete */
> > +#define IMAGE_SCN_RESERVED_3 0x00000010
> > +#define IMAGE_SCN_CNT_CODE 0x00000020 /* .text */
> > +#define IMAGE_SCN_CNT_INITIALIZED_DATA 0x00000040 /* .data */
> > +#define IMAGE_SCN_CNT_UNINITIALIZED_DATA 0x00000080 /* .bss */
> > +#define IMAGE_SCN_LNK_OTHER 0x00000100 /* reserved */
> > +#define IMAGE_SCN_LNK_INFO 0x00000200 /* .drectve comments */
> > +#define IMAGE_SCN_RESERVED_4 0x00000400
> > +#define IMAGE_SCN_LNK_REMOVE 0x00000800 /* .o only - scn to be rm'd*/
> > +#define IMAGE_SCN_LNK_COMDAT 0x00001000 /* .o only - COMDAT data */
> > +#define IMAGE_SCN_RESERVED_5 0x00002000 /* spec omits this */
> > +#define IMAGE_SCN_RESERVED_6 0x00004000 /* spec omits this */
> > +#define IMAGE_SCN_GPREL 0x00008000 /* global pointer referenced data */
> > +/* spec lists 0x20000 twice, I suspect they meant 0x10000 for one of them */
> > +#define IMAGE_SCN_MEM_PURGEABLE 0x00010000 /* reserved for "future" use */
> > +#define IMAGE_SCN_16BIT 0x00020000 /* reserved for "future" use */
> > +#define IMAGE_SCN_LOCKED 0x00040000 /* reserved for "future" use */
> > +#define IMAGE_SCN_PRELOAD 0x00080000 /* reserved for "future" use */
> > +/* and here they just stuck a 1-byte integer in the middle of a bitfield */
> > +#define IMAGE_SCN_ALIGN_1BYTES 0x00100000 /* it does what it says on the box */
> > +#define IMAGE_SCN_ALIGN_2BYTES 0x00200000
> > +#define IMAGE_SCN_ALIGN_4BYTES 0x00300000
> > +#define IMAGE_SCN_ALIGN_8BYTES 0x00400000
> > +#define IMAGE_SCN_ALIGN_16BYTES 0x00500000
> > +#define IMAGE_SCN_ALIGN_32BYTES 0x00600000
> > +#define IMAGE_SCN_ALIGN_64BYTES 0x00700000
> > +#define IMAGE_SCN_ALIGN_128BYTES 0x00800000
> > +#define IMAGE_SCN_ALIGN_256BYTES 0x00900000
> > +#define IMAGE_SCN_ALIGN_512BYTES 0x00a00000
> > +#define IMAGE_SCN_ALIGN_1024BYTES 0x00b00000
> > +#define IMAGE_SCN_ALIGN_2048BYTES 0x00c00000
> > +#define IMAGE_SCN_ALIGN_4096BYTES 0x00d00000
> > +#define IMAGE_SCN_ALIGN_8192BYTES 0x00e00000
> > +#define IMAGE_SCN_LNK_NRELOC_OVFL 0x01000000 /* extended relocations */
> > +#define IMAGE_SCN_MEM_DISCARDABLE 0x02000000 /* scn can be discarded */
> > +#define IMAGE_SCN_MEM_NOT_CACHED 0x04000000 /* cannot be cached */
> > +#define IMAGE_SCN_MEM_NOT_PAGED 0x08000000 /* not pageable */
> > +#define IMAGE_SCN_MEM_SHARED 0x10000000 /* can be shared */
> > +#define IMAGE_SCN_MEM_EXECUTE 0x20000000 /* can be executed as code */
> > +#define IMAGE_SCN_MEM_READ 0x40000000 /* readable */
> > +#define IMAGE_SCN_MEM_WRITE 0x80000000 /* writeable */
> > +
> > +#define IMAGE_DEBUG_TYPE_CODEVIEW 2
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +struct mz_hdr {
> > + uint16_t magic; /* MZ_MAGIC */
> > + uint16_t lbsize; /* size of last used block */
> > + uint16_t blocks; /* pages in file, 0x3 */
> > + uint16_t relocs; /* relocations */
> > + uint16_t hdrsize; /* header size in "paragraphs" */
> > + uint16_t min_extra_pps; /* .bss */
> > + uint16_t max_extra_pps; /* runtime limit for the arena size */
> > + uint16_t ss; /* relative stack segment */
> > + uint16_t sp; /* initial %sp register */
> > + uint16_t checksum; /* word checksum */
> > + uint16_t ip; /* initial %ip register */
> > + uint16_t cs; /* initial %cs relative to load segment */
> > + uint16_t reloc_table_offset; /* offset of the first relocation */
> > + uint16_t overlay_num; /* overlay number. set to 0. */
> > + uint16_t reserved0[4]; /* reserved */
> > + uint16_t oem_id; /* oem identifier */
> > + uint16_t oem_info; /* oem specific */
> > + uint16_t reserved1[10]; /* reserved */
> > + uint32_t peaddr; /* address of pe header */
> > + char message[]; /* message to print */
> > +};
>
> We already have an instance of this struct in common/efi/pe.c. I think
> it wouldn't be very desirable to have two (different) instances.
>
> > +struct mz_reloc {
> > + uint16_t offset;
> > + uint16_t segment;
> > +};
>
> We aren't going to need this anywhere, are we?
>
> > +struct pe_hdr {
> > + uint32_t magic; /* PE magic */
> > + uint16_t machine; /* machine type */
> > + uint16_t sections; /* number of sections */
> > + uint32_t timestamp; /* time_t */
> > + uint32_t symbol_table; /* symbol table offset */
> > + uint32_t symbols; /* number of symbols */
> > + uint16_t opt_hdr_size; /* size of optional header */
> > + uint16_t flags; /* flags */
> > +};
>
> And again another (different) instance of this and further struct-s
> already exists. Same for the section header further down.
>
> > +/* the fact that pe32 isn't padded where pe32+ is 64-bit means union won't
> > + * work right. vomit. */
>
> Noticing here in particular, but being an issue elsewhere as well:
> Unless this file is to be a verbatim copy taken from somewhere (in
> which case it should probably be introduced in a separate commit
> with an Origin: tag), comments want to adhere of ./CODING_STYLE.
>
> > +struct pe32_opt_hdr {
> > + /* "standard" header */
> > + uint16_t magic; /* file type */
> > + uint8_t ld_major; /* linker major version */
> > + uint8_t ld_minor; /* linker minor version */
> > + uint32_t text_size; /* size of text section(s) */
> > + uint32_t data_size; /* size of data section(s) */
> > + uint32_t bss_size; /* size of bss section(s) */
> > + uint32_t entry_point; /* file offset of entry point */
> > + uint32_t code_base; /* relative code addr in ram */
> > + uint32_t data_base; /* relative data addr in ram */
> > + /* "windows" header */
> > + uint32_t image_base; /* preferred load address */
> > + uint32_t section_align; /* alignment in bytes */
> > + uint32_t file_align; /* file alignment in bytes */
> > + uint16_t os_major; /* major OS version */
> > + uint16_t os_minor; /* minor OS version */
> > + uint16_t image_major; /* major image version */
> > + uint16_t image_minor; /* minor image version */
> > + uint16_t subsys_major; /* major subsystem version */
> > + uint16_t subsys_minor; /* minor subsystem version */
> > + uint32_t win32_version; /* reserved, must be 0 */
> > + uint32_t image_size; /* image size */
> > + uint32_t header_size; /* header size rounded up to file_align */
> > + uint32_t csum; /* checksum */
> > + uint16_t subsys; /* subsystem */
> > + uint16_t dll_flags; /* executable characteristics */
> > + uint32_t stack_size_req; /* amt of stack requested */
> > + uint32_t stack_size; /* amt of stack required */
> > + uint32_t heap_size_req; /* amt of heap requested */
> > + uint32_t heap_size; /* amt of heap required */
> > + uint32_t loader_flags; /* reserved, must be 0 */
> > + uint32_t data_dirs; /* number of data dir entries */
> > +};
> > +
> > +struct pe32plus_opt_hdr {
> > + uint16_t magic; /* file type */
> > + uint8_t ld_major; /* linker major version */
> > + uint8_t ld_minor; /* linker minor version */
> > + uint32_t text_size; /* size of text section(s) */
> > + uint32_t data_size; /* size of data section(s) */
> > + uint32_t bss_size; /* size of bss section(s) */
> > + uint32_t entry_point; /* file offset of entry point */
> > + uint32_t code_base; /* relative code addr in ram */
> > + /* "windows" header */
> > + uint64_t image_base; /* preferred load address */
> > + uint32_t section_align; /* alignment in bytes */
> > + uint32_t file_align; /* file alignment in bytes */
> > + uint16_t os_major; /* major OS version */
> > + uint16_t os_minor; /* minor OS version */
> > + uint16_t image_major; /* major image version */
> > + uint16_t image_minor; /* minor image version */
> > + uint16_t subsys_major; /* major subsystem version */
> > + uint16_t subsys_minor; /* minor subsystem version */
> > + uint32_t win32_version; /* reserved, must be 0 */
> > + uint32_t image_size; /* image size */
> > + uint32_t header_size; /* header size rounded up to file_align */
> > + uint32_t csum; /* checksum */
> > + uint16_t subsys; /* subsystem */
> > + uint16_t dll_flags; /* executable characteristics */
> > + uint64_t stack_size_req; /* amt of stack requested */
> > + uint64_t stack_size; /* amt of stack required */
> > + uint64_t heap_size_req; /* amt of heap requested */
> > + uint64_t heap_size; /* amt of heap required */
> > + uint32_t loader_flags; /* reserved, must be 0 */
> > + uint32_t data_dirs; /* number of data dir entries */
> > +};
> > +
> > +struct data_dirent {
> > + uint32_t virtual_address; /* relative to load address */
> > + uint32_t size;
> > +};
>
> Will we need this and ...
>
> > +struct data_directory {
> > + struct data_dirent exports; /* .edata */
> > + struct data_dirent imports; /* .idata */
> > + struct data_dirent resources; /* .rsrc */
> > + struct data_dirent exceptions; /* .pdata */
> > + struct data_dirent certs; /* certs */
> > + struct data_dirent base_relocations; /* .reloc */
> > + struct data_dirent debug; /* .debug */
> > + struct data_dirent arch; /* reservered */
> > + struct data_dirent global_ptr; /* global pointer reg. Size=0 */
> > + struct data_dirent tls; /* .tls */
> > + struct data_dirent load_config; /* load configuration structure */
> > + struct data_dirent bound_imports; /* no idea */
> > + struct data_dirent import_addrs; /* import address table */
> > + struct data_dirent delay_imports; /* delay-load import table */
> > + struct data_dirent clr_runtime_hdr; /* .cor (object only) */
> > + struct data_dirent reserved;
> > +};
>
> ... this?
>
> > +struct section_header {
> > + char name[8]; /* name or string tbl offset */
> > + uint32_t virtual_size; /* size of loaded section in ram */
> > + uint32_t virtual_address; /* relative virtual address */
> > + uint32_t raw_data_size; /* size of the section */
> > + uint32_t data_addr; /* file pointer to first page of sec */
> > + uint32_t relocs; /* file pointer to relocation entries */
> > + uint32_t line_numbers; /* line numbers */
> > + uint16_t num_relocs; /* number of relocations */
> > + uint16_t num_lin_numbers; /* COFF line count. */
> > + uint32_t flags;
> > +};
> > +
> > +enum x64_coff_reloc_type {
> > + IMAGE_REL_AMD64_ABSOLUTE = 0,
> > + IMAGE_REL_AMD64_ADDR64,
> > + IMAGE_REL_AMD64_ADDR32,
> > + IMAGE_REL_AMD64_ADDR32N,
> > + IMAGE_REL_AMD64_REL32,
> > + IMAGE_REL_AMD64_REL32_1,
> > + IMAGE_REL_AMD64_REL32_2,
> > + IMAGE_REL_AMD64_REL32_3,
> > + IMAGE_REL_AMD64_REL32_4,
> > + IMAGE_REL_AMD64_REL32_5,
> > + IMAGE_REL_AMD64_SECTION,
> > + IMAGE_REL_AMD64_SECREL,
> > + IMAGE_REL_AMD64_SECREL7,
> > + IMAGE_REL_AMD64_TOKEN,
> > + IMAGE_REL_AMD64_SREL32,
> > + IMAGE_REL_AMD64_PAIR,
> > + IMAGE_REL_AMD64_SSPAN32,
> > +};
> > +
> > +enum arm_coff_reloc_type {
> > + IMAGE_REL_ARM_ABSOLUTE,
> > + IMAGE_REL_ARM_ADDR32,
> > + IMAGE_REL_ARM_ADDR32N,
> > + IMAGE_REL_ARM_BRANCH2,
> > + IMAGE_REL_ARM_BRANCH1,
> > + IMAGE_REL_ARM_SECTION,
> > + IMAGE_REL_ARM_SECREL,
> > +};
> > +
> > +enum sh_coff_reloc_type {
> > + IMAGE_REL_SH3_ABSOLUTE,
> > + IMAGE_REL_SH3_DIRECT16,
> > + IMAGE_REL_SH3_DIRECT32,
> > + IMAGE_REL_SH3_DIRECT8,
> > + IMAGE_REL_SH3_DIRECT8_WORD,
> > + IMAGE_REL_SH3_DIRECT8_LONG,
> > + IMAGE_REL_SH3_DIRECT4,
> > + IMAGE_REL_SH3_DIRECT4_WORD,
> > + IMAGE_REL_SH3_DIRECT4_LONG,
> > + IMAGE_REL_SH3_PCREL8_WORD,
> > + IMAGE_REL_SH3_PCREL8_LONG,
> > + IMAGE_REL_SH3_PCREL12_WORD,
> > + IMAGE_REL_SH3_STARTOF_SECTION,
> > + IMAGE_REL_SH3_SIZEOF_SECTION,
> > + IMAGE_REL_SH3_SECTION,
> > + IMAGE_REL_SH3_SECREL,
> > + IMAGE_REL_SH3_DIRECT32_NB,
> > + IMAGE_REL_SH3_GPREL4_LONG,
> > + IMAGE_REL_SH3_TOKEN,
> > + IMAGE_REL_SHM_PCRELPT,
> > + IMAGE_REL_SHM_REFLO,
> > + IMAGE_REL_SHM_REFHALF,
> > + IMAGE_REL_SHM_RELLO,
> > + IMAGE_REL_SHM_RELHALF,
> > + IMAGE_REL_SHM_PAIR,
> > + IMAGE_REL_SHM_NOMODE,
> > +};
> > +
> > +enum ppc_coff_reloc_type {
> > + IMAGE_REL_PPC_ABSOLUTE,
> > + IMAGE_REL_PPC_ADDR64,
> > + IMAGE_REL_PPC_ADDR32,
> > + IMAGE_REL_PPC_ADDR24,
> > + IMAGE_REL_PPC_ADDR16,
> > + IMAGE_REL_PPC_ADDR14,
> > + IMAGE_REL_PPC_REL24,
> > + IMAGE_REL_PPC_REL14,
> > + IMAGE_REL_PPC_ADDR32N,
> > + IMAGE_REL_PPC_SECREL,
> > + IMAGE_REL_PPC_SECTION,
> > + IMAGE_REL_PPC_SECREL16,
> > + IMAGE_REL_PPC_REFHI,
> > + IMAGE_REL_PPC_REFLO,
> > + IMAGE_REL_PPC_PAIR,
> > + IMAGE_REL_PPC_SECRELLO,
> > + IMAGE_REL_PPC_GPREL,
> > + IMAGE_REL_PPC_TOKEN,
> > +};
> > +
> > +enum x86_coff_reloc_type {
> > + IMAGE_REL_I386_ABSOLUTE,
> > + IMAGE_REL_I386_DIR16,
> > + IMAGE_REL_I386_REL16,
> > + IMAGE_REL_I386_DIR32,
> > + IMAGE_REL_I386_DIR32NB,
> > + IMAGE_REL_I386_SEG12,
> > + IMAGE_REL_I386_SECTION,
> > + IMAGE_REL_I386_SECREL,
> > + IMAGE_REL_I386_TOKEN,
> > + IMAGE_REL_I386_SECREL7,
> > + IMAGE_REL_I386_REL32,
> > +};
> > +
> > +enum ia64_coff_reloc_type {
> > + IMAGE_REL_IA64_ABSOLUTE,
> > + IMAGE_REL_IA64_IMM14,
> > + IMAGE_REL_IA64_IMM22,
> > + IMAGE_REL_IA64_IMM64,
> > + IMAGE_REL_IA64_DIR32,
> > + IMAGE_REL_IA64_DIR64,
> > + IMAGE_REL_IA64_PCREL21B,
> > + IMAGE_REL_IA64_PCREL21M,
> > + IMAGE_REL_IA64_PCREL21F,
> > + IMAGE_REL_IA64_GPREL22,
> > + IMAGE_REL_IA64_LTOFF22,
> > + IMAGE_REL_IA64_SECTION,
> > + IMAGE_REL_IA64_SECREL22,
> > + IMAGE_REL_IA64_SECREL64I,
> > + IMAGE_REL_IA64_SECREL32,
> > + IMAGE_REL_IA64_DIR32NB,
> > + IMAGE_REL_IA64_SREL14,
> > + IMAGE_REL_IA64_SREL22,
> > + IMAGE_REL_IA64_SREL32,
> > + IMAGE_REL_IA64_UREL32,
> > + IMAGE_REL_IA64_PCREL60X,
> > + IMAGE_REL_IA64_PCREL60B,
> > + IMAGE_REL_IA64_PCREL60F,
> > + IMAGE_REL_IA64_PCREL60I,
> > + IMAGE_REL_IA64_PCREL60M,
> > + IMAGE_REL_IA64_IMMGPREL6,
> > + IMAGE_REL_IA64_TOKEN,
> > + IMAGE_REL_IA64_GPREL32,
> > + IMAGE_REL_IA64_ADDEND,
> > +};
>
> All sorts of relocation types, but none for RISC-V? Are we going to need
> any of this?
>
> > +struct coff_reloc {
> > + uint32_t virtual_address;
> > + uint32_t symbol_table_index;
> > +
> > + union {
> > + enum x64_coff_reloc_type x64_type;
> > + enum arm_coff_reloc_type arm_type;
> > + enum sh_coff_reloc_type sh_type;
> > + enum ppc_coff_reloc_type ppc_type;
> > + enum x86_coff_reloc_type x86_type;
> > + enum ia64_coff_reloc_type ia64_type;
> > + uint16_t data;
> > + };
> > +};
> > +
> > +/*
> > + * Definitions for the contents of the certs data block
> > + */
> > +#define WIN_CERT_TYPE_PKCS_SIGNED_DATA 0x0002
> > +#define WIN_CERT_TYPE_EFI_OKCS115 0x0EF0
> > +#define WIN_CERT_TYPE_EFI_GUID 0x0EF1
> > +
> > +#define WIN_CERT_REVISION_1_0 0x0100
> > +#define WIN_CERT_REVISION_2_0 0x0200
> > +
> > +struct win_certificate {
> > + uint32_t length;
> > + uint16_t revision;
> > + uint16_t cert_type;
> > +};
>
> Or any of this?
>
Regarding pe.h file content, we wanted to keep it as generic as
possible (structures definition according to PE format which can be
used for multiple architectures). Specifically for RISC-V as you
noticed, we are not using lots of structures (data directories,
relocation structures, certificates, etc.). Therefore, we can reduce
it to only those used atm, but in that case we won't have a generic PE
header definition anymore. Regarding structures which are already
defined in common/efi/pe.c, meaning that with our change we have two
versions of same structures, we can remove those, but in that case it
could be confusing for someone who is trying to map fields from pe.h
to actual image header in assembly code. Summary I would keep this
header with its original content, but if you think that it contains
too much overhead we can reduce it to relevant structures only.
BR,
Milan
On 04.07.2024 19:21, Milan Đokić wrote:
> On Wed, Jul 3, 2024 at 5:55 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 03.07.2024 02:04, Milan Djokic wrote:
>>> +#ifdef CONFIG_RISCV_EFI
>>> + /*
>>> + * This instruction decodes to "MZ" ASCII required by UEFI.
>>> + */
>>> + c.li s4,-13
>>> + c.j xen_start
>>> +#else
>>> + /* jump to start kernel */
>>> + jal xen_start
>>
>> JAL, not J? Why?
>>
> We use jal explicitly here to highlight that we want to occupy 32 bits
> in order to align with header format (and EFI case where we use two
> 16-bits instructions). Although it will also work by using "j"
> directly, it could be implicitly compressed to 16 bits (if C extension
> is available) which would make header layout less obvious imo.
According to e.g. ...
>>> +#endif
>>> + .balign 8
>>
>> This won't do what you want unless "start" itself is also suitably aligned.
>> It'll be as long as it's first in the section, but better make such explicit.
>>
> I understand, we'll also explicitly align "start"
>
>>> +#ifdef CONFIG_RISCV_64
>>> + /* Image load offset(2MB) from start of RAM */
>>> + .quad 0x200000
>>> +#else
>>> + /* Image load offset(4MB) from start of RAM */
>>> + .quad 0x400000
>>> +#endif
... the #ifdef here, you aim at having code be suitable for RV32, too.
However, JAL has a compressed form there, so its use would make things
"less obvious" there as well. I'm inclined to say that since the
subsequent ".balign 8" adds padding NOPs anyway, there's no real
difference whether that adds 32 bits worth of NOPs or 48 bits. If you
really wanted to "hide" the difference, imo ".balign 4" would be the
way to go.
In any event, if there would still be a reason to stick to JAL, you'd
want to name the reason(s) in a code comment.
>> What these constants derive from? I expect they aren't really "magic" .
>>
> These offsets are commonly used for RISCV SOCs. In general, these can
> have different values, depending on memory layout, but in order to be
> compatible with
> linux bootloaders we just followed the common practice.
So entirely magic, really. Such imo needs commenting on.
>>> @@ -60,6 +100,7 @@ FUNC(start)
>>> mv a1, s1
>>>
>>> tail start_xen
>>> +
>>> END(start)
>>>
>>> .section .text, "ax", %progbits
>>
>> What is this hunk about?
>>
> Not sure which part of the code you refer to here. This section is
> part of the original riscv implementation. We only grouped this
> section under the new, xen_start label since we have different
> handling for EFI and ELF formats in the beginning of "start" function
> before jumping to xen_start.
"Hunk" is common term for a single piece of "diff" output, starting at
the @@ line. To word it differently: What reason is there to insert a
blank line in this very patch?
>>> --- /dev/null
>>> +++ b/xen/include/efi/pe.h
>>> @@ -0,0 +1,458 @@
>>> +#ifndef __PE_H
>>
>> This probably wants (needs?) to gain an SPDX line.
>>
> Yes, it was originally taken over from the linux kernel (with some
> modifications from our side), but we omitted the SPDX line by mistake.
>
>>> +#define __PE_H
>>> +
>>> +/*
>>> + * Linux EFI stub v1.0 adds the following functionality:
>>> + * - Loading initrd from the LINUX_EFI_INITRD_MEDIA_GUID device path,
>>> + * - Loading/starting the kernel from firmware that targets a different
>>> + * machine type, via the entrypoint exposed in the .compat PE/COFF section.
>>> + *
>>> + * The recommended way of loading and starting v1.0 or later kernels is to use
>>> + * the LoadImage() and StartImage() EFI boot services, and expose the initrd
>>> + * via the LINUX_EFI_INITRD_MEDIA_GUID device path.
>>> + *
>>> + * Versions older than v1.0 support initrd loading via the image load options
>>> + * (using initrd=, limited to the volume from which the kernel itself was
>>> + * loaded), or via arch specific means (bootparams, DT, etc).
>>> + *
>>> + * On x86, LoadImage() and StartImage() can be omitted if the EFI handover
>>> + * protocol is implemented, which can be inferred from the version,
>>> + * handover_offset and xloadflags fields in the bootparams structure.
>>> + */
>>> +#define LINUX_EFISTUB_MAJOR_VERSION 0x1
>>> +#define LINUX_EFISTUB_MINOR_VERSION 0x0
>>> +
>>> +#define MZ_MAGIC 0x5a4d /* "MZ" */
>>> +
>>> +#define PE_MAGIC 0x00004550 /* "PE\0\0" */
>>> +#define PE_OPT_MAGIC_PE32 0x010b
>>> +#define PE_OPT_MAGIC_PE32_ROM 0x0107
>>> +#define PE_OPT_MAGIC_PE32PLUS 0x020b
>>> +
>>> +/* machine type */
>>> +#define IMAGE_FILE_MACHINE_UNKNOWN 0x0000
>>> +#define IMAGE_FILE_MACHINE_AM33 0x01d3
>>> +#define IMAGE_FILE_MACHINE_AMD64 0x8664
>>> +#define IMAGE_FILE_MACHINE_ARM 0x01c0
>>> +#define IMAGE_FILE_MACHINE_ARMV7 0x01c4
>>> +#define IMAGE_FILE_MACHINE_ARM64 0xaa64
>>> +#define IMAGE_FILE_MACHINE_EBC 0x0ebc
>>> +#define IMAGE_FILE_MACHINE_I386 0x014c
>>> +#define IMAGE_FILE_MACHINE_IA64 0x0200
>>> +#define IMAGE_FILE_MACHINE_M32R 0x9041
>>> +#define IMAGE_FILE_MACHINE_MIPS16 0x0266
>>> +#define IMAGE_FILE_MACHINE_MIPSFPU 0x0366
>>> +#define IMAGE_FILE_MACHINE_MIPSFPU16 0x0466
>>> +#define IMAGE_FILE_MACHINE_POWERPC 0x01f0
>>> +#define IMAGE_FILE_MACHINE_POWERPCFP 0x01f1
>>> +#define IMAGE_FILE_MACHINE_R4000 0x0166
>>> +#define IMAGE_FILE_MACHINE_RISCV32 0x5032
>>> +#define IMAGE_FILE_MACHINE_RISCV64 0x5064
>>> +#define IMAGE_FILE_MACHINE_RISCV128 0x5128
>>> +#define IMAGE_FILE_MACHINE_SH3 0x01a2
>>> +#define IMAGE_FILE_MACHINE_SH3DSP 0x01a3
>>> +#define IMAGE_FILE_MACHINE_SH3E 0x01a4
>>> +#define IMAGE_FILE_MACHINE_SH4 0x01a6
>>> +#define IMAGE_FILE_MACHINE_SH5 0x01a8
>>> +#define IMAGE_FILE_MACHINE_THUMB 0x01c2
>>> +#define IMAGE_FILE_MACHINE_WCEMIPSV2 0x0169
>>> +
>>> +/* flags */
>>> +#define IMAGE_FILE_RELOCS_STRIPPED 0x0001
>>> +#define IMAGE_FILE_EXECUTABLE_IMAGE 0x0002
>>> +#define IMAGE_FILE_LINE_NUMS_STRIPPED 0x0004
>>> +#define IMAGE_FILE_LOCAL_SYMS_STRIPPED 0x0008
>>> +#define IMAGE_FILE_AGGRESSIVE_WS_TRIM 0x0010
>>> +#define IMAGE_FILE_LARGE_ADDRESS_AWARE 0x0020
>>> +#define IMAGE_FILE_16BIT_MACHINE 0x0040
>>> +#define IMAGE_FILE_BYTES_REVERSED_LO 0x0080
>>> +#define IMAGE_FILE_32BIT_MACHINE 0x0100
>>> +#define IMAGE_FILE_DEBUG_STRIPPED 0x0200
>>> +#define IMAGE_FILE_REMOVABLE_RUN_FROM_SWAP 0x0400
>>> +#define IMAGE_FILE_NET_RUN_FROM_SWAP 0x0800
>>> +#define IMAGE_FILE_SYSTEM 0x1000
>>> +#define IMAGE_FILE_DLL 0x2000
>>> +#define IMAGE_FILE_UP_SYSTEM_ONLY 0x4000
>>> +#define IMAGE_FILE_BYTES_REVERSED_HI 0x8000
>>> +
>>> +#define IMAGE_FILE_OPT_ROM_MAGIC 0x107
>>> +#define IMAGE_FILE_OPT_PE32_MAGIC 0x10b
>>> +#define IMAGE_FILE_OPT_PE32_PLUS_MAGIC 0x20b
>>> +
>>> +#define IMAGE_SUBSYSTEM_UNKNOWN 0
>>> +#define IMAGE_SUBSYSTEM_NATIVE 1
>>> +#define IMAGE_SUBSYSTEM_WINDOWS_GUI 2
>>> +#define IMAGE_SUBSYSTEM_WINDOWS_CUI 3
>>> +#define IMAGE_SUBSYSTEM_POSIX_CUI 7
>>> +#define IMAGE_SUBSYSTEM_WINDOWS_CE_GUI 9
>>> +#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10
>>> +#define IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER 11
>>> +#define IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER 12
>>> +#define IMAGE_SUBSYSTEM_EFI_ROM_IMAGE 13
>>> +#define IMAGE_SUBSYSTEM_XBOX 14
>>> +
>>> +#define IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE 0x0040
>>> +#define IMAGE_DLL_CHARACTERISTICS_FORCE_INTEGRITY 0x0080
>>> +#define IMAGE_DLL_CHARACTERISTICS_NX_COMPAT 0x0100
>>> +#define IMAGE_DLLCHARACTERISTICS_NO_ISOLATION 0x0200
>>> +#define IMAGE_DLLCHARACTERISTICS_NO_SEH 0x0400
>>> +#define IMAGE_DLLCHARACTERISTICS_NO_BIND 0x0800
>>> +#define IMAGE_DLLCHARACTERISTICS_WDM_DRIVER 0x2000
>>> +#define IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE 0x8000
>>> +
>>> +/* they actually defined 0x00000000 as well, but I think we'll skip that one. */
>>> +#define IMAGE_SCN_RESERVED_0 0x00000001
>>> +#define IMAGE_SCN_RESERVED_1 0x00000002
>>> +#define IMAGE_SCN_RESERVED_2 0x00000004
>>> +#define IMAGE_SCN_TYPE_NO_PAD 0x00000008 /* don't pad - obsolete */
>>> +#define IMAGE_SCN_RESERVED_3 0x00000010
>>> +#define IMAGE_SCN_CNT_CODE 0x00000020 /* .text */
>>> +#define IMAGE_SCN_CNT_INITIALIZED_DATA 0x00000040 /* .data */
>>> +#define IMAGE_SCN_CNT_UNINITIALIZED_DATA 0x00000080 /* .bss */
>>> +#define IMAGE_SCN_LNK_OTHER 0x00000100 /* reserved */
>>> +#define IMAGE_SCN_LNK_INFO 0x00000200 /* .drectve comments */
>>> +#define IMAGE_SCN_RESERVED_4 0x00000400
>>> +#define IMAGE_SCN_LNK_REMOVE 0x00000800 /* .o only - scn to be rm'd*/
>>> +#define IMAGE_SCN_LNK_COMDAT 0x00001000 /* .o only - COMDAT data */
>>> +#define IMAGE_SCN_RESERVED_5 0x00002000 /* spec omits this */
>>> +#define IMAGE_SCN_RESERVED_6 0x00004000 /* spec omits this */
>>> +#define IMAGE_SCN_GPREL 0x00008000 /* global pointer referenced data */
>>> +/* spec lists 0x20000 twice, I suspect they meant 0x10000 for one of them */
>>> +#define IMAGE_SCN_MEM_PURGEABLE 0x00010000 /* reserved for "future" use */
>>> +#define IMAGE_SCN_16BIT 0x00020000 /* reserved for "future" use */
>>> +#define IMAGE_SCN_LOCKED 0x00040000 /* reserved for "future" use */
>>> +#define IMAGE_SCN_PRELOAD 0x00080000 /* reserved for "future" use */
>>> +/* and here they just stuck a 1-byte integer in the middle of a bitfield */
>>> +#define IMAGE_SCN_ALIGN_1BYTES 0x00100000 /* it does what it says on the box */
>>> +#define IMAGE_SCN_ALIGN_2BYTES 0x00200000
>>> +#define IMAGE_SCN_ALIGN_4BYTES 0x00300000
>>> +#define IMAGE_SCN_ALIGN_8BYTES 0x00400000
>>> +#define IMAGE_SCN_ALIGN_16BYTES 0x00500000
>>> +#define IMAGE_SCN_ALIGN_32BYTES 0x00600000
>>> +#define IMAGE_SCN_ALIGN_64BYTES 0x00700000
>>> +#define IMAGE_SCN_ALIGN_128BYTES 0x00800000
>>> +#define IMAGE_SCN_ALIGN_256BYTES 0x00900000
>>> +#define IMAGE_SCN_ALIGN_512BYTES 0x00a00000
>>> +#define IMAGE_SCN_ALIGN_1024BYTES 0x00b00000
>>> +#define IMAGE_SCN_ALIGN_2048BYTES 0x00c00000
>>> +#define IMAGE_SCN_ALIGN_4096BYTES 0x00d00000
>>> +#define IMAGE_SCN_ALIGN_8192BYTES 0x00e00000
>>> +#define IMAGE_SCN_LNK_NRELOC_OVFL 0x01000000 /* extended relocations */
>>> +#define IMAGE_SCN_MEM_DISCARDABLE 0x02000000 /* scn can be discarded */
>>> +#define IMAGE_SCN_MEM_NOT_CACHED 0x04000000 /* cannot be cached */
>>> +#define IMAGE_SCN_MEM_NOT_PAGED 0x08000000 /* not pageable */
>>> +#define IMAGE_SCN_MEM_SHARED 0x10000000 /* can be shared */
>>> +#define IMAGE_SCN_MEM_EXECUTE 0x20000000 /* can be executed as code */
>>> +#define IMAGE_SCN_MEM_READ 0x40000000 /* readable */
>>> +#define IMAGE_SCN_MEM_WRITE 0x80000000 /* writeable */
>>> +
>>> +#define IMAGE_DEBUG_TYPE_CODEVIEW 2
>>> +
>>> +#ifndef __ASSEMBLY__
>>> +
>>> +struct mz_hdr {
>>> + uint16_t magic; /* MZ_MAGIC */
>>> + uint16_t lbsize; /* size of last used block */
>>> + uint16_t blocks; /* pages in file, 0x3 */
>>> + uint16_t relocs; /* relocations */
>>> + uint16_t hdrsize; /* header size in "paragraphs" */
>>> + uint16_t min_extra_pps; /* .bss */
>>> + uint16_t max_extra_pps; /* runtime limit for the arena size */
>>> + uint16_t ss; /* relative stack segment */
>>> + uint16_t sp; /* initial %sp register */
>>> + uint16_t checksum; /* word checksum */
>>> + uint16_t ip; /* initial %ip register */
>>> + uint16_t cs; /* initial %cs relative to load segment */
>>> + uint16_t reloc_table_offset; /* offset of the first relocation */
>>> + uint16_t overlay_num; /* overlay number. set to 0. */
>>> + uint16_t reserved0[4]; /* reserved */
>>> + uint16_t oem_id; /* oem identifier */
>>> + uint16_t oem_info; /* oem specific */
>>> + uint16_t reserved1[10]; /* reserved */
>>> + uint32_t peaddr; /* address of pe header */
>>> + char message[]; /* message to print */
>>> +};
>>
>> We already have an instance of this struct in common/efi/pe.c. I think
>> it wouldn't be very desirable to have two (different) instances.
>>
>>> +struct mz_reloc {
>>> + uint16_t offset;
>>> + uint16_t segment;
>>> +};
>>
>> We aren't going to need this anywhere, are we?
>>
>>> +struct pe_hdr {
>>> + uint32_t magic; /* PE magic */
>>> + uint16_t machine; /* machine type */
>>> + uint16_t sections; /* number of sections */
>>> + uint32_t timestamp; /* time_t */
>>> + uint32_t symbol_table; /* symbol table offset */
>>> + uint32_t symbols; /* number of symbols */
>>> + uint16_t opt_hdr_size; /* size of optional header */
>>> + uint16_t flags; /* flags */
>>> +};
>>
>> And again another (different) instance of this and further struct-s
>> already exists. Same for the section header further down.
>>
>>> +/* the fact that pe32 isn't padded where pe32+ is 64-bit means union won't
>>> + * work right. vomit. */
>>
>> Noticing here in particular, but being an issue elsewhere as well:
>> Unless this file is to be a verbatim copy taken from somewhere (in
>> which case it should probably be introduced in a separate commit
>> with an Origin: tag), comments want to adhere of ./CODING_STYLE.
>>
>>> +struct pe32_opt_hdr {
>>> + /* "standard" header */
>>> + uint16_t magic; /* file type */
>>> + uint8_t ld_major; /* linker major version */
>>> + uint8_t ld_minor; /* linker minor version */
>>> + uint32_t text_size; /* size of text section(s) */
>>> + uint32_t data_size; /* size of data section(s) */
>>> + uint32_t bss_size; /* size of bss section(s) */
>>> + uint32_t entry_point; /* file offset of entry point */
>>> + uint32_t code_base; /* relative code addr in ram */
>>> + uint32_t data_base; /* relative data addr in ram */
>>> + /* "windows" header */
>>> + uint32_t image_base; /* preferred load address */
>>> + uint32_t section_align; /* alignment in bytes */
>>> + uint32_t file_align; /* file alignment in bytes */
>>> + uint16_t os_major; /* major OS version */
>>> + uint16_t os_minor; /* minor OS version */
>>> + uint16_t image_major; /* major image version */
>>> + uint16_t image_minor; /* minor image version */
>>> + uint16_t subsys_major; /* major subsystem version */
>>> + uint16_t subsys_minor; /* minor subsystem version */
>>> + uint32_t win32_version; /* reserved, must be 0 */
>>> + uint32_t image_size; /* image size */
>>> + uint32_t header_size; /* header size rounded up to file_align */
>>> + uint32_t csum; /* checksum */
>>> + uint16_t subsys; /* subsystem */
>>> + uint16_t dll_flags; /* executable characteristics */
>>> + uint32_t stack_size_req; /* amt of stack requested */
>>> + uint32_t stack_size; /* amt of stack required */
>>> + uint32_t heap_size_req; /* amt of heap requested */
>>> + uint32_t heap_size; /* amt of heap required */
>>> + uint32_t loader_flags; /* reserved, must be 0 */
>>> + uint32_t data_dirs; /* number of data dir entries */
>>> +};
>>> +
>>> +struct pe32plus_opt_hdr {
>>> + uint16_t magic; /* file type */
>>> + uint8_t ld_major; /* linker major version */
>>> + uint8_t ld_minor; /* linker minor version */
>>> + uint32_t text_size; /* size of text section(s) */
>>> + uint32_t data_size; /* size of data section(s) */
>>> + uint32_t bss_size; /* size of bss section(s) */
>>> + uint32_t entry_point; /* file offset of entry point */
>>> + uint32_t code_base; /* relative code addr in ram */
>>> + /* "windows" header */
>>> + uint64_t image_base; /* preferred load address */
>>> + uint32_t section_align; /* alignment in bytes */
>>> + uint32_t file_align; /* file alignment in bytes */
>>> + uint16_t os_major; /* major OS version */
>>> + uint16_t os_minor; /* minor OS version */
>>> + uint16_t image_major; /* major image version */
>>> + uint16_t image_minor; /* minor image version */
>>> + uint16_t subsys_major; /* major subsystem version */
>>> + uint16_t subsys_minor; /* minor subsystem version */
>>> + uint32_t win32_version; /* reserved, must be 0 */
>>> + uint32_t image_size; /* image size */
>>> + uint32_t header_size; /* header size rounded up to file_align */
>>> + uint32_t csum; /* checksum */
>>> + uint16_t subsys; /* subsystem */
>>> + uint16_t dll_flags; /* executable characteristics */
>>> + uint64_t stack_size_req; /* amt of stack requested */
>>> + uint64_t stack_size; /* amt of stack required */
>>> + uint64_t heap_size_req; /* amt of heap requested */
>>> + uint64_t heap_size; /* amt of heap required */
>>> + uint32_t loader_flags; /* reserved, must be 0 */
>>> + uint32_t data_dirs; /* number of data dir entries */
>>> +};
>>> +
>>> +struct data_dirent {
>>> + uint32_t virtual_address; /* relative to load address */
>>> + uint32_t size;
>>> +};
>>
>> Will we need this and ...
>>
>>> +struct data_directory {
>>> + struct data_dirent exports; /* .edata */
>>> + struct data_dirent imports; /* .idata */
>>> + struct data_dirent resources; /* .rsrc */
>>> + struct data_dirent exceptions; /* .pdata */
>>> + struct data_dirent certs; /* certs */
>>> + struct data_dirent base_relocations; /* .reloc */
>>> + struct data_dirent debug; /* .debug */
>>> + struct data_dirent arch; /* reservered */
>>> + struct data_dirent global_ptr; /* global pointer reg. Size=0 */
>>> + struct data_dirent tls; /* .tls */
>>> + struct data_dirent load_config; /* load configuration structure */
>>> + struct data_dirent bound_imports; /* no idea */
>>> + struct data_dirent import_addrs; /* import address table */
>>> + struct data_dirent delay_imports; /* delay-load import table */
>>> + struct data_dirent clr_runtime_hdr; /* .cor (object only) */
>>> + struct data_dirent reserved;
>>> +};
>>
>> ... this?
>>
>>> +struct section_header {
>>> + char name[8]; /* name or string tbl offset */
>>> + uint32_t virtual_size; /* size of loaded section in ram */
>>> + uint32_t virtual_address; /* relative virtual address */
>>> + uint32_t raw_data_size; /* size of the section */
>>> + uint32_t data_addr; /* file pointer to first page of sec */
>>> + uint32_t relocs; /* file pointer to relocation entries */
>>> + uint32_t line_numbers; /* line numbers */
>>> + uint16_t num_relocs; /* number of relocations */
>>> + uint16_t num_lin_numbers; /* COFF line count. */
>>> + uint32_t flags;
>>> +};
>>> +
>>> +enum x64_coff_reloc_type {
>>> + IMAGE_REL_AMD64_ABSOLUTE = 0,
>>> + IMAGE_REL_AMD64_ADDR64,
>>> + IMAGE_REL_AMD64_ADDR32,
>>> + IMAGE_REL_AMD64_ADDR32N,
>>> + IMAGE_REL_AMD64_REL32,
>>> + IMAGE_REL_AMD64_REL32_1,
>>> + IMAGE_REL_AMD64_REL32_2,
>>> + IMAGE_REL_AMD64_REL32_3,
>>> + IMAGE_REL_AMD64_REL32_4,
>>> + IMAGE_REL_AMD64_REL32_5,
>>> + IMAGE_REL_AMD64_SECTION,
>>> + IMAGE_REL_AMD64_SECREL,
>>> + IMAGE_REL_AMD64_SECREL7,
>>> + IMAGE_REL_AMD64_TOKEN,
>>> + IMAGE_REL_AMD64_SREL32,
>>> + IMAGE_REL_AMD64_PAIR,
>>> + IMAGE_REL_AMD64_SSPAN32,
>>> +};
>>> +
>>> +enum arm_coff_reloc_type {
>>> + IMAGE_REL_ARM_ABSOLUTE,
>>> + IMAGE_REL_ARM_ADDR32,
>>> + IMAGE_REL_ARM_ADDR32N,
>>> + IMAGE_REL_ARM_BRANCH2,
>>> + IMAGE_REL_ARM_BRANCH1,
>>> + IMAGE_REL_ARM_SECTION,
>>> + IMAGE_REL_ARM_SECREL,
>>> +};
>>> +
>>> +enum sh_coff_reloc_type {
>>> + IMAGE_REL_SH3_ABSOLUTE,
>>> + IMAGE_REL_SH3_DIRECT16,
>>> + IMAGE_REL_SH3_DIRECT32,
>>> + IMAGE_REL_SH3_DIRECT8,
>>> + IMAGE_REL_SH3_DIRECT8_WORD,
>>> + IMAGE_REL_SH3_DIRECT8_LONG,
>>> + IMAGE_REL_SH3_DIRECT4,
>>> + IMAGE_REL_SH3_DIRECT4_WORD,
>>> + IMAGE_REL_SH3_DIRECT4_LONG,
>>> + IMAGE_REL_SH3_PCREL8_WORD,
>>> + IMAGE_REL_SH3_PCREL8_LONG,
>>> + IMAGE_REL_SH3_PCREL12_WORD,
>>> + IMAGE_REL_SH3_STARTOF_SECTION,
>>> + IMAGE_REL_SH3_SIZEOF_SECTION,
>>> + IMAGE_REL_SH3_SECTION,
>>> + IMAGE_REL_SH3_SECREL,
>>> + IMAGE_REL_SH3_DIRECT32_NB,
>>> + IMAGE_REL_SH3_GPREL4_LONG,
>>> + IMAGE_REL_SH3_TOKEN,
>>> + IMAGE_REL_SHM_PCRELPT,
>>> + IMAGE_REL_SHM_REFLO,
>>> + IMAGE_REL_SHM_REFHALF,
>>> + IMAGE_REL_SHM_RELLO,
>>> + IMAGE_REL_SHM_RELHALF,
>>> + IMAGE_REL_SHM_PAIR,
>>> + IMAGE_REL_SHM_NOMODE,
>>> +};
>>> +
>>> +enum ppc_coff_reloc_type {
>>> + IMAGE_REL_PPC_ABSOLUTE,
>>> + IMAGE_REL_PPC_ADDR64,
>>> + IMAGE_REL_PPC_ADDR32,
>>> + IMAGE_REL_PPC_ADDR24,
>>> + IMAGE_REL_PPC_ADDR16,
>>> + IMAGE_REL_PPC_ADDR14,
>>> + IMAGE_REL_PPC_REL24,
>>> + IMAGE_REL_PPC_REL14,
>>> + IMAGE_REL_PPC_ADDR32N,
>>> + IMAGE_REL_PPC_SECREL,
>>> + IMAGE_REL_PPC_SECTION,
>>> + IMAGE_REL_PPC_SECREL16,
>>> + IMAGE_REL_PPC_REFHI,
>>> + IMAGE_REL_PPC_REFLO,
>>> + IMAGE_REL_PPC_PAIR,
>>> + IMAGE_REL_PPC_SECRELLO,
>>> + IMAGE_REL_PPC_GPREL,
>>> + IMAGE_REL_PPC_TOKEN,
>>> +};
>>> +
>>> +enum x86_coff_reloc_type {
>>> + IMAGE_REL_I386_ABSOLUTE,
>>> + IMAGE_REL_I386_DIR16,
>>> + IMAGE_REL_I386_REL16,
>>> + IMAGE_REL_I386_DIR32,
>>> + IMAGE_REL_I386_DIR32NB,
>>> + IMAGE_REL_I386_SEG12,
>>> + IMAGE_REL_I386_SECTION,
>>> + IMAGE_REL_I386_SECREL,
>>> + IMAGE_REL_I386_TOKEN,
>>> + IMAGE_REL_I386_SECREL7,
>>> + IMAGE_REL_I386_REL32,
>>> +};
>>> +
>>> +enum ia64_coff_reloc_type {
>>> + IMAGE_REL_IA64_ABSOLUTE,
>>> + IMAGE_REL_IA64_IMM14,
>>> + IMAGE_REL_IA64_IMM22,
>>> + IMAGE_REL_IA64_IMM64,
>>> + IMAGE_REL_IA64_DIR32,
>>> + IMAGE_REL_IA64_DIR64,
>>> + IMAGE_REL_IA64_PCREL21B,
>>> + IMAGE_REL_IA64_PCREL21M,
>>> + IMAGE_REL_IA64_PCREL21F,
>>> + IMAGE_REL_IA64_GPREL22,
>>> + IMAGE_REL_IA64_LTOFF22,
>>> + IMAGE_REL_IA64_SECTION,
>>> + IMAGE_REL_IA64_SECREL22,
>>> + IMAGE_REL_IA64_SECREL64I,
>>> + IMAGE_REL_IA64_SECREL32,
>>> + IMAGE_REL_IA64_DIR32NB,
>>> + IMAGE_REL_IA64_SREL14,
>>> + IMAGE_REL_IA64_SREL22,
>>> + IMAGE_REL_IA64_SREL32,
>>> + IMAGE_REL_IA64_UREL32,
>>> + IMAGE_REL_IA64_PCREL60X,
>>> + IMAGE_REL_IA64_PCREL60B,
>>> + IMAGE_REL_IA64_PCREL60F,
>>> + IMAGE_REL_IA64_PCREL60I,
>>> + IMAGE_REL_IA64_PCREL60M,
>>> + IMAGE_REL_IA64_IMMGPREL6,
>>> + IMAGE_REL_IA64_TOKEN,
>>> + IMAGE_REL_IA64_GPREL32,
>>> + IMAGE_REL_IA64_ADDEND,
>>> +};
>>
>> All sorts of relocation types, but none for RISC-V? Are we going to need
>> any of this?
>>
>>> +struct coff_reloc {
>>> + uint32_t virtual_address;
>>> + uint32_t symbol_table_index;
>>> +
>>> + union {
>>> + enum x64_coff_reloc_type x64_type;
>>> + enum arm_coff_reloc_type arm_type;
>>> + enum sh_coff_reloc_type sh_type;
>>> + enum ppc_coff_reloc_type ppc_type;
>>> + enum x86_coff_reloc_type x86_type;
>>> + enum ia64_coff_reloc_type ia64_type;
>>> + uint16_t data;
>>> + };
>>> +};
>>> +
>>> +/*
>>> + * Definitions for the contents of the certs data block
>>> + */
>>> +#define WIN_CERT_TYPE_PKCS_SIGNED_DATA 0x0002
>>> +#define WIN_CERT_TYPE_EFI_OKCS115 0x0EF0
>>> +#define WIN_CERT_TYPE_EFI_GUID 0x0EF1
>>> +
>>> +#define WIN_CERT_REVISION_1_0 0x0100
>>> +#define WIN_CERT_REVISION_2_0 0x0200
>>> +
>>> +struct win_certificate {
>>> + uint32_t length;
>>> + uint16_t revision;
>>> + uint16_t cert_type;
>>> +};
>>
>> Or any of this?
>>
> Regarding pe.h file content, we wanted to keep it as generic as
> possible (structures definition according to PE format which can be
> used for multiple architectures). Specifically for RISC-V as you
> noticed, we are not using lots of structures (data directories,
> relocation structures, certificates, etc.). Therefore, we can reduce
> it to only those used atm, but in that case we won't have a generic PE
> header definition anymore. Regarding structures which are already
> defined in common/efi/pe.c, meaning that with our change we have two
> versions of same structures, we can remove those, but in that case it
> could be confusing for someone who is trying to map fields from pe.h
> to actual image header in assembly code. Summary I would keep this
> header with its original content, but if you think that it contains
> too much overhead we can reduce it to relevant structures only.
Actually I not only don't mind this header, but would consider it
superior to the present state of things. Just that then imo it would
want introducing in a separate commit, with suitable description /
justification. That could (should) then be followed by a patch using
this header's struct-s / definitions in pre-existing code, purging
any duplication from there.
Jan
On Mon, Jul 8, 2024 at 11:32 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 04.07.2024 19:21, Milan Đokić wrote:
> > On Wed, Jul 3, 2024 at 5:55 PM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 03.07.2024 02:04, Milan Djokic wrote:
> >>> +#ifdef CONFIG_RISCV_EFI
> >>> + /*
> >>> + * This instruction decodes to "MZ" ASCII required by UEFI.
> >>> + */
> >>> + c.li s4,-13
> >>> + c.j xen_start
> >>> +#else
> >>> + /* jump to start kernel */
> >>> + jal xen_start
> >>
> >> JAL, not J? Why?
> >>
> > We use jal explicitly here to highlight that we want to occupy 32 bits
> > in order to align with header format (and EFI case where we use two
> > 16-bits instructions). Although it will also work by using "j"
> > directly, it could be implicitly compressed to 16 bits (if C extension
> > is available) which would make header layout less obvious imo.
>
> According to e.g. ...
>
> >>> +#endif
> >>> + .balign 8
> >>
> >> This won't do what you want unless "start" itself is also suitably aligned.
> >> It'll be as long as it's first in the section, but better make such explicit.
> >>
> > I understand, we'll also explicitly align "start"
> >
> >>> +#ifdef CONFIG_RISCV_64
> >>> + /* Image load offset(2MB) from start of RAM */
> >>> + .quad 0x200000
> >>> +#else
> >>> + /* Image load offset(4MB) from start of RAM */
> >>> + .quad 0x400000
> >>> +#endif
>
> ... the #ifdef here, you aim at having code be suitable for RV32, too.
> However, JAL has a compressed form there, so its use would make things
> "less obvious" there as well. I'm inclined to say that since the
> subsequent ".balign 8" adds padding NOPs anyway, there's no real
> difference whether that adds 32 bits worth of NOPs or 48 bits. If you
> really wanted to "hide" the difference, imo ".balign 4" would be the
> way to go.
>
> In any event, if there would still be a reason to stick to JAL, you'd
> want to name the reason(s) in a code comment.
>
No specific reason to use jal from a functional point of view, just
aesthetic reasons as I mentioned in a previous comment. Also jal is
used across head.S file, so we also keep some consistency in that
manner. I just don't get the reason why "j" is more suitable as a
pseudo instruction since it also comes down to jal as a base
instruction.
Of course, we can easily switch to "j" here, just want to know why we
are doing so.
> >> What these constants derive from? I expect they aren't really "magic" .
> >>
> > These offsets are commonly used for RISCV SOCs. In general, these can
> > have different values, depending on memory layout, but in order to be
> > compatible with
> > linux bootloaders we just followed the common practice.
>
> So entirely magic, really. Such imo needs commenting on.
>
Sure, we'll comment on this one.
> >>> @@ -60,6 +100,7 @@ FUNC(start)
> >>> mv a1, s1
> >>>
> >>> tail start_xen
> >>> +
> >>> END(start)
> >>>
> >>> .section .text, "ax", %progbits
> >>
> >> What is this hunk about?
> >>
> > Not sure which part of the code you refer to here. This section is
> > part of the original riscv implementation. We only grouped this
> > section under the new, xen_start label since we have different
> > handling for EFI and ELF formats in the beginning of "start" function
> > before jumping to xen_start.
>
> "Hunk" is common term for a single piece of "diff" output, starting at
> the @@ line. To word it differently: What reason is there to insert a
> blank line in this very patch?
>
Sry, I haven't seen the blank line diff at all. We'll remove this one,
it was added by mistake.
> >>> --- /dev/null
> >>> +++ b/xen/include/efi/pe.h
> >>> @@ -0,0 +1,458 @@
> >>> +#ifndef __PE_H
> >>
> >> This probably wants (needs?) to gain an SPDX line.
> >>
> > Yes, it was originally taken over from the linux kernel (with some
> > modifications from our side), but we omitted the SPDX line by mistake.
> >
> >>> +#define __PE_H
> >>> +
> >>> +/*
> >>> + * Linux EFI stub v1.0 adds the following functionality:
> >>> + * - Loading initrd from the LINUX_EFI_INITRD_MEDIA_GUID device path,
> >>> + * - Loading/starting the kernel from firmware that targets a different
> >>> + * machine type, via the entrypoint exposed in the .compat PE/COFF section.
> >>> + *
> >>> + * The recommended way of loading and starting v1.0 or later kernels is to use
> >>> + * the LoadImage() and StartImage() EFI boot services, and expose the initrd
> >>> + * via the LINUX_EFI_INITRD_MEDIA_GUID device path.
> >>> + *
> >>> + * Versions older than v1.0 support initrd loading via the image load options
> >>> + * (using initrd=, limited to the volume from which the kernel itself was
> >>> + * loaded), or via arch specific means (bootparams, DT, etc).
> >>> + *
> >>> + * On x86, LoadImage() and StartImage() can be omitted if the EFI handover
> >>> + * protocol is implemented, which can be inferred from the version,
> >>> + * handover_offset and xloadflags fields in the bootparams structure.
> >>> + */
> >>> +#define LINUX_EFISTUB_MAJOR_VERSION 0x1
> >>> +#define LINUX_EFISTUB_MINOR_VERSION 0x0
> >>> +
> >>> +#define MZ_MAGIC 0x5a4d /* "MZ" */
> >>> +
> >>> +#define PE_MAGIC 0x00004550 /* "PE\0\0" */
> >>> +#define PE_OPT_MAGIC_PE32 0x010b
> >>> +#define PE_OPT_MAGIC_PE32_ROM 0x0107
> >>> +#define PE_OPT_MAGIC_PE32PLUS 0x020b
> >>> +
> >>> +/* machine type */
> >>> +#define IMAGE_FILE_MACHINE_UNKNOWN 0x0000
> >>> +#define IMAGE_FILE_MACHINE_AM33 0x01d3
> >>> +#define IMAGE_FILE_MACHINE_AMD64 0x8664
> >>> +#define IMAGE_FILE_MACHINE_ARM 0x01c0
> >>> +#define IMAGE_FILE_MACHINE_ARMV7 0x01c4
> >>> +#define IMAGE_FILE_MACHINE_ARM64 0xaa64
> >>> +#define IMAGE_FILE_MACHINE_EBC 0x0ebc
> >>> +#define IMAGE_FILE_MACHINE_I386 0x014c
> >>> +#define IMAGE_FILE_MACHINE_IA64 0x0200
> >>> +#define IMAGE_FILE_MACHINE_M32R 0x9041
> >>> +#define IMAGE_FILE_MACHINE_MIPS16 0x0266
> >>> +#define IMAGE_FILE_MACHINE_MIPSFPU 0x0366
> >>> +#define IMAGE_FILE_MACHINE_MIPSFPU16 0x0466
> >>> +#define IMAGE_FILE_MACHINE_POWERPC 0x01f0
> >>> +#define IMAGE_FILE_MACHINE_POWERPCFP 0x01f1
> >>> +#define IMAGE_FILE_MACHINE_R4000 0x0166
> >>> +#define IMAGE_FILE_MACHINE_RISCV32 0x5032
> >>> +#define IMAGE_FILE_MACHINE_RISCV64 0x5064
> >>> +#define IMAGE_FILE_MACHINE_RISCV128 0x5128
> >>> +#define IMAGE_FILE_MACHINE_SH3 0x01a2
> >>> +#define IMAGE_FILE_MACHINE_SH3DSP 0x01a3
> >>> +#define IMAGE_FILE_MACHINE_SH3E 0x01a4
> >>> +#define IMAGE_FILE_MACHINE_SH4 0x01a6
> >>> +#define IMAGE_FILE_MACHINE_SH5 0x01a8
> >>> +#define IMAGE_FILE_MACHINE_THUMB 0x01c2
> >>> +#define IMAGE_FILE_MACHINE_WCEMIPSV2 0x0169
> >>> +
> >>> +/* flags */
> >>> +#define IMAGE_FILE_RELOCS_STRIPPED 0x0001
> >>> +#define IMAGE_FILE_EXECUTABLE_IMAGE 0x0002
> >>> +#define IMAGE_FILE_LINE_NUMS_STRIPPED 0x0004
> >>> +#define IMAGE_FILE_LOCAL_SYMS_STRIPPED 0x0008
> >>> +#define IMAGE_FILE_AGGRESSIVE_WS_TRIM 0x0010
> >>> +#define IMAGE_FILE_LARGE_ADDRESS_AWARE 0x0020
> >>> +#define IMAGE_FILE_16BIT_MACHINE 0x0040
> >>> +#define IMAGE_FILE_BYTES_REVERSED_LO 0x0080
> >>> +#define IMAGE_FILE_32BIT_MACHINE 0x0100
> >>> +#define IMAGE_FILE_DEBUG_STRIPPED 0x0200
> >>> +#define IMAGE_FILE_REMOVABLE_RUN_FROM_SWAP 0x0400
> >>> +#define IMAGE_FILE_NET_RUN_FROM_SWAP 0x0800
> >>> +#define IMAGE_FILE_SYSTEM 0x1000
> >>> +#define IMAGE_FILE_DLL 0x2000
> >>> +#define IMAGE_FILE_UP_SYSTEM_ONLY 0x4000
> >>> +#define IMAGE_FILE_BYTES_REVERSED_HI 0x8000
> >>> +
> >>> +#define IMAGE_FILE_OPT_ROM_MAGIC 0x107
> >>> +#define IMAGE_FILE_OPT_PE32_MAGIC 0x10b
> >>> +#define IMAGE_FILE_OPT_PE32_PLUS_MAGIC 0x20b
> >>> +
> >>> +#define IMAGE_SUBSYSTEM_UNKNOWN 0
> >>> +#define IMAGE_SUBSYSTEM_NATIVE 1
> >>> +#define IMAGE_SUBSYSTEM_WINDOWS_GUI 2
> >>> +#define IMAGE_SUBSYSTEM_WINDOWS_CUI 3
> >>> +#define IMAGE_SUBSYSTEM_POSIX_CUI 7
> >>> +#define IMAGE_SUBSYSTEM_WINDOWS_CE_GUI 9
> >>> +#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10
> >>> +#define IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER 11
> >>> +#define IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER 12
> >>> +#define IMAGE_SUBSYSTEM_EFI_ROM_IMAGE 13
> >>> +#define IMAGE_SUBSYSTEM_XBOX 14
> >>> +
> >>> +#define IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE 0x0040
> >>> +#define IMAGE_DLL_CHARACTERISTICS_FORCE_INTEGRITY 0x0080
> >>> +#define IMAGE_DLL_CHARACTERISTICS_NX_COMPAT 0x0100
> >>> +#define IMAGE_DLLCHARACTERISTICS_NO_ISOLATION 0x0200
> >>> +#define IMAGE_DLLCHARACTERISTICS_NO_SEH 0x0400
> >>> +#define IMAGE_DLLCHARACTERISTICS_NO_BIND 0x0800
> >>> +#define IMAGE_DLLCHARACTERISTICS_WDM_DRIVER 0x2000
> >>> +#define IMAGE_DLLCHARACTERISTICS_TERMINAL_SERVER_AWARE 0x8000
> >>> +
> >>> +/* they actually defined 0x00000000 as well, but I think we'll skip that one. */
> >>> +#define IMAGE_SCN_RESERVED_0 0x00000001
> >>> +#define IMAGE_SCN_RESERVED_1 0x00000002
> >>> +#define IMAGE_SCN_RESERVED_2 0x00000004
> >>> +#define IMAGE_SCN_TYPE_NO_PAD 0x00000008 /* don't pad - obsolete */
> >>> +#define IMAGE_SCN_RESERVED_3 0x00000010
> >>> +#define IMAGE_SCN_CNT_CODE 0x00000020 /* .text */
> >>> +#define IMAGE_SCN_CNT_INITIALIZED_DATA 0x00000040 /* .data */
> >>> +#define IMAGE_SCN_CNT_UNINITIALIZED_DATA 0x00000080 /* .bss */
> >>> +#define IMAGE_SCN_LNK_OTHER 0x00000100 /* reserved */
> >>> +#define IMAGE_SCN_LNK_INFO 0x00000200 /* .drectve comments */
> >>> +#define IMAGE_SCN_RESERVED_4 0x00000400
> >>> +#define IMAGE_SCN_LNK_REMOVE 0x00000800 /* .o only - scn to be rm'd*/
> >>> +#define IMAGE_SCN_LNK_COMDAT 0x00001000 /* .o only - COMDAT data */
> >>> +#define IMAGE_SCN_RESERVED_5 0x00002000 /* spec omits this */
> >>> +#define IMAGE_SCN_RESERVED_6 0x00004000 /* spec omits this */
> >>> +#define IMAGE_SCN_GPREL 0x00008000 /* global pointer referenced data */
> >>> +/* spec lists 0x20000 twice, I suspect they meant 0x10000 for one of them */
> >>> +#define IMAGE_SCN_MEM_PURGEABLE 0x00010000 /* reserved for "future" use */
> >>> +#define IMAGE_SCN_16BIT 0x00020000 /* reserved for "future" use */
> >>> +#define IMAGE_SCN_LOCKED 0x00040000 /* reserved for "future" use */
> >>> +#define IMAGE_SCN_PRELOAD 0x00080000 /* reserved for "future" use */
> >>> +/* and here they just stuck a 1-byte integer in the middle of a bitfield */
> >>> +#define IMAGE_SCN_ALIGN_1BYTES 0x00100000 /* it does what it says on the box */
> >>> +#define IMAGE_SCN_ALIGN_2BYTES 0x00200000
> >>> +#define IMAGE_SCN_ALIGN_4BYTES 0x00300000
> >>> +#define IMAGE_SCN_ALIGN_8BYTES 0x00400000
> >>> +#define IMAGE_SCN_ALIGN_16BYTES 0x00500000
> >>> +#define IMAGE_SCN_ALIGN_32BYTES 0x00600000
> >>> +#define IMAGE_SCN_ALIGN_64BYTES 0x00700000
> >>> +#define IMAGE_SCN_ALIGN_128BYTES 0x00800000
> >>> +#define IMAGE_SCN_ALIGN_256BYTES 0x00900000
> >>> +#define IMAGE_SCN_ALIGN_512BYTES 0x00a00000
> >>> +#define IMAGE_SCN_ALIGN_1024BYTES 0x00b00000
> >>> +#define IMAGE_SCN_ALIGN_2048BYTES 0x00c00000
> >>> +#define IMAGE_SCN_ALIGN_4096BYTES 0x00d00000
> >>> +#define IMAGE_SCN_ALIGN_8192BYTES 0x00e00000
> >>> +#define IMAGE_SCN_LNK_NRELOC_OVFL 0x01000000 /* extended relocations */
> >>> +#define IMAGE_SCN_MEM_DISCARDABLE 0x02000000 /* scn can be discarded */
> >>> +#define IMAGE_SCN_MEM_NOT_CACHED 0x04000000 /* cannot be cached */
> >>> +#define IMAGE_SCN_MEM_NOT_PAGED 0x08000000 /* not pageable */
> >>> +#define IMAGE_SCN_MEM_SHARED 0x10000000 /* can be shared */
> >>> +#define IMAGE_SCN_MEM_EXECUTE 0x20000000 /* can be executed as code */
> >>> +#define IMAGE_SCN_MEM_READ 0x40000000 /* readable */
> >>> +#define IMAGE_SCN_MEM_WRITE 0x80000000 /* writeable */
> >>> +
> >>> +#define IMAGE_DEBUG_TYPE_CODEVIEW 2
> >>> +
> >>> +#ifndef __ASSEMBLY__
> >>> +
> >>> +struct mz_hdr {
> >>> + uint16_t magic; /* MZ_MAGIC */
> >>> + uint16_t lbsize; /* size of last used block */
> >>> + uint16_t blocks; /* pages in file, 0x3 */
> >>> + uint16_t relocs; /* relocations */
> >>> + uint16_t hdrsize; /* header size in "paragraphs" */
> >>> + uint16_t min_extra_pps; /* .bss */
> >>> + uint16_t max_extra_pps; /* runtime limit for the arena size */
> >>> + uint16_t ss; /* relative stack segment */
> >>> + uint16_t sp; /* initial %sp register */
> >>> + uint16_t checksum; /* word checksum */
> >>> + uint16_t ip; /* initial %ip register */
> >>> + uint16_t cs; /* initial %cs relative to load segment */
> >>> + uint16_t reloc_table_offset; /* offset of the first relocation */
> >>> + uint16_t overlay_num; /* overlay number. set to 0. */
> >>> + uint16_t reserved0[4]; /* reserved */
> >>> + uint16_t oem_id; /* oem identifier */
> >>> + uint16_t oem_info; /* oem specific */
> >>> + uint16_t reserved1[10]; /* reserved */
> >>> + uint32_t peaddr; /* address of pe header */
> >>> + char message[]; /* message to print */
> >>> +};
> >>
> >> We already have an instance of this struct in common/efi/pe.c. I think
> >> it wouldn't be very desirable to have two (different) instances.
> >>
> >>> +struct mz_reloc {
> >>> + uint16_t offset;
> >>> + uint16_t segment;
> >>> +};
> >>
> >> We aren't going to need this anywhere, are we?
> >>
> >>> +struct pe_hdr {
> >>> + uint32_t magic; /* PE magic */
> >>> + uint16_t machine; /* machine type */
> >>> + uint16_t sections; /* number of sections */
> >>> + uint32_t timestamp; /* time_t */
> >>> + uint32_t symbol_table; /* symbol table offset */
> >>> + uint32_t symbols; /* number of symbols */
> >>> + uint16_t opt_hdr_size; /* size of optional header */
> >>> + uint16_t flags; /* flags */
> >>> +};
> >>
> >> And again another (different) instance of this and further struct-s
> >> already exists. Same for the section header further down.
> >>
> >>> +/* the fact that pe32 isn't padded where pe32+ is 64-bit means union won't
> >>> + * work right. vomit. */
> >>
> >> Noticing here in particular, but being an issue elsewhere as well:
> >> Unless this file is to be a verbatim copy taken from somewhere (in
> >> which case it should probably be introduced in a separate commit
> >> with an Origin: tag), comments want to adhere of ./CODING_STYLE.
> >>
> >>> +struct pe32_opt_hdr {
> >>> + /* "standard" header */
> >>> + uint16_t magic; /* file type */
> >>> + uint8_t ld_major; /* linker major version */
> >>> + uint8_t ld_minor; /* linker minor version */
> >>> + uint32_t text_size; /* size of text section(s) */
> >>> + uint32_t data_size; /* size of data section(s) */
> >>> + uint32_t bss_size; /* size of bss section(s) */
> >>> + uint32_t entry_point; /* file offset of entry point */
> >>> + uint32_t code_base; /* relative code addr in ram */
> >>> + uint32_t data_base; /* relative data addr in ram */
> >>> + /* "windows" header */
> >>> + uint32_t image_base; /* preferred load address */
> >>> + uint32_t section_align; /* alignment in bytes */
> >>> + uint32_t file_align; /* file alignment in bytes */
> >>> + uint16_t os_major; /* major OS version */
> >>> + uint16_t os_minor; /* minor OS version */
> >>> + uint16_t image_major; /* major image version */
> >>> + uint16_t image_minor; /* minor image version */
> >>> + uint16_t subsys_major; /* major subsystem version */
> >>> + uint16_t subsys_minor; /* minor subsystem version */
> >>> + uint32_t win32_version; /* reserved, must be 0 */
> >>> + uint32_t image_size; /* image size */
> >>> + uint32_t header_size; /* header size rounded up to file_align */
> >>> + uint32_t csum; /* checksum */
> >>> + uint16_t subsys; /* subsystem */
> >>> + uint16_t dll_flags; /* executable characteristics */
> >>> + uint32_t stack_size_req; /* amt of stack requested */
> >>> + uint32_t stack_size; /* amt of stack required */
> >>> + uint32_t heap_size_req; /* amt of heap requested */
> >>> + uint32_t heap_size; /* amt of heap required */
> >>> + uint32_t loader_flags; /* reserved, must be 0 */
> >>> + uint32_t data_dirs; /* number of data dir entries */
> >>> +};
> >>> +
> >>> +struct pe32plus_opt_hdr {
> >>> + uint16_t magic; /* file type */
> >>> + uint8_t ld_major; /* linker major version */
> >>> + uint8_t ld_minor; /* linker minor version */
> >>> + uint32_t text_size; /* size of text section(s) */
> >>> + uint32_t data_size; /* size of data section(s) */
> >>> + uint32_t bss_size; /* size of bss section(s) */
> >>> + uint32_t entry_point; /* file offset of entry point */
> >>> + uint32_t code_base; /* relative code addr in ram */
> >>> + /* "windows" header */
> >>> + uint64_t image_base; /* preferred load address */
> >>> + uint32_t section_align; /* alignment in bytes */
> >>> + uint32_t file_align; /* file alignment in bytes */
> >>> + uint16_t os_major; /* major OS version */
> >>> + uint16_t os_minor; /* minor OS version */
> >>> + uint16_t image_major; /* major image version */
> >>> + uint16_t image_minor; /* minor image version */
> >>> + uint16_t subsys_major; /* major subsystem version */
> >>> + uint16_t subsys_minor; /* minor subsystem version */
> >>> + uint32_t win32_version; /* reserved, must be 0 */
> >>> + uint32_t image_size; /* image size */
> >>> + uint32_t header_size; /* header size rounded up to file_align */
> >>> + uint32_t csum; /* checksum */
> >>> + uint16_t subsys; /* subsystem */
> >>> + uint16_t dll_flags; /* executable characteristics */
> >>> + uint64_t stack_size_req; /* amt of stack requested */
> >>> + uint64_t stack_size; /* amt of stack required */
> >>> + uint64_t heap_size_req; /* amt of heap requested */
> >>> + uint64_t heap_size; /* amt of heap required */
> >>> + uint32_t loader_flags; /* reserved, must be 0 */
> >>> + uint32_t data_dirs; /* number of data dir entries */
> >>> +};
> >>> +
> >>> +struct data_dirent {
> >>> + uint32_t virtual_address; /* relative to load address */
> >>> + uint32_t size;
> >>> +};
> >>
> >> Will we need this and ...
> >>
> >>> +struct data_directory {
> >>> + struct data_dirent exports; /* .edata */
> >>> + struct data_dirent imports; /* .idata */
> >>> + struct data_dirent resources; /* .rsrc */
> >>> + struct data_dirent exceptions; /* .pdata */
> >>> + struct data_dirent certs; /* certs */
> >>> + struct data_dirent base_relocations; /* .reloc */
> >>> + struct data_dirent debug; /* .debug */
> >>> + struct data_dirent arch; /* reservered */
> >>> + struct data_dirent global_ptr; /* global pointer reg. Size=0 */
> >>> + struct data_dirent tls; /* .tls */
> >>> + struct data_dirent load_config; /* load configuration structure */
> >>> + struct data_dirent bound_imports; /* no idea */
> >>> + struct data_dirent import_addrs; /* import address table */
> >>> + struct data_dirent delay_imports; /* delay-load import table */
> >>> + struct data_dirent clr_runtime_hdr; /* .cor (object only) */
> >>> + struct data_dirent reserved;
> >>> +};
> >>
> >> ... this?
> >>
> >>> +struct section_header {
> >>> + char name[8]; /* name or string tbl offset */
> >>> + uint32_t virtual_size; /* size of loaded section in ram */
> >>> + uint32_t virtual_address; /* relative virtual address */
> >>> + uint32_t raw_data_size; /* size of the section */
> >>> + uint32_t data_addr; /* file pointer to first page of sec */
> >>> + uint32_t relocs; /* file pointer to relocation entries */
> >>> + uint32_t line_numbers; /* line numbers */
> >>> + uint16_t num_relocs; /* number of relocations */
> >>> + uint16_t num_lin_numbers; /* COFF line count. */
> >>> + uint32_t flags;
> >>> +};
> >>> +
> >>> +enum x64_coff_reloc_type {
> >>> + IMAGE_REL_AMD64_ABSOLUTE = 0,
> >>> + IMAGE_REL_AMD64_ADDR64,
> >>> + IMAGE_REL_AMD64_ADDR32,
> >>> + IMAGE_REL_AMD64_ADDR32N,
> >>> + IMAGE_REL_AMD64_REL32,
> >>> + IMAGE_REL_AMD64_REL32_1,
> >>> + IMAGE_REL_AMD64_REL32_2,
> >>> + IMAGE_REL_AMD64_REL32_3,
> >>> + IMAGE_REL_AMD64_REL32_4,
> >>> + IMAGE_REL_AMD64_REL32_5,
> >>> + IMAGE_REL_AMD64_SECTION,
> >>> + IMAGE_REL_AMD64_SECREL,
> >>> + IMAGE_REL_AMD64_SECREL7,
> >>> + IMAGE_REL_AMD64_TOKEN,
> >>> + IMAGE_REL_AMD64_SREL32,
> >>> + IMAGE_REL_AMD64_PAIR,
> >>> + IMAGE_REL_AMD64_SSPAN32,
> >>> +};
> >>> +
> >>> +enum arm_coff_reloc_type {
> >>> + IMAGE_REL_ARM_ABSOLUTE,
> >>> + IMAGE_REL_ARM_ADDR32,
> >>> + IMAGE_REL_ARM_ADDR32N,
> >>> + IMAGE_REL_ARM_BRANCH2,
> >>> + IMAGE_REL_ARM_BRANCH1,
> >>> + IMAGE_REL_ARM_SECTION,
> >>> + IMAGE_REL_ARM_SECREL,
> >>> +};
> >>> +
> >>> +enum sh_coff_reloc_type {
> >>> + IMAGE_REL_SH3_ABSOLUTE,
> >>> + IMAGE_REL_SH3_DIRECT16,
> >>> + IMAGE_REL_SH3_DIRECT32,
> >>> + IMAGE_REL_SH3_DIRECT8,
> >>> + IMAGE_REL_SH3_DIRECT8_WORD,
> >>> + IMAGE_REL_SH3_DIRECT8_LONG,
> >>> + IMAGE_REL_SH3_DIRECT4,
> >>> + IMAGE_REL_SH3_DIRECT4_WORD,
> >>> + IMAGE_REL_SH3_DIRECT4_LONG,
> >>> + IMAGE_REL_SH3_PCREL8_WORD,
> >>> + IMAGE_REL_SH3_PCREL8_LONG,
> >>> + IMAGE_REL_SH3_PCREL12_WORD,
> >>> + IMAGE_REL_SH3_STARTOF_SECTION,
> >>> + IMAGE_REL_SH3_SIZEOF_SECTION,
> >>> + IMAGE_REL_SH3_SECTION,
> >>> + IMAGE_REL_SH3_SECREL,
> >>> + IMAGE_REL_SH3_DIRECT32_NB,
> >>> + IMAGE_REL_SH3_GPREL4_LONG,
> >>> + IMAGE_REL_SH3_TOKEN,
> >>> + IMAGE_REL_SHM_PCRELPT,
> >>> + IMAGE_REL_SHM_REFLO,
> >>> + IMAGE_REL_SHM_REFHALF,
> >>> + IMAGE_REL_SHM_RELLO,
> >>> + IMAGE_REL_SHM_RELHALF,
> >>> + IMAGE_REL_SHM_PAIR,
> >>> + IMAGE_REL_SHM_NOMODE,
> >>> +};
> >>> +
> >>> +enum ppc_coff_reloc_type {
> >>> + IMAGE_REL_PPC_ABSOLUTE,
> >>> + IMAGE_REL_PPC_ADDR64,
> >>> + IMAGE_REL_PPC_ADDR32,
> >>> + IMAGE_REL_PPC_ADDR24,
> >>> + IMAGE_REL_PPC_ADDR16,
> >>> + IMAGE_REL_PPC_ADDR14,
> >>> + IMAGE_REL_PPC_REL24,
> >>> + IMAGE_REL_PPC_REL14,
> >>> + IMAGE_REL_PPC_ADDR32N,
> >>> + IMAGE_REL_PPC_SECREL,
> >>> + IMAGE_REL_PPC_SECTION,
> >>> + IMAGE_REL_PPC_SECREL16,
> >>> + IMAGE_REL_PPC_REFHI,
> >>> + IMAGE_REL_PPC_REFLO,
> >>> + IMAGE_REL_PPC_PAIR,
> >>> + IMAGE_REL_PPC_SECRELLO,
> >>> + IMAGE_REL_PPC_GPREL,
> >>> + IMAGE_REL_PPC_TOKEN,
> >>> +};
> >>> +
> >>> +enum x86_coff_reloc_type {
> >>> + IMAGE_REL_I386_ABSOLUTE,
> >>> + IMAGE_REL_I386_DIR16,
> >>> + IMAGE_REL_I386_REL16,
> >>> + IMAGE_REL_I386_DIR32,
> >>> + IMAGE_REL_I386_DIR32NB,
> >>> + IMAGE_REL_I386_SEG12,
> >>> + IMAGE_REL_I386_SECTION,
> >>> + IMAGE_REL_I386_SECREL,
> >>> + IMAGE_REL_I386_TOKEN,
> >>> + IMAGE_REL_I386_SECREL7,
> >>> + IMAGE_REL_I386_REL32,
> >>> +};
> >>> +
> >>> +enum ia64_coff_reloc_type {
> >>> + IMAGE_REL_IA64_ABSOLUTE,
> >>> + IMAGE_REL_IA64_IMM14,
> >>> + IMAGE_REL_IA64_IMM22,
> >>> + IMAGE_REL_IA64_IMM64,
> >>> + IMAGE_REL_IA64_DIR32,
> >>> + IMAGE_REL_IA64_DIR64,
> >>> + IMAGE_REL_IA64_PCREL21B,
> >>> + IMAGE_REL_IA64_PCREL21M,
> >>> + IMAGE_REL_IA64_PCREL21F,
> >>> + IMAGE_REL_IA64_GPREL22,
> >>> + IMAGE_REL_IA64_LTOFF22,
> >>> + IMAGE_REL_IA64_SECTION,
> >>> + IMAGE_REL_IA64_SECREL22,
> >>> + IMAGE_REL_IA64_SECREL64I,
> >>> + IMAGE_REL_IA64_SECREL32,
> >>> + IMAGE_REL_IA64_DIR32NB,
> >>> + IMAGE_REL_IA64_SREL14,
> >>> + IMAGE_REL_IA64_SREL22,
> >>> + IMAGE_REL_IA64_SREL32,
> >>> + IMAGE_REL_IA64_UREL32,
> >>> + IMAGE_REL_IA64_PCREL60X,
> >>> + IMAGE_REL_IA64_PCREL60B,
> >>> + IMAGE_REL_IA64_PCREL60F,
> >>> + IMAGE_REL_IA64_PCREL60I,
> >>> + IMAGE_REL_IA64_PCREL60M,
> >>> + IMAGE_REL_IA64_IMMGPREL6,
> >>> + IMAGE_REL_IA64_TOKEN,
> >>> + IMAGE_REL_IA64_GPREL32,
> >>> + IMAGE_REL_IA64_ADDEND,
> >>> +};
> >>
> >> All sorts of relocation types, but none for RISC-V? Are we going to need
> >> any of this?
> >>
> >>> +struct coff_reloc {
> >>> + uint32_t virtual_address;
> >>> + uint32_t symbol_table_index;
> >>> +
> >>> + union {
> >>> + enum x64_coff_reloc_type x64_type;
> >>> + enum arm_coff_reloc_type arm_type;
> >>> + enum sh_coff_reloc_type sh_type;
> >>> + enum ppc_coff_reloc_type ppc_type;
> >>> + enum x86_coff_reloc_type x86_type;
> >>> + enum ia64_coff_reloc_type ia64_type;
> >>> + uint16_t data;
> >>> + };
> >>> +};
> >>> +
> >>> +/*
> >>> + * Definitions for the contents of the certs data block
> >>> + */
> >>> +#define WIN_CERT_TYPE_PKCS_SIGNED_DATA 0x0002
> >>> +#define WIN_CERT_TYPE_EFI_OKCS115 0x0EF0
> >>> +#define WIN_CERT_TYPE_EFI_GUID 0x0EF1
> >>> +
> >>> +#define WIN_CERT_REVISION_1_0 0x0100
> >>> +#define WIN_CERT_REVISION_2_0 0x0200
> >>> +
> >>> +struct win_certificate {
> >>> + uint32_t length;
> >>> + uint16_t revision;
> >>> + uint16_t cert_type;
> >>> +};
> >>
> >> Or any of this?
> >>
> > Regarding pe.h file content, we wanted to keep it as generic as
> > possible (structures definition according to PE format which can be
> > used for multiple architectures). Specifically for RISC-V as you
> > noticed, we are not using lots of structures (data directories,
> > relocation structures, certificates, etc.). Therefore, we can reduce
> > it to only those used atm, but in that case we won't have a generic PE
> > header definition anymore. Regarding structures which are already
> > defined in common/efi/pe.c, meaning that with our change we have two
> > versions of same structures, we can remove those, but in that case it
> > could be confusing for someone who is trying to map fields from pe.h
> > to actual image header in assembly code. Summary I would keep this
> > header with its original content, but if you think that it contains
> > too much overhead we can reduce it to relevant structures only.
>
> Actually I not only don't mind this header, but would consider it
> superior to the present state of things. Just that then imo it would
> want introducing in a separate commit, with suitable description /
> justification. That could (should) then be followed by a patch using
> this header's struct-s / definitions in pre-existing code, purging
> any duplication from there.
>
So we should have one commit for pe.h only, a subsequent commit where
x86 implementation is modified to use structures from pe.h and a third
commit where we add EFI stub for RISC-V?
And we can submit the first two commits separately from the RISC-V EFI
stub for which we'll wait on @Oleksii opinion on whether EFI
application format is needed upstream?
Just checking if my understanding is correct.
BR,
Milan
On 10.07.2024 16:44, Milan Đokić wrote: > On Mon, Jul 8, 2024 at 11:32 AM Jan Beulich <jbeulich@suse.com> wrote: >> On 04.07.2024 19:21, Milan Đokić wrote: >>> On Wed, Jul 3, 2024 at 5:55 PM Jan Beulich <jbeulich@suse.com> wrote: >>>> On 03.07.2024 02:04, Milan Djokic wrote: >>>>> +#ifdef CONFIG_RISCV_EFI >>>>> + /* >>>>> + * This instruction decodes to "MZ" ASCII required by UEFI. >>>>> + */ >>>>> + c.li s4,-13 >>>>> + c.j xen_start >>>>> +#else >>>>> + /* jump to start kernel */ >>>>> + jal xen_start >>>> >>>> JAL, not J? Why? >>>> >>> We use jal explicitly here to highlight that we want to occupy 32 bits >>> in order to align with header format (and EFI case where we use two >>> 16-bits instructions). Although it will also work by using "j" >>> directly, it could be implicitly compressed to 16 bits (if C extension >>> is available) which would make header layout less obvious imo. >> >> According to e.g. ... >> >>>>> +#endif >>>>> + .balign 8 >>>> >>>> This won't do what you want unless "start" itself is also suitably aligned. >>>> It'll be as long as it's first in the section, but better make such explicit. >>>> >>> I understand, we'll also explicitly align "start" >>> >>>>> +#ifdef CONFIG_RISCV_64 >>>>> + /* Image load offset(2MB) from start of RAM */ >>>>> + .quad 0x200000 >>>>> +#else >>>>> + /* Image load offset(4MB) from start of RAM */ >>>>> + .quad 0x400000 >>>>> +#endif >> >> ... the #ifdef here, you aim at having code be suitable for RV32, too. >> However, JAL has a compressed form there, so its use would make things >> "less obvious" there as well. I'm inclined to say that since the >> subsequent ".balign 8" adds padding NOPs anyway, there's no real >> difference whether that adds 32 bits worth of NOPs or 48 bits. If you >> really wanted to "hide" the difference, imo ".balign 4" would be the >> way to go. >> >> In any event, if there would still be a reason to stick to JAL, you'd >> want to name the reason(s) in a code comment. >> > No specific reason to use jal from a functional point of view, just > aesthetic reasons as I mentioned in a previous comment. Also jal is > used across head.S file, so we also keep some consistency in that > manner. I just don't get the reason why "j" is more suitable as a > pseudo instruction since it also comes down to jal as a base > instruction. > Of course, we can easily switch to "j" here, just want to know why we > are doing so. Consistency with the other path, which uses C.J. Plus avoiding questions along the lines of mine: Why JAL when this isn't a function call, but a plaing branch? >>>>> --- /dev/null >>>>> +++ b/xen/include/efi/pe.h >>>>> [...] >>> Regarding pe.h file content, we wanted to keep it as generic as >>> possible (structures definition according to PE format which can be >>> used for multiple architectures). Specifically for RISC-V as you >>> noticed, we are not using lots of structures (data directories, >>> relocation structures, certificates, etc.). Therefore, we can reduce >>> it to only those used atm, but in that case we won't have a generic PE >>> header definition anymore. Regarding structures which are already >>> defined in common/efi/pe.c, meaning that with our change we have two >>> versions of same structures, we can remove those, but in that case it >>> could be confusing for someone who is trying to map fields from pe.h >>> to actual image header in assembly code. Summary I would keep this >>> header with its original content, but if you think that it contains >>> too much overhead we can reduce it to relevant structures only. >> >> Actually I not only don't mind this header, but would consider it >> superior to the present state of things. Just that then imo it would >> want introducing in a separate commit, with suitable description / >> justification. That could (should) then be followed by a patch using >> this header's struct-s / definitions in pre-existing code, purging >> any duplication from there. >> > So we should have one commit for pe.h only, a subsequent commit where > x86 implementation is modified to use structures from pe.h and a third > commit where we add EFI stub for RISC-V? That's (in my view) the minimum level of splitting, yes. > And we can submit the first two commits separately from the RISC-V EFI > stub for which we'll wait on @Oleksii opinion on whether EFI > application format is needed upstream? Yes. I also wouldn't insist on the conversion part to be done right away, just as long as it gets done not too far in the future. However, since you say "whether": Imo the question isn't whether we need that (the answer there is Yes as long as EFI exists for RISC-V), but what form it should take. If we can properly link an EFI binary, I'd generally consider this preferable over any hand crafting. Yet of course there may be reasons why using the tool chain has to be ruled out. Jan
On Wed, 2024-07-03 at 17:55 +0200, Jan Beulich wrote: > So, first: Please Cc all applicable maintainers. It would probably be > prudent > to also Cc Oleksii, who's doing most of the RISC-V work now (but > Oleksii, > please correct me if you don't want to be Cc-ed). Thanks for adding me and I will be happy for be Cc-ed for RISC-V related patches. I will take a look at the patch next week. Sorry for delay. ~ Oleksii
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.
Note that this patch also represents initial EFI application format support (image
contains EFI application header embeded into binary when CONFIG_RISCV_EFI
is enabled). For full EFI application Xen support, boot/runtime services
and system table handling are yet to be implemented.
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>
---
Changes since v1 (following review comments from Jan Beulich):
* Fix coding style
* Extended image header with all the necessary
PE/COFF (EFI) fields (instead of only those used by
U-boot)
* Removed usage of deprecated types
---
xen/arch/riscv/Kconfig | 9 ++
xen/arch/riscv/include/asm/pe.h | 148 ++++++++++++++++++
.../riscv/include/asm/riscv_image_header.h | 54 +++++++
xen/arch/riscv/riscv64/head.S | 141 ++++++++++++++++-
xen/arch/riscv/xen.lds.S | 6 +-
5 files changed, 356 insertions(+), 2 deletions(-)
create mode 100644 xen/arch/riscv/include/asm/pe.h
create mode 100644 xen/arch/riscv/include/asm/riscv_image_header.h
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/pe.h b/xen/arch/riscv/include/asm/pe.h
new file mode 100644
index 0000000000..084de1e712
--- /dev/null
+++ b/xen/arch/riscv/include/asm/pe.h
@@ -0,0 +1,148 @@
+#ifndef _ASM_RISCV_PE_H
+#define _ASM_RISCV_PE_H
+
+#define LINUX_EFISTUB_MAJOR_VERSION 0x1
+#define LINUX_EFISTUB_MINOR_VERSION 0x0
+
+#define MZ_MAGIC 0x5a4d /* "MZ" */
+
+#define PE_MAGIC 0x00004550 /* "PE\0\0" */
+#define PE_OPT_MAGIC_PE32 0x010b
+#define PE_OPT_MAGIC_PE32PLUS 0x020b
+
+/* machine type */
+#define IMAGE_FILE_MACHINE_RISCV32 0x5032
+#define IMAGE_FILE_MACHINE_RISCV64 0x5064
+
+/* flags */
+#define IMAGE_FILE_EXECUTABLE_IMAGE 0x0002
+#define IMAGE_FILE_LINE_NUMS_STRIPPED 0x0004
+#define IMAGE_FILE_DEBUG_STRIPPED 0x0200
+#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10
+
+#define IMAGE_SCN_CNT_CODE 0x00000020 /* .text */
+#define IMAGE_SCN_CNT_INITIALIZED_DATA 0x00000040 /* .data */
+#define IMAGE_SCN_MEM_EXECUTE 0x20000000
+#define IMAGE_SCN_MEM_READ 0x40000000 /* readable */
+#define IMAGE_SCN_MEM_WRITE 0x80000000 /* writeable */
+
+#ifndef __ASSEMBLY__
+
+struct mz_hdr {
+ uint16_t magic; /* MZ_MAGIC */
+ uint16_t lbsize; /* size of last used block */
+ uint16_t blocks; /* pages in file, 0x3 */
+ uint16_t relocs; /* relocations */
+ uint16_t hdrsize; /* header size in "paragraphs" */
+ uint16_t min_extra_pps; /* .bss */
+ uint16_t max_extra_pps; /* runtime limit for the arena size */
+ uint16_t ss; /* relative stack segment */
+ uint16_t sp; /* initial %sp register */
+ uint16_t checksum; /* word checksum */
+ uint16_t ip; /* initial %ip register */
+ uint16_t cs; /* initial %cs relative to load segment */
+ uint16_t reloc_table_offset; /* offset of the first relocation */
+ uint16_t overlay_num;
+ uint16_t reserved0[4];
+ uint16_t oem_id;
+ uint16_t oem_info;
+ uint16_t reserved1[10];
+ uint32_t peaddr; /* address of pe header */
+ char message[]; /* message to print */
+};
+
+struct pe_hdr {
+ uint32_t magic; /* PE magic */
+ uint16_t machine; /* machine type */
+ uint16_t sections; /* number of sections */
+ uint32_t timestamp;
+ uint32_t symbol_table; /* symbol table offset */
+ uint32_t symbols; /* number of symbols */
+ uint16_t opt_hdr_size; /* size of optional header */
+ uint16_t flags; /* flags */
+};
+
+struct pe32_opt_hdr {
+ /* "standard" header */
+ uint16_t magic; /* file type */
+ uint8_t ld_major; /* linker major version */
+ uint8_t ld_minor; /* linker minor version */
+ uint32_t text_size;
+ uint32_t data_size;
+ uint32_t bss_size;
+ uint32_t entry_point; /* file offset of entry point */
+ uint32_t code_base; /* relative code addr in ram */
+ uint32_t data_base; /* relative data addr in ram */
+ /* "extra" header fields */
+ uint32_t image_base; /* preferred load address */
+ uint32_t section_align; /* alignment in bytes */
+ uint32_t file_align; /* file alignment in bytes */
+ uint16_t os_major;
+ uint16_t os_minor;
+ uint16_t image_major;
+ uint16_t image_minor;
+ uint16_t subsys_major;
+ uint16_t subsys_minor;
+ uint32_t win32_version; /* reserved, must be 0 */
+ uint32_t image_size;
+ uint32_t header_size;
+ uint32_t csum;
+ uint16_t subsys;
+ uint16_t dll_flags;
+ uint32_t stack_size_req; /* amt of stack requested */
+ uint32_t stack_size; /* amt of stack required */
+ uint32_t heap_size_req; /* amt of heap requested */
+ uint32_t heap_size; /* amt of heap required */
+ uint32_t loader_flags; /* reserved, must be 0 */
+ uint32_t data_dirs; /* number of data dir entries */
+};
+
+struct pe32plus_opt_hdr {
+ uint16_t magic; /* file type */
+ uint8_t ld_major; /* linker major version */
+ uint8_t ld_minor; /* linker minor version */
+ uint32_t text_size;
+ uint32_t data_size;
+ uint32_t bss_size;
+ uint32_t entry_point; /* file offset of entry point */
+ uint32_t code_base; /* relative code addr in ram */
+ /* "extra" header fields */
+ uint64_t image_base; /* preferred load address */
+ uint32_t section_align; /* alignment in bytes */
+ uint32_t file_align; /* file alignment in bytes */
+ uint16_t os_major;
+ uint16_t os_minor;
+ uint16_t image_major;
+ uint16_t image_minor;
+ uint16_t subsys_major;
+ uint16_t subsys_minor;
+ uint32_t win32_version; /* reserved, must be 0 */
+ uint32_t image_size;
+ uint32_t header_size;
+ uint32_t csum;
+ uint16_t subsys;
+ uint16_t dll_flags;
+ uint64_t stack_size_req; /* amt of stack requested */
+ uint64_t stack_size; /* amt of stack required */
+ uint64_t heap_size_req; /* amt of heap requested */
+ uint64_t heap_size; /* amt of heap required */
+ uint32_t loader_flags; /* reserved, must be 0 */
+ uint32_t data_dirs; /* number of data dir entries */
+};
+
+struct section_header {
+ char name[8]; /* name or "/12\0" string tbl offset */
+ uint32_t virtual_size; /* size of loaded section in ram */
+ uint32_t virtual_address; /* relative virtual address */
+ uint32_t raw_data_size; /* size of the section */
+ uint32_t data_addr; /* file pointer to first page of section */
+ uint32_t relocs; /* file pointer to relocation entries */
+ uint32_t line_numbers;
+ uint16_t num_relocs;
+ uint16_t num_lin_numbers;
+ uint32_t flags;
+};
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ASM_RISCV_PE_H */
diff --git a/xen/arch/riscv/include/asm/riscv_image_header.h b/xen/arch/riscv/include/asm/riscv_image_header.h
new file mode 100644
index 0000000000..89c7511d56
--- /dev/null
+++ b/xen/arch/riscv/include/asm/riscv_image_header.h
@@ -0,0 +1,54 @@
+#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_LE 0
+#define RISCV_IMAGE_FLAG_BE 1
+
+#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_LE
+
+#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
+{
+ uint32_t code0;
+ uint32_t code1;
+ uint64_t text_offset;
+ uint64_t image_size;
+ uint64_t res1;
+ uint64_t res2;
+ uint64_t res3;
+ uint64_t magic;
+ uint32_t res4;
+ uint32_t res5;
+};
+#endif /* __ASSEMBLY__ */
+#endif /* _ASM_RISCV_IMAGE_H */
diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
index 3261e9fce8..609638b921 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -1,14 +1,150 @@
#include <asm/asm.h>
#include <asm/riscv_encoding.h>
+#include <asm/riscv_image_header.h>
+#ifdef CONFIG_RISCV_EFI
+#include <asm/pe.h>
+#endif
.section .text.header, "ax", %progbits
/*
* OpenSBI pass to start():
* a0 -> hart_id ( bootcpu_id )
- * a1 -> dtb_base
+ * a1 -> dtb_base
*/
FUNC(start)
+
+efi_head:
+
+#ifdef CONFIG_RISCV_EFI
+ /*
+ * This instruction decodes to "MZ" ASCII required by UEFI.
+ */
+ c.li s4,-13
+ j xen_start
+#else
+ /* jump to start kernel */
+ j xen_start
+ /* reserved */
+ .word 0
+#endif
+ .balign 8
+#ifdef CONFIG_RISCV_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
+#ifndef CONFIG_RISCV_EFI
+ .word 0
+#else
+ .word pe_head_start - efi_head
+pe_head_start:
+ .long PE_MAGIC
+coff_header:
+#ifdef CONFIG_RISCV_64
+ .short IMAGE_FILE_MACHINE_RISCV64 /* Machine */
+#else
+ .short IMAGE_FILE_MACHINE_RISCV32 /* Machine */
+#endif
+ .short section_count /* NumberOfSections */
+ .long 0 /* TimeDateStamp */
+ .long 0 /* PointerToSymbolTable */
+ .long 0 /* NumberOfSymbols */
+ .short section_table - optional_header /* SizeOfOptionalHeader */
+ .short IMAGE_FILE_DEBUG_STRIPPED | \
+ IMAGE_FILE_EXECUTABLE_IMAGE | \
+ IMAGE_FILE_LINE_NUMS_STRIPPED /* Characteristics */
+
+optional_header:
+#ifdef CONFIG_RISCV_64
+ .short PE_OPT_MAGIC_PE32PLUS /* PE32+ format */
+#else
+ .short PE_OPT_MAGIC_PE32 /* PE32 format */
+#endif
+ .byte 0x02 /* MajorLinkerVersion */
+ .byte 0x14 /* MinorLinkerVersion */
+ .long _end - xen_start /* SizeOfCode */
+ .long 0 /* SizeOfInitializedData */
+ .long 0 /* SizeOfUninitializedData */
+ .long 0 /* AddressOfEntryPoint */
+ .long xen_start - efi_head /* BaseOfCode */
+
+extra_header_fields:
+ .quad 0 /* ImageBase */
+ .long PECOFF_SECTION_ALIGNMENT /* SectionAlignment */
+ .long PECOFF_FILE_ALIGNMENT /* FileAlignment */
+ .short 0 /* MajorOperatingSystemVersion */
+ .short 0 /* MinorOperatingSystemVersion */
+ .short LINUX_EFISTUB_MAJOR_VERSION /* MajorImageVersion */
+ .short LINUX_EFISTUB_MINOR_VERSION /* MinorImageVersion */
+ .short 0 /* MajorSubsystemVersion */
+ .short 0 /* MinorSubsystemVersion */
+ .long 0 /* Win32VersionValue */
+ .long _end - efi_head /* SizeOfImage */
+
+ /* Everything before the xen image is considered part of the header */
+ .long xen_start - efi_head /* SizeOfHeaders */
+ .long 0 /* CheckSum */
+ .short IMAGE_SUBSYSTEM_EFI_APPLICATION /* Subsystem */
+ .short 0 /* DllCharacteristics */
+ .quad 0 /* SizeOfStackReserve */
+ .quad 0 /* SizeOfStackCommit */
+ .quad 0 /* SizeOfHeapReserve */
+ .quad 0 /* SizeOfHeapCommit */
+ .long 0 /* LoaderFlags */
+ .long (section_table - .) / 8 /* NumberOfRvaAndSizes */
+ .quad 0 /* ExportTable */
+ .quad 0 /* ImportTable */
+ .quad 0 /* ResourceTable */
+ .quad 0 /* ExceptionTable */
+ .quad 0 /* CertificationTable */
+ .quad 0 /* BaseRelocationTable */
+
+/* Section table */
+section_table:
+ .ascii ".text\0\0\0"
+ .long 0
+ .long 0
+ .long 0 /* SizeOfRawData */
+ .long 0 /* PointerToRawData */
+ .long 0 /* PointerToRelocations */
+ .long 0 /* PointerToLineNumbers */
+ .short 0 /* NumberOfRelocations */
+ .short 0 /* NumberOfLineNumbers */
+ .long IMAGE_SCN_CNT_CODE | \
+ IMAGE_SCN_MEM_READ | \
+ IMAGE_SCN_MEM_EXECUTE /* Characteristics */
+
+ .ascii ".data\0\0\0"
+ .long _end - xen_start /* VirtualSize */
+ .long xen_start - efi_head /* VirtualAddress */
+ .long __init_end_efi - xen_start /* SizeOfRawData */
+ .long xen_start - efi_head /* PointerToRawData */
+ .long 0 /* PointerToRelocations */
+ .long 0 /* PointerToLineNumbers */
+ .short 0 /* NumberOfRelocations */
+ .short 0 /* NumberOfLineNumbers */
+ .long IMAGE_SCN_CNT_INITIALIZED_DATA | \
+ IMAGE_SCN_MEM_READ | \
+ IMAGE_SCN_MEM_WRITE /* Characteristics */
+
+ .set section_count, (. - section_table) / 40
+
+ .balign 0x1000
+#endif/* CONFIG_RISCV_EFI */
+
+FUNC(xen_start)
/* Mask all interrupts */
csrw CSR_SIE, zero
@@ -60,6 +196,9 @@ FUNC(start)
mv a1, s1
tail start_xen
+
+END(xen_start)
+
END(start)
.section .text, "ax", %progbits
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index 8510a87c4d..2eddde43c1 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -12,6 +12,9 @@ PHDRS
#endif
}
+PECOFF_SECTION_ALIGNMENT = 0x1000;
+PECOFF_FILE_ALIGNMENT = 0x200;
+
SECTIONS
{
. = XEN_VIRT_START;
@@ -144,7 +147,7 @@ SECTIONS
.got.plt : {
*(.got.plt)
} : text
-
+ __init_end_efi = .;
. = ALIGN(POINTER_ALIGN);
__init_end = .;
@@ -165,6 +168,7 @@ SECTIONS
. = ALIGN(POINTER_ALIGN);
__bss_end = .;
} :text
+
_end = . ;
/* Section for the device tree blob (if any). */
--
2.25.1On 12.06.2024 14:15, milandjokic1995@gmail.com wrote:
> 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.
> Note that this patch also represents initial EFI application format support (image
> contains EFI application header embeded into binary when CONFIG_RISCV_EFI
> is enabled). For full EFI application Xen support, boot/runtime services
> and system table handling are yet to be implemented.
>
> 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>
This isn't you, is it? Your S-o-b is going to be needed, too.
> --- 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
Nit: This line can be omitted (and if I'm not mistaken we generally do omit
such).
> + 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.
I don't think my v1 comment on this was addressed.
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/pe.h
> @@ -0,0 +1,148 @@
> +#ifndef _ASM_RISCV_PE_H
> +#define _ASM_RISCV_PE_H
> +
> +#define LINUX_EFISTUB_MAJOR_VERSION 0x1
> +#define LINUX_EFISTUB_MINOR_VERSION 0x0
> +
> +#define MZ_MAGIC 0x5a4d /* "MZ" */
> +
> +#define PE_MAGIC 0x00004550 /* "PE\0\0" */
> +#define PE_OPT_MAGIC_PE32 0x010b
> +#define PE_OPT_MAGIC_PE32PLUS 0x020b
> +
> +/* machine type */
> +#define IMAGE_FILE_MACHINE_RISCV32 0x5032
> +#define IMAGE_FILE_MACHINE_RISCV64 0x5064
Apart from this, hardly anything in this header is RISC-V specific.
Please consider moving to xen/include/xen/.
> +/* flags */
> +#define IMAGE_FILE_EXECUTABLE_IMAGE 0x0002
> +#define IMAGE_FILE_LINE_NUMS_STRIPPED 0x0004
> +#define IMAGE_FILE_DEBUG_STRIPPED 0x0200
> +#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10
> +
> +#define IMAGE_SCN_CNT_CODE 0x00000020 /* .text */
> +#define IMAGE_SCN_CNT_INITIALIZED_DATA 0x00000040 /* .data */
> +#define IMAGE_SCN_MEM_EXECUTE 0x20000000
> +#define IMAGE_SCN_MEM_READ 0x40000000 /* readable */
> +#define IMAGE_SCN_MEM_WRITE 0x80000000 /* writeable */
> +
> +#ifndef __ASSEMBLY__
> +
> +struct mz_hdr {
> + uint16_t magic; /* MZ_MAGIC */
> + uint16_t lbsize; /* size of last used block */
> + uint16_t blocks; /* pages in file, 0x3 */
> + uint16_t relocs; /* relocations */
> + uint16_t hdrsize; /* header size in "paragraphs" */
> + uint16_t min_extra_pps; /* .bss */
> + uint16_t max_extra_pps; /* runtime limit for the arena size */
> + uint16_t ss; /* relative stack segment */
> + uint16_t sp; /* initial %sp register */
> + uint16_t checksum; /* word checksum */
> + uint16_t ip; /* initial %ip register */
> + uint16_t cs; /* initial %cs relative to load segment */
> + uint16_t reloc_table_offset; /* offset of the first relocation */
> + uint16_t overlay_num;
> + uint16_t reserved0[4];
> + uint16_t oem_id;
> + uint16_t oem_info;
> + uint16_t reserved1[10];
> + uint32_t peaddr; /* address of pe header */
> + char message[]; /* message to print */
> +};
> +
> +struct pe_hdr {
> + uint32_t magic; /* PE magic */
> + uint16_t machine; /* machine type */
> + uint16_t sections; /* number of sections */
> + uint32_t timestamp;
> + uint32_t symbol_table; /* symbol table offset */
> + uint32_t symbols; /* number of symbols */
> + uint16_t opt_hdr_size; /* size of optional header */
> + uint16_t flags; /* flags */
> +};
> +
> +struct pe32_opt_hdr {
> + /* "standard" header */
> + uint16_t magic; /* file type */
> + uint8_t ld_major; /* linker major version */
> + uint8_t ld_minor; /* linker minor version */
> + uint32_t text_size;
> + uint32_t data_size;
> + uint32_t bss_size;
> + uint32_t entry_point; /* file offset of entry point */
> + uint32_t code_base; /* relative code addr in ram */
> + uint32_t data_base; /* relative data addr in ram */
> + /* "extra" header fields */
> + uint32_t image_base; /* preferred load address */
> + uint32_t section_align; /* alignment in bytes */
> + uint32_t file_align; /* file alignment in bytes */
> + uint16_t os_major;
> + uint16_t os_minor;
> + uint16_t image_major;
> + uint16_t image_minor;
> + uint16_t subsys_major;
> + uint16_t subsys_minor;
> + uint32_t win32_version; /* reserved, must be 0 */
> + uint32_t image_size;
> + uint32_t header_size;
> + uint32_t csum;
> + uint16_t subsys;
> + uint16_t dll_flags;
> + uint32_t stack_size_req; /* amt of stack requested */
> + uint32_t stack_size; /* amt of stack required */
> + uint32_t heap_size_req; /* amt of heap requested */
> + uint32_t heap_size; /* amt of heap required */
> + uint32_t loader_flags; /* reserved, must be 0 */
> + uint32_t data_dirs; /* number of data dir entries */
> +};
> +
> +struct pe32plus_opt_hdr {
> + uint16_t magic; /* file type */
> + uint8_t ld_major; /* linker major version */
> + uint8_t ld_minor; /* linker minor version */
> + uint32_t text_size;
> + uint32_t data_size;
> + uint32_t bss_size;
> + uint32_t entry_point; /* file offset of entry point */
> + uint32_t code_base; /* relative code addr in ram */
> + /* "extra" header fields */
> + uint64_t image_base; /* preferred load address */
> + uint32_t section_align; /* alignment in bytes */
> + uint32_t file_align; /* file alignment in bytes */
> + uint16_t os_major;
> + uint16_t os_minor;
> + uint16_t image_major;
> + uint16_t image_minor;
> + uint16_t subsys_major;
> + uint16_t subsys_minor;
> + uint32_t win32_version; /* reserved, must be 0 */
> + uint32_t image_size;
> + uint32_t header_size;
> + uint32_t csum;
> + uint16_t subsys;
> + uint16_t dll_flags;
> + uint64_t stack_size_req; /* amt of stack requested */
> + uint64_t stack_size; /* amt of stack required */
> + uint64_t heap_size_req; /* amt of heap requested */
> + uint64_t heap_size; /* amt of heap required */
> + uint32_t loader_flags; /* reserved, must be 0 */
> + uint32_t data_dirs; /* number of data dir entries */
> +};
> +
> +struct section_header {
> + char name[8]; /* name or "/12\0" string tbl offset */
Why 12?
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/riscv_image_header.h
Is this file taken from somewhere else, kind of making it desirable to keep
its name in sync with the original? Otherwise: We prefer dashes over underscores
in new files' names.
> @@ -0,0 +1,54 @@
> +#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_LE 0
> +#define RISCV_IMAGE_FLAG_BE 1
> +
> +#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_LE
> +
> +#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
You saying "xen": Is there anything Xen-specific in this struct?
> + * @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
> +{
> + uint32_t code0;
> + uint32_t code1;
> + uint64_t text_offset;
> + uint64_t image_size;
> + uint64_t res1;
> + uint64_t res2;
> + uint64_t res3;
> + uint64_t magic;
> + uint32_t res4;
> + uint32_t res5;
> +};
> +#endif /* __ASSEMBLY__ */
> +#endif /* _ASM_RISCV_IMAGE_H */
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -1,14 +1,150 @@
> #include <asm/asm.h>
> #include <asm/riscv_encoding.h>
> +#include <asm/riscv_image_header.h>
> +#ifdef CONFIG_RISCV_EFI
> +#include <asm/pe.h>
> +#endif
>
> .section .text.header, "ax", %progbits
>
> /*
> * OpenSBI pass to start():
> * a0 -> hart_id ( bootcpu_id )
> - * a1 -> dtb_base
> + * a1 -> dtb_base
> */
> FUNC(start)
> +
> +efi_head:
Why is this ...
> +#ifdef CONFIG_RISCV_EFI
... ahead of this?
> + /*
> + * This instruction decodes to "MZ" ASCII required by UEFI.
> + */
> + c.li s4,-13
IOW RISCV_EFI ought to be (made) dependent upon RISCV_ISA_C?
> + j xen_start
Doesn't this then need to be c.j, to be sure it fits in 16 bits (and
raise an assembler error if not)?
> +#else
> + /* jump to start kernel */
> + j xen_start
> + /* reserved */
> + .word 0
What struct field does this correspond to? Or if not a struct field,
why is this needed?
Also I can't help the impression that the resulting layout will be
different depending on whether RISCV_ISA_C is enabled, as the "j"
may translate to a 16-bit or 32-bit insn.
I wonder anyway what use everything from here to ...
> +#endif
> + .balign 8
> +#ifdef CONFIG_RISCV_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
> +#ifndef CONFIG_RISCV_EFI
> + .word 0
... here is when RISCV_EFI=n. You add data which wasn't needed so far,
and for which it also isn't explained why it would suddenly be needed.
I also can't bring several of the fields above in sync with the
struct riscv_image_header definition in the header. Please can you
annotate each field with a comment naming the corresponding C struct
field (like you do further down, at least in a way)?
> +#else
> + .word pe_head_start - efi_head
> +pe_head_start:
> + .long PE_MAGIC
> +coff_header:
> +#ifdef CONFIG_RISCV_64
> + .short IMAGE_FILE_MACHINE_RISCV64 /* Machine */
> +#else
> + .short IMAGE_FILE_MACHINE_RISCV32 /* Machine */
> +#endif
> + .short section_count /* NumberOfSections */
> + .long 0 /* TimeDateStamp */
> + .long 0 /* PointerToSymbolTable */
> + .long 0 /* NumberOfSymbols */
> + .short section_table - optional_header /* SizeOfOptionalHeader */
> + .short IMAGE_FILE_DEBUG_STRIPPED | \
> + IMAGE_FILE_EXECUTABLE_IMAGE | \
> + IMAGE_FILE_LINE_NUMS_STRIPPED /* Characteristics */
> +
> +optional_header:
> +#ifdef CONFIG_RISCV_64
> + .short PE_OPT_MAGIC_PE32PLUS /* PE32+ format */
> +#else
> + .short PE_OPT_MAGIC_PE32 /* PE32 format */
> +#endif
> + .byte 0x02 /* MajorLinkerVersion */
> + .byte 0x14 /* MinorLinkerVersion */
> + .long _end - xen_start /* SizeOfCode */
> + .long 0 /* SizeOfInitializedData */
> + .long 0 /* SizeOfUninitializedData */
> + .long 0 /* AddressOfEntryPoint */
> + .long xen_start - efi_head /* BaseOfCode */
> +
> +extra_header_fields:
> + .quad 0 /* ImageBase */
This is quad only for PE32+, iirc. In PE32 it's two separate 32-bit
fields instead. The overall result may be tolerable (a data RVA of 0
can't be quite right, but we may be able to get away with that), but
it will at least want commenting on.
Any anyway - further up in the RISC-V header struct you use .word and
.dword. Why .long and .quad here? That's at least somewhat confusing.
> + .long PECOFF_SECTION_ALIGNMENT /* SectionAlignment */
> + .long PECOFF_FILE_ALIGNMENT /* FileAlignment */
> + .short 0 /* MajorOperatingSystemVersion */
> + .short 0 /* MinorOperatingSystemVersion */
> + .short LINUX_EFISTUB_MAJOR_VERSION /* MajorImageVersion */
> + .short LINUX_EFISTUB_MINOR_VERSION /* MinorImageVersion */
> + .short 0 /* MajorSubsystemVersion */
> + .short 0 /* MinorSubsystemVersion */
> + .long 0 /* Win32VersionValue */
> + .long _end - efi_head /* SizeOfImage */
> +
> + /* Everything before the xen image is considered part of the header */
> + .long xen_start - efi_head /* SizeOfHeaders */
> + .long 0 /* CheckSum */
> + .short IMAGE_SUBSYSTEM_EFI_APPLICATION /* Subsystem */
> + .short 0 /* DllCharacteristics */
> + .quad 0 /* SizeOfStackReserve */
> + .quad 0 /* SizeOfStackCommit */
> + .quad 0 /* SizeOfHeapReserve */
> + .quad 0 /* SizeOfHeapCommit */
All of these are again 32 bits only in PE32, if I'm not mistaken.
> + .long 0 /* LoaderFlags */
> + .long (section_table - .) / 8 /* NumberOfRvaAndSizes */
> + .quad 0 /* ExportTable */
> + .quad 0 /* ImportTable */
> + .quad 0 /* ResourceTable */
> + .quad 0 /* ExceptionTable */
> + .quad 0 /* CertificationTable */
> + .quad 0 /* BaseRelocationTable */
Would you mind clarifying on what basis this set of 6 entries was
chosen?
> +/* Section table */
> +section_table:
> + .ascii ".text\0\0\0"
> + .long 0
> + .long 0
> + .long 0 /* SizeOfRawData */
> + .long 0 /* PointerToRawData */
> + .long 0 /* PointerToRelocations */
> + .long 0 /* PointerToLineNumbers */
> + .short 0 /* NumberOfRelocations */
> + .short 0 /* NumberOfLineNumbers */
> + .long IMAGE_SCN_CNT_CODE | \
> + IMAGE_SCN_MEM_READ | \
> + IMAGE_SCN_MEM_EXECUTE /* Characteristics */
> +
> + .ascii ".data\0\0\0"
> + .long _end - xen_start /* VirtualSize */
> + .long xen_start - efi_head /* VirtualAddress */
> + .long __init_end_efi - xen_start /* SizeOfRawData */
> + .long xen_start - efi_head /* PointerToRawData */
> + .long 0 /* PointerToRelocations */
> + .long 0 /* PointerToLineNumbers */
> + .short 0 /* NumberOfRelocations */
> + .short 0 /* NumberOfLineNumbers */
> + .long IMAGE_SCN_CNT_INITIALIZED_DATA | \
> + IMAGE_SCN_MEM_READ | \
> + IMAGE_SCN_MEM_WRITE /* Characteristics */
IOW no code and the entire image expressed as data. Interesting.
No matter whether that has a reason or is completely arbitrary, I
think it, too, wants commenting on.
> + .set section_count, (. - section_table) / 40
> +
> + .balign 0x1000
> +#endif/* CONFIG_RISCV_EFI */
> +
> +FUNC(xen_start)
> /* Mask all interrupts */
> csrw CSR_SIE, zero
>
> @@ -60,6 +196,9 @@ FUNC(start)
> mv a1, s1
>
> tail start_xen
> +
> +END(xen_start)
> +
> END(start)
I don't think you addressed my function nesting comment here either.
> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -12,6 +12,9 @@ PHDRS
> #endif
> }
>
> +PECOFF_SECTION_ALIGNMENT = 0x1000;
> +PECOFF_FILE_ALIGNMENT = 0x200;
> +
> SECTIONS
> {
> . = XEN_VIRT_START;
> @@ -144,7 +147,7 @@ SECTIONS
> .got.plt : {
> *(.got.plt)
> } : text
> -
> + __init_end_efi = .;
Why does the blank line disappear? And why is ...
> . = ALIGN(POINTER_ALIGN);
> __init_end = .;
... __init_end not good enough? (I think I can guess the answer, but
then I further think the name of the symbol is misleading. )
> @@ -165,6 +168,7 @@ SECTIONS
> . = ALIGN(POINTER_ALIGN);
> __bss_end = .;
> } :text
> +
> _end = . ;
Interestingly an unrelated blank line suddenly appears here.
Jan
> Signed-off-by: Nikola Jelic <nikola.jelic@rt-rk.com>
This isn't you, is it? Your S-o-b is going to be needed, too.
Nikola.jelic@rt-rk.com is the initial author of the patch, I'll add myself also if necessary
> +config RISCV_EFI
> + bool "UEFI boot service support"
> + depends on RISCV_64
> + default n
Nit: This line can be omitted (and if I'm not mistaken we generally do omit
such).
If we remove the default value, EFI header shall be included into xen image by default. We want to keep it as optional for now, and generate plain elf as default format (until full EFI support is implemented)
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/pe.h
> @@ -0,0 +1,148 @@
> +#ifndef _ASM_RISCV_PE_H
> +#define _ASM_RISCV_PE_H
> +
> +#define LINUX_EFISTUB_MAJOR_VERSION 0x1
> +#define LINUX_EFISTUB_MINOR_VERSION 0x0
> +
> +#define MZ_MAGIC 0x5a4d /* "MZ" */
> +
> +#define PE_MAGIC 0x00004550 /* "PE\0\0" */
> +#define PE_OPT_MAGIC_PE32 0x010b
> +#define PE_OPT_MAGIC_PE32PLUS 0x020b
> +
> +/* machine type */
> +#define IMAGE_FILE_MACHINE_RISCV32 0x5032
> +#define IMAGE_FILE_MACHINE_RISCV64 0x5064
Apart from this, hardly anything in this header is RISC-V specific.
Please consider moving to xen/include/xen/.
We shall move generic part into xen/include/xen/efi. This is something which should be considered for use on arm/x86 also. Currently PE/COFF header is directly embedded into
head.S for arm/x86
> + char name[8]; /* name or "/12\0" string tbl offset */
Why 12?
Either section name is specified in this field or string table offset if section name can't fit into 8 bytes, which is the case here. Btw this is taken over from linux kernel together with the PE/COFF header structures: https://github.com/torvalds/linux/blob/master/include/linux/pe.h
> + * struct riscv_image_header - riscv xen image header
You saying "xen": Is there anything Xen-specific in this struct?
Not really related to xen, this is generic riscv PE image header, comment fixed in new version
> + .long 0 /* LoaderFlags */
> + .long (section_table - .) / 8 /* NumberOfRvaAndSizes */
> + .quad 0 /* ExportTable */
> + .quad 0 /* ImportTable */
> + .quad 0 /* ResourceTable */
> + .quad 0 /* ExceptionTable */
> + .quad 0 /* CertificationTable */
> + .quad 0 /* BaseRelocationTable */
Would you mind clarifying on what basis this set of 6 entries was
chosen?
These fields and their sizes are defined in official PE format, see details from specification bellow
[cid:542690de-3bb0-4708-a447-996a03277578]
> +/* Section table */
> +section_table:
> + .ascii ".text\0\0\0"
> + .long 0
> + .long 0
> + .long 0 /* SizeOfRawData */
> + .long 0 /* PointerToRawData */
> + .long 0 /* PointerToRelocations */
> + .long 0 /* PointerToLineNumbers */
> + .short 0 /* NumberOfRelocations */
> + .short 0 /* NumberOfLineNumbers */
> + .long IMAGE_SCN_CNT_CODE | \
> + IMAGE_SCN_MEM_READ | \
> + IMAGE_SCN_MEM_EXECUTE /* Characteristics */
> +
> + .ascii ".data\0\0\0"
> + .long _end - xen_start /* VirtualSize */
> + .long xen_start - efi_head /* VirtualAddress */
> + .long __init_end_efi - xen_start /* SizeOfRawData */
> + .long xen_start - efi_head /* PointerToRawData */
> + .long 0 /* PointerToRelocations */
> + .long 0 /* PointerToLineNumbers */
> + .short 0 /* NumberOfRelocations */
> + .short 0 /* NumberOfLineNumbers */
> + .long IMAGE_SCN_CNT_INITIALIZED_DATA | \
> + IMAGE_SCN_MEM_READ | \
> + IMAGE_SCN_MEM_WRITE /* Characteristics */
IOW no code and the entire image expressed as data. Interesting.
No matter whether that has a reason or is completely arbitrary, I
think it, too, wants commenting on.
This is correct, currently we have extended image with PE/COFF (EFI) header which allows xen boot from EFI loader (or U-boot) environment. And these updates are pure data. We are actively working on the implementation of Boot/Runtime services which shall be in the code section part and enable full UEFI compatible xen application for riscv.
Why does the blank line disappear? And why is ...
> . = ALIGN(POINTER_ALIGN);
> __init_end = .;
... __init_end not good enough? (I think I can guess the answer, but
then I further think the name of the symbol is misleading. )
Init_end_efi is used only when EFI sections are included into image. We have aligned with arm implementation here, you can take a look also there.
Regarding other comments, we've fixed our code structure following kernel EFI app implementation for RISC-V. This will be obvious in the updated patch itself, just wanted to address comments which need additional explanation here before submitting new patch version.
Best regards,
Milan
________________________________
From: Jan Beulich <jbeulich@suse.com>
Sent: Thursday, June 13, 2024 2:59 PM
To: milandjokic1995@gmail.com <milandjokic1995@gmail.com>
Cc: Milan Djokic <milan.djokic@rt-rk.com>; Nikola Jelic <nikola.jelic@rt-rk.com>; Alistair Francis <alistair.francis@wdc.com>; Bob Eshleman <bobbyeshleman@gmail.com>; Connor Davis <connojdavis@gmail.com>; Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH] xen/riscv: PE/COFF image header for RISC-V target
On 12.06.2024 14:15, milandjokic1995@gmail.com wrote:
> 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.
> Note that this patch also represents initial EFI application format support (image
> contains EFI application header embeded into binary when CONFIG_RISCV_EFI
> is enabled). For full EFI application Xen support, boot/runtime services
> and system table handling are yet to be implemented.
>
> 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>
This isn't you, is it? Your S-o-b is going to be needed, too.
> --- 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
Nit: This line can be omitted (and if I'm not mistaken we generally do omit
such).
> + 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.
I don't think my v1 comment on this was addressed.
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/pe.h
> @@ -0,0 +1,148 @@
> +#ifndef _ASM_RISCV_PE_H
> +#define _ASM_RISCV_PE_H
> +
> +#define LINUX_EFISTUB_MAJOR_VERSION 0x1
> +#define LINUX_EFISTUB_MINOR_VERSION 0x0
> +
> +#define MZ_MAGIC 0x5a4d /* "MZ" */
> +
> +#define PE_MAGIC 0x00004550 /* "PE\0\0" */
> +#define PE_OPT_MAGIC_PE32 0x010b
> +#define PE_OPT_MAGIC_PE32PLUS 0x020b
> +
> +/* machine type */
> +#define IMAGE_FILE_MACHINE_RISCV32 0x5032
> +#define IMAGE_FILE_MACHINE_RISCV64 0x5064
Apart from this, hardly anything in this header is RISC-V specific.
Please consider moving to xen/include/xen/.
> +/* flags */
> +#define IMAGE_FILE_EXECUTABLE_IMAGE 0x0002
> +#define IMAGE_FILE_LINE_NUMS_STRIPPED 0x0004
> +#define IMAGE_FILE_DEBUG_STRIPPED 0x0200
> +#define IMAGE_SUBSYSTEM_EFI_APPLICATION 10
> +
> +#define IMAGE_SCN_CNT_CODE 0x00000020 /* .text */
> +#define IMAGE_SCN_CNT_INITIALIZED_DATA 0x00000040 /* .data */
> +#define IMAGE_SCN_MEM_EXECUTE 0x20000000
> +#define IMAGE_SCN_MEM_READ 0x40000000 /* readable */
> +#define IMAGE_SCN_MEM_WRITE 0x80000000 /* writeable */
> +
> +#ifndef __ASSEMBLY__
> +
> +struct mz_hdr {
> + uint16_t magic; /* MZ_MAGIC */
> + uint16_t lbsize; /* size of last used block */
> + uint16_t blocks; /* pages in file, 0x3 */
> + uint16_t relocs; /* relocations */
> + uint16_t hdrsize; /* header size in "paragraphs" */
> + uint16_t min_extra_pps; /* .bss */
> + uint16_t max_extra_pps; /* runtime limit for the arena size */
> + uint16_t ss; /* relative stack segment */
> + uint16_t sp; /* initial %sp register */
> + uint16_t checksum; /* word checksum */
> + uint16_t ip; /* initial %ip register */
> + uint16_t cs; /* initial %cs relative to load segment */
> + uint16_t reloc_table_offset; /* offset of the first relocation */
> + uint16_t overlay_num;
> + uint16_t reserved0[4];
> + uint16_t oem_id;
> + uint16_t oem_info;
> + uint16_t reserved1[10];
> + uint32_t peaddr; /* address of pe header */
> + char message[]; /* message to print */
> +};
> +
> +struct pe_hdr {
> + uint32_t magic; /* PE magic */
> + uint16_t machine; /* machine type */
> + uint16_t sections; /* number of sections */
> + uint32_t timestamp;
> + uint32_t symbol_table; /* symbol table offset */
> + uint32_t symbols; /* number of symbols */
> + uint16_t opt_hdr_size; /* size of optional header */
> + uint16_t flags; /* flags */
> +};
> +
> +struct pe32_opt_hdr {
> + /* "standard" header */
> + uint16_t magic; /* file type */
> + uint8_t ld_major; /* linker major version */
> + uint8_t ld_minor; /* linker minor version */
> + uint32_t text_size;
> + uint32_t data_size;
> + uint32_t bss_size;
> + uint32_t entry_point; /* file offset of entry point */
> + uint32_t code_base; /* relative code addr in ram */
> + uint32_t data_base; /* relative data addr in ram */
> + /* "extra" header fields */
> + uint32_t image_base; /* preferred load address */
> + uint32_t section_align; /* alignment in bytes */
> + uint32_t file_align; /* file alignment in bytes */
> + uint16_t os_major;
> + uint16_t os_minor;
> + uint16_t image_major;
> + uint16_t image_minor;
> + uint16_t subsys_major;
> + uint16_t subsys_minor;
> + uint32_t win32_version; /* reserved, must be 0 */
> + uint32_t image_size;
> + uint32_t header_size;
> + uint32_t csum;
> + uint16_t subsys;
> + uint16_t dll_flags;
> + uint32_t stack_size_req; /* amt of stack requested */
> + uint32_t stack_size; /* amt of stack required */
> + uint32_t heap_size_req; /* amt of heap requested */
> + uint32_t heap_size; /* amt of heap required */
> + uint32_t loader_flags; /* reserved, must be 0 */
> + uint32_t data_dirs; /* number of data dir entries */
> +};
> +
> +struct pe32plus_opt_hdr {
> + uint16_t magic; /* file type */
> + uint8_t ld_major; /* linker major version */
> + uint8_t ld_minor; /* linker minor version */
> + uint32_t text_size;
> + uint32_t data_size;
> + uint32_t bss_size;
> + uint32_t entry_point; /* file offset of entry point */
> + uint32_t code_base; /* relative code addr in ram */
> + /* "extra" header fields */
> + uint64_t image_base; /* preferred load address */
> + uint32_t section_align; /* alignment in bytes */
> + uint32_t file_align; /* file alignment in bytes */
> + uint16_t os_major;
> + uint16_t os_minor;
> + uint16_t image_major;
> + uint16_t image_minor;
> + uint16_t subsys_major;
> + uint16_t subsys_minor;
> + uint32_t win32_version; /* reserved, must be 0 */
> + uint32_t image_size;
> + uint32_t header_size;
> + uint32_t csum;
> + uint16_t subsys;
> + uint16_t dll_flags;
> + uint64_t stack_size_req; /* amt of stack requested */
> + uint64_t stack_size; /* amt of stack required */
> + uint64_t heap_size_req; /* amt of heap requested */
> + uint64_t heap_size; /* amt of heap required */
> + uint32_t loader_flags; /* reserved, must be 0 */
> + uint32_t data_dirs; /* number of data dir entries */
> +};
> +
> +struct section_header {
> + char name[8]; /* name or "/12\0" string tbl offset */
Why 12?
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/riscv_image_header.h
Is this file taken from somewhere else, kind of making it desirable to keep
its name in sync with the original? Otherwise: We prefer dashes over underscores
in new files' names.
> @@ -0,0 +1,54 @@
> +#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_LE 0
> +#define RISCV_IMAGE_FLAG_BE 1
> +
> +#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_LE
> +
> +#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
You saying "xen": Is there anything Xen-specific in this struct?
> + * @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
> +{
> + uint32_t code0;
> + uint32_t code1;
> + uint64_t text_offset;
> + uint64_t image_size;
> + uint64_t res1;
> + uint64_t res2;
> + uint64_t res3;
> + uint64_t magic;
> + uint32_t res4;
> + uint32_t res5;
> +};
> +#endif /* __ASSEMBLY__ */
> +#endif /* _ASM_RISCV_IMAGE_H */
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -1,14 +1,150 @@
> #include <asm/asm.h>
> #include <asm/riscv_encoding.h>
> +#include <asm/riscv_image_header.h>
> +#ifdef CONFIG_RISCV_EFI
> +#include <asm/pe.h>
> +#endif
>
> .section .text.header, "ax", %progbits
>
> /*
> * OpenSBI pass to start():
> * a0 -> hart_id ( bootcpu_id )
> - * a1 -> dtb_base
> + * a1 -> dtb_base
> */
> FUNC(start)
> +
> +efi_head:
Why is this ...
> +#ifdef CONFIG_RISCV_EFI
... ahead of this?
> + /*
> + * This instruction decodes to "MZ" ASCII required by UEFI.
> + */
> + c.li s4,-13
IOW RISCV_EFI ought to be (made) dependent upon RISCV_ISA_C?
> + j xen_start
Doesn't this then need to be c.j, to be sure it fits in 16 bits (and
raise an assembler error if not)?
> +#else
> + /* jump to start kernel */
> + j xen_start
> + /* reserved */
> + .word 0
What struct field does this correspond to? Or if not a struct field,
why is this needed?
Also I can't help the impression that the resulting layout will be
different depending on whether RISCV_ISA_C is enabled, as the "j"
may translate to a 16-bit or 32-bit insn.
I wonder anyway what use everything from here to ...
> +#endif
> + .balign 8
> +#ifdef CONFIG_RISCV_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
> +#ifndef CONFIG_RISCV_EFI
> + .word 0
... here is when RISCV_EFI=n. You add data which wasn't needed so far,
and for which it also isn't explained why it would suddenly be needed.
I also can't bring several of the fields above in sync with the
struct riscv_image_header definition in the header. Please can you
annotate each field with a comment naming the corresponding C struct
field (like you do further down, at least in a way)?
> +#else
> + .word pe_head_start - efi_head
> +pe_head_start:
> + .long PE_MAGIC
> +coff_header:
> +#ifdef CONFIG_RISCV_64
> + .short IMAGE_FILE_MACHINE_RISCV64 /* Machine */
> +#else
> + .short IMAGE_FILE_MACHINE_RISCV32 /* Machine */
> +#endif
> + .short section_count /* NumberOfSections */
> + .long 0 /* TimeDateStamp */
> + .long 0 /* PointerToSymbolTable */
> + .long 0 /* NumberOfSymbols */
> + .short section_table - optional_header /* SizeOfOptionalHeader */
> + .short IMAGE_FILE_DEBUG_STRIPPED | \
> + IMAGE_FILE_EXECUTABLE_IMAGE | \
> + IMAGE_FILE_LINE_NUMS_STRIPPED /* Characteristics */
> +
> +optional_header:
> +#ifdef CONFIG_RISCV_64
> + .short PE_OPT_MAGIC_PE32PLUS /* PE32+ format */
> +#else
> + .short PE_OPT_MAGIC_PE32 /* PE32 format */
> +#endif
> + .byte 0x02 /* MajorLinkerVersion */
> + .byte 0x14 /* MinorLinkerVersion */
> + .long _end - xen_start /* SizeOfCode */
> + .long 0 /* SizeOfInitializedData */
> + .long 0 /* SizeOfUninitializedData */
> + .long 0 /* AddressOfEntryPoint */
> + .long xen_start - efi_head /* BaseOfCode */
> +
> +extra_header_fields:
> + .quad 0 /* ImageBase */
This is quad only for PE32+, iirc. In PE32 it's two separate 32-bit
fields instead. The overall result may be tolerable (a data RVA of 0
can't be quite right, but we may be able to get away with that), but
it will at least want commenting on.
Any anyway - further up in the RISC-V header struct you use .word and
.dword. Why .long and .quad here? That's at least somewhat confusing.
> + .long PECOFF_SECTION_ALIGNMENT /* SectionAlignment */
> + .long PECOFF_FILE_ALIGNMENT /* FileAlignment */
> + .short 0 /* MajorOperatingSystemVersion */
> + .short 0 /* MinorOperatingSystemVersion */
> + .short LINUX_EFISTUB_MAJOR_VERSION /* MajorImageVersion */
> + .short LINUX_EFISTUB_MINOR_VERSION /* MinorImageVersion */
> + .short 0 /* MajorSubsystemVersion */
> + .short 0 /* MinorSubsystemVersion */
> + .long 0 /* Win32VersionValue */
> + .long _end - efi_head /* SizeOfImage */
> +
> + /* Everything before the xen image is considered part of the header */
> + .long xen_start - efi_head /* SizeOfHeaders */
> + .long 0 /* CheckSum */
> + .short IMAGE_SUBSYSTEM_EFI_APPLICATION /* Subsystem */
> + .short 0 /* DllCharacteristics */
> + .quad 0 /* SizeOfStackReserve */
> + .quad 0 /* SizeOfStackCommit */
> + .quad 0 /* SizeOfHeapReserve */
> + .quad 0 /* SizeOfHeapCommit */
All of these are again 32 bits only in PE32, if I'm not mistaken.
> + .long 0 /* LoaderFlags */
> + .long (section_table - .) / 8 /* NumberOfRvaAndSizes */
> + .quad 0 /* ExportTable */
> + .quad 0 /* ImportTable */
> + .quad 0 /* ResourceTable */
> + .quad 0 /* ExceptionTable */
> + .quad 0 /* CertificationTable */
> + .quad 0 /* BaseRelocationTable */
Would you mind clarifying on what basis this set of 6 entries was
chosen?
> +/* Section table */
> +section_table:
> + .ascii ".text\0\0\0"
> + .long 0
> + .long 0
> + .long 0 /* SizeOfRawData */
> + .long 0 /* PointerToRawData */
> + .long 0 /* PointerToRelocations */
> + .long 0 /* PointerToLineNumbers */
> + .short 0 /* NumberOfRelocations */
> + .short 0 /* NumberOfLineNumbers */
> + .long IMAGE_SCN_CNT_CODE | \
> + IMAGE_SCN_MEM_READ | \
> + IMAGE_SCN_MEM_EXECUTE /* Characteristics */
> +
> + .ascii ".data\0\0\0"
> + .long _end - xen_start /* VirtualSize */
> + .long xen_start - efi_head /* VirtualAddress */
> + .long __init_end_efi - xen_start /* SizeOfRawData */
> + .long xen_start - efi_head /* PointerToRawData */
> + .long 0 /* PointerToRelocations */
> + .long 0 /* PointerToLineNumbers */
> + .short 0 /* NumberOfRelocations */
> + .short 0 /* NumberOfLineNumbers */
> + .long IMAGE_SCN_CNT_INITIALIZED_DATA | \
> + IMAGE_SCN_MEM_READ | \
> + IMAGE_SCN_MEM_WRITE /* Characteristics */
IOW no code and the entire image expressed as data. Interesting.
No matter whether that has a reason or is completely arbitrary, I
think it, too, wants commenting on.
> + .set section_count, (. - section_table) / 40
> +
> + .balign 0x1000
> +#endif/* CONFIG_RISCV_EFI */
> +
> +FUNC(xen_start)
> /* Mask all interrupts */
> csrw CSR_SIE, zero
>
> @@ -60,6 +196,9 @@ FUNC(start)
> mv a1, s1
>
> tail start_xen
> +
> +END(xen_start)
> +
> END(start)
I don't think you addressed my function nesting comment here either.
> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -12,6 +12,9 @@ PHDRS
> #endif
> }
>
> +PECOFF_SECTION_ALIGNMENT = 0x1000;
> +PECOFF_FILE_ALIGNMENT = 0x200;
> +
> SECTIONS
> {
> . = XEN_VIRT_START;
> @@ -144,7 +147,7 @@ SECTIONS
> .got.plt : {
> *(.got.plt)
> } : text
> -
> + __init_end_efi = .;
Why does the blank line disappear? And why is ...
> . = ALIGN(POINTER_ALIGN);
> __init_end = .;
... __init_end not good enough? (I think I can guess the answer, but
then I further think the name of the symbol is misleading. )
> @@ -165,6 +168,7 @@ SECTIONS
> . = ALIGN(POINTER_ALIGN);
> __bss_end = .;
> } :text
> +
> _end = . ;
Interestingly an unrelated blank line suddenly appears here.
Jan
CONFIDENTIALITY: The contents of this e-mail are confidential and intended only for the above addressee(s). If you are not the intended recipient, or the person responsible for delivering it to the intended recipient, copying or delivering it to anyone else or using it in any unauthorized manner is prohibited and may be unlawful. If you receive this e-mail by mistake, please notify the sender and the systems administrator at straymail@rt-rk.com immediately.
On 26.06.2024 18:16, Milan Djokic wrote: >> Signed-off-by: Nikola Jelic <nikola.jelic@rt-rk.com> > > This isn't you, is it? Your S-o-b is going to be needed, too. > > Nikola.jelic@rt-rk.com is the initial author of the patch, I'll add myself also if necessary > >> +config RISCV_EFI >> + bool "UEFI boot service support" >> + depends on RISCV_64 >> + default n > > Nit: This line can be omitted (and if I'm not mistaken we generally do omit > such). > > If we remove the default value, EFI header shall be included into xen image by default. Why's this? Or in other words, what are you deriving this from? Not specifying a default implicitly means "n", from all I know. > We want to keep it as optional for now, and generate plain elf as default format (until full EFI support is implemented) I fully second this. >> --- /dev/null >> +++ b/xen/arch/riscv/include/asm/pe.h >> @@ -0,0 +1,148 @@ >> +#ifndef _ASM_RISCV_PE_H >> +#define _ASM_RISCV_PE_H >> + >> +#define LINUX_EFISTUB_MAJOR_VERSION 0x1 >> +#define LINUX_EFISTUB_MINOR_VERSION 0x0 >> + >> +#define MZ_MAGIC 0x5a4d /* "MZ" */ >> + >> +#define PE_MAGIC 0x00004550 /* "PE\0\0" */ >> +#define PE_OPT_MAGIC_PE32 0x010b >> +#define PE_OPT_MAGIC_PE32PLUS 0x020b >> + >> +/* machine type */ >> +#define IMAGE_FILE_MACHINE_RISCV32 0x5032 >> +#define IMAGE_FILE_MACHINE_RISCV64 0x5064 > > Apart from this, hardly anything in this header is RISC-V specific. > Please consider moving to xen/include/xen/. > > We shall move generic part into xen/include/xen/efi. This is something which should be considered for use on arm/x86 also. It isn't, no. That's the case for Arm only so far afaict. > Currently PE/COFF header is directly embedded into > head.S for arm/x86 > >> + char name[8]; /* name or "/12\0" string tbl offset */ > > Why 12? > > Either section name is specified in this field or string table offset if section name can't fit into 8 bytes, which is the case here. Well, yes, I'm certainly aware of that. But the question wasn't about the format, it was specifically about the hardcoded value 12. Why not 11 or 13? > Btw this is taken over from linux kernel together with the PE/COFF header structures: https://github.com/torvalds/linux/blob/master/include/linux/pe.h Which is in no way removing the need for you to be able to explain the changes you're making. >> + * struct riscv_image_header - riscv xen image header > > You saying "xen": Is there anything Xen-specific in this struct? > > Not really related to xen, this is generic riscv PE image header, comment fixed in new version > >> + .long 0 /* LoaderFlags */ >> + .long (section_table - .) / 8 /* NumberOfRvaAndSizes */ >> + .quad 0 /* ExportTable */ >> + .quad 0 /* ImportTable */ >> + .quad 0 /* ResourceTable */ >> + .quad 0 /* ExceptionTable */ >> + .quad 0 /* CertificationTable */ >> + .quad 0 /* BaseRelocationTable */ > > Would you mind clarifying on what basis this set of 6 entries was > chosen? > > These fields and their sizes are defined in official PE format, see details from specification bellow > > [cid:542690de-3bb0-4708-a447-996a03277578] Again, I'm aware of the specification. Yet like the 12 above the 6 here looks arbitrarily chosen. There are more entries in this table which are permitted to be present (and well-defined). There could also be fewer of them; any absent entry is implicitly holding the value 0 afaia. >> +/* Section table */ >> +section_table: >> + .ascii ".text\0\0\0" >> + .long 0 >> + .long 0 >> + .long 0 /* SizeOfRawData */ >> + .long 0 /* PointerToRawData */ >> + .long 0 /* PointerToRelocations */ >> + .long 0 /* PointerToLineNumbers */ >> + .short 0 /* NumberOfRelocations */ >> + .short 0 /* NumberOfLineNumbers */ >> + .long IMAGE_SCN_CNT_CODE | \ >> + IMAGE_SCN_MEM_READ | \ >> + IMAGE_SCN_MEM_EXECUTE /* Characteristics */ >> + >> + .ascii ".data\0\0\0" >> + .long _end - xen_start /* VirtualSize */ >> + .long xen_start - efi_head /* VirtualAddress */ >> + .long __init_end_efi - xen_start /* SizeOfRawData */ >> + .long xen_start - efi_head /* PointerToRawData */ >> + .long 0 /* PointerToRelocations */ >> + .long 0 /* PointerToLineNumbers */ >> + .short 0 /* NumberOfRelocations */ >> + .short 0 /* NumberOfLineNumbers */ >> + .long IMAGE_SCN_CNT_INITIALIZED_DATA | \ >> + IMAGE_SCN_MEM_READ | \ >> + IMAGE_SCN_MEM_WRITE /* Characteristics */ > > IOW no code and the entire image expressed as data. Interesting. > No matter whether that has a reason or is completely arbitrary, I > think it, too, wants commenting on. > > This is correct, currently we have extended image with PE/COFF (EFI) header which allows xen boot from EFI loader (or U-boot) environment. And these updates are pure data. We are actively working on the implementation of Boot/Runtime services which shall be in the code section part and enable full UEFI compatible xen application for riscv. Such a choice, even if transient, needs explaining in the description (or maybe even a code comment) then. > Why does the blank line disappear? And why is ... > >> . = ALIGN(POINTER_ALIGN); >> __init_end = .; > > ... __init_end not good enough? (I think I can guess the answer, but > then I further think the name of the symbol is misleading. ) > > Init_end_efi is used only when EFI sections are included into image. Again, my question was different: I asked why a symbol we have already isn't good enough, i.e. why another one needs adding. > We have aligned with arm implementation here, you can take a look also there. And yet again, as per above, you need to be able to explain your decisions. You can't just say "it's done this way elsewhere as well". What if that "elsewhere" has an obvious or maybe just subtle bug? Jan
On Thu, Jun 27, 2024 at 10:55 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 26.06.2024 18:16, Milan Djokic wrote:
> >> +config RISCV_EFI
> >> + bool "UEFI boot service support"
> >> + depends on RISCV_64
> >> + default n
> >
> > Nit: This line can be omitted (and if I'm not mistaken we generally do omit
> > such).
> >
> > If we remove the default value, EFI header shall be included into xen image by default.
>
> Why's this? Or in other words, what are you deriving this from? Not specifying
> a default implicitly means "n", from all I know.
>
My assumption regarding option value when default is not specified was
wrong. You're correct, we'll omit the default line.
>
> > Currently PE/COFF header is directly embedded into
> > head.S for arm/x86
> >
> >> + char name[8]; /* name or "/12\0" string tbl offset */
> >
> > Why 12?
> >
> > Either section name is specified in this field or string table offset if section name can't fit into 8 bytes, which is the case here.
>
> Well, yes, I'm certainly aware of that. But the question wasn't about the
> format, it was specifically about the hardcoded value 12. Why not 11 or 13?
>
I've misinterpreted your original question here. I realize now that
this comment ("name or /12/0") is confusing (and incorrect). It was
taken over from linux kernel which on the other hand took this over
from pesign package, so I assume that pesign had its own string table
layout and thus hardcoded 12 offset for its specific usecase. Since in
general we can have different offsets and even more than one (if e.g.
2 section names exceed 8-byte size) we'll change this comment not to
contain 12 offset hint.
>
> >> + * struct riscv_image_header - riscv xen image header
> >
> > You saying "xen": Is there anything Xen-specific in this struct?
> >
> > Not really related to xen, this is generic riscv PE image header, comment fixed in new version
> >
> >> + .long 0 /* LoaderFlags */
> >> + .long (section_table - .) / 8 /* NumberOfRvaAndSizes */
> >> + .quad 0 /* ExportTable */
> >> + .quad 0 /* ImportTable */
> >> + .quad 0 /* ResourceTable */
> >> + .quad 0 /* ExceptionTable */
> >> + .quad 0 /* CertificationTable */
> >> + .quad 0 /* BaseRelocationTable */
> >
> > Would you mind clarifying on what basis this set of 6 entries was
> > chosen?
> >
> > These fields and their sizes are defined in official PE format, see details from specification bellow
> >
> > [cid:542690de-3bb0-4708-a447-996a03277578]
>
> Again, I'm aware of the specification. Yet like the 12 above the 6 here
> looks arbitrarily chosen. There are more entries in this table which
> are permitted to be present (and well-defined). There could also be
> fewer of them; any absent entry is implicitly holding the value 0 afaia.
>
We can omit all of them since directories are not used at all in this
case. Even those 6 are set to 0 (which means not used according to
PE). One more case where we wanted to align with linux kernel / xen
arm implementation, but it is redundant in our case
> >> +/* Section table */
> >> +section_table:
> >> + .ascii ".text\0\0\0"
> >> + .long 0
> >> + .long 0
> >> + .long 0 /* SizeOfRawData */
> >> + .long 0 /* PointerToRawData */
> >> + .long 0 /* PointerToRelocations */
> >> + .long 0 /* PointerToLineNumbers */
> >> + .short 0 /* NumberOfRelocations */
> >> + .short 0 /* NumberOfLineNumbers */
> >> + .long IMAGE_SCN_CNT_CODE | \
> >> + IMAGE_SCN_MEM_READ | \
> >> + IMAGE_SCN_MEM_EXECUTE /* Characteristics */
> >> +
> >> + .ascii ".data\0\0\0"
> >> + .long _end - xen_start /* VirtualSize */
> >> + .long xen_start - efi_head /* VirtualAddress */
> >> + .long __init_end_efi - xen_start /* SizeOfRawData */
> >> + .long xen_start - efi_head /* PointerToRawData */
> >> + .long 0 /* PointerToRelocations */
> >> + .long 0 /* PointerToLineNumbers */
> >> + .short 0 /* NumberOfRelocations */
> >> + .short 0 /* NumberOfLineNumbers */
> >> + .long IMAGE_SCN_CNT_INITIALIZED_DATA | \
> >> + IMAGE_SCN_MEM_READ | \
> >> + IMAGE_SCN_MEM_WRITE /* Characteristics */
> >
> > IOW no code and the entire image expressed as data. Interesting.
> > No matter whether that has a reason or is completely arbitrary, I
> > think it, too, wants commenting on.
> >
> > This is correct, currently we have extended image with PE/COFF (EFI) header which allows xen boot from EFI loader (or U-boot) environment. And these updates are pure data. We are actively working on the implementation of Boot/Runtime services which shall be in the code section part and enable full UEFI compatible xen application for riscv.
>
> Such a choice, even if transient, needs explaining in the description
> (or maybe even a code comment) then.
We'll clarify this part in code directly
>
> > Why does the blank line disappear? And why is ...
> >
> >> . = ALIGN(POINTER_ALIGN);
> >> __init_end = .;
> >
> > ... __init_end not good enough? (I think I can guess the answer, but
> > then I further think the name of the symbol is misleading. )
> >
> > Init_end_efi is used only when EFI sections are included into image.
>
> Again, my question was different: I asked why a symbol we have already
> isn't good enough, i.e. why another one needs adding.
>
Similar as for data directories fields above, _init_end_efi is also
redundant for RISC-V case, we'll use _init_end directly instead.
> > We have aligned with arm implementation here, you can take a look also there.
>
> And yet again, as per above, you need to be able to explain your decisions.
> You can't just say "it's done this way elsewhere as well". What if that
> "elsewhere" has an obvious or maybe just subtle bug?
This is perfectly clear. We'll restructure our changes in the next
version in that manner.
BR,
Milan
© 2016 - 2024 Red Hat, Inc.