[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array

From: Jack Schwartz
Subject: Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct
Date: Tue, 6 Feb 2018 12:35:45 -0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

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:

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
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 * 
+    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, 
-    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 */
-                                | 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", 
@@ -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);
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
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
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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]