| Message ID | 1413521529-1697-1-git-send-email-ar2000jp@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
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
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
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
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
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
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 */
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(-)