[Xen-devel] [v8][PATCH 07/16] hvmloader/e820: construct guest e820 table
Now use the hypervisor-supplied memory map to build our final e820 table:
* Add regions for BIOS ranges and other special mappings not in the
hypervisor map
* Add in the hypervisor regions
* Adjust the lowmem and highmem regions if we've had to relocate
memory (adding a highmem region if necessary)
* Sort all the ranges so that they appear in memory order.
CC: Keir Fraser k...@xen.org
CC: Jan Beulich jbeul...@suse.com
CC: Andrew Cooper andrew.coop...@citrix.com
CC: Ian Jackson ian.jack...@eu.citrix.com
CC: Stefano Stabellini stefano.stabell...@eu.citrix.com
CC: Ian Campbell ian.campb...@citrix.com
CC: Wei Liu wei.l...@citrix.com
Signed-off-by: Tiejun Chen tiejun.c...@intel.com
---
v8:
* define low_mem_end as uint32_t
* Correct those two wrong loops, memory_map.nr_map - nr
when we're trying to revise low/high memory e820 entries.
* Improve code comments and the patch head description
* Add one check if highmem is just populated by hvmloader itself
v5 ~ v7:
* Nothing is changed.
v4:
* Rename local variable, low_mem_pgend, to low_mem_end.
* Improve some code comments
* Adjust highmem after lowmem is changed.
tools/firmware/hvmloader/e820.c | 92 +
1 file changed, 83 insertions(+), 9 deletions(-)
diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
index b72baa5..aa678a7 100644
--- a/tools/firmware/hvmloader/e820.c
+++ b/tools/firmware/hvmloader/e820.c
@@ -108,7 +108,9 @@ 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;
+uint64_t add_high_mem = 0;
+uint32_t low_mem_end = hvm_info-low_mem_pgend PAGE_SHIFT;
if ( !lowmem_reserved_base )
lowmem_reserved_base = 0xA;
@@ -152,13 +154,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 = 0x10;
-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
@@ -194,9 +189,73 @@ int build_e820_table(struct e820entry *e820,
nr++;
}
+/*
+ * 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] = memory_map.map[i];
+nr++;
+}
+
+/* Low RAM goes here. Reserve space for special pages. */
+BUG_ON(low_mem_end (2u 20));
-if ( hvm_info-high_mem_pgend )
+/*
+ * 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 )
+{
+for ( i = 0; i nr; i++ )
+{
+if ( e820[i].type == E820_RAM
+ e820[i].addr == (1ull 32))
+{
+e820[i].size += add_high_mem;
+add_high_mem = 0;
+break;
+}
+}
+}
+
+/* Or this is just populated by hvmloader itself. */
+if ( add_high_mem )
+{
+/*
+ * hvmloader should always update hvm_info-high_mem_pgend
+ * when it relocates RAM anywhere.
+ */
+BUG_ON( !hvm_info-high_mem_pgend );
+
e820[nr].addr = ((uint64_t)1 32);
e820[nr].size =
((uint64_t)hvm_info-high_mem_pgend PAGE_SHIFT) - e820[nr].addr;
@@ -204,6 +263,21 @@ int build_e820_table(struct e820entry *e820,
nr++;
}
+/* 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;
+
Re: [Xen-devel] [v8][PATCH 07/16] hvmloader/e820: construct guest e820 table
+/*
+ * And then we also need to adjust highmem.
+ */
+if ( add_high_mem )
+{
+for ( i = 0; i nr; i++ )
+{
+if ( e820[i].type == E820_RAM
+ e820[i].addr == (1ull 32))
+{
+e820[i].size += add_high_mem;
+add_high_mem = 0;
+break;
+}
+}
+}
+
+/* Or this is just populated by hvmloader itself. */
This should probably say something like:
If there was no highmem region, we need to create one.
Okay, If there was no highmem entry, we need to create one.
+if ( add_high_mem )
+{
+/*
+ * hvmloader should always update hvm_info-high_mem_pgend
+ * when it relocates RAM anywhere.
+ */
+BUG_ON( !hvm_info-high_mem_pgend );
+
e820[nr].addr = ((uint64_t)1 32);
e820[nr].size =
((uint64_t)hvm_info-high_mem_pgend PAGE_SHIFT) -
e820[nr].addr;
In theory add_high_mem and hvm_info-high_mem_pgend PAGE_SHIFT -
4GiB is the same, but it seems like asking for trouble to assume so
No, its not true in the first if( add_high_mem ) conditional.
Before we enter hvmloader, there are two cases:
#1. hvm_info-high_mem_pgend == 0
So we wouldn't have a highmem entry in e820. But hvmloader may relocate
RAM upward highmem (add_high_mem) to get sufficient mmio, so
hvm_info-high_mem_pgend is expanded somewhere (4GiB + add_high_mem).
Then we would fall into the second if( add_high_mem ) conditional.
#2. hvm_info-high_mem_pgend != 0
We always walk into the first if( add_high_mem ) conditional. But here
add_high_mem just represents that highmem section expanded by
hvmloader, its really not the whole higmem:(hvm_info-high_mem_pgend
PAGE_SHIFT - 4GiB).
Thanks
Tiejun
without checking.
Perhaps in the first if( add_high_mem ) conditional, you can
BUG_ON(add_high_mem != ((hvm_info-high_mem_pgend PAGE_SHIFT) -
(ull1 32))) ?
Other than that, this looks good, thanks.
-George
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [v8][PATCH 07/16] hvmloader/e820: construct guest e820 table
On Thu, Jul 16, 2015 at 2:12 PM, Chen, Tiejun tiejun.c...@intel.com wrote:
+if ( add_high_mem )
+{
+/*
+ * hvmloader should always update hvm_info-high_mem_pgend
+ * when it relocates RAM anywhere.
+ */
+BUG_ON( !hvm_info-high_mem_pgend );
+
e820[nr].addr = ((uint64_t)1 32);
e820[nr].size =
((uint64_t)hvm_info-high_mem_pgend PAGE_SHIFT) -
e820[nr].addr;
In theory add_high_mem and hvm_info-high_mem_pgend PAGE_SHIFT -
4GiB is the same, but it seems like asking for trouble to assume so
No, its not true in the first if( add_high_mem ) conditional.
Before we enter hvmloader, there are two cases:
#1. hvm_info-high_mem_pgend == 0
So we wouldn't have a highmem entry in e820. But hvmloader may relocate RAM
upward highmem (add_high_mem) to get sufficient mmio, so
hvm_info-high_mem_pgend is expanded somewhere (4GiB + add_high_mem).
Then we would fall into the second if( add_high_mem ) conditional.
#2. hvm_info-high_mem_pgend != 0
We always walk into the first if( add_high_mem ) conditional. But here
add_high_mem just represents that highmem section expanded by hvmloader,
its really not the whole higmem:(hvm_info-high_mem_pgend PAGE_SHIFT -
4GiB).
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));
}
--
-George
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [v8][PATCH 07/16] hvmloader/e820: construct guest e820 table
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 = 0xA;
@@ -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 = 0x10;
-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;
+
Re: [Xen-devel] [v8][PATCH 07/16] hvmloader/e820: construct guest e820 table
On 2015/7/16 23:16, George Dunlap wrote:
On 07/16/2015 04:04 PM, Chen, Tiejun wrote:
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 = 0xA;
@@ -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 = 0x10;
-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 =
Re: [Xen-devel] [v8][PATCH 07/16] hvmloader/e820: construct guest e820 table
On 07/16/2015 04:04 PM, Chen, Tiejun wrote:
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 = 0xA;
@@ -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 = 0x10;
-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. */
+
Re: [Xen-devel] [v8][PATCH 07/16] hvmloader/e820: construct guest e820 table
On 07/16/2015 04:29 PM, Chen, Tiejun wrote:
On 2015/7/16 23:16, George Dunlap wrote:
On 07/16/2015 04:04 PM, Chen, Tiejun wrote:
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 = 0xA;
@@ -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 = 0x10;
-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 +=
Re: [Xen-devel] [v8][PATCH 07/16] hvmloader/e820: construct guest e820 table
Honestly I didn't try to change that point but maybe I'm missing something?
Yes, you are missing something. :-) I told you exactly what I wanted
changed and what I said could remain the same:
By all means, calculate high_mem_end so it's easier to read. But then,
when creating a new region, set e820[nr].size = add_high_mem, so that
the BUG_ON() that follows actually checks something useful.
Just to be clear, I want the second if() statement to look like this:
+if ( i == nr )
+{
+e820[nr].addr = ((uint64_t)1 32);
+e820[nr].size = add_high_mem;
Ahh, when you're replying this, I also see this difference and realize
what you meant. Sorry to this inconvenience and I'll sync this line into
my tree :)
Thanks
Tiejun
+e820[nr].type = E820_RAM;
+nr++;
+}
Think about why and maybe that will help you understand what I'm talking
about.
-George
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [v8][PATCH 07/16] hvmloader/e820: construct guest e820 table
On Thu, Jul 16, 2015 at 7:52 AM, Tiejun Chen tiejun.c...@intel.com wrote:
Now use the hypervisor-supplied memory map to build our final e820 table:
* Add regions for BIOS ranges and other special mappings not in the
hypervisor map
* Add in the hypervisor regions
* Adjust the lowmem and highmem regions if we've had to relocate
memory (adding a highmem region if necessary)
* Sort all the ranges so that they appear in memory order.
CC: Keir Fraser k...@xen.org
CC: Jan Beulich jbeul...@suse.com
CC: Andrew Cooper andrew.coop...@citrix.com
CC: Ian Jackson ian.jack...@eu.citrix.com
CC: Stefano Stabellini stefano.stabell...@eu.citrix.com
CC: Ian Campbell ian.campb...@citrix.com
CC: Wei Liu wei.l...@citrix.com
Signed-off-by: Tiejun Chen tiejun.c...@intel.com
---
v8:
* define low_mem_end as uint32_t
* Correct those two wrong loops, memory_map.nr_map - nr
when we're trying to revise low/high memory e820 entries.
* Improve code comments and the patch head description
* Add one check if highmem is just populated by hvmloader itself
v5 ~ v7:
* Nothing is changed.
v4:
* Rename local variable, low_mem_pgend, to low_mem_end.
* Improve some code comments
* Adjust highmem after lowmem is changed.
tools/firmware/hvmloader/e820.c | 92
+
1 file changed, 83 insertions(+), 9 deletions(-)
diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
index b72baa5..aa678a7 100644
--- a/tools/firmware/hvmloader/e820.c
+++ b/tools/firmware/hvmloader/e820.c
@@ -108,7 +108,9 @@ 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;
+uint64_t add_high_mem = 0;
+uint32_t low_mem_end = hvm_info-low_mem_pgend PAGE_SHIFT;
if ( !lowmem_reserved_base )
lowmem_reserved_base = 0xA;
@@ -152,13 +154,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 = 0x10;
-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
@@ -194,9 +189,73 @@ int build_e820_table(struct e820entry *e820,
nr++;
}
+/*
+ * 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] = memory_map.map[i];
+nr++;
+}
+
+/* Low RAM goes here. Reserve space for special pages. */
+BUG_ON(low_mem_end (2u 20));
-if ( hvm_info-high_mem_pgend )
+/*
+ * 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 )
+{
+for ( i = 0; i nr; i++ )
+{
+if ( e820[i].type == E820_RAM
+ e820[i].addr == (1ull 32))
+{
+e820[i].size += add_high_mem;
+add_high_mem = 0;
+break;
+}
+}
+}
+
+/* Or this is just populated by hvmloader itself. */
This should probably say something like:
If there was no highmem region, we need to create one.
+if ( add_high_mem )
+{
+/*
+ * hvmloader should always update hvm_info-high_mem_pgend
+ * when it relocates RAM anywhere.
+ */
+BUG_ON( !hvm_info-high_mem_pgend );
+
e820[nr].addr = ((uint64_t)1 32);
e820[nr].size =
((uint64_t)hvm_info-high_mem_pgend PAGE_SHIFT) -
e820[nr].addr;
In theory add_high_mem and
