Re: [PATCH RFC 5/7] xen/x86: Add multiboot2 protocol support

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 09/08/14 00:04, Daniel Kiper wrote:
> Add multiboot2 protocol support. This way we are laying the foundation
> for EFI + GRUB2 + Xen development.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  xen/arch/x86/boot/head.S  |  112 ++++++++++++++++++++++++++++++++++++++++++++-
>  xen/arch/x86/boot/reloc.c |  103 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 212 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 79bce3c..f0ea6d0 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -1,5 +1,6 @@
>  #include <xen/config.h>
>  #include <xen/multiboot.h>
> +#include <xen/multiboot2.h>
>  #include <public/xen.h>
>  #include <asm/asm_defns.h>
>  #include <asm/desc.h>
> @@ -33,6 +34,68 @@ ENTRY(start)
>          /* 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
> +
> +multiboot2_header:
> +        /* Magic number indicating a Multiboot2 header. */
> +        .long   MULTIBOOT2_HEADER_MAGIC
> +        /* Architecture: i386. */
> +        .long   MULTIBOOT2_ARCHITECTURE_I386
> +        /* Multiboot2 header length. */
> +        .long   multiboot2_header_end - multiboot2_header
> +        /* Multiboot2 header checksum. */
> +        .long   -(MULTIBOOT2_HEADER_MAGIC + MULTIBOOT2_ARCHITECTURE_I386 + \
> +                        (multiboot2_header_end - multiboot2_header))
> +
> +        /* Multiboot2 tags... */
> +multiboot2_info_req:
> +        /* Multiboot2 information request tag. */
> +        .short  MULTIBOOT2_HEADER_TAG_INFORMATION_REQUEST
> +        .short  MULTIBOOT2_HEADER_TAG_REQUIRED
> +        .long   multiboot2_info_req_end - multiboot2_info_req
> +        .long   MULTIBOOT2_TAG_TYPE_MMAP
> +multiboot2_info_req_end:
> +
> +        /*
> +         * Align Xen image and modules at page boundry.
> +         *
> +         * .balignl MULTIBOOT2_TAG_ALIGN, MULTIBOOT2_TAG_TYPE_END is a hack
> +         * to avoid bug related to Multiboot2 information request tag in earlier
> +         * versions of GRUB2.
> +         *
> +         * DO NOT MOVE THIS TAG! ANY CHANGE HERE MAY BREAK COMPATIBILITY
> +         * WITH EARLIER GRUB2 VERSIONS!
> +         */
> +        .balignl MULTIBOOT2_TAG_ALIGN, MULTIBOOT2_TAG_TYPE_END
> +        .short   MULTIBOOT2_HEADER_TAG_MODULE_ALIGN
> +        .short   MULTIBOOT2_HEADER_TAG_REQUIRED
> +        .long    8 /* Tag size. */
> +
> +        /* 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. */
> +multiboot2_header_end:
> +
>          .section .init.rodata, "a", @progbits
>          .align 4
>  
> @@ -81,10 +144,55 @@ __start:
>          mov     %ecx,%es
>          mov     %ecx,%ss
>  
> +        /* Set mem_lower to 0 */
> +        xor     %edx,%edx
> +

How does this affect mem_lower? it doesn't appear relevant in context.

>          /* Check for Multiboot bootloader */
> -        cmp     $0x2BADB002,%eax
> -        jne     not_multiboot
> +        cmp     $MULTIBOOT_BOOTLOADER_MAGIC,%eax
> +        je      1f

This $MULTIBOOT_BOOTLOADER_MAGIC should probably be separate, as there
are other bits of code which should use multiboot manifest constants.

It looks as if this should be "je multiboot1_entry" ...

> +
> +        /* Check for Multiboot2 bootloader */
> +        cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> +        je      2f
> +
> +        jmp     not_multiboot
> +
> +1:
> +        /* Get mem_lower from Multiboot information */
> +        testb   $MBI_MEMLIMITS,(%ebx)
> +        jz      0f                  /* not available? BDA value will be fine */
>  
> +        mov     4(%ebx),%edx
> +        shl     $10-4,%edx
> +        jmp     0f

and these 0f's should be "setup_trampoline" or similar.  You have also
removed some sanity checks of the multiboot information, which need
reintroducing.

> +
> +2:

This 2 should be "multiboot2_entry"

> +        /* Get Multiboot2 information address */
> +        mov     %ebx,%ecx
> +        add     $8,%ecx

Any manifest constants for this, or at least a reference to some
documentation describing how the tags are laid out?

> +
> +3:
> +        /* Get mem_lower from Multiboot2 information */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx)
> +        jne     4f
> +
> +        mov     8(%ecx),%edx
> +        shl     $10-4,%edx
> +        jmp     5f

BASIC_MEMINFO is the only tag we look for.  No need to spin through all
of them if we found it.  Also, needs the same sanity checking as the
multiboot1 information.

~Andrew

> +
> +4:
> +        /* Is it the end of Multiboot2 information? */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_END,(%ecx)
> +        je      0f
> +
> +5:
> +        /* Go to next Multiboot2 information tag */
> +        add     4(%ecx),%ecx
> +        add     $(MULTIBOOT2_TAG_ALIGN-1),%ecx
> +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> +        jmp     3b
> +
> +0:
>          /* Set up trampoline segment 64k below EBDA */
>          movzwl  0x40e,%ecx          /* EBDA segment */
>          cmp     $0xa000,%ecx        /* sanity check (high) */
> diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> index 29f4887..eaf3226 100644
> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -18,8 +18,12 @@ typedef unsigned long u32;
>  typedef unsigned long long u64;
>  
>  #include "../../../include/xen/multiboot.h"
> +#include "../../../include/xen/multiboot2.h"
>  #include "../../../include/asm/mbd.h"
>  
> +#define ALIGN_UP(addr, align) \
> +                ((addr + (typeof(addr))align - 1) & ~((typeof(addr))align - 1))
> +
>  /*
>   * __HERE__ IS TRUE ENTRY POINT!!!
>   *
> @@ -144,6 +148,100 @@ static mbd_t *mb_mbd(mbd_t *mbd, multiboot_info_t *mbi)
>      return mbd;
>  }
>  
> +static mbd_t *mb2_mbd(mbd_t *mbd, void *mbi)
> +{
> +    int i, mod_idx = 0;
> +    memory_map_t *mmap_dst;
> +    multiboot2_memory_map_t *mmap_src;
> +    multiboot2_tag_t *tag;
> +    u32 ptr;
> +    boot_module_t *mbd_mods;
> +
> +    /* Skip Multiboot2 information fixed part. */
> +    tag = mbi + sizeof(u32) * 2;
> +
> +    while ( 1 )
> +    {
> +        if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE )
> +            ++mbd->mods_nr;
> +        else if ( tag->type == MULTIBOOT2_TAG_TYPE_END )
> +        {
> +            mbd->mods = alloc_struct(mbd->mods_nr * sizeof(boot_module_t));
> +            mbd_mods = (boot_module_t *)mbd->mods;
> +            break;
> +        }
> +
> +        /* Go to next Multiboot2 information tag. */
> +        tag = (multiboot2_tag_t *)(ALIGN_UP((u32)tag + tag->size, MULTIBOOT2_TAG_ALIGN));
> +    }
> +
> +    /* Skip Multiboot2 information fixed part. */
> +    tag = mbi + sizeof(u32) * 2;
> +
> +    while ( 1 )
> +    {
> +        switch ( tag->type )
> +        {
> +        case MULTIBOOT2_TAG_TYPE_BOOT_LOADER_NAME:
> +            ptr = (u32)((multiboot2_tag_string_t *)tag)->string;
> +            mbd->boot_loader_name = copy_string(ptr);
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_CMDLINE:
> +            ptr = (u32)((multiboot2_tag_string_t *)tag)->string;
> +            mbd->cmdline = copy_string(ptr);
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO:
> +            mbd->mem_lower = ((multiboot2_tag_basic_meminfo_t *)tag)->mem_lower;
> +            mbd->mem_upper = ((multiboot2_tag_basic_meminfo_t *)tag)->mem_upper;
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_MMAP:
> +            mbd->mmap_size = ((multiboot2_tag_mmap_t *)tag)->size;
> +            mbd->mmap_size -= sizeof(multiboot2_tag_mmap_t);
> +            mbd->mmap_size += sizeof(((multiboot2_tag_mmap_t){0}).entries);
> +            mbd->mmap_size /= ((multiboot2_tag_mmap_t *)tag)->entry_size;
> +            mbd->mmap_size *= sizeof(memory_map_t);
> +
> +            mbd->mmap = alloc_struct(mbd->mmap_size);
> +
> +            mmap_src = ((multiboot2_tag_mmap_t *)tag)->entries;
> +            mmap_dst = (memory_map_t *)mbd->mmap;
> +
> +            for (i = 0; i < mbd->mmap_size / 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;
> +            }
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_MODULE:
> +            mbd_mods[mod_idx].start = (u32)((multiboot2_tag_module_t *)tag)->mod_start;
> +            mbd_mods[mod_idx].end = (u32)((multiboot2_tag_module_t *)tag)->mod_end;
> +            ptr = (u32)((multiboot2_tag_module_t *)tag)->cmdline;
> +            mbd_mods[mod_idx].cmdline = copy_string(ptr);
> +            mbd_mods[mod_idx].relocated = 0;
> +            ++mod_idx;
> +            break;
> +
> +        case MULTIBOOT2_TAG_TYPE_END:
> +            return mbd;
> +
> +        default:
> +            break;
> +        }
> +
> +        /* Go to next Multiboot2 information tag. */
> +        tag = (multiboot2_tag_t *)(ALIGN_UP((u32)tag + tag->size, MULTIBOOT2_TAG_ALIGN));
> +    }
> +}
> +
>  /*
>   * __THIS__ IS NOT ENTRY POINT!!!
>   * PLEASE LOOK AT THE BEGINNING OF THIS FILE!!!
> @@ -158,5 +256,8 @@ mbd_t *__reloc(void *mbi, u32 mb_magic)
>      mbd = (mbd_t *)alloc_struct(sizeof(mbd_t));
>      zero_struct((u32)mbd, sizeof(mbd_t));
>  
> -    return mb_mbd(mbd, mbi);
> +    if ( mb_magic == MULTIBOOT_BOOTLOADER_MAGIC )
> +        return mb_mbd(mbd, mbi);
> +    else
> +        return mb2_mbd(mbd, mbi);
>  }