[U-Boot] [PATCH] image: Fix Android boot image support

[U-Boot] [PATCH] image: Fix Android boot image support

[Ahmad Draidi]

This patch makes the following changes:
- Set kernel entry point correctly
- Append bootargs from image to global bootargs instead
        of replacing them
- Return end address instead of size from android_image_get_end()
- Give correct parameter to genimg_get_format() in boot_get_ramdisk()

Signed-off-by: Ahmad Draidi <ar2000jp@gmail.com>
Cc: Tom Rini <trini@ti.com>
---
 common/bootm.c         |  4 ++--
 common/image-android.c | 34 +++++++++++++++++++++++++++-------
 common/image.c         |  3 ++-
 3 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index ff81a27..c04a3b0 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -144,11 +144,11 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int argc,
 		images.os.type = IH_TYPE_KERNEL;
 		images.os.comp = IH_COMP_NONE;
 		images.os.os = IH_OS_LINUX;
-		images.ep = images.os.load;
-		ep_found = true;
 
 		images.os.end = android_image_get_end(os_hdr);
 		images.os.load = android_image_get_kload(os_hdr);
+		images.ep = images.os.load;
+		ep_found = true;
 		break;
 #endif
 	default:
diff --git a/common/image-android.c b/common/image-android.c
index 6ded7e2..badaa7e 100644
--- a/common/image-android.c
+++ b/common/image-android.c
@@ -7,6 +7,7 @@
 #include <common.h>
 #include <image.h>
 #include <android_image.h>
+#include <malloc.h>
 
 static char andr_tmp_str[ANDR_BOOT_ARGS_SIZE + 1];
 
@@ -25,12 +26,30 @@ int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
 
 	printf("Kernel load addr 0x%08x size %u KiB\n",
 	       hdr->kernel_addr, DIV_ROUND_UP(hdr->kernel_size, 1024));
+
 	strncpy(andr_tmp_str, hdr->cmdline, ANDR_BOOT_ARGS_SIZE);
 	andr_tmp_str[ANDR_BOOT_ARGS_SIZE] = '\0';
 	if (strlen(andr_tmp_str)) {
 		printf("Kernel command line: %s\n", andr_tmp_str);
-		setenv("bootargs", andr_tmp_str);
+		char *bootargs = getenv("bootargs");
+		if (bootargs == NULL) {
+			setenv("bootargs", andr_tmp_str);
+		} else {
+			char *newbootargs = malloc(strlen(bootargs) +
+						strlen(andr_tmp_str) + 1);
+			if (newbootargs == NULL) {
+				puts("Error: malloc in android_image_get_kernel failed!\n");
+				return -1;
+			}
+
+			strcpy(newbootargs, bootargs);
+			strcat(newbootargs, " ");
+			strncat(newbootargs, andr_tmp_str, ANDR_BOOT_ARGS_SIZE);
+
+			setenv("bootargs", newbootargs);
+		}
 	}
+
 	if (hdr->ramdisk_size)
 		printf("RAM disk load addr 0x%08x size %u KiB\n",
 		       hdr->ramdisk_addr,
@@ -52,17 +71,18 @@ int android_image_check_header(const struct andr_img_hdr *hdr)
 
 ulong android_image_get_end(const struct andr_img_hdr *hdr)
 {
-	u32 size = 0;
+	ulong end;
 	/*
 	 * The header takes a full page, the remaining components are aligned
 	 * on page boundary
 	 */
-	size += hdr->page_size;
-	size += ALIGN(hdr->kernel_size, hdr->page_size);
-	size += ALIGN(hdr->ramdisk_size, hdr->page_size);
-	size += ALIGN(hdr->second_size, hdr->page_size);
+	end = (ulong)hdr;
+	end += hdr->page_size;
+	end += ALIGN(hdr->kernel_size, hdr->page_size);
+	end += ALIGN(hdr->ramdisk_size, hdr->page_size);
+	end += ALIGN(hdr->second_size, hdr->page_size);
 
-	return size;
+	return end;
 }
 
 ulong android_image_get_kload(const struct andr_img_hdr *hdr)
diff --git a/common/image.c b/common/image.c
index 085771c..e21c848 100644
--- a/common/image.c
+++ b/common/image.c
@@ -1009,7 +1009,8 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images,
 		image_multi_getimg(images->legacy_hdr_os, 1, &rd_data, &rd_len);
 	}
 #ifdef CONFIG_ANDROID_BOOT_IMAGE
-	else if ((genimg_get_format(images) == IMAGE_FORMAT_ANDROID) &&
+	else if ((genimg_get_format((void *)images->os.start)
+			== IMAGE_FORMAT_ANDROID) &&
 		 (!android_image_get_ramdisk((void *)images->os.start,
 		 &rd_data, &rd_len))) {
 		/* empty */
-- 
2.1.1

[U-Boot] [PATCH] image: Fix Android boot image support

[Simon Glass]

Hi,

On 16 October 2014 22:52, Ahmad Draidi <ar2000jp@gmail.com> wrote:
> This patch makes the following changes:
> - Set kernel entry point correctly
> - Append bootargs from image to global bootargs instead
>         of replacing them
> - Return end address instead of size from android_image_get_end()
> - Give correct parameter to genimg_get_format() in boot_get_ramdisk()

I wonder if it might be possible to add a sandbox test for this? I
feel that we are at great risk of breaking these things without some
automated tests. There are tests for FIT and legacy images.

Regards,
Simon

[U-Boot] [PATCH] image: Fix Android boot image support

[Ahmad Draidi]

Hello Mr. Glass

On Fri, Oct 17, 2014 at 11:19 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi,
>
> On 16 October 2014 22:52, Ahmad Draidi <ar2000jp@gmail.com> wrote:
>> This patch makes the following changes:
>> - Set kernel entry point correctly
>> - Append bootargs from image to global bootargs instead
>>         of replacing them
>> - Return end address instead of size from android_image_get_end()
>> - Give correct parameter to genimg_get_format() in boot_get_ramdisk()
>
> I wonder if it might be possible to add a sandbox test for this? I
> feel that we are at great risk of breaking these things without some
> automated tests. There are tests for FIT and legacy images.
>
> Regards,
> Simon

I'll see if I can write one.
If I do, should I submit it as a separate patch series? Sorry, I'm new to this.
Any notes on this patch?

Regards,
Ahmad Draidi

[U-Boot] [PATCH] image: Fix Android boot image support

[Simon Glass]

Hi Ahmad,

On 17 October 2014 23:34, Ahmad Draidi <ar2000jp@gmail.com> wrote:
> Hello Mr. Glass
>
> On Fri, Oct 17, 2014 at 11:19 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi,
>>
>> On 16 October 2014 22:52, Ahmad Draidi <ar2000jp@gmail.com> wrote:
>>> This patch makes the following changes:
>>> - Set kernel entry point correctly
>>> - Append bootargs from image to global bootargs instead
>>>         of replacing them
>>> - Return end address instead of size from android_image_get_end()
>>> - Give correct parameter to genimg_get_format() in boot_get_ramdisk()
>>
>> I wonder if it might be possible to add a sandbox test for this? I
>> feel that we are at great risk of breaking these things without some
>> automated tests. There are tests for FIT and legacy images.
>>
>> Regards,
>> Simon
>
> I'll see if I can write one.
> If I do, should I submit it as a separate patch series? Sorry, I'm new to this.
> Any notes on this patch?

You can submit it as a separate patch but perhaps in the same series.
I'll check the patch.

Regards,
Simon

[U-Boot] [PATCH] image: Fix Android boot image support

[Simon Glass]

Hi,

On 16 October 2014 22:52, Ahmad Draidi <ar2000jp@gmail.com> wrote:
> This patch makes the following changes:
> - Set kernel entry point correctly
> - Append bootargs from image to global bootargs instead
>         of replacing them
> - Return end address instead of size from android_image_get_end()
> - Give correct parameter to genimg_get_format() in boot_get_ramdisk()
>
> Signed-off-by: Ahmad Draidi <ar2000jp@gmail.com>
> Cc: Tom Rini <trini@ti.com>
> ---
>  common/bootm.c         |  4 ++--
>  common/image-android.c | 34 +++++++++++++++++++++++++++-------
>  common/image.c         |  3 ++-
>  3 files changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/common/bootm.c b/common/bootm.c
> index ff81a27..c04a3b0 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -144,11 +144,11 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int argc,
>                 images.os.type = IH_TYPE_KERNEL;
>                 images.os.comp = IH_COMP_NONE;
>                 images.os.os = IH_OS_LINUX;
> -               images.ep = images.os.load;
> -               ep_found = true;
>
>                 images.os.end = android_image_get_end(os_hdr);
>                 images.os.load = android_image_get_kload(os_hdr);
> +               images.ep = images.os.load;
> +               ep_found = true;
>                 break;
>  #endif
>         default:
> diff --git a/common/image-android.c b/common/image-android.c
> index 6ded7e2..badaa7e 100644
> --- a/common/image-android.c
> +++ b/common/image-android.c
> @@ -7,6 +7,7 @@
>  #include <common.h>
>  #include <image.h>
>  #include <android_image.h>
> +#include <malloc.h>
>
>  static char andr_tmp_str[ANDR_BOOT_ARGS_SIZE + 1];
>
> @@ -25,12 +26,30 @@ int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
>
>         printf("Kernel load addr 0x%08x size %u KiB\n",
>                hdr->kernel_addr, DIV_ROUND_UP(hdr->kernel_size, 1024));
> +
>         strncpy(andr_tmp_str, hdr->cmdline, ANDR_BOOT_ARGS_SIZE);
>         andr_tmp_str[ANDR_BOOT_ARGS_SIZE] = '\0';
>         if (strlen(andr_tmp_str)) {
>                 printf("Kernel command line: %s\n", andr_tmp_str);
> -               setenv("bootargs", andr_tmp_str);
> +               char *bootargs = getenv("bootargs");
> +               if (bootargs == NULL) {
> +                       setenv("bootargs", andr_tmp_str);
> +               } else {
> +                       char *newbootargs = malloc(strlen(bootargs) +
> +                                               strlen(andr_tmp_str) + 1);
> +                       if (newbootargs == NULL) {
> +                               puts("Error: malloc in android_image_get_kernel failed!\n");
> +                               return -1;
> +                       }
> +
> +                       strcpy(newbootargs, bootargs);
> +                       strcat(newbootargs, " ");
> +                       strncat(newbootargs, andr_tmp_str, ANDR_BOOT_ARGS_SIZE);

Why have ANDR_BOOT_ARGS_SIZE? If you are going to malloc() anyway, you
may as well avoid the limit. Something like:

char *bootargs = getenv("bootargs");
int len = 0;

if (*hdr->cmdline)
   len += strlen(hdr->cmdline);
if (bootargs)
   len += strlen(bootargs);
malloc(len +1) bytes
copy them in

> +
> +                       setenv("bootargs", newbootargs);
> +               }
>         }
> +
>         if (hdr->ramdisk_size)
>                 printf("RAM disk load addr 0x%08x size %u KiB\n",
>                        hdr->ramdisk_addr,
> @@ -52,17 +71,18 @@ int android_image_check_header(const struct andr_img_hdr *hdr)
>
>  ulong android_image_get_end(const struct andr_img_hdr *hdr)
>  {
> -       u32 size = 0;
> +       ulong end;
>         /*
>          * The header takes a full page, the remaining components are aligned
>          * on page boundary
>          */
> -       size += hdr->page_size;
> -       size += ALIGN(hdr->kernel_size, hdr->page_size);
> -       size += ALIGN(hdr->ramdisk_size, hdr->page_size);
> -       size += ALIGN(hdr->second_size, hdr->page_size);
> +       end = (ulong)hdr;
> +       end += hdr->page_size;
> +       end += ALIGN(hdr->kernel_size, hdr->page_size);
> +       end += ALIGN(hdr->ramdisk_size, hdr->page_size);
> +       end += ALIGN(hdr->second_size, hdr->page_size);
>
> -       return size;
> +       return end;
>  }
>
>  ulong android_image_get_kload(const struct andr_img_hdr *hdr)
> diff --git a/common/image.c b/common/image.c
> index 085771c..e21c848 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -1009,7 +1009,8 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images,
>                 image_multi_getimg(images->legacy_hdr_os, 1, &rd_data, &rd_len);
>         }
>  #ifdef CONFIG_ANDROID_BOOT_IMAGE
> -       else if ((genimg_get_format(images) == IMAGE_FORMAT_ANDROID) &&
> +       else if ((genimg_get_format((void *)images->os.start)
> +                       == IMAGE_FORMAT_ANDROID) &&
>                  (!android_image_get_ramdisk((void *)images->os.start,
>                  &rd_data, &rd_len))) {
>                 /* empty */
> --

Regards,
Simon

[U-Boot] [PATCH] image: Fix Android boot image support

[Ahmad Draidi]

Hello, Mr. Glass

On Tue, Oct 21, 2014 at 12:42 AM, Simon Glass <sjg@chromium.org> wrote:
>
> Why have ANDR_BOOT_ARGS_SIZE? If you are going to malloc() anyway, you
> may as well avoid the limit. Something like:
>
> char *bootargs = getenv("bootargs");
> int len = 0;
>
> if (*hdr->cmdline)
>    len += strlen(hdr->cmdline);
> if (bootargs)
>    len += strlen(bootargs);
> malloc(len +1) bytes
> copy them in
>

I wanted to make my modifications as little as possible, since the
code might be addressing some corner case in the image making tool.
I checked the code for the mkbootimg tool, and verified that there are
no need for the extra checks.
I'll send the second version of the patch soon.

Regards,
Ahmad Draidi

[U-Boot] [PATCH v2] image: Fix Android boot image support

[Ahmad Draidi]

This patch makes the following changes:
- Set kernel entry point correctly
- Append bootargs from image to global bootargs instead
        of replacing them
- Return end address instead of size from android_image_get_end()
- Give correct parameter to genimg_get_format() in boot_get_ramdisk()

Signed-off-by: Ahmad Draidi <ar2000jp@gmail.com>
Cc: Tom Rini <trini@ti.com>
---
Changes for v2:
   - Cleanup bootargs copying code
   - Coding Style cleanup
---
 common/bootm.c         |  4 ++--
 common/image-android.c | 45 ++++++++++++++++++++++++++++++++++-----------
 common/image.c         |  3 ++-
 3 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index ff81a27..c04a3b0 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -144,11 +144,11 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int argc,
 		images.os.type = IH_TYPE_KERNEL;
 		images.os.comp = IH_COMP_NONE;
 		images.os.os = IH_OS_LINUX;
-		images.ep = images.os.load;
-		ep_found = true;
 
 		images.os.end = android_image_get_end(os_hdr);
 		images.os.load = android_image_get_kload(os_hdr);
+		images.ep = images.os.load;
+		ep_found = true;
 		break;
 #endif
 	default:
diff --git a/common/image-android.c b/common/image-android.c
index 6ded7e2..673cbca 100644
--- a/common/image-android.c
+++ b/common/image-android.c
@@ -7,6 +7,7 @@
 #include <common.h>
 #include <image.h>
 #include <android_image.h>
+#include <malloc.h>
 
 static char andr_tmp_str[ANDR_BOOT_ARGS_SIZE + 1];
 
@@ -25,12 +26,33 @@ int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
 
 	printf("Kernel load addr 0x%08x size %u KiB\n",
 	       hdr->kernel_addr, DIV_ROUND_UP(hdr->kernel_size, 1024));
-	strncpy(andr_tmp_str, hdr->cmdline, ANDR_BOOT_ARGS_SIZE);
-	andr_tmp_str[ANDR_BOOT_ARGS_SIZE] = '\0';
-	if (strlen(andr_tmp_str)) {
-		printf("Kernel command line: %s\n", andr_tmp_str);
-		setenv("bootargs", andr_tmp_str);
+
+	int len = 0;
+	if (*hdr->cmdline) {
+		printf("Kernel command line: %s\n", hdr->cmdline);
+		len += strlen(hdr->cmdline);
+	}
+
+	char *bootargs = getenv("bootargs");
+	if (bootargs)
+		len += strlen(bootargs);
+
+	char *newbootargs = malloc(len + 2);
+	if (!newbootargs) {
+		puts("Error: malloc in android_image_get_kernel failed!\n");
+		return -1;
 	}
+	*newbootargs = '\0';
+
+	if (bootargs) {
+		strcpy(newbootargs, bootargs);
+		strcat(newbootargs, " ");
+	}
+	if (*hdr->cmdline)
+		strcat(newbootargs, hdr->cmdline);
+
+	setenv("bootargs", newbootargs);
+
 	if (hdr->ramdisk_size)
 		printf("RAM disk load addr 0x%08x size %u KiB\n",
 		       hdr->ramdisk_addr,
@@ -52,17 +74,18 @@ int android_image_check_header(const struct andr_img_hdr *hdr)
 
 ulong android_image_get_end(const struct andr_img_hdr *hdr)
 {
-	u32 size = 0;
+	ulong end;
 	/*
 	 * The header takes a full page, the remaining components are aligned
 	 * on page boundary
 	 */
-	size += hdr->page_size;
-	size += ALIGN(hdr->kernel_size, hdr->page_size);
-	size += ALIGN(hdr->ramdisk_size, hdr->page_size);
-	size += ALIGN(hdr->second_size, hdr->page_size);
+	end = (ulong)hdr;
+	end += hdr->page_size;
+	end += ALIGN(hdr->kernel_size, hdr->page_size);
+	end += ALIGN(hdr->ramdisk_size, hdr->page_size);
+	end += ALIGN(hdr->second_size, hdr->page_size);
 
-	return size;
+	return end;
 }
 
 ulong android_image_get_kload(const struct andr_img_hdr *hdr)
diff --git a/common/image.c b/common/image.c
index 085771c..e21c848 100644
--- a/common/image.c
+++ b/common/image.c
@@ -1009,7 +1009,8 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images,
 		image_multi_getimg(images->legacy_hdr_os, 1, &rd_data, &rd_len);
 	}
 #ifdef CONFIG_ANDROID_BOOT_IMAGE
-	else if ((genimg_get_format(images) == IMAGE_FORMAT_ANDROID) &&
+	else if ((genimg_get_format((void *)images->os.start)
+			== IMAGE_FORMAT_ANDROID) &&
 		 (!android_image_get_ramdisk((void *)images->os.start,
 		 &rd_data, &rd_len))) {
 		/* empty */
-- 
2.1.1

[U-Boot] [PATCH v2] image: Fix Android boot image support

[Simon Glass]

Hi Ahmad,

On 21 October 2014 10:55, Ahmad Draidi <ar2000jp@gmail.com> wrote:
> This patch makes the following changes:
> - Set kernel entry point correctly
> - Append bootargs from image to global bootargs instead
>         of replacing them
> - Return end address instead of size from android_image_get_end()
> - Give correct parameter to genimg_get_format() in boot_get_ramdisk()
>
> Signed-off-by: Ahmad Draidi <ar2000jp@gmail.com>
> Cc: Tom Rini <trini@ti.com>
> ---
> Changes for v2:
>    - Cleanup bootargs copying code
>    - Coding Style cleanup

One little nit below but it looks OK to me. I'm assume that no one
would want to replace the command line completely?

I hope you can write a test too. Re your comment about not wanting to
change the code much - if we go that way the code will get really
ugly. When you add features you often need to refactor. When there are
tests, it becomes easier to know you have not broken things.

Reviewed-by: Simon Glass <sjg@chromium.org>

> ---
>  common/bootm.c         |  4 ++--
>  common/image-android.c | 45 ++++++++++++++++++++++++++++++++++-----------
>  common/image.c         |  3 ++-
>  3 files changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/common/bootm.c b/common/bootm.c
> index ff81a27..c04a3b0 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -144,11 +144,11 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int argc,
>                 images.os.type = IH_TYPE_KERNEL;
>                 images.os.comp = IH_COMP_NONE;
>                 images.os.os = IH_OS_LINUX;
> -               images.ep = images.os.load;
> -               ep_found = true;
>
>                 images.os.end = android_image_get_end(os_hdr);
>                 images.os.load = android_image_get_kload(os_hdr);
> +               images.ep = images.os.load;
> +               ep_found = true;
>                 break;
>  #endif
>         default:
> diff --git a/common/image-android.c b/common/image-android.c
> index 6ded7e2..673cbca 100644
> --- a/common/image-android.c
> +++ b/common/image-android.c
> @@ -7,6 +7,7 @@
>  #include <common.h>
>  #include <image.h>
>  #include <android_image.h>
> +#include <malloc.h>
>
>  static char andr_tmp_str[ANDR_BOOT_ARGS_SIZE + 1];
>
> @@ -25,12 +26,33 @@ int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
>
>         printf("Kernel load addr 0x%08x size %u KiB\n",
>                hdr->kernel_addr, DIV_ROUND_UP(hdr->kernel_size, 1024));
> -       strncpy(andr_tmp_str, hdr->cmdline, ANDR_BOOT_ARGS_SIZE);
> -       andr_tmp_str[ANDR_BOOT_ARGS_SIZE] = '\0';
> -       if (strlen(andr_tmp_str)) {
> -               printf("Kernel command line: %s\n", andr_tmp_str);
> -               setenv("bootargs", andr_tmp_str);
> +
> +       int len = 0;
> +       if (*hdr->cmdline) {
> +               printf("Kernel command line: %s\n", hdr->cmdline);
> +               len += strlen(hdr->cmdline);
> +       }
> +
> +       char *bootargs = getenv("bootargs");
> +       if (bootargs)
> +               len += strlen(bootargs);
> +
> +       char *newbootargs = malloc(len + 2);
> +       if (!newbootargs) {
> +               puts("Error: malloc in android_image_get_kernel failed!\n");
> +               return -1;

nit: return -ENOMEM - suggest adding a comment to
android_image_get_kernel() so its args and return value are
documented.

>         }
> +       *newbootargs = '\0';
> +
> +       if (bootargs) {
> +               strcpy(newbootargs, bootargs);
> +               strcat(newbootargs, " ");
> +       }
> +       if (*hdr->cmdline)
> +               strcat(newbootargs, hdr->cmdline);
> +
> +       setenv("bootargs", newbootargs);
> +
>         if (hdr->ramdisk_size)
>                 printf("RAM disk load addr 0x%08x size %u KiB\n",
>                        hdr->ramdisk_addr,
> @@ -52,17 +74,18 @@ int android_image_check_header(const struct andr_img_hdr *hdr)
>
>  ulong android_image_get_end(const struct andr_img_hdr *hdr)
>  {
> -       u32 size = 0;
> +       ulong end;
>         /*
>          * The header takes a full page, the remaining components are aligned
>          * on page boundary
>          */
> -       size += hdr->page_size;
> -       size += ALIGN(hdr->kernel_size, hdr->page_size);
> -       size += ALIGN(hdr->ramdisk_size, hdr->page_size);
> -       size += ALIGN(hdr->second_size, hdr->page_size);
> +       end = (ulong)hdr;
> +       end += hdr->page_size;
> +       end += ALIGN(hdr->kernel_size, hdr->page_size);
> +       end += ALIGN(hdr->ramdisk_size, hdr->page_size);
> +       end += ALIGN(hdr->second_size, hdr->page_size);
>
> -       return size;
> +       return end;
>  }
>
>  ulong android_image_get_kload(const struct andr_img_hdr *hdr)
> diff --git a/common/image.c b/common/image.c
> index 085771c..e21c848 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -1009,7 +1009,8 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images,
>                 image_multi_getimg(images->legacy_hdr_os, 1, &rd_data, &rd_len);
>         }
>  #ifdef CONFIG_ANDROID_BOOT_IMAGE
> -       else if ((genimg_get_format(images) == IMAGE_FORMAT_ANDROID) &&
> +       else if ((genimg_get_format((void *)images->os.start)
> +                       == IMAGE_FORMAT_ANDROID) &&
>                  (!android_image_get_ramdisk((void *)images->os.start,
>                  &rd_data, &rd_len))) {
>                 /* empty */
> --
> 2.1.1

Regards,
Simon

[U-Boot] [PATCH v2] image: Fix Android boot image support

[Ahmad Draidi]

Hello, Mr. Glass.

On Wednesday 22 October 2014 11:29:00 AM Simon Glass wrote:
> One little nit below but it looks OK to me. I'm assume that no one
> would want to replace the command line completely?
> 

In some setups, one image can be used with several versions, or even
    different boards of hardware. The bootloader passes a command line
    argument to specify this. Also, the header file for the mkbootimg
    specifies that the "kernel_args[] is appended to the kernel
    commandline". So I thought this would make it consistent with other
    bootloaders, and the spec.

> I hope you can write a test too. Re your comment about not wanting to
> change the code much - if we go that way the code will get really
> ugly. When you add features you often need to refactor. When there are
> tests, it becomes easier to know you have not broken things.

Thanks for the tip.
I'll try to write a test if I get the time.
One thing comes to mind. To be able to write a test, we'll need an image
    to test against. I think pulling in the mkbootimg tool is the right move.
    The other option I can think of is bundling a small dummy image with
    U-Boot, which I think is a bit ugly. What do you think?

> Reviewed-by: Simon Glass <sjg@chromium.org>
> nit: return -ENOMEM - suggest adding a comment to
> android_image_get_kernel() so its args and return value are
> documented.
> 

Thank you. I'll send in a new version soon.

> Regards,
> Simon

Regards,
Ahmad Draidi

[U-Boot] [PATCH v3] image: Fix Android boot image support

[Ahmad Draidi]

This patch makes the following changes:
- Set kernel entry point correctly
- Append bootargs from image to global bootargs instead
        of replacing them
- Return end address instead of size from android_image_get_end()
- Give correct parameter to genimg_get_format() in boot_get_ramdisk()
- Move ramdisk message printing from android_image_get_kernel() to
	android_image_get_ramdisk()

Signed-off-by: Ahmad Draidi <ar2000jp@gmail.com>
Cc: Tom Rini <trini@ti.com>
---
Changes for v2:
   - Cleanup bootargs copying code
   - Coding Style cleanup
Changes for v3:
   - In android_image_get_kernel(), return -ENOMEM instead of -1
	on malloc() failure
   - Move ramdisk message printing from android_image_get_kernel()
	to android_image_get_ramdisk()
   - Add android_image_get_kernel() documentation comment
---
 common/bootm.c         |  4 +--
 common/image-android.c | 68 +++++++++++++++++++++++++++++++++++++++-----------
 common/image.c         |  3 ++-
 3 files changed, 57 insertions(+), 18 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index ff81a27..c04a3b0 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -144,11 +144,11 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int argc,
 		images.os.type = IH_TYPE_KERNEL;
 		images.os.comp = IH_COMP_NONE;
 		images.os.os = IH_OS_LINUX;
-		images.ep = images.os.load;
-		ep_found = true;
 
 		images.os.end = android_image_get_end(os_hdr);
 		images.os.load = android_image_get_kload(os_hdr);
+		images.ep = images.os.load;
+		ep_found = true;
 		break;
 #endif
 	default:
diff --git a/common/image-android.c b/common/image-android.c
index 6ded7e2..59079fc 100644
--- a/common/image-android.c
+++ b/common/image-android.c
@@ -7,9 +7,26 @@
 #include <common.h>
 #include <image.h>
 #include <android_image.h>
+#include <malloc.h>
+#include <errno.h>
 
 static char andr_tmp_str[ANDR_BOOT_ARGS_SIZE + 1];
 
+/**
+ * android_image_get_kernel() - processes kernel part of Android boot images
+ * @hdr:	Pointer to image header, which is at the start
+ *			of the image.
+ * @verify:	Checksum verification flag. Currently unimplemented.
+ * @os_data:	Pointer to a ulong variable, will hold os data start
+ *			address.
+ * @os_len:	Pointer to a ulong variable, will hold os data length.
+ *
+ * This function returns the os image's start address and length. Also,
+ * it appends the kernel command line to the bootargs env variable.
+ *
+ * Return: Zero, os start address and length on success,
+ *		otherwise on failure.
+ */
 int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
 			     ulong *os_data, ulong *os_len)
 {
@@ -25,16 +42,32 @@ int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
 
 	printf("Kernel load addr 0x%08x size %u KiB\n",
 	       hdr->kernel_addr, DIV_ROUND_UP(hdr->kernel_size, 1024));
-	strncpy(andr_tmp_str, hdr->cmdline, ANDR_BOOT_ARGS_SIZE);
-	andr_tmp_str[ANDR_BOOT_ARGS_SIZE] = '\0';
-	if (strlen(andr_tmp_str)) {
-		printf("Kernel command line: %s\n", andr_tmp_str);
-		setenv("bootargs", andr_tmp_str);
+
+	int len = 0;
+	if (*hdr->cmdline) {
+		printf("Kernel command line: %s\n", hdr->cmdline);
+		len += strlen(hdr->cmdline);
+	}
+
+	char *bootargs = getenv("bootargs");
+	if (bootargs)
+		len += strlen(bootargs);
+
+	char *newbootargs = malloc(len + 2);
+	if (!newbootargs) {
+		puts("Error: malloc in android_image_get_kernel failed!\n");
+		return -ENOMEM;
+	}
+	*newbootargs = '\0';
+
+	if (bootargs) {
+		strcpy(newbootargs, bootargs);
+		strcat(newbootargs, " ");
 	}
-	if (hdr->ramdisk_size)
-		printf("RAM disk load addr 0x%08x size %u KiB\n",
-		       hdr->ramdisk_addr,
-		       DIV_ROUND_UP(hdr->ramdisk_size, 1024));
+	if (*hdr->cmdline)
+		strcat(newbootargs, hdr->cmdline);
+
+	setenv("bootargs", newbootargs);
 
 	if (os_data) {
 		*os_data = (ulong)hdr;
@@ -52,17 +85,18 @@ int android_image_check_header(const struct andr_img_hdr *hdr)
 
 ulong android_image_get_end(const struct andr_img_hdr *hdr)
 {
-	u32 size = 0;
+	ulong end;
 	/*
 	 * The header takes a full page, the remaining components are aligned
 	 * on page boundary
 	 */
-	size += hdr->page_size;
-	size += ALIGN(hdr->kernel_size, hdr->page_size);
-	size += ALIGN(hdr->ramdisk_size, hdr->page_size);
-	size += ALIGN(hdr->second_size, hdr->page_size);
+	end = (ulong)hdr;
+	end += hdr->page_size;
+	end += ALIGN(hdr->kernel_size, hdr->page_size);
+	end += ALIGN(hdr->ramdisk_size, hdr->page_size);
+	end += ALIGN(hdr->second_size, hdr->page_size);
 
-	return size;
+	return end;
 }
 
 ulong android_image_get_kload(const struct andr_img_hdr *hdr)
@@ -75,6 +109,10 @@ int android_image_get_ramdisk(const struct andr_img_hdr *hdr,
 {
 	if (!hdr->ramdisk_size)
 		return -1;
+
+	printf("RAM disk load addr 0x%08x size %u KiB\n",
+	       hdr->ramdisk_addr, DIV_ROUND_UP(hdr->ramdisk_size, 1024));
+
 	*rd_data = (unsigned long)hdr;
 	*rd_data += hdr->page_size;
 	*rd_data += ALIGN(hdr->kernel_size, hdr->page_size);
diff --git a/common/image.c b/common/image.c
index 085771c..e21c848 100644
--- a/common/image.c
+++ b/common/image.c
@@ -1009,7 +1009,8 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images,
 		image_multi_getimg(images->legacy_hdr_os, 1, &rd_data, &rd_len);
 	}
 #ifdef CONFIG_ANDROID_BOOT_IMAGE
-	else if ((genimg_get_format(images) == IMAGE_FORMAT_ANDROID) &&
+	else if ((genimg_get_format((void *)images->os.start)
+			== IMAGE_FORMAT_ANDROID) &&
 		 (!android_image_get_ramdisk((void *)images->os.start,
 		 &rd_data, &rd_len))) {
 		/* empty */
-- 
2.1.1

[U-Boot] [PATCH v2] image: Fix Android boot image support

[Simon Glass]

Hi Ahmad,

On 23 October 2014 00:08, Ahmad Draidi <ar2000jp@gmail.com> wrote:
> Hello, Mr. Glass.
>
> On Wednesday 22 October 2014 11:29:00 AM Simon Glass wrote:
>> One little nit below but it looks OK to me. I'm assume that no one
>> would want to replace the command line completely?
>>
>
> In some setups, one image can be used with several versions, or even
>     different boards of hardware. The bootloader passes a command line
>     argument to specify this. Also, the header file for the mkbootimg
>     specifies that the "kernel_args[] is appended to the kernel
>     commandline". So I thought this would make it consistent with other
>     bootloaders, and the spec.
>
>> I hope you can write a test too. Re your comment about not wanting to
>> change the code much - if we go that way the code will get really
>> ugly. When you add features you often need to refactor. When there are
>> tests, it becomes easier to know you have not broken things.
>
> Thanks for the tip.
> I'll try to write a test if I get the time.
> One thing comes to mind. To be able to write a test, we'll need an image
>     to test against. I think pulling in the mkbootimg tool is the right move.
>     The other option I can think of is bundling a small dummy image with
>     U-Boot, which I think is a bit ugly. What do you think?

Yes bringing in the tool sounds good. It should probably be integrated
as just another output from mkimage.

>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> nit: return -ENOMEM - suggest adding a comment to
>> android_image_get_kernel() so its args and return value are
>> documented.
>>
>
> Thank you. I'll send in a new version soon.
>
>> Regards,
>> Simon
>
> Regards,
> Ahmad Draidi

Regards.
Simon

[U-Boot] [PATCH v3] image: Fix Android boot image support

[Simon Glass]

On 23 October 2014 11:50, Ahmad Draidi <ar2000jp@gmail.com> wrote:
> This patch makes the following changes:
> - Set kernel entry point correctly
> - Append bootargs from image to global bootargs instead
>         of replacing them
> - Return end address instead of size from android_image_get_end()
> - Give correct parameter to genimg_get_format() in boot_get_ramdisk()
> - Move ramdisk message printing from android_image_get_kernel() to
>         android_image_get_ramdisk()
>
> Signed-off-by: Ahmad Draidi <ar2000jp@gmail.com>
> Cc: Tom Rini <trini@ti.com>

Reviewed-by: Simon Glass <sjg@chromium.org>

BTW if you have not already seen it, take a look at patman
(tools/patman/patman) for checking and sending patches.

Regards,
Simon

[U-Boot] [U-Boot,v3] image: Fix Android boot image support

[Tom Rini]

On Thu, Oct 23, 2014 at 08:50:07PM +0300, Ahmad Draidi wrote:

> This patch makes the following changes:
> - Set kernel entry point correctly
> - Append bootargs from image to global bootargs instead
>         of replacing them
> - Return end address instead of size from android_image_get_end()
> - Give correct parameter to genimg_get_format() in boot_get_ramdisk()
> - Move ramdisk message printing from android_image_get_kernel() to
> 	android_image_get_ramdisk()
> 
> Signed-off-by: Ahmad Draidi <ar2000jp@gmail.com>
> Cc: Tom Rini <trini@ti.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141027/5e1ac415/attachment.pgp>