From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v7][PATCH 07/16] hvmloader/e820: construct guest e820 table Date: Tue, 14 Jul 2015 13:22:16 +0800 Message-ID: <55A49C88.2060803@intel.com> References: <1436420047-25356-1-git-send-email-tiejun.chen@intel.com> <1436420047-25356-8-git-send-email-tiejun.chen@intel.com> <55A3DADC0200007800090355@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55A3DADC0200007800090355@mail.emea.novell.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: Jan Beulich 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 >> 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... I hope the following can work for you, ... but finally we should sort them into an increasing order since we shouldn't assume the original order is always good. > >> --- 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) [snip] > >> + */ >> + 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] You're right. I should lookup all e820[] like this, for ( i = 0; i < nr; i++ ) > >> + 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. Here I'm looking at the e820 entry indicating low memory. Because low_mem_end = hvm_info->low_mem_pgend << PAGE_SHIFT; and when we allocate MMIO in pci.c, its possible to populate RAM so hvm_info->low_mem_pgend would be changed over there. So we need to compensate this loss with high memory. Here memory_map[] also records the original low/high memory, so if low_mem_end is less-than the original we need this compensation. So here we have two steps to address this issue, #1. Calculate the loss > >> + { >> + 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. > #2. Compensate the loss >> + if ( add_high_mem ) >> + { >> + for ( i = 0; i < memory_map.nr_map; i++ ) s/memory_map.nr_map/nr >> + { >> + 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. > I can add this at the beginning of #1 loop, 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. Thanks Tiejun