Re: [Xen-devel] [PATCH for-xen-4.5 v3 06/16] x86: Introduce boot_info structure

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 8 Oct 2014, 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
> 
> WARNING: ARM build has not been tested yet.

I am not completely against this patch series for 4.5 but I am really
wary of allowing this amount of changes to EFI (which is now working on
ARM) at this stage of the release cycle without even being built tested
on ARM.

We need to make sure it doesn't introduce any regressions on x86 but
also on ARM, so even if the ARM bits are not implemented, I think it
should be required that the series is not only compiled but also
properly tested on ARM too.


> Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> ---
> 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     |    3 ++-
>  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 |   46 
> +++++++++++++++++++++++++++++++++++++++
>  7 files changed, 96 insertions(+), 4 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 ef8c735..647c33b 100644
> --- a/xen/arch/x86/boot/x86_64.S
> +++ b/xen/arch/x86/boot/x86_64.S
> @@ -32,9 +32,15 @@
>          /* Init mbi. */
>          mov     mbd_pa(%rip),%edi
>          call    __init_mbi
> +        pushq   %rax
> +
> +        /* Init boot_info. */
> +        mov     mbd_pa(%rip),%edi
> +        call    __init_boot_info
>  
> -        /* Pass off the mbi to C land. */
> -        movq    %rax,%rdi
> +        /* Pass off the mbi and 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 a2799aa..83bd255 100644
> --- a/xen/arch/x86/boot_info.c
> +++ b/xen/arch/x86/boot_info.c
> @@ -20,11 +20,17 @@
>  #include <xen/init.h>
>  #include <xen/multiboot.h>
>  
> +#include <asm/boot_info.h>
>  #include <asm/mbd.h>
>  #include <asm/page.h>
>  
>  static multiboot_info_t __read_mostly mbi;
>  
> +static boot_info_t __read_mostly boot_info_mb = {
> +    .warn_msg = NULL,
> +    .err_msg = NULL
> +};
> +
>  extern void enable_exception_support(void);
>  
>  unsigned long __init __init_mbi(u32 mbd_pa)
> @@ -57,3 +63,8 @@ unsigned long __init __init_mbi(u32 mbd_pa)
>  
>      return __pa(&mbi);
>  }
> +
> +paddr_t __init __init_boot_info(u32 mbd_pa)
> +{
> +    return __pa(&boot_info_mb);
> +}
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 71030b0..6d7c222 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -254,7 +254,8 @@ 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" (&mbi),
> +                     "S" (&boot_info_efi)
>                     : "memory" );
>      for( ; ; ); /* not reached */
>  }
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 24e1be3..d2a1450 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;
> @@ -548,7 +551,7 @@ void __init enable_exception_support(void)
>      /* Full exception support from here on in. */
>  }
>  
> -void __init noreturn __start_xen(unsigned long mbi_p)
> +void __init noreturn __start_xen(unsigned long mbi_p, paddr_t boot_info_pa)
>  {
>      char *memmap_type = NULL;
>      char *cmdline, *kextra, *loader;
> @@ -565,6 +568,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          .stop_bits = 1
>      };
>  
> +    boot_info = __va(boot_info_pa);
> +
>      if ( efi_enabled ) {
>          enable_exception_support();
>      }
> @@ -613,6 +618,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);
> +
> +    if ( boot_info->warn_msg )
> +        printk(boot_info->warn_msg);
> +
>      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..9ff3c0f
> --- /dev/null
> +++ b/xen/include/asm-x86/boot_info.h
> @@ -0,0 +1,46 @@
> +/*
> + * 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 __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.
> +     */
> +    char *warn_msg;
> +
> +    /*
> +     * Info about error occurred during boot_info initialization. NULL if 
> everything
> +     * went OK. Otherwise boot_info is not fully/properly initialized.
> +     */
> +    char *err_msg;
> +} boot_info_t;
> +
> +extern boot_info_t *boot_info;
> +#endif /* __BOOT_INFO_H__ */
> -- 
> 1.7.10.4
> 

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