[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-xen-4.5 v3 04/16] x86: Introduce MultiBoot Data (MBD) type
On 08/10/14 18:52, 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> > --- > v3 - suggestions/fixes: > - rename some variables > (suggested by Andrew Cooper), > - remove unneeded initialization > (suggested by Andrew Cooper), > - improve comments > (suggested by Andrew Cooper), > - further patch split rearrangement > (suggested by Andrew Cooper and Jan Beulich). > > 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 | 2 +- > xen/arch/x86/boot/reloc.c | 69 +++++++++++++++++++++----------- > xen/arch/x86/boot/x86_64.S | 10 +++-- > xen/arch/x86/boot_info.c | 59 +++++++++++++++++++++++++++ > xen/arch/x86/setup.c | 41 ++++++++++++------- > xen/arch/x86/x86_64/asm-offsets.c | 5 +-- > xen/include/asm-x86/mbd.h | 79 > +++++++++++++++++++++++++++++++++++++ > 9 files changed, 225 insertions(+), 50 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 86ca5f8..8425e65 100644 > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -43,6 +43,7 @@ obj-y += pci.o > obj-y += percpu.o > obj-y += physdev.o > obj-y += psr.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..dd1a027 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_pa),%ebx > + mov MBD_cmdline_pa(%ebx),%eax What sets the value of MBD_cmdline_pa? You are blindly consuming it here without checks the flags for its presence, but I can't spot anything in this patch which ever sets it. > 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_pa(%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 cd43952..f1b872a 100644 > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -120,7 +120,7 @@ __start: > mov $sym_phys(cpu0_stack)+1024,%esp > push %ebx > call reloc > - mov %eax,sym_phys(multiboot_ptr) > + mov %eax,sym_phys(mbd_pa) > > /* 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 b678f67..59edb4d 100644 > --- a/xen/arch/x86/boot/reloc.c > +++ b/xen/arch/x86/boot/reloc.c > @@ -12,8 +12,11 @@ > > typedef unsigned int u32; > > +#include "../../../include/xen/compiler.h" > #include "../../../include/xen/multiboot.h" > > +#include "../../../include/asm/mbd.h" > + > /* entered with %eax = BOOT_TRAMPOLINE */ > asm ( > " .text \n" > @@ -22,7 +25,7 @@ asm ( > " call 1f \n" > "1: pop %ebx \n" > " mov %eax,alloc-1b(%ebx) \n" > - " jmp reloc \n" > + " jmp __reloc \n" Why do you need to change the name of the reloc() function? reloc() seems like a fine name. > ); > > /* > @@ -50,6 +53,13 @@ static u32 alloc_struct(u32 bytes) > return s; > } > > +static void zero_struct(u32 s, u32 bytes) "static void memclear(void *ptr, size_t bytes)" would seem to be more appropriate, and avoid you needing to cast the pointers you actually pass into it. > +{ > + asm volatile( > + " rep stosb \n" For a single instruction instruction asm statement, just use 'asm volatile ("rep stosb" ...' No need for these multi-line hoops to create a readable .S > + : "+D" (s), "+c" (bytes) : "a" (0)); > +} > + > static u32 copy_struct(u32 src, u32 bytes) > { > u32 dst, dst_asm; > @@ -77,41 +87,56 @@ static u32 copy_string(u32 src) > 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 = (multiboot_info_t *)copy_struct((u32)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 = copy_string(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; > + } Now I am completely confused. here you set mbd from mbi, but lower in init_mbi, you set mbi from mbd. What is the intended dataflow? > + > + 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 = (module_t *)copy_struct( > - 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 = copy_string(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 = copy_struct(mbi->mmap_addr, mbi->mmap_length); > + return mbd; > +} > > - if ( mbi->flags & MBI_LOADERNAME ) > - mbi->boot_loader_name = copy_string(mbi->boot_loader_name); > +static mbd_t __used *__reloc(void *mbi) > +{ > + mbd_t *mbd; > > - /* Mask features we don't understand or don't relocate. */ > - mbi->flags &= (MBI_MEMLIMITS | > - MBI_BOOTDEV | > - MBI_CMDLINE | > - MBI_MODULES | > - MBI_MEMMAP | > - MBI_LOADERNAME); > + mbd = (mbd_t *)alloc_struct(sizeof(mbd_t)); > + zero_struct((u32)mbd, sizeof(mbd_t)); > > - return mbi; > + 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..ef8c735 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 mbi. */ This is wholly useless as a comment. "Initialise Multiboot Info" would be better, but still identifiable from the name of the function. > + mov mbd_pa(%rip),%edi > + call __init_mbi > + > + /* Pass off the mbi to C land. */ Again, for the commit it would be useful to expand mbi. > + movq %rax,%rdi > call __start_xen > ud2 /* Force a panic (invalid opcode). */ > > @@ -38,7 +42,7 @@ > > .data > .align 8 > -multiboot_ptr: > +mbd_pa: > .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..a2799aa > --- /dev/null > +++ b/xen/arch/x86/boot_info.c > @@ -0,0 +1,59 @@ > +/* > + * 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/cache.h> > +#include <xen/init.h> > +#include <xen/multiboot.h> > + > +#include <asm/mbd.h> > +#include <asm/page.h> > + > +static multiboot_info_t __read_mostly mbi; > + > +extern void enable_exception_support(void); No externs in .c files. This should be in a header file. (I am still not particularly happy about splitting exception support like this, but I can't currently suggest a better alternative) > + > +unsigned long __init __init_mbi(u32 mbd_pa) > +{ > + mbd_t *mbd = __va(mbd_pa); > + > + enable_exception_support(); > + > + if ( mbd->boot_loader_name ) { Xen style > + 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 8c8b91f..24e1be3 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -529,6 +529,25 @@ static char * __init cmdline_cook(char *p, char > *loader_name) > return p; > } > > +void __init enable_exception_support(void) This should have bsp somewhere in the name, as it is specifically different to the ap method of gaining exception support. > +{ > + /* 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. */ This comment serves no purpose now that it is in a function named "enable_exception_support()" > +} > + > void __init noreturn __start_xen(unsigned long mbi_p) > { > char *memmap_type = NULL; > @@ -546,21 +565,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 ) { Xen style. > + enable_exception_support(); > + } > + else > + { > + /* Exception support was enabled before __start_xen() call. */ > + } > > 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..dab0ae1 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_pa, 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..45e7285 > --- /dev/null > +++ b/xen/include/asm-x86/mbd.h > @@ -0,0 +1,79 @@ > +/* > + * 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__ > + > +/* > + * Do not include any headers here! > + * > + * This file is used by xen/arch/x86/boot/reloc.c > + * and any include statement here will break its build. > + * It means that every basic type used below must be defined > + * before any usage of this header. > + */ > + > +/* Module type. */ > +typedef struct { > + u32 start; > + u32 end; > + > + /* A module command line address. */ > + 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 { > + /* Boot loader name physical address. */ > + u32 boot_loader_name; > + > + /* Xen command line physical address. */ > + u32 cmdline; > + > + /* > + * 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; > + > + /* Physical address of memory map provided by bootloader. */ > + u32 mmap; > + > + /* Number of modules. */ > + u32 mods_nr; > + > + /* Physical address of modules description (boot_module_t *). */ This is surely the list of modules, so the pa of a list of module descriptions? ~Andrew > + 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 |