From patchwork Tue Oct 20 18:22:51 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adam Lackorzynski X-Patchwork-Id: 36479 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 0A2B6B7B91 for ; Wed, 21 Oct 2009 05:23:46 +1100 (EST) Received: from localhost ([127.0.0.1]:33247 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1N0JN1-0001yN-8r for incoming@patchwork.ozlabs.org; Tue, 20 Oct 2009 14:23:43 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1N0JML-0001x7-Pm for qemu-devel@nongnu.org; Tue, 20 Oct 2009 14:23:01 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1N0JMG-0001ul-Rf for qemu-devel@nongnu.org; Tue, 20 Oct 2009 14:23:00 -0400 Received: from [199.232.76.173] (port=34064 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1N0JMG-0001uf-DL for qemu-devel@nongnu.org; Tue, 20 Oct 2009 14:22:56 -0400 Received: from os.inf.tu-dresden.de ([141.76.48.99]:39102) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1N0JMF-0006zx-PV for qemu-devel@nongnu.org; Tue, 20 Oct 2009 14:22:56 -0400 Received: from erwin.inf.tu-dresden.de ([141.76.48.80] helo=os.inf.tu-dresden.de) by os.inf.tu-dresden.de with esmtps (TLSv1:AES128-SHA:128) (Exim 4.69) id 1N0JMD-0000HB-DY; Tue, 20 Oct 2009 20:22:53 +0200 Date: Tue, 20 Oct 2009 20:22:51 +0200 From: Adam Lackorzynski To: Kevin Wolf Subject: Re: [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules Message-ID: <20091020182251.GD8648@os.inf.tu-dresden.de> References: <1255268921-5403-1-git-send-email-adam@os.inf.tu-dresden.de> <1255268921-5403-2-git-send-email-adam@os.inf.tu-dresden.de> <4AD3085A.1050906@redhat.com> <20091014161114.GE25818@os.inf.tu-dresden.de> <4ADC2394.5050403@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <4ADC2394.5050403@redhat.com> User-Agent: Mutt/1.5.20 (2009-06-14) X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6 (newer, 3) Cc: qemu-devel@nongnu.org X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Hi, On Mon Oct 19, 2009 at 10:30:12 +0200, Kevin Wolf wrote: > Am 14.10.2009 18:11, schrieb Adam Lackorzynski: > > Subject: [PATCH 3/3] multiboot: Support arbitrary number of modules > > > > Signed-off-by: Adam Lackorzynski > > Looks good in general. I'm adding some minor comments inline. > > > + > > +#define MULTIBOOT_FLAGS_MEMORY (1 << 0) > > +#define MULTIBOOT_FLAGS_BOOT_DEVICE (1 << 1) > > +#define MULTIBOOT_FLAGS_CMDLINE (1 << 2) > > +#define MULTIBOOT_FLAGS_MODULES (1 << 3) > > +#define MULTIBOOT_FLAGS_MMAP (1 << 6) > > You could align the (1 << n) to the same column. Moved to the enum altogether. > > +static void *mb_buf; > > +static uint32_t mb_buf_phys; /* address in target */ > > +static int mb_buf_size; /* size of mb_buf in bytes */ > > +static int mb_mods_avail; /* available slots for mb_mods */ > > +static int mb_mods_count; > > +static int mb_cmdlines_pos; > > Maybe it's just me, but I don't like using global variables for this. > You could put them into a struct and pass it as a parameter to the > functions. Done. > > + cur_end += TARGET_PAGE_ALIGN(cur_end + mb_buf_size); > > Sure that this isn't one cur_end too much if you're using += ? Yes, well spotted, thanks. > > + asprintf(&kcmdline, "%s %s", > > kernel_filename, kernel_cmdline); > > Use snprintf instead of asprintf, the latter isn't available everywhere. Done. New version follows Subject: [PATCH] multiboot: Support arbitrary number of modules Signed-off-by: Adam Lackorzynski --- hw/pc.c | 182 ++++++++++++++++++++++++++++++++++++++++++--------------------- 1 files changed, 121 insertions(+), 61 deletions(-) diff --git a/hw/pc.c b/hw/pc.c index 408d6d6..e229cf6 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -588,6 +588,72 @@ static long get_file_size(FILE *f) #error multiboot struct needs to fit in 16 bit real mode #endif +enum { + /* Multiboot info */ + MBI_FLAGS = 0, + MBI_MEM_LOWER = 4, + MBI_MEM_UPPER = 8, + MBI_BOOT_DEVICE = 12, + MBI_CMDLINE = 16, + MBI_MODS_COUNT = 20, + MBI_MODS_ADDR = 24, + MBI_MMAP_ADDR = 48, + + MBI_SIZE = 88, + + /* Multiboot modules */ + MB_MOD_START = 0, + MB_MOD_END = 4, + MB_MOD_CMDLINE = 8, + + MB_MOD_SIZE = 16, + + /* Region offsets */ + ADDR_E820_MAP = MULTIBOOT_STRUCT_ADDR + 0, + ADDR_MBI = ADDR_E820_MAP + 0x500, + + /* Multiboot flags */ + MULTIBOOT_FLAGS_MEMORY = 1 << 0, + MULTIBOOT_FLAGS_BOOT_DEVICE = 1 << 1, + MULTIBOOT_FLAGS_CMDLINE = 1 << 2, + MULTIBOOT_FLAGS_MODULES = 1 << 3, + MULTIBOOT_FLAGS_MMAP = 1 << 6, +}; + +struct mb_state { + void *mb_buf; + uint32_t mb_buf_phys; /* address in target */ + int mb_buf_size; /* size of mb_buf in bytes */ + int mb_mods_avail; /* available slots for mb_mods */ + int mb_mods_count; + int mb_cmdlines_pos; +}; + +static uint32_t mb_add_cmdline(struct mb_state *s, const char *cmdline) +{ + int len = strlen(cmdline) + 1; + uint32_t p = s->mb_cmdlines_pos; + + assert(p + len <= s->mb_buf_size); + pstrcpy((char *)s->mb_buf + p, len, cmdline); + s->mb_cmdlines_pos += len; + return s->mb_buf_phys + p; +} + +static void mb_add_mod(struct mb_state *s, + uint32_t start, uint32_t end, + uint32_t cmdline_phys) +{ + char *p; + assert(s->mb_mods_count < s->mb_mods_avail); + + p = (char *)s->mb_buf + MB_MOD_SIZE * s->mb_mods_count; + stl_p(p + MB_MOD_START, start); + stl_p(p + MB_MOD_END, end); + stl_p(p + MB_MOD_CMDLINE, cmdline_phys); + s->mb_mods_count++; +} + static int load_multiboot(void *fw_cfg, FILE *f, const char *kernel_filename, @@ -600,11 +666,9 @@ static int load_multiboot(void *fw_cfg, uint32_t mh_entry_addr; uint32_t mh_load_addr; uint32_t mb_kernel_size; - uint32_t mmap_addr = MULTIBOOT_STRUCT_ADDR; - uint32_t mb_bootinfo = MULTIBOOT_STRUCT_ADDR + 0x500; - uint32_t mb_mod_end; - uint8_t bootinfo[0x500]; - uint32_t cmdline = 0x200; + uint32_t cur_end; + struct mb_state mbs; + uint8_t bootinfo[MBI_SIZE]; /* Ok, let's see if it is a multiboot image. The header is 12x32bit long, so the latest entry may be 8192 - 48. */ @@ -628,6 +692,7 @@ static int load_multiboot(void *fw_cfg, fprintf(stderr, "qemu: I believe we found a multiboot image!\n"); #endif memset(bootinfo, 0, sizeof(bootinfo)); + memset(&mbs, 0, sizeof(mbs)); if (flags & 0x00000004) { /* MULTIBOOT_HEADER_HAS_VBE */ fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n"); @@ -687,90 +752,82 @@ static int load_multiboot(void *fw_cfg, fclose(f); } - /* blob size is only the kernel for now */ - mb_mod_end = mh_load_addr + mb_kernel_size; + cur_end = TARGET_PAGE_ALIGN(mh_load_addr + mb_kernel_size); + + /* Calculate space for cmdlines and mb_mods */ + mbs.mb_buf_size += strlen(kernel_filename) + 1; + mbs.mb_buf_size += strlen(kernel_cmdline) + 1; + if (initrd_filename) { + const char *r = initrd_filename; + mbs.mb_buf_size += strlen(r) + 1; + mbs.mb_mods_avail = 1; + while ((r = strchr(r, ','))) { + mbs.mb_mods_avail++; + r++; + } + mbs.mb_buf_size += MB_MOD_SIZE * mbs.mb_mods_avail; + } + + mbs.mb_buf = qemu_mallocz(mbs.mb_buf_size); + mbs.mb_buf_phys = cur_end; + + mbs.mb_cmdlines_pos = mbs.mb_mods_avail * MB_MOD_SIZE; + + cur_end = TARGET_PAGE_ALIGN(cur_end + mbs.mb_buf_size); - /* load modules */ - stl_p(bootinfo + 20, 0x0); /* mods_count */ if (initrd_filename) { - uint32_t mb_mod_info = 0x100; - uint32_t mb_mod_cmdline = 0x300; - uint32_t mb_mod_start = mh_load_addr; - uint32_t mb_mod_length = mb_kernel_size; char *next_initrd; - char *next_space; - int mb_mod_count = 0; do { - if (mb_mod_info + 16 > mb_mod_cmdline) { - printf("WARNING: Too many modules loaded, aborting.\n"); - break; - } + char *next_space; + uint32_t mb_mod_length; next_initrd = strchr(initrd_filename, ','); if (next_initrd) *next_initrd = '\0'; /* if a space comes after the module filename, treat everything after that as parameters */ - pstrcpy((char*)bootinfo + mb_mod_cmdline, - sizeof(bootinfo) - mb_mod_cmdline, - initrd_filename); - stl_p(bootinfo + mb_mod_info + 8, mb_bootinfo + mb_mod_cmdline); /* string */ - mb_mod_cmdline += strlen(initrd_filename) + 1; - if (mb_mod_cmdline > sizeof(bootinfo)) { - mb_mod_cmdline = sizeof(bootinfo); - printf("WARNING: Too many module cmdlines loaded, aborting.\n"); - break; - } + uint32_t c = mb_add_cmdline(&mbs, initrd_filename); if ((next_space = strchr(initrd_filename, ' '))) *next_space = '\0'; #ifdef DEBUG_MULTIBOOT printf("multiboot loading module: %s\n", initrd_filename); #endif - mb_mod_start = (mb_mod_start + mb_mod_length + (TARGET_PAGE_SIZE - 1)) - & (TARGET_PAGE_MASK); mb_mod_length = get_image_size(initrd_filename); if (mb_mod_length < 0) { fprintf(stderr, "failed to get %s image size\n", initrd_filename); exit(1); } - mb_mod_end = mb_mod_start + mb_mod_length; - rom_add_file_fixed(initrd_filename, mb_mod_start); + rom_add_file_fixed(initrd_filename, cur_end); - mb_mod_count++; - stl_p(bootinfo + mb_mod_info + 0, mb_mod_start); - stl_p(bootinfo + mb_mod_info + 4, mb_mod_start + mb_mod_length); - stl_p(bootinfo + mb_mod_info + 12, 0x0); /* reserved */ + mb_add_mod(&mbs, cur_end, cur_end + mb_mod_length, c); #ifdef DEBUG_MULTIBOOT - printf("mod_start: %#x\nmod_end: %#x\n", mb_mod_start, - mb_mod_start + mb_mod_length); + printf("mod_start: %#x\nmod_end: %#x\n", cur_end, + cur_end + mb_mod_length); #endif + cur_end = TARGET_PAGE_ALIGN(cur_end + mb_mod_length); initrd_filename = next_initrd+1; - mb_mod_info += 16; } while (next_initrd); - stl_p(bootinfo + 20, mb_mod_count); /* mods_count */ - stl_p(bootinfo + 24, mb_bootinfo + 0x100); /* mods_addr */ } /* Commandline support */ - stl_p(bootinfo + 16, mb_bootinfo + cmdline); - snprintf((char*)bootinfo + cmdline, 0x100, "%s %s", + 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 + MBI_MODS_ADDR, mbs.mb_buf_phys); + stl_p(bootinfo + MBI_MODS_COUNT, mbs.mb_mods_count); /* mods_count */ /* the kernel is where we want it to be now */ -#define MULTIBOOT_FLAGS_MEMORY (1 << 0) -#define MULTIBOOT_FLAGS_BOOT_DEVICE (1 << 1) -#define MULTIBOOT_FLAGS_CMDLINE (1 << 2) -#define MULTIBOOT_FLAGS_MODULES (1 << 3) -#define MULTIBOOT_FLAGS_MMAP (1 << 6) - stl_p(bootinfo, MULTIBOOT_FLAGS_MEMORY - | MULTIBOOT_FLAGS_BOOT_DEVICE - | MULTIBOOT_FLAGS_CMDLINE - | MULTIBOOT_FLAGS_MODULES - | MULTIBOOT_FLAGS_MMAP); - stl_p(bootinfo + 4, 640); /* mem_lower */ - stl_p(bootinfo + 8, ram_size / 1024); /* mem_upper */ - stl_p(bootinfo + 12, 0x8001ffff); /* XXX: use the -boot switch? */ - stl_p(bootinfo + 48, mmap_addr); /* mmap_addr */ + stl_p(bootinfo + MBI_FLAGS, MULTIBOOT_FLAGS_MEMORY + | MULTIBOOT_FLAGS_BOOT_DEVICE + | MULTIBOOT_FLAGS_CMDLINE + | MULTIBOOT_FLAGS_MODULES + | MULTIBOOT_FLAGS_MMAP); + stl_p(bootinfo + MBI_MEM_LOWER, 640); + stl_p(bootinfo + MBI_MEM_UPPER, ram_size / 1024); + stl_p(bootinfo + MBI_BOOT_DEVICE, 0x8001ffff); /* XXX: use the -boot switch? */ + stl_p(bootinfo + MBI_MMAP_ADDR, ADDR_E820_MAP); #ifdef DEBUG_MULTIBOOT fprintf(stderr, "multiboot: mh_entry_addr = %#x\n", mh_entry_addr); @@ -778,11 +835,14 @@ static int load_multiboot(void *fw_cfg, /* Pass variables to option rom */ fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, mh_entry_addr); - fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_ADDR, mb_bootinfo); - fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, mmap_addr); + fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_ADDR, ADDR_MBI); + fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, ADDR_E820_MAP); rom_add_blob_fixed("multiboot-info", bootinfo, sizeof(bootinfo), - mb_bootinfo); + ADDR_MBI); + + rom_add_blob_fixed("multiboot-info-mods+cmdl", mbs.mb_buf, + mbs.mb_buf_size, mbs.mb_buf_phys); option_rom[nb_option_roms] = "multiboot.bin"; nb_option_roms++;