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