|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |