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

[Jack Schwartz]

Hi Anatol and Kevin.

Kevin and Anatol, thanks for your replies.

A few comments inline close to the bottom...

On 2018-02-05 13:43, Anatol Pomozov wrote:
> Hi
>
> On Wed, Jan 31, 2018 at 1:12 AM, Kevin Wolf <address@hidden> wrote:
> > 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.
> This change suppose to be a refactoring and tries to avoid functional
> changes. I can remove it in a separate change.
>
> > 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?
> I do not think there is a justification for it. +1 for keeping it in "/include/"
> > 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.
OK.  This makes the include list cleaner without having to add another 
subdirectory.

... or put both multiboot header files under include/hw/i386 to keep 
them together in an existing, appropriate and more specific directory.
> > There is already ./hw/i386/multiboot.h file so it is going to be
> > confusing to have multiple files with the same name.
> >
> > I propose to rename ./hw/i386/multiboot.h to something like
> > ./hw/i386/multiboot_loader.h or ./hw/i386/load_multiboot.h
Filename change sounds good.

Also, to avoid confusion with tests/multiboot/multiboot.h, I suggest 
renaming that header file to tests/multiboot/multiboot_test.h

    Thanks,
    Jack