[PATCH v3 06/19] android: boot: move to andr_image_data structure
Safae Ouajih
souajih at baylibre.com
Thu Feb 9 17:49:24 CET 2023
On 09/02/2023 15:26, Mattijs Korpershoek wrote:
> On Mon, Feb 06, 2023 at 00:50, Safae Ouajih <souajih at baylibre.com> wrote:
>
>> Move from andr_boot_img_hdr_v0 to andr_image_data
>> structure to prepare for boot image header
>> version 3 and 4.
>>
>> Signed-off-by: Safae Ouajih <souajih at baylibre.com>
>> ---
>> boot/image-android.c | 121 +++++++++++++++++++++++--------------------
>> cmd/abootimg.c | 31 +++++------
>> 2 files changed, 81 insertions(+), 71 deletions(-)
>>
>> diff --git a/boot/image-android.c b/boot/image-android.c
>> index ea05c1814f..15a735e230 100644
>> --- a/boot/image-android.c
>> +++ b/boot/image-android.c
>> @@ -86,7 +86,7 @@ bool android_image_get_data(const void *boot_hdr, struct andr_image_data *data)
>> return true;
>> }
>>
>> -static ulong android_image_get_kernel_addr(const struct andr_boot_img_hdr_v0 *hdr)
>> +static ulong android_image_get_kernel_addr(struct andr_image_data *img_data)
>> {
>> /*
>> * All the Android tools that generate a boot.img use this
>> @@ -99,17 +99,17 @@ static ulong android_image_get_kernel_addr(const struct andr_boot_img_hdr_v0 *hd
>> *
>> * Otherwise, we will return the actual value set by the user.
>> */
>> - if (hdr->kernel_addr == ANDROID_IMAGE_DEFAULT_KERNEL_ADDR)
>> - return (ulong)hdr + hdr->page_size;
>> + if (img_data->kernel_addr == ANDROID_IMAGE_DEFAULT_KERNEL_ADDR)
>> + return img_data->kernel_ptr;
>>
>> /*
>> * abootimg creates images where all load addresses are 0
>> * and we need to fix them.
>> */
>> - if (hdr->kernel_addr == 0 && hdr->ramdisk_addr == 0)
>> + if (img_data->kernel_addr == 0 && img_data->ramdisk_addr == 0)
>> return env_get_ulong("kernel_addr_r", 16, 0);
>>
>> - return hdr->kernel_addr;
>> + return img_data->kernel_addr;
>> }
>>
>> /**
>> @@ -130,27 +130,33 @@ static ulong android_image_get_kernel_addr(const struct andr_boot_img_hdr_v0 *hd
>> int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, int verify,
>> ulong *os_data, ulong *os_len)
>> {
>> - u32 kernel_addr = android_image_get_kernel_addr(hdr);
>> - const struct legacy_img_hdr *ihdr = (const struct legacy_img_hdr *)
>> - ((uintptr_t)hdr + hdr->page_size);
>> + struct andr_image_data img_data = {0};
>> + u32 kernel_addr;
>> + const struct legacy_img_hdr *ihdr;
>> +
>> + if (!android_image_get_data(hdr, &img_data))
>> + return -EINVAL;
>> +
>> + kernel_addr = android_image_get_kernel_addr(&img_data);
>> + ihdr = (const struct legacy_img_hdr *)img_data.kernel_ptr;
>>
>> /*
>> * Not all Android tools use the id field for signing the image with
>> * sha1 (or anything) so we don't check it. It is not obvious that the
>> * string is null terminated so we take care of this.
>> */
>> - strncpy(andr_tmp_str, hdr->name, ANDR_BOOT_NAME_SIZE);
>> + strlcpy(andr_tmp_str, img_data.image_name, ANDR_BOOT_NAME_SIZE);
> While strlcpy is probably better than strncpy here, it has nothing to do
> with this change. Either mention in the commit message that this is
> intentional or drop this change.
>
> Note that strncpy was used in v2 of this patch. Why change it to strlcpy here?
Hello Mattijs,
Thank you for the review,
This is done to fix checkpatch.pl warning : WARNING: strlcpy is
preferred over strncpy
>
>> andr_tmp_str[ANDR_BOOT_NAME_SIZE] = '\0';
>> if (strlen(andr_tmp_str))
>> printf("Android's image name: %s\n", andr_tmp_str);
>>
>> printf("Kernel load addr 0x%08x size %u KiB\n",
>> - kernel_addr, DIV_ROUND_UP(hdr->kernel_size, 1024));
>> + kernel_addr, DIV_ROUND_UP(img_data.kernel_size, 1024));
>>
>> int len = 0;
>> - if (*hdr->cmdline) {
>> - printf("Kernel command line: %s\n", hdr->cmdline);
>> - len += strlen(hdr->cmdline);
>> + if (*img_data.kcmdline) {
>> + printf("Kernel command line: %s\n", img_data.kcmdline);
>> + len += strlen(img_data.kcmdline);
>> }
>>
>> char *bootargs = env_get("bootargs");
>> @@ -168,8 +174,9 @@ int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, int verify,
>> strcpy(newbootargs, bootargs);
>> strcat(newbootargs, " ");
>> }
>> - if (*hdr->cmdline)
>> - strcat(newbootargs, hdr->cmdline);
>> +
>> + if (*img_data.kcmdline)
>> + strcat(newbootargs, img_data.kcmdline);
>>
>> env_set("bootargs", newbootargs);
>>
>> @@ -177,15 +184,14 @@ int android_image_get_kernel(const struct andr_boot_img_hdr_v0 *hdr, int verify,
>> if (image_get_magic(ihdr) == IH_MAGIC) {
>> *os_data = image_get_data(ihdr);
>> } else {
>> - *os_data = (ulong)hdr;
>> - *os_data += hdr->page_size;
>> + *os_data = img_data.kernel_ptr;
>> }
>> }
>> if (os_len) {
>> if (image_get_magic(ihdr) == IH_MAGIC)
>> *os_len = image_get_data_size(ihdr);
>> else
>> - *os_len = hdr->kernel_size;
>> + *os_len = img_data.kernel_size;
>> }
>> return 0;
>> }
>> @@ -197,30 +203,25 @@ bool is_android_boot_image_header(const struct andr_boot_img_hdr_v0 *hdr)
>>
>> ulong android_image_get_end(const struct andr_boot_img_hdr_v0 *hdr)
>> {
>> - ulong end;
>> -
>> - /*
>> - * The header takes a full page, the remaining components are aligned
>> - * on page boundary
>> - */
>> - 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);
>> + struct andr_image_data img_data;
>>
>> - if (hdr->header_version >= 1)
>> - end += ALIGN(hdr->recovery_dtbo_size, hdr->page_size);
>> + if (!android_image_get_data(hdr, &img_data))
>> + return -EINVAL;
>>
>> - if (hdr->header_version >= 2)
>> - end += ALIGN(hdr->dtb_size, hdr->page_size);
>> + if (img_data.header_version > 2)
>> + return 0;
>>
>> - return end;
>> + return img_data.boot_img_total_size;
>> }
>>
>> ulong android_image_get_kload(const struct andr_boot_img_hdr_v0 *hdr)
>> {
>> - return android_image_get_kernel_addr(hdr);
>> + struct andr_image_data img_data;
>> +
>> + if (!android_image_get_data(hdr, &img_data))
>> + return -EINVAL;
>> +
>> + return android_image_get_kernel_addr(&img_data);
>> }
>>
>> ulong android_image_get_kcomp(const struct andr_boot_img_hdr_v0 *hdr)
>> @@ -243,38 +244,43 @@ ulong android_image_get_kcomp(const struct andr_boot_img_hdr_v0 *hdr)
>> int android_image_get_ramdisk(const struct andr_boot_img_hdr_v0 *hdr,
>> ulong *rd_data, ulong *rd_len)
>> {
>> - if (!hdr->ramdisk_size) {
>> + struct andr_image_data img_data = {0};
>> +
>> + if (!android_image_get_data(hdr, &img_data))
>> + return -EINVAL;
>> +
>> + if (!img_data.ramdisk_size) {
>> *rd_data = *rd_len = 0;
>> return -1;
>> }
>>
>> - printf("RAM disk load addr 0x%08x size %u KiB\n",
>> - hdr->ramdisk_addr, DIV_ROUND_UP(hdr->ramdisk_size, 1024));
>> + printf("RAM disk load addr 0x%08lx size %u KiB\n",
>> + img_data.ramdisk_ptr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
>>
>> - *rd_data = (unsigned long)hdr;
>> - *rd_data += hdr->page_size;
>> - *rd_data += ALIGN(hdr->kernel_size, hdr->page_size);
>> + *rd_data = img_data.ramdisk_ptr;
>>
>> - *rd_len = hdr->ramdisk_size;
>> + *rd_len = img_data.ramdisk_size;
>> return 0;
>> }
>>
>> int android_image_get_second(const struct andr_boot_img_hdr_v0 *hdr,
>> ulong *second_data, ulong *second_len)
>> {
>> - if (!hdr->second_size) {
>> + struct andr_image_data img_data;
>> +
>> + if (!android_image_get_data(hdr, &img_data))
>> + return -EINVAL;
>> +
>> + if (!img_data.second_size) {
>> *second_data = *second_len = 0;
>> return -1;
>> }
>>
>> - *second_data = (unsigned long)hdr;
>> - *second_data += hdr->page_size;
>> - *second_data += ALIGN(hdr->kernel_size, hdr->page_size);
>> - *second_data += ALIGN(hdr->ramdisk_size, hdr->page_size);
>> + *second_data = img_data.second_ptr;
>>
>> printf("second address is 0x%lx\n",*second_data);
>>
>> - *second_len = hdr->second_size;
>> + *second_len = img_data.second_size;
>> return 0;
>> }
>>
>> @@ -401,17 +407,22 @@ exit:
>> bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr,
>> u32 *size)
>> {
>> + struct andr_image_data img_data;
>> const struct andr_boot_img_hdr_v0 *hdr;
>> - bool res;
>> +
>> + hdr = map_sysmem(hdr_addr, sizeof(*hdr));
>> + if (!android_image_get_data(hdr, &img_data)) {
>> + unmap_sysmem(hdr);
>> + return false;
>> + }
>> + unmap_sysmem(hdr);
>> +
>> ulong dtb_img_addr; /* address of DTB part in boot image */
>> u32 dtb_img_size; /* size of DTB payload in boot image */
>> ulong dtb_addr; /* address of DTB blob with specified index */
>> u32 i; /* index iterator */
>>
>> - res = android_image_get_dtb_img_addr(hdr_addr, &dtb_img_addr);
>> - if (!res)
>> - return false;
>> -
>> + android_image_get_dtb_img_addr(hdr_addr, &dtb_img_addr);
> Why drop the error handling here?
> Shouldn't we bail out early if this fails?
Will be added in a v4 as well as all your remarks
BRs,
Safae
>> /* Check if DTB area of boot image is in DTBO format */
>> if (android_dt_check_header(dtb_img_addr)) {
>> return android_dt_get_fdt_by_index(dtb_img_addr, index, addr,
>> @@ -419,9 +430,7 @@ bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr,
>> }
>>
>> /* Find out the address of DTB with specified index in concat blobs */
>> - hdr = map_sysmem(hdr_addr, sizeof(*hdr));
>> - dtb_img_size = hdr->dtb_size;
>> - unmap_sysmem(hdr);
>> + dtb_img_size = img_data.dtb_size;
>> i = 0;
>> dtb_addr = dtb_img_addr;
>> while (dtb_addr < dtb_img_addr + dtb_img_size) {
>> diff --git a/cmd/abootimg.c b/cmd/abootimg.c
>> index b5cfb141ef..f04a7c7c8e 100644
>> --- a/cmd/abootimg.c
>> +++ b/cmd/abootimg.c
>> @@ -66,33 +66,34 @@ static int abootimg_get_recovery_dtbo(int argc, char *const argv[])
>>
>> static int abootimg_get_dtb_load_addr(int argc, char *const argv[])
>> {
>> - const struct andr_boot_img_hdr_v0 *hdr;
>> - int res = CMD_RET_SUCCESS;
>> -
>> if (argc > 1)
>> return CMD_RET_USAGE;
>> + struct andr_image_data img_data = {0};
>> + const struct andr_boot_img_hdr_v0 *hdr;
> Don't move this around. keep the variable declarations at the beginning
> of the function as done in the other functions.
>
>>
>> hdr = map_sysmem(abootimg_addr(), sizeof(*hdr));
>> - if (!is_android_boot_image_header(hdr)) {
>> - printf("Error: Boot Image header is incorrect\n");
>> - res = CMD_RET_FAILURE;
>> - goto exit;
>> + if (!android_image_get_data(hdr, &img_data)) {
>> + unmap_sysmem(hdr);
>> + return CMD_RET_FAILURE;
>> }
>> + unmap_sysmem(hdr);
>>
>> - if (hdr->header_version < 2) {
>> + if (img_data.header_version < 2) {
>> printf("Error: header_version must be >= 2 for this\n");
>> - res = CMD_RET_FAILURE;
>> - goto exit;
>> + return CMD_RET_FAILURE;
>> + }
>> +
>> + if (!img_data.dtb_load_addr) {
>> + printf("Error: failed to read dtb_load_addr\n");
>> + return CMD_RET_FAILURE;
>> }
>>
>> if (argc == 0)
>> - printf("%lx\n", (ulong)hdr->dtb_addr);
>> + printf("%lx\n", (ulong)img_data.dtb_load_addr);
>> else
>> - env_set_hex(argv[0], (ulong)hdr->dtb_addr);
>> + env_set_hex(argv[0], (ulong)img_data.dtb_load_addr);
>>
>> -exit:
>> - unmap_sysmem(hdr);
>> - return res;
>> + return CMD_RET_SUCCESS;
>> }
>>
>> static int abootimg_get_dtb_by_index(int argc, char *const argv[])
>> --
>> 2.34.1
More information about the U-Boot
mailing list