[PATCH 0/3] kexec: Support for uImage RAMDisks

[PATCH 0/3] kexec: Support for uImage RAMDisks

[Suzuki K. Poulose]

The following series adds support for uImage RAMDisks in kexec-tools.

The first two patches in the series modifies the common uImage handling code.

The third patch in the series adds PPC support for processing the ramdisks.

NOTE: Tested *only* on PPC. Though the other archs are not affected much,
I would appreciate some help in (compile) testing these patches on ARM and SH,
just to make sure we are safe there.

With these patches, one can easily add support in other archs for supporting
the uImage RAMDisks.

---

Suzuki K. Poulose (3):
      kexec/uImage: Return the image type for uImage_probe
      kexec/uImage: Recognize uImage RAM Disks
      ppc/uImage: Add support for RAM Disks


 kexec/arch/arm/kexec-uImage-arm.c |    8 +++++++-
 kexec/arch/ppc/kexec-uImage-ppc.c |   30 ++++++++++++++++++++++++++++--
 kexec/arch/sh/kexec-uImage-sh.c   |    9 ++++++++-
 kexec/kexec-uImage.c              |   18 ++++++++++++------
 4 files changed, 55 insertions(+), 10 deletions(-)

-- 
Suzuki


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 1/3] kexec/uImage: Return the image type for uImage_probe

[Suzuki K. Poulose]

From: Suzuki K. Poulose <suzuki@in.ibm.com>

uImage supports different types of payloads, including kernel,
ramdisks etc. uImage_probe() as of now checks whether the supplied
payload is of type KERNEL ( i.e, IH_TYPE_KERNEL or IH_TYPE_KERNEL_NOLOAD ).

Change this behaviour to return the image type, if it is one of the supported
payloads. This change is in prepartion to support ramdisks in uImage format.

Also, I have changed the code to check for the OS type only for the kernel images.

Is this check really needed ? We shouldn't bother what OS is being loaded. Isn't it ?

This change would allow the user to decide if the image is of their interest.

Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
---
 kexec/arch/arm/kexec-uImage-arm.c |    8 +++++++-
 kexec/arch/ppc/kexec-uImage-ppc.c |    8 +++++++-
 kexec/arch/sh/kexec-uImage-sh.c   |    9 ++++++++-
 kexec/kexec-uImage.c              |   16 ++++++++++------
 4 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/kexec/arch/arm/kexec-uImage-arm.c b/kexec/arch/arm/kexec-uImage-arm.c
index 4875185..dc1bcb8 100644
--- a/kexec/arch/arm/kexec-uImage-arm.c
+++ b/kexec/arch/arm/kexec-uImage-arm.c
@@ -11,7 +11,13 @@
 
 int uImage_arm_probe(const char *buf, off_t len)
 {
-	return uImage_probe(buf, len, IH_ARCH_ARM);
+	int type;
+	int rc = -1;
+
+	type = uImage_probe(buf, len, IH_ARCH_ARM);
+	if (type == IH_TYPE_KERNEL || type == IH_TYPE_KERNEL_NOLOAD)
+		rc = 0;
+	return rc;
 }
 
 int uImage_arm_load(int argc, char **argv, const char *buf, off_t len,
diff --git a/kexec/arch/ppc/kexec-uImage-ppc.c b/kexec/arch/ppc/kexec-uImage-ppc.c
index e55bf94..eaea3c1 100644
--- a/kexec/arch/ppc/kexec-uImage-ppc.c
+++ b/kexec/arch/ppc/kexec-uImage-ppc.c
@@ -48,7 +48,13 @@ void uImage_ppc_usage(void)
 
 int uImage_ppc_probe(const char *buf, off_t len)
 {
-	return uImage_probe(buf, len, IH_ARCH_PPC);
+	int type;
+	int rc = -1;
+
+	type = uImage_probe(buf, len, IH_ARCH_PPC);
+	if (type == IH_TYPE_KERNEL || type == IH_TYPE_KERNEL_NOLOAD)
+		rc = 0;
+	return rc;
 }
 
 static int ppc_load_bare_bits(int argc, char **argv, const char *buf,
diff --git a/kexec/arch/sh/kexec-uImage-sh.c b/kexec/arch/sh/kexec-uImage-sh.c
index e983165..54bb073 100644
--- a/kexec/arch/sh/kexec-uImage-sh.c
+++ b/kexec/arch/sh/kexec-uImage-sh.c
@@ -13,7 +13,14 @@
 
 int uImage_sh_probe(const char *buf, off_t len)
 {
-	return uImage_probe(buf, len, IH_ARCH_SH);
+	int type;
+	int rc = -1;
+
+	type = uImage_probe(buf, len, IH_ARCH_SH);
+	if (type == IH_TYPE_KERNEL || type == IH_TYPE_KERNEL_NOLOAD)
+		rc = uImage_probe(buf, len, IH_ARCH_SH);
+
+	return rc;
 }
 
 int uImage_sh_load(int argc, char **argv, const char *buf, off_t len,
diff --git a/kexec/kexec-uImage.c b/kexec/kexec-uImage.c
index 3bc85c5..dcbd635 100644
--- a/kexec/kexec-uImage.c
+++ b/kexec/kexec-uImage.c
@@ -16,6 +16,10 @@
  * Basic uImage loader. Not rocket science.
  */
 
+/*
+ * Returns the image type if everything goes well. This would
+ * allow the user to decide if the image is of their interest.
+ */
 int uImage_probe(const unsigned char *buf, off_t len, unsigned int arch)
 {
 	struct image_header header;
@@ -42,17 +46,17 @@ int uImage_probe(const unsigned char *buf, off_t len, unsigned int arch)
 	switch (header.ih_type) {
 	case IH_TYPE_KERNEL:
 	case IH_TYPE_KERNEL_NOLOAD:
+		/* XXX: Should we really check this ? */
+		if (header.ih_os != IH_OS_LINUX) {
+			printf("uImage os %d unsupported\n", header.ih_os);
+			return -1;
+		}
 		break;
 	default:
 		printf("uImage type %d unsupported\n", header.ih_type);
 		return -1;
 	}
 
-	if (header.ih_os != IH_OS_LINUX) {
-		printf("uImage os %d unsupported\n", header.ih_os);
-		return -1;
-	}
-
 	if (header.ih_arch != arch) {
 		printf("uImage arch %d unsupported\n", header.ih_arch);
 		return -1;
@@ -84,7 +88,7 @@ int uImage_probe(const unsigned char *buf, off_t len, unsigned int arch)
 		return -1;
 	}
 #endif
-	return 0;
+	return (int)header.ih_type;
 }
 
 #ifdef HAVE_LIBZ


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 2/3] kexec/uImage: Recognize uImage RAM Disks

[Suzuki K. Poulose]

From: Suzuki K. Poulose <suzuki@in.ibm.com>

Add IH_TYPE_RAMDISK as a recognized image type.

Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
---
 kexec/kexec-uImage.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/kexec/kexec-uImage.c b/kexec/kexec-uImage.c
index dcbd635..701eeb1 100644
--- a/kexec/kexec-uImage.c
+++ b/kexec/kexec-uImage.c
@@ -52,6 +52,8 @@ int uImage_probe(const unsigned char *buf, off_t len, unsigned int arch)
 			return -1;
 		}
 		break;
+	case IH_TYPE_RAMDISK:
+		break;
 	default:
 		printf("uImage type %d unsupported\n", header.ih_type);
 		return -1;


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 3/3] ppc/uImage: Add support for RAM Disks

[Suzuki K. Poulose]

From: Suzuki K. Poulose <suzuki@in.ibm.com>

Handle the RAM Disks in uImage format

Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
---
 kexec/arch/ppc/kexec-uImage-ppc.c |   22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/kexec/arch/ppc/kexec-uImage-ppc.c b/kexec/arch/ppc/kexec-uImage-ppc.c
index eaea3c1..58935c0 100644
--- a/kexec/arch/ppc/kexec-uImage-ppc.c
+++ b/kexec/arch/ppc/kexec-uImage-ppc.c
@@ -46,6 +46,26 @@ void uImage_ppc_usage(void)
 	);
 }
 
+char* slurp_ramdisk_ppc(const char *filename, off_t *r_size)
+{
+	struct Image_info img;
+	off_t size;
+	const unsigned char *buf = 
+			(const unsigned char *)slurp_file(filename, &size);
+
+	/* Check if this is a uImage RAMDisk */
+	if (buf != (void*)0 &&
+		uImage_probe(buf, size, IH_ARCH_PPC) == IH_TYPE_RAMDISK) {
+		if (uImage_load(buf, size, &img) != 0)
+			die ("uImage: Reading %ld bytes from %s failed\n", size, filename);
+		buf = img.buf;
+		size = img.len;
+	}
+
+	*r_size = size;
+	return buf;
+}
+	
 int uImage_ppc_probe(const char *buf, off_t len)
 {
 	int type;
@@ -196,7 +216,7 @@ static int ppc_load_bare_bits(int argc, char **argv, const char *buf,
 	blob_buf = fixup_dtb_init(info, blob_buf, &blob_size, load_addr, &dtb_addr);
 
 	if (ramdisk) {
-		seg_buf = slurp_file(ramdisk, &seg_size);
+		seg_buf = slurp_ramdisk_ppc(ramdisk, &seg_size);
 		/* Load ramdisk at top of memory */
 		hole_addr = add_buffer(info, seg_buf, seg_size, seg_size,
 				0, dtb_addr + blob_size, max_addr, -1);


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] ppc/uImage: Add support for RAM Disks

[McClintock Matthew-B29882]

On Fri, Jan 11, 2013 at 12:42 AM, Suzuki K. Poulose <suzuki@in.ibm.com> wrote:
> From: Suzuki K. Poulose <suzuki@in.ibm.com>
>
> Handle the RAM Disks in uImage format

As opposed to simple ext2.gz ramdisk?

-M

>
> Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
> ---
>  kexec/arch/ppc/kexec-uImage-ppc.c |   22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/kexec/arch/ppc/kexec-uImage-ppc.c b/kexec/arch/ppc/kexec-uImage-ppc.c
> index eaea3c1..58935c0 100644
> --- a/kexec/arch/ppc/kexec-uImage-ppc.c
> +++ b/kexec/arch/ppc/kexec-uImage-ppc.c
> @@ -46,6 +46,26 @@ void uImage_ppc_usage(void)
>         );
>  }
>
> +char* slurp_ramdisk_ppc(const char *filename, off_t *r_size)
> +{
> +       struct Image_info img;
> +       off_t size;
> +       const unsigned char *buf =
> +                       (const unsigned char *)slurp_file(filename, &size);
> +
> +       /* Check if this is a uImage RAMDisk */
> +       if (buf != (void*)0 &&
> +               uImage_probe(buf, size, IH_ARCH_PPC) == IH_TYPE_RAMDISK) {
> +               if (uImage_load(buf, size, &img) != 0)
> +                       die ("uImage: Reading %ld bytes from %s failed\n", size, filename);
> +               buf = img.buf;
> +               size = img.len;
> +       }
> +
> +       *r_size = size;
> +       return buf;
> +}
> +
>  int uImage_ppc_probe(const char *buf, off_t len)
>  {
>         int type;
> @@ -196,7 +216,7 @@ static int ppc_load_bare_bits(int argc, char **argv, const char *buf,
>         blob_buf = fixup_dtb_init(info, blob_buf, &blob_size, load_addr, &dtb_addr);
>
>         if (ramdisk) {
> -               seg_buf = slurp_file(ramdisk, &seg_size);
> +               seg_buf = slurp_ramdisk_ppc(ramdisk, &seg_size);
>                 /* Load ramdisk at top of memory */
>                 hole_addr = add_buffer(info, seg_buf, seg_size, seg_size,
>                                 0, dtb_addr + blob_size, max_addr, -1);
>
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] ppc/uImage: Add support for RAM Disks

[Suzuki K. Poulose]

On 01/11/2013 12:41 PM, McClintock Matthew-B29882 wrote:
> On Fri, Jan 11, 2013 at 12:42 AM, Suzuki K. Poulose <suzuki@in.ibm.com> wrote:
>> From: Suzuki K. Poulose <suzuki@in.ibm.com>
>>
>> Handle the RAM Disks in uImage format
>
> As opposed to simple ext2.gz ramdisk?

Yep. Any supported ramdisk format can be wrapped up in a uImage file.

Thanks
Suzuki

>
> -M
>
>>
>> Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
>> ---
>>   kexec/arch/ppc/kexec-uImage-ppc.c |   22 +++++++++++++++++++++-
>>   1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/kexec/arch/ppc/kexec-uImage-ppc.c b/kexec/arch/ppc/kexec-uImage-ppc.c
>> index eaea3c1..58935c0 100644
>> --- a/kexec/arch/ppc/kexec-uImage-ppc.c
>> +++ b/kexec/arch/ppc/kexec-uImage-ppc.c
>> @@ -46,6 +46,26 @@ void uImage_ppc_usage(void)
>>          );
>>   }
>>
>> +char* slurp_ramdisk_ppc(const char *filename, off_t *r_size)
>> +{
>> +       struct Image_info img;
>> +       off_t size;
>> +       const unsigned char *buf =
>> +                       (const unsigned char *)slurp_file(filename, &size);
>> +
>> +       /* Check if this is a uImage RAMDisk */
>> +       if (buf != (void*)0 &&
>> +               uImage_probe(buf, size, IH_ARCH_PPC) == IH_TYPE_RAMDISK) {
>> +               if (uImage_load(buf, size, &img) != 0)
>> +                       die ("uImage: Reading %ld bytes from %s failed\n", size, filename);
>> +               buf = img.buf;
>> +               size = img.len;
>> +       }
>> +
>> +       *r_size = size;
>> +       return buf;
>> +}
>> +
>>   int uImage_ppc_probe(const char *buf, off_t len)
>>   {
>>          int type;
>> @@ -196,7 +216,7 @@ static int ppc_load_bare_bits(int argc, char **argv, const char *buf,
>>          blob_buf = fixup_dtb_init(info, blob_buf, &blob_size, load_addr, &dtb_addr);
>>
>>          if (ramdisk) {
>> -               seg_buf = slurp_file(ramdisk, &seg_size);
>> +               seg_buf = slurp_ramdisk_ppc(ramdisk, &seg_size);
>>                  /* Load ramdisk at top of memory */
>>                  hole_addr = add_buffer(info, seg_buf, seg_size, seg_size,
>>                                  0, dtb_addr + blob_size, max_addr, -1);
>>
>>
>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] ppc/uImage: Add support for RAM Disks

[McClintock Matthew-B29882]

On Sun, Jan 13, 2013 at 11:14 PM, Suzuki K. Poulose <suzuki@in.ibm.com> wrote:
> On 01/11/2013 12:41 PM, McClintock Matthew-B29882 wrote:
>>
>> On Fri, Jan 11, 2013 at 12:42 AM, Suzuki K. Poulose <suzuki@in.ibm.com>
>> wrote:
>>>
>>> From: Suzuki K. Poulose <suzuki@in.ibm.com>
>>>
>>> Handle the RAM Disks in uImage format
>>
>>
>> As opposed to simple ext2.gz ramdisk?

OK - it's been a while but we only supported uImage for kernel image
not ramdisk. That's what threw me off when I wrote this email I
believe.

-M

>
>
> Yep. Any supported ramdisk format can be wrapped up in a uImage file.
>
> Thanks
> Suzuki
>
>
>>
>> -M
>>
>>>
>>> Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
>>> ---
>>>   kexec/arch/ppc/kexec-uImage-ppc.c |   22 +++++++++++++++++++++-
>>>   1 file changed, 21 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kexec/arch/ppc/kexec-uImage-ppc.c
>>> b/kexec/arch/ppc/kexec-uImage-ppc.c
>>> index eaea3c1..58935c0 100644
>>> --- a/kexec/arch/ppc/kexec-uImage-ppc.c
>>> +++ b/kexec/arch/ppc/kexec-uImage-ppc.c
>>> @@ -46,6 +46,26 @@ void uImage_ppc_usage(void)
>>>          );
>>>   }
>>>
>>> +char* slurp_ramdisk_ppc(const char *filename, off_t *r_size)
>>> +{
>>> +       struct Image_info img;
>>> +       off_t size;
>>> +       const unsigned char *buf =
>>> +                       (const unsigned char *)slurp_file(filename,
>>> &size);
>>> +
>>> +       /* Check if this is a uImage RAMDisk */
>>> +       if (buf != (void*)0 &&
>>> +               uImage_probe(buf, size, IH_ARCH_PPC) == IH_TYPE_RAMDISK)
>>> {
>>> +               if (uImage_load(buf, size, &img) != 0)
>>> +                       die ("uImage: Reading %ld bytes from %s
>>> failed\n", size, filename);
>>> +               buf = img.buf;
>>> +               size = img.len;
>>> +       }
>>> +
>>> +       *r_size = size;
>>> +       return buf;
>>> +}
>>> +
>>>   int uImage_ppc_probe(const char *buf, off_t len)
>>>   {
>>>          int type;
>>> @@ -196,7 +216,7 @@ static int ppc_load_bare_bits(int argc, char **argv,
>>> const char *buf,
>>>          blob_buf = fixup_dtb_init(info, blob_buf, &blob_size, load_addr,
>>> &dtb_addr);
>>>
>>>          if (ramdisk) {
>>> -               seg_buf = slurp_file(ramdisk, &seg_size);
>>> +               seg_buf = slurp_ramdisk_ppc(ramdisk, &seg_size);
>>>                  /* Load ramdisk at top of memory */
>>>                  hole_addr = add_buffer(info, seg_buf, seg_size,
>>> seg_size,
>>>                                  0, dtb_addr + blob_size, max_addr, -1);
>>>
>>>
>>> _______________________________________________
>>> kexec mailing list
>>> kexec@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/kexec
>>
>>
>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec
>>
>
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 1/3] kexec/uImage: Return the image type for uImage_probe

[Sebastian Andrzej Siewior]

* Suzuki K. Poulose | 2013-01-11 12:11:36 [+0530]:

>From: Suzuki K. Poulose <suzuki@in.ibm.com>
>
>uImage supports different types of payloads, including kernel,
>ramdisks etc. uImage_probe() as of now checks whether the supplied
>payload is of type KERNEL ( i.e, IH_TYPE_KERNEL or IH_TYPE_KERNEL_NOLOAD ).
>
>Change this behaviour to return the image type, if it is one of the supported
>payloads. This change is in prepartion to support ramdisks in uImage format.
>
>Also, I have changed the code to check for the OS type only for the kernel images.
>
>Is this check really needed ? We shouldn't bother what OS is being loaded. Isn't it ?

We should bother because mostly linux is supported. Other operating
systems like NetBSD might have other ABI for the enter point or some
special requirements. The entry ABI for linux comapared to WindowsCE is
different for ARM just to name one example.

>This change would allow the user to decide if the image is of their interest.
>
>Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
>---
> kexec/arch/arm/kexec-uImage-arm.c |    8 +++++++-
> kexec/arch/ppc/kexec-uImage-ppc.c |    8 +++++++-
> kexec/arch/sh/kexec-uImage-sh.c   |    9 ++++++++-
> kexec/kexec-uImage.c              |   16 ++++++++++------
> 4 files changed, 32 insertions(+), 9 deletions(-)
>
>diff --git a/kexec/arch/arm/kexec-uImage-arm.c b/kexec/arch/arm/kexec-uImage-arm.c
>index 4875185..dc1bcb8 100644
>--- a/kexec/arch/arm/kexec-uImage-arm.c
>+++ b/kexec/arch/arm/kexec-uImage-arm.c
>@@ -11,7 +11,13 @@
> 
> int uImage_arm_probe(const char *buf, off_t len)
> {
>-	return uImage_probe(buf, len, IH_ARCH_ARM);
>+	int type;
>+	int rc = -1;
>+
>+	type = uImage_probe(buf, len, IH_ARCH_ARM);
>+	if (type == IH_TYPE_KERNEL || type == IH_TYPE_KERNEL_NOLOAD)
>+		rc = 0;
>+	return rc;
> }
> 
> int uImage_arm_load(int argc, char **argv, const char *buf, off_t len,
>diff --git a/kexec/arch/ppc/kexec-uImage-ppc.c b/kexec/arch/ppc/kexec-uImage-ppc.c
>index e55bf94..eaea3c1 100644
>--- a/kexec/arch/ppc/kexec-uImage-ppc.c
>+++ b/kexec/arch/ppc/kexec-uImage-ppc.c
>@@ -48,7 +48,13 @@ void uImage_ppc_usage(void)
> 
> int uImage_ppc_probe(const char *buf, off_t len)
> {
>-	return uImage_probe(buf, len, IH_ARCH_PPC);
>+	int type;
>+	int rc = -1;
>+
>+	type = uImage_probe(buf, len, IH_ARCH_PPC);
>+	if (type == IH_TYPE_KERNEL || type == IH_TYPE_KERNEL_NOLOAD)
>+		rc = 0;
>+	return rc;
> }
> 
> static int ppc_load_bare_bits(int argc, char **argv, const char *buf,
>diff --git a/kexec/arch/sh/kexec-uImage-sh.c b/kexec/arch/sh/kexec-uImage-sh.c
>index e983165..54bb073 100644
>--- a/kexec/arch/sh/kexec-uImage-sh.c
>+++ b/kexec/arch/sh/kexec-uImage-sh.c
>@@ -13,7 +13,14 @@
> 
> int uImage_sh_probe(const char *buf, off_t len)
> {
>-	return uImage_probe(buf, len, IH_ARCH_SH);
>+	int type;
>+	int rc = -1;
>+
>+	type = uImage_probe(buf, len, IH_ARCH_SH);
>+	if (type == IH_TYPE_KERNEL || type == IH_TYPE_KERNEL_NOLOAD)
>+		rc = uImage_probe(buf, len, IH_ARCH_SH);

This looks like a typo compared to ppc/arm above. If so you could create
uImage_probe_kernel() which has the type == check.

>+
>+	return rc;
> }
> 
> int uImage_sh_load(int argc, char **argv, const char *buf, off_t len,
>diff --git a/kexec/kexec-uImage.c b/kexec/kexec-uImage.c
>index 3bc85c5..dcbd635 100644
>--- a/kexec/kexec-uImage.c
>+++ b/kexec/kexec-uImage.c
>@@ -16,6 +16,10 @@
>  * Basic uImage loader. Not rocket science.
>  */
> 
>+/*
>+ * Returns the image type if everything goes well. This would
>+ * allow the user to decide if the image is of their interest.
>+ */
> int uImage_probe(const unsigned char *buf, off_t len, unsigned int arch)
> {
> 	struct image_header header;
>@@ -42,17 +46,17 @@ int uImage_probe(const unsigned char *buf, off_t len, unsigned int arch)
> 	switch (header.ih_type) {
> 	case IH_TYPE_KERNEL:
> 	case IH_TYPE_KERNEL_NOLOAD:
>+		/* XXX: Should we really check this ? */
Yes.
>+		if (header.ih_os != IH_OS_LINUX) {
>+			printf("uImage os %d unsupported\n", header.ih_os);
>+			return -1;
>+		}

Why did move this piece. Is it because ramdisk does not have an OS
selected?

> 		break;
> 	default:
> 		printf("uImage type %d unsupported\n", header.ih_type);
> 		return -1;
> 	}
> 
>-	if (header.ih_os != IH_OS_LINUX) {
>-		printf("uImage os %d unsupported\n", header.ih_os);
>-		return -1;
>-	}
>-
> 	if (header.ih_arch != arch) {
> 		printf("uImage arch %d unsupported\n", header.ih_arch);
> 		return -1;
>@@ -84,7 +88,7 @@ int uImage_probe(const unsigned char *buf, off_t len, unsigned int arch)
> 		return -1;
> 	}
> #endif
>-	return 0;
>+	return (int)header.ih_type;

Why do you return type here?

> }
> 
> #ifdef HAVE_LIBZ
>

Sebastian

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] ppc/uImage: Add support for RAM Disks

[Sebastian Andrzej Siewior]

* Suzuki K. Poulose | 2013-01-11 12:12:11 [+0530]:

only some stylish comments…

>diff --git a/kexec/arch/ppc/kexec-uImage-ppc.c b/kexec/arch/ppc/kexec-uImage-ppc.c
>index eaea3c1..58935c0 100644
>--- a/kexec/arch/ppc/kexec-uImage-ppc.c
>+++ b/kexec/arch/ppc/kexec-uImage-ppc.c
>@@ -46,6 +46,26 @@ void uImage_ppc_usage(void)
> 	);
> }
> 
>+char* slurp_ramdisk_ppc(const char *filename, off_t *r_size)
char *slurp_ramdisk_ppc(const char *filename, off_t *r_size)
?

>+{
>+	struct Image_info img;
>+	off_t size;
>+	const unsigned char *buf = 
>+			(const unsigned char *)slurp_file(filename, &size);

is the const cast required here?

>+	/* Check if this is a uImage RAMDisk */
>+	if (buf != (void*)0 &&
this should be NULL if at all

>+		uImage_probe(buf, size, IH_ARCH_PPC) == IH_TYPE_RAMDISK) {
and maybe splitted in more than one line

>+		if (uImage_load(buf, size, &img) != 0)
>+			die ("uImage: Reading %ld bytes from %s failed\n", size, filename);
no space between the function.

>+		buf = img.buf;
>+		size = img.len;
>+	}
>+
>+	*r_size = size;
>+	return buf;
>+}
>+	
> int uImage_ppc_probe(const char *buf, off_t len)
> {
> 	int type;
>@@ -196,7 +216,7 @@ static int ppc_load_bare_bits(int argc, char **argv, const char *buf,
> 	blob_buf = fixup_dtb_init(info, blob_buf, &blob_size, load_addr, &dtb_addr);
> 
> 	if (ramdisk) {
>-		seg_buf = slurp_file(ramdisk, &seg_size);
>+		seg_buf = slurp_ramdisk_ppc(ramdisk, &seg_size);

I'm not sure but this kinda breaks the case where someone was using a
plain file without the uImage header. I don't know if someone used this
at all but it seems we have code for this. The way you load the uImage
is very generic and I would expect to load it the same way on ARM or SH.

One thing that is different compared to kernel: If the compression is
set to GZ on a ramdisk image you should not uncompress it. I think that
uImage_load() decompresses the .gz part for you but I am not sure.

> 		/* Load ramdisk at top of memory */
> 		hole_addr = add_buffer(info, seg_buf, seg_size, seg_size,
> 				0, dtb_addr + blob_size, max_addr, -1);
>

Sebastian

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] ppc/uImage: Add support for RAM Disks

[Sebastian Andrzej Siewior]

* McClintock Matthew-B29882 | 2013-01-14 15:14:16 [+0000]:

>On Sun, Jan 13, 2013 at 11:14 PM, Suzuki K. Poulose <suzuki@in.ibm.com> wrote:
>> On 01/11/2013 12:41 PM, McClintock Matthew-B29882 wrote:
>>>
>>> On Fri, Jan 11, 2013 at 12:42 AM, Suzuki K. Poulose <suzuki@in.ibm.com>
>>> wrote:
>>>>
>>>> From: Suzuki K. Poulose <suzuki@in.ibm.com>
>>>>
>>>> Handle the RAM Disks in uImage format
>>>
>>>
>>> As opposed to simple ext2.gz ramdisk?
>
>OK - it's been a while but we only supported uImage for kernel image
>not ramdisk. That's what threw me off when I wrote this email I
>believe.

Who is "we"? Did you use kexec and pass ext2.gz as a ramdisk?

u-boot supports this like for ever. You start the kernel via
 bootm x y z

where y is the address of the ramdisk / initrd. passing the address the
ext2.gz files does not work.
As an alternative to this you can only pass x to bootm and the image has
the "uImage.FIT" format which can contain the kernel, device tree and a
ramdisk in one piece.

>-M

Sebastian

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] ppc/uImage: Add support for RAM Disks

[McClintock Matthew-B29882]

On Wed, Jan 16, 2013 at 11:30 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> Who is "we"? Did you use kexec and pass ext2.gz as a ramdisk?

We = me for the most part.

Yes, that's what I did, I never passed in a uboot wrapped image.

-M

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 1/3] kexec/uImage: Return the image type for uImage_probe

[Suzuki K. Poulose]

On 01/16/2013 10:50 PM, Sebastian Andrzej Siewior wrote:
> * Suzuki K. Poulose | 2013-01-11 12:11:36 [+0530]:
>
>> From: Suzuki K. Poulose <suzuki@in.ibm.com>
>>
>> uImage supports different types of payloads, including kernel,
>> ramdisks etc. uImage_probe() as of now checks whether the supplied
>> payload is of type KERNEL ( i.e, IH_TYPE_KERNEL or IH_TYPE_KERNEL_NOLOAD ).
>>
>> Change this behaviour to return the image type, if it is one of the supported
>> payloads. This change is in prepartion to support ramdisks in uImage format.
>>
>> Also, I have changed the code to check for the OS type only for the kernel images.
>>
>> Is this check really needed ? We shouldn't bother what OS is being loaded. Isn't it ?
>
> We should bother because mostly linux is supported. Other operating
> systems like NetBSD might have other ABI for the enter point or some
> special requirements. The entry ABI for linux comapared to WindowsCE is
> different for ARM just to name one example.
>

Ok. Makes sense now. Thanks for the clarification.

>> This change would allow the user to decide if the image is of their interest.
>>
>> Signed-off-by: Suzuki K. Poulose <suzuki@in.ibm.com>
>> ---
>> kexec/arch/arm/kexec-uImage-arm.c |    8 +++++++-
>> kexec/arch/ppc/kexec-uImage-ppc.c |    8 +++++++-
>> kexec/arch/sh/kexec-uImage-sh.c   |    9 ++++++++-
>> kexec/kexec-uImage.c              |   16 ++++++++++------
>> 4 files changed, 32 insertions(+), 9 deletions(-)
>>
>> diff --git a/kexec/arch/arm/kexec-uImage-arm.c b/kexec/arch/arm/kexec-uImage-arm.c
>> index 4875185..dc1bcb8 100644
>> --- a/kexec/arch/arm/kexec-uImage-arm.c
>> +++ b/kexec/arch/arm/kexec-uImage-arm.c
>> @@ -11,7 +11,13 @@
>>
>> int uImage_arm_probe(const char *buf, off_t len)
>> {
>> -	return uImage_probe(buf, len, IH_ARCH_ARM);
>> +	int type;
>> +	int rc = -1;
>> +
>> +	type = uImage_probe(buf, len, IH_ARCH_ARM);
>> +	if (type == IH_TYPE_KERNEL || type == IH_TYPE_KERNEL_NOLOAD)
>> +		rc = 0;
>> +	return rc;
>> }
>>
>> int uImage_arm_load(int argc, char **argv, const char *buf, off_t len,
>> diff --git a/kexec/arch/ppc/kexec-uImage-ppc.c b/kexec/arch/ppc/kexec-uImage-ppc.c
>> index e55bf94..eaea3c1 100644
>> --- a/kexec/arch/ppc/kexec-uImage-ppc.c
>> +++ b/kexec/arch/ppc/kexec-uImage-ppc.c
>> @@ -48,7 +48,13 @@ void uImage_ppc_usage(void)
>>
>> int uImage_ppc_probe(const char *buf, off_t len)
>> {
>> -	return uImage_probe(buf, len, IH_ARCH_PPC);
>> +	int type;
>> +	int rc = -1;
>> +
>> +	type = uImage_probe(buf, len, IH_ARCH_PPC);
>> +	if (type == IH_TYPE_KERNEL || type == IH_TYPE_KERNEL_NOLOAD)
>> +		rc = 0;
>> +	return rc;
>> }
>>
>> static int ppc_load_bare_bits(int argc, char **argv, const char *buf,
>> diff --git a/kexec/arch/sh/kexec-uImage-sh.c b/kexec/arch/sh/kexec-uImage-sh.c
>> index e983165..54bb073 100644
>> --- a/kexec/arch/sh/kexec-uImage-sh.c
>> +++ b/kexec/arch/sh/kexec-uImage-sh.c
>> @@ -13,7 +13,14 @@
>>
>> int uImage_sh_probe(const char *buf, off_t len)
>> {
>> -	return uImage_probe(buf, len, IH_ARCH_SH);
>> +	int type;
>> +	int rc = -1;
>> +
>> +	type = uImage_probe(buf, len, IH_ARCH_SH);
>> +	if (type == IH_TYPE_KERNEL || type == IH_TYPE_KERNEL_NOLOAD)
>> +		rc = uImage_probe(buf, len, IH_ARCH_SH);
>
> This looks like a typo compared to ppc/arm above. If so you could create
> uImage_probe_kernel() which has the type == check.
>

Yes, indeed. Thanks for catching that. Will fix that. It should have
been :
	rc = 0;

>> +
>> +	return rc;
>> }
>>
>> int uImage_sh_load(int argc, char **argv, const char *buf, off_t len,
>> diff --git a/kexec/kexec-uImage.c b/kexec/kexec-uImage.c
>> index 3bc85c5..dcbd635 100644
>> --- a/kexec/kexec-uImage.c
>> +++ b/kexec/kexec-uImage.c
>> @@ -16,6 +16,10 @@
>>   * Basic uImage loader. Not rocket science.
>>   */
>>
>> +/*
>> + * Returns the image type if everything goes well. This would
>> + * allow the user to decide if the image is of their interest.
>> + */
>> int uImage_probe(const unsigned char *buf, off_t len, unsigned int arch)
>> {
>> 	struct image_header header;
>> @@ -42,17 +46,17 @@ int uImage_probe(const unsigned char *buf, off_t len, unsigned int arch)
>> 	switch (header.ih_type) {
>> 	case IH_TYPE_KERNEL:
>> 	case IH_TYPE_KERNEL_NOLOAD:
>> +		/* XXX: Should we really check this ? */
> Yes.
>> +		if (header.ih_os != IH_OS_LINUX) {
>> +			printf("uImage os %d unsupported\n", header.ih_os);
>> +			return -1;
>> +		}
>
> Why did move this piece. Is it because ramdisk does not have an OS
> selected?
>
To be frank, I don't know. Is it otherwise ?

>> 		break;
>> 	default:
>> 		printf("uImage type %d unsupported\n", header.ih_type);
>> 		return -1;
>> 	}
>>
>> -	if (header.ih_os != IH_OS_LINUX) {
>> -		printf("uImage os %d unsupported\n", header.ih_os);
>> -		return -1;
>> -	}
>> -
>> 	if (header.ih_arch != arch) {
>> 		printf("uImage arch %d unsupported\n", header.ih_arch);
>> 		return -1;
>> @@ -84,7 +88,7 @@ int uImage_probe(const unsigned char *buf, off_t len, unsigned int arch)
>> 		return -1;
>> 	}
>> #endif
>> -	return 0;
>> +	return (int)header.ih_type;
>
> Why do you return type here?
>
This will help the caller to decide if the target is of his interest,
and deal with it. This will be used in the later patches in the series
to check for a RAMDISK type. See patch 2/3 and 3/3.

Thanks
Suzuki




_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] ppc/uImage: Add support for RAM Disks

[Suzuki K. Poulose]

On 01/16/2013 10:56 PM, Sebastian Andrzej Siewior wrote:
> * Suzuki K. Poulose | 2013-01-11 12:12:11 [+0530]:
>
> only some stylish comments…
>
>> diff --git a/kexec/arch/ppc/kexec-uImage-ppc.c b/kexec/arch/ppc/kexec-uImage-ppc.c
>> index eaea3c1..58935c0 100644
>> --- a/kexec/arch/ppc/kexec-uImage-ppc.c
>> +++ b/kexec/arch/ppc/kexec-uImage-ppc.c
>> @@ -46,6 +46,26 @@ void uImage_ppc_usage(void)
>> 	);
>> }
>>
>> +char* slurp_ramdisk_ppc(const char *filename, off_t *r_size)
> char *slurp_ramdisk_ppc(const char *filename, off_t *r_size)
> ?

OK
>
>> +{
>> +	struct Image_info img;
>> +	off_t size;
>> +	const unsigned char *buf =
>> +			(const unsigned char *)slurp_file(filename, &size);
>
> is the const cast required here?

Yes, the uImage_probe() expects (const unsigned char *) as the first
argument.
>
>> +	/* Check if this is a uImage RAMDisk */
>> +	if (buf != (void*)0 &&
> this should be NULL if at all
>
OK
>> +		uImage_probe(buf, size, IH_ARCH_PPC) == IH_TYPE_RAMDISK) {
> and maybe splitted in more than one line
>
OK

>> +		if (uImage_load(buf, size, &img) != 0)
>> +			die ("uImage: Reading %ld bytes from %s failed\n", size, filename);
> no space between the function.
>

OK
>> +		buf = img.buf;
>> +		size = img.len;
>> +	}
>> +
>> +	*r_size = size;
>> +	return buf;
>> +}
>> +	
>> int uImage_ppc_probe(const char *buf, off_t len)
>> {
>> 	int type;
>> @@ -196,7 +216,7 @@ static int ppc_load_bare_bits(int argc, char **argv, const char *buf,
>> 	blob_buf = fixup_dtb_init(info, blob_buf, &blob_size, load_addr, &dtb_addr);
>>
>> 	if (ramdisk) {
>> -		seg_buf = slurp_file(ramdisk, &seg_size);
>> +		seg_buf = slurp_ramdisk_ppc(ramdisk, &seg_size);
>
> I'm not sure but this kinda breaks the case where someone was using a
> plain file without the uImage header. I don't know if someone used this
> at all but it seems we have code for this. The way you load the uImage
> is very generic and I would expect to load it the same way on ARM or SH.
>
If the ramdisk doesn't have uImage header, the slurp_ramdisk_ppc()
defaults to the original behaviour. i.e, it just does a slurp_file()
as it used to do earlier.


> One thing that is different compared to kernel: If the compression is
> set to GZ on a ramdisk image you should not uncompress it. I think that
> uImage_load() decompresses the .gz part for you but I am not sure.
>

Did you mean, uImage_load() shouldn't do a decompression for ramdisk ?
We rely on uImage_load to get what is required.

>> 		/* Load ramdisk at top of memory */
>> 		hole_addr = add_buffer(info, seg_buf, seg_size, seg_size,
>> 				0, dtb_addr + blob_size, max_addr, -1);
>>
>

Thanks for the detailed review.

Suzuki

> Sebastian
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] ppc/uImage: Add support for RAM Disks

[Sebastian Andrzej Siewior]

On 01/17/2013 10:47 AM, Suzuki K. Poulose wrote:
>>> +{
>>> +    struct Image_info img;
>>> +    off_t size;
>>> +    const unsigned char *buf =
>>> +            (const unsigned char *)slurp_file(filename, &size);
>>
>> is the const cast required here?
> 
> Yes, the uImage_probe() expects (const unsigned char *) as the first
> argument.

That is okay, you can pass a non-const pointer to function that expects
a const pointer. You just can't pass a const pointer to a non-const
function and you should not cast in that situtation as well.

>>> +        buf = img.buf;
>>> +        size = img.len;
>>> +    }
>>> +
>>> +    *r_size = size;
>>> +    return buf;
>>> +}
>>> +   
>>> int uImage_ppc_probe(const char *buf, off_t len)
>>> {
>>>     int type;
>>> @@ -196,7 +216,7 @@ static int ppc_load_bare_bits(int argc, char
>>> **argv, const char *buf,
>>>     blob_buf = fixup_dtb_init(info, blob_buf, &blob_size, load_addr,
>>> &dtb_addr);
>>>
>>>     if (ramdisk) {
>>> -        seg_buf = slurp_file(ramdisk, &seg_size);
>>> +        seg_buf = slurp_ramdisk_ppc(ramdisk, &seg_size);
>>
>> I'm not sure but this kinda breaks the case where someone was using a
>> plain file without the uImage header. I don't know if someone used this
>> at all but it seems we have code for this. The way you load the uImage
>> is very generic and I would expect to load it the same way on ARM or SH.
>>
> If the ramdisk doesn't have uImage header, the slurp_ramdisk_ppc()
> defaults to the original behaviour. i.e, it just does a slurp_file()
> as it used to do earlier.
that is okay then.

>> One thing that is different compared to kernel: If the compression is
>> set to GZ on a ramdisk image you should not uncompress it. I think that
>> uImage_load() decompresses the .gz part for you but I am not sure.
>>
> 
> Did you mean, uImage_load() shouldn't do a decompression for ramdisk ?

Yes. If you look at u-boot source, you will notice that it will
decompress a kernel if it is gzip/bzip2/whatever compressed and ignore
the compression field in case of a ramdisk. I think you should behave
like u-boot does.

Sebastian

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec