From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [v8][PATCH 07/16] hvmloader/e820: construct guest e820 table Date: Thu, 16 Jul 2015 12:47:19 +0100 Message-ID: References: <1437029582-19564-1-git-send-email-tiejun.chen@intel.com> <1437029582-19564-8-git-send-email-tiejun.chen@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1437029582-19564-8-git-send-email-tiejun.chen@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tiejun Chen Cc: Keir Fraser , Ian Campbell , Stefano Stabellini , Andrew Cooper , Ian Jackson , "xen-devel@lists.xen.org" , Jan Beulich , Wei Liu List-Id: xen-devel@lists.xenproject.org On Thu, Jul 16, 2015 at 7:52 AM, Tiejun Chen 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 > CC: Jan Beulich > CC: Andrew Cooper > CC: Ian Jackson > CC: Stefano Stabellini > CC: Ian Campbell > CC: Wei Liu > Signed-off-by: Tiejun Chen > --- > 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 = 0xA0000; > @@ -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 = 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 > @@ -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 hvm_info->high_mem_pgend << PAGE_SHIFT - 4GiB is the same, but it seems like asking for trouble to assume so 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