Re: [Xen-devel] [PATCH v5 09/16] x86: add multiboot2 protocol support

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/08/2016 23:43, Daniel Kiper wrote:
> @@ -106,3 +121,122 @@ 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;
> +    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 = (multiboot_info_t *)ptr;

It occurs to me that a lot of this code would be more simple if you also
pulled the

#define _p(val) ((void *)(unsigned long)val)

in from the usual header files.  It would shorten a lot of the
(multiboot_info_t *) casting, and reduce most multi-line statements to
single lines.

Can I also suggest using:

static multiboot_info_t *mbi2_mbi(u32 mbi_in_addr)
{
    const multiboot_info_t *mbi_in = _p(mbi_in_addr);
...

This would avoid repeatedly needing to cast the old mbi_in integer to
access fields like ->total_size.

> +    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) )

Braces here please
{

> +        if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE )
> +            ++mbi_out->mods_count;
> +        else if ( tag->type == MULTIBOOT2_TAG_TYPE_END )
> +            break;

}

Given the complexity of the for statement, the exact code contained
inside the loop could do with this added clarity.

> +
> +    if ( mbi_out->mods_count )
> +    {

In the case that this condition evaluates false, and...

> +        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:
> +            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 = (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_src = (void *)mmap_src + i * get_mb2_data(tag, mmap, 
> entry_size);
> +                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;
> +            }
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_MODULE:

... this tag gets found, then a NULL mbi_out_mods gets followed.

I realise that this calculation is a result of a double loop over the
tag list, but it would be better to code slightly more resiliently
(given both the very liberal use of casting, and the possiblity that
something else is modifying the memory while we are looking at it). 
Something like:

if ( mod_idx < mbi_out->mods_count )
{

> +            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);
> +            mbi_out_mods[mod_idx].reserved = 0;
> +            ++mod_idx;

}

should do.

~Andrew

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