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

[Kevin Wolf]

Am 31.01.2018 um 00:15 hat Jack Schwartz geschrieben:
> 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.

The Multiboot code is essentially unmaintained. It's technically part of
the PC/x86 subsystem, but their maintainers don't seem to care at all.
So in order to make any progress here, I decided that I will send a
pull request for Multiboot once we have the patches ready (as a one-time
thing, without officially making myself the maintainer).

So I am the closest thing to a maintainer that we have here, and while
I'm familiar with some of the Multiboot-specific code, I don't really
know the ELF code and don't have a lot of time to spend here.

Therefore it's very welcome if you review the patches of each other,
even if you're not perfectly familiar with the code, as there is
probably noone else who could do a better review.

> On 01/29/18 12:43, Anatol Pomozov wrote:
> > @@ -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?

As far as I am concerned, I honestly don't mind either way. It's
trivial code, so we won't lose anything by removing it.

The ideal solution would be just implementing support for it, of course.
If we wanted to do that, I think we would have to pass the values
through fw_cfg and then set the VBE mode in the Mutiboot option rom.

> >           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"

Good point, but do we even have multiple header files for Multiboot to
justify a whole subdirectory?

There is include/hw/loader.h and include/elf.h, so I would suggest that
we add the new header as either include/hw/multiboot.h or directly
include/multiboot.h.

Kevin