Re: [PATCH v2 08/23] x86: add multiboot2 protocol support

["Jan Beulich" <JBeulich@suse.com>]

>>> On 20.07.15 at 16:29, <daniel.kiper@oracle.com> wrote:
> v2 - not fixed yet:
>    - dynamic dependency generation for xen/arch/x86/boot/reloc.S;
>      this requires more work; I am not sure that it pays because
>      potential patch requires more changes than addition of just
>      multiboot2.h to Makefile
>      (suggested by Jan Beulich),

With the compiler.h inclusion done I don't think this is as necessary
anymore as it was previously.

>    - isolated/stray __packed attribute usage for multiboot2_memory_map_t
>      (suggested by Jan Beulich).

This one I don't, however, see why you didn't address. And with
un-addressed issues I think you should have tagged this RFC).

> @@ -19,6 +20,28 @@
>  #define BOOT_PSEUDORM_CS 0x0020
>  #define BOOT_PSEUDORM_DS 0x0028
>  
> +#define MB2_HT(name)      (MULTIBOOT2_HEADER_TAG_##name)
> +#define MB2_TT(name)      (MULTIBOOT2_TAG_TYPE_##name)
> +
> +        .macro mb2ht_args arg, args:vararg

Please make sure the oldest binutils we support allow for this
(introduced in 2.17; the oldest supported SLE seems to have it,
but I'm not sure about RHEL).

> +        .long \arg
> +        .ifnb \args
> +        mb2ht_args \args
> +        .endif
> +        .endm
> +
> +        .macro mb2ht_init type, req, args:vararg
> +        .align MULTIBOOT2_TAG_ALIGN
> +        0:

Labels belong in the first column. Also I generally recommend against
using numeric labels in macros (due to the risk of conflict with label
uses around the macro use sites) - use \@ instead to make labels
unique (again provided the oldest supported binutils handle this).

> @@ -119,10 +213,11 @@ __start:
>  
>          /* Save the Multiboot info struct (after relocation) for later use. */
>          mov     $sym_phys(cpu0_stack)+1024,%esp
> +        push    %eax                /* Multiboot magic. */
>          push    %ebx                /* Multiboot information address. */
>          push    %ecx                /* Boot trampoline address. */
>          call    reloc
> -        add     $8,%esp             /* Remove reloc() args from stack. */
> +        add     $12,%esp            /* Remove reloc() args from stack. */

The latest now it becomes clear that the arguments passed are
kind of backwards: One would expect the qualifying value (i.e.
the magic) to come first, then the info pointer, and last the
trampoline address.

> @@ -86,12 +104,12 @@ static u32 copy_string(u32 src)
>      return copy_mem(src, p - src + 1);
>  }
>  
> -multiboot_info_t *reloc(u32 mbi_in)
> +static multiboot_info_t *mbi_mbi(void *mbi_in)

I don't see why this needs to become a pointer all of the sudden, ...

>  {
>      int i;
>      multiboot_info_t *mbi_out;
>  
> -    mbi_out = (multiboot_info_t *)copy_mem(mbi_in, sizeof(*mbi_out));
> +    mbi_out = (multiboot_info_t *)copy_mem((u32)mbi_in, sizeof(*mbi_out));

... adding back a cast that we'd better avoid here.

> @@ -127,3 +145,123 @@ multiboot_info_t *reloc(u32 mbi_in)
>  
>      return mbi_out;
>  }
> +
> +static multiboot_info_t *mbi2_mbi(void *mbi_in)

Why not "const multiboot2_fixed_t *"?

> +{
> +    /* Do not complain that mbi_out_mods is not initialized. */
> +    module_t *mbi_out_mods = (module_t *)0;
> +    memory_map_t *mmap_dst;
> +    multiboot2_memory_map_t *mmap_src;

const

> +    multiboot2_tag_t *tag;

const?

> +    multiboot_info_t *mbi_out;
> +    u32 ptr;
> +    unsigned int i, mod_idx = 0;
> +
> +    mbi_out = (multiboot_info_t *)alloc_mem(sizeof(*mbi_out));
> +    zero_mem((u32)mbi_out, sizeof(*mbi_out));

Please avoid one of the two casts (e.g. by using "ptr" as intermediate
storage for the alloc_mem() result.

> +
> +    /* Skip Multiboot2 information fixed part. */
> +    tag = mbi_in + sizeof(multiboot2_fixed_t);
> +
> +    for ( ; ; )
> +    {
> +        if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE )
> +            ++mbi_out->mods_count;
> +        else if ( tag->type == MULTIBOOT2_TAG_TYPE_END )
> +        {
> +            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;

Even if we don't expect Xen to be booted without any modules,
wouldn't you better do this only when mbi_out->mods_count > 0?

> +            break;
> +        }
> +
> +        /* Go to next Multiboot2 information tag. */
> +        tag = (multiboot2_tag_t *)(ALIGN_UP((u32)tag + tag->size, MULTIBOOT2_TAG_ALIGN));

Please drop the stray pair of parentheses around the invocation of
ALIGN_UP(). Also - long line.

> +        /* Check Multiboot2 information total size just in case. */
> +	if ( (void *)tag - mbi_in >= ((multiboot2_fixed_t *)mbi_in)->total_size )

Hard tab.

> +            break;
> +    }
> +
> +    /* Skip Multiboot2 information fixed part. */
> +    tag = mbi_in + sizeof(multiboot2_fixed_t);
> +
> +    for ( ; ; )
> +    {
> +        switch ( tag->type )
> +        {
> +        case MULTIBOOT2_TAG_TYPE_BOOT_LOADER_NAME:
> +            mbi_out->flags |= MBI_LOADERNAME;
> +            ptr = (u32)get_mb2_data(tag, multiboot2_tag_string_t, string);

Please consider adding a get_mb2_string() wrapping get_mb2_data()
but eliminating the recurring casts from all use sites.

> +            mbi_out->boot_loader_name = copy_string(ptr);
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_CMDLINE:
> +            mbi_out->flags |= MBI_CMDLINE;
> +            ptr = (u32)get_mb2_data(tag, multiboot2_tag_string_t, 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, multiboot2_tag_basic_meminfo_t, mem_lower);
> +            mbi_out->mem_upper = get_mb2_data(tag, multiboot2_tag_basic_meminfo_t, mem_upper);
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_MMAP:
> +            mbi_out->flags |= MBI_MEMMAP;
> +            mbi_out->mmap_length = get_mb2_data(tag, multiboot2_tag_mmap_t, size);
> +            mbi_out->mmap_length -= sizeof(multiboot2_tag_mmap_t);
> +            mbi_out->mmap_length += sizeof(((multiboot2_tag_mmap_t){0}).entries);

Afaict this sizeof() evaluates to zero. And even if it didn't I can't see
what the line is good for.

> +            mbi_out->mmap_length /= get_mb2_data(tag, multiboot2_tag_mmap_t, 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, multiboot2_tag_mmap_t, entries);
> +            mmap_dst = (memory_map_t *)mbi_out->mmap_addr;
> +
> +            for ( i = 0; i < mbi_out->mmap_length / sizeof(memory_map_t); ++i )
> +            {
> +                mmap_dst[i].size = sizeof(memory_map_t);
> +                mmap_dst[i].size -= sizeof(((memory_map_t){0}).size);
> +                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);
> +                mmap_dst[i].type = mmap_src[i].type;
> +            }

So in the calculations ahead of the loop you're careful to honor
entry_size. Why do you ignore it in the loop (assuming entry_size ==
sizeof(multiboot2_memory_map_t))?

> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_MODULE:
> +            mbi_out_mods[mod_idx].mod_start = get_mb2_data(tag, multiboot2_tag_module_t, mod_start);
> +            mbi_out_mods[mod_idx].mod_end = get_mb2_data(tag, multiboot2_tag_module_t, mod_end);

Long lines. Also by now the redundancy of the multiboot2_tag_ prefix
and the _t suffix in each of the invocations tells us that it would be
quite nice if attaching them to the actually relevant part of the name
would better be done inside the macro.

> +            ptr = (u32)get_mb2_data(tag, multiboot2_tag_module_t, cmdline);
> +            mbi_out_mods[mod_idx].string = copy_string(ptr);
> +            mbi_out_mods[mod_idx].reserved = 0;
> +            ++mod_idx;
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_END:
> +            return mbi_out;
> +
> +        default:
> +            break;
> +        }
> +
> +        /* Go to next Multiboot2 information tag. */
> +        tag = (multiboot2_tag_t *)(ALIGN_UP((u32)tag + tag->size, MULTIBOOT2_TAG_ALIGN));
> +
> +        /* Check Multiboot2 information total size just in case. */
> +	if ( (void *)tag - mbi_in >= ((multiboot2_fixed_t *)mbi_in)->total_size )

Hard tab again.

> +static multiboot_info_t __attribute__((__used__)) *reloc(void *mbi_in, u32 mb_magic)

Dropping the "static" would permit to also drop the "used" attribute.
Since it was that way before, why didn't you keep it that way?

Jan