Re: [Xen-devel] [PATCH v6 04/15] x86: add multiboot2 protocol support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.16 at 22:18, <daniel.kiper@xxxxxxxxxx> wrote:
> @@ -65,13 +82,11 @@ static u32 copy_string(u32 src)
>      return copy_mem(src, p - src + 1);
>  }
>  
> -multiboot_info_t __stdcall *reloc(u32 mbi_in, u32 trampoline)
> +static multiboot_info_t *mbi_mbi(u32 mbi_in)

This is rather unhelpful a name - how about mbi_reloc() (and then
below mbi2_reloc() accordingly)?

> +static multiboot_info_t *mbi2_mbi(u32 mbi_in)
> +{
> +    const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
> +    const multiboot2_memory_map_t *mmap_src;
> +    const multiboot2_tag_t *tag;
> +    module_t *mbi_out_mods = NULL;
> +    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 = _p(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 = _p(ptr); (u32)tag - mbi_in < mbi_fix->total_size;
> +          tag = _p(ALIGN_UP((u32)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) )
> +    {
> +        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;

Even if this is the first adjustment to the field right now, code would
be more robust wrt future additions if you used |= here just like you
do further down.

> +        mbi_out->mods_addr = alloc_mem(mbi_out->mods_count * 
> sizeof(module_t));
> +        mbi_out_mods = _p(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 = _p(ptr); (u32)tag - mbi_in < mbi_fix->total_size;
> +          tag = _p(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:
> +            if ( get_mb2_data(tag, mmap, entry_size) < sizeof(*mmap_src) )
> +                break;
> +
> +            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);
> +            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 = _p(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);

Just like you already do in other sizeof() instances, this as well as the
one in the loop control would better be sizeof(*mmap_dst).

> +                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->addr;
> +                mmap_dst[i].base_addr_high = (u32)(mmap_src->addr >> 32);
> +                mmap_dst[i].length_low = (u32)mmap_src->len;
> +                mmap_dst[i].length_high = (u32)(mmap_src->len >> 32);
> +                mmap_dst[i].type = mmap_src->type;
> +                mmap_src = _p(mmap_src) + get_mb2_data(tag, mmap, 
> entry_size);
> +            }
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_MODULE:
> +            if ( mod_idx >= mbi_out->mods_count )
> +                break;
> +
> +            mbi_out_mods[mod_idx].mod_start = get_mb2_data(tag, module, 
> mod_start);
> +            mbi_out_mods[mod_idx].mod_end = get_mb2_data(tag, module, 
> mod_end);
> +            ptr = get_mb2_string(tag, module, cmdline);
> +            mbi_out_mods[mod_idx].string = copy_string(ptr);

How is no (or an empty) command line being represented? Surely
you could avoid allocation and copying in that case? And should it be
possible for the cmdline field to be zero, you'd definitely have to
avoid it.

Jan

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