Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct

[Jack Schwartz <jack.schwartz@oracle.com>]

Hi Anatol and Kevin.

Even though I am new to Qemu, I have a patch to deliver for multiboot.c 
(as you know) and so I feel familiar enough to do a review of this 
patch.  One comment is probably more for maintainers.

Comments inline...


On 01/29/18 12:43, Anatol Pomozov wrote:
> Using C structs makes the code more readable and prevents type conversion
> errors.
>
> Borrow multiboot1 header from GRUB project.
>
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
>   hw/i386/multiboot.c         | 124 +++++++++------------
>   hw/i386/multiboot_header.h  | 254 ++++++++++++++++++++++++++++++++++++++++++++
>   tests/multiboot/mmap.c      |  14 +--
>   tests/multiboot/mmap.out    |  10 ++
>   tests/multiboot/modules.c   |  12 ++-
>   tests/multiboot/modules.out |  10 ++
>   tests/multiboot/multiboot.h |  66 ------------
>   7 files changed, 339 insertions(+), 151 deletions(-)
>   create mode 100644 hw/i386/multiboot_header.h
>   delete mode 100644 tests/multiboot/multiboot.h
>
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index c7b70c91d5..c6d05ca46b 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -28,6 +28,7 @@
>   #include "hw/hw.h"
>   #include "hw/nvram/fw_cfg.h"
>   #include "multiboot.h"
> +#include "multiboot_header.h"
>   #include "hw/loader.h"
>   #include "elf.h"
>   #include "sysemu/sysemu.h"
> @@ -47,39 +48,9 @@
>   #error multiboot struct needs to fit in 16 bit real mode
>   #endif
>   
> -enum {
> -    /* Multiboot info */
> -    MBI_FLAGS       = 0,
> -    MBI_MEM_LOWER   = 4,
> -    MBI_MEM_UPPER   = 8,
> -    MBI_BOOT_DEVICE = 12,
> -    MBI_CMDLINE     = 16,
> -    MBI_MODS_COUNT  = 20,
> -    MBI_MODS_ADDR   = 24,
> -    MBI_MMAP_ADDR   = 48,
> -    MBI_BOOTLOADER  = 64,
> -
> -    MBI_SIZE        = 88,
> -
> -    /* Multiboot modules */
> -    MB_MOD_START    = 0,
> -    MB_MOD_END      = 4,
> -    MB_MOD_CMDLINE  = 8,
> -
> -    MB_MOD_SIZE     = 16,
> -
> -    /* Region offsets */
> -    ADDR_E820_MAP = MULTIBOOT_STRUCT_ADDR + 0,
> -    ADDR_MBI      = ADDR_E820_MAP + 0x500,
> -
> -    /* Multiboot flags */
> -    MULTIBOOT_FLAGS_MEMORY      = 1 << 0,
> -    MULTIBOOT_FLAGS_BOOT_DEVICE = 1 << 1,
> -    MULTIBOOT_FLAGS_CMDLINE     = 1 << 2,
> -    MULTIBOOT_FLAGS_MODULES     = 1 << 3,
> -    MULTIBOOT_FLAGS_MMAP        = 1 << 6,
> -    MULTIBOOT_FLAGS_BOOTLOADER  = 1 << 9,
> -};
> +/* Region offsets */
> +#define ADDR_E820_MAP MULTIBOOT_STRUCT_ADDR
> +#define ADDR_MBI      (ADDR_E820_MAP + 0x500)
>   
>   typedef struct {
>       /* buffer holding kernel, cmdlines and mb_infos */
> @@ -128,14 +99,15 @@ static void mb_add_mod(MultibootState *s,
>                          hwaddr start, hwaddr end,
>                          hwaddr cmdline_phys)
>   {
> -    char *p;
> +    multiboot_module_t *mod;
>       assert(s->mb_mods_count < s->mb_mods_avail);
>   
> -    p = (char *)s->mb_buf + s->offset_mbinfo + MB_MOD_SIZE * s->mb_mods_count;
> +    mod = s->mb_buf + s->offset_mbinfo +
> +          sizeof(multiboot_module_t) * s->mb_mods_count;
>   
> -    stl_p(p + MB_MOD_START,   start);
> -    stl_p(p + MB_MOD_END,     end);
> -    stl_p(p + MB_MOD_CMDLINE, cmdline_phys);
> +    stl_p(&mod->mod_start, start);
> +    stl_p(&mod->mod_end,   end);
> +    stl_p(&mod->cmdline,   cmdline_phys);
>   
>       mb_debug("mod%02d: "TARGET_FMT_plx" - "TARGET_FMT_plx"\n",
>                s->mb_mods_count, start, end);
> @@ -151,26 +123,29 @@ int load_multiboot(FWCfgState *fw_cfg,
>                      int kernel_file_size,
>                      uint8_t *header)
>   {
> -    int i, is_multiboot = 0;
> +    int i;
> +    bool is_multiboot = false;
>       uint32_t flags = 0;
>       uint32_t mh_entry_addr;
>       uint32_t mh_load_addr;
>       uint32_t mb_kernel_size;
>       MultibootState mbs;
> -    uint8_t bootinfo[MBI_SIZE];
> +    multiboot_info_t bootinfo;
>       uint8_t *mb_bootinfo_data;
>       uint32_t cmdline_len;
> +    struct multiboot_header *multiboot_header;
>   
>       /* Ok, let's see if it is a multiboot image.
>          The header is 12x32bit long, so the latest entry may be 8192 - 48. */
>       for (i = 0; i < (8192 - 48); i += 4) {
> -        if (ldl_p(header+i) == 0x1BADB002) {
> -            uint32_t checksum = ldl_p(header+i+8);
> -            flags = ldl_p(header+i+4);
> +        multiboot_header = (struct multiboot_header *)(header + i);
> +        if (ldl_p(&multiboot_header->magic) == MULTIBOOT_HEADER_MAGIC) {
> +            uint32_t checksum = ldl_p(&multiboot_header->checksum);
> +            flags = ldl_p(&multiboot_header->flags);
>               checksum += flags;
> -            checksum += (uint32_t)0x1BADB002;
> +            checksum += (uint32_t)MULTIBOOT_HEADER_MAGIC;
>               if (!checksum) {
> -                is_multiboot = 1;
> +                is_multiboot = true;
>                   break;
>               }
>           }
> @@ -180,13 +155,13 @@ int load_multiboot(FWCfgState *fw_cfg,
>           return 0; /* no multiboot */
>   
>       mb_debug("qemu: I believe we found a multiboot image!\n");
> -    memset(bootinfo, 0, sizeof(bootinfo));
> +    memset(&bootinfo, 0, sizeof(bootinfo));
>       memset(&mbs, 0, sizeof(mbs));
>   
> -    if (flags & 0x00000004) { /* MULTIBOOT_HEADER_HAS_VBE */
> +    if (flags & MULTIBOOT_VIDEO_MODE) {
>           fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n");
>       }
> -    if (!(flags & 0x00010000)) { /* MULTIBOOT_HEADER_HAS_ADDR */
> +    if (!(flags & MULTIBOOT_AOUT_KLUDGE)) {
>           uint64_t elf_entry;
>           uint64_t elf_low, elf_high;
>           int kernel_size;
> @@ -217,12 +192,12 @@ int load_multiboot(FWCfgState *fw_cfg,
>           mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
>                     mb_kernel_size, (size_t)mh_entry_addr);
>       } else {
> -        /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_ADDR. */
> -        uint32_t mh_header_addr = ldl_p(header+i+12);
> -        uint32_t mh_load_end_addr = ldl_p(header+i+20);
> -        uint32_t mh_bss_end_addr = ldl_p(header+i+24);
> +        /* 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);
> +        uint32_t mh_bss_end_addr = ldl_p(&multiboot_header->bss_end_addr);
> +        mh_load_addr = ldl_p(&multiboot_header->load_addr);
>   
> -        mh_load_addr = ldl_p(header+i+16);
>           if (mh_header_addr < mh_load_addr) {
>               fprintf(stderr, "invalid mh_load_addr address\n");
>               exit(1);
> @@ -230,7 +205,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>   
>           uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
>           uint32_t mb_load_size = 0;
> -        mh_entry_addr = ldl_p(header+i+28);
> +        mh_entry_addr = ldl_p(&multiboot_header->entry_addr);
>   
>           if (mh_load_end_addr) {
>               if (mh_bss_end_addr < mh_load_addr) {
> @@ -253,11 +228,11 @@ int load_multiboot(FWCfgState *fw_cfg,
>               mb_load_size = mb_kernel_size;
>           }
>   
> -        /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
> -        uint32_t mh_mode_type = ldl_p(header+i+32);
> -        uint32_t mh_width = ldl_p(header+i+36);
> -        uint32_t mh_height = ldl_p(header+i+40);
> -        uint32_t mh_depth = ldl_p(header+i+44); */
> +        /* 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); */
This question is probably more for maintainers...

In the patch series I submitted, people were OK that I was going to 
delete these lines since they were only comments anyway.  Your approach 
leaves these lines in and updates them even though they are comments.  
Which way is preferred?
>           mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
>           mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
> @@ -295,14 +270,15 @@ int load_multiboot(FWCfgState *fw_cfg,
>       }
>   
>       mbs.mb_buf_size += cmdline_len;
> -    mbs.mb_buf_size += MB_MOD_SIZE * mbs.mb_mods_avail;
> +    mbs.mb_buf_size += sizeof(multiboot_module_t) * mbs.mb_mods_avail;
>       mbs.mb_buf_size += strlen(bootloader_name) + 1;
>   
>       mbs.mb_buf_size = TARGET_PAGE_ALIGN(mbs.mb_buf_size);
>   
>       /* enlarge mb_buf to hold cmdlines, bootloader, mb-info structs */
>       mbs.mb_buf            = g_realloc(mbs.mb_buf, mbs.mb_buf_size);
> -    mbs.offset_cmdlines   = mbs.offset_mbinfo + mbs.mb_mods_avail * MB_MOD_SIZE;
> +    mbs.offset_cmdlines   = mbs.offset_mbinfo +
> +        mbs.mb_mods_avail * sizeof(multiboot_module_t);
>       mbs.offset_bootloader = mbs.offset_cmdlines + cmdline_len;
>   
>       if (initrd_filename) {
> @@ -348,22 +324,22 @@ int load_multiboot(FWCfgState *fw_cfg,
>       char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
>       snprintf(kcmdline, sizeof(kcmdline), "%s %s",
>                kernel_filename, kernel_cmdline);
> -    stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline));
> +    stl_p(&bootinfo.cmdline, mb_add_cmdline(&mbs, kcmdline));
>   
> -    stl_p(bootinfo + MBI_BOOTLOADER, mb_add_bootloader(&mbs, bootloader_name));
> +    stl_p(&bootinfo.boot_loader_name, mb_add_bootloader(&mbs, bootloader_name));
>   
> -    stl_p(bootinfo + MBI_MODS_ADDR,  mbs.mb_buf_phys + mbs.offset_mbinfo);
> -    stl_p(bootinfo + MBI_MODS_COUNT, mbs.mb_mods_count); /* mods_count */
> +    stl_p(&bootinfo.mods_addr,  mbs.mb_buf_phys + mbs.offset_mbinfo);
> +    stl_p(&bootinfo.mods_count, mbs.mb_mods_count); /* mods_count */
>   
>       /* the kernel is where we want it to be now */
> -    stl_p(bootinfo + MBI_FLAGS, MULTIBOOT_FLAGS_MEMORY
> -                                | MULTIBOOT_FLAGS_BOOT_DEVICE
> -                                | MULTIBOOT_FLAGS_CMDLINE
> -                                | MULTIBOOT_FLAGS_MODULES
> -                                | MULTIBOOT_FLAGS_MMAP
> -                                | MULTIBOOT_FLAGS_BOOTLOADER);
> -    stl_p(bootinfo + MBI_BOOT_DEVICE, 0x8000ffff); /* XXX: use the -boot switch? */
> -    stl_p(bootinfo + MBI_MMAP_ADDR,   ADDR_E820_MAP);
> +    stl_p(&bootinfo.flags, MULTIBOOT_INFO_MEMORY
> +                           | MULTIBOOT_INFO_BOOTDEV
> +                           | MULTIBOOT_INFO_CMDLINE
> +                           | MULTIBOOT_INFO_MODS
> +                           | MULTIBOOT_INFO_MEM_MAP
> +                           | MULTIBOOT_INFO_BOOT_LOADER_NAME);
> +    stl_p(&bootinfo.boot_device, 0x8000ffff); /* XXX: use the -boot switch? */
> +    stl_p(&bootinfo.mmap_addr, ADDR_E820_MAP);
>   
>       mb_debug("multiboot: mh_entry_addr = %#x\n", mh_entry_addr);
>       mb_debug("           mb_buf_phys   = "TARGET_FMT_plx"\n", mbs.mb_buf_phys);
> @@ -371,7 +347,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>       mb_debug("           mb_mods_count = %d\n", mbs.mb_mods_count);
>   
>       /* save bootinfo off the stack */
> -    mb_bootinfo_data = g_memdup(bootinfo, sizeof(bootinfo));
> +    mb_bootinfo_data = g_memdup(&bootinfo, sizeof(bootinfo));
>   
>       /* Pass variables to option rom */
>       fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ENTRY, mh_entry_addr);
<snip>
> diff --git a/tests/multiboot/mmap.c b/tests/multiboot/mmap.c
> index 766b003f38..9cba8b07e0 100644
> --- a/tests/multiboot/mmap.c
> +++ b/tests/multiboot/mmap.c
> @@ -21,15 +21,17 @@
>    */
>   
>   #include "libc.h"
> -#include "multiboot.h"
> +#include "../../hw/i386/multiboot_header.h"
Suggestion: create a multiboot subdir under include, and put 
multiboot_header.h and other multiboot includes there.  This way you can 
just #include "multiboot/multiboot_header.h" which seems cleaner to me 
than "../../hw/i386/multiboot_header.h"
> -int test_main(uint32_t magic, struct mb_info *mbi)
> +int test_main(uint32_t magic, struct multiboot_info *mbi)
>   {
>       uintptr_t entry_addr;
> -    struct mb_mmap_entry *entry;
> +    struct multiboot_mmap_entry *entry;
>   
>       (void) magic;
>   
> +    printf("Multiboot header at %x\n\n", mbi);
> +
>       printf("Lower memory: %dk\n", mbi->mem_lower);
>       printf("Upper memory: %dk\n", mbi->mem_upper);
>   
> @@ -39,11 +41,11 @@ int test_main(uint32_t magic, struct mb_info *mbi)
>            entry_addr < mbi->mmap_addr + mbi->mmap_length;
>            entry_addr += entry->size + 4)
>       {
> -        entry = (struct mb_mmap_entry*) entry_addr;
> +        entry = (struct multiboot_mmap_entry*) entry_addr;
>   
>           printf("%#llx - %#llx: type %d [entry size: %d]\n",
> -               entry->base_addr,
> -               entry->base_addr + entry->length,
> +               entry->addr,
> +               entry->addr + entry->len,
>                  entry->type,
>                  entry->size);
>       }
<snip>
>   
> diff --git a/tests/multiboot/modules.c b/tests/multiboot/modules.c
> index 531601fb30..a1da0ded44 100644
> --- a/tests/multiboot/modules.c
> +++ b/tests/multiboot/modules.c
> @@ -21,19 +21,21 @@
>    */
>   
>   #include "libc.h"
> -#include "multiboot.h"
> +#include "../../hw/i386/multiboot_header.h"
Same comment about putting multiboot_header.h into a multiboot subdir in 
"include" subtree.

I have this same comment, as well as one other, for patch 2 sent separately.

     Thanks,
     Jack
> -int test_main(uint32_t magic, struct mb_info *mbi)
> +int test_main(uint32_t magic, struct multiboot_info *mbi)
>   {
> -    struct mb_module *mod;
> +    struct multiboot_mod_list *mod;
>       unsigned int i;
>   
>       (void) magic;
>   
> +    printf("Multiboot header at %x\n\n", mbi);
> +
>       printf("Module list with %d entries at %x\n",
>              mbi->mods_count, mbi->mods_addr);
>   
> -    for (i = 0, mod = (struct mb_module*) mbi->mods_addr;
> +    for (i = 0, mod = (struct multiboot_mod_list*) mbi->mods_addr;
>            i < mbi->mods_count;
>            i++, mod++)
>       {
> @@ -41,7 +43,7 @@ int test_main(uint32_t magic, struct mb_info *mbi)
>           unsigned int size = mod->mod_end - mod->mod_start;
>   
>           printf("[%p] Module: %x - %x (%d bytes) '%s'\n",
> -               mod, mod->mod_start, mod->mod_end, size, mod->string);
> +               mod, mod->mod_start, mod->mod_end, size, mod->cmdline);
>   
>           /* Print test file, but remove the newline at the end */
>           if (size < sizeof(buf)) {
>
<snip>