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

[Daniel Kiper]

On Fri, Feb 20, 2015 at 04:06:26PM +0000, Jan Beulich wrote:
> >>> 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?

Hmmm... I am a bit confused. Here 
http://lists.xen.org/archives/html/xen-devel/2014-10/msg02094.html
you told a bit different thing and relevant patch was accepted as commit 
c42070df66c9fcedf477959b8371b85aa4ac4933
(x86/boot: fix reloc.S build dependencies). I can try to do what you suggest, 
this is not a problem
for me, however, I would like to be sure what is your final decision in that 
case.

> > @@ -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).

I did this deliberately. I calculate size using labels when it is really
needed, e.g. in tags which size depends on number of elements. However,
most tags have strictly defined sizes. So, that is why I dropped labels
and entered simple numbers. Though I agree that it can be improved.
I think that we can define relevant tag structures in multiboot2.h and
generate relevant constants using asm-offsets.c. Is it OK for you?

> > +        /* 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.

Do you ask about ending fullstops? Am I right?

> 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.

OK.

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

static multiboot_info_t __used *reloc(void *mbi_in, u32 mb_magic)
Because of this ------> ^^^^^^

> > @@ -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?

Tag MULTIBOOT2_TAG_TYPE_END is always added as the end marker
but we can check also mbi_in->total_size just in case.

> > +#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?

This is not a must but I tried to mimic things in GRUB2 as much as possible.

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

Ha! This thing was taken from GRUB2 source.
I was asking this question myself many times.
I have not found real explanation for this
but if you wish I can dive into code and
try to find one (if it exists).

Daniel