On Fri, Feb 20, 2015 at 04:06:26PM +0000, Jan Beulich wrote: > >>> On 30.01.15 at 18:54, <daniel.ki...@oracle.com> 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 <k...@xen.org> > > + * 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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel