|
[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 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".
> +
> + /*
> + * 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,
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.
> + .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).
> + .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.
> +
> +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.
> --- 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.
> @@ -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.
> + 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()?
> +
> + while ( 1 )
To avoid "condition is constant warnings" on certain compilers, I'd
recommend using for ( ; ; ) instead of 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)
Coding style.
> + {
> + 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?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |