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 a...@os.inf.tu-dresden.de:
>>> From: Adam Lackorzynski <a...@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 <a...@os.inf.tu-dresden.de>
>>
>> Acked-by: Kevin Wolf <kw...@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 <a...@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
