Re: [Xen-devel] [PATCH for-xen-4.5 v3 09/16] x86: Move legacy BIOS memory map stuff to boot_info

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 8 Oct 2014, Daniel Kiper wrote:
> Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> ---
>  xen/arch/x86/boot_info.c        |  105 
> ++++++++++++++++++++++++++++++++++++---
>  xen/arch/x86/efi/efi-boot.h     |   18 +++----
>  xen/arch/x86/setup.c            |   73 ++-------------------------
>  xen/common/efi/runtime.c        |    7 +++
>  xen/include/asm-x86/boot_info.h |   22 ++++++++
>  xen/include/asm-x86/e820.h      |    8 ---
>  6 files changed, 138 insertions(+), 95 deletions(-)
> 
> diff --git a/xen/arch/x86/boot_info.c b/xen/arch/x86/boot_info.c
> index 7101f6c..53e890b 100644
> --- a/xen/arch/x86/boot_info.c
> +++ b/xen/arch/x86/boot_info.c
> @@ -15,40 +15,127 @@
>   * with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +/*
> + * Some ideas are taken (out) from xen/arch/x86/boot/reloc.c,
> + * xen/arch/x86/efi/boot.c and xen/arch/x86/setup.c.
> + */
> +
>  #include <xen/types.h>
>  #include <xen/cache.h>
>  #include <xen/init.h>
>  #include <xen/multiboot.h>
>  
>  #include <asm/boot_info.h>
> +#include <asm/e820.h>
>  #include <asm/mbd.h>
>  #include <asm/page.h>
>  
> +/* These symbols live in the boot trampoline. Access via bootsym(). */
> +extern struct e820entry e820map[];
> +extern unsigned int e820nr;
> +extern unsigned int lowmem_kb, highmem_kb;
> +
>  static multiboot_info_t __read_mostly mbi;
>  
>  static boot_info_t __read_mostly boot_info_mb = {
>      .boot_loader_name = "UNKNOWN",
>      .cmdline = NULL,
> +    .mmap_type = NULL,
> +    .mem_upper = 0,
> +    .e820map_nr = 0,
> +    .e820map = NULL,
>      .warn_msg = NULL,
>      .err_msg = NULL
>  };
>  
> +#define e820_raw bootsym(e820map)
> +#define e820_raw_nr bootsym(e820nr)
> +
>  extern void enable_exception_support(void);
>  
> +static void __init init_mmap(boot_info_t *boot_info, mbd_t *mbd)
> +{
> +    int bytes = 0;
> +    memory_map_t *map;
> +
> +    if ( e820_raw_nr )
> +        boot_info->mmap_type = "Xen-e820";
> +    else if ( mbd->mmap_size )
> +    {
> +        boot_info->mmap_type = "Multiboot-e820";
> +
> +        while ( (bytes < mbd->mmap_size) && (e820_raw_nr < E820MAX) )
> +        {
> +            /*
> +             * This is a gross workaround for a BIOS bug. Some bootloaders do
> +             * not write e820 map entries into pre-zeroed memory. This is
> +             * okay if the BIOS fills in all fields of the map entry, but
> +             * some broken BIOSes do not bother to write the high word of
> +             * the length field if the length is smaller than 4GB. We
> +             * detect and fix this by flagging sections below 4GB that
> +             * appear to be larger than 4GB in size.
> +             */
> +            map = __va(mbd->mmap + bytes);
> +
> +            if ( !map->base_addr_high && map->length_high )
> +            {
> +                map->length_high = 0;
> +                boot_info->warn_msg = "WARNING: Buggy e820 map detected and 
> fixed "
> +                                "(truncated length fields).\n";
> +            }
> +
> +            e820_raw[e820_raw_nr].addr =
> +                ((u64)map->base_addr_high << 32) | (u64)map->base_addr_low;
> +            e820_raw[e820_raw_nr].size =
> +                ((u64)map->length_high << 32) | (u64)map->length_low;
> +            e820_raw[e820_raw_nr].type = map->type;
> +            e820_raw_nr++;
> +
> +            bytes += map->size + 4;
> +        }
> +    }
> +    else if ( bootsym(lowmem_kb) )
> +    {
> +        boot_info->mmap_type = "Xen-e801";
> +
> +        e820_raw[0].addr = 0;
> +        e820_raw[0].size = bootsym(lowmem_kb) << 10;
> +        e820_raw[0].type = E820_RAM;
> +        e820_raw[1].addr = 0x100000;
> +        e820_raw[1].size = bootsym(highmem_kb) << 10;
> +        e820_raw[1].type = E820_RAM;
> +        e820_raw_nr = 2;
> +    }
> +    else if ( mbd->mem_lower || mbd->mem_upper )
> +    {
> +        boot_info->mmap_type = "Multiboot-e801";
> +
> +        e820_raw[0].addr = 0;
> +        e820_raw[0].size = mbd->mem_lower << 10;
> +        e820_raw[0].type = E820_RAM;
> +        e820_raw[1].addr = 0x100000;
> +        e820_raw[1].size = mbd->mem_upper << 10;
> +        e820_raw[1].type = E820_RAM;
> +        e820_raw_nr = 2;
> +    }
> +    else
> +    {
> +        boot_info->err_msg = "Bootloader provided no memory information.\n";
> +        return;
> +    }
> +
> +    boot_info->mem_upper = mbd->mem_upper;
> +
> +    boot_info->e820map_nr = e820_raw_nr;
> +    boot_info->e820map = e820_raw;
> +}
> +
>  unsigned long __init __init_mbi(u32 mbd_pa)
>  {
>      mbd_t *mbd = __va(mbd_pa);
>  
>      enable_exception_support();
>  
> -    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;
> @@ -66,5 +153,7 @@ paddr_t __init __init_boot_info(u32 mbd_pa)
>      if ( mbd->cmdline )
>          boot_info_mb.cmdline = __va(mbd->cmdline);
>  
> +    init_mmap(&boot_info_mb, mbd);
> +
>      return __pa(&boot_info_mb);
>  }
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 3b5628a..8e57ac2 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -148,7 +148,7 @@ static void __init 
> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>      unsigned int i;
>  
>      /* Populate E820 table and check trampoline area availability. */
> -    e = e820map - 1;
> +    e = boot_info_efi.e820map - 1;
>      for ( i = 0; i < map_size; i += desc_size )
>      {
>          EFI_MEMORY_DESCRIPTOR *desc = map + i;
> @@ -182,10 +182,10 @@ static void __init 
> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>              type = E820_NVS;
>              break;
>          }
> -        if ( e820nr && type == e->type &&
> +        if ( boot_info_efi.e820map_nr && type == e->type &&
>               desc->PhysicalStart == e->addr + e->size )
>              e->size += len;
> -        else if ( !len || e820nr >= E820MAX )
> +        else if ( !len || boot_info_efi.e820map_nr >= E820MAX )
>              continue;
>          else
>          {
> @@ -193,7 +193,7 @@ static void __init 
> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>              e->addr = desc->PhysicalStart;
>              e->size = len;
>              e->type = type;
> -            ++e820nr;
> +            ++boot_info_efi.e820map_nr;
>          }
>      }
>  
> @@ -201,12 +201,12 @@ static void __init 
> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>  
>  static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
>  {
> -    place_string_u32(&mbi.mem_upper, NULL);
> -    mbi.mem_upper -= map_size;
> -    mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
> -    if ( mbi.mem_upper < xen_phys_start )
> +    place_string_u32(&boot_info_efi.mem_upper, NULL);
> +    boot_info_efi.mem_upper -= map_size;
> +    boot_info_efi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
> +    if ( boot_info_efi.mem_upper < xen_phys_start )
>          return NULL;
> -    return (void *)(long)mbi.mem_upper;
> +    return (void *)(long)boot_info_efi.mem_upper;
>  }
>  
>  static void __init efi_arch_pre_exit_boot(void)
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index d7416d3..de29d9e 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -553,13 +553,12 @@ void __init enable_exception_support(void)
>  
>  void __init noreturn __start_xen(unsigned long mbi_p, paddr_t boot_info_pa)
>  {
> -    char *memmap_type = NULL;
>      char *cmdline, *kextra;
>      unsigned int initrdidx, domcr_flags = DOMCRF_s3_integrity;
>      multiboot_info_t *mbi = __va(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;
> +    int i, j;
>      bool_t acpi_boot_table_init_done = 0;
>      struct domain *dom0;
>      struct ns16550_defaults ns16550 = {
> @@ -692,77 +691,11 @@ void __init noreturn __start_xen(unsigned long mbi_p, 
> paddr_t boot_info_pa)
>          /* Make boot page tables match non-EFI boot. */
>          l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] =
>              l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR);
> -
> -        memmap_type = boot_info->boot_loader_name;
> -    }
> -    else if ( e820_raw_nr != 0 )
> -    {
> -        memmap_type = "Xen-e820";
>      }
> -    else if ( mbi->flags & MBI_MEMMAP )
> -    {
> -        memmap_type = "Multiboot-e820";
> -        while ( (bytes < mbi->mmap_length) && (e820_raw_nr < E820MAX) )
> -        {
> -            memory_map_t *map = __va(mbi->mmap_addr + bytes);
> -
> -            /*
> -             * This is a gross workaround for a BIOS bug. Some bootloaders do
> -             * not write e820 map entries into pre-zeroed memory. This is
> -             * okay if the BIOS fills in all fields of the map entry, but
> -             * some broken BIOSes do not bother to write the high word of
> -             * the length field if the length is smaller than 4GB. We
> -             * detect and fix this by flagging sections below 4GB that
> -             * appear to be larger than 4GB in size.
> -             */
> -            if ( (map->base_addr_high == 0) && (map->length_high != 0) )
> -            {
> -                if ( !e820_warn )
> -                {
> -                    printk("WARNING: Buggy e820 map detected and fixed "
> -                           "(truncated length fields).\n");
> -                    e820_warn = 1;
> -                }
> -                map->length_high = 0;
> -            }
> -
> -            e820_raw[e820_raw_nr].addr = 
> -                ((u64)map->base_addr_high << 32) | (u64)map->base_addr_low;
> -            e820_raw[e820_raw_nr].size = 
> -                ((u64)map->length_high << 32) | (u64)map->length_low;
> -            e820_raw[e820_raw_nr].type = map->type;
> -            e820_raw_nr++;
> -
> -            bytes += map->size + 4;
> -        }
> -    }
> -    else if ( bootsym(lowmem_kb) )
> -    {
> -        memmap_type = "Xen-e801";
> -        e820_raw[0].addr = 0;
> -        e820_raw[0].size = bootsym(lowmem_kb) << 10;
> -        e820_raw[0].type = E820_RAM;
> -        e820_raw[1].addr = 0x100000;
> -        e820_raw[1].size = bootsym(highmem_kb) << 10;
> -        e820_raw[1].type = E820_RAM;
> -        e820_raw_nr = 2;
> -    }
> -    else if ( mbi->flags & MBI_MEMLIMITS )
> -    {
> -        memmap_type = "Multiboot-e801";
> -        e820_raw[0].addr = 0;
> -        e820_raw[0].size = mbi->mem_lower << 10;
> -        e820_raw[0].type = E820_RAM;
> -        e820_raw[1].addr = 0x100000;
> -        e820_raw[1].size = mbi->mem_upper << 10;
> -        e820_raw[1].type = E820_RAM;
> -        e820_raw_nr = 2;
> -    }
> -    else
> -        panic("Bootloader provided no memory information.");
>  
>      /* Sanitise the raw E820 map to produce a final clean version. */
> -    max_page = raw_max_page = init_e820(memmap_type, e820_raw, &e820_raw_nr);
> +    max_page = raw_max_page = init_e820(boot_info->mmap_type, 
> boot_info->e820map,
> +                                        &boot_info->e820map_nr);
>  
>      /* Create a temporary copy of the E820 map. */
>      memcpy(&boot_e820, &e820, sizeof(e820));
> diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
> index ee2ee2d..865d0bc 100644
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -7,6 +7,7 @@
>  #include <xen/time.h>
>  #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
>  #include <asm/boot_info.h>
> +#include <asm/e820.h>
>  #endif
>  
>  DEFINE_XEN_GUEST_HANDLE(CHAR16);
> @@ -53,9 +54,15 @@ struct efi __read_mostly efi = {
>  const struct efi_pci_rom *__read_mostly efi_pci_roms;
>  
>  #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
> +extern struct e820entry e820map[];
> +
>  boot_info_t __read_mostly boot_info_efi = {
>      .boot_loader_name = "EFI",
>      .cmdline = NULL,
> +    .mmap_type = "EFI",
> +    .mem_upper = 0,
> +    .e820map_nr = 0,
> +    .e820map = e820map,
>      .warn_msg = NULL,
>      .err_msg = NULL
>  };

This is not going in the right direction of getting rid of arch-specific
concepts from xen/common/efi/runtime.c.


> diff --git a/xen/include/asm-x86/boot_info.h b/xen/include/asm-x86/boot_info.h
> index d74ed67..640a420 100644
> --- a/xen/include/asm-x86/boot_info.h
> +++ b/xen/include/asm-x86/boot_info.h
> @@ -18,6 +18,10 @@
>  #ifndef __BOOT_INFO_H__
>  #define __BOOT_INFO_H__
>  
> +#include <xen/types.h>
> +
> +#include <asm/e820.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
> @@ -35,6 +39,24 @@ typedef struct {
>      /* Xen command line. */
>      char *cmdline;
>  
> +    /* Memory map type (source of memory map). */
> +    char *mmap_type;

I realize that you inherited mmap_type from existing code, but shouldn't
this be an enum?


> +    /*
> +     * Amount of upper memory (in KiB) accordingly to The Multiboot
> +     * Specification version 0.6.96.
> +     */
> +    u32 mem_upper;
> +
> +    /* Number of memory map entries provided by Xen preloader. */
> +    unsigned int e820map_nr;
> +
> +    /*
> +     * Memory map provided by Xen preloader. It should always point
> +     * to an area able to accommodate at least E820MAX entries.
> +     */
> +    struct e820entry *e820map;
> +
>      /*
>       * Info about warning occurred during boot_info initialization.
>       * NULL if everything went OK.
> diff --git a/xen/include/asm-x86/e820.h b/xen/include/asm-x86/e820.h
> index d9ff4eb..8727afb 100644
> --- a/xen/include/asm-x86/e820.h
> +++ b/xen/include/asm-x86/e820.h
> @@ -33,12 +33,4 @@ extern int e820_add_range(
>  extern unsigned long init_e820(const char *, struct e820entry *, unsigned 
> int *);
>  extern struct e820map e820;
>  
> -/* These symbols live in the boot trampoline. */
> -extern struct e820entry e820map[];
> -extern unsigned int e820nr;
> -extern unsigned int lowmem_kb, highmem_kb;
> -
> -#define e820_raw bootsym(e820map)
> -#define e820_raw_nr bootsym(e820nr)
> -
>  #endif /*__E820_HEADER*/
> -- 
> 1.7.10.4
> 

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