[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