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