Re: [Qemu-devel] [PATCH 2/4] multiboot: load elf sections and section headers

[Jack Schwartz]

Hi Anatol.

I have one comment about sections.c headers (and didn't really look 
further in that file), and start.S.

Comments inline...

On 01/29/18 12:43, Anatol Pomozov 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 <address@hidden>
> ---
>   hw/core/loader.c                         |   8 +--
>   hw/i386/multiboot.c                      |  17 +++--
>   hw/s390x/ipl.c                           |   2 +-
>   include/hw/elf_ops.h                     | 110 +++++++++++++++++++++++++++++--
>   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               |  57 ++++++++++++++++
>   tests/multiboot/start.S                  |   2 +-
>   11 files changed, 248 insertions(+), 28 deletions(-)
>   create mode 100755 tests/multiboot/generate_sections_out.py
>   create mode 100644 tests/multiboot/sections.c
<snip>
> diff --git a/tests/multiboot/sections.c b/tests/multiboot/sections.c
> new file mode 100644
> index 0000000000..64060510ce
> --- /dev/null
> +++ b/tests/multiboot/sections.c
> @@ -0,0 +1,57 @@
> +/*
> + * 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"
As like the first patch, I suggest creating a "multiboot" subdir in the 
"include" subtree and putting all multiboot header files there. 
Including "multiboot/multiboot_header.h" would be cleaner.
> +#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" : "doesn'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)
C headers can be included in assembly files.  Since you are bringing 
over multiboot_header.h, why not include it and use its values instead 
of re-defining them here?

    Thanks,
    Jack
>   
>   .align  4