[PATCH 1/2] powerpc/crashdump: Fix issues with kexec and 36bit physical addr

[PATCH 1/2] powerpc/crashdump: Fix issues with kexec and 36bit physical addr

[Matthew McClintock]

Fix sizes of variables so correct values are exported via /proc.
Cast variable in comparison to avoid compiler error.

Signed-off-by: Matthew McClintock <msm@freescale.com>
---
 arch/powerpc/kernel/crash_dump.c    |    4 ++--
 arch/powerpc/kernel/machine_kexec.c |    4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/crash_dump.c b/arch/powerpc/kernel/crash_dump.c
index 5fb667a..71cadde 100644
--- a/arch/powerpc/kernel/crash_dump.c
+++ b/arch/powerpc/kernel/crash_dump.c
@@ -128,9 +128,9 @@ ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
 	if (!csize)
 		return 0;
 
-	csize = min(csize, PAGE_SIZE);
+	csize = min(csize, (size_t)PAGE_SIZE);
 
-	if (pfn < max_pfn) {
+	if ((min_low_pfn < pfn) && (pfn < max_pfn)) {
 		vaddr = __va(pfn << PAGE_SHIFT);
 		csize = copy_oldmem_vaddr(vaddr, buf, csize, offset, userbuf);
 	} else {
diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
index bb3d893..ec7f054 100644
--- a/arch/powerpc/kernel/machine_kexec.c
+++ b/arch/powerpc/kernel/machine_kexec.c
@@ -145,6 +145,7 @@ int overlaps_crashkernel(unsigned long start, unsigned long size)
 
 /* Values we need to export to the second kernel via the device tree. */
 static unsigned long kernel_end;
+static unsigned long crashk_start;
 static unsigned long crashk_size;
 
 static struct property kernel_end_prop = {
@@ -156,7 +157,7 @@ static struct property kernel_end_prop = {
 static struct property crashk_base_prop = {
 	.name = "linux,crashkernel-base",
 	.length = sizeof(unsigned long),
-	.value = &crashk_res.start,
+	.value = &crashk_start,
 };
 
 static struct property crashk_size_prop = {
@@ -180,6 +181,7 @@ static void __init export_crashk_values(struct device_node *node)
 		prom_remove_property(node, prop);
 
 	if (crashk_res.start != 0) {
+		crashk_start = crashk_res.start;
 		prom_add_property(node, &crashk_base_prop);
 		crashk_size = crashk_res.end - crashk_res.start + 1;
 		prom_add_property(node, &crashk_size_prop);
-- 
1.6.6.1

[PATCH 2/2] powerpc/booke: Enable building of a crash dump kernel

[Matthew McClintock]

Enable building crash dump kernel as well as expose the flat
device tree for kexec to update to boot the crash kernel
---
 arch/powerpc/Kconfig       |    2 +-
 arch/powerpc/kernel/prom.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 042f2f0..0b60c57 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -368,7 +368,7 @@ config KEXEC
 
 config CRASH_DUMP
 	bool "Build a kdump crash kernel"
-	depends on PPC64 || 6xx
+	depends on PPC64 || 6xx || FSL_BOOKE
 	select RELOCATABLE if PPC64
 	help
 	  Build a kernel suitable for use as a kdump capture kernel.
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 05131d6..fd9359a 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -892,7 +892,7 @@ struct device_node *of_get_cpu_node(int cpu, unsigned int *thread)
 }
 EXPORT_SYMBOL(of_get_cpu_node);
 
-#if defined(CONFIG_DEBUG_FS) && defined(DEBUG)
+#if defined(CONFIG_DEBUG_FS) && (defined(DEBUG) || defined(CONFIG_KEXEC))
 static struct debugfs_blob_wrapper flat_dt_blob;
 
 static int __init export_flat_device_tree(void)
-- 
1.6.6.1

Re: [1/2] powerpc/crashdump: Fix issues with kexec and 36bit physical addr

[Milton Miller]

On Wed, 07 Jul 2010 around 10:51:20 -0000 Matthew McClintock wrote:
> 
> Fix sizes of variables so correct values are exported via /proc.
> Cast variable in comparison to avoid compiler error.
> 
> Signed-off-by: Matthew McClintock <msm@freescale.com>
> 
>  
> -	csize = min(csize, PAGE_SIZE);
> +	csize = min(csize, (size_t)PAGE_SIZE);

no use min_t

>  
> -	if (pfn < max_pfn) {
> +	if ((min_low_pfn < pfn) && (pfn < max_pfn)) {
>  		vaddr = __va(pfn << PAGE_SHIFT);
>  		csize = copy_oldmem_vaddr(vaddr, buf, csize, offset, userbuf);
>  	} else {
> diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
> index bb3d893..ec7f054 100644
> --- a/arch/powerpc/kernel/machine_kexec.c
> +++ b/arch/powerpc/kernel/machine_kexec.c
> @@ -145,6 +145,7 @@ int overlaps_crashkernel(unsigned long start, unsigned long size)
>  
>  /* Values we need to export to the second kernel via the device tree. */
>  static unsigned long kernel_end;
> +static unsigned long crashk_start;
>  static unsigned long crashk_size;
>  
>  static struct property kernel_end_prop = {
> @@ -156,7 +157,7 @@ static struct property kernel_end_prop = {
>  static struct property crashk_base_prop = {
>  	.name = "linux,crashkernel-base",
>  	.length = sizeof(unsigned long),
> -	.value = &crashk_res.start,
> +	.value = &crashk_start,
>  };
>  

This is wrong, its truncating the start and size.

Change the variables to be physaddr_t and the length to be sizeof(physaddr_t).

Since these properites only contain one variable, the number of cells
can be inferred from the property size like we do for reading the initrd-size.

Technically they should be an array of be32 but we can make that a comment.

By the way, why does 32 bit care about the running kernel's size? aka
linux,kernel-end?  64 bit book 3S needs it because we use mmu hooks
to copy the pages to their destination, but I thought ppc32 was using
a relocatable copy routine.  Are we missing the code to create
temp ref tlbs on the fly for book 3E?

>  static struct property crashk_size_prop = {
> @@ -180,6 +181,7 @@ static void __init export_crashk_values(struct device_node *node)
>  		prom_remove_property(node, prop);
>  
>  	if (crashk_res.start != 0) {
> +		crashk_start = crashk_res.start;
>  		prom_add_property(node, &crashk_base_prop);
>  		crashk_size = crashk_res.end - crashk_res.start + 1;
>  		prom_add_property(node, &crashk_size_prop);

I guess we use the reuse of the resources varables, but such is
common code vs userspace.

milton

Re: [1/2] powerpc/crashdump: Fix issues with kexec and 36bit physical addr

[Benjamin Herrenschmidt]

On Thu, 2010-07-08 at 04:07 -0500, Milton Miller wrote:
> On Wed, 07 Jul 2010 around 10:51:20 -0000 Matthew McClintock wrote:
> > 
> > Fix sizes of variables so correct values are exported via /proc.
> > Cast variable in comparison to avoid compiler error.
> >

I'm afraid I already pulled that in. Please send a fixup patch.

Cheers,
Ben.

> > Signed-off-by: Matthew McClintock <msm@freescale.com>
> > 
> >  
> > -	csize = min(csize, PAGE_SIZE);
> > +	csize = min(csize, (size_t)PAGE_SIZE);
> 
> no use min_t
> 
> >  
> > -	if (pfn < max_pfn) {
> > +	if ((min_low_pfn < pfn) && (pfn < max_pfn)) {
> >  		vaddr = __va(pfn << PAGE_SHIFT);
> >  		csize = copy_oldmem_vaddr(vaddr, buf, csize, offset, userbuf);
> >  	} else {
> > diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
> > index bb3d893..ec7f054 100644
> > --- a/arch/powerpc/kernel/machine_kexec.c
> > +++ b/arch/powerpc/kernel/machine_kexec.c
> > @@ -145,6 +145,7 @@ int overlaps_crashkernel(unsigned long start, unsigned long size)
> >  
> >  /* Values we need to export to the second kernel via the device tree. */
> >  static unsigned long kernel_end;
> > +static unsigned long crashk_start;
> >  static unsigned long crashk_size;
> >  
> >  static struct property kernel_end_prop = {
> > @@ -156,7 +157,7 @@ static struct property kernel_end_prop = {
> >  static struct property crashk_base_prop = {
> >  	.name = "linux,crashkernel-base",
> >  	.length = sizeof(unsigned long),
> > -	.value = &crashk_res.start,
> > +	.value = &crashk_start,
> >  };
> >  
> 
> This is wrong, its truncating the start and size.
> 
> Change the variables to be physaddr_t and the length to be sizeof(physaddr_t).
> 
> Since these properites only contain one variable, the number of cells
> can be inferred from the property size like we do for reading the initrd-size.
> 
> Technically they should be an array of be32 but we can make that a comment.
> 
> By the way, why does 32 bit care about the running kernel's size? aka
> linux,kernel-end?  64 bit book 3S needs it because we use mmu hooks
> to copy the pages to their destination, but I thought ppc32 was using
> a relocatable copy routine.  Are we missing the code to create
> temp ref tlbs on the fly for book 3E?
> 
> >  static struct property crashk_size_prop = {
> > @@ -180,6 +181,7 @@ static void __init export_crashk_values(struct device_node *node)
> >  		prom_remove_property(node, prop);
> >  
> >  	if (crashk_res.start != 0) {
> > +		crashk_start = crashk_res.start;
> >  		prom_add_property(node, &crashk_base_prop);
> >  		crashk_size = crashk_res.end - crashk_res.start + 1;
> >  		prom_add_property(node, &crashk_size_prop);
> 
> I guess we use the reuse of the resources varables, but such is
> common code vs userspace.
> 
> milton

Re: [1/2] powerpc/crashdump: Fix issues with kexec and 36bit physical addr

[Kumar Gala]

On Jul 8, 2010, at 5:49 AM, Benjamin Herrenschmidt wrote:

> On Thu, 2010-07-08 at 04:07 -0500, Milton Miller wrote:
>> On Wed, 07 Jul 2010 around 10:51:20 -0000 Matthew McClintock wrote:
>>> 
>>> Fix sizes of variables so correct values are exported via /proc.
>>> Cast variable in comparison to avoid compiler error.
>>> 
> 
> I'm afraid I already pulled that in. Please send a fixup patch.
> 
> Cheers,
> Ben.

pulled into which tree?

- k

Re: [1/2] powerpc/crashdump: Fix issues with kexec and 36bit physical addr

[Matthew McClintock]

On Thu, 2010-07-08 at 04:07 -0500, Milton Miller wrote:
> On Wed, 07 Jul 2010 around 10:51:20 -0000 Matthew McClintock wrote:
> > 
> > Fix sizes of variables so correct values are exported via /proc.
> > Cast variable in comparison to avoid compiler error.
> > 
> > Signed-off-by: Matthew McClintock <msm@freescale.com>
> > 
> >  
> > -	csize = min(csize, PAGE_SIZE);
> > +	csize = min(csize, (size_t)PAGE_SIZE);
> 
> no use min_t

Ok

> 
> >  
> > -	if (pfn < max_pfn) {
> > +	if ((min_low_pfn < pfn) && (pfn < max_pfn)) {
> >  		vaddr = __va(pfn << PAGE_SHIFT);
> >  		csize = copy_oldmem_vaddr(vaddr, buf, csize, offset, userbuf);
> >  	} else {
> > diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
> > index bb3d893..ec7f054 100644
> > --- a/arch/powerpc/kernel/machine_kexec.c
> > +++ b/arch/powerpc/kernel/machine_kexec.c
> > @@ -145,6 +145,7 @@ int overlaps_crashkernel(unsigned long start, unsigned long size)
> >  
> >  /* Values we need to export to the second kernel via the device tree. */
> >  static unsigned long kernel_end;
> > +static unsigned long crashk_start;
> >  static unsigned long crashk_size;
> >  
> >  static struct property kernel_end_prop = {
> > @@ -156,7 +157,7 @@ static struct property kernel_end_prop = {
> >  static struct property crashk_base_prop = {
> >  	.name = "linux,crashkernel-base",
> >  	.length = sizeof(unsigned long),
> > -	.value = &crashk_res.start,
> > +	.value = &crashk_start,
> >  };
> >  
> 
> This is wrong, its truncating the start and size.
> 
> Change the variables to be physaddr_t and the length to be sizeof(physaddr_t).
> 
> Since these properites only contain one variable, the number of cells
> can be inferred from the property size like we do for reading the initrd-size.
> 
> Technically they should be an array of be32 but we can make that a comment.

I don't disagree but this can break kexec if phys_addr_t != unsigned
long. Also, doesn't the crash kernel have to live below 2GB so unsigned
long is always fine?

Will still change unless I hear otherwise.

> 
> By the way, why does 32 bit care about the running kernel's size? aka
> linux,kernel-end?  64 bit book 3S needs it because we use mmu hooks
> to copy the pages to their destination, but I thought ppc32 was using
> a relocatable copy routine.  Are we missing the code to create
> temp ref tlbs on the fly for book 3E?

This is not really in this patch or did I miss something? Kexec uses
kernel-end just to add as a invalid region. Not crucial though for 32
bit.

> 
> >  static struct property crashk_size_prop = {
> > @@ -180,6 +181,7 @@ static void __init export_crashk_values(struct device_node *node)
> >  		prom_remove_property(node, prop);
> >  
> >  	if (crashk_res.start != 0) {
> > +		crashk_start = crashk_res.start;
> >  		prom_add_property(node, &crashk_base_prop);
> >  		crashk_size = crashk_res.end - crashk_res.start + 1;
> >  		prom_add_property(node, &crashk_size_prop);
> 
> I guess we use the reuse of the resources varables, but such is
> common code vs userspace.
> 
> milton

Re: [1/2] powerpc/crashdump: Fix issues with kexec and 36bit physical addr

[Benjamin Herrenschmidt]

On Thu, 2010-07-08 at 07:40 -0500, Kumar Gala wrote:
> On Jul 8, 2010, at 5:49 AM, Benjamin Herrenschmidt wrote:
> 
> > On Thu, 2010-07-08 at 04:07 -0500, Milton Miller wrote:
> >> On Wed, 07 Jul 2010 around 10:51:20 -0000 Matthew McClintock wrote:
> >>> 
> >>> Fix sizes of variables so correct values are exported via /proc.
> >>> Cast variable in comparison to avoid compiler error.
> >>> 
> > 
> > I'm afraid I already pulled that in. Please send a fixup patch.
> > 

> pulled into which tree?

My confusion. It was in my to-be-posted -next but I hadn't actually
uploaded it yet so I've dropped the patch for now. Matthew, don't send
an incremental fixup, a whole new patch will do. I've also dropped the
second patch that enables building of crash dump kernels for now.

I've left the MPIC CPU reset one though.

Cheers,
Ben.

[PATCH 1/2] powerpc/crashdump: Fix issues with kexec and 36bit physical addr

[Matthew McClintock]

Fix sizes of variables so correct values are exported via /proc.
Cast variable in comparison to avoid compiler error.

Signed-off-by: Matthew McClintock <msm@freescale.com>
---
 arch/powerpc/kernel/crash_dump.c    |    4 ++--
 arch/powerpc/kernel/machine_kexec.c |   10 +++++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/kernel/crash_dump.c b/arch/powerpc/kernel/crash_dump.c
index 5fb667a..d254132 100644
--- a/arch/powerpc/kernel/crash_dump.c
+++ b/arch/powerpc/kernel/crash_dump.c
@@ -128,9 +128,9 @@ ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
 	if (!csize)
 		return 0;
 
-	csize = min(csize, PAGE_SIZE);
+	csize = min_t(size_t, csize, PAGE_SIZE);
 
-	if (pfn < max_pfn) {
+	if ((min_low_pfn < pfn) && (pfn < max_pfn)) {
 		vaddr = __va(pfn << PAGE_SHIFT);
 		csize = copy_oldmem_vaddr(vaddr, buf, csize, offset, userbuf);
 	} else {
diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
index bb3d893..6ff15f0 100644
--- a/arch/powerpc/kernel/machine_kexec.c
+++ b/arch/powerpc/kernel/machine_kexec.c
@@ -144,24 +144,24 @@ int overlaps_crashkernel(unsigned long start, unsigned long size)
 }
 
 /* Values we need to export to the second kernel via the device tree. */
-static unsigned long kernel_end;
-static unsigned long crashk_size;
+static phys_addr_t kernel_end;
+static phys_addr_t crashk_size;
 
 static struct property kernel_end_prop = {
 	.name = "linux,kernel-end",
-	.length = sizeof(unsigned long),
+	.length = sizeof(phys_addr_t),
 	.value = &kernel_end,
 };
 
 static struct property crashk_base_prop = {
 	.name = "linux,crashkernel-base",
-	.length = sizeof(unsigned long),
+	.length = sizeof(phys_addr_t),
 	.value = &crashk_res.start,
 };
 
 static struct property crashk_size_prop = {
 	.name = "linux,crashkernel-size",
-	.length = sizeof(unsigned long),
+	.length = sizeof(phys_addr_t),
 	.value = &crashk_size,
 };
 
-- 
1.6.6.1