From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NGIGQ-0005KK-7S for qemu-devel@nongnu.org; Thu, 03 Dec 2009 15:26:58 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NGIGL-0005IY-Kl for qemu-devel@nongnu.org; Thu, 03 Dec 2009 15:26:57 -0500 Received: from [199.232.76.173] (port=33446 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NGIGL-0005IS-EW for qemu-devel@nongnu.org; Thu, 03 Dec 2009 15:26:53 -0500 Received: from mail-iw0-f197.google.com ([209.85.223.197]:51966) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NGIGL-0007xe-0s for qemu-devel@nongnu.org; Thu, 03 Dec 2009 15:26:53 -0500 Received: by iwn35 with SMTP id 35so1189801iwn.4 for ; Thu, 03 Dec 2009 12:26:52 -0800 (PST) Message-ID: <4B181F09.2070704@codemonkey.ws> Date: Thu, 03 Dec 2009 14:26:49 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 2/2] multiboot: Support arbitrary number of modules References: <20091130222544.GA21068@os.inf.tu-dresden.de> <20091130222650.GB21068@os.inf.tu-dresden.de> In-Reply-To: <20091130222650.GB21068@os.inf.tu-dresden.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Adam Lackorzynski Cc: qemu-devel@nongnu.org Adam Lackorzynski wrote: > multiboot: Support arbitrary number of modules > > > Signed-off-by: Adam Lackorzynski > --- > hw/pc.c | 216 +++++++++++++++++++++++++++++++++++++++++--------------------- > 1 files changed, 143 insertions(+), 73 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index 6bcfe1b..163bec1 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -507,6 +507,79 @@ 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; /* buffer holding kernel, cmdlines and mb_infos */ > + 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 modules infos */ > + int mb_mods_count; /* currently used slots of mb modules */ > + int offset_mbinfo; /* offset of mb-info's in bytes */ > + int offset_cmdlines; /* offset in buffer for cmdlines */ > + int offset_mods; /* offset of modules in bytes */ > +}; > Should be MultibootState. > +static uint32_t mb_add_cmdline(struct mb_state *s, const char *cmdline) > +{ > + int len = strlen(cmdline) + 1; > + uint32_t p = s->offset_cmdlines; > + > + pstrcpy((char *)s->mb_buf + p, len, cmdline); > + s->offset_cmdlines += 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 + s->offset_mbinfo + 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); > + > +#ifdef DEBUG_MULTIBOOT > + fprintf(stderr, "mod%02d: %08x - %08x\n", s->mb_mods_count, start, end); > +#endif > A debug macro would be a lot nicer. > + s->mb_mods_count++; > +} > + > static int load_multiboot(void *fw_cfg, > FILE *f, > const char *kernel_filename, > @@ -519,12 +592,8 @@ 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; > - uint8_t *mb_kernel_data; > + struct mb_state mbs; > + uint8_t bootinfo[MBI_SIZE]; > uint8_t *mb_bootinfo_data; > > /* Ok, let's see if it is a multiboot image. > @@ -549,6 +618,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"); > @@ -566,8 +636,8 @@ static int load_multiboot(void *fw_cfg, > mh_load_addr = mh_entry_addr = elf_entry; > mb_kernel_size = kernel_size; > > - mb_kernel_data = qemu_malloc(mb_kernel_size); > - if (rom_copy(mb_kernel_data, elf_entry, kernel_size) != kernel_size) { > + mbs.mb_buf = qemu_malloc(mb_kernel_size); > + if (rom_copy(mbs.mb_buf, elf_entry, kernel_size) != kernel_size) { > fprintf(stderr, "Error while fetching elf kernel from rom\n"); > exit(1); > } > @@ -604,104 +674,104 @@ static int load_multiboot(void *fw_cfg, > mb_kernel_size, mh_load_addr); > #endif > > - mb_kernel_data = qemu_malloc(mb_kernel_size); > + mbs.mb_buf = qemu_malloc(mb_kernel_size); > fseek(f, mb_kernel_text_offset, SEEK_SET); > - fread(mb_kernel_data, 1, mb_kernel_size, f); > + fread(mbs.mb_buf, 1, mb_kernel_size, f); > fclose(f); > } > > - /* blob size is only the kernel for now */ > - mb_mod_end = mh_load_addr + mb_kernel_size; > + mbs.mb_buf_phys = mh_load_addr; > + > + mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_kernel_size); > + mbs.offset_mbinfo = mbs.mb_buf_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_size = TARGET_PAGE_ALIGN(mbs.mb_buf_size); > + > + // enlarge mb_buf to hold cmdlines and mb-info structs > Please avoid C99 comments. > + > + mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_mod_length + mbs.mb_buf_size); > + mbs.mb_buf = qemu_realloc(mbs.mb_buf, mbs.mb_buf_size); > + > + load_image(initrd_filename, (unsigned char *)mbs.mb_buf + offs); > + mb_add_mod(&mbs, mbs.mb_buf_phys + offs, > + mbs.mb_buf_phys + offs + mb_mod_length, c); > You've got a mix of tabs and spaces. Regards, Anthony Liguori