Re: [Xen-devel] [v9][PATCH 07/16] hvmloader/e820: construct guest e820 table
Before you add memory_map.nr_map, you should be able to iterate from 0 to (not inclusive) nr. At least as far as I recall the original patch. Sorry, I really don't understand what you want. Before we add memory_map.nr_map, e820[0, nr) don't include low/high memory, right? Why? memory_map is representing the reserved areas only, isn't it? If that's not the case, then of course everything is fine. I'm pretty sure the memory map we get here is an extension of the original PV-only get_e820 hypercall, which *does* include both the lowmem and highmem regions. In any case, it's pretty clear from the patched code that Tiejun is removing the old code which created the lowmem and highmem regions and is not replacing it. Where do you think the highmem region he's looking for was coming from? On second thoughts, I prefer to check/sync memory_map.map[] before copy them into e820 since ultimately this can make sure hvm_info, memory_map.map[] and e820 are on the same page. Anyway, I would send out v10 to address others so please further post your comments over there. Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v9][PATCH 07/16] hvmloader/e820: construct guest e820 table
On Fri, Jul 17, 2015 at 5:06 PM, Jan Beulich jbeul...@suse.com wrote: On 17.07.15 at 17:54, tiejun.c...@intel.com wrote: +for ( i = nr-1; i memory_map.nr_map; i-- ) Before you add memory_map.nr_map, you should be able to iterate from 0 to (not inclusive) nr. At least as far as I recall the original patch. Sorry, I really don't understand what you want. Before we add memory_map.nr_map, e820[0, nr) don't include low/high memory, right? Why? memory_map is representing the reserved areas only, isn't it? If that's not the case, then of course everything is fine. I'm pretty sure the memory map we get here is an extension of the original PV-only get_e820 hypercall, which *does* include both the lowmem and highmem regions. In any case, it's pretty clear from the patched code that Tiejun is removing the old code which created the lowmem and highmem regions and is not replacing it. Where do you think the highmem region he's looking for was coming from? -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v9][PATCH 07/16] hvmloader/e820: construct guest e820 table
On 2015/7/17 18:50, Jan Beulich wrote:
On 17.07.15 at 11:09, tiejun.c...@intel.com wrote:
And then of course there's the question of whether nr is really
the right upper loop bound here: Just prior to this you added
the hypervisor supplied entries - why would you need to iterate
over them here? I.e. I'd see this better be moved ahead of that
other code.
Sounds you mean I should sync low/high memory in memory_map.map[]
beforehand and then fulfill e820 like this,
Why would you want/need to sync into memory_map.map[]?
But actually I just felt this make our process clear.
That's certainly not what I suggested.
Do you mean I should check low/high mem before we add the hypervisor
supplied entries like this?
+for ( i = nr-1; i memory_map.nr_map; 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;
+break;
+}
+}
+
+/* And then we also need to adjust highmem. */
+if ( add_high_mem )
+{
+/* Modify the existing highmem region if it exists. */
+for ( i = nr-1 ; i memory_map.nr_map; 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 == memory_map.nr_map )
+{
+e820[nr].addr = ((uint64_t)1 32);
+e820[nr].size = add_high_mem;
+e820[nr].type = E820_RAM;
+i = nr;
+nr++;
+}
+
+/* A sanity check if high memory is broken. */
+BUG_ON( high_mem_end != e820[i].addr + e820[i].size);
+}
Thanks
Tiejun
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [v9][PATCH 07/16] hvmloader/e820: construct guest e820 table
On 17.07.15 at 17:22, tiejun.c...@intel.com wrote: Do you mean I should check low/high mem before we add the hypervisor supplied entries Yes. like this? Not exactly: +for ( i = nr-1; i memory_map.nr_map; i-- ) Before you add memory_map.nr_map, you should be able to iterate from 0 to (not inclusive) nr. At least as far as I recall the original patch. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v9][PATCH 07/16] hvmloader/e820: construct guest e820 table
On 17.07.15 at 02:45, 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
... hypervisor supplied regions?
--- 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;
Just like previously said for low_mem_end - why not uint32_t?
@@ -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
DYM We may have relocated RAM ...?
+ * 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
Blank line between declarations and statements please.
+ 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;
+}
+}
The way it's written I take it that you assume there to be exactly
one region that the adjustment needs to be done for. Iirc this is
correct with the current model, but why would you continue the
loop then afterwards? Placing a break in the if()'s body would
document the fact that only one such region should exist, and
would eliminate questions as to whether add_high_mem shouldn't
be updated (+=) instead of simply being assigned a new value.
And then of course there's the question of whether nr is really
the right upper loop bound here: Just prior to this you added
the hypervisor supplied entries - why would you need to iterate
over them here? I.e. I'd see this better be moved ahead of that
other code.
+/*
+ * And then we also need to adjust highmem.
+ */
I'm sure I gave this comment before: This is a single line comment, so
its style should be that of a single line comment.
+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 = add_high_mem;
+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);
+}
Remind me again please - what prevents the highmem region from
colliding with hypervisor supplied entries?
Also, what if the resulting region exceeds the addressable range
(guest's view of CPUID[8008].EAX[0:7])?
+/* 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];
Please make this the initializer of tmp and add the once again missing
blank line.
Jan
+e820[j] = e820[i];
+e820[i] = tmp;
+}
+}
+}
+
Re: [Xen-devel] [v9][PATCH 07/16] hvmloader/e820: construct guest e820 table
The way it's written I take it that you assume there to be exactly
one region that the adjustment needs to be done for. Iirc this is
correct with the current model, but why would you continue the
loop then afterwards? Placing a break in the if()'s body would
document the fact that only one such region should exist, and
would eliminate questions as to whether add_high_mem shouldn't
be updated (+=) instead of simply being assigned a new value.
Yes, break should be added here.
And then of course there's the question of whether nr is really
the right upper loop bound here: Just prior to this you added
the hypervisor supplied entries - why would you need to iterate
over them here? I.e. I'd see this better be moved ahead of that
other code.
Sounds you mean I should sync low/high memory in memory_map.map[]
beforehand and then fulfill e820 like this,
diff --git a/tools/firmware/hvmloader/e820.c
b/tools/firmware/hvmloader/e820.c
index 7a414ab..b0aa48d 100644
--- a/tools/firmware/hvmloader/e820.c
+++ b/tools/firmware/hvmloader/e820.c
@@ -105,7 +105,11 @@ 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;
+uint32_t add_high_mem = 0;
+uint64_t high_mem_end = (uint64_t)hvm_info-high_mem_pgend
PAGE_SHIFT;
+uint64_t map_start, map_size, map_end;
if ( !lowmem_reserved_base )
lowmem_reserved_base = 0xA;
@@ -149,13 +153,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 +188,101 @@ int build_e820_table(struct e820entry *e820,
nr++;
}
+/* Low RAM goes here. Reserve space for special pages. */
+BUG_ON(low_mem_end (2u 20));
-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
+ *
+ * Note we just have one low memory entry and one high mmeory entry if
+ * exists.
+ *
+ * But we may have relocated 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 firstly.
+ */
+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;
+map_start = memory_map.map[i].addr;
+map_size = memory_map.map[i].size;
+map_end = map_start + map_end;
+
+/* If we need to adjust lowmem. */
+if ( memory_map.map[i].type == E820_RAM
+ low_mem_end map_start low_mem_end map_end )
+{
+add_high_mem = map_end - low_mem_end;
+memory_map.map[i].size = low_mem_end - map_start;
+break;
+}
+}
+
+/* If we need to adjust highmem. */
+if ( add_high_mem )
+{
+/* Modify the existing highmem region if it exists. */
+for ( i = 0; i memory_map.nr_map; i++ )
+{
+map_start = memory_map.map[i].addr;
+map_size = memory_map.map[i].size;
+map_end = map_start + map_end;
+
+if ( memory_map.map[i].type == E820_RAM
+ map_start == ((uint64_t)1 32))
+{
+memory_map.map[i].size += add_high_mem;
+break;
+}
+}
+
+/* If there was no highmem region, just create one. */
+if ( i == memory_map.nr_map )
+{
+memory_map.map[i].addr = ((uint64_t)1 32);
+memory_map.map[i].size = add_high_mem;
+memory_map.map[i].type = E820_RAM;
+}
+
+/* A sanity check if high memory is broken. */
+BUG_ON( high_mem_end !=
+memory_map.map[i].addr + memory_map.map[i].size);
+}
+
+/* Now fulfill e820. */
+for ( i = 0; i memory_map.nr_map; i++ )
Re: [Xen-devel] [v9][PATCH 07/16] hvmloader/e820: construct guest e820 table
+for ( i = nr-1; i memory_map.nr_map; i-- )
Before you add memory_map.nr_map, you should be able to iterate
from 0 to (not inclusive) nr. At least as far as I recall the original
patch.
Sorry, I really don't understand what you want.
Before we add memory_map.nr_map, e820[0, nr) don't include low/high
memory, right? So sounds you want me to
for ( i = 0 i memory_map.nr_map; i++ )
{
if we need to adjust low memory, we just set final low e820 entry;
if we need to adjust high memory, we just set final high e820 entry;
}
Right? But its impossible to do this since we can't assume
memory_map.map[low memory] is always prior to memory_map.map[high memory].
If I still follow your way, please don't mind to show a pseudocode help
me understand what you want.
Thanks
Tiejun
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [v9][PATCH 07/16] hvmloader/e820: construct guest e820 table
On 17.07.15 at 17:54, tiejun.c...@intel.com wrote: +for ( i = nr-1; i memory_map.nr_map; i-- ) Before you add memory_map.nr_map, you should be able to iterate from 0 to (not inclusive) nr. At least as far as I recall the original patch. Sorry, I really don't understand what you want. Before we add memory_map.nr_map, e820[0, nr) don't include low/high memory, right? Why? memory_map is representing the reserved areas only, isn't it? If that's not the case, then of course everything is fine. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v9][PATCH 07/16] hvmloader/e820: construct guest e820 table
Remind me again please - what prevents the highmem region from colliding with hypervisor supplied entries? Also, what if the resulting region exceeds the addressable range (guest's view of CPUID[8008].EAX[0:7])? Any idea to this? I think this issue also exists previously. Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v9][PATCH 07/16] hvmloader/e820: construct guest e820 table
On 17.07.15 at 11:09, tiejun.c...@intel.com wrote: And then of course there's the question of whether nr is really the right upper loop bound here: Just prior to this you added the hypervisor supplied entries - why would you need to iterate over them here? I.e. I'd see this better be moved ahead of that other code. Sounds you mean I should sync low/high memory in memory_map.map[] beforehand and then fulfill e820 like this, Why would you want/need to sync into memory_map.map[]? That's certainly not what I suggested. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v9][PATCH 07/16] hvmloader/e820: construct guest e820 table
On 17.07.15 at 11:27, tiejun.c...@intel.com wrote: Remind me again please - what prevents the highmem region from colliding with hypervisor supplied entries? Also, what if the resulting region exceeds the addressable range (guest's view of CPUID[8008].EAX[0:7])? Any idea to this? I think this issue also exists previously. Oh, right, so I guess leave this as is for now. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v9][PATCH 07/16] hvmloader/e820: construct guest e820 table
On 2015/7/18 0:06, Jan Beulich wrote: On 17.07.15 at 17:54, tiejun.c...@intel.com wrote: +for ( i = nr-1; i memory_map.nr_map; i-- ) Before you add memory_map.nr_map, you should be able to iterate from 0 to (not inclusive) nr. At least as far as I recall the original patch. Sorry, I really don't understand what you want. Before we add memory_map.nr_map, e820[0, nr) don't include low/high memory, right? Why? memory_map is representing the reserved areas only, isn't it? + * 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 Am I wrong with your expectation? Thanks Tiejun If that's not the case, then of course everything is fine. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [v9][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
---
v9:
* Refine that chunk of codes to check/modify highmem
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 | 99 +++--
1 file changed, 85 insertions(+), 14 deletions(-)
diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
index 7a414ab..49d420a 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 = add_high_mem;
+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
