|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/riscv: PE/COFF image header for RISC-V target
On 12.06.2024 14:15, milandjokic1995@xxxxxxxxx wrote:
> From: Nikola Jelic <nikola.jelic@xxxxxxxxx>
>
> 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@xxxxxxxxx>
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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |