* [Qemu-devel] [PATCH 1/2] multiboot: Fix cmdline of modules @ 2009-10-11 13:48 adam 2009-10-11 13:48 ` [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules adam 2009-10-12 10:28 ` [Qemu-devel] [PATCH 1/2] multiboot: Fix cmdline of modules Kevin Wolf 0 siblings, 2 replies; 9+ messages in thread From: adam @ 2009-10-11 13:48 UTC (permalink / raw) To: qemu-devel From: Adam Lackorzynski <adam@os.inf.tu-dresden.de> Fix address specified for cmdline value of module in multiboot structure. Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de> --- hw/pc.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/hw/pc.c b/hw/pc.c index 19bef49..e34ad9c 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -710,7 +710,7 @@ static int load_multiboot(void *fw_cfg, pstrcpy((char*)bootinfo + mb_mod_cmdline, sizeof(bootinfo) - mb_mod_cmdline, initrd_filename); - stl_p(bootinfo + mb_mod_info + 8, mb_mod_cmdline); /* string */ + 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); -- 1.6.4.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules 2009-10-11 13:48 [Qemu-devel] [PATCH 1/2] multiboot: Fix cmdline of modules adam @ 2009-10-11 13:48 ` adam 2009-10-12 10:43 ` Kevin Wolf 2009-10-12 10:28 ` [Qemu-devel] [PATCH 1/2] multiboot: Fix cmdline of modules Kevin Wolf 1 sibling, 1 reply; 9+ messages in thread From: adam @ 2009-10-11 13:48 UTC (permalink / raw) To: qemu-devel From: Adam Lackorzynski <adam@os.inf.tu-dresden.de> Add size checks to avoid overwriting the multiboot structure when too many modules are loaded. Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de> --- hw/pc.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-) diff --git a/hw/pc.c b/hw/pc.c index e34ad9c..b190d22 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -702,6 +702,10 @@ static int load_multiboot(void *fw_cfg, int mb_mod_count = 0; do { + if (mb_mod_info + 16 > mb_mod_cmdline) { + printf("WARNING: Too many modules loaded, aborting.\n"); + break; + } next_initrd = strchr(initrd_filename, ','); if (next_initrd) *next_initrd = '\0'; @@ -712,8 +716,11 @@ static int load_multiboot(void *fw_cfg, 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)) + if (mb_mod_cmdline > sizeof(bootinfo)) { mb_mod_cmdline = sizeof(bootinfo); + printf("WARNING: Too many module cmdlines loaded, aborting.\n"); + break; + } if ((next_space = strchr(initrd_filename, ' '))) *next_space = '\0'; #ifdef DEBUG_MULTIBOOT -- 1.6.4.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules 2009-10-11 13:48 ` [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules adam @ 2009-10-12 10:43 ` Kevin Wolf 2009-10-12 16:40 ` Adam Lackorzynski 2009-10-14 16:11 ` Adam Lackorzynski 0 siblings, 2 replies; 9+ messages in thread From: Kevin Wolf @ 2009-10-12 10:43 UTC (permalink / raw) To: adam; +Cc: qemu-devel Am 11.10.2009 15:48, schrieb adam@os.inf.tu-dresden.de: > From: Adam Lackorzynski <adam@os.inf.tu-dresden.de> > > Add size checks to avoid overwriting the multiboot structure > when too many modules are loaded. > > Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de> Acked-by: Kevin Wolf <kwolf@redhat.com> Looks correct, too, and is definitely an improvement over the current code. But can't we make it more dynamic in a second step? I don't like arbitrary fixed limits. Kevin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules 2009-10-12 10:43 ` Kevin Wolf @ 2009-10-12 16:40 ` Adam Lackorzynski 2009-10-14 16:11 ` Adam Lackorzynski 1 sibling, 0 replies; 9+ messages in thread From: Adam Lackorzynski @ 2009-10-12 16:40 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel On Mon Oct 12, 2009 at 12:43:38 +0200, Kevin Wolf wrote: > Am 11.10.2009 15:48, schrieb adam@os.inf.tu-dresden.de: > > From: Adam Lackorzynski <adam@os.inf.tu-dresden.de> > > > > Add size checks to avoid overwriting the multiboot structure > > when too many modules are loaded. > > > > Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de> > > Acked-by: Kevin Wolf <kwolf@redhat.com> > > Looks correct, too, and is definitely an improvement over the current > code. But can't we make it more dynamic in a second step? I don't like > arbitrary fixed limits. Yes, definitely. I'm looking into this. Adam -- Adam adam@os.inf.tu-dresden.de Lackorzynski http://os.inf.tu-dresden.de/~adam/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules 2009-10-12 10:43 ` Kevin Wolf 2009-10-12 16:40 ` Adam Lackorzynski @ 2009-10-14 16:11 ` Adam Lackorzynski 2009-10-19 8:30 ` Kevin Wolf 1 sibling, 1 reply; 9+ messages in thread From: Adam Lackorzynski @ 2009-10-14 16:11 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel Hi, On Mon Oct 12, 2009 at 12:43:38 +0200, Kevin Wolf wrote: > Am 11.10.2009 15:48, schrieb adam@os.inf.tu-dresden.de: > > From: Adam Lackorzynski <adam@os.inf.tu-dresden.de> > > > > Add size checks to avoid overwriting the multiboot structure > > when too many modules are loaded. > > > > Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de> > > Acked-by: Kevin Wolf <kwolf@redhat.com> > > Looks correct, too, and is definitely an improvement over the current > code. But can't we make it more dynamic in a second step? I don't like > arbitrary fixed limits. So, what do you think about the following? Removes any limit and loaded over hundred modules in my setup. Subject: [PATCH 3/3] multiboot: Support arbitrary number of modules Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de> --- hw/pc.c | 177 +++++++++++++++++++++++++++++++++++++++++---------------------- 1 files changed, 116 insertions(+), 61 deletions(-) diff --git a/hw/pc.c b/hw/pc.c index 21771c9..df01ab7 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -588,6 +588,68 @@ 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, +}; + +#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) + +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; + +static uint32_t mb_add_cmdline(const char *cmdline) +{ + int len = strlen(cmdline) + 1; + uint32_t p = mb_cmdlines_pos; + + assert(p + len <= mb_buf_size); + pstrcpy((char *)mb_buf + p, len, cmdline); + mb_cmdlines_pos += len; + return mb_buf_phys + p; +} + +static void mb_add_mod(uint32_t start, uint32_t end, + uint32_t cmdline_phys) +{ + char *p; + assert(mb_mods_count < mb_mods_avail); + + p = (char *)mb_buf + MB_MOD_SIZE * 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); + mb_mods_count++; +} + static int load_multiboot(void *fw_cfg, FILE *f, const char *kernel_filename, @@ -600,11 +662,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; + uint32_t cur_end; + 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. */ @@ -687,90 +746,83 @@ 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 */ + mb_buf_size += strlen(kernel_filename) + 1; + mb_buf_size += strlen(kernel_cmdline) + 1; + if (initrd_filename) { + const char *r = initrd_filename; + mb_buf_size += strlen(r) + 1; + mb_mods_avail = 1; + while ((r = strchr(r, ','))) { + mb_mods_avail++; + r++; + } + mb_buf_size += MB_MOD_SIZE * mb_mods_avail; + } + + mb_buf = qemu_mallocz(mb_buf_size); + mb_buf_phys = cur_end; + + mb_cmdlines_pos = mb_mods_avail * MB_MOD_SIZE; + + cur_end += TARGET_PAGE_ALIGN(cur_end + 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(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(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; + asprintf(&kcmdline, "%s %s", kernel_filename, kernel_cmdline); + stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(kcmdline)); + free(kcmdline); + + stl_p(bootinfo + MBI_MODS_ADDR, mb_buf_phys); + stl_p(bootinfo + MBI_MODS_COUNT, 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 +830,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", mb_buf, mb_buf_size, + mb_buf_phys); option_rom[nb_option_roms] = "multiboot.bin"; nb_option_roms++; Adam -- Adam adam@os.inf.tu-dresden.de Lackorzynski http://os.inf.tu-dresden.de/~adam/ ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules 2009-10-14 16:11 ` Adam Lackorzynski @ 2009-10-19 8:30 ` Kevin Wolf 2009-10-20 18:22 ` Adam Lackorzynski 0 siblings, 1 reply; 9+ messages in thread From: Kevin Wolf @ 2009-10-19 8:30 UTC (permalink / raw) To: Adam Lackorzynski; +Cc: qemu-devel Am 14.10.2009 18:11, schrieb Adam Lackorzynski: > Hi, > > On Mon Oct 12, 2009 at 12:43:38 +0200, Kevin Wolf wrote: >> Am 11.10.2009 15:48, schrieb adam@os.inf.tu-dresden.de: >>> From: Adam Lackorzynski <adam@os.inf.tu-dresden.de> >>> >>> Add size checks to avoid overwriting the multiboot structure >>> when too many modules are loaded. >>> >>> Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de> >> >> Acked-by: Kevin Wolf <kwolf@redhat.com> >> >> Looks correct, too, and is definitely an improvement over the current >> code. But can't we make it more dynamic in a second step? I don't like >> arbitrary fixed limits. > > So, what do you think about the following? Removes any limit and loaded > over hundred modules in my setup. > > > Subject: [PATCH 3/3] multiboot: Support arbitrary number of modules > > Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de> Looks good in general. I'm adding some minor comments inline. > --- > hw/pc.c | 177 +++++++++++++++++++++++++++++++++++++++++---------------------- > 1 files changed, 116 insertions(+), 61 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index 21771c9..df01ab7 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -588,6 +588,68 @@ 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, > +}; > + > +#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. > + > +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. > + > +static uint32_t mb_add_cmdline(const char *cmdline) > +{ > + int len = strlen(cmdline) + 1; > + uint32_t p = mb_cmdlines_pos; > + > + assert(p + len <= mb_buf_size); > + pstrcpy((char *)mb_buf + p, len, cmdline); > + mb_cmdlines_pos += len; > + return mb_buf_phys + p; > +} > + > +static void mb_add_mod(uint32_t start, uint32_t end, > + uint32_t cmdline_phys) > +{ > + char *p; > + assert(mb_mods_count < mb_mods_avail); > + > + p = (char *)mb_buf + MB_MOD_SIZE * 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); > + mb_mods_count++; > +} > + > static int load_multiboot(void *fw_cfg, > FILE *f, > const char *kernel_filename, > @@ -600,11 +662,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; > + uint32_t cur_end; > + 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. */ > @@ -687,90 +746,83 @@ 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 */ > + mb_buf_size += strlen(kernel_filename) + 1; > + mb_buf_size += strlen(kernel_cmdline) + 1; > + if (initrd_filename) { > + const char *r = initrd_filename; > + mb_buf_size += strlen(r) + 1; > + mb_mods_avail = 1; > + while ((r = strchr(r, ','))) { > + mb_mods_avail++; > + r++; > + } > + mb_buf_size += MB_MOD_SIZE * mb_mods_avail; > + } > + > + mb_buf = qemu_mallocz(mb_buf_size); > + mb_buf_phys = cur_end; > + > + mb_cmdlines_pos = mb_mods_avail * MB_MOD_SIZE; > + > + cur_end += TARGET_PAGE_ALIGN(cur_end + mb_buf_size); Sure that this isn't one cur_end too much if you're using += ? > > - /* 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(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(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; > + asprintf(&kcmdline, "%s %s", > kernel_filename, kernel_cmdline); Use snprintf instead of asprintf, the latter isn't available everywhere. Kevin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules 2009-10-19 8:30 ` Kevin Wolf @ 2009-10-20 18:22 ` Adam Lackorzynski [not found] ` <m3aazl8y4m.fsf@neno.mitica> 0 siblings, 1 reply; 9+ messages in thread From: Adam Lackorzynski @ 2009-10-20 18:22 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel 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 <adam@os.inf.tu-dresden.de> > > 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 <adam@os.inf.tu-dresden.de> --- 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++; -- 1.6.4.3 Adam -- Adam adam@os.inf.tu-dresden.de Lackorzynski http://os.inf.tu-dresden.de/~adam/ ^ permalink raw reply related [flat|nested] 9+ messages in thread
[parent not found: <m3aazl8y4m.fsf@neno.mitica>]
* [Qemu-devel] Re: [PATCH 2/2] multiboot: Limit number of multiboot modules [not found] ` <m3aazl8y4m.fsf@neno.mitica> @ 2009-10-21 9:29 ` Kevin Wolf 0 siblings, 0 replies; 9+ messages in thread From: Kevin Wolf @ 2009-10-21 9:29 UTC (permalink / raw) To: Juan Quintela; +Cc: qemu-devel Am 21.10.2009 01:05, schrieb Juan Quintela: > Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote: > > Hi > >> +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, >> +}; > > Why do you use a single enum, without name, and repeating the values? > I think that using more than one enum is better here. Right, this looks a bit strange even though it's correct. Otherwise the new patch looks okay and I'd suggest to submit it for inclusion. Kevin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] multiboot: Fix cmdline of modules 2009-10-11 13:48 [Qemu-devel] [PATCH 1/2] multiboot: Fix cmdline of modules adam 2009-10-11 13:48 ` [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules adam @ 2009-10-12 10:28 ` Kevin Wolf 1 sibling, 0 replies; 9+ messages in thread From: Kevin Wolf @ 2009-10-12 10:28 UTC (permalink / raw) To: adam; +Cc: qemu-devel Am 11.10.2009 15:48, schrieb adam@os.inf.tu-dresden.de: > From: Adam Lackorzynski <adam@os.inf.tu-dresden.de> > > Fix address specified for cmdline value of module in multiboot structure. > > Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de> Looks right and fixes my test case. Acked-by: Kevin Wolf <kwolf@redhat.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-10-21 9:30 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-10-11 13:48 [Qemu-devel] [PATCH 1/2] multiboot: Fix cmdline of modules adam
2009-10-11 13:48 ` [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules adam
2009-10-12 10:43 ` Kevin Wolf
2009-10-12 16:40 ` Adam Lackorzynski
2009-10-14 16:11 ` Adam Lackorzynski
2009-10-19 8:30 ` Kevin Wolf
2009-10-20 18:22 ` Adam Lackorzynski
[not found] ` <m3aazl8y4m.fsf@neno.mitica>
2009-10-21 9:29 ` [Qemu-devel] " Kevin Wolf
2009-10-12 10:28 ` [Qemu-devel] [PATCH 1/2] multiboot: Fix cmdline of modules Kevin Wolf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).