Skip to contentSkip to conversation

kdump: Allow shrinking of kdump region to be overridden

PATCH In review Anton Blanchard 5 participants 12 messages Aug 25, 2010 – Mar 24, 2011

Subsystems (4)
PowerPC Live update kexec handover (KHO) kexec
lore
Share & tools
lore

Anton Blanchard

author
#0 lore raw

PATCH 1/2 kdump: Allow shrinking of kdump region to be overridden

On ppc64 the crashkernel region almost always overlaps an area of firmware. This works fine except when using the sysfs interface to reduce the kdump region. If we free the firmware area we are guaranteed to crash.

Rename free_reserved_phys_range to crash_free_reserved_phys_range and make it a weak function so we can override it.

Signed-off-by Anton Blanchard <[email protected]>

---

kernel/kexec.c +3 -2
Index: powerpc.git/kernel/kexec.c
===================================================================
@@ -1097,7 +1097,8 @@ size_t crash_get_memory_size(void)
 	return size;
 }
 
-static void free_reserved_phys_range(unsigned long begin, unsigned long end)
+void __weak crash_free_reserved_phys_range(unsigned long begin,
+					   unsigned long end)
 {
 	unsigned long addr;
 

@@ -1133,7 +1134,7 @@ int crash_shrink_memory(unsigned long ne
 	start = roundup(start, PAGE_SIZE);
 	end = roundup(start + new_size, PAGE_SIZE);
 
-	free_reserved_phys_range(end, crashk_res.end);
+	crash_free_reserved_phys_range(end, crashk_res.end);
 
 	if ((start == end) && (crashk_res.parent != NULL))
 		release_resource(&crashk_res);
include/linux/kexec.h +1 -0
Index: powerpc.git/include/linux/kexec.h
===================================================================
@@ -208,6 +208,7 @@ int __init parse_crashkernel(char *cmdli
 		unsigned long long *crash_size, unsigned long long *crash_base);
 int crash_shrink_memory(unsigned long new_size);
 size_t crash_get_memory_size(void);
+void crash_free_reserved_phys_range(unsigned long begin, unsigned long end);
 
 #else /* !CONFIG_KEXEC */
 struct pt_regs;
author
#1 lore raw

PATCH 2/2 powerpc: kdump: Override crash_free_reserved_phys_range to avoid freeing RTAS

The crashkernel region will almost always overlap RTAS. If we free the crashkernel region via "echo 0 > /sys/kernel/kexec_crash_size" then we will free RTAS and the machine will crash in confusing and exciting ways.

Override crash_free_reserved_phys_range and check for overlap with RTAS.

Signed-off-by Anton Blanchard <[email protected]>

---

arch/powerpc/kernel/crash_dump.c +33 -0
Index: powerpc.git/arch/powerpc/kernel/crash_dump.c
===================================================================
@@ -19,6 +19,7 @@
 #include <asm/prom.h>
 #include <asm/firmware.h>
 #include <asm/uaccess.h>
+#include <asm/rtas.h>
 
 #ifdef DEBUG
 #include <asm/udbg.h>

@@ -141,3 +142,35 @@ ssize_t copy_oldmem_page(unsigned long p
 
 	return csize;
 }
+
+#ifdef CONFIG_PPC_RTAS
+/*
+ * The crashkernel region will almost always overlap the RTAS region, so
+ * we have to be careful when shrinking the crashkernel region.
+ */
+void crash_free_reserved_phys_range(unsigned long begin, unsigned long end)
+{
+	unsigned long addr;
+	const u32 *basep, *sizep;
+	unsigned int rtas_start = 0, rtas_end = 0;
+
+	basep = of_get_property(rtas.dev, "linux,rtas-base", NULL);
+	sizep = of_get_property(rtas.dev, "rtas-size", NULL);
+
+	if (basep && sizep) {
+		rtas_start = *basep;
+		rtas_end = *basep + *sizep;
+	}
+
+	for (addr = begin; addr < end; addr += PAGE_SIZE) {
+		/* Does this page overlap with the RTAS region? */
+		if (addr <= rtas_end && ((addr + PAGE_SIZE) > rtas_start))
+			continue;
+
+		ClearPageReserved(pfn_to_page(addr >> PAGE_SHIFT));
+		init_page_count(pfn_to_page(addr >> PAGE_SHIFT));
+		free_page((unsigned long)__va(addr));
+		totalram_pages++;
+	}
+}
+#endif
#2 lore raw
In reply to Anton Blanchard
Quoted 3 lines · Anton Blanchard

On ppc64 the crashkernel region almost always overlaps an area of firmware. This works fine except when using the sysfs interface to reduce the kdump region. If we free the firmware area we are guaranteed to crash.

That is ppc64 bug. firmware should not be in the reserved region. Any random kernel like thing can be put in to that region at any valid address and the fact that shrinking the region frees your firmware means that using that region could also stomp your firmware (which I assume would be a bad thing).

So please fix the ppc64 reservation.

Eric

#3 lore raw
Quoted 11 lines · Eric W. Biederman
Quoted 3 lines · Anton Blanchard

On ppc64 the crashkernel region almost always overlaps an area of firmware. This works fine except when using the sysfs interface to reduce the kdump region. If we free the firmware area we are guaranteed to crash.

That is ppc64 bug. firmware should not be in the reserved region. Any random kernel like thing can be put in to that region at any valid address and the fact that shrinking the region frees your firmware means that using that region could also stomp your firmware (which I assume would be a bad thing).

The issue only happens while shrinking the region using sysfs interface. We already have checks in kexec for not to stomp over on the firmware overlap area while loading capture kernel. Currently we do a top-down allocation for the firmware region which means it sits at the top of the RMO, right in the middle of the crashdump region. We can not move the crashkernel region beyond firmware region because kernel needs its some of memory in RMO region.

Quoted 8 lines

So please fix the ppc64 reservation.

Eric
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://lists.ozlabs.org/listinfo/linuxppc-dev

#4 lore raw
Quoted 12 lines · Mahesh Jagannath Salgaonkar
Quoted 11 lines · Eric W. Biederman
Quoted 3 lines · Anton Blanchard

On ppc64 the crashkernel region almost always overlaps an area of firmware. This works fine except when using the sysfs interface to reduce the kdump region. If we free the firmware area we are guaranteed to crash.

That is ppc64 bug. firmware should not be in the reserved region. Any random kernel like thing can be put in to that region at any valid address and the fact that shrinking the region frees your firmware means that using that region could also stomp your firmware (which I assume would be a bad thing).

Quoted 7 lines

The issue only happens while shrinking the region using sysfs interface. We already have checks in kexec for not to stomp over on the firmware overlap area while loading capture kernel. Currently we do a top-down allocation for the firmware region which means it sits at the top of the RMO, right in the middle of the crashdump region. We can not move the crashkernel region beyond firmware region because kernel needs its some of memory in RMO region.

The crashkernel region is specified via kernel cmdline, so why not just drop a failure when it overlaps with RMO region? Am I missing something?

Thanks.

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

author
#5 lore raw

Hi,

Quoted 3 lines

The crashkernel region is specified via kernel cmdline, so why not just drop a failure when it overlaps with RMO region? Am I missing something?

Unfortunately a ppc64 kernel requires a chunk of RMO memory. We would need the ability to specify multiple crashkernel regions - about 32MB in the RMO and the rest can be anywhere. That sounds pretty fragile for a user to configure successfully on the cmdline.

Thats why the ppc64 crashkernel region begins mid way through the RMO region. It means both kernels get a chunk of RMO and we only have to deal with one crashkernel reservation in all the tools and documentation.

Anton

#6 lore raw
Quoted 12 lines · Anton Blanchard

Hi,

Quoted 3 lines

The crashkernel region is specified via kernel cmdline, so why not just drop a failure when it overlaps with RMO region? Am I missing something?

Unfortunately a ppc64 kernel requires a chunk of RMO memory. We would need the ability to specify multiple crashkernel regions - about 32MB in the RMO and the rest can be anywhere. That sounds pretty fragile for a user to configure successfully on the cmdline.

Quoted 5 lines

Thats why the ppc64 crashkernel region begins mid way through the RMO region. It means both kernels get a chunk of RMO and we only have to deal with one crashkernel reservation in all the tools and documentation.

So, when I specify 128M in cmdline, 32M of them are RMO, and the rest 96M are normal memory? And when I want to free all of them, actually the 32M RMO will never be freed?

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#7 lore raw
Quoted 19 lines · Américo Wang
Quoted 12 lines · Anton Blanchard

Hi,

Quoted 3 lines

The crashkernel region is specified via kernel cmdline, so why not just drop a failure when it overlaps with RMO region? Am I missing something?

Unfortunately a ppc64 kernel requires a chunk of RMO memory. We would need the ability to specify multiple crashkernel regions - about 32MB in the RMO and the rest can be anywhere. That sounds pretty fragile for a user to configure successfully on the cmdline.

Quoted 5 lines

Thats why the ppc64 crashkernel region begins mid way through the RMO region. It means both kernels get a chunk of RMO and we only have to deal with one crashkernel reservation in all the tools and documentation.

Quoted 3 lines

So, when I specify 128M in cmdline, 32M of them are RMO, and the rest 96M are normal memory? And when I want to free all of them, actually the 32M RMO will never be freed?

In ppc64 implementation the crashkernel region begins mid way through the RMO and spans across into the normal memory. For e.g. on power system with 128M RMO size, when user specifies 128M in cmdline, the crashkernel starts from default offset 64M through 192M. Which means 64M in RMO region and rest beyond RMO.

<-------------------RMO--------------->

                             <---rtas-->
0                    64M               128M                     End
|=====================|=================|===========|============|
           Crash_start|<------------128M----------->| Crash End

During free we do free all of them including RMO region. But since the rtas region is always on top of RMO, crashkernel memory overlaps rtas region and we endup freeing that even, which is causing the crash.

Quoted 6 lines

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Signature · 1 line
Signed-off-by: Mahesh Salgaonkar <[email protected]>
#8 lore raw
Quoted 5 lines · Mahesh J Salgaonkar

During free we do free all of them including RMO region. But since the rtas region is always on top of RMO, crashkernel memory overlaps rtas region and we endup freeing that even, which is causing the crash.

Okay, but with this patch applied, we will just ignore rtas region, right? Thus, when I echo 0 to free all the 128M crashkernel memory, the final result will be 32M left, which means crash_size will still show 32M. This looks odd.

How about skipping the 32M as a whole? I mean once the region being freed has overlap with this rtas region, skip the whole rtas region, and let crash_size
show 0?

Thanks.

#9 lore raw
Quoted 9 lines · Américo Wang
Quoted 5 lines · Mahesh J Salgaonkar

During free we do free all of them including RMO region. But since the rtas region is always on top of RMO, crashkernel memory overlaps rtas region and we endup freeing that even, which is causing the crash.

Okay, but with this patch applied, we will just ignore rtas region, right?

Correct.

Quoted 8 lines

Thus, when I echo 0 to free all the 128M crashkernel memory, the final result will be 32M left, which means crash_size will still show 32M. This looks odd.

How about skipping the 32M as a whole? I mean once the region being freed has overlap with this rtas region, skip the whole rtas region, and let crash_size
show 0?

The existing code from crash_shrink_memory() function reduces the crash size to 0 when echo'ed 0. I did test this patchset and verified that /sys/kernel/kexec_crash_size show 0 value.

Thanks,
-Mahesh.

Quoted 5 lines

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Signature · 1 line
Signed-off-by: Mahesh Salgaonkar <[email protected]>
Acked-by
#10 lore raw
Quoted 17 lines · Mahesh J Salgaonkar
Quoted 11 lines · Américo Wang
Quoted 7 lines · Mahesh J Salgaonkar

During free we do free all of them including RMO region. But since the rtas region is always on top of RMO, crashkernel memory overlaps rtas region and we endup freeing that even, which is causing the crash.

Okay, but with this patch applied, we will just ignore rtas region, right?

Correct.

Quoted 4 lines

Thus, when I echo 0 to free all the 128M crashkernel memory, the final result will be 32M left, which means crash_size will still show 32M. This looks odd.

Quoted 7 lines
Quoted 4 lines

How about skipping the 32M as a whole? I mean once the region being freed has overlap with this rtas region, skip the whole rtas region, and let crash_size
show 0?

The existing code from crash_shrink_memory() function reduces the crash size to 0 when echo'ed 0. I did test this patchset and verified that /sys/kernel/kexec_crash_size show 0 value.

Oh, ok.

Acked-by WANG Cong <[email protected]>

Thanks.

#11 lore raw
Quoted 13 lines · WANG Cong
Quoted 11 lines · Mahesh J Salgaonkar
Quoted 2 lines

Okay, but with this patch applied, we will just ignore rtas region, right?

Correct.

Quoted 8 lines

Thus, when I echo 0 to free all the 128M crashkernel memory, the final result will be 32M left, which means crash_size will still show 32M. This looks odd.

How about skipping the 32M as a whole? I mean once the region being freed has overlap with this rtas region, skip the whole rtas region, and let crash_size
show 0?

Quoted 7 lines
Quoted 3 lines

The existing code from crash_shrink_memory() function reduces the crash size to 0 when echo'ed 0. I did test this patchset and verified that /sys/kernel/kexec_crash_size show 0 value.

Oh, ok.

Acked-by WANG Cong <[email protected]>

So Eric, what's the right approach to get that merged ? This is a bug gating an important delivery for us, and the patch doesn't appear terribly invasive ? :-)

I can send it to Linus myself if you prefer and give me your Ack.

Cheers,
Ben.

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec