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: Tue, 14 Jul 2015 11:48:25 +0100 Message-ID: <55A505190200007800090953@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> <55A3DADC0200007800090355@mail.emea.novell.com> <55A49C88.2060803@intel.com> <55A4F3300200007800090868@mail.emea.novell.com> <55A4E2D2.1070503@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55A4E2D2.1070503@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 14.07.15 at 12:22, wrote: > On 2015/7/14 17:32, Jan Beulich wrote: >>>>> On 14.07.15 at 07:22, wrote: >>>>> + 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++ ) >> >> Hmm, I would have thought you only care about either part of >> the just glued together map. >> >>>>> + 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. >> >> And I'm not disputing your intentions - I'm merely pointing out that >> afaics the code above doesn't match these intentions. In particular >> (as said) I don't see why you need to check low_mem_end < end. >> > > Before we probably relocate RAM, > > low_mem_end = hvm_info->low_mem_pgend << PAGE_SHIFT > > and the e820 entry specific to low memory, > > [e820[X].addr, end] > > Here, end = e820[X].addr + e820[X].size; > > Now low_mem_end = end. > > After that, low_mem_end < end. so if > > (low_mem_end > e820[X].addr && low_mem_end < end) is true, this means > that associated RAM entry is hitting, right? Then we need to revise this > entry as [e820[X].addr, low_mem_end], and compensate [end - low_mem_end] > to high memory. Anything I'm still wrong here? Ah, I think I see now what I misunderstood. Jan