* [PATCH] cleanup for __start_xen()
@ 2009-11-30 6:25 Xiao Guangrong
2009-11-30 17:39 ` Ian Jackson
0 siblings, 1 reply; 8+ messages in thread
From: Xiao Guangrong @ 2009-11-30 6:25 UTC (permalink / raw)
To: Keir Fraser, xen-devel
Remove repetitive judgment in __start_xen()
Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
diff --git a/a/xen/arch/x86/setup.c b/b/xen/arch/x86/setup.c
index d87d082..563e46a 100644
--- a/a/xen/arch/x86/setup.c
+++ b/b/xen/arch/x86/setup.c
@@ -729,7 +729,7 @@ void __init __start_xen(unsigned long mbi_p)
#endif
/* Is the region suitable for relocating the multiboot modules? */
- if ( !initial_images_start && (s < e) &&
+ if ( !initial_images_start &&
((e-s) >= (modules_length+modules_headroom)) )
{
initial_images_end = e;
@@ -747,7 +747,7 @@ void __init __start_xen(unsigned long mbi_p)
}
}
- if ( !kexec_crash_area.start && (s < e) &&
+ if ( !kexec_crash_area.start &&
((e-s) >= kexec_crash_area.size) )
{
e = (e - kexec_crash_area.size) & PAGE_MASK;
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] cleanup for __start_xen()
2009-11-30 6:25 [PATCH] cleanup for __start_xen() Xiao Guangrong
@ 2009-11-30 17:39 ` Ian Jackson
2009-11-30 17:42 ` Ian Jackson
0 siblings, 1 reply; 8+ messages in thread
From: Ian Jackson @ 2009-11-30 17:39 UTC (permalink / raw)
To: Xiao Guangrong; +Cc: xen-devel, Keir Fraser
Xiao Guangrong writes ("[Xen-devel] [PATCH] cleanup for __start_xen()"):
> Remove repetitive judgment in __start_xen()
>
> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
>
> diff --git a/a/xen/arch/x86/setup.c b/b/xen/arch/x86/setup.c
> index d87d082..563e46a 100644
> --- a/a/xen/arch/x86/setup.c
> +++ b/b/xen/arch/x86/setup.c
> @@ -729,7 +729,7 @@ void __init __start_xen(unsigned long mbi_p)
> #endif
>
> /* Is the region suitable for relocating the multiboot modules? */
> - 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.
Ian.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cleanup for __start_xen()
2009-11-30 17:39 ` Ian Jackson
@ 2009-11-30 17:42 ` Ian Jackson
2009-11-30 18:52 ` Keir Fraser
0 siblings, 1 reply; 8+ messages in thread
From: Ian Jackson @ 2009-11-30 17:42 UTC (permalink / raw)
To: Xiao Guangrong, Keir Fraser, xen-devel
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.
Ian.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cleanup for __start_xen()
2009-11-30 17:42 ` Ian Jackson
@ 2009-11-30 18:52 ` Keir Fraser
2009-12-01 1:00 ` Xiao Guangrong
0 siblings, 1 reply; 8+ messages in thread
From: Keir Fraser @ 2009-11-30 18:52 UTC (permalink / raw)
To: Ian Jackson, Xiao Guangrong, xen-devel
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...
-- Keir
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cleanup for __start_xen()
2009-11-30 18:52 ` Keir Fraser
@ 2009-12-01 1:00 ` Xiao Guangrong
2009-12-01 1:16 ` Xiao Guangrong
0 siblings, 1 reply; 8+ messages in thread
From: Xiao Guangrong @ 2009-12-01 1:00 UTC (permalink / raw)
To: Keir Fraser; +Cc: xen-devel, Ian Jackson
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 ... */
......
}
Thanks,
Xiao
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cleanup for __start_xen()
2009-12-01 1:00 ` Xiao Guangrong
@ 2009-12-01 1:16 ` Xiao Guangrong
2009-12-01 9:42 ` Tim Deegan
0 siblings, 1 reply; 8+ messages in thread
From: Xiao Guangrong @ 2009-12-01 1:16 UTC (permalink / raw)
To: Keir Fraser; +Cc: xen-devel, Ian Jackson
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...
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cleanup for __start_xen()
2009-12-01 1:16 ` Xiao Guangrong
@ 2009-12-01 9:42 ` Tim Deegan
2009-12-01 12:07 ` Keir Fraser
0 siblings, 1 reply; 8+ messages in thread
From: Tim Deegan @ 2009-12-01 9:42 UTC (permalink / raw)
To: Xiao Guangrong; +Cc: xen-devel, Ian Jackson, Keir Fraser
At 01:16 +0000 on 01 Dec (1259630197), Xiao Guangrong wrote:
> e is moved in [e-(modules_length+modules_headroom), e] range,
> so, s is not overrun e too...
OK, but:
- this is not obvious (hence this thread);
- even Keir makes mistakes here from time to time; and
- it's a single test on code that runs at start of day
so maybe we should just leave it as it was?
Cheers,
Tim.
--
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cleanup for __start_xen()
2009-12-01 9:42 ` Tim Deegan
@ 2009-12-01 12:07 ` Keir Fraser
0 siblings, 0 replies; 8+ messages in thread
From: Keir Fraser @ 2009-12-01 12:07 UTC (permalink / raw)
To: Tim Deegan, Xiao Guangrong; +Cc: xen-devel, Ian Jackson
On 01/12/2009 09:42, "Tim Deegan" <Tim.Deegan@eu.citrix.com> wrote:
> At 01:16 +0000 on 01 Dec (1259630197), Xiao Guangrong wrote:
>> e is moved in [e-(modules_length+modules_headroom), e] range,
>> so, s is not overrun e too...
>
> OK, but:
> - this is not obvious (hence this thread);
> - even Keir makes mistakes here from time to time; and
> - it's a single test on code that runs at start of day
> so maybe we should just leave it as it was?
I'll put it back and fix my other pre-existing mistake also.
-- Keir
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-12-01 12:07 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-11-30 6:25 [PATCH] cleanup for __start_xen() Xiao Guangrong
2009-11-30 17:39 ` Ian Jackson
2009-11-30 17:42 ` Ian Jackson
2009-11-30 18:52 ` Keir Fraser
2009-12-01 1:00 ` Xiao Guangrong
2009-12-01 1:16 ` Xiao Guangrong
2009-12-01 9:42 ` Tim Deegan
2009-12-01 12:07 ` Keir Fraser
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.