* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.