From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Chen, Tiejun" <tiejun.chen@intel.com>
Subject: Re: [v7][PATCH 07/16] hvmloader/e820: construct guest
	e820 table
Date: Tue, 14 Jul 2015 18:22:10 +0800
Message-ID: <55A4E2D2.1070503@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>
	<55A49C88.2060803@intel.com>
	<55A4F3300200007800090868@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <55A4F3300200007800090868@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: Wei Liu <wei.liu2@citrix.com>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>, xen-devel@lists.xen.org, Keir Fraser <keir@xen.org>
List-Id: xen-devel@lists.xenproject.org

On 2015/7/14 17:32, Jan Beulich wrote:
>>>> On 14.07.15 at 07:22, <tiejun.chen@intel.com> 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?

Thanks
Tiejun