From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46317) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ej9yE-0006AY-MQ for qemu-devel@nongnu.org; Tue, 06 Feb 2018 15:36:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ej9yB-0005jt-B4 for qemu-devel@nongnu.org; Tue, 06 Feb 2018 15:36:02 -0500 Received: from userp2130.oracle.com ([156.151.31.86]:45220) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ej9yB-0005ip-1G for qemu-devel@nongnu.org; Tue, 06 Feb 2018 15:35:59 -0500 References: <20180129204344.196196-1-anatol.pomozov@gmail.com> <0052103f-9d90-fe35-eac1-b10966c829f6@oracle.com> <20180131091217.GA3598@localhost.localdomain> From: Jack Schwartz Message-ID: Date: Tue, 6 Feb 2018 12:35:45 -0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anatol Pomozov , Kevin Wolf Cc: QEMU Developers 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 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 revie= w >>> 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-ti= me >> 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 =3D mb_kernel_size; >>>> } >>>> - /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE. >>>> - uint32_t mh_mode_type =3D ldl_p(header+i+32); >>>> - uint32_t mh_width =3D ldl_p(header+i+36); >>>> - uint32_t mh_height =3D ldl_p(header+i+40); >>>> - uint32_t mh_depth =3D ldl_p(header+i+44); */ >>>> + /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE. >>>> + uint32_t mh_mode_type =3D ldl_p(&multiboot_header->mode_typ= e); >>>> + uint32_t mh_width =3D ldl_p(&multiboot_header->width); >>>> + uint32_t mh_height =3D ldl_p(&multiboot_header->height); >>>> + uint32_t mh_depth =3D 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 d= elete >>> these lines since they were only comments anyway. Your approach leav= es >>> 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 cours= e. >> 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 =3D %#x\n", mh_header= _addr); >>>> mb_debug("multiboot: mh_load_addr =3D %#x\n", mh_load_add= r); >>>> @@ -295,14 +270,15 @@ int load_multiboot(FWCfgState *fw_cfg, >>>> } >>>> mbs.mb_buf_size +=3D cmdline_len; >>>> - mbs.mb_buf_size +=3D MB_MOD_SIZE * mbs.mb_mods_avail; >>>> + mbs.mb_buf_size +=3D sizeof(multiboot_module_t) * mbs.mb_mods_a= vail; >>>> mbs.mb_buf_size +=3D strlen(bootloader_name) + 1; >>>> mbs.mb_buf_size =3D TARGET_PAGE_ALIGN(mbs.mb_buf_size); >>>> /* enlarge mb_buf to hold cmdlines, bootloader, mb-info struc= ts */ >>>> mbs.mb_buf =3D g_realloc(mbs.mb_buf, mbs.mb_buf_si= ze); >>>> - mbs.offset_cmdlines =3D mbs.offset_mbinfo + mbs.mb_mods_avail= * MB_MOD_SIZE; >>>> + mbs.offset_cmdlines =3D mbs.offset_mbinfo + >>>> + mbs.mb_mods_avail * sizeof(multiboot_module_t); >>>> mbs.offset_bootloader =3D 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, bootlo= ader_name)); >>>> + stl_p(&bootinfo.boot_loader_name, mb_add_bootloader(&mbs, bootl= oader_name)); >>>> - stl_p(bootinfo + MBI_MODS_ADDR, mbs.mb_buf_phys + mbs.offset_m= binfo); >>>> - stl_p(bootinfo + MBI_MODS_COUNT, mbs.mb_mods_count); /* mods_co= unt */ >>>> + 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 =3D %#x\n", mh_entry_addr)= ; >>>> mb_debug(" mb_buf_phys =3D "TARGET_FMT_plx"\n", m= bs.mb_buf_phys); >>>> @@ -371,7 +347,7 @@ int load_multiboot(FWCfgState *fw_cfg, >>>> mb_debug(" mb_mods_count =3D %d\n", mbs.mb_mods_cou= nt); >>>> /* save bootinfo off the stack */ >>>> - mb_bootinfo_data =3D g_memdup(bootinfo, sizeof(bootinfo)); >>>> + mb_bootinfo_data =3D 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 m= e 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 tha= t >> we add the new header as either include/hw/multiboot.h or directly >> include/multiboot.h. OK.=C2=A0 This makes the include list cleaner without having to add anoth= er=20 subdirectory. ... or put both multiboot header files under include/hw/i386 to keep=20 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=20 renaming that header file to tests/multiboot/multiboot_test.h =C2=A0=C2=A0=C2=A0 Thanks, =C2=A0=C2=A0=C2=A0 Jack