2b034922 breaks kexec on ARM

[Simon Horman]

On Mon, Nov 07, 2011 at 09:47:55PM +0000, Will Deacon wrote:
> On Mon, Nov 07, 2011 at 08:55:43PM +0000, Lei Wen wrote:
> > Apologize for messing things up...
> > But we have make in-depth testing over that patch already in our local
> > code base...
> 
> Hmm, well even if we put Russell's concerns aside for a moment, the code
> does:
> 
> 		/* Loading another kernel to reboot into */
>                 if ((flags & KEXEC_ON_CRASH) == 0)
>                         result = kimage_normal_alloc(&image, entry,
>                                                         nr_segments, segments);
>                 /* Loading another kernel to switch to if this one crashes */
>                 else if (flags & KEXEC_ON_CRASH) {
>                         /* Free any current crash dump kernel before
>                          * we corrupt it.
>                          */
>                         kimage_free(xchg(&kexec_crash_image, NULL));
>                         result = kimage_crash_alloc(&image, entry,
>                                                      nr_segments, segments);
>                         crash_map_reserved_pages();
>                 }
>                 if (result)
>                         goto out;
> 
>                 if (flags & KEXEC_PRESERVE_CONTEXT)
>                         image->preserve_context = 1;
>                 result = machine_kexec_prepare(image);
> 
> At this point the image may be fresh out of kimage_normal_malloc, which sets
> image->head to 0 (i.e. a NULL indirection page). So when machine_kexec_prepare
> tries to determine the page list, it's NULL and relocate_new_kernel will do:
> 
> 	/*
> 	 * If there is no indirection page (we are doing crashdumps)
> 	 * skip any relocation.
> 	 */
> 	cmp	r0, #0
> 	beq	2f
> 
> and not actually relocate the kernel sections at all! In fact, looking back at
> kernel/kexec.c, right after calling machine_kexec_prepare it goes and loads
> the segments:
> 
>                 if (result)
>                         goto out;
> 
>                 for (i = 0; i < nr_segments; i++) {
>                         result = kimage_load_segment(image, &image->segment[i]);
>                         if (result)
>                                 goto out;
>                 }
> 
> so only at this point is it safe to start setting up the page list (as the
> image entries are now populated).
> 
> So why is this passing your in-depth testing? It could be that:
> 
>   (1) You're only testing crashdumps, which don't need the indirection page.
>   (2) You don't have the poison_init_mem patches in arch/arm/mm/init.c and
>       actually end up using the old kernel that's still kicking around in
>       memory.
>   (3) Your testing isn't that in-depth after all.
> 
> It's probably a combination of all three, but the fact remains that this patch
> is breaking things so you should probably roll another version if you really
> need to move parts of the image setup earlier on.

Hi Will, Hi All,

FWIW, as I acked this patch in the first place, I think that this patch
should be reverted and a fresh version considered latter if appropriate.