|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 04/18] xen/x86: add multiboot2 protocol support
>>> On 30.01.15 at 18:54, <daniel.kiper@xxxxxxxxxx> 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?
> @@ -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).
> +
> + /* 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.
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.
> + 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 <keir@xxxxxxx>
> + * 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?
> @@ -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?
> +#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?
> +
> +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?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |