[3/4] multiboot: load elf sections and section headers

Message ID	20171012235439.19457-4-anatol.pomozov@gmail.com
State	New
Headers	show Return-Path: <qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org> From: Anatol Pomozov <anatol.pomozov@gmail.com> To: qemu-devel@nongnu.org Date: Thu, 12 Oct 2017 16:54:38 -0700 Message-Id: <20171012235439.19457-4-anatol.pomozov@gmail.com> In-Reply-To: <20171012235439.19457-1-anatol.pomozov@gmail.com> References: <20171012235439.19457-1-anatol.pomozov@gmail.com> Subject: [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers Precedence: list Cc: kwolf@redhat.com, ehabkost@redhat.com, Anatol Pomozov <anatol.pomozov@gmail.com>, agraf@suse.de, pbonzini@redhat.com, rth@twiddle.net Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" <qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>
Series	[1/4] multiboot: Change multiboot_info from array of bytes to a C struct \| expand [1/4] multiboot: Change multiboot_info from array of bytes to a C struct [2/4] multiboot: load any machine type of ELF [3/4] multiboot: load elf sections and section headers [4/4] multiboot: make tests work with clang

Anatol Pomozov Oct. 12, 2017, 11:54 p.m. UTC

Multiboot may load section headers and all sections (even those that are
not part of any segment) to target memory.

Tested with an ELF application that uses data from strings table
section.

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 hw/core/loader.c                         |   8 +--
 hw/i386/multiboot.c                      |  83 +++++++++++++-----------
 hw/s390x/ipl.c                           |   2 +-
 include/hw/elf_ops.h                     | 107 ++++++++++++++++++++++++++++++-
 include/hw/loader.h                      |  11 +++-
 tests/multiboot/Makefile                 |   8 ++-
 tests/multiboot/generate_sections_out.py |  33 ++++++++++
 tests/multiboot/modules.out              |  22 +++----
 tests/multiboot/run_test.sh              |   6 +-
 tests/multiboot/sections.c               |  55 ++++++++++++++++
 tests/multiboot/start.S                  |   2 +-
 11 files changed, 276 insertions(+), 61 deletions(-)
 create mode 100755 tests/multiboot/generate_sections_out.py
 create mode 100644 tests/multiboot/sections.c

Anatol Pomozov Oct. 18, 2017, 5:22 p.m. UTC | #1

Hello qemu-devs,

This patchset has been posted a while ago. I would like to move
forward with it and look at the next task (e.g. multiboot2 support in
QEMU). Who is the multiboot code maintainer who can help to review
this patchset and give go/no-go answer?

On Thu, Oct 12, 2017 at 4:54 PM, Anatol Pomozov
<anatol.pomozov@gmail.com> wrote:
> Multiboot may load section headers and all sections (even those that are
> not part of any segment) to target memory.
>
> Tested with an ELF application that uses data from strings table
> section.
>
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
>  hw/core/loader.c                         |   8 +--
>  hw/i386/multiboot.c                      |  83 +++++++++++++-----------
>  hw/s390x/ipl.c                           |   2 +-
>  include/hw/elf_ops.h                     | 107 ++++++++++++++++++++++++++++++-
>  include/hw/loader.h                      |  11 +++-
>  tests/multiboot/Makefile                 |   8 ++-
>  tests/multiboot/generate_sections_out.py |  33 ++++++++++
>  tests/multiboot/modules.out              |  22 +++----
>  tests/multiboot/run_test.sh              |   6 +-
>  tests/multiboot/sections.c               |  55 ++++++++++++++++
>  tests/multiboot/start.S                  |   2 +-
>  11 files changed, 276 insertions(+), 61 deletions(-)
>  create mode 100755 tests/multiboot/generate_sections_out.py
>  create mode 100644 tests/multiboot/sections.c
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 4593061445..a8787f7685 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -439,7 +439,7 @@ int load_elf_as(const char *filename,
>  {
>      return load_elf_ram(filename, translate_fn, translate_opaque,
>                          pentry, lowaddr, highaddr, big_endian, elf_machine,
> -                        clear_lsb, data_swab, as, true);
> +                        clear_lsb, data_swab, as, true, NULL);
>  }
>
>  /* return < 0 if error, otherwise the number of bytes loaded in memory */
> @@ -448,7 +448,7 @@ int load_elf_ram(const char *filename,
>                   void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
>                   uint64_t *highaddr, int big_endian, int elf_machine,
>                   int clear_lsb, int data_swab, AddressSpace *as,
> -                 bool load_rom)
> +                 bool load_rom, SectionsData *sections)
>  {
>      int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
>      uint8_t e_ident[EI_NIDENT];
> @@ -488,11 +488,11 @@ int load_elf_ram(const char *filename,
>      if (e_ident[EI_CLASS] == ELFCLASS64) {
>          ret = load_elf64(filename, fd, translate_fn, translate_opaque, must_swab,
>                           pentry, lowaddr, highaddr, elf_machine, clear_lsb,
> -                         data_swab, as, load_rom);
> +                         data_swab, as, load_rom, sections);
>      } else {
>          ret = load_elf32(filename, fd, translate_fn, translate_opaque, must_swab,
>                           pentry, lowaddr, highaddr, elf_machine, clear_lsb,
> -                         data_swab, as, load_rom);
> +                         data_swab, as, load_rom, sections);
>      }
>
>   fail:
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index 7dacd6d827..841d05160a 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -125,7 +125,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>  {
>      int i;
>      bool is_multiboot = false;
> -    uint32_t flags = 0;
> +    uint32_t flags = 0, bootinfo_flags = 0;
>      uint32_t mh_entry_addr;
>      uint32_t mh_load_addr;
>      uint32_t mb_kernel_size;
> @@ -134,6 +134,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>      uint8_t *mb_bootinfo_data;
>      uint32_t cmdline_len;
>      struct multiboot_header *multiboot_header;
> +    SectionsData sections;
>
>      /* Ok, let's see if it is a multiboot image.
>         The header is 12x32bit long, so the latest entry may be 8192 - 48. */
> @@ -161,37 +162,8 @@ int load_multiboot(FWCfgState *fw_cfg,
>      if (flags & MULTIBOOT_VIDEO_MODE) {
>          fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n");
>      }
> -    if (!(flags & MULTIBOOT_AOUT_KLUDGE)) {
> -        uint64_t elf_entry;
> -        uint64_t elf_low, elf_high;
> -        int kernel_size;
> -        fclose(f);
>
> -        if (((struct elf64_hdr*)header)->e_machine == EM_X86_64) {
> -            fprintf(stderr, "Cannot load x86-64 image, give a 32bit one.\n");
> -            exit(1);
> -        }
> -
> -        kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
> -                               &elf_low, &elf_high, 0, EM_NONE,
> -                               0, 0);
> -        if (kernel_size < 0) {
> -            fprintf(stderr, "Error while loading elf kernel\n");
> -            exit(1);
> -        }
> -        mh_load_addr = elf_low;
> -        mb_kernel_size = elf_high - elf_low;
> -        mh_entry_addr = elf_entry;
> -
> -        mbs.mb_buf = g_malloc(mb_kernel_size);
> -        if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
> -            fprintf(stderr, "Error while fetching elf kernel from rom\n");
> -            exit(1);
> -        }
> -
> -        mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
> -                  mb_kernel_size, (size_t)mh_entry_addr);
> -    } else {
> +    if (flags & MULTIBOOT_AOUT_KLUDGE) {
>          /* Valid if mh_flags sets MULTIBOOT_AOUT_KLUDGE. */
>          uint32_t mh_header_addr = ldl_p(&multiboot_header->header_addr);
>          uint32_t mh_load_end_addr = ldl_p(&multiboot_header->load_end_addr);
> @@ -228,12 +200,6 @@ int load_multiboot(FWCfgState *fw_cfg,
>              mb_load_size = mb_kernel_size;
>          }
>
> -        /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
> -        uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
> -        uint32_t mh_width = ldl_p(&multiboot_header->width);
> -        uint32_t mh_height = ldl_p(&multiboot_header->height);
> -        uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
> -
>          mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
>          mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
>          mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
> @@ -248,9 +214,45 @@ int load_multiboot(FWCfgState *fw_cfg,
>              exit(1);
>          }
>          memset(mbs.mb_buf + mb_load_size, 0, mb_kernel_size - mb_load_size);
> -        fclose(f);
> +    } else {
> +        uint64_t elf_entry;
> +        uint64_t elf_low, elf_high;
> +        int kernel_size;
> +
> +        kernel_size = load_elf_ram(kernel_filename, NULL, NULL, &elf_entry,
> +                               &elf_low, &elf_high, 0, I386_ELF_MACHINE,
> +                               0, 0, NULL, true, &sections);
> +        if (kernel_size < 0) {
> +            fprintf(stderr, "Error while loading elf kernel\n");
> +            exit(1);
> +        }
> +        mh_load_addr = elf_low;
> +        mb_kernel_size = elf_high - elf_low;
> +        mh_entry_addr = elf_entry;
> +
> +        mbs.mb_buf = g_malloc(mb_kernel_size);
> +        if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
> +            fprintf(stderr, "Error while fetching elf kernel from rom\n");
> +            exit(1);
> +        }
> +
> +        mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
> +                  mb_kernel_size, (size_t)mh_entry_addr);
> +
> +        stl_p(&bootinfo.u.elf_sec.num, sections.num);
> +        stl_p(&bootinfo.u.elf_sec.size, sections.size);
> +        stl_p(&bootinfo.u.elf_sec.addr, sections.addr);
> +        stl_p(&bootinfo.u.elf_sec.shndx, sections.shndx);
> +
> +        bootinfo_flags |= MULTIBOOT_INFO_ELF_SHDR;
>      }
>
> +    /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
> +    uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
> +    uint32_t mh_width = ldl_p(&multiboot_header->width);
> +    uint32_t mh_height = ldl_p(&multiboot_header->height);
> +    uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
> +
>      mbs.mb_buf_phys = mh_load_addr;
>
>      mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_kernel_size);
> @@ -332,7 +334,8 @@ int load_multiboot(FWCfgState *fw_cfg,
>      stl_p(&bootinfo.mods_count, mbs.mb_mods_count); /* mods_count */
>
>      /* the kernel is where we want it to be now */
> -    stl_p(&bootinfo.flags, MULTIBOOT_INFO_MEMORY
> +    stl_p(&bootinfo.flags, bootinfo_flags
> +                           | MULTIBOOT_INFO_MEMORY
>                             | MULTIBOOT_INFO_BOOTDEV
>                             | MULTIBOOT_INFO_CMDLINE
>                             | MULTIBOOT_INFO_MODS
> @@ -365,5 +368,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>      option_rom[nb_option_roms].bootindex = 0;
>      nb_option_roms++;
>
> +    fclose(f);
> +
>      return 1; /* yes, we are multiboot */
>  }
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d06fc12b6..4d9cc6261a 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -327,7 +327,7 @@ static int load_netboot_image(Error **errp)
>      }
>
>      img_size = load_elf_ram(netboot_filename, NULL, NULL, &ipl->start_addr,
> -                            NULL, NULL, 1, EM_S390, 0, 0, NULL, false);
> +                            NULL, NULL, 1, EM_S390, 0, 0, NULL, false, NULL);
>
>      if (img_size < 0) {
>          img_size = load_image_size(netboot_filename, ram_ptr, ram_size);
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index d192e7e2a3..7a7f7983a4 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -264,12 +264,13 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>                                int must_swab, uint64_t *pentry,
>                                uint64_t *lowaddr, uint64_t *highaddr,
>                                int elf_machine, int clear_lsb, int data_swab,
> -                              AddressSpace *as, bool load_rom)
> +                              AddressSpace *as, bool load_rom,
> +                              SectionsData *sections)
>  {
>      struct elfhdr ehdr;
>      struct elf_phdr *phdr = NULL, *ph;
>      int size, i, total_size;
> -    elf_word mem_size, file_size;
> +    elf_word mem_size, file_size, sec_size;
>      uint64_t addr, low = (uint64_t)-1, high = 0;
>      uint8_t *data = NULL;
>      char label[128];
> @@ -481,6 +482,108 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>          }
>      }
>      g_free(phdr);
> +
> +    if (sections) {
> +        struct elf_shdr *shdr = NULL, *sh;
> +        int shsize;
> +
> +        // user requested loading all ELF sections
> +        shsize = ehdr.e_shnum * sizeof(shdr[0]);
> +        if (lseek(fd, ehdr.e_shoff, SEEK_SET) != ehdr.e_shoff) {
> +            goto fail;
> +        }
> +        shdr = g_malloc0(shsize);
> +        if (!shdr)
> +            goto fail;
> +        if (read(fd, shdr, shsize) != shsize)
> +            goto fail;
> +        if (must_swab) {
> +            for (i = 0; i < ehdr.e_shnum; i++) {
> +                sh = &shdr[i];
> +                glue(bswap_shdr, SZ)(sh);
> +            }
> +        }
> +
> +        for(i = 0; i < ehdr.e_shnum; i++) {
> +            sh = &shdr[i];
> +            if (sh->sh_type == SHT_NULL)
> +                continue;
> +            // if section has address then it is loaded (XXX: is it true?), no need to load it again
> +            if (sh->sh_addr)
> +                continue;
> +
> +            // append section data at the end of the loaded segments
> +            addr = ROUND_UP(high, sh->sh_addralign);
> +            sh->sh_addr = addr;
> +
> +            sec_size = sh->sh_size; /* Size of the section data */
> +            data = g_malloc0(sec_size);
> +            if (sec_size > 0) {
> +                if (lseek(fd, sh->sh_offset, SEEK_SET) < 0) {
> +                    goto fail;
> +                }
> +                if (read(fd, data, sec_size) != sec_size) {
> +                    goto fail;
> +                }
> +            }
> +
> +            // As these sections are not loadable no need to perform reloaction
> +            // using translate_fn()
> +
> +            if (data_swab) {
> +                int j;
> +                for (j = 0; j < sec_size; j += (1 << data_swab)) {
> +                    uint8_t *dp = data + j;
> +                    switch (data_swab) {
> +                    case (1):
> +                        *(uint16_t *)dp = bswap16(*(uint16_t *)dp);
> +                        break;
> +                    case (2):
> +                        *(uint32_t *)dp = bswap32(*(uint32_t *)dp);
> +                        break;
> +                    case (3):
> +                        *(uint64_t *)dp = bswap64(*(uint64_t *)dp);
> +                        break;
> +                    default:
> +                        g_assert_not_reached();
> +                    }
> +                }
> +            }
> +
> +            if (load_rom) {
> +                snprintf(label, sizeof(label), "shdr #%d: %s", i, name);
> +
> +                /* rom_add_elf_program() seize the ownership of 'data' */
> +                rom_add_elf_program(label, data, sec_size, sec_size, addr, as);
> +            } else {
> +                cpu_physical_memory_write(addr, data, sec_size);
> +                g_free(data);
> +            }
> +
> +            total_size += sec_size;
> +            high = addr + sec_size;
> +
> +            data = NULL;
> +        }
> +
> +        sections->num = ehdr.e_shnum;
> +        sections->size = ehdr.e_shentsize;
> +        sections->addr = high; // Address where we load the sections header
> +        sections->shndx = ehdr.e_shstrndx;
> +
> +        // Append section headers at the end of loaded segments/sections
> +        if (load_rom) {
> +            snprintf(label, sizeof(label), "shdrs");
> +
> +            /* rom_add_elf_program() seize the ownership of 'shdr' */
> +            rom_add_elf_program(label, shdr, shsize, shsize, high, as);
> +        } else {
> +            cpu_physical_memory_write(high, shdr, shsize);
> +            g_free(shdr);
> +        }
> +        high += shsize;
> +    }
> +
>      if (lowaddr)
>          *lowaddr = (uint64_t)(elf_sword)low;
>      if (highaddr)
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 355fe0f5a2..3cf2d6da0f 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -65,6 +65,13 @@ int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
>  #define ELF_LOAD_WRONG_ENDIAN -4
>  const char *load_elf_strerror(int error);
>
> +typedef struct {
> +    uint32_t num; // number of loaded sections
> +    uint32_t size; // size of entry in sections header list
> +    uint32_t addr; // address of loaded sections header
> +    uint32_t shndx; // number of section that contains string table
> +} SectionsData;
> +
>  /** load_elf_ram:
>   * @filename: Path of ELF file
>   * @translate_fn: optional function to translate load addresses
> @@ -82,6 +89,8 @@ const char *load_elf_strerror(int error);
>   * @as: The AddressSpace to load the ELF to. The value of address_space_memory
>   *      is used if nothing is supplied here.
>   * @load_rom : Load ELF binary as ROM
> + * @sections: If parameter is non-NULL then all ELF sections loaded into memory
> + *      and this structure is populated.
>   *
>   * Load an ELF file's contents to the emulated system's address space.
>   * Clients may optionally specify a callback to perform address
> @@ -99,7 +108,7 @@ int load_elf_ram(const char *filename,
>                   void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
>                   uint64_t *highaddr, int big_endian, int elf_machine,
>                   int clear_lsb, int data_swab, AddressSpace *as,
> -                 bool load_rom);
> +                 bool load_rom, SectionsData *sections);
>
>  /** load_elf_as:
>   * Same as load_elf_ram(), but always loads the elf as ROM
> diff --git a/tests/multiboot/Makefile b/tests/multiboot/Makefile
> index 36f01dc647..b6a5056347 100644
> --- a/tests/multiboot/Makefile
> +++ b/tests/multiboot/Makefile
> @@ -6,7 +6,7 @@ LD=ld
>  LDFLAGS=-melf_i386 -T link.ld
>  LIBS=$(shell $(CC) $(CCFLAGS) -print-libgcc-file-name)
>
> -all: mmap.elf modules.elf
> +all: mmap.elf modules.elf sections.elf sections.out
>
>  mmap.elf: start.o mmap.o libc.o
>         $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
> @@ -14,6 +14,12 @@ mmap.elf: start.o mmap.o libc.o
>  modules.elf: start.o modules.o libc.o
>         $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
>
> +sections.elf: start.o sections.o libc.o
> +       $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
> +
> +sections.out: sections.elf generate_sections_out.py
> +       python2 generate_sections_out.py $^ > $@
> +
>  %.o: %.c
>         $(CC) $(CCFLAGS) -c -o $@ $^
>
> diff --git a/tests/multiboot/generate_sections_out.py b/tests/multiboot/generate_sections_out.py
> new file mode 100755
> index 0000000000..8077856823
> --- /dev/null
> +++ b/tests/multiboot/generate_sections_out.py
> @@ -0,0 +1,33 @@
> +#!/usr/bin/env python2
> +
> +import sys
> +
> +from elftools.elf.elffile import ELFFile
> +
> +def roundup(num, align):
> +  return (num + align - 1) / align * align
> +
> +def main(filename):
> +  print "\n\n\n=== Running test case: sections.elf  ===\n"
> +  print "Multiboot header at 9500, ELF section headers present\n"
> +
> +  with open(filename, 'rb') as f:
> +    elf = ELFFile(f)
> +    header = elf.header
> +    load_addr = 0x100000  # we load image starting from 1M
> +    sections = ""
> +    for s in elf.iter_sections():
> +      align = s.header.sh_addralign
> +      addr = 0
> +      if s.header.sh_type != 'SHT_NULL':
> +        addr = s.header.sh_addr
> +        if addr == 0:
> +          addr = roundup(load_addr, s.header.sh_addralign)
> +        load_addr = addr + s.header.sh_size
> +      sections += "Elf section name=%s addr=%x size=%d\n" % (s.name, addr, s.header.sh_size)
> +
> +    print "Sections list with %d entries of size %d at %x, string index %d" % (header.e_shnum, header.e_shentsize, load_addr, header.e_shstrndx)
> +    print sections,
> +
> +if __name__ == '__main__':
> +  main(sys.argv[1])
> diff --git a/tests/multiboot/modules.out b/tests/multiboot/modules.out
> index 0da7b39374..b6c1a01be1 100644
> --- a/tests/multiboot/modules.out
> +++ b/tests/multiboot/modules.out
> @@ -5,15 +5,15 @@
>
>  Multiboot header at 9500
>
> -Module list with 0 entries at 102000
> +Module list with 0 entries at 104000
>
>
>  === Running test case: modules.elf -initrd module.txt ===
>
>  Multiboot header at 9500
>
> -Module list with 1 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
> +Module list with 1 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt'
>           Content: 'This is a test file that is used as a multiboot module.'
>
>
> @@ -21,8 +21,8 @@ Module list with 1 entries at 102000
>
>  Multiboot header at 9500
>
> -Module list with 1 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument'
> +Module list with 1 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt argument'
>           Content: 'This is a test file that is used as a multiboot module.'
>
>
> @@ -30,8 +30,8 @@ Module list with 1 entries at 102000
>
>  Multiboot header at 9500
>
> -Module list with 1 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument,with,commas'
> +Module list with 1 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt argument,with,commas'
>           Content: 'This is a test file that is used as a multiboot module.'
>
>
> @@ -39,10 +39,10 @@ Module list with 1 entries at 102000
>
>  Multiboot header at 9500
>
> -Module list with 3 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
> +Module list with 3 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt'
>           Content: 'This is a test file that is used as a multiboot module.'
> -[102010] Module: 104000 - 104038 (56 bytes) 'module.txt argument'
> +[104010] Module: 106000 - 106038 (56 bytes) 'module.txt argument'
>           Content: 'This is a test file that is used as a multiboot module.'
> -[102020] Module: 105000 - 105038 (56 bytes) 'module.txt'
> +[104020] Module: 107000 - 107038 (56 bytes) 'module.txt'
>           Content: 'This is a test file that is used as a multiboot module.'
> diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
> index 0278148b43..f04e35cbf0 100755
> --- a/tests/multiboot/run_test.sh
> +++ b/tests/multiboot/run_test.sh
> @@ -56,9 +56,13 @@ modules() {
>      run_qemu modules.elf -initrd "module.txt,module.txt argument,module.txt"
>  }
>
> +sections() {
> +    run_qemu sections.elf
> +}
> +
>  make all
>
> -for t in mmap modules; do
> +for t in mmap modules sections; do
>
>      echo > test.log
>      $t
> diff --git a/tests/multiboot/sections.c b/tests/multiboot/sections.c
> new file mode 100644
> index 0000000000..05a88f99ac
> --- /dev/null
> +++ b/tests/multiboot/sections.c
> @@ -0,0 +1,55 @@
> +/*
> + * Copyright (c) 2017 Anatol Pomozov <anatol.pomozov@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "libc.h"
> +#include "../../hw/i386/multiboot_header.h"
> +#include "../../include/elf.h"
> +
> +int test_main(uint32_t magic, struct multiboot_info *mbi)
> +{
> +    void *p;
> +    unsigned int i;
> +
> +    (void) magic;
> +    multiboot_elf_section_header_table_t shdr;
> +
> +    printf("Multiboot header at %x, ELF section headers %s\n\n", mbi,
> +        mbi->flags & MULTIBOOT_INFO_ELF_SHDR ? "present" : "don't present");
> +
> +    shdr = mbi->u.elf_sec;
> +    printf("Sections list with %d entries of size %d at %x, string index %d\n",
> +        shdr.num, shdr.size, shdr.addr, shdr.shndx);
> +
> +    const char *string_table = (char *)((Elf32_Shdr *)(uintptr_t)(shdr.addr + shdr.shndx * shdr.size))->sh_addr;
> +
> +    for (i = 0, p = (void *)shdr.addr;
> +         i < shdr.num;
> +         i++, p += shdr.size)
> +    {
> +        Elf32_Shdr *sec;
> +
> +        sec = (Elf32_Shdr *)p;
> +        printf("Elf section name=%s addr=%lx size=%ld\n", string_table + sec->sh_name, sec->sh_addr, sec->sh_size);
> +    }
> +
> +    return 0;
> +}
> diff --git a/tests/multiboot/start.S b/tests/multiboot/start.S
> index 7d33959650..bd404100c2 100644
> --- a/tests/multiboot/start.S
> +++ b/tests/multiboot/start.S
> @@ -23,7 +23,7 @@
>  .section multiboot
>
>  #define MB_MAGIC 0x1badb002
> -#define MB_FLAGS 0x0
> +#define MB_FLAGS 0x2
>  #define MB_CHECKSUM -(MB_MAGIC + MB_FLAGS)
>
>  .align  4
> --
> 2.15.0.rc0.271.g36b669edcc-goog
>

Kevin Wolf Oct. 19, 2017, 9:36 a.m. UTC | #2

Am 18.10.2017 um 19:22 hat Anatol Pomozov geschrieben:
> Hello qemu-devs,
> 
> This patchset has been posted a while ago. I would like to move
> forward with it and look at the next task (e.g. multiboot2 support in
> QEMU). Who is the multiboot code maintainer who can help to review
> this patchset and give go/no-go answer?

I think that's exactly your problem, there is nobody who feels
responsible for Multiboot support. Officially, it is part of the PC/x86
subsystems:

$ scripts/get_maintainer.pl -f hw/i386/multiboot.c
"Michael S. Tsirkin" <mst@redhat.com> (supporter:PC)
Paolo Bonzini <pbonzini@redhat.com> (maintainer:X86)
Richard Henderson <rth@twiddle.net> (maintainer:X86)
Eduardo Habkost <ehabkost@redhat.com> (maintainer:X86)
qemu-devel@nongnu.org (open list:All patches CC here)

I'm not sure if any of them know much about Multiboot. Myself, I am
personally interested in having Multiboot support, which is why I've
even commented on your patches, but I'm not the maintainer.

The last few Multiboot patches seem to have been merged by Paolo.

Kevin

Anatol Pomozov Oct. 31, 2017, 6:38 p.m. UTC | #3

Hi

On Thu, Oct 19, 2017 at 2:36 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 18.10.2017 um 19:22 hat Anatol Pomozov geschrieben:
>> Hello qemu-devs,
>>
>> This patchset has been posted a while ago. I would like to move
>> forward with it and look at the next task (e.g. multiboot2 support in
>> QEMU). Who is the multiboot code maintainer who can help to review
>> this patchset and give go/no-go answer?
>
> I think that's exactly your problem, there is nobody who feels
> responsible for Multiboot support. Officially, it is part of the PC/x86
> subsystems:

In this case I would like to become the official Qemu/Multiboot
maintainer. I do development related to x86 osdev and actively use
qemu for my projects. I am interested in having a feature-rich and
robust Multiboot implementation. In mid term I see several tasks for
me:
 - finish and merge my cleanup + MULTIBOOT_INFO_ELF_SHDR patch series
 - add Multiboot2 support
 - (optional) look at MIPS support for Multiboot. I do not have/use
MIPS but it is beneficial to add multiplatform support to Multiboot
 - reviewing incoming patches


A bit about me. My name is Anatol Pomozov and at my spare time I
participate at several open sources projects. I am an Arch developer
since 2013 where I maintain a bunch of packages (including Arch qemu
package [1]). I have ~40 patches in the Linux kernel tree, where the
biggest contribution is a driver for audio codec NAU8825. I
contributed a lot of small patches to numerous projects (compilers,
toolchains, ...)

[1] https://www.archlinux.org/packages/extra/i686/qemu/

Anatol Pomozov Nov. 17, 2017, 9:33 p.m. UTC | #4

Hello

On Tue, Oct 31, 2017 at 11:38 AM, Anatol Pomozov
<anatol.pomozov@gmail.com> wrote:
> Hi
>
> On Thu, Oct 19, 2017 at 2:36 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 18.10.2017 um 19:22 hat Anatol Pomozov geschrieben:
>>> Hello qemu-devs,
>>>
>>> This patchset has been posted a while ago. I would like to move
>>> forward with it and look at the next task (e.g. multiboot2 support in
>>> QEMU). Who is the multiboot code maintainer who can help to review
>>> this patchset and give go/no-go answer?
>>
>> I think that's exactly your problem, there is nobody who feels
>> responsible for Multiboot support. Officially, it is part of the PC/x86
>> subsystems:
>
> In this case I would like to become the official Qemu/Multiboot
> maintainer. I do development related to x86 osdev and actively use
> qemu for my projects. I am interested in having a feature-rich and
> robust Multiboot implementation.

It is very unfortunate to see lack of interest in improving
qemu/multiboot functionality. I guess my best option for now is to
avoid using qemu multiboot implementation and use grub instead.

Kevin Wolf Nov. 20, 2017, 4:45 p.m. UTC | #5

Am 17.11.2017 um 22:33 hat Anatol Pomozov geschrieben:
> On Tue, Oct 31, 2017 at 11:38 AM, Anatol Pomozov
> <anatol.pomozov@gmail.com> wrote:
> > Hi
> >
> > On Thu, Oct 19, 2017 at 2:36 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> >> Am 18.10.2017 um 19:22 hat Anatol Pomozov geschrieben:
> >>> Hello qemu-devs,
> >>>
> >>> This patchset has been posted a while ago. I would like to move
> >>> forward with it and look at the next task (e.g. multiboot2 support in
> >>> QEMU). Who is the multiboot code maintainer who can help to review
> >>> this patchset and give go/no-go answer?
> >>
> >> I think that's exactly your problem, there is nobody who feels
> >> responsible for Multiboot support. Officially, it is part of the PC/x86
> >> subsystems:
> >
> > In this case I would like to become the official Qemu/Multiboot
> > maintainer. I do development related to x86 osdev and actively use
> > qemu for my projects. I am interested in having a feature-rich and
> > robust Multiboot implementation.
> 
> It is very unfortunate to see lack of interest in improving
> qemu/multiboot functionality. I guess my best option for now is to
> avoid using qemu multiboot implementation and use grub instead.

Let's wait until 2.11 is out and we're out of freeze, and then I think
your best bet is to just prepare a pull request that adds you to
MAINTAINERS and applies the patches you created so far.

Peter, does this sound right?

Kevin

Kevin Wolf Jan. 15, 2018, 3:41 p.m. UTC | #6

Am 13.10.2017 um 01:54 hat Anatol Pomozov geschrieben:
> Multiboot may load section headers and all sections (even those that are
> not part of any segment) to target memory.
> 
> Tested with an ELF application that uses data from strings table
> section.
> 
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
>  hw/core/loader.c                         |   8 +--
>  hw/i386/multiboot.c                      |  83 +++++++++++++-----------
>  hw/s390x/ipl.c                           |   2 +-
>  include/hw/elf_ops.h                     | 107 ++++++++++++++++++++++++++++++-
>  include/hw/loader.h                      |  11 +++-
>  tests/multiboot/Makefile                 |   8 ++-
>  tests/multiboot/generate_sections_out.py |  33 ++++++++++
>  tests/multiboot/modules.out              |  22 +++----
>  tests/multiboot/run_test.sh              |   6 +-
>  tests/multiboot/sections.c               |  55 ++++++++++++++++
>  tests/multiboot/start.S                  |   2 +-
>  11 files changed, 276 insertions(+), 61 deletions(-)
>  create mode 100755 tests/multiboot/generate_sections_out.py
>  create mode 100644 tests/multiboot/sections.c
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 4593061445..a8787f7685 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -439,7 +439,7 @@ int load_elf_as(const char *filename,
>  {
>      return load_elf_ram(filename, translate_fn, translate_opaque,
>                          pentry, lowaddr, highaddr, big_endian, elf_machine,
> -                        clear_lsb, data_swab, as, true);
> +                        clear_lsb, data_swab, as, true, NULL);
>  }
>  
>  /* return < 0 if error, otherwise the number of bytes loaded in memory */
> @@ -448,7 +448,7 @@ int load_elf_ram(const char *filename,
>                   void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
>                   uint64_t *highaddr, int big_endian, int elf_machine,
>                   int clear_lsb, int data_swab, AddressSpace *as,
> -                 bool load_rom)
> +                 bool load_rom, SectionsData *sections)
>  {
>      int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
>      uint8_t e_ident[EI_NIDENT];
> @@ -488,11 +488,11 @@ int load_elf_ram(const char *filename,
>      if (e_ident[EI_CLASS] == ELFCLASS64) {
>          ret = load_elf64(filename, fd, translate_fn, translate_opaque, must_swab,
>                           pentry, lowaddr, highaddr, elf_machine, clear_lsb,
> -                         data_swab, as, load_rom);
> +                         data_swab, as, load_rom, sections);
>      } else {
>          ret = load_elf32(filename, fd, translate_fn, translate_opaque, must_swab,
>                           pentry, lowaddr, highaddr, elf_machine, clear_lsb,
> -                         data_swab, as, load_rom);
> +                         data_swab, as, load_rom, sections);
>      }
>  
>   fail:
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index 7dacd6d827..841d05160a 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -125,7 +125,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>  {
>      int i;
>      bool is_multiboot = false;
> -    uint32_t flags = 0;
> +    uint32_t flags = 0, bootinfo_flags = 0;
>      uint32_t mh_entry_addr;
>      uint32_t mh_load_addr;
>      uint32_t mb_kernel_size;
> @@ -134,6 +134,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>      uint8_t *mb_bootinfo_data;
>      uint32_t cmdline_len;
>      struct multiboot_header *multiboot_header;
> +    SectionsData sections;
>  
>      /* Ok, let's see if it is a multiboot image.
>         The header is 12x32bit long, so the latest entry may be 8192 - 48. */
> @@ -161,37 +162,8 @@ int load_multiboot(FWCfgState *fw_cfg,
>      if (flags & MULTIBOOT_VIDEO_MODE) {
>          fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n");
>      }
> -    if (!(flags & MULTIBOOT_AOUT_KLUDGE)) {
> -        uint64_t elf_entry;
> -        uint64_t elf_low, elf_high;
> -        int kernel_size;
> -        fclose(f);
>  
> -        if (((struct elf64_hdr*)header)->e_machine == EM_X86_64) {
> -            fprintf(stderr, "Cannot load x86-64 image, give a 32bit one.\n");
> -            exit(1);
> -        }

I'm not sure why you're reversing the condition and moving this branch
down, but in the code movements the EM_X86_64 check got lost. Please
keep it, we don't support 64 bit ELFs at the moment. If you want to
change this, it should be a separate patch.

> -        kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
> -                               &elf_low, &elf_high, 0, EM_NONE,
> -                               0, 0);
> -        if (kernel_size < 0) {
> -            fprintf(stderr, "Error while loading elf kernel\n");
> -            exit(1);
> -        }
> -        mh_load_addr = elf_low;
> -        mb_kernel_size = elf_high - elf_low;
> -        mh_entry_addr = elf_entry;
> -
> -        mbs.mb_buf = g_malloc(mb_kernel_size);
> -        if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
> -            fprintf(stderr, "Error while fetching elf kernel from rom\n");
> -            exit(1);
> -        }
> -
> -        mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
> -                  mb_kernel_size, (size_t)mh_entry_addr);
> -    } else {
> +    if (flags & MULTIBOOT_AOUT_KLUDGE) {
>          /* Valid if mh_flags sets MULTIBOOT_AOUT_KLUDGE. */
>          uint32_t mh_header_addr = ldl_p(&multiboot_header->header_addr);
>          uint32_t mh_load_end_addr = ldl_p(&multiboot_header->load_end_addr);
> @@ -228,12 +200,6 @@ int load_multiboot(FWCfgState *fw_cfg,
>              mb_load_size = mb_kernel_size;
>          }
>  
> -        /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
> -        uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
> -        uint32_t mh_width = ldl_p(&multiboot_header->width);
> -        uint32_t mh_height = ldl_p(&multiboot_header->height);
> -        uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
> -
>          mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
>          mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
>          mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
> @@ -248,9 +214,45 @@ int load_multiboot(FWCfgState *fw_cfg,
>              exit(1);
>          }
>          memset(mbs.mb_buf + mb_load_size, 0, mb_kernel_size - mb_load_size);
> -        fclose(f);
> +    } else {
> +        uint64_t elf_entry;
> +        uint64_t elf_low, elf_high;
> +        int kernel_size;
> +
> +        kernel_size = load_elf_ram(kernel_filename, NULL, NULL, &elf_entry,
> +                               &elf_low, &elf_high, 0, I386_ELF_MACHINE,
> +                               0, 0, NULL, true, &sections);
> +        if (kernel_size < 0) {
> +            fprintf(stderr, "Error while loading elf kernel\n");
> +            exit(1);
> +        }
> +        mh_load_addr = elf_low;
> +        mb_kernel_size = elf_high - elf_low;
> +        mh_entry_addr = elf_entry;
> +
> +        mbs.mb_buf = g_malloc(mb_kernel_size);
> +        if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
> +            fprintf(stderr, "Error while fetching elf kernel from rom\n");
> +            exit(1);
> +        }
> +
> +        mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
> +                  mb_kernel_size, (size_t)mh_entry_addr);
> +
> +        stl_p(&bootinfo.u.elf_sec.num, sections.num);
> +        stl_p(&bootinfo.u.elf_sec.size, sections.size);
> +        stl_p(&bootinfo.u.elf_sec.addr, sections.addr);
> +        stl_p(&bootinfo.u.elf_sec.shndx, sections.shndx);
> +
> +        bootinfo_flags |= MULTIBOOT_INFO_ELF_SHDR;
>      }
>  
> +    /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
> +    uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
> +    uint32_t mh_width = ldl_p(&multiboot_header->width);
> +    uint32_t mh_height = ldl_p(&multiboot_header->height);
> +    uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
> +
>      mbs.mb_buf_phys = mh_load_addr;
>  
>      mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_kernel_size);
> @@ -332,7 +334,8 @@ int load_multiboot(FWCfgState *fw_cfg,
>      stl_p(&bootinfo.mods_count, mbs.mb_mods_count); /* mods_count */
>  
>      /* the kernel is where we want it to be now */
> -    stl_p(&bootinfo.flags, MULTIBOOT_INFO_MEMORY
> +    stl_p(&bootinfo.flags, bootinfo_flags
> +                           | MULTIBOOT_INFO_MEMORY
>                             | MULTIBOOT_INFO_BOOTDEV
>                             | MULTIBOOT_INFO_CMDLINE
>                             | MULTIBOOT_INFO_MODS
> @@ -365,5 +368,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>      option_rom[nb_option_roms].bootindex = 0;
>      nb_option_roms++;
>  
> +    fclose(f);
> +
>      return 1; /* yes, we are multiboot */
>  }
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d06fc12b6..4d9cc6261a 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -327,7 +327,7 @@ static int load_netboot_image(Error **errp)
>      }
>  
>      img_size = load_elf_ram(netboot_filename, NULL, NULL, &ipl->start_addr,
> -                            NULL, NULL, 1, EM_S390, 0, 0, NULL, false);
> +                            NULL, NULL, 1, EM_S390, 0, 0, NULL, false, NULL);
>  
>      if (img_size < 0) {
>          img_size = load_image_size(netboot_filename, ram_ptr, ram_size);
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index d192e7e2a3..7a7f7983a4 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h

The code in this file is rather old and not quite compliant with the
QEMU coding style. This is not an excuse not to apply the correct coding
style to new additions to the file:

> @@ -264,12 +264,13 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>                                int must_swab, uint64_t *pentry,
>                                uint64_t *lowaddr, uint64_t *highaddr,
>                                int elf_machine, int clear_lsb, int data_swab,
> -                              AddressSpace *as, bool load_rom)
> +                              AddressSpace *as, bool load_rom,
> +                              SectionsData *sections)
>  {
>      struct elfhdr ehdr;
>      struct elf_phdr *phdr = NULL, *ph;
>      int size, i, total_size;
> -    elf_word mem_size, file_size;
> +    elf_word mem_size, file_size, sec_size;
>      uint64_t addr, low = (uint64_t)-1, high = 0;
>      uint8_t *data = NULL;
>      char label[128];
> @@ -481,6 +482,108 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>          }
>      }
>      g_free(phdr);
> +
> +    if (sections) {
> +        struct elf_shdr *shdr = NULL, *sh;
> +        int shsize;
> +
> +        // user requested loading all ELF sections

Please use C-style comments: /* ... */

> +        shsize = ehdr.e_shnum * sizeof(shdr[0]);
> +        if (lseek(fd, ehdr.e_shoff, SEEK_SET) != ehdr.e_shoff) {
> +            goto fail;
> +        }
> +        shdr = g_malloc0(shsize);
> +        if (!shdr)
> +            goto fail;

g_malloc0() never returns NULL. If you want it to return NULL instead of
aborting qemu when the allocation fails, you need g_try_malloc0().

Also, the QEMU coding style requires braces after an if condition. I'm
mentioning this only here, but it applies to multiple places in the
whole patch.

> +        if (read(fd, shdr, shsize) != shsize)
> +            goto fail;
> +        if (must_swab) {
> +            for (i = 0; i < ehdr.e_shnum; i++) {
> +                sh = &shdr[i];
> +                glue(bswap_shdr, SZ)(sh);
> +            }
> +        }
> +
> +        for(i = 0; i < ehdr.e_shnum; i++) {

A space character is required between for and the bracket.

> +            sh = &shdr[i];
> +            if (sh->sh_type == SHT_NULL)
> +                continue;
> +            // if section has address then it is loaded (XXX: is it true?), no need to load it again

This exceeds the limit of 80 characters per line.

> +            if (sh->sh_addr)
> +                continue;
> +
> +            // append section data at the end of the loaded segments
> +            addr = ROUND_UP(high, sh->sh_addralign);
> +            sh->sh_addr = addr;
> +
> +            sec_size = sh->sh_size; /* Size of the section data */
> +            data = g_malloc0(sec_size);
> +            if (sec_size > 0) {
> +                if (lseek(fd, sh->sh_offset, SEEK_SET) < 0) {
> +                    goto fail;
> +                }
> +                if (read(fd, data, sec_size) != sec_size) {
> +                    goto fail;
> +                }
> +            }
> +
> +            // As these sections are not loadable no need to perform reloaction
> +            // using translate_fn()
> +
> +            if (data_swab) {
> +                int j;
> +                for (j = 0; j < sec_size; j += (1 << data_swab)) {

Is sec_size guaranteed to be aligned? I don't see a check to verify
this. Otherwise, could we call bswap64() on the last byte of section,
accessing seven more bytes outside the allocated buffer?

> +                    uint8_t *dp = data + j;
> +                    switch (data_swab) {
> +                    case (1):

Why 'case (1):' instead of 'case 1:' looks odd.

> +                        *(uint16_t *)dp = bswap16(*(uint16_t *)dp);
> +                        break;
> +                    case (2):
> +                        *(uint32_t *)dp = bswap32(*(uint32_t *)dp);
> +                        break;
> +                    case (3):
> +                        *(uint64_t *)dp = bswap64(*(uint64_t *)dp);
> +                        break;
> +                    default:
> +                        g_assert_not_reached();
> +                    }
> +                }
> +            }
> +
> +            if (load_rom) {
> +                snprintf(label, sizeof(label), "shdr #%d: %s", i, name);
> +
> +                /* rom_add_elf_program() seize the ownership of 'data' */
> +                rom_add_elf_program(label, data, sec_size, sec_size, addr, as);
> +            } else {
> +                cpu_physical_memory_write(addr, data, sec_size);
> +                g_free(data);
> +            }
> +
> +            total_size += sec_size;
> +            high = addr + sec_size;
> +
> +            data = NULL;
> +        }
> +
> +        sections->num = ehdr.e_shnum;
> +        sections->size = ehdr.e_shentsize;
> +        sections->addr = high; // Address where we load the sections header
> +        sections->shndx = ehdr.e_shstrndx;
> +
> +        // Append section headers at the end of loaded segments/sections
> +        if (load_rom) {
> +            snprintf(label, sizeof(label), "shdrs");
> +
> +            /* rom_add_elf_program() seize the ownership of 'shdr' */
> +            rom_add_elf_program(label, shdr, shsize, shsize, high, as);
> +        } else {
> +            cpu_physical_memory_write(high, shdr, shsize);
> +            g_free(shdr);
> +        }
> +        high += shsize;
> +    }
> +
>      if (lowaddr)
>          *lowaddr = (uint64_t)(elf_sword)low;
>      if (highaddr)
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 355fe0f5a2..3cf2d6da0f 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -65,6 +65,13 @@ int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
>  #define ELF_LOAD_WRONG_ENDIAN -4
>  const char *load_elf_strerror(int error);
>  
> +typedef struct {
> +    uint32_t num; // number of loaded sections
> +    uint32_t size; // size of entry in sections header list
> +    uint32_t addr; // address of loaded sections header
> +    uint32_t shndx; // number of section that contains string table
> +} SectionsData;
> +
>  /** load_elf_ram:
>   * @filename: Path of ELF file
>   * @translate_fn: optional function to translate load addresses
> @@ -82,6 +89,8 @@ const char *load_elf_strerror(int error);
>   * @as: The AddressSpace to load the ELF to. The value of address_space_memory
>   *      is used if nothing is supplied here.
>   * @load_rom : Load ELF binary as ROM
> + * @sections: If parameter is non-NULL then all ELF sections loaded into memory
> + *      and this structure is populated.
>   *
>   * Load an ELF file's contents to the emulated system's address space.
>   * Clients may optionally specify a callback to perform address
> @@ -99,7 +108,7 @@ int load_elf_ram(const char *filename,
>                   void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
>                   uint64_t *highaddr, int big_endian, int elf_machine,
>                   int clear_lsb, int data_swab, AddressSpace *as,
> -                 bool load_rom);
> +                 bool load_rom, SectionsData *sections);
>  
>  /** load_elf_as:
>   * Same as load_elf_ram(), but always loads the elf as ROM
> diff --git a/tests/multiboot/Makefile b/tests/multiboot/Makefile
> index 36f01dc647..b6a5056347 100644
> --- a/tests/multiboot/Makefile
> +++ b/tests/multiboot/Makefile
> @@ -6,7 +6,7 @@ LD=ld
>  LDFLAGS=-melf_i386 -T link.ld
>  LIBS=$(shell $(CC) $(CCFLAGS) -print-libgcc-file-name)
>  
> -all: mmap.elf modules.elf
> +all: mmap.elf modules.elf sections.elf sections.out
>  
>  mmap.elf: start.o mmap.o libc.o
>  	$(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
> @@ -14,6 +14,12 @@ mmap.elf: start.o mmap.o libc.o
>  modules.elf: start.o modules.o libc.o
>  	$(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
>  
> +sections.elf: start.o sections.o libc.o
> +	$(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
> +
> +sections.out: sections.elf generate_sections_out.py
> +	python2 generate_sections_out.py $^ > $@
> +
>  %.o: %.c
>  	$(CC) $(CCFLAGS) -c -o $@ $^
>  
> diff --git a/tests/multiboot/generate_sections_out.py b/tests/multiboot/generate_sections_out.py
> new file mode 100755
> index 0000000000..8077856823
> --- /dev/null
> +++ b/tests/multiboot/generate_sections_out.py
> @@ -0,0 +1,33 @@
> +#!/usr/bin/env python2
> +
> +import sys
> +
> +from elftools.elf.elffile import ELFFile

I don't think we can expect this module to be present. For example, on
my system, attempting to run the test fails:

ImportError: No module named elftools.elf.elffile

Maybe we can parse the output of readelf instead?

> +def roundup(num, align):
> +  return (num + align - 1) / align * align
> +
> +def main(filename):
> +  print "\n\n\n=== Running test case: sections.elf  ===\n"
> +  print "Multiboot header at 9500, ELF section headers present\n"
> +
> +  with open(filename, 'rb') as f:
> +    elf = ELFFile(f)
> +    header = elf.header
> +    load_addr = 0x100000  # we load image starting from 1M
> +    sections = ""
> +    for s in elf.iter_sections():
> +      align = s.header.sh_addralign
> +      addr = 0
> +      if s.header.sh_type != 'SHT_NULL':
> +        addr = s.header.sh_addr
> +        if addr == 0:
> +          addr = roundup(load_addr, s.header.sh_addralign)
> +        load_addr = addr + s.header.sh_size
> +      sections += "Elf section name=%s addr=%x size=%d\n" % (s.name, addr, s.header.sh_size)
> +
> +    print "Sections list with %d entries of size %d at %x, string index %d" % (header.e_shnum, header.e_shentsize, load_addr, header.e_shstrndx)

We're not as strict about the 80 characters rule in Python code, but I
think this line could use being wrapped.

> +    print sections,
> +
> +if __name__ == '__main__':
> +  main(sys.argv[1])
> diff --git a/tests/multiboot/modules.out b/tests/multiboot/modules.out
> index 0da7b39374..b6c1a01be1 100644
> --- a/tests/multiboot/modules.out
> +++ b/tests/multiboot/modules.out
> @@ -5,15 +5,15 @@
>  
>  Multiboot header at 9500
>  
> -Module list with 0 entries at 102000
> +Module list with 0 entries at 104000
>  
>  
>  === Running test case: modules.elf -initrd module.txt ===
>  
>  Multiboot header at 9500
>  
> -Module list with 1 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
> +Module list with 1 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt'
>           Content: 'This is a test file that is used as a multiboot module.'
>  
>  
> @@ -21,8 +21,8 @@ Module list with 1 entries at 102000
>  
>  Multiboot header at 9500
>  
> -Module list with 1 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument'
> +Module list with 1 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt argument'
>           Content: 'This is a test file that is used as a multiboot module.'
>  
>  
> @@ -30,8 +30,8 @@ Module list with 1 entries at 102000
>  
>  Multiboot header at 9500
>  
> -Module list with 1 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument,with,commas'
> +Module list with 1 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt argument,with,commas'
>           Content: 'This is a test file that is used as a multiboot module.'
>  
>  
> @@ -39,10 +39,10 @@ Module list with 1 entries at 102000
>  
>  Multiboot header at 9500
>  
> -Module list with 3 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
> +Module list with 3 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt'
>           Content: 'This is a test file that is used as a multiboot module.'
> -[102010] Module: 104000 - 104038 (56 bytes) 'module.txt argument'
> +[104010] Module: 106000 - 106038 (56 bytes) 'module.txt argument'
>           Content: 'This is a test file that is used as a multiboot module.'
> -[102020] Module: 105000 - 105038 (56 bytes) 'module.txt'
> +[104020] Module: 107000 - 107038 (56 bytes) 'module.txt'
>           Content: 'This is a test file that is used as a multiboot module.'
> diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
> index 0278148b43..f04e35cbf0 100755
> --- a/tests/multiboot/run_test.sh
> +++ b/tests/multiboot/run_test.sh
> @@ -56,9 +56,13 @@ modules() {
>      run_qemu modules.elf -initrd "module.txt,module.txt argument,module.txt"
>  }
>  
> +sections() {
> +    run_qemu sections.elf
> +}
> +
>  make all
>  
> -for t in mmap modules; do
> +for t in mmap modules sections; do
>  
>      echo > test.log
>      $t
> diff --git a/tests/multiboot/sections.c b/tests/multiboot/sections.c
> new file mode 100644
> index 0000000000..05a88f99ac
> --- /dev/null
> +++ b/tests/multiboot/sections.c
> @@ -0,0 +1,55 @@
> +/*
> + * Copyright (c) 2017 Anatol Pomozov <anatol.pomozov@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "libc.h"
> +#include "../../hw/i386/multiboot_header.h"
> +#include "../../include/elf.h"
> +
> +int test_main(uint32_t magic, struct multiboot_info *mbi)
> +{
> +    void *p;
> +    unsigned int i;
> +
> +    (void) magic;
> +    multiboot_elf_section_header_table_t shdr;
> +
> +    printf("Multiboot header at %x, ELF section headers %s\n\n", mbi,
> +        mbi->flags & MULTIBOOT_INFO_ELF_SHDR ? "present" : "don't present");
> +
> +    shdr = mbi->u.elf_sec;
> +    printf("Sections list with %d entries of size %d at %x, string index %d\n",
> +        shdr.num, shdr.size, shdr.addr, shdr.shndx);
> +
> +    const char *string_table = (char *)((Elf32_Shdr *)(uintptr_t)(shdr.addr + shdr.shndx * shdr.size))->sh_addr;

80 characters again.

> +
> +    for (i = 0, p = (void *)shdr.addr;
> +         i < shdr.num;
> +         i++, p += shdr.size)
> +    {
> +        Elf32_Shdr *sec;
> +
> +        sec = (Elf32_Shdr *)p;
> +        printf("Elf section name=%s addr=%lx size=%ld\n", string_table + sec->sh_name, sec->sh_addr, sec->sh_size);

And here.

> +    }
> +
> +    return 0;
> +}

Should we try to access one of the section headers to make sure that we
didn't only get the correct addresses, but that data is actually loaded
there?

Kevin

Anatol Pomozov Jan. 29, 2018, 7:16 p.m. UTC | #7

Hi

On Mon, Jan 15, 2018 at 7:41 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 13.10.2017 um 01:54 hat Anatol Pomozov geschrieben:
>> Multiboot may load section headers and all sections (even those that are
>> not part of any segment) to target memory.
>>
>> Tested with an ELF application that uses data from strings table
>> section.
>>
>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>> ---
>>  hw/core/loader.c                         |   8 +--
>>  hw/i386/multiboot.c                      |  83 +++++++++++++-----------
>>  hw/s390x/ipl.c                           |   2 +-
>>  include/hw/elf_ops.h                     | 107 ++++++++++++++++++++++++++++++-
>>  include/hw/loader.h                      |  11 +++-
>>  tests/multiboot/Makefile                 |   8 ++-
>>  tests/multiboot/generate_sections_out.py |  33 ++++++++++
>>  tests/multiboot/modules.out              |  22 +++----
>>  tests/multiboot/run_test.sh              |   6 +-
>>  tests/multiboot/sections.c               |  55 ++++++++++++++++
>>  tests/multiboot/start.S                  |   2 +-
>>  11 files changed, 276 insertions(+), 61 deletions(-)
>>  create mode 100755 tests/multiboot/generate_sections_out.py
>>  create mode 100644 tests/multiboot/sections.c
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 4593061445..a8787f7685 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -439,7 +439,7 @@ int load_elf_as(const char *filename,
>>  {
>>      return load_elf_ram(filename, translate_fn, translate_opaque,
>>                          pentry, lowaddr, highaddr, big_endian, elf_machine,
>> -                        clear_lsb, data_swab, as, true);
>> +                        clear_lsb, data_swab, as, true, NULL);
>>  }
>>
>>  /* return < 0 if error, otherwise the number of bytes loaded in memory */
>> @@ -448,7 +448,7 @@ int load_elf_ram(const char *filename,
>>                   void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
>>                   uint64_t *highaddr, int big_endian, int elf_machine,
>>                   int clear_lsb, int data_swab, AddressSpace *as,
>> -                 bool load_rom)
>> +                 bool load_rom, SectionsData *sections)
>>  {
>>      int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
>>      uint8_t e_ident[EI_NIDENT];
>> @@ -488,11 +488,11 @@ int load_elf_ram(const char *filename,
>>      if (e_ident[EI_CLASS] == ELFCLASS64) {
>>          ret = load_elf64(filename, fd, translate_fn, translate_opaque, must_swab,
>>                           pentry, lowaddr, highaddr, elf_machine, clear_lsb,
>> -                         data_swab, as, load_rom);
>> +                         data_swab, as, load_rom, sections);
>>      } else {
>>          ret = load_elf32(filename, fd, translate_fn, translate_opaque, must_swab,
>>                           pentry, lowaddr, highaddr, elf_machine, clear_lsb,
>> -                         data_swab, as, load_rom);
>> +                         data_swab, as, load_rom, sections);
>>      }
>>
>>   fail:
>> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
>> index 7dacd6d827..841d05160a 100644
>> --- a/hw/i386/multiboot.c
>> +++ b/hw/i386/multiboot.c
>> @@ -125,7 +125,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>>  {
>>      int i;
>>      bool is_multiboot = false;
>> -    uint32_t flags = 0;
>> +    uint32_t flags = 0, bootinfo_flags = 0;
>>      uint32_t mh_entry_addr;
>>      uint32_t mh_load_addr;
>>      uint32_t mb_kernel_size;
>> @@ -134,6 +134,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>>      uint8_t *mb_bootinfo_data;
>>      uint32_t cmdline_len;
>>      struct multiboot_header *multiboot_header;
>> +    SectionsData sections;
>>
>>      /* Ok, let's see if it is a multiboot image.
>>         The header is 12x32bit long, so the latest entry may be 8192 - 48. */
>> @@ -161,37 +162,8 @@ int load_multiboot(FWCfgState *fw_cfg,
>>      if (flags & MULTIBOOT_VIDEO_MODE) {
>>          fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n");
>>      }
>> -    if (!(flags & MULTIBOOT_AOUT_KLUDGE)) {
>> -        uint64_t elf_entry;
>> -        uint64_t elf_low, elf_high;
>> -        int kernel_size;
>> -        fclose(f);
>>
>> -        if (((struct elf64_hdr*)header)->e_machine == EM_X86_64) {
>> -            fprintf(stderr, "Cannot load x86-64 image, give a 32bit one.\n");
>> -            exit(1);
>> -        }
>
> I'm not sure why you're reversing the condition and moving this branch
> down, but in the code movements the EM_X86_64 check got lost. Please
> keep it, we don't support 64 bit ELFs at the moment. If you want to
> change this, it should be a separate patch.

Ok I moved it to a separate patch.

>
>> -        kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
>> -                               &elf_low, &elf_high, 0, EM_NONE,
>> -                               0, 0);
>> -        if (kernel_size < 0) {
>> -            fprintf(stderr, "Error while loading elf kernel\n");
>> -            exit(1);
>> -        }
>> -        mh_load_addr = elf_low;
>> -        mb_kernel_size = elf_high - elf_low;
>> -        mh_entry_addr = elf_entry;
>> -
>> -        mbs.mb_buf = g_malloc(mb_kernel_size);
>> -        if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
>> -            fprintf(stderr, "Error while fetching elf kernel from rom\n");
>> -            exit(1);
>> -        }
>> -
>> -        mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
>> -                  mb_kernel_size, (size_t)mh_entry_addr);
>> -    } else {
>> +    if (flags & MULTIBOOT_AOUT_KLUDGE) {
>>          /* Valid if mh_flags sets MULTIBOOT_AOUT_KLUDGE. */
>>          uint32_t mh_header_addr = ldl_p(&multiboot_header->header_addr);
>>          uint32_t mh_load_end_addr = ldl_p(&multiboot_header->load_end_addr);
>> @@ -228,12 +200,6 @@ int load_multiboot(FWCfgState *fw_cfg,
>>              mb_load_size = mb_kernel_size;
>>          }
>>
>> -        /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
>> -        uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
>> -        uint32_t mh_width = ldl_p(&multiboot_header->width);
>> -        uint32_t mh_height = ldl_p(&multiboot_header->height);
>> -        uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
>> -
>>          mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
>>          mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
>>          mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
>> @@ -248,9 +214,45 @@ int load_multiboot(FWCfgState *fw_cfg,
>>              exit(1);
>>          }
>>          memset(mbs.mb_buf + mb_load_size, 0, mb_kernel_size - mb_load_size);
>> -        fclose(f);
>> +    } else {
>> +        uint64_t elf_entry;
>> +        uint64_t elf_low, elf_high;
>> +        int kernel_size;
>> +
>> +        kernel_size = load_elf_ram(kernel_filename, NULL, NULL, &elf_entry,
>> +                               &elf_low, &elf_high, 0, I386_ELF_MACHINE,
>> +                               0, 0, NULL, true, &sections);
>> +        if (kernel_size < 0) {
>> +            fprintf(stderr, "Error while loading elf kernel\n");
>> +            exit(1);
>> +        }
>> +        mh_load_addr = elf_low;
>> +        mb_kernel_size = elf_high - elf_low;
>> +        mh_entry_addr = elf_entry;
>> +
>> +        mbs.mb_buf = g_malloc(mb_kernel_size);
>> +        if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
>> +            fprintf(stderr, "Error while fetching elf kernel from rom\n");
>> +            exit(1);
>> +        }
>> +
>> +        mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
>> +                  mb_kernel_size, (size_t)mh_entry_addr);
>> +
>> +        stl_p(&bootinfo.u.elf_sec.num, sections.num);
>> +        stl_p(&bootinfo.u.elf_sec.size, sections.size);
>> +        stl_p(&bootinfo.u.elf_sec.addr, sections.addr);
>> +        stl_p(&bootinfo.u.elf_sec.shndx, sections.shndx);
>> +
>> +        bootinfo_flags |= MULTIBOOT_INFO_ELF_SHDR;
>>      }
>>
>> +    /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
>> +    uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
>> +    uint32_t mh_width = ldl_p(&multiboot_header->width);
>> +    uint32_t mh_height = ldl_p(&multiboot_header->height);
>> +    uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
>> +
>>      mbs.mb_buf_phys = mh_load_addr;
>>
>>      mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_kernel_size);
>> @@ -332,7 +334,8 @@ int load_multiboot(FWCfgState *fw_cfg,
>>      stl_p(&bootinfo.mods_count, mbs.mb_mods_count); /* mods_count */
>>
>>      /* the kernel is where we want it to be now */
>> -    stl_p(&bootinfo.flags, MULTIBOOT_INFO_MEMORY
>> +    stl_p(&bootinfo.flags, bootinfo_flags
>> +                           | MULTIBOOT_INFO_MEMORY
>>                             | MULTIBOOT_INFO_BOOTDEV
>>                             | MULTIBOOT_INFO_CMDLINE
>>                             | MULTIBOOT_INFO_MODS
>> @@ -365,5 +368,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>>      option_rom[nb_option_roms].bootindex = 0;
>>      nb_option_roms++;
>>
>> +    fclose(f);
>> +
>>      return 1; /* yes, we are multiboot */
>>  }
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 0d06fc12b6..4d9cc6261a 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -327,7 +327,7 @@ static int load_netboot_image(Error **errp)
>>      }
>>
>>      img_size = load_elf_ram(netboot_filename, NULL, NULL, &ipl->start_addr,
>> -                            NULL, NULL, 1, EM_S390, 0, 0, NULL, false);
>> +                            NULL, NULL, 1, EM_S390, 0, 0, NULL, false, NULL);
>>
>>      if (img_size < 0) {
>>          img_size = load_image_size(netboot_filename, ram_ptr, ram_size);
>> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
>> index d192e7e2a3..7a7f7983a4 100644
>> --- a/include/hw/elf_ops.h
>> +++ b/include/hw/elf_ops.h
>
> The code in this file is rather old and not quite compliant with the
> QEMU coding style.

The code I added follows existing style in that file.

> This is not an excuse not to apply the correct coding
> style to new additions to the file:

While fixing coding style in this file is a great idea I am not sure
if it makes sense to mix functional changes with formatting changes.
It sounds like formatting should be sent separately.

Even better if there is a way to reformat code automatically. Have you
considered tools like 'clang-format'? I use it at my daytime project
and it is a huge time saver.

>
>> @@ -264,12 +264,13 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>>                                int must_swab, uint64_t *pentry,
>>                                uint64_t *lowaddr, uint64_t *highaddr,
>>                                int elf_machine, int clear_lsb, int data_swab,
>> -                              AddressSpace *as, bool load_rom)
>> +                              AddressSpace *as, bool load_rom,
>> +                              SectionsData *sections)
>>  {
>>      struct elfhdr ehdr;
>>      struct elf_phdr *phdr = NULL, *ph;
>>      int size, i, total_size;
>> -    elf_word mem_size, file_size;
>> +    elf_word mem_size, file_size, sec_size;
>>      uint64_t addr, low = (uint64_t)-1, high = 0;
>>      uint8_t *data = NULL;
>>      char label[128];
>> @@ -481,6 +482,108 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>>          }
>>      }
>>      g_free(phdr);
>> +
>> +    if (sections) {
>> +        struct elf_shdr *shdr = NULL, *sh;
>> +        int shsize;
>> +
>> +        // user requested loading all ELF sections
>
> Please use C-style comments: /* ... */
done

>
>> +        shsize = ehdr.e_shnum * sizeof(shdr[0]);
>> +        if (lseek(fd, ehdr.e_shoff, SEEK_SET) != ehdr.e_shoff) {
>> +            goto fail;
>> +        }
>> +        shdr = g_malloc0(shsize);
>> +        if (!shdr)
>> +            goto fail;
>
> g_malloc0() never returns NULL. If you want it to return NULL instead of
> aborting qemu when the allocation fails, you need g_try_malloc0().
>
> Also, the QEMU coding style requires braces after an if condition. I'm
> mentioning this only here, but it applies to multiple places in the
> whole patch.
>
>> +        if (read(fd, shdr, shsize) != shsize)
>> +            goto fail;
>> +        if (must_swab) {
>> +            for (i = 0; i < ehdr.e_shnum; i++) {
>> +                sh = &shdr[i];
>> +                glue(bswap_shdr, SZ)(sh);
>> +            }
>> +        }
>> +
>> +        for(i = 0; i < ehdr.e_shnum; i++) {
>
> A space character is required between for and the bracket.

Fixed.

BTW current QEMU code contains 383 such formatting errors.

$ git grep 'for(' | wc -l
383

clang-format will help to fix it.

>
>> +            sh = &shdr[i];
>> +            if (sh->sh_type == SHT_NULL)
>> +                continue;
>> +            // if section has address then it is loaded (XXX: is it true?), no need to load it again
>
> This exceeds the limit of 80 characters per line.

fixed

>
>> +            if (sh->sh_addr)
>> +                continue;
>> +
>> +            // append section data at the end of the loaded segments
>> +            addr = ROUND_UP(high, sh->sh_addralign);
>> +            sh->sh_addr = addr;
>> +
>> +            sec_size = sh->sh_size; /* Size of the section data */
>> +            data = g_malloc0(sec_size);
>> +            if (sec_size > 0) {
>> +                if (lseek(fd, sh->sh_offset, SEEK_SET) < 0) {
>> +                    goto fail;
>> +                }
>> +                if (read(fd, data, sec_size) != sec_size) {
>> +                    goto fail;
>> +                }
>> +            }
>> +
>> +            // As these sections are not loadable no need to perform reloaction
>> +            // using translate_fn()
>> +
>> +            if (data_swab) {
>> +                int j;
>> +                for (j = 0; j < sec_size; j += (1 << data_swab)) {
>
> Is sec_size guaranteed to be aligned? I don't see a check to verify
> this. Otherwise, could we call bswap64() on the last byte of section,
> accessing seven more bytes outside the allocated buffer?

In this particular case I was following existing code in this file
above glue(load_elf, SZ). As long as original code works this code
should work as well.

I looked at code and the only place that has non-zero data_swab value
is at hw/arm/boot.c. But ARM is not supported by multiboot (at least
now). In other cases, when data_swab is zero no byteswapping is
performed.

>
>> +                    uint8_t *dp = data + j;
>> +                    switch (data_swab) {
>> +                    case (1):
>
> Why 'case (1):' instead of 'case 1:' looks odd.

I have no idea. My code follows existing codestyle in this file below.

>
>> +                        *(uint16_t *)dp = bswap16(*(uint16_t *)dp);
>> +                        break;
>> +                    case (2):
>> +                        *(uint32_t *)dp = bswap32(*(uint32_t *)dp);
>> +                        break;
>> +                    case (3):
>> +                        *(uint64_t *)dp = bswap64(*(uint64_t *)dp);
>> +                        break;
>> +                    default:
>> +                        g_assert_not_reached();
>> +                    }
>> +                }
>> +            }
>> +
>> +            if (load_rom) {
>> +                snprintf(label, sizeof(label), "shdr #%d: %s", i, name);
>> +
>> +                /* rom_add_elf_program() seize the ownership of 'data' */
>> +                rom_add_elf_program(label, data, sec_size, sec_size, addr, as);
>> +            } else {
>> +                cpu_physical_memory_write(addr, data, sec_size);
>> +                g_free(data);
>> +            }
>> +
>> +            total_size += sec_size;
>> +            high = addr + sec_size;
>> +
>> +            data = NULL;
>> +        }
>> +
>> +        sections->num = ehdr.e_shnum;
>> +        sections->size = ehdr.e_shentsize;
>> +        sections->addr = high; // Address where we load the sections header
>> +        sections->shndx = ehdr.e_shstrndx;
>> +
>> +        // Append section headers at the end of loaded segments/sections
>> +        if (load_rom) {
>> +            snprintf(label, sizeof(label), "shdrs");
>> +
>> +            /* rom_add_elf_program() seize the ownership of 'shdr' */
>> +            rom_add_elf_program(label, shdr, shsize, shsize, high, as);
>> +        } else {
>> +            cpu_physical_memory_write(high, shdr, shsize);
>> +            g_free(shdr);
>> +        }
>> +        high += shsize;
>> +    }
>> +
>>      if (lowaddr)
>>          *lowaddr = (uint64_t)(elf_sword)low;
>>      if (highaddr)
>> diff --git a/include/hw/loader.h b/include/hw/loader.h
>> index 355fe0f5a2..3cf2d6da0f 100644
>> --- a/include/hw/loader.h
>> +++ b/include/hw/loader.h
>> @@ -65,6 +65,13 @@ int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
>>  #define ELF_LOAD_WRONG_ENDIAN -4
>>  const char *load_elf_strerror(int error);
>>
>> +typedef struct {
>> +    uint32_t num; // number of loaded sections
>> +    uint32_t size; // size of entry in sections header list
>> +    uint32_t addr; // address of loaded sections header
>> +    uint32_t shndx; // number of section that contains string table
>> +} SectionsData;
>> +
>>  /** load_elf_ram:
>>   * @filename: Path of ELF file
>>   * @translate_fn: optional function to translate load addresses
>> @@ -82,6 +89,8 @@ const char *load_elf_strerror(int error);
>>   * @as: The AddressSpace to load the ELF to. The value of address_space_memory
>>   *      is used if nothing is supplied here.
>>   * @load_rom : Load ELF binary as ROM
>> + * @sections: If parameter is non-NULL then all ELF sections loaded into memory
>> + *      and this structure is populated.
>>   *
>>   * Load an ELF file's contents to the emulated system's address space.
>>   * Clients may optionally specify a callback to perform address
>> @@ -99,7 +108,7 @@ int load_elf_ram(const char *filename,
>>                   void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
>>                   uint64_t *highaddr, int big_endian, int elf_machine,
>>                   int clear_lsb, int data_swab, AddressSpace *as,
>> -                 bool load_rom);
>> +                 bool load_rom, SectionsData *sections);
>>
>>  /** load_elf_as:
>>   * Same as load_elf_ram(), but always loads the elf as ROM
>> diff --git a/tests/multiboot/Makefile b/tests/multiboot/Makefile
>> index 36f01dc647..b6a5056347 100644
>> --- a/tests/multiboot/Makefile
>> +++ b/tests/multiboot/Makefile
>> @@ -6,7 +6,7 @@ LD=ld
>>  LDFLAGS=-melf_i386 -T link.ld
>>  LIBS=$(shell $(CC) $(CCFLAGS) -print-libgcc-file-name)
>>
>> -all: mmap.elf modules.elf
>> +all: mmap.elf modules.elf sections.elf sections.out
>>
>>  mmap.elf: start.o mmap.o libc.o
>>       $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
>> @@ -14,6 +14,12 @@ mmap.elf: start.o mmap.o libc.o
>>  modules.elf: start.o modules.o libc.o
>>       $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
>>
>> +sections.elf: start.o sections.o libc.o
>> +     $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
>> +
>> +sections.out: sections.elf generate_sections_out.py
>> +     python2 generate_sections_out.py $^ > $@
>> +
>>  %.o: %.c
>>       $(CC) $(CCFLAGS) -c -o $@ $^
>>
>> diff --git a/tests/multiboot/generate_sections_out.py b/tests/multiboot/generate_sections_out.py
>> new file mode 100755
>> index 0000000000..8077856823
>> --- /dev/null
>> +++ b/tests/multiboot/generate_sections_out.py
>> @@ -0,0 +1,33 @@
>> +#!/usr/bin/env python2
>> +
>> +import sys
>> +
>> +from elftools.elf.elffile import ELFFile
>
> I don't think we can expect this module to be present. For example, on
> my system, attempting to run the test fails:
>
> ImportError: No module named elftools.elf.elffile
>
> Maybe we can parse the output of readelf instead?

Does readelf avilable at all supported platforms (Windows, MacOSX)?
How can we sure that readelf output stays consistent across versions/platforms?

Using this library is a great way to avoid the problems.

>> +def roundup(num, align):
>> +  return (num + align - 1) / align * align
>> +
>> +def main(filename):
>> +  print "\n\n\n=== Running test case: sections.elf  ===\n"
>> +  print "Multiboot header at 9500, ELF section headers present\n"
>> +
>> +  with open(filename, 'rb') as f:
>> +    elf = ELFFile(f)
>> +    header = elf.header
>> +    load_addr = 0x100000  # we load image starting from 1M
>> +    sections = ""
>> +    for s in elf.iter_sections():
>> +      align = s.header.sh_addralign
>> +      addr = 0
>> +      if s.header.sh_type != 'SHT_NULL':
>> +        addr = s.header.sh_addr
>> +        if addr == 0:
>> +          addr = roundup(load_addr, s.header.sh_addralign)
>> +        load_addr = addr + s.header.sh_size
>> +      sections += "Elf section name=%s addr=%x size=%d\n" % (s.name, addr, s.header.sh_size)
>> +
>> +    print "Sections list with %d entries of size %d at %x, string index %d" % (header.e_shnum, header.e_shentsize, load_addr, header.e_shstrndx)
>
> We're not as strict about the 80 characters rule in Python code, but I
> think this line could use being wrapped.
>
>> +    print sections,
>> +
>> +if __name__ == '__main__':
>> +  main(sys.argv[1])
>> diff --git a/tests/multiboot/modules.out b/tests/multiboot/modules.out
>> index 0da7b39374..b6c1a01be1 100644
>> --- a/tests/multiboot/modules.out
>> +++ b/tests/multiboot/modules.out
>> @@ -5,15 +5,15 @@
>>
>>  Multiboot header at 9500
>>
>> -Module list with 0 entries at 102000
>> +Module list with 0 entries at 104000
>>
>>
>>  === Running test case: modules.elf -initrd module.txt ===
>>
>>  Multiboot header at 9500
>>
>> -Module list with 1 entries at 102000
>> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
>> +Module list with 1 entries at 104000
>> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt'
>>           Content: 'This is a test file that is used as a multiboot module.'
>>
>>
>> @@ -21,8 +21,8 @@ Module list with 1 entries at 102000
>>
>>  Multiboot header at 9500
>>
>> -Module list with 1 entries at 102000
>> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument'
>> +Module list with 1 entries at 104000
>> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt argument'
>>           Content: 'This is a test file that is used as a multiboot module.'
>>
>>
>> @@ -30,8 +30,8 @@ Module list with 1 entries at 102000
>>
>>  Multiboot header at 9500
>>
>> -Module list with 1 entries at 102000
>> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument,with,commas'
>> +Module list with 1 entries at 104000
>> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt argument,with,commas'
>>           Content: 'This is a test file that is used as a multiboot module.'
>>
>>
>> @@ -39,10 +39,10 @@ Module list with 1 entries at 102000
>>
>>  Multiboot header at 9500
>>
>> -Module list with 3 entries at 102000
>> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
>> +Module list with 3 entries at 104000
>> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt'
>>           Content: 'This is a test file that is used as a multiboot module.'
>> -[102010] Module: 104000 - 104038 (56 bytes) 'module.txt argument'
>> +[104010] Module: 106000 - 106038 (56 bytes) 'module.txt argument'
>>           Content: 'This is a test file that is used as a multiboot module.'
>> -[102020] Module: 105000 - 105038 (56 bytes) 'module.txt'
>> +[104020] Module: 107000 - 107038 (56 bytes) 'module.txt'
>>           Content: 'This is a test file that is used as a multiboot module.'
>> diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
>> index 0278148b43..f04e35cbf0 100755
>> --- a/tests/multiboot/run_test.sh
>> +++ b/tests/multiboot/run_test.sh
>> @@ -56,9 +56,13 @@ modules() {
>>      run_qemu modules.elf -initrd "module.txt,module.txt argument,module.txt"
>>  }
>>
>> +sections() {
>> +    run_qemu sections.elf
>> +}
>> +
>>  make all
>>
>> -for t in mmap modules; do
>> +for t in mmap modules sections; do
>>
>>      echo > test.log
>>      $t
>> diff --git a/tests/multiboot/sections.c b/tests/multiboot/sections.c
>> new file mode 100644
>> index 0000000000..05a88f99ac
>> --- /dev/null
>> +++ b/tests/multiboot/sections.c
>> @@ -0,0 +1,55 @@
>> +/*
>> + * Copyright (c) 2017 Anatol Pomozov <anatol.pomozov@gmail.com>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "libc.h"
>> +#include "../../hw/i386/multiboot_header.h"
>> +#include "../../include/elf.h"
>> +
>> +int test_main(uint32_t magic, struct multiboot_info *mbi)
>> +{
>> +    void *p;
>> +    unsigned int i;
>> +
>> +    (void) magic;
>> +    multiboot_elf_section_header_table_t shdr;
>> +
>> +    printf("Multiboot header at %x, ELF section headers %s\n\n", mbi,
>> +        mbi->flags & MULTIBOOT_INFO_ELF_SHDR ? "present" : "don't present");
>> +
>> +    shdr = mbi->u.elf_sec;
>> +    printf("Sections list with %d entries of size %d at %x, string index %d\n",
>> +        shdr.num, shdr.size, shdr.addr, shdr.shndx);
>> +
>> +    const char *string_table = (char *)((Elf32_Shdr *)(uintptr_t)(shdr.addr + shdr.shndx * shdr.size))->sh_addr;
>
> 80 characters again.

fixed.

>
>> +
>> +    for (i = 0, p = (void *)shdr.addr;
>> +         i < shdr.num;
>> +         i++, p += shdr.size)
>> +    {
>> +        Elf32_Shdr *sec;
>> +
>> +        sec = (Elf32_Shdr *)p;
>> +        printf("Elf section name=%s addr=%lx size=%ld\n", string_table + sec->sh_name, sec->sh_addr, sec->sh_size);
>
> And here.

fixed.
>
>> +    }
>> +
>> +    return 0;
>> +}
>
> Should we try to access one of the section headers to make sure that we
> didn't only get the correct addresses, but that data is actually loaded
> there?
>
> Kevin

[3/4] multiboot: load elf sections and section headers

Commit Message

Comments

Patch