| Message ID | 20250129223641.1888833-2-jonas@kwiboo.se |
|---|---|
| State | Changes Requested |
| Delegated to: | Quentin Schulz |
| Headers | show |
| Series | rockchip: mkimage: Improve support for v2 image format | expand |
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? > 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. > 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 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. > } 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. Looks good otherwise! Cheers, Quentin
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
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; 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)); 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; } 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; printf("Image Type: Rockchip %s (%s) boot image\n", spl_info->spl_hdr,
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(-)