[U-Boot] [PATCH 1/2] image: add support for Android's boot image format

[Sebastian Andrzej Siewior]

* Wolfgang Denk | 2011-11-21 21:19:07 [+0100]:

>Dear Sebastian Andrzej Siewior,
Hi Wolfgang,

>Please provide _exact_ reference where this code has been copied from,
>see bullet 4 etc. at
>http://www.denx.de/wiki/view/U-Boot/Patches#Attributing_Code_Copyrights_Sign
>
>Also please provide exact information about the applicable licenses
>for this copied code.

I added a detailed description to the repository and the commit head
where I took it from.

>...
>>  	/* get image parameters */
>> -	switch (genimg_get_format(os_hdr)) {
>> +	img_type = genimg_get_format(os_hdr);
>> +	switch (img_type) {
>...
>> +#ifdef CONFIG_ANDROID_BOOT_IMAGE
>> +	} else if (img_type == IMAGE_FORMAT_ANDROID) {
>> +		images.ep = images.os.load;
>> +#endif
>
>Why don't you handle the Andoid image case inside the switch() ?

Because the code flow looks like:
|         if (images.legacy_hdr_valid) {
|                 images.ep = image_get_ep(&images.legacy_hdr_os_copy);
| #if defined(CONFIG_FIT)
|         } else if (images.fit_uname_os) {
...
|                 }
| #endif
| #ifdef CONFIG_ANDROID_BOOT_IMAGE
...
| #endif
|         } else {
|                 puts("Could not find kernel entry point!\n");
|                 return 1;
|         }
|

So if I don't add an extra android block here, I end up in this else
part complaining about the entrypoint.
I guess that this part could be merged into the previous switch
statement if you want me to.

>> +#ifdef CONFIG_ANDROID_BOOT_IMAGE
>> +static char andr_tmp_str[ANDR_BOOT_ARGS_SIZE + 1];
>> +static int android_image_get_kernel(struct andr_img_hdr *hdr, int verify)
>> +{
>> +	/*
>> +	 * 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);
>> +	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",
>> +			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);
>> +	}
>> +	if (hdr->ramdisk_size)
>> +		printf("RAM disk load addr 0x%08x size %u KiB\n",
>> +				hdr->ramdisk_addr,
>> +				DIV_ROUND_UP(hdr->ramdisk_size, 1024));
>> +	return 0;
>> +}
>> +#endif
>
>This and similar Android image related code shoudl eventually go into
>a separate file, exposing only a few functions.
Okay.

>> +#ifdef CONFIG_ANDROID_BOOT_IMAGE
>> +	if (format == IMAGE_FORMAT_INVALID) {
>> +		const struct andr_img_hdr *ahdr = img_addr;
>>  
>> +		if (!memcmp(ANDR_BOOT_MAGIC, ahdr->magic, ANDR_BOOT_MAGIC_SIZE))
>> +			format = IMAGE_FORMAT_ANDROID;
>> +	}
>> +#endif
>
>This is all we have for testing for a valid image?

More or less, yes. The fastboot client [0] does nothing else if you
provide it a kernel (and it creates the ANDROID image out of it).
The comment field is used by mkbootimg/mkbootimg.c tool to stash a sha1
checksum. The format how to create the sha1 checksum (i.e. pad, don't
pad, include header or not, ...) isn't documented (atleast I did not
find anything) so the only source of documentation is
the source code wich is under Apache2 license which is compatible with
GPLv3 but not with v2 so I can't look at it.

[0] http://android-dls.com/wiki/index.php?title=Fastboot

>> index 0000000..b231b66
>> --- /dev/null
>> +++ b/include/android_image.h
>> @@ -0,0 +1,89 @@
>> +/*
>> + * This is from the Android Project,
>> + * bootloader/legacy/include/boot/bootimg.h
>> + *
>> + * Copyright (C) 2008 The Android Open Source Project
>> + * All rights reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *  * Redistributions of source code must retain the above copyright
>> + *    notice, this list of conditions and the following disclaimer.
>> + *  * Redistributions in binary form must reproduce the above copyright
>> + *    notice, this list of conditions and the following disclaimer in
>> + *    the documentation and/or other materials provided with the
>> + *    distribution.
>
>Sorry, but this is not GPL compatible.

Ehm. Is this the All rights reserved issue? If so then I assumed that I
cleared up things in
  http://lists.denx.de/pipermail/u-boot/2011-September/101793.html

>
>Best regards,
>
>Wolfgang Denk

Sebastian