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

[Anatol Pomozov]

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.

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