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

Re: [Xen-devel] [RFC][PATCH 13/13] hvmloader/e820: construct guest e820 table

On 2015/5/15 14:25, Jan Beulich wrote:
On 15.05.15 at 08:11, <tiejun.chen@xxxxxxxxx> wrote:
On 2015/4/20 22:29, Jan Beulich wrote:
On 10.04.15 at 11:22, <tiejun.chen@xxxxxxxxx> wrote:
@@ -119,10 +120,6 @@ int build_e820_table(struct e820entry *e820,

       /* 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++;

I think the above comment needs adjustment with all this code
removed. I also wonder how meaningful the BUG_ON() is with
->low_mem_pgend no longer used for E820 table construction.
Perhaps this needs another BUG_ON() validating that the field
matches some value from memory_map.map[]?

But I think hvm_info->low_mem_pgend is still correct, right?

I think so, but as said it's becoming less used and hence less
relevant here.


additionally, there's no any obvious flag to indicate which
memory_map.map[x] is that last low memory map.

I didn't imply it would be immediately obvious _how_ to do this.
I'm merely wanting to avoid leaving meaningless BUG_ON()s in
the code, while meaningful ones are amiss.

Maybe we should lookup all .map[] to get the lowest memory map and then BUG_ON?

Even we may separate the
low memory to construct memory_map.map[]...


Sorry I just mean that the low memory is not represented with only one memory_map.map[] in some cases. Is it impossible? Even in the future? Or actually we always consider the lowest memory map?

@@ -159,16 +156,37 @@ int build_e820_table(struct e820entry *e820,

-    if ( hvm_info->high_mem_pgend )
+    /* Construct the remaining according memory_map[]. */
+    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].addr = memory_map.map[i].addr;
+        e820[nr].size = memory_map.map[i].size;
+        e820[nr].type = memory_map.map[i].type;

Afaict you could use structure assignment here to make this
more readable.

Sorry, are you saying this?

memcpy(&e820[nr], &memory_map.map[i], sizeof(struct e820entry));

No, structure assignment (which, other than memcpy(), is type safe):

     e820[nr] = memory_map.map[i];



Xen-devel mailing list



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