[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH for-xen-4.5 v4 11/18] x86: move legacy BIOS memory map stuff to boot_info



On 17/10/2014 15:12, 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, 135 insertions(+), 98 deletions(-)

diff --git a/xen/arch/x86/boot_info.c b/xen/arch/x86/boot_info.c
index 7d8b0e5..9e4af78 100644
--- a/xen/arch/x86/boot_info.c
+++ b/xen/arch/x86/boot_info.c
@@ -16,45 +16,126 @@
   * 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.
+ */
+

I don't think this comment adds any productive value. I would just discard it.

  #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>
  #include <asm/setup.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;

Strictly speaking, these are uint32_t's rather than unsigned ints.

+
  static multiboot_info_t __read_mostly mbi;
static boot_info_t __read_mostly boot_info_mb = {
      .boot_loader_name = "UNKNOWN",
      .cmdline = NULL,
+    .mmap_src = NULL,
+    .mem_upper = 0,
+    .e820map_nr = 0,
+    .e820map = NULL,
      .warn_msg = NULL,
      .err_msg = NULL
  };
-unsigned long __init __init_mbi(u32 mbd_pa)

This is a rather peculiar way for git/diff to have split the patch. Does the patient algorithm yield a more intelligible patch?

+#define e820_raw bootsym(e820map)
+#define e820_raw_nr bootsym(e820nr)
+
+static void __init init_mmap(boot_info_t *boot_info, mbd_t *mbd)
  {
-    mbd_t *mbd = __va(mbd_pa);
+    int bytes = 0;
+    memory_map_t *map;
- enable_bsp_exception_support();
+    if ( e820_raw_nr )
+        boot_info->mmap_src = "Xen-e820";
+    else if ( mbd->mmap_size )
+    {
+        boot_info->mmap_src = "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 ( mbd->mem_lower || mbd->mem_upper )
+            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) )
      {
-        mbi.flags |= MBI_MEMLIMITS;
-        mbi.mem_lower = mbd->mem_lower;
-        mbi.mem_upper = mbd->mem_upper;
+        boot_info->mmap_src = "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_src = "Multiboot-e801";
- if ( mbd->mmap_size )
+        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
      {
-        mbi.flags |= MBI_MEMMAP;
-        mbi.mmap_length = mbd->mmap_size;
-        mbi.mmap_addr = mbd->mmap;
+        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_bsp_exception_support();
+
      if ( mbd->mods_nr )
      {
          mbi.flags |= MBI_MODULES;
@@ -75,5 +156,7 @@ boot_info_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 &boot_info_mb;
  }
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index f02e604..96e758c 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;

Having a variable by the name of "e820map" of type "e820entry *" is confusing alongside the type "e320map", but that is a fault of the original code.

However, how can this code ever have worked? the symbol e820map is a bootsym and not valid to be used as a regular pointer like this.

      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 8f83969..32d9a3a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -551,13 +551,12 @@ void __init enable_bsp_exception_support(void)
void __init noreturn __start_xen(unsigned long mbi_p, boot_info_t *boot_info_ptr)
  {
-    char *memmap_type = NULL;
      char *cmdline, *kextra;
      unsigned int initrdidx, domcr_flags = DOMCRF_s3_integrity;
      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;
+    int i, j;
      bool_t acpi_boot_table_init_done = 0;
      struct domain *dom0;
      struct ns16550_defaults ns16550 = {
@@ -691,77 +690,11 @@ void __init noreturn __start_xen(unsigned long mbi_p, 
boot_info_t *boot_info_ptr
          /* 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_src, 
boot_info->e820map,
+                                        &boot_info->e820map_nr);

A possible consideration for cleanup. As this is the single caller of init_e820, and all the information is derived from boot_info, init_e820 could drop all 3 of its parameters, and read boot_info straight.

~Andrew

/* 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..03c6658 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_src = "EFI",
+    .mem_upper = 0,
+    .e820map_nr = 0,
+    .e820map = e820map,
      .warn_msg = NULL,
      .err_msg = NULL
  };
diff --git a/xen/include/asm-x86/boot_info.h b/xen/include/asm-x86/boot_info.h
index 44d1674..a882c0c 100644
--- a/xen/include/asm-x86/boot_info.h
+++ b/xen/include/asm-x86/boot_info.h
@@ -19,6 +19,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
@@ -36,6 +40,24 @@ typedef struct {
      /* Xen command line. */
      char *cmdline;
+ /* Memory map source name. */
+    const char *mmap_src;
+
+    /*
+     * 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*/


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.