|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-xen-4.5 v4 06/18] x86: introduce MultiBoot Data (MBD) type
On 17/10/14 15:11, 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).
"for unused data, which is what currently happens."
> 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.
I have finally worked out why I am so confused by this patch which
initialises mbi from mbd, and mbd from mbi.
You appear to be:
* Introducing the concept of MBD
* Introducing a new global by the name "mbi"
The dataflow at the end of the patch is now:
* Take multiboot1_info from bootloader
* Covert it to MBD in reloc()
* For compatibility reasons, reinitialise the new global from mbd so the
v1 code in __start_xen() continues to work.
Details like this *absolutely must* be in the message, especially when
the code looks buggy to an insufficiently-informed reviewer.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> ---
> v4 - suggestions/fixes:
> - rename reloc() function
> (suggested by Andrew Cooper),
> - rename enable_bsp_exception_support() function
> (suggested by Andrew Cooper),
> - move enable_bsp_exception_support() function
> declaration to header file
> (suggested by Andrew Cooper and Jan Beulich),
> - change sizeof(<expression>) to sizeof(<type>) where possible
> (suggested by Jan Beulich),
> - set only mbi flag only if relevant data is valid
> (suggested by Jan Beulich),
> - improve inline assembly
> (suggested by Andrew Cooper),
> - improve comments
> (suggested by Andrew Cooper),
> - fix copyright
> (suggested by Konrad Rzeszutek Wilk),
> - fix coding style
> (suggested by Andrew Cooper, Jan Beulich and Konrad Rzeszutek Wilk).
>
> 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/Makefile | 3 +-
> xen/arch/x86/boot/cmdline.S | 9 ++---
> xen/arch/x86/boot/head.S | 2 +-
> xen/arch/x86/boot/reloc.c | 66 ++++++++++++++++++++----------
> xen/arch/x86/boot/x86_64.S | 10 +++--
> xen/arch/x86/boot_info.c | 70 ++++++++++++++++++++++++++++++++
> xen/arch/x86/efi/efi-boot.h | 2 +-
> xen/arch/x86/setup.c | 42 +++++++++++--------
> xen/arch/x86/x86_64/asm-offsets.c | 6 +--
> xen/include/asm-x86/mbd.h | 80
> +++++++++++++++++++++++++++++++++++++
> xen/include/asm-x86/setup.h | 2 +
> 12 files changed, 241 insertions(+), 52 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/Makefile b/xen/arch/x86/boot/Makefile
> index 3cab36a..e6ff2b7 100644
> --- 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/asm-x86/mbd.h
>
> export RELOC_DEPS
>
> diff --git a/xen/arch/x86/boot/cmdline.S b/xen/arch/x86/boot/cmdline.S
> index 00687eb..b622c6b 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(%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 0bdbc65..64a0ff7 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 e1c83f4..9792fcd 100644
> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -31,8 +31,12 @@ asm (
> );
>
> typedef unsigned int u32;
> +
> +#include "../../../include/xen/compiler.h"
> #include "../../../include/xen/multiboot.h"
>
> +#include "../../../include/asm/mbd.h"
> +
> static u32 alloc_struct(u32 bytes)
> {
> u32 s;
> @@ -49,6 +53,11 @@ static u32 alloc_struct(u32 bytes)
> return s;
> }
>
> +static void zero_struct(u32 s, u32 bytes)
> +{
> + asm volatile("rep stosb" : "+D" (s), "+c" (bytes) : "a" (0));
As with copy_struct(), s and bytes are just inputs.
> +}
> +
> static u32 copy_struct(u32 src, u32 bytes)
> {
> u32 dst, dst_asm;
> @@ -74,41 +83,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;
> + }
> +
> + 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));
> + zero_struct((u32)mbd, sizeof(*mbd));
>
> - 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..fc7ce74 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
> + /* Initialize the Multiboot info struct. */
> + mov mbd_pa(%rip),%edi
> + call __init_mbi
> +
> + /* Pass off the Multiboot info struct 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_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..64bc4d9
> --- /dev/null
> +++ b/xen/arch/x86/boot_info.c
> @@ -0,0 +1,70 @@
> +/*
> + * Copyright (c) 2013, 2014 Oracle Corp.
> + * Author: 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>
> +#include <asm/setup.h>
> +
> +static multiboot_info_t __read_mostly mbi;
> +
> +unsigned long __init __init_mbi(u32 mbd_pa)
This function is for compatibility while boot_info is introduced across
the series, and removed later.
This is fine, but leave a comment here in the code, to help review.
> +{
> + mbd_t *mbd = __va(mbd_pa);
> +
> + enable_bsp_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;
> + }
> +
> + if ( mbd->mem_lower || mbd->mem_upper )
> + {
> + mbi.flags |= MBI_MEMLIMITS;
> + mbi.mem_lower = mbd->mem_lower;
> + mbi.mem_upper = mbd->mem_upper;
> + }
> +
> + if ( mbd->mmap_size )
> + {
> + mbi.flags |= MBI_MEMMAP;
> + mbi.mmap_length = mbd->mmap_size;
> + mbi.mmap_addr = mbd->mmap;
> + }
> +
> + if ( mbd->mods_nr )
> + {
> + mbi.flags |= MBI_MODULES;
> + mbi.mods_count = mbd->mods_nr;
> + mbi.mods_addr = mbd->mods;
> + }
> +
> + return (unsigned long)&mbi;
> +}
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 4348cfe..5c85854 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -245,7 +245,7 @@ static void __init noreturn efi_arch_post_exit_boot(void)
> [cs] "ir" (__HYPERVISOR_CS),
> [ds] "r" (__HYPERVISOR_DS),
> [stkoff] "i" (STACK_SIZE - sizeof(struct cpu_info)),
> - "D" (&mbi)
> + "D" (__va(&mbi))
> : "memory" );
> for( ; ; ); /* not reached */
> }
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index bfbb310..eacb9b2 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -529,12 +529,29 @@ static char * __init cmdline_cook(char *p, const char
> *loader_name)
> return p;
> }
>
> +void __init enable_bsp_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();
> +}
> +
> void __init noreturn __start_xen(unsigned long mbi_p)
> {
> char *memmap_type = NULL;
> char *cmdline, *kextra, *loader;
> unsigned int initrdidx, domcr_flags = DOMCRF_s3_integrity;
> - multiboot_info_t *mbi = __va(mbi_p);
> + multiboot_info_t *mbi = (multiboot_info_t *)mbi_p;
> module_t *mod = (module_t *)__va(mbi->mods_addr);
> unsigned long nr_pages, raw_max_page, modules_headroom, *module_map;
> int i, j, e820_warn = 0, bytes = 0;
> @@ -546,21 +563,14 @@ 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 )
> + {
> + enable_bsp_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 2de9cad..7364dd6 100644
> --- a/xen/arch/x86/x86_64/asm-offsets.c
> +++ b/xen/arch/x86/x86_64/asm-offsets.c
> @@ -9,10 +9,11 @@
> #include <xen/perfc.h>
> #include <xen/sched.h>
> #include <xen/bitops.h>
> +#include <xen/multiboot.h>
> #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 " */<==\"" \
> @@ -166,6 +167,5 @@ void __dummy__(void)
> OFFSET(MBI_mem_lower, multiboot_info_t, mem_lower);
> 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..11c7bcd
> --- /dev/null
> +++ b/xen/include/asm-x86/mbd.h
> @@ -0,0 +1,80 @@
> +/*
> + * Copyright (c) 2013, 2014 Oracle Corp.
> + * Author: 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 an array of module descriptions (boot_module_t
> *). */
> + u32 mods;
> +} mbd_t;
> +#endif /* __MBD_H__ */
> diff --git a/xen/include/asm-x86/setup.h b/xen/include/asm-x86/setup.h
> index 8f8c6f3..d051ee6 100644
> --- a/xen/include/asm-x86/setup.h
> +++ b/xen/include/asm-x86/setup.h
> @@ -5,6 +5,8 @@
>
> extern unsigned long xenheap_initial_phys_start;
>
> +extern void enable_bsp_exception_support(void);
> +
Altering the exception support should be in a separate patch. It is a
logically distinct piece of work.
~Andrew
> void early_cpu_init(void);
> void early_time_init(void);
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |