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;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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... _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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.] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel