2b034922 breaks kexec on ARM

2b034922 breaks kexec on ARM

[Will Deacon]

Hello,

I've just finished rebasing and reworking my kexec patches so that they sit
nicely on top of Russell's recent reset work. The only problem is that
commit 2b034922 ("ARM: 7098/1: kdump: copy kernel relocation code at the
kexec prepare stage") is causing kexec to fail.

The symptoms are that the .init.text section is not loaded as part of the
new kernel image, so when we try to do the SMP/UP fixups we hit a whole sea
of poison left there by the previous kernel.

So my guess is that machine_kexec_prepare *is* too early for preparing the
reboot_code_buffer and, unless anybody has a good reason not to, I'd like to
revert the patch causing these problems.

Thanks,

Will

2b034922 breaks kexec on ARM

[Russell King - ARM Linux]

On Mon, Nov 07, 2011 at 06:51:25PM +0000, Will Deacon wrote:
> Hello,
> 
> I've just finished rebasing and reworking my kexec patches so that they sit
> nicely on top of Russell's recent reset work. The only problem is that
> commit 2b034922 ("ARM: 7098/1: kdump: copy kernel relocation code at the
> kexec prepare stage") is causing kexec to fail.
> 
> The symptoms are that the .init.text section is not loaded as part of the
> new kernel image, so when we try to do the SMP/UP fixups we hit a whole sea
> of poison left there by the previous kernel.
> 
> So my guess is that machine_kexec_prepare *is* too early for preparing the
> reboot_code_buffer and, unless anybody has a good reason not to, I'd like to
> revert the patch causing these problems.

Hmm, having looked at kernel/kexec.c, this patch is clearly wrong.
There are two kernels which can be loaded into kexec - a crash kernel
and a default kernel.  These are kept entirely separately (and the
per-kernel context is stored in 'image').

However, this patch makes machine_kexec_prepare() store into some
_global_ variables, which means if you've loaded a crash kernel
followed by a default kernel, the values for the crash kernel will
be corrupted.

So yes, I think a revert of this patch is the right thing.

I think the only valid thing its doing is moving the copy of the
reboot code earlier, but I don't see the value of moving just that.

2b034922 breaks kexec on ARM

[Will Deacon]

On Mon, Nov 07, 2011 at 07:03:15PM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 07, 2011 at 06:51:25PM +0000, Will Deacon wrote:
> > Hello,
> > 
> > I've just finished rebasing and reworking my kexec patches so that they sit
> > nicely on top of Russell's recent reset work. The only problem is that
> > commit 2b034922 ("ARM: 7098/1: kdump: copy kernel relocation code at the
> > kexec prepare stage") is causing kexec to fail.
> > 
> > The symptoms are that the .init.text section is not loaded as part of the
> > new kernel image, so when we try to do the SMP/UP fixups we hit a whole sea
> > of poison left there by the previous kernel.
> > 
> > So my guess is that machine_kexec_prepare *is* too early for preparing the
> > reboot_code_buffer and, unless anybody has a good reason not to, I'd like to
> > revert the patch causing these problems.
> 
> Hmm, having looked at kernel/kexec.c, this patch is clearly wrong.
> There are two kernels which can be loaded into kexec - a crash kernel
> and a default kernel.  These are kept entirely separately (and the
> per-kernel context is stored in 'image').
> 
> However, this patch makes machine_kexec_prepare() store into some
> _global_ variables, which means if you've loaded a crash kernel
> followed by a default kernel, the values for the crash kernel will
> be corrupted.
> 
> So yes, I think a revert of this patch is the right thing.

Great. Would you like me to submit the revert to the patch system or are you
happy just to do it using git?

> I think the only valid thing its doing is moving the copy of the
> reboot code earlier, but I don't see the value of moving just that.

Neither can I. It seems to be happy enough with the patch reverted, so if it
ain't broke...

Will

2b034922 breaks kexec on ARM

[Russell King - ARM Linux]

On Mon, Nov 07, 2011 at 07:14:53PM +0000, Will Deacon wrote:
> On Mon, Nov 07, 2011 at 07:03:15PM +0000, Russell King - ARM Linux wrote:
> > Hmm, having looked at kernel/kexec.c, this patch is clearly wrong.
> > There are two kernels which can be loaded into kexec - a crash kernel
> > and a default kernel.  These are kept entirely separately (and the
> > per-kernel context is stored in 'image').
> > 
> > However, this patch makes machine_kexec_prepare() store into some
> > _global_ variables, which means if you've loaded a crash kernel
> > followed by a default kernel, the values for the crash kernel will
> > be corrupted.
> > 
> > So yes, I think a revert of this patch is the right thing.
> 
> Great. Would you like me to submit the revert to the patch system or are you
> happy just to do it using git?

I've reverted it myself (see the tip of the fixes branch).  It's slightly
annoying that the branch was only pulled last night.

> > I think the only valid thing its doing is moving the copy of the
> > reboot code earlier, but I don't see the value of moving just that.
> 
> Neither can I. It seems to be happy enough with the patch reverted, so if it
> ain't broke...

I guess this is where the problems of poor commit descriptions crop up.
We don't know _why_ the original author of the patch wanted to make the
change - we don't know if they were seeing a problem.

2b034922 breaks kexec on ARM

[Lei Wen]

On Tue, Nov 8, 2011 at 3:20 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Nov 07, 2011 at 07:14:53PM +0000, Will Deacon wrote:
>> On Mon, Nov 07, 2011 at 07:03:15PM +0000, Russell King - ARM Linux wrote:
>> > Hmm, having looked at kernel/kexec.c, this patch is clearly wrong.
>> > There are two kernels which can be loaded into kexec - a crash kernel
>> > and a default kernel. ?These are kept entirely separately (and the
>> > per-kernel context is stored in 'image').
>> >
>> > However, this patch makes machine_kexec_prepare() store into some
>> > _global_ variables, which means if you've loaded a crash kernel
>> > followed by a default kernel, the values for the crash kernel will
>> > be corrupted.
>> >
>> > So yes, I think a revert of this patch is the right thing.
>>
>> Great. Would you like me to submit the revert to the patch system or are you
>> happy just to do it using git?
>
> I've reverted it myself (see the tip of the fixes branch). ?It's slightly
> annoying that the branch was only pulled last night.
>
>> > I think the only valid thing its doing is moving the copy of the
>> > reboot code earlier, but I don't see the value of moving just that.
>>
>> Neither can I. It seems to be happy enough with the patch reverted, so if it
>> ain't broke...
>
> I guess this is where the problems of poor commit descriptions crop up.
> We don't know _why_ the original author of the patch wanted to make the
> change - we don't know if they were seeing a problem.
>

Apologize for messing things up...
But we have make in-depth testing over that patch already in our local
code base...
Does this problem happens for the kernel is running on a SMP soc? Our current
testing haven't extend to SMP one, but only have test on non-smp.

Best regards,
Lei

2b034922 breaks kexec on ARM

[Russell King - ARM Linux]

On Tue, Nov 08, 2011 at 04:55:43AM +0800, Lei Wen wrote:
> On Tue, Nov 8, 2011 at 3:20 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Nov 07, 2011 at 07:14:53PM +0000, Will Deacon wrote:
> >> On Mon, Nov 07, 2011 at 07:03:15PM +0000, Russell King - ARM Linux wrote:
> >> > Hmm, having looked at kernel/kexec.c, this patch is clearly wrong.
> >> > There are two kernels which can be loaded into kexec - a crash kernel
> >> > and a default kernel. ?These are kept entirely separately (and the
> >> > per-kernel context is stored in 'image').
> >> >
> >> > However, this patch makes machine_kexec_prepare() store into some
> >> > _global_ variables, which means if you've loaded a crash kernel
> >> > followed by a default kernel, the values for the crash kernel will
> >> > be corrupted.
> >> >
> >> > So yes, I think a revert of this patch is the right thing.
> >>
> >> Great. Would you like me to submit the revert to the patch system or are you
> >> happy just to do it using git?
> >
> > I've reverted it myself (see the tip of the fixes branch). ?It's slightly
> > annoying that the branch was only pulled last night.
> >
> >> > I think the only valid thing its doing is moving the copy of the
> >> > reboot code earlier, but I don't see the value of moving just that.
> >>
> >> Neither can I. It seems to be happy enough with the patch reverted, so if it
> >> ain't broke...
> >
> > I guess this is where the problems of poor commit descriptions crop up.
> > We don't know _why_ the original author of the patch wanted to make the
> > change - we don't know if they were seeing a problem.
> >
> 
> Apologize for messing things up...
> But we have make in-depth testing over that patch already in our local
> code base...
> Does this problem happens for the kernel is running on a SMP soc? Our current
> testing haven't extend to SMP one, but only have test on non-smp.

It doesn't matter - as I've pointed out, the change is _fundamentally_
wrong because you're storing state in global variables which refers to
one of two possible images which may be loaded.

That can not work properly for every case.

And you haven't explained why you want the change.

2b034922 breaks kexec on ARM

[Lei Wen]

Hi Russel,

On Tue, Nov 8, 2011 at 5:03 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Nov 08, 2011 at 04:55:43AM +0800, Lei Wen wrote:
>> On Tue, Nov 8, 2011 at 3:20 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Mon, Nov 07, 2011 at 07:14:53PM +0000, Will Deacon wrote:
>> >> On Mon, Nov 07, 2011 at 07:03:15PM +0000, Russell King - ARM Linux wrote:
>> >> > Hmm, having looked at kernel/kexec.c, this patch is clearly wrong.
>> >> > There are two kernels which can be loaded into kexec - a crash kernel
>> >> > and a default kernel. ?These are kept entirely separately (and the
>> >> > per-kernel context is stored in 'image').
>> >> >
>> >> > However, this patch makes machine_kexec_prepare() store into some
>> >> > _global_ variables, which means if you've loaded a crash kernel
>> >> > followed by a default kernel, the values for the crash kernel will
>> >> > be corrupted.
>> >> >
>> >> > So yes, I think a revert of this patch is the right thing.
>> >>
>> >> Great. Would you like me to submit the revert to the patch system or are you
>> >> happy just to do it using git?
>> >
>> > I've reverted it myself (see the tip of the fixes branch). ?It's slightly
>> > annoying that the branch was only pulled last night.
>> >
>> >> > I think the only valid thing its doing is moving the copy of the
>> >> > reboot code earlier, but I don't see the value of moving just that.
>> >>
>> >> Neither can I. It seems to be happy enough with the patch reverted, so if it
>> >> ain't broke...
>> >
>> > I guess this is where the problems of poor commit descriptions crop up.
>> > We don't know _why_ the original author of the patch wanted to make the
>> > change - we don't know if they were seeing a problem.
>> >
>>
>> Apologize for messing things up...
>> But we have make in-depth testing over that patch already in our local
>> code base...
>> Does this problem happens for the kernel is running on a SMP soc? Our current
>> testing haven't extend to SMP one, but only have test on non-smp.
>
> It doesn't matter - as I've pointed out, the change is _fundamentally_
> wrong because you're storing state in global variables which refers to
> one of two possible images which may be loaded.

I still cannot get understand your point there... Why image->start would refer
to the default kernel? You means kexec load two kernel at the same time?

>
> That can not work properly for every case.
>
> And you haven't explained why you want the change.
The original intension for this posting is for we want to have a chance to
launch the crash kernel, even when default kernel is hanging, which
means it has no chance to trigger the panic path that lead to load the
crash kernel.

Sorry for not including detailed explaination, I would take care of
this for latter posting patch...

Thanks,
Lei

2b034922 breaks kexec on ARM

[Will Deacon]

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.

Will

2b034922 breaks kexec on ARM

[Russell King - ARM Linux]

On Tue, Nov 08, 2011 at 05:15:13AM +0800, Lei Wen wrote:
> Hi Russel,
> 
> On Tue, Nov 8, 2011 at 5:03 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, Nov 08, 2011 at 04:55:43AM +0800, Lei Wen wrote:
> >> On Tue, Nov 8, 2011 at 3:20 AM, Russell King - ARM Linux
> >> <linux@arm.linux.org.uk> wrote:
> >> > On Mon, Nov 07, 2011 at 07:14:53PM +0000, Will Deacon wrote:
> >> >> On Mon, Nov 07, 2011 at 07:03:15PM +0000, Russell King - ARM Linux wrote:
> >> >> > Hmm, having looked at kernel/kexec.c, this patch is clearly wrong.
> >> >> > There are two kernels which can be loaded into kexec - a crash kernel
> >> >> > and a default kernel. ?These are kept entirely separately (and the
> >> >> > per-kernel context is stored in 'image').
> >> >> >
> >> >> > However, this patch makes machine_kexec_prepare() store into some
> >> >> > _global_ variables, which means if you've loaded a crash kernel
> >> >> > followed by a default kernel, the values for the crash kernel will
> >> >> > be corrupted.
> >> >> >
> >> >> > So yes, I think a revert of this patch is the right thing.
> >> >>
> >> >> Great. Would you like me to submit the revert to the patch system or are you
> >> >> happy just to do it using git?
> >> >
> >> > I've reverted it myself (see the tip of the fixes branch). ?It's slightly
> >> > annoying that the branch was only pulled last night.
> >> >
> >> >> > I think the only valid thing its doing is moving the copy of the
> >> >> > reboot code earlier, but I don't see the value of moving just that.
> >> >>
> >> >> Neither can I. It seems to be happy enough with the patch reverted, so if it
> >> >> ain't broke...
> >> >
> >> > I guess this is where the problems of poor commit descriptions crop up.
> >> > We don't know _why_ the original author of the patch wanted to make the
> >> > change - we don't know if they were seeing a problem.
> >> >
> >>
> >> Apologize for messing things up...
> >> But we have make in-depth testing over that patch already in our local
> >> code base...
> >> Does this problem happens for the kernel is running on a SMP soc? Our current
> >> testing haven't extend to SMP one, but only have test on non-smp.
> >
> > It doesn't matter - as I've pointed out, the change is _fundamentally_
> > wrong because you're storing state in global variables which refers to
> > one of two possible images which may be loaded.
> 
> I still cannot get understand your point there... Why image->start would refer
> to the default kernel? You means kexec load two kernel at the same time?

I've now stated the problem twice.  Let me re-state it again:

You are storing state in global variables which refers to one of two
possible images which may be loaded.

Take a moment to _think_ about what effect this sequence of events has:

1. loading a crash kernel has with image->type = KEXEC_TYPE_CRASH.
   This calls machine_kexec_prepare() for this kernel, which with your
   patch ends up writing to these variables:
	kexec_start_address
	kexec_indirection_page
	kexec_mach_type
	kexec_boot_atags

   this will be loaded and retained in memory.

2. loading a normal kexec image.
   Again, this calls machine_kexec_prepare() for this kernel, which with
   your patch ends up writing to these variables:
	kexec_start_address
	kexec_indirection_page
	kexec_mach_type
	kexec_boot_atags

3. you experience a crash at this point, and the kernel which is attempted
   to be booted is the one with image->type = KEXEC_TYPE_CRASH.  However,
	kexec_start_address
	kexec_indirection_page
	kexec_mach_type
	kexec_boot_atags
   are all set for the normal image, not the kexec image.

See - storing this state in global variables does _NOT_ work if it has to
persist to the point where the kexec actually happens.

> The original intension for this posting is for we want to have a chance to
> launch the crash kernel, even when default kernel is hanging, which
> means it has no chance to trigger the panic path that lead to load the
> crash kernel.

Right.  That's a better explanation.

It doesn't get us away from the fact that the patch is _technically_
wrong though for the point I make above.  Those variables must stay
inside machine_kexec() so they are correctly initialized for the
kernel being booted.

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.

2b034922 breaks kexec on ARM

[Russell King - ARM Linux]

On Tue, Nov 08, 2011 at 09:59:22AM +0900, Simon Horman wrote:
> 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.

I've already reverted it - it's in my 'fixes' branch.

Thanks.

2b034922 breaks kexec on ARM

[Simon Horman]

On Tue, Nov 08, 2011 at 09:01:55AM +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 08, 2011 at 09:59:22AM +0900, Simon Horman wrote:
> > 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.
> 
> I've already reverted it - it's in my 'fixes' branch.

Thanks

2b034922 breaks kexec on ARM

[Lei Wen]

On Tue, Nov 8, 2011 at 5:31 PM, Simon Horman <horms@verge.net.au> wrote:
> On Tue, Nov 08, 2011 at 09:01:55AM +0000, Russell King - ARM Linux wrote:
>> On Tue, Nov 08, 2011 at 09:59:22AM +0900, Simon Horman wrote:
>> > 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.
>>
>> I've already reverted it - it's in my 'fixes' branch.
>
> Thanks
>

Thanks all, I would dig into it to see how to handle it correctly.

Best regards,
Lei