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

[Anthony Liguori <aliguori@linux.vnet.ibm.com>]

Adam Lackorzynski wrote:
> multiboot: Support arbitrary number of modules.
>
> Addressed comments by Anthony.
>
> Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
> ---
>  hw/pc.c |  260 ++++++++++++++++++++++++++++++++++++++------------------------
>  1 files changed, 159 insertions(+), 101 deletions(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 8c1b7ea..47d7796 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -51,6 +51,12 @@
>  /* Show multiboot debug output */
>  //#define DEBUG_MULTIBOOT
>  
> +#ifdef DEBUG_MULTIBOOT
> +#define mb_debug(a...) fprintf(stderr, ## a)
> +#else
> +#define mb_debug(a...)
> +#endif
> +
>  #define BIOS_FILENAME "bios.bin"
>  
>  #define PC_MAX_BIOS_SIZE (4 * 1024 * 1024)
> @@ -512,6 +518,77 @@ 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 MultibootState {
> +    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 typedef struct MultibootState.  See CODING_STYLE.  Also, 
shouldn't these offsets be something other than int?  Does multiboot 
always load in 32-bit mode (never 64-bit mode?).

mb_buf_phys should not be a u32.  It should be a target address type.

-- 
Regards,

Anthony Liguori