Re: [Xen-devel] [PATCH for-xen-4.5 v4 08/18] x86: introduce boot_info structure

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/10/2014 15:12, Daniel Kiper wrote:
> This patch introduces boot_info structure. Subsequent patches will move
> step by step all boot related data to above mentioned struct. At the end
> of this process multiboot (v1) protocol dependency will be broken. It means
> that most of Xen code (excluding preloader) could be bootloader agnostic
> and does not need almost any knowledge about boot protocol. Additionally,
> it will be possible 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_boot_info() -> boot_info_mb -> __start_xen() -> 
> boot_info
>            /
>   BIOS ->-/
>
>    * multiboot2 is not implemented yet. Look for it in later patches.
>
> Here is boot data flow for EFI platform:
>
>    EFI -> efi_start() -> boot_info_efi -> __start_xen() -> boot_info
>
> Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> ---
> v4 - suggestions/fixes:
>     - change some variables "char *" type to "const char *" type
>       (suggested by Andrew Cooper),
>     - do ARM test build
>       (suggested by Ian Campbell and Stefano Stabellini).
>
> v3 - suggestions/fixes:
>     - further patch split rearrangement
>       (suggested by Andrew Cooper).
>
> v2 - suggestions/fixes:
>     - rename XBI to boot_info
>       (suggested by Andrew Cooper),
>     - use more meaningful types in boot_info structure
>       (suggested by Andrew Cooper, Jan Beulich and Stefano Stabellini),
>     - improve boot_info structure comment
>       (suggested by Andrew Cooper and Jan Beulich),
>     - do data shuffling after exception support initialization
>       (suggested by Andrew Cooper),
>     - patch split rearrangement
>       (suggested by Andrew Cooper and Jan Beulich).
> ---
>   xen/arch/x86/boot/x86_64.S      |   10 +++++++--
>   xen/arch/x86/boot_info.c        |   11 +++++++++
>   xen/arch/x86/efi/efi-boot.h     |    1 +
>   xen/arch/x86/setup.c            |   13 ++++++++++-
>   xen/common/efi/efi.h            |    7 ++++++
>   xen/common/efi/runtime.c        |   10 +++++++++
>   xen/include/asm-x86/boot_info.h |   47 +++++++++++++++++++++++++++++++++++++++
>   7 files changed, 96 insertions(+), 3 deletions(-)
>   create mode 100644 xen/include/asm-x86/boot_info.h
>
> diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
> index fc7ce74..2305b56 100644
> --- a/xen/arch/x86/boot/x86_64.S
> +++ b/xen/arch/x86/boot/x86_64.S
> @@ -32,9 +32,15 @@
>           /* Initialize the Multiboot info struct. */
>           mov     mbd_pa(%rip),%edi
>           call    __init_mbi
> +        pushq   %rax
> +
> +        /* Initialize the boot_info. */
> +        mov     mbd_pa(%rip),%edi
> +        call    __init_boot_info
>   
> -        /* Pass off the Multiboot info struct to C land. */
> -        movq    %rax,%rdi
> +        /* Pass off the Multiboot info struct and the boot_info to C land. */
> +        popq    %rdi
> +        movq    %rax,%rsi
>           call    __start_xen
>           ud2     /* Force a panic (invalid opcode). */
>   
> diff --git a/xen/arch/x86/boot_info.c b/xen/arch/x86/boot_info.c
> index 64bc4d9..c5f2a18 100644
> --- a/xen/arch/x86/boot_info.c
> +++ b/xen/arch/x86/boot_info.c
> @@ -21,12 +21,18 @@
>   #include <xen/init.h>
>   #include <xen/multiboot.h>
>   
> +#include <asm/boot_info.h>
>   #include <asm/mbd.h>
>   #include <asm/page.h>
>   #include <asm/setup.h>
>   
>   static multiboot_info_t __read_mostly mbi;
>   
> +static boot_info_t __read_mostly boot_info_mb = {
> +    .warn_msg = NULL,
> +    .err_msg = NULL
Almost all of these fields are 0.  It would be neater to only initialise 
the ones which need to be non-0.
> +};
> +
>   unsigned long __init __init_mbi(u32 mbd_pa)
>   {
>       mbd_t *mbd = __va(mbd_pa);
> @@ -68,3 +74,8 @@ unsigned long __init __init_mbi(u32 mbd_pa)
>   
>       return (unsigned long)&mbi;
>   }
> +
> +boot_info_t __init *__init_boot_info(u32 mbd_pa)
No need for leading underscores on the function name.

> +{
> +    return &boot_info_mb;
> +}
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 9458e7f..3da1a2a 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -255,6 +255,7 @@ static void __init noreturn efi_arch_post_exit_boot(void)
>                        [ds] "r" (__HYPERVISOR_DS),
>                        [stkoff] "i" (STACK_SIZE - sizeof(struct cpu_info)),
>                        "D" (__va(&mbi))
> +                     "S" (__va(&boot_info_efi))
>                      : "memory" );
>       for( ; ; ); /* not reached */
>   }
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index eacb9b2..18fa3e4 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -49,6 +49,7 @@
>   #include <xen/cpu.h>
>   #include <asm/nmi.h>
>   #include <asm/alternative.h>
> +#include <asm/boot_info.h>
>   
>   /* opt_nosmp: If true, secondary processors are ignored. */
>   static bool_t __initdata opt_nosmp;
> @@ -92,6 +93,8 @@ unsigned long __initdata highmem_start;
>   size_param("highmem-start", highmem_start);
>   #endif
>   
> +boot_info_t *boot_info;
> +
>   cpumask_t __read_mostly cpu_present_map;
>   
>   unsigned long __read_mostly xen_phys_start;
> @@ -546,7 +549,7 @@ void __init enable_bsp_exception_support(void)
>       sort_exception_tables();
>   }
>   
> -void __init noreturn __start_xen(unsigned long mbi_p)
> +void __init noreturn __start_xen(unsigned long mbi_p, boot_info_t 
> *boot_info_ptr)
>   {
>       char *memmap_type = NULL;
>       char *cmdline, *kextra, *loader;
> @@ -563,6 +566,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>           .stop_bits = 1
>       };
>   
> +    boot_info = boot_info_ptr;
> +
>       if ( efi_enabled )
>       {
>           enable_bsp_exception_support();
> @@ -612,6 +617,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>       ehci_dbgp_init();
>       console_init_preirq();
>   
> +    if ( boot_info->err_msg )
> +        panic(boot_info->err_msg);
panic("Fatal error from early boot: %s", boot_info->err_msg);

although I suspect any fatal error from boot will prevent us from 
reaching this point.
> +
> +    if ( boot_info->warn_msg )
> +        printk(boot_info->warn_msg);
printk(XENLOG_WARNING "Warning from early boot: %s\n", boot_info->warn_msg);

~Andrew

> +
>       printk("Bootloader: %s\n", loader);
>   
>       printk("Command line: %s\n", cmdline);
> diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
> index bee3b77..526f57c 100644
> --- a/xen/common/efi/efi.h
> +++ b/xen/common/efi/efi.h
> @@ -8,6 +8,9 @@
>   #include <xen/efi.h>
>   #include <xen/spinlock.h>
>   #include <asm/page.h>
> +#ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
> +#include <asm/boot_info.h>
> +#endif
>   
>   struct efi_pci_rom {
>       const struct efi_pci_rom *next;
> @@ -25,6 +28,10 @@ extern const CHAR16 *efi_fw_vendor;
>   
>   extern EFI_RUNTIME_SERVICES *efi_rs;
>   
> +#ifndef CONFIG_ARM /* TODO - boot_info is not implemented on ARM yet */
> +extern boot_info_t boot_info_efi;
> +#endif
> +
>   extern UINTN efi_memmap_size, efi_mdesc_size;
>   extern void *efi_memmap;
>   
> diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
> index 1c43d10..eb0acae 100644
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -5,6 +5,9 @@
>   #include <xen/guest_access.h>
>   #include <xen/irq.h>
>   #include <xen/time.h>
> +#ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
> +#include <asm/boot_info.h>
> +#endif
>   
>   DEFINE_XEN_GUEST_HANDLE(CHAR16);
>   
> @@ -50,6 +53,13 @@ struct efi __read_mostly efi = {
>   const struct efi_pci_rom *__read_mostly efi_pci_roms;
>   
>   #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
> +boot_info_t __read_mostly boot_info_efi = {
> +    .warn_msg = NULL,
> +    .err_msg = NULL
> +};
> +#endif
> +
> +#ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
>   unsigned long efi_rs_enter(void)
>   {
>       static const u16 fcw = FCW_DEFAULT;
> diff --git a/xen/include/asm-x86/boot_info.h b/xen/include/asm-x86/boot_info.h
> new file mode 100644
> index 0000000..f0a76b2
> --- /dev/null
> +++ b/xen/include/asm-x86/boot_info.h
> @@ -0,0 +1,47 @@
> +/*
> + * 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 __BOOT_INFO_H__
> +#define __BOOT_INFO_H__
> +
> +/*
> + * Define boot_info type. It will be used to define variable which in turn
> + * will store data collected by bootloader and preloader. This way it will
> + * be possible to make most of Xen code bootloader agnostic.
> + *
> + * Some members should have relevant EFI/ACPI types. However, due to type
> + * conflicts among ACPI and EFI headers it is not possible to use required
> + * EFI/ACPI types here. Instead of them there are simple types in use which
> + * are compatible as much as possible with relevant EFI/ACPI types.
> + */
> +typedef struct {
> +    /*
> +     * Info about warning occurred during boot_info initialization.
> +     * NULL if everything went OK.
> +     */
> +    const char *warn_msg;
> +
> +    /*
> +     * Info about error occurred during boot_info initialization. NULL if 
> everything
> +     * went OK. Otherwise boot_info is not fully/properly initialized.
> +     */
> +    const char *err_msg;
> +} boot_info_t;
> +
> +extern boot_info_t *boot_info;
> +#endif /* __BOOT_INFO_H__ */

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel