From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [v7][PATCH 07/16] hvmloader/e820: construct guest e820 table Date: Mon, 13 Jul 2015 14:35:56 +0100 Message-ID: <55A3DADC0200007800090355@mail.emea.novell.com> 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> Content-Disposition: inline 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: Wei Liu , Ian Campbell , Stefano Stabellini , Andrew Cooper , Ian Jackson , xen-devel@lists.xen.org, Keir Fraser List-Id: xen-devel@lists.xenproject.org >>> On 09.07.15 at 07:33, wrote: > Now we can use that memory map to build our final > e820 table but it may need to reorder all e820 > entries. "it" being what? I'm afraid I can't really make sense of the second half of the sentence... > --- 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; For the last one I don't see why uint64_t; uint32_t should be just fine and less (binary) code. > @@ -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)); > + > + /* > + * We may need to adjust real lowmem end since we may > + * populate RAM to get enough MMIO previously. "populate"? Don't you mean "relocate"? > + */ > + for ( i = 0; i < memory_map.nr_map; i++ ) > + { > + uint64_t end = e820[i].addr + e820[i].size; Either loop index/boundary or used array are wrong here: In the earlier loop you copied memory_map[0...nr_map-1] to e820[n...n+nr_map-1], but here you're looping looking at e820[0...nr_map-1] > + if ( e820[i].type == E820_RAM && > + low_mem_end > e820[i].addr && low_mem_end < end ) Assuming you mean to look at the RDM e820[] entries here, this is not a correct check: You don't care about partly or fully contained, all you care about is whether low_mem_end extends beyond the start of the region. > + { > + add_high_mem = end - low_mem_end; > + e820[i].size = low_mem_end - e820[i].addr; > + } > + } > + > + /* > + * And then we also need to adjust highmem. > + */ A single line comment should use the respective comment style. > + 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; > + } > + } But looking at the code I think the comment should be extended to state that we currently expect there to be exactly one such RAM region. > + /* Finally we need to reorder all e820 entries. */ "reorder"? Perhaps "sort"? But despite the many comments - the patch looks a lot better now than earlier versions. Jan