[Qemu-devel] [PATCH 1/2] multiboot: Fix cmdline of modules

[Qemu-devel] [PATCH 1/2] multiboot: Fix cmdline of modules

[adam]

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

[Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules

[adam]

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

Re: [Qemu-devel] [PATCH 1/2] multiboot: Fix cmdline of modules

[Kevin Wolf]

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>

Re: [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules

[Adam Lackorzynski]

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/

Re: [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules

[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>
---
 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/

Re: [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 2/2] multiboot: Limit number of multiboot modules

[Adam Lackorzynski]

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/

[Qemu-devel] Re: [PATCH 2/2] multiboot: Limit number of multiboot modules

[Kevin Wolf]

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