image: Fix Android boot image support

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()

Hi,

Quoted 6 lines · 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()

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

Hello Mr. Glass

Quoted 14 lines · Simon Glass

Hi,

Quoted 6 lines · 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()

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.

Quoted 2 lines

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?

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 <[email protected]>

Cc Tom Rini <[email protected]>

---
 common/bootm.c         |  4 ++--
 common/image-android.c | 34 +++++++++++++++++++++++++++-------
 common/image.c         |  3 ++-
 3 files changed, 31 insertions(+), 10 deletions(-)

common/bootm.c +2 -2

index ff81a27..c04a3b0 100644
@@ -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:

common/image-android.c +6 -1

index 6ded7e2..badaa7e 100644
@@ -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 {

common/image-android.c +11 -6

+
+                       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)

common/image.c +0 -0

index 085771c..e21c848 100644

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

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 <[email protected]>

Cc Tom Rini <[email protected]>

---
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(-)

common/bootm.c +2 -2

index ff81a27..c04a3b0 100644
@@ -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:

common/image-android.c +6 -5

index 6ded7e2..673cbca 100644
@@ -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);

common/image-android.c +18 -6

        }
+       *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)

common/image.c +2 -2

index 085771c..e21c848 100644
@@ -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

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 <[email protected]>

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

Regards,
Simon

Hello, Mr. Glass.

Quoted 3 lines · Simon Glass

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.

Quoted 4 lines

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?

Quoted 6 lines

Reviewed-by Simon Glass <[email protected]>

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.

Quoted 2 lines

Regards,
Simon

Regards,
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 <[email protected]>

Cc Tom Rini <[email protected]>

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 <[email protected]>

Cc Tom Rini <[email protected]>

Reviewed-by Simon Glass <[email protected]>