From mboxrd@z Thu Jan  1 00:00:00 1970
From: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Subject: Re: [PATCH] cleanup for __start_xen()
Date: Tue, 01 Dec 2009 09:16:37 +0800
Message-ID: <4B146E75.7000802@cn.fujitsu.com>
References: <C739C4F9.30C3%keir.fraser@eu.citrix.com>
	<4B146AAA.40508@cn.fujitsu.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <4B146AAA.40508@cn.fujitsu.com>
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Keir Fraser <keir.fraser@eu.citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>, Ian Jackson <Ian.Jackson@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org



Xiao Guangrong wrote:
> 
> Keir Fraser wrote:
>> On 30/11/2009 17:42, "Ian Jackson" <Ian.Jackson@eu.citrix.com> wrote:
>>
>>> I wrote:
>>>> Xiao Guangrong writes ("[Xen-devel] [PATCH] cleanup for __start_xen()"):
>>>>> -        if ( !initial_images_start && (s < e) &&
>>>>> +        if ( !initial_images_start &&
>>>> This is wrong.  s and e are uint64_t so if !(s < e), (e-s) will be
>>>> large and positive.
>>> I see this has already been applied (20523).  It should be reverted, I
>>> think.
>> None of the if() blocks in the loop will make e<s, as that would imply that
>> the block had allocated itself a chunk of memory that starts below s. So it
>> is actually safe to remove the checks, as we know e>=s. But now I look at it
>> I think I broke the module-relocation block some time ago -- it ends up with
>> 'e' being too large by modules_headroom. :-( Will look into that more
>> tomorrow...
>>
> 
> I thinks remove this judgment is very safe, because we have judge it at the 
> begin of this loop:
> 
> for ( i = boot_e820.nr_map-1; i >= 0; i-- )
>     {
>         uint64_t s, e, mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
> 	
> 	<snip>
> 
> 	if ( (boot_e820.map[i].type != E820_RAM) || (s >= e) )	/* NOTICE HERE*/
>             continue;
> 	
> 	/* it must s < e while run below code, not need check it again ... */
> 	......
> }
> 

And after below if():
 if ( !initial_images_start && (s < e) &&
             ((e-s) >= (modules_length+modules_headroom)) )
        {
            initial_images_end = e;
            e = (e - modules_length) & PAGE_MASK;
            initial_images_start = e;
            e -= modules_headroom;
            initial_images_base = e;
            e += modules_length + modules_headroom;
            for ( j = mbi->mods_count-1; j >= 0; j-- )
            {
                e -= mod[j].mod_end - mod[j].mod_start;
                move_memory(e, mod[j].mod_start, mod[j].mod_end);
                mod[j].mod_end += e - mod[j].mod_start;
                mod[j].mod_start = e;
            }
        }
}

e is moved in [e-(modules_length+modules_headroom), e] range,
so, s is not overrun e too...