[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section he
|
From: |
Anatol Pomozov |
|
Subject: |
Re: [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers |
|
Date: |
Mon, 29 Jan 2018 11:16:13 -0800 |
Hi
On Mon, Jan 15, 2018 at 7:41 AM, Kevin Wolf <address@hidden> 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 <address@hidden>
>> ---
>> 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, §ions);
>> + 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 <address@hidden>
>> + *
>> + * 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