Hi On Wed, Jan 31, 2018 at 1:12 AM, Kevin Wolf <kw...@redhat.com> 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