From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [v7][PATCH 07/16] hvmloader/e820: construct guest e820 table Date: Wed, 15 Jul 2015 17:00:31 +0100 Message-ID: References: <1436420047-25356-1-git-send-email-tiejun.chen@intel.com> <1436420047-25356-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: <1436420047-25356-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 9, 2015 at 6:33 AM, Tiejun Chen wrote: > Now we can use that memory map to build our final > e820 table but it may need to reorder all e820 > entries. I think I would say: -- 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 > --- > 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 | 80 +++++++++++++++++++++++++++++++++-------- > 1 file changed, 66 insertions(+), 14 deletions(-) > > diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c > index 3e53c47..aa2569f 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; > + uint64_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,16 +189,73 @@ 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)); Won't this BUG if the guest was actually given less than 2GiB of RAM? > + > + /* > + * We may need to adjust real lowmem end since we may > + * populate RAM to get enough MMIO previously. > + */ > + for ( i = 0; 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; > + } > + } > + > + /* > + * And then we also need to adjust highmem. > + */ > + if ( add_high_mem ) > + { > + for ( i = 0; i < memory_map.nr_map; i++ ) > + { > + if ( e820[i].type == E820_RAM && > + e820[i].addr > (1ull << 32)) > + e820[i].size += add_high_mem; > + } > + } What if there was originally no high memory, but resizing the pci hole meant we had to create a high memory region? Other than those things, looks good. -George