Re: [Xen-devel] [PATCH v4 09/19] x86: add multiboot2 protocol support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.08.16 at 01:04, <daniel.kiper@xxxxxxxxxx> wrote:
> @@ -106,3 +121,119 @@ multiboot_info_t __stdcall *reloc(u32 mbi_in, u32 
> trampoline)
>  
>      return mbi_out;
>  }
> +
> +static multiboot_info_t *mbi2_mbi(u32 mbi_in)
> +{
> +    const multiboot2_memory_map_t *mmap_src;
> +    const multiboot2_tag_t *tag;
> +    /* Do not complain that mbi_out_mods is not initialized. */
> +    module_t *mbi_out_mods = NULL;

Please drop the comment.

> +    memory_map_t *mmap_dst;
> +    multiboot_info_t *mbi_out;
> +    u32 ptr;
> +    unsigned int i, mod_idx = 0;
> +
> +    ptr = alloc_mem(sizeof(*mbi_out));
> +    mbi_out = (multiboot_info_t *)ptr;
> +    zero_mem(ptr, sizeof(*mbi_out));
> +
> +    /* Skip Multiboot2 information fixed part. */
> +    ptr = ALIGN_UP(mbi_in + sizeof(multiboot2_fixed_t), 
> MULTIBOOT2_TAG_ALIGN);
> +
> +    /* Get the number of modules. */
> +    for ( tag = (multiboot2_tag_t *)ptr;
> +          (u32)tag - mbi_in < ((multiboot2_fixed_t *)mbi_in)->total_size;
> +          tag = (multiboot2_tag_t *)ALIGN_UP((u32)tag + tag->size, 
> MULTIBOOT2_TAG_ALIGN) )

There's still a lot of casting here, but I agree it's not straightforward
to improve the situation.

> +        if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE )
> +            ++mbi_out->mods_count;
> +        else if ( tag->type == MULTIBOOT2_TAG_TYPE_END )
> +            break;
> +
> +    if ( mbi_out->mods_count )
> +    {
> +        mbi_out->flags = MBI_MODULES;
> +        mbi_out->mods_addr = alloc_mem(mbi_out->mods_count * 
> sizeof(module_t));
> +        mbi_out_mods = (module_t *)mbi_out->mods_addr;
> +    }
> +
> +    /* Skip Multiboot2 information fixed part. */
> +    ptr = ALIGN_UP(mbi_in + sizeof(multiboot2_fixed_t), 
> MULTIBOOT2_TAG_ALIGN);
> +
> +    /* Put all needed data into mbi_out. */
> +    for ( tag = (multiboot2_tag_t *)ptr;
> +          (u32)tag - mbi_in < ((multiboot2_fixed_t *)mbi_in)->total_size;
> +          tag = (multiboot2_tag_t *)ALIGN_UP((u32)tag + tag->size, 
> MULTIBOOT2_TAG_ALIGN) )
> +        switch ( tag->type )
> +        {
> +        case MULTIBOOT2_TAG_TYPE_BOOT_LOADER_NAME:
> +            mbi_out->flags |= MBI_LOADERNAME;
> +            ptr = get_mb2_string(tag, string, string);
> +            mbi_out->boot_loader_name = copy_string(ptr);
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_CMDLINE:
> +            mbi_out->flags |= MBI_CMDLINE;
> +            ptr = get_mb2_string(tag, string, string);
> +            mbi_out->cmdline = copy_string(ptr);
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO:
> +            mbi_out->flags |= MBI_MEMLIMITS;
> +            mbi_out->mem_lower = get_mb2_data(tag, basic_meminfo, mem_lower);
> +            mbi_out->mem_upper = get_mb2_data(tag, basic_meminfo, mem_upper);
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_MMAP:
> +            mbi_out->flags |= MBI_MEMMAP;
> +            mbi_out->mmap_length = get_mb2_data(tag, mmap, size);
> +            mbi_out->mmap_length -= sizeof(multiboot2_tag_mmap_t);
> +            mbi_out->mmap_length /= get_mb2_data(tag, mmap, entry_size);

Okay, here you use the entry size as provided by grub, allowing
compatibility even when the boot loader uses a newer layout (with
extra fields added to the end). However ...

> +            mbi_out->mmap_length *= sizeof(memory_map_t);
> +
> +            mbi_out->mmap_addr = alloc_mem(mbi_out->mmap_length);
> +
> +            mmap_src = get_mb2_data(tag, mmap, entries);
> +            mmap_dst = (memory_map_t *)mbi_out->mmap_addr;
> +
> +            for ( i = 0; i < mbi_out->mmap_length / sizeof(memory_map_t); 
> i++ )
> +            {
> +                /* Init size member properly. */
> +                mmap_dst[i].size = sizeof(memory_map_t);
> +                mmap_dst[i].size -= sizeof(((memory_map_t){0}).size);
> +                /* Now copy a given region data. */
> +                mmap_dst[i].base_addr_low = (u32)mmap_src[i].addr;
> +                mmap_dst[i].base_addr_high = (u32)(mmap_src[i].addr >> 32);
> +                mmap_dst[i].length_low = (u32)mmap_src[i].len;
> +                mmap_dst[i].length_high = (u32)(mmap_src[i].len >> 32);

... here you index an array of type multiboot2_memory_map_t.

Also note that in any event you should check that
entry_size >= sizeof(*mmap_src) (or, if you don't want [or need]
to go with the flexible model, ==).

> +typedef struct {
> +    u32 type;
> +    u32 size;
> +    char string[0];

This is not a public header - please omit the 0 here and in a similar
place further down, as we're using all sorts of gcc extensions anyway.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel