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 <a...@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 <a...@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 a...@os.inf.tu-dresden.de
Lackorzynski http://os.inf.tu-dresden.de/~adam/
