From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 11 Apr 2019 03:43:21 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CE8923003705; Thu, 11 Apr 2019 10:43:20 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-225.rdu2.redhat.com [10.10.120.225]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0D71960BF7; Thu, 11 Apr 2019 10:43:18 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 05/31] OvmfPkg/XenOvmf: Creating an ELF header To: devel@edk2.groups.io, anthony.perard@citrix.com Cc: Jordan Justen , Ard Biesheuvel , Julien Grall , xen-devel@lists.xenproject.org References: <20190409110844.14746-1-anthony.perard@citrix.com> <20190409110844.14746-6-anthony.perard@citrix.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 11 Apr 2019 12:43:17 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190409110844.14746-6-anthony.perard@citrix.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.47]); Thu, 11 Apr 2019 10:43:20 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/09/19 13:08, Anthony PERARD wrote: > This header replace the embedded variable store. > > The ELF header explain to a loader to load the binary at the address > 1MB, then jump to the PVH entry point which will be created in a later > patch. > > That patch include generate_elf_header.c which can be use to regenerate > the ELF header, but this will be a manual step. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Anthony PERARD > --- > > Notes: > This "ELF header" might not be the best way, but with it the Xen > toolstack can load OVMF like any other PVH-compatible kernel without > modification. Is there a better way? > > The generate_elf_header.c file was used to generate the series of hex > number. It isn't at a right place but I'm not sure where to put this C > file. Maybe in the commit message or maybe it could be forgotten since > I've added all the comments in the .fdf file. > > OvmfPkg/XenOvmf.fdf | 82 +++++++++++++++++++- > generate_elf_header.c | 78 +++++++++++++++++++ > 2 files changed, 157 insertions(+), 3 deletions(-) Some things need to be fixed for sure; some other things you might want to explore as possible improvements. Generators (that need to be run manually) are perfectly fine. "must": (1) The location (the root dir) of this file is inappropriate. It needs to go under OvmfPkg, and it must state Xen in the name somewhere. I think right under OvmfPkg/ is fine, because that's where the FDF / FDF-include files are too. Also, the filename should use CamelCase. (2) The generator must definitely include a copyright notice and a license (preferably an SPDX license identifier). If you diverge from BSD-2-Clause-Patent, then you might want to mention this exception in OvmfPkg/License.txt too. (I got some other patches pending for that, so please check those if you can.) (3) Near the hex array in the FDF file, there should be a reminder about the section being generated, and preferably a pointer to the generator. See for example the top of "OvmfPkg/QemuVideoDxe/VbeShim.h". Possible improvements: (4) Document, at the top of the generator, how the generator is supposed to be built & run, and what packages (if any) it requires. It's fine if those instructions are OS/distro specific. (5) If you rewrote the generatory in python, perl, or shell, it could be easier to use. (6) I vaguely recall that we can now use C-like structs in DATA regions in the FDF files. I've never tried it myself, and now I can't find anything related in the TianoCore bugzilla, the FDF spec, or the BaseTools git history. If you wanted to research this, you'd have to talk to the BaseTools maintainers. If you fix (1) through (3) and ignore the rest, I'll be OK to ACK the patch. Thanks, Laszlo > diff --git a/OvmfPkg/XenOvmf.fdf b/OvmfPkg/XenOvmf.fdf > index 295e30ca3f..20ebacd673 100644 > --- a/OvmfPkg/XenOvmf.fdf > +++ b/OvmfPkg/XenOvmf.fdf > @@ -21,8 +21,8 @@ [Defines] > !include OvmfPkg.fdf.inc > > # > -# Build the variable store and the firmware code as one unified flash device > -# image. > +# This will allow the flash device image to be recognize as an ELF, with first > +# an ELF headers, then the firmware code. > # > [FD.OVMF] > BaseAddress = $(FW_BASE_ADDRESS) > @@ -31,7 +31,83 @@ [FD.OVMF] > BlockSize = $(BLOCK_SIZE) > NumBlocks = $(FW_BLOCKS) > > -!include VarStore.fdf.inc > +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) > +0x00000000|0x0000e000 > +!endif > +!if $(FD_SIZE_IN_KB) == 4096 > +0x00000000|0x00040000 > +!endif > +#was NV_VARIABLE_STORE > +DATA = { > + # ELF file header > + 0x7f, 0x45, 0x4c, 0x46, # e_ident[0..3]: Magic number > + 0x01, # File class: 32-bit objects > + 0x01, # Data encoding: 2's complement, little endian > + 0x01, # File version > + 0x03, # OS ABI identification: Object uses GNU ELF extensions > + 0x00, # ABI version > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, # e_ident[EI_PAD...] > + > + 0x02, 0x00, # e_type = Executable file > + 0x03, 0x00, # e_machine = Intel 80386 > + 0x01, 0x00, 0x00, 0x00, # e_version > + 0xd0, 0xff, 0x2f, 0x00, # e_entry: Entry point virtual address > + 0x34, 0x00, 0x00, 0x00, # e_phoff: Program header table file offset > + 0x00, 0x00, 0x00, 0x00, # e_shoff: Section header table file offset > + 0x00, 0x00, 0x00, 0x00, # e_flags: Processor-specific flags > + 0x34, 0x00, # e_ehsize: ELF header size > + 0x20, 0x00, # e_phentsize: Program header table entry size > + 0x01, 0x00, # e_phnum: Program header table entry count > + 0x00, 0x00, # e_shentsize: Section header table entry size > + 0x00, 0x00, # e_shnum: Section header table entry count > + 0xff, 0xff, # e_shstrndx > + > + # ELF Program segment header > + 0x01, 0x00, 0x00, 0x00, # p_type = Loadable program segment > + 0x00, 0x00, 0x00, 0x00, # p_offset > + 0x00, 0x00, 0x10, 0x00, # p_vaddr: Segment virtual address > + 0x00, 0x00, 0x10, 0x00, # p_paddr: Segment physical address > + 0x00, 0x00, 0x20, 0x00, # p_filesz: Segment size in file > + 0x00, 0x00, 0x20, 0x00, # p_memsz: Segment size in memory > + 0x07, 0x00, 0x00, 0x00, # p_flags = Segment is executable | writable | readable > + 0x00, 0x00, 0x00, 0x00 # p_align > + > +} > + > +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) > +0x0000e000|0x00001000 > +!endif > +!if $(FD_SIZE_IN_KB) == 4096 > +0x00040000|0x00001000 > +!endif > +#NV_EVENT_LOG > + > +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) > +0x0000f000|0x00001000 > +!endif > +!if $(FD_SIZE_IN_KB) == 4096 > +0x00041000|0x00001000 > +!endif > +#NV_FTW_WORKING > +DATA = { > + # EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER->Signature = gEdkiiWorkingBlockSignatureGuid = > + # { 0x9e58292b, 0x7c68, 0x497d, { 0xa0, 0xce, 0x65, 0x0, 0xfd, 0x9f, 0x1b, 0x95 }} > + 0x2b, 0x29, 0x58, 0x9e, 0x68, 0x7c, 0x7d, 0x49, > + 0xa0, 0xce, 0x65, 0x0, 0xfd, 0x9f, 0x1b, 0x95, > + # Crc:UINT32 #WorkingBlockValid:1, WorkingBlockInvalid:1, Reserved > + 0x2c, 0xaf, 0x2c, 0x64, 0xFE, 0xFF, 0xFF, 0xFF, > + # WriteQueueSize: UINT64 > + 0xE0, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 > +} > + > +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) > +0x00010000|0x00010000 > +!endif > +!if $(FD_SIZE_IN_KB) == 4096 > +0x00042000|0x00042000 > +!endif > +#NV_FTW_SPARE > + > > $(VARS_SIZE)|$(FVMAIN_SIZE) > FV = FVMAIN_COMPACT > diff --git a/generate_elf_header.c b/generate_elf_header.c > new file mode 100644 > index 0000000000..5bb87df0d6 > --- /dev/null > +++ b/generate_elf_header.c > @@ -0,0 +1,78 @@ > +#include "elf.h" > +#include "stdio.h" > +#include "stddef.h" > + > +/* > + * TODO: > + * this header will need a XEN_ELFNOTE_PHYS32_ENTRY > + * right now, it works without, but that might change. > + */ > + > +void print_hdr(void *s, size_t size) { > + char *c = s; > + > + while (size--) { > + printf("0x%02hhx, ", *(c++)); > + } > +} > +int main(void){ > + // FW_SIZE > + size_t ovmf_blob_size = 0x00200000; > + // Load OVMF at 1MB when running as PVH guest > + uint32_t ovmf_base_address = 0x00100000; > + > + /* ELF file header */ > + Elf32_Ehdr hdr = { > + .e_ident = ELFMAG, > + .e_type = ET_EXEC, > + .e_machine = EM_386, > + .e_version = EV_CURRENT, > + // PVH entry point > + .e_entry = ovmf_base_address + ovmf_blob_size - 0x30, > + }; > + > + hdr.e_ident[EI_CLASS] = ELFCLASS32; > + hdr.e_ident[EI_DATA] = ELFDATA2LSB; > + hdr.e_ident[EI_VERSION] = EV_CURRENT; > + hdr.e_ident[EI_OSABI] = ELFOSABI_LINUX; > + hdr.e_flags = R_386_NONE; > + hdr.e_ehsize = sizeof (hdr); > + /* about program header */ > + hdr.e_phoff = sizeof (hdr); > + /* about section header */ > + hdr.e_shentsize = 0; > + hdr.e_shnum = 0; > + hdr.e_shstrndx = -1; > + > + /* program header */ > + Elf32_Phdr phdr = { > + .p_type = PT_LOAD, > + .p_offset = 0, // load everything > + .p_paddr = ovmf_base_address, > + .p_filesz = ovmf_blob_size, > + .p_memsz = ovmf_blob_size, > + .p_flags = PF_X | PF_W | PF_R, > + .p_align = 0, // is this needed? > + }; > + phdr.p_vaddr = phdr.p_paddr; > + > + hdr.e_phnum += 1; > + hdr.e_phentsize += sizeof (phdr); > + > + /* section header */ > + Elf32_Shdr shdr = { > + }; > + > + size_t hdr_size = sizeof(hdr); > + size_t entry_off = offsetof(typeof(hdr), e_entry); > + printf("# ELF file header\n"); > + print_hdr(&hdr, entry_off); > + printf("\n# hdr.e_entry\n"); > + print_hdr(&hdr.e_entry, sizeof(hdr.e_entry)); > + printf("\n# the rest\n"); > + print_hdr(&hdr.e_entry + 1, hdr_size - entry_off - sizeof(hdr.e_entry)); > + printf("\n# Program segment header\n"); > + print_hdr(&phdr, sizeof (phdr)); > + > + return 0; > +} >