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