|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 4/7] xen/x86: Migrate to XBI structure
On 09/08/14 00:04, Daniel Kiper wrote:
> We have all constants and structures in place. So, finally break multiboot
> (v1)
> protocol dependency. It means that most of Xen code (excluding preloader)
> could be bootloader agnostic and does not need almost any knowledge about
> boot protocol. Additionally, we are able to pass all boot data to
> __start_xen()
> in one bucket without any side channels. I do not mention that we are also
> able to easily identify boot data in Xen code.
>
> Here is boot data flow for legacy BIOS platform:
>
> BIOS -> GRUB -> multiboot[12]* -> __reloc() -> MBD ->-\
> /
> -----------------<-----------------
> \
> \
> ---> __init_xbi() -> XBI_MB -> __start_xen() -> XBI
> /
> BIOS ->-/
>
> * multiboot2 is not implemented yet. Look for it in next patch.
>
> Here is boot data flow for EFI platform:
>
> EFI -> efi_start() -> XBI_EFI -> __start_xen() -> XBI
>
> WARNING: ARM build could be broken by this patch. We need to agree XBI
> integration into ARM. Personally I think that it is worth storing all
> data from any bootloader and preloader in XBI on any architecture. This
> give a chance to share more code between architectures. However, every
> architecture should define its own XBI (in relevant include/asm directory).
> Despite that it looks that some parts of it could be common, e.g. modules
> data, command line arguments, boot loader name, EFI data, etc., even if types
> would not be the same. So, as it was stated above a lot of code could be
> shared among architectures.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
This patch is massive, and really needs splitting up somewhat. There
are several logically distinct bits to it.
> ---
> 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 | 153 ++++++++++++-----
> xen/arch/x86/boot/x86_64.S | 10 +-
> xen/arch/x86/dmi_scan.c | 7 +-
> xen/arch/x86/domain_build.c | 24 +--
> xen/arch/x86/efi/boot.c | 212 +++++++++++------------
> xen/arch/x86/efi/efi.h | 3 -
> xen/arch/x86/efi/runtime.c | 52 ++++--
> xen/arch/x86/init_xbi.c | 254 +++++++++++++++++++++++++++
> xen/arch/x86/microcode.c | 39 ++---
> xen/arch/x86/mpparse.c | 9 +-
> xen/arch/x86/platform_hypercall.c | 19 +--
> xen/arch/x86/setup.c | 340
> +++++++++++--------------------------
> xen/arch/x86/x86_64/asm-offsets.c | 5 +-
> xen/drivers/acpi/osl.c | 9 +-
> xen/drivers/video/vesa.c | 5 +-
> xen/drivers/video/vga.c | 16 +-
> xen/include/asm-x86/config.h | 2 -
> xen/include/asm-x86/e820.h | 8 -
> xen/include/asm-x86/edd.h | 6 -
> xen/include/asm-x86/setup.h | 10 +-
> xen/include/xen/efi.h | 10 --
> xen/include/xen/vga.h | 4 -
> xen/include/xsm/xsm.h | 14 +-
> xen/xsm/xsm_core.c | 6 +-
> xen/xsm/xsm_policy.c | 14 +-
> 28 files changed, 723 insertions(+), 549 deletions(-)
> create mode 100644 xen/arch/x86/init_xbi.c
>
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index c1e244d..bb2264a 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 += init_xbi.o
init_xbi is not a fantastic filename. xbi alone would be better
(subject to my concerned about the name xbi).
> 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
> + mov MBD_cmdline(%ebx),%eax
FLAGS & MBI_CMDLINE is the prerequisite for the 'cmdline' field being
valid. You cannot drop that check (although making use of some manifest
constants would help)
> 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..29f4887 100644
> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -1,40 +1,56 @@
> -/******************************************************************************
> +/*
> * 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/multiboot.h"
> +#include "../../../include/asm/mbd.h"
> +
> +/*
> + * __HERE__ IS TRUE ENTRY POINT!!!
> + *
> + * 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 +58,105 @@ 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(
> + " cld \n"
__start has already performed cld for us. There is nothing which will
have set it between then and here.
> + " xor %%eax,%%eax \n"
> + " rep stosb \n"
> + : "+D" (s), "+c" (bytes) : : "eax");
Why not "a" (0) and drop the xor and eax clobber?
> +}
> +
> +static u32 copy_struct(u32 src, u32 bytes)
> +{
> + u32 dst;
> +
> + dst = alloc_struct(bytes);
> +
> + asm volatile(
> + " cld \n"
> + " mov %2,%%edi \n"
> " rep movsb \n"
> - : "=&r" (new), "+c" (bytes), "+S" (old)
> - : : "edx", "edi");
> - return new;
> + : "+S" (src), "+c" (bytes) : "r" (dst) : "edi");
Again, drop the mov %2,%%edi and use proper parameters without clobbers
> +
> + 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;
> + module_t *mbi_mods;
> + boot_module_t *mbd_mods;
> +
> + 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;
> +/*
> + * __THIS__ IS NOT ENTRY POINT!!!
> + * PLEASE LOOK AT THE BEGINNING OF THIS FILE!!!
I don't think this needs stating...
> + *
> + * It could be a static but then compiler complains:
> + * error: â__relocâ defined but not used.
> + */
> +mbd_t *__reloc(void *mbi, u32 mb_magic)
static mbd_t *__reloc(void *mbi, u32 mb_magic) __attribute__((used)) is
the correct way of informing gcc that it is referenced from inline assembly.
> +{
> + 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..ec39d38 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 Xen Boot Info. */
> + mov mbd_ptr(%rip),%edi
> + call __init_xbi
This is a large quantity of code shuffling bits of data, which does not
strictly need to be done at this point.
Please defer it until after full trap support has been set up in
__start_xen().
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |