[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/5] xen/x86: Introduce MultiBoot Data (MBD) type
On 24/09/14 18:19, Daniel Kiper wrote: > Introduce MultiBoot Data (MBD) type. It is used to define variable > which carry over data from multiboot protocol (any version) through > Xen preloader stage. Later all or parts of this data is used > to initialize boot_info structure. boot_info is introduced > in later patches. > > MBD helps to break multiboot (v1) protocol dependency. Using it > we are able to save space on trampoline (we do not allocate space > for unused data what happens in current preloader implementation). > Additionally, we are able to easily add new members to MBD if we > want support for new features or protocols. > > There is not plan to share MBD among architectures. It will be > nice if boot_info will be shared among architectures. Please > check later patches for more details. > > Code found in xen/arch/x86/boot_info.c moves MBD data to mbi struct > which is referenced from main Xen code. This is temporary solution > which helps to split patches into logical parts. Later it will be > replaced by final version of boot_info support. > > Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx> Reviewing changes to ASM code is *very* hard, and this patch is still very complicated. Can I suggest patches split along the following lines. * Shuffling of registers in head.S to end up with %eax and %ecx as indended for reloc.c, and %ebx currently unclobbered, and adjusting reloc.c for %ecx. Review for this is a simple case of verifying that the changes are just a strict shuffling of registers. * Whitespace and misc non-MBD cleanup to reloc.c * Altering existing helper functions in preparation for MBD support * Actually introduce mbd_t and use it. In addition, I have some specific comments about the changes themselves. > --- > v2 - suggestions/fixes: > - improve inline assembly > (suggested by Andrew Cooper and Jan Beulich), > - use __used attribute > (suggested by Andrew Cooper), > - patch split rearrangement > (suggested by Andrew Cooper and Jan Beulich). > --- > xen/arch/x86/Makefile | 1 + > xen/arch/x86/boot/cmdline.S | 9 +-- > xen/arch/x86/boot/head.S | 31 ++++---- > xen/arch/x86/boot/reloc.c | 147 > +++++++++++++++++++++++++------------ > xen/arch/x86/boot/x86_64.S | 10 ++- > xen/arch/x86/boot_info.c | 58 +++++++++++++++ > xen/arch/x86/setup.c | 41 +++++++---- > xen/arch/x86/x86_64/asm-offsets.c | 5 +- > xen/include/asm-x86/mbd.h | 70 ++++++++++++++++++ > 9 files changed, 284 insertions(+), 88 deletions(-) > create mode 100644 xen/arch/x86/boot_info.c > create mode 100644 xen/include/asm-x86/mbd.h > > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile > index c1e244d..c465bb3 100644 > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -42,6 +42,7 @@ obj-y += numa.o > obj-y += pci.o > obj-y += percpu.o > obj-y += physdev.o > +obj-y += boot_info.o > obj-y += setup.o > obj-y += shutdown.o > obj-y += smp.o > diff --git a/xen/arch/x86/boot/cmdline.S b/xen/arch/x86/boot/cmdline.S > index 00687eb..7316011 100644 > --- a/xen/arch/x86/boot/cmdline.S > +++ b/xen/arch/x86/boot/cmdline.S > @@ -152,17 +152,14 @@ cmdline_parse_early: > pusha > > /* Bail if there is no command line to parse. */ > - mov sym_phys(multiboot_ptr),%ebx > - mov MB_flags(%ebx),%eax > - test $4,%al > - jz .Lcmdline_exit > - mov MB_cmdline(%ebx),%eax > + mov sym_phys(mbd_ptr),%ebx As you are already changing the name of this symbol, please call it mbd_pa or something to indicate that it genuinely is a physical address, not a pointer in the C sense of the term. > + mov MBD_cmdline(%ebx),%eax > test %eax,%eax > jz .Lcmdline_exit > > /* Check for 'no-real-mode' command-line option. */ > pushl $sym_phys(.Lno_rm_opt) > - pushl MB_cmdline(%ebx) > + pushl MBD_cmdline(%ebx) > call .Lfind_option > test %eax,%eax > setnz %al > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > index 10ecf51..79bce3c 100644 > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -86,14 +86,14 @@ __start: > jne not_multiboot > > /* Set up trampoline segment 64k below EBDA */ > - movzwl 0x40e,%eax /* EBDA segment */ > - cmp $0xa000,%eax /* sanity check (high) */ > + movzwl 0x40e,%ecx /* EBDA segment */ > + cmp $0xa000,%ecx /* sanity check (high) */ > jae 0f > - cmp $0x4000,%eax /* sanity check (low) */ > + cmp $0x4000,%ecx /* sanity check (low) */ > jae 1f > 0: > - movzwl 0x413,%eax /* use base memory size on failure */ > - shl $10-4,%eax > + movzwl 0x413,%ecx /* use base memory size on failure */ > + shl $10-4,%ecx > 1: > /* > * Compare the value in the BDA with the information from the > @@ -105,22 +105,23 @@ __start: > cmp $0x100,%edx /* is the multiboot value too small? */ > jb 2f /* if so, do not use it */ > shl $10-4,%edx > - cmp %eax,%edx /* compare with BDA value */ > - cmovb %edx,%eax /* and use the smaller */ > + cmp %ecx,%edx /* compare with BDA value */ > + cmovb %edx,%ecx /* and use the smaller */ > > 2: /* Reserve 64kb for the trampoline */ > - sub $0x1000,%eax > + sub $0x1000,%ecx > > /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */ > - xor %al, %al > - shl $4, %eax > - mov %eax,sym_phys(trampoline_phys) > + xor %cl, %cl > + shl $4, %ecx > + mov %ecx,sym_phys(trampoline_phys) > > - /* Save the Multiboot info struct (after relocation) for later use. > */ > + /* Save the Multiboot data (after relocation) for later use. */ > mov $sym_phys(cpu0_stack)+1024,%esp > - push %ebx > - call reloc > - mov %eax,sym_phys(multiboot_ptr) > + push %eax /* Multiboot magic */ > + push %ebx /* Multiboot information address */ > + call reloc /* %ecx contains trampoline address */ > + mov %eax,sym_phys(mbd_ptr) > > /* Initialize BSS (no nasty surprises!) */ > mov $sym_phys(__bss_start),%edi > diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c > index fa0fb6b..35a991e 100644 > --- a/xen/arch/x86/boot/reloc.c > +++ b/xen/arch/x86/boot/reloc.c > @@ -1,40 +1,58 @@ > -/****************************************************************************** > +/* > * reloc.c > - * > + * > * 32-bit flat memory-map routines for relocating Multiboot structures > * and modules. This is most easily done early with paging disabled. > - * > + * > * Copyright (c) 2009, Citrix Systems, Inc. > - * > + * Copyright (c) 2013, 2014 Oracle Co., Daniel Kiper > + * > * Authors: > * Keir Fraser <keir@xxxxxxx> > + * Daniel Kiper > */ > > -/* entered with %eax = BOOT_TRAMPOLINE */ > +typedef unsigned char u8; > +typedef unsigned short u16; > +typedef unsigned long u32; > +typedef unsigned long long u64; > + > +#include "../../../include/xen/compiler.h" > +#include "../../../include/xen/multiboot.h" > + > +#include "../../../include/asm/mbd.h" How about playing with -I for this file in the Makefile to allow #include <xen/compiler.h> and so ? > + > +/* > + * __HERE__ IS ENTRY POINT!!! I am still of the firm opinion that anyone capable of editing this file ought to know understand the _start symbol, making this comment useless. > + * > + * It is entered from xen/arch/x86/boot/head.S with: > + * - %eax = MULTIBOOT_MAGIC, > + * - %ebx = MULTIBOOT_INFORMATION_ADDRESS, > + * - %ecx = BOOT_TRAMPOLINE. > + */ > asm ( > " .text \n" > " .globl _start \n" > "_start: \n" > " call 1f \n" > "1: pop %ebx \n" > - " mov %eax,alloc-1b(%ebx) \n" > - " jmp reloc \n" > + " mov %ecx,alloc-1b(%ebx) \n" > + " jmp __reloc \n" > ); > > -/* This is our data. Because the code must be relocatable, no BSS is > - * allowed. All data is accessed PC-relative with inline assembly. > +/* > + * This is our data. Because the code must be relocatable, no BSS is > + * allowed. All data is accessed PC-relative with inline assembly. > */ > asm ( > "alloc: \n" > " .long 0 \n" > ); > > -typedef unsigned int u32; > -#include "../../../include/xen/multiboot.h" > - > -static void *reloc_mbi_struct(void *old, unsigned int bytes) > +static u32 alloc_struct(u32 bytes) > { > - void *new; > + u32 s; > + > asm( > " call 1f \n" > "1: pop %%edx \n" > @@ -42,58 +60,95 @@ static void *reloc_mbi_struct(void *old, unsigned int > bytes) > " sub %1,%0 \n" > " and $~15,%0 \n" > " mov %0,alloc-1b(%%edx) \n" > - " mov %0,%%edi \n" > + : "=&r" (s) : "r" (bytes) : "edx"); > + > + return s; > +} > + > +static void zero_struct(u32 s, u32 bytes) > +{ > + asm volatile( > + " rep stosb \n" > + : "+D" (s), "+c" (bytes) : "a" (0)); > +} > + > +static u32 copy_struct(u32 src, u32 bytes) > +{ > + u32 dst, dst_asm; > + > + dst = alloc_struct(bytes); > + dst_asm = dst; > + > + asm volatile( > " rep movsb \n" > - : "=&r" (new), "+c" (bytes), "+S" (old) > - : : "edx", "edi"); > - return new; > + : "+S" (src), "+D" (dst_asm), "+c" (bytes)); > + > + return dst; > } > > -static char *reloc_mbi_string(char *old) > +static u32 copy_string(u32 src) > { > char *p; > - for ( p = old; *p != '\0'; p++ ) > + > + if ( src == 0 ) > + return 0; > + > + for ( p = (char *)src; *p != '\0'; p++ ) > continue; > - return reloc_mbi_struct(old, p - old + 1); > + > + return copy_struct(src, p - (char *)src + 1); > } > > -multiboot_info_t *reloc(multiboot_info_t *mbi_old) > +static mbd_t *mb_mbd(mbd_t *mbd, multiboot_info_t *mbi) > { > - multiboot_info_t *mbi = reloc_mbi_struct(mbi_old, sizeof(*mbi)); > - int i; > + boot_module_t *mbd_mods; > + module_t *mbi_mods; > + u32 i; > + > + if ( mbi->flags & MBI_LOADERNAME ) > + mbd->boot_loader_name = copy_string(mbi->boot_loader_name); > > if ( mbi->flags & MBI_CMDLINE ) > - mbi->cmdline = (u32)reloc_mbi_string((char *)mbi->cmdline); > + mbd->cmdline = copy_string(mbi->cmdline); > + > + if ( mbi->flags & MBI_MEMLIMITS ) > + { > + mbd->mem_lower = mbi->mem_lower; > + mbd->mem_upper = mbi->mem_upper; > + } > + > + if ( mbi->flags & MBI_MEMMAP ) > + { > + mbd->mmap_size = mbi->mmap_length; > + mbd->mmap = copy_struct(mbi->mmap_addr, mbi->mmap_length); > + } > > if ( mbi->flags & MBI_MODULES ) > { > - module_t *mods = reloc_mbi_struct( > - (module_t *)mbi->mods_addr, mbi->mods_count * sizeof(module_t)); > + mbd->mods_nr = mbi->mods_count; > + mbd->mods = alloc_struct(mbi->mods_count * sizeof(boot_module_t)); > > - mbi->mods_addr = (u32)mods; > + mbi_mods = (module_t *)mbi->mods_addr; > + mbd_mods = (boot_module_t *)mbd->mods; > > for ( i = 0; i < mbi->mods_count; i++ ) > { > - if ( mods[i].string ) > - mods[i].string = (u32)reloc_mbi_string((char > *)mods[i].string); > + mbd_mods[i].start = mbi_mods[i].mod_start; > + mbd_mods[i].end = mbi_mods[i].mod_end; > + mbd_mods[i].cmdline = copy_string(mbi_mods[i].string); > + mbd_mods[i].relocated = 0; > } > } > > - if ( mbi->flags & MBI_MEMMAP ) > - mbi->mmap_addr = (u32)reloc_mbi_struct( > - (memory_map_t *)mbi->mmap_addr, mbi->mmap_length); > + return mbd; > +} > > - if ( mbi->flags & MBI_LOADERNAME ) > - mbi->boot_loader_name = (u32)reloc_mbi_string( > - (char *)mbi->boot_loader_name); > - > - /* Mask features we don't understand or don't relocate. */ > - mbi->flags &= (MBI_MEMLIMITS | > - MBI_BOOTDEV | > - MBI_CMDLINE | > - MBI_MODULES | > - MBI_MEMMAP | > - MBI_LOADERNAME); > - > - return mbi; > +static mbd_t __used *__reloc(void *mbi, u32 mb_magic) > +{ > + mbd_t *mbd; > + > + mbd = (mbd_t *)alloc_struct(sizeof(mbd_t)); > + zero_struct((u32)mbd, sizeof(mbd_t)); > + > + return mb_mbd(mbd, mbi); > } > diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S > index bfbafd2..34243b1 100644 > --- a/xen/arch/x86/boot/x86_64.S > +++ b/xen/arch/x86/boot/x86_64.S > @@ -29,8 +29,12 @@ > test %ebx,%ebx > jnz start_secondary > > - /* Pass off the Multiboot info structure to C land. */ > - mov multiboot_ptr(%rip),%edi > + /* Init boot_info. */ > + mov mbd_ptr(%rip),%edi > + call __init_boot_info > + > + /* Pass off the boot_info to C land. */ > + movq %rax,%rdi > call __start_xen > ud2 /* Force a panic (invalid opcode). */ > > @@ -38,7 +42,7 @@ > > .data > .align 8 > -multiboot_ptr: > +mbd_ptr: > .long 0 > > .word 0 > diff --git a/xen/arch/x86/boot_info.c b/xen/arch/x86/boot_info.c > new file mode 100644 > index 0000000..70830c2 > --- /dev/null > +++ b/xen/arch/x86/boot_info.c > @@ -0,0 +1,58 @@ > +/* > + * Copyright (c) 2013, 2014 Oracle Co., Daniel Kiper > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <xen/types.h> > +#include <xen/init.h> > +#include <xen/multiboot.h> > + > +#include <asm/mbd.h> > +#include <asm/page.h> > + > +static multiboot_info_t mbi = {}; "= {}" is redundant for a static declaration. > + > +extern void enable_exception_support(void); > + > +unsigned long __init __init_boot_info(u32 mbd_ptr) > +{ > + mbd_t *mbd = __va(mbd_ptr); > + > + enable_exception_support(); > + > + if ( mbd->boot_loader_name ) { > + mbi.flags = MBI_LOADERNAME; > + mbi.boot_loader_name = mbd->boot_loader_name; > + } > + > + if ( mbd->cmdline ) { > + mbi.flags |= MBI_CMDLINE; > + mbi.cmdline = mbd->cmdline; > + } > + > + mbi.flags |= MBI_MEMLIMITS; > + mbi.mem_lower = mbd->mem_lower; > + mbi.mem_upper = mbd->mem_upper; > + > + mbi.flags |= MBI_MEMMAP; > + mbi.mmap_length = mbd->mmap_size; > + mbi.mmap_addr = mbd->mmap; > + > + mbi.flags |= MBI_MODULES; > + mbi.mods_count = mbd->mods_nr; > + mbi.mods_addr = mbd->mods; > + > + return __pa(&mbi); > +} > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index 6a814cd..9391efa 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -539,6 +539,25 @@ static char * __init cmdline_cook(char *p, char > *loader_name) > return p; > } > > +void __init enable_exception_support(void) > +{ > + /* Critical region without IDT or TSS. Any fault is deadly! */ > + > + set_processor_id(0); > + set_current((struct vcpu *)0xfffff000); /* debug sanity. */ > + idle_vcpu[0] = current; > + > + percpu_init_areas(); > + > + init_idt_traps(); > + load_system_tables(); > + > + smp_prepare_boot_cpu(); > + sort_exception_tables(); > + > + /* Full exception support from here on in. */ > +} > + > void __init noreturn __start_xen(unsigned long mbi_p) > { > char *memmap_type = NULL; > @@ -556,21 +575,13 @@ void __init noreturn __start_xen(unsigned long mbi_p) > .stop_bits = 1 > }; > > - /* Critical region without IDT or TSS. Any fault is deadly! */ > - > - set_processor_id(0); > - set_current((struct vcpu *)0xfffff000); /* debug sanity. */ > - idle_vcpu[0] = current; > - > - percpu_init_areas(); > - > - init_idt_traps(); > - load_system_tables(); > - > - smp_prepare_boot_cpu(); > - sort_exception_tables(); > - > - /* Full exception support from here on in. */ > + if ( !efi_enabled ) { > + /* Exception support was enabled before __start_xen() call. */ > + } > + else > + { > + enable_exception_support(); > + } For this, absolutely not. Pass mbi_pa into __start_xen() and have __start_xen() call __init_boot_info() after it has set up exception support. None of the contents of MBD are needed beforehand. > > loader = (mbi->flags & MBI_LOADERNAME) > ? (char *)__va(mbi->boot_loader_name) : "unknown"; > diff --git a/xen/arch/x86/x86_64/asm-offsets.c > b/xen/arch/x86/x86_64/asm-offsets.c > index 3994f4d..2123eb3 100644 > --- a/xen/arch/x86/x86_64/asm-offsets.c > +++ b/xen/arch/x86/x86_64/asm-offsets.c > @@ -12,7 +12,7 @@ > #include <compat/xen.h> > #include <asm/fixmap.h> > #include <asm/hardirq.h> > -#include <xen/multiboot.h> > +#include <asm/mbd.h> > > #define DEFINE(_sym, _val) \ > asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \ > @@ -163,6 +163,5 @@ void __dummy__(void) > OFFSET(CPUINFO_features, struct cpuinfo_x86, x86_capability); > BLANK(); > > - OFFSET(MB_flags, multiboot_info_t, flags); > - OFFSET(MB_cmdline, multiboot_info_t, cmdline); > + OFFSET(MBD_cmdline, mbd_t, cmdline); > } > diff --git a/xen/include/asm-x86/mbd.h b/xen/include/asm-x86/mbd.h > new file mode 100644 > index 0000000..96e3044 > --- /dev/null > +++ b/xen/include/asm-x86/mbd.h > @@ -0,0 +1,70 @@ > +/* > + * Copyright (c) 2013, 2014 Oracle Co., Daniel Kiper > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#ifndef __MBD_H__ > +#define __MBD_H__ #include <xen/types.h> Do not rely on the translation using including this file to make u32 available for you. > + > +/* Module type. */ > +typedef struct { > + u32 start; > + u32 end; > + > + /* Pointer to a module command line. */ > + u32 cmdline; > + > + /* If relocated != 0 then a given module was relocated. */ > + u32 relocated; > +} boot_module_t; > + > +/* > + * MultiBoot Data (MBD) type. It is used to define variable which > + * carry over data from multiboot protocol (any version) through > + * Xen preloader stage. Later all or parts of this data is used > + * to initialize boot_info structure. > + */ > +typedef struct { > + /* Pointer to boot loader name. */ > + u32 boot_loader_name; > + > + /* Pointer to Xen command line. */ > + u32 cmdline; These need to be very clear about being physical addresses rather than pointers. ~Andrew > + > + /* > + * Amount of lower memory (in KiB) accordingly to The Multiboot > + * Specification version 0.6.96. > + */ > + u32 mem_lower; > + > + /* > + * Amount of upper memory (in KiB) accordingly to The Multiboot > + * Specification version 0.6.96. > + */ > + u32 mem_upper; > + > + /* Size (in bytes) of memory map provided by bootloader. */ > + u32 mmap_size; > + > + /* Pointer to memory map provided by bootloader. */ > + u32 mmap; > + > + /* Number of modules. */ > + u32 mods_nr; > + > + /* Pointer to modules description (boot_module_t *). */ > + u32 mods; > +} mbd_t; > +#endif /* __MBD_H__ */ _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |