Re: [PATCH 1/6] rockchip: mkimage: Split size_and_off and size_and_nimage

[Jonas Karlman <jonas@kwiboo.se>]

Hi Quentin,

On 2025-02-05 16:40, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 1/29/25 11:36 PM, Jonas Karlman wrote:
>> Split 32-bit size_and_off and size_and_nimage fields of the v2 image
>> format header into their own 16-bit size, offset and num_images fields.
>>
>> Set num_images based on number of images passed by the datafile
>> parameter and size based on the offset to the hash field to fix using a
>> single init data file and no boot data file for the v2 image format.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>>   tools/rkcommon.c | 44 ++++++++++++++++++++++++--------------------
>>   1 file changed, 24 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/rkcommon.c b/tools/rkcommon.c
>> index 3e52236b15a8..de3fd2d3f3c2 100644
>> --- a/tools/rkcommon.c
>> +++ b/tools/rkcommon.c
>> @@ -34,15 +34,16 @@ enum hash_type {
>>   /**
>>    * struct image_entry
>>    *
>> - * @size_and_off:	[31:16]image size;[15:0]image offset
>> - * @address:	default as 0xFFFFFFFF
>> + * @offset:	image offset (unit as 512 byte blocks)
>> + * @size:	image size (unit as 512 byte blocks)
>> + * @address:	load address (default as 0xFFFFFFFF)
>>    * @flag:	no use
>>    * @counter:	no use
>>    * @hash:	hash of image
>> - *
>>    */
>>   struct image_entry {
>> -	uint32_t size_and_off;
>> +	uint16_t offset;
>> +	uint16_t size;
>>   	uint32_t address;
>>   	uint32_t flag;
>>   	uint32_t counter;
>> @@ -56,16 +57,17 @@ struct image_entry {
>>    * This is stored at SD card block 64 (where each block is 512 bytes)
>>    *
>>    * @magic:	Magic (must be RK_MAGIC_V2)
>> - * @size_and_nimage:	[31:16]number of images;[15:0]
>> - *			offset to hash field of header(unit as 4Byte)
>> - * @boot_flag:	[3:0]hash type(0:none,1:sha256,2:sha512)
>> - * @signature:	hash or signature for header info
>> - *
>> + * @size:	offset to hash field of header (unit as 4 bytes)
>> + * @num_images:	number of images
>> + * @boot_flag:	[3:0] hash type (0:none, 1:sha256, 2:sha512)
>> + * @images:	images
>> + * @hash:	hash or signature for header info
>>    */
>>   struct header0_info_v2 {
>>   	uint32_t magic;
>>   	uint8_t reserved[4];
>> -	uint32_t size_and_nimage;
>> +	uint16_t size;
>> +	uint16_t num_images;
>>   	uint32_t boot_flag;
>>   	uint8_t reserved1[104];
>>   	struct image_entry images[4];
>> @@ -332,17 +334,18 @@ static void rkcommon_set_header0_v2(void *buf, struct image_tool_params *params)
>>   	printf("Image Type:   Rockchip %s boot image\n",
>>   		rkcommon_get_spl_hdr(params));
>>   	memset(buf, '\0', RK_INIT_OFFSET * RK_BLK_SIZE);
>> -	hdr->magic   = cpu_to_le32(RK_MAGIC_V2);
>> -	hdr->size_and_nimage = cpu_to_le32((2 << 16) + 384);
>> +	hdr->magic = cpu_to_le32(RK_MAGIC_V2);
>>   	hdr->boot_flag = cpu_to_le32(HASH_SHA256);
>>   	sector_offset = 4;
>>   	image_size_array[0] = spl_params.init_size;
>>   	image_size_array[1] = spl_params.boot_size;
>>   
>>   	for (i = 0; i < 2; i++) {
>> +		if (!image_size_array[i])
>> +			break;
> 
> This isn't related to this change I believe, can you please make it its 
> own commit so it doesn't get lost in the diff and has its own individual 
> commit log?

Yeah, I mostly wanted/needed a test to stop the loop on the "last"
supplied input file/image in a later patch. Adding it here made the
later diff little bit smaller :-)

> 
>>   		image_sector_count = image_size_array[i] / RK_BLK_SIZE;
>> -		hdr->images[i].size_and_off = cpu_to_le32((image_sector_count
>> -							<< 16) + sector_offset);
>> +		hdr->images[i].offset = cpu_to_le16(sector_offset);
>> +		hdr->images[i].size = cpu_to_le16(image_sector_count);
>>   		hdr->images[i].address = 0xFFFFFFFF;
>>   		hdr->images[i].counter = cpu_to_le32(i + 1);
>>   		image_ptr = buf + sector_offset * RK_BLK_SIZE;
>> @@ -351,6 +354,8 @@ static void rkcommon_set_header0_v2(void *buf, struct image_tool_params *params)
>>   		sector_offset = sector_offset + image_sector_count;
>>   	}
>>   
>> +	hdr->num_images = cpu_to_le16(i);
>> +	hdr->size = cpu_to_le16(offsetof(typeof(*hdr), hash) / sizeof(uint32_t));
> 
> Same here. Just do a migration commit (possibly one for struct 
> image_entry and another one for struct header0_info_v2) first and then 
> adapt so it handles image_size_array[1] = 0. We don't today so a 
> separate patch explaining the usecase would be nice.

I was trying to avoid having to create too many commits, and mostly
cared about the end result, guess I can split this even further in a v2.

All in-tree use from binman will always supply two input files/images
for v2 image format, so has not really been an issue or need to handle
image_size_array[1] = 0, not sure what would happen if you try to run
mkimage manually without the second boot image.

> 
>>   	do_sha256_hash(buf, (void *)hdr->hash - buf, hdr->hash);
>>   }
>>   
>> @@ -497,10 +502,8 @@ void rkcommon_print_header(const void *buf, struct image_tool_params *params)
>>   			return;
>>   		}
>>   
>> -		init_size = header0_v2.images[0].size_and_off >> 16;
>> -		init_size = init_size * RK_BLK_SIZE;
>> -		boot_size = header0_v2.images[1].size_and_off >> 16;
>> -		boot_size = boot_size * RK_BLK_SIZE;
>> +		init_size = le16_to_cpu(header0_v2.images[0].size) * RK_BLK_SIZE;
>> +		boot_size = le16_to_cpu(header0_v2.images[1].size) * RK_BLK_SIZE;
> 
> Ditto. Separate patch for the le16_to_cpu would be nice as I assume this 
> is not a side-effect of switching to two u16 instead of one u32. This 
> likely fixes a bug :)

I have no idea if any of this was or is really working on big-endian
prior to (or after), there was some bit manipulation and change to
use leXX_to_cpu seemed appropriate, will try to split it out in v2.
Could possible try to use qemu for some big-endian testing.

> 
> I was wondering if we shouldn't have CI to generate a handful of 
> Rockchip dummy binaries with the header on different endianness so we 
> can catch those. I remember we had someone fix those for v1 already.

Sound like a nice task for someone else :-)

> 
>>   	} else {
>>   		ret = rkcommon_parse_header(buf, &header0, &spl_info);
>>   
>> @@ -514,8 +517,9 @@ void rkcommon_print_header(const void *buf, struct image_tool_params *params)
>>   		}
>>   
>>   		image_type = ret;
>> -		init_size = header0.init_size * RK_BLK_SIZE;
>> -		boot_size = header0.init_boot_size * RK_BLK_SIZE - init_size;
>> +		init_size = le16_to_cpu(header0.init_size) * RK_BLK_SIZE;
>> +		boot_size = le16_to_cpu(header0.init_boot_size) * RK_BLK_SIZE -
>> +			    init_size;
>>   
> 
> Ditto, separate patch for le16_to_cpu.

Sure, will split out to a few more patches for v2.

Regards,
Jonas

> 
> Looks good otherwise!
> 
> Cheers,
> Quentin