Re: [edk2-devel] [PATCH v2 05/31] OvmfPkg/XenOvmf: Creating an ELF header

["Laszlo Ersek" <lersek@redhat.com>]

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 <anthony.perard@citrix.com>
> ---
> 
> 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;
> +}
>