[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Mon, Aug 11, 2014 at 11:33:32AM +0100, Jan Beulich wrote:
> >>> On 09.08.14 at 01:04, <daniel.kiper@xxxxxxxxxx> wrote:
> > @@ -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:
>
> While I'm fine with this label, ...
>
> > +        /* 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:
>
> ... this and ...
>
> > +        /* 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:
>
> ... this are purely auxiliary and as such shouldn't end up in the
> symbol table. Please prefix such labels with ".L".

OK.

> > +
> > +        /*
> > +         * 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!
> > +         */
>
> Question is - since your ultimate goal of getting UEFI to work this way
> won't be achievable with older GrUB2 anyway, do we care at all? Also,

I think that it makes sense to have multiboot2 protocol implementation
working on every GRUB2 version. This way it will be quite easy to use
that same Xen image regardless of GRUB and multiboot protocol version.
Additionally, I discovered that above mentioned bug depends on 
multiboot2_info_req
contents which is very annoying and not very easy to discover. So, this
hack ensures that regardless of multiboot2_info_req contents Xen image
behaves always in the same way on every GRUB2 version (with or without bug).

> at least a reasonable hint towards the nature of the referenced bug
> should be added here, as in the end it's not too unlikely for there to
> be more than one bug in an area like this. I.e. future people reading
> this or working on the code should have a handle to decide whether
> the hack is still applicable without first having to guess which specific
> bug this is about.

OK.

> > +        .balignl MULTIBOOT2_TAG_ALIGN, MULTIBOOT2_TAG_TYPE_END
> > +        .short   MULTIBOOT2_HEADER_TAG_MODULE_ALIGN
> > +        .short   MULTIBOOT2_HEADER_TAG_REQUIRED
>
> Readability would certainly benefit if you macro-ized the tag
> generation, at least to avoid the many redundant
> MULTIBOOT2_HEADER_TAG_ prefixes (but perhaps also the
> alignment).

OK.

> > +        .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
> > +
> >          /* Check for Multiboot bootloader */
> > -        cmp     $0x2BADB002,%eax
> > -        jne     not_multiboot
> > +        cmp     $MULTIBOOT_BOOTLOADER_MAGIC,%eax
> > +        je      1f
> > +
> > +        /* 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
>
> This code isn't being moved here from elsewhere, but also isn't
> multiboot2 related - what's this about? If it's really needed for
> something, this should be in a separate patch imo.

Here we are reading data from multiboot[1] protocol. Label
below is start of multiboot2 protocol implementation. We
need rearrange a bit multiboot[1] protocol implementation
to have both protocols working.

> > +2:
> > +        /* Get Multiboot2 information address */
> > +        mov     %ebx,%ecx
> > +        add     $8,%ecx
> > +
> > +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
> > +
> > +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:
>
> Please consider giving some or all, but at the very least the last,
> labels descriptive names instead of just using numeric ones.

OK.

>
> > --- 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))
>
> Even if just used locally, please make sure such macros are properly
> parenthesized.

You mean?

> > @@ -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;
>
> unsigned for both afaict.

OK.

> > +    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;
>
> Is there no way to properly express this via e.g. an offsetof()?

I will check it.

> > +
> > +    while ( 1 )
>
> To avoid "condition is constant warnings" on certain compilers, I'd
> recommend using for ( ; ; ) instead of while ( 1 ).

OK.

> > +    {
> > +        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)
>
> Coding style.

You mean?

> > +            {
> > +                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;
>
> The massive amount of casts throughout the entire switch is clearly
> unfortunate - can you please try to do something about this?

I will try to fix it.

Daniel

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.