Re: [PATCH 04/18] xen/x86: add multiboot2 protocol support

[Jan Beulich]

>>> On 30.01.15 at 18:54, <address@hidden> wrote:
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,6 +1,7 @@
>  obj-bin-y += head.o
>  
> -RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h 
> $(BASEDIR)/include/xen/multiboot.h
> +RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h 
> $(BASEDIR)/include/xen/compiler.h \
> +          $(BASEDIR)/include/xen/multiboot.h 
> $(BASEDIR)/include/xen/multiboot2.h

Perhaps we should rather have the compiler generate the
dependencies for us when compiling reloc.o?

> @@ -32,6 +33,59 @@ ENTRY(start)
>          .long   MULTIBOOT_HEADER_FLAGS
>          /* Checksum: must be the negated sum of the first two fields. */
>          .long   -(MULTIBOOT_HEADER_MAGIC + MULTIBOOT_HEADER_FLAGS)
> + 
> +/*** MULTIBOOT2 HEADER ****/
> +/* Some ideas are taken from grub-2.00/grub-core/tests/boot/kernel-i386.S 
> file. */
> +        .align  MULTIBOOT2_HEADER_ALIGN
> +
> +.Lmultiboot2_header:
> +        /* Magic number indicating a Multiboot2 header. */
> +        .long   MULTIBOOT2_HEADER_MAGIC
> +        /* Architecture: i386. */
> +        .long   MULTIBOOT2_ARCHITECTURE_I386
> +        /* Multiboot2 header length. */
> +        .long   .Lmultiboot2_header_end - .Lmultiboot2_header
> +        /* Multiboot2 header checksum. */
> +        .long   -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 + \
> +                        (.Lmultiboot2_header_end - .Lmultiboot2_header))

So here ...

> +        /* Multiboot2 tags... */
> +.Lmultiboot2_info_req:
> +        /* Multiboot2 information request tag. */
> +        .short  MULTIBOOT2_HEADER_TAG_INFORMATION_REQUEST
> +        .short  MULTIBOOT2_HEADER_TAG_REQUIRED
> +        .long   .Lmultiboot2_info_req_end - .Lmultiboot2_info_req

.. and here you properly calculate the length, while ...

> +        .long   MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO
> +        .long   MULTIBOOT2_TAG_TYPE_MMAP
> +.Lmultiboot2_info_req_end:
> +
> +        .align  MULTIBOOT2_TAG_ALIGN
> +        .short  MULTIBOOT2_HEADER_TAG_MODULE_ALIGN
> +        .short  MULTIBOOT2_HEADER_TAG_REQUIRED
> +        .long   8 /* Tag size. */

... here and further down you hard-code it. Please be consistent
(and if you go the calculation route, perhaps introduce some
redundancy reducing macro).

> +
> +        /* Console flags tag. */
> +        .align  MULTIBOOT2_TAG_ALIGN
> +        .short  MULTIBOOT2_HEADER_TAG_CONSOLE_FLAGS
> +        .short  MULTIBOOT2_HEADER_TAG_OPTIONAL
> +        .long   12 /* Tag size. */
> +        .long   MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED
> +
> +        /* Framebuffer tag. */
> +        .align  MULTIBOOT2_TAG_ALIGN
> +        .short  MULTIBOOT2_HEADER_TAG_FRAMEBUFFER
> +        .short  MULTIBOOT2_HEADER_TAG_OPTIONAL
> +        .long   20 /* Tag size. */
> +        .long   0 /* Number of the columns - no preference. */
> +        .long   0 /* Number of the lines - no preference. */
> +        .long   0 /* Number of bits per pixel - no preference. */
> +
> +        /* Multiboot2 header end tag. */
> +        .align  MULTIBOOT2_TAG_ALIGN
> +        .short  MULTIBOOT2_HEADER_TAG_END
> +        .short  0
> +        .long   8 /* Tag size. */
> +.Lmultiboot2_header_end:
>  
>          .section .init.rodata, "a", @progbits
>          .align 4
> @@ -81,10 +135,51 @@ __start:
>          mov     %ecx,%es
>          mov     %ecx,%ss
>  
> +        /* Bootloaders may set multiboot[12].mem_lower to a nonzero value */

Above you meet coding style requirements, but here you stop to do
so (ongoing below)? Even if pre-existing neighboring comments aren't
well formed, please don't make the situation worse.

Also please don't say 12 here - I first even mistook this as an array
index, but you seem to mean 1 or 2. Please use {1,2} instead.

> +        xor     %edx,%edx
> +
>          /* Check for Multiboot bootloader */
>          cmp     $MULTIBOOT_BOOTLOADER_MAGIC,%eax
> -        jne     not_multiboot
> +        je      multiboot1_proto
>  
> +        /* Check for Multiboot2 bootloader */
> +        cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> +        je      multiboot2_proto
> +
> +        jmp     not_multiboot
> +
> +multiboot1_proto:
> +        /* Get mem_lower from Multiboot information */
> +        testb   $MBI_MEMLIMITS,(%ebx)

MB_flags(%ebx)

> +        jz      trampoline_setup    /* not available? BDA value will be fine 
> */
> +
> +        mov     MB_mem_lower(%ebx),%edx

Please use "cmovnz" or "or" instead of "jz" and "mov".

> +        jmp     trampoline_setup
> +
> +multiboot2_proto:
> +        /* Skip Multiboot2 information fixed part */
> +        lea     MB2_fixed_sizeof(%ebx),%ecx
> +
> +0:
> +        /* Get mem_lower from Multiboot2 information */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx)
> +        jne     1f
> +
> +        mov     MB2_mem_lower(%ecx),%edx
> +        jmp     trampoline_setup
> +
> +1:
> +        /* Is it the end of Multiboot2 information? */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_END,(%ecx)

This lacks the use a suitable MB2_* definition (even if that ends up
being zero).

> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -5,19 +5,26 @@
>   * and modules. This is most easily done early with paging disabled.
>   *
>   * Copyright (c) 2009, Citrix Systems, Inc.
> + * Copyright (c) 2013, 2014, 2015 Oracle Corp.
>   *
>   * Authors:
>   *    Keir Fraser <address@hidden>
> + *    Daniel Kiper

If you add yourself here, then please (following the existing example)
with email address.

> -/* entered with %eax = BOOT_TRAMPOLINE */
> +/*
> + * This entry point is entered from xen/arch/x86/boot/head.S with:
> + *   - %eax = MULTIBOOT_MAGIC,
> + *   - %ebx = MULTIBOOT_INFORMATION_ADDRESS,
> + *   - %ecx = BOOT_TRAMPOLINE.

Then why do you push %eax and %ebx above?

> @@ -31,7 +38,16 @@ asm (
>      );
>  
>  typedef unsigned int u32;
> +typedef unsigned long long u64;
> +
> +#include "../../../include/xen/compiler.h"

Why?

> @@ -74,40 +95,153 @@ static u32 copy_string(u32 src)
>      return copy_struct(src, p - (char *)src + 1);
>  }
>  
> -multiboot_info_t *reloc(multiboot_info_t *mbi_old)
> +static multiboot_info_t *mbi_mbi(void *mbi_in)
>  {
> -    multiboot_info_t *mbi = (multiboot_info_t *)copy_struct((u32)mbi_old, 
> sizeof(*mbi));
>      int i;
> +    multiboot_info_t *mbi_out;
>  
> -    if ( mbi->flags & MBI_CMDLINE )
> -        mbi->cmdline = copy_string(mbi->cmdline);
> +    mbi_out = (multiboot_info_t *)copy_struct((u32)mbi_in, sizeof(*mbi_out));

Why can this not remain the initializer of the variable? Also the
unrelated renaming mbi -> mbi_out and mbi_old ->mbi_in only makes
reading the patch more cumbersome. If you absolutely feel like you
need to rename them, do this in a separate patch.

> +static multiboot_info_t *mbi2_mbi(void *mbi_in)
> +{
> +    module_t *mbi_out_mods;
> +    memory_map_t *mmap_dst;
> +    multiboot2_memory_map_t *mmap_src;
> +    multiboot2_tag_t *tag;
> +    multiboot_info_t *mbi_out;
> +    u32 ptr;
> +    unsigned int i, mod_idx = 0;
> +
> +    mbi_out = (multiboot_info_t *)alloc_struct(sizeof(*mbi_out));
> +    zero_struct((u32)mbi_out, sizeof(*mbi_out));
> +
> +    /* 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_struct(mbi_out->mods_count * 
> sizeof(module_t));
> +            mbi_out_mods = (module_t *)mbi_out->mods_addr;
> +            break;
> +        }
> +
> +        /* Go to next Multiboot2 information tag. */
> +        tag = (multiboot2_tag_t *)(ALIGN_UP((u32)tag + tag->size, 
> MULTIBOOT2_TAG_ALIGN));
> +    }

Shouldn't you also break out of the loop when mbi_in->total_size
gets exceeded?

> +#ifndef __ASSEMBLY__
> +typedef struct {
> +    u32 total_size;
> +    u32 reserved;
> +} multiboot2_fixed_t;
> +
> +typedef struct {
> +    u32 type;
> +    u32 size;
> +} multiboot2_tag_t;
> +
> +typedef struct {
> +    u32 type;
> +    u32 size;
> +    char string[0];
> +} multiboot2_tag_string_t;

Are these _tag_ parts of the name really needed?

> +
> +typedef struct {
> +    u32 type;
> +    u32 size;
> +    u32 mem_lower;
> +    u32 mem_upper;
> +} multiboot2_tag_basic_meminfo_t;
> +
> +typedef struct __packed {

Why __packed when all the others aren't?

Jan