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

[Anthony Liguori <anthony@codemonkey.ws>]

Adam Lackorzynski wrote:
> multiboot: Support arbitrary number of modules
>
>
> Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
> ---
>  hw/pc.c |  216 +++++++++++++++++++++++++++++++++++++++++---------------------
>  1 files changed, 143 insertions(+), 73 deletions(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 6bcfe1b..163bec1 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -507,6 +507,79 @@ 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;         /* buffer holding kernel, cmdlines and mb_infos */
> +    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 modules infos */
> +    int mb_mods_count;    /* currently used slots of mb modules */
> +    int offset_mbinfo;    /* offset of mb-info's in bytes */
> +    int offset_cmdlines;  /* offset in buffer for cmdlines */
> +    int offset_mods;      /* offset of modules in bytes */
> +};
>   

Should be MultibootState.

> +static uint32_t mb_add_cmdline(struct mb_state *s, const char *cmdline)
> +{
> +    int len = strlen(cmdline) + 1;
> +    uint32_t p = s->offset_cmdlines;
> +
> +    pstrcpy((char *)s->mb_buf + p, len, cmdline);
> +    s->offset_cmdlines += 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 + s->offset_mbinfo + 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);
> +
> +#ifdef DEBUG_MULTIBOOT
> +    fprintf(stderr, "mod%02d: %08x - %08x\n", s->mb_mods_count, start, end);
> +#endif
>   

A debug macro would be a lot nicer.

> +    s->mb_mods_count++;
> +}
> +
>  static int load_multiboot(void *fw_cfg,
>                            FILE *f,
>                            const char *kernel_filename,
> @@ -519,12 +592,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;
> -    uint8_t *mb_kernel_data;
> +    struct mb_state mbs;
> +    uint8_t bootinfo[MBI_SIZE];
>      uint8_t *mb_bootinfo_data;
>  
>      /* Ok, let's see if it is a multiboot image.
> @@ -549,6 +618,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");
> @@ -566,8 +636,8 @@ static int load_multiboot(void *fw_cfg,
>          mh_load_addr = mh_entry_addr = elf_entry;
>          mb_kernel_size = kernel_size;
>  
> -        mb_kernel_data = qemu_malloc(mb_kernel_size);
> -        if (rom_copy(mb_kernel_data, elf_entry, kernel_size) != kernel_size) {
> +        mbs.mb_buf = qemu_malloc(mb_kernel_size);
> +        if (rom_copy(mbs.mb_buf, elf_entry, kernel_size) != kernel_size) {
>              fprintf(stderr, "Error while fetching elf kernel from rom\n");
>              exit(1);
>          }
> @@ -604,104 +674,104 @@ static int load_multiboot(void *fw_cfg,
>                  mb_kernel_size, mh_load_addr);
>  #endif
>  
> -        mb_kernel_data = qemu_malloc(mb_kernel_size);
> +        mbs.mb_buf = qemu_malloc(mb_kernel_size);
>          fseek(f, mb_kernel_text_offset, SEEK_SET);
> -        fread(mb_kernel_data, 1, mb_kernel_size, f);
> +        fread(mbs.mb_buf, 1, mb_kernel_size, f);
>          fclose(f);
>      }
>  
> -    /* blob size is only the kernel for now */
> -    mb_mod_end = mh_load_addr + mb_kernel_size;
> +    mbs.mb_buf_phys = mh_load_addr;
> +
> +    mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_kernel_size);
> +    mbs.offset_mbinfo = mbs.mb_buf_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_size = TARGET_PAGE_ALIGN(mbs.mb_buf_size);
> +
> +    // enlarge mb_buf to hold cmdlines and mb-info structs
>   

Please avoid C99 comments.

> +
> +	    mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_mod_length + mbs.mb_buf_size);
> +	    mbs.mb_buf = qemu_realloc(mbs.mb_buf, mbs.mb_buf_size);
> +
> +            load_image(initrd_filename, (unsigned char *)mbs.mb_buf + offs);
> +            mb_add_mod(&mbs, mbs.mb_buf_phys + offs,
> +                       mbs.mb_buf_phys + offs + mb_mod_length, c);
>   

You've got a mix of tabs and spaces.

Regards,

Anthony Liguori