Re: [v8][PATCH 07/16] hvmloader/e820: construct guest e820 table

["Chen, Tiejun" <tiejun.chen@intel.com>]

> Yes, sorry, add_high_mem will be the size of memory *relocated*, not
> the actual end of it (unless, as you say, the original highmem region
> didn't exist).
>
> What I really meant was that either way, after adjusting the highmem
> region in the e820, the end of that region should correspond to
> hvm_info->high_mem_pgend.
>
> What about something like this?
> ---
>      /*
>       * And then we also need to adjust highmem.
>       */
>      if ( add_high_mem )
>      {
>          /*
>           * Modify the existing highmem region if it exists
>           */
>          for ( i = 0; i < nr; i++ )
>          {
>              if ( e820[i].type == E820_RAM &&
>                   e820[i].addr == (1ull << 32))
>              {
>                  e820[i].size += add_high_mem;
>                  break;
>              }
>          }
>
>          /*
>           * If we didn't find a highmem region, make one
>           */
>          if ( i == nr )
>          {
>              e820[nr].addr = ((uint64_t)1 << 32);
>              e820[nr].size = e820[nr].addr + add_high_mem;
>              e820[nr].type = E820_RAM;
>              nr++;
>          }
>
>          /*
>           * Either way, at this point i points to the entry containing
>           * highmem.  Compare it to what's in hvm_info as a sanity
>           * check.
>           */
>          BUG_ON(e820[i].addr+e820[i].size !=
>                 ((uint64_t)hvm_info->high_mem_pgend << PAGE_SHIFT));
>      }
>

Looks really better.

I just introduce a little change based on yours, and I post this as a whole,

diff --git a/tools/firmware/hvmloader/e820.c 
b/tools/firmware/hvmloader/e820.c
index 7a414ab..8c9b01f 100644
--- a/tools/firmware/hvmloader/e820.c
+++ b/tools/firmware/hvmloader/e820.c
@@ -105,7 +105,10 @@ int build_e820_table(struct e820entry *e820,
                       unsigned int lowmem_reserved_base,
                       unsigned int bios_image_base)
  {
-    unsigned int nr = 0;
+    unsigned int nr = 0, i, j;
+    uint32_t low_mem_end = hvm_info->low_mem_pgend << PAGE_SHIFT;
+    uint64_t high_mem_end = (uint64_t)hvm_info->high_mem_pgend << 
PAGE_SHIFT;
+    uint64_t add_high_mem = 0;

      if ( !lowmem_reserved_base )
              lowmem_reserved_base = 0xA0000;
@@ -149,13 +152,6 @@ int build_e820_table(struct e820entry *e820,
      e820[nr].type = E820_RESERVED;
      nr++;

-    /* Low RAM goes here. Reserve space for special pages. */
-    BUG_ON((hvm_info->low_mem_pgend << PAGE_SHIFT) < (2u << 20));
-    e820[nr].addr = 0x100000;
-    e820[nr].size = (hvm_info->low_mem_pgend << PAGE_SHIFT) - 
e820[nr].addr;
-    e820[nr].type = E820_RAM;
-    nr++;
-
      /*
       * Explicitly reserve space for special pages.
       * This space starts at RESERVED_MEMBASE an extends to cover various
@@ -191,16 +187,91 @@ int build_e820_table(struct e820entry *e820,
          nr++;
      }

-
-    if ( hvm_info->high_mem_pgend )
+    /*
+     * Construct E820 table according to recorded memory map.
+     *
+     * The memory map created by toolstack may include,
+     *
+     * #1. Low memory region
+     *
+     * Low RAM starts at least from 1M to make sure all standard regions
+     * of the PC memory map, like BIOS, VGA memory-mapped I/O and vgabios,
+     * have enough space.
+     *
+     * #2. Reserved regions if they exist
+     *
+     * #3. High memory region if it exists
+     */
+    for ( i = 0; i < memory_map.nr_map; i++ )
      {
-        e820[nr].addr = ((uint64_t)1 << 32);
-        e820[nr].size =
-            ((uint64_t)hvm_info->high_mem_pgend << PAGE_SHIFT) - 
e820[nr].addr;
-        e820[nr].type = E820_RAM;
+        e820[nr] = memory_map.map[i];
          nr++;
      }

+    /* Low RAM goes here. Reserve space for special pages. */
+    BUG_ON(low_mem_end < (2u << 20));
+
+    /*
+     * Its possible to relocate RAM to allocate sufficient MMIO previously
+     * so low_mem_pgend would be changed over there. And here memory_map[]
+     * records the original low/high memory, so if low_mem_end is less than
+     * the original we need to revise low/high memory range in e820.
+     */
+    for ( i = 0; i < nr; i++ )
+    {
+        uint64_t end = e820[i].addr + e820[i].size;
+        if ( e820[i].type == E820_RAM &&
+             low_mem_end > e820[i].addr && low_mem_end < end )
+        {
+            add_high_mem = end - low_mem_end;
+            e820[i].size = low_mem_end - e820[i].addr;
+        }
+    }
+
+    /*
+     * And then we also need to adjust highmem.
+     */
+    if ( add_high_mem )
+    {
+        /* Modify the existing highmem region if it exists. */
+        for ( i = 0; i < nr; i++ )
+        {
+            if ( e820[i].type == E820_RAM &&
+                 e820[i].addr == ((uint64_t)1 << 32))
+            {
+                e820[i].size += add_high_mem;
+                break;
+            }
+        }
+
+        /* If there was no highmem region, just create one. */
+        if ( i == nr )
+        {
+            e820[nr].addr = ((uint64_t)1 << 32);
+            e820[nr].size = high_mem_end  - e820[nr].addr;
+            e820[nr].type = E820_RAM;
+            nr++;
+        }
+
+        /* A sanity check if high memory is broken. */
+        BUG_ON( high_mem_end != e820[i].addr + e820[i].size);
+    }
+
+    /* Finally we need to sort all e820 entries. */
+    for ( j = 0; j < nr-1; j++ )
+    {
+        for ( i = j+1; i < nr; i++ )
+        {
+            if ( e820[j].addr > e820[i].addr )
+            {
+                struct e820entry tmp;
+                tmp = e820[j];
+                e820[j] = e820[i];
+                e820[i] = tmp;
+            }
+        }
+    }
+
      return nr;
  }

Thanks
Tiejun