Hi Junhui, The patch looks good to me, I only have a few minor comments.
On 23:51 Wed 09 Sep , Junhui Liu wrote: > The SpacemiT BootROM loads the FSBL from a boot image consisting of a > fixed 4 KiB header with two metadata headers and authentication > information, followed by a 32-byte-aligned SPL payload and a trailing > authentication area. K1 and K3 share the same layout, but use different > integrity and authentication schemes. K1 requires RSA-signed images, > while K3 uses CRC32 to verify images in non-secure boot mode. > > Add mkimage type "smtimage" for the K3 CRC32 path. Populate both > metadata headers, fill their CRC32 values and the payload CRC32, and > reserve the authentication areas required by the layout. > > The image header has no reliable K1/K3 identifier, so select the SoC > with -n. Omit dumpimage support since it cannot take -n to distinguish s/-n/-n option/ > between the variants. > > Only "-n k3" in non-secure boot mode is supported. Route SoC differences > through a variant table so K1 and RSA authentication support can be > added later. > > Tested-by: E Shattow <[email protected]> > Signed-off-by: Junhui Liu <[email protected]> > --- > boot/image.c | 1 + > include/image.h | 1 + > tools/Makefile | 1 + > tools/smtimage.c | 202 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 205 insertions(+) > > diff --git a/boot/image.c b/boot/image.c > index 185d52ba492f..3a31ef663710 100644 > --- a/boot/image.c > +++ b/boot/image.c > @@ -179,6 +179,7 @@ static const table_entry_t uimage_type[] = { > { IH_TYPE_TFA_BL31, "tfa-bl31", "TFA BL31 Image", }, > { IH_TYPE_STM32IMAGE_V2, "stm32imagev2", "STMicroelectronics > STM32 Image V2.0" }, > { IH_TYPE_AMLIMAGE, "amlimage", "Amlogic Boot Image" }, > + { IH_TYPE_SMTIMAGE, "smtimage", "SpacemiT Boot Image" }, I found most entry align with 4 space.. should do same as above line > { -1, "", "", }, > }; > > diff --git a/include/image.h b/include/image.h > index 6edcb1995bfe..53aad2dfde3f 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -235,6 +235,7 @@ enum image_type_t { > IH_TYPE_TFA_BL31, /* TFA BL31 image */ > IH_TYPE_STM32IMAGE_V2, /* STMicroelectronics STM32 Image V2.0 > */ > IH_TYPE_AMLIMAGE, /* Amlogic Boot Image */ > + IH_TYPE_SMTIMAGE, /* SpacemiT Boot Image */ > > IH_TYPE_COUNT, /* Number of image types */ > }; > diff --git a/tools/Makefile b/tools/Makefile > index 535a5d51c89e..fc0aa52455aa 100644 > --- a/tools/Makefile > +++ b/tools/Makefile > @@ -138,6 +138,7 @@ dumpimage-mkimage-objs := aisimage.o \ > pbl_crc32.o \ > renesas_spkgimage.o \ > sfspl.o \ > + smtimage.o \ > vybridimage.o \ > stm32image.o \ > $(ROCKCHIP_OBS) \ > diff --git a/tools/smtimage.c b/tools/smtimage.c > new file mode 100644 > index 000000000000..96e8b5d8d171 > --- /dev/null > +++ b/tools/smtimage.c > @@ -0,0 +1,202 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) 2026 Junhui Liu <[email protected]> > + * > + * The SpacemiT boot image consists of a fixed image header followed by an > + * aligned SPL payload and a trailing authentication area. The image header > + * contains two metadata headers and areas reserved for authentication data. > + * > + * The image layout is: > + * > + * 0x000 root key > + * 0x100 header0 > + * 0x120 key metadata > + * 0x300 OEM keys > + * 0xb00 signature0 > + * 0xfe0 header1 > + * 0x1000 SPL payload, with its end aligned to 32 bytes > + * payload end signature1 (0x100 bytes) > + */ > + > +#include <image.h> > +#include <u-boot/crc.h> > +#include "imagetool.h" > + > +#define SMT_MAGIC "AIHD" .. > +#define SMT_PAYLOAD_ALIGN 32 it occur to me, it's not necessary to introduce a local macro which only used once? while it's quite obvious what ALIGN() means > +#define SMT_AUTH_SIZE 0x100 > + .. > +#define SMT_FLAG_CRC32 (1U << 0) is this a local flag? how about defining as BIT(0)? which is more readable IMO > + > +/** > + * struct smtimage_header - metadata header within a SpacemiT boot image > header > + * > + * @magic: Magic (must be SMT_MAGIC) > + * @version: Image version used when anti-rollback is enabled by eFuse > + * @secure: Version slot used when anti-rollback is enabled by eFuse > + * @reserved: Reserved > + * @image_size: Header0 stores the image header size > + * Header1 stores the aligned payload size > + * @load_addr: Unused > + * @header_crc: Header0 and Header1 store the CRC32 of the preceding 24 > bytes on K3 > + * @image_crc: Header1 stores the CRC32 of the aligned payload on K3 > + */ > +struct smtimage_header { > + uint8_t magic[4]; > + uint8_t version; > + uint8_t secure; > + uint16_t reserved; > + uint64_t image_size; > + uint64_t load_addr; > + uint32_t header_crc; > + uint32_t image_crc; > +} __packed; > + > +/** > + * struct smtimage_image_header - fixed header area of a SpacemiT boot image > + * > + * @root_key: Root public key > + * @header0: Image layout and load information > + * @key_data: Key metadata > + * @oem_keys: OEM public-key area > + * @signature0: Signature of the image configuration > + * @reserved: Reserved > + * @header1: SPL payload information > + */ > +struct smtimage_image_header { > + uint8_t root_key[0x100]; > + struct smtimage_header header0; > + uint8_t key_data[0x1e0]; > + uint8_t oem_keys[0x800]; > + uint8_t signature0[SMT_AUTH_SIZE]; > + uint8_t reserved[0x3e0]; > + struct smtimage_header header1; > +} __packed; > + > +/** > + * struct smtimage_variant - SoC-specific image properties > + * > + * @name: SoC name passed to mkimage with -n > + * @flags: SoC-specific image flags > + */ > +struct smtimage_variant { > + const char *name; > + unsigned int flags; > +}; > + > +static const struct smtimage_variant smtimage_variants[] = { > + { > + .name = "k3", > + .flags = SMT_FLAG_CRC32, > + }, you may want to define a macro if adding too many more variants which will make code more compact.. #define VARIANT(name, flag) \ { \ .name = "k3", \ .flags = SMT_FLAG_CRC32, \ } but I'm fine with current version if you're relunctant to change > +}; > + > +static const struct smtimage_variant *smtimage_get_variant(const char *name) > +{ > + int i; > + > + if (!name || !*name) > + return NULL; > + > + for (i = 0; i < ARRAY_SIZE(smtimage_variants); i++) > + if (!strcmp(name, smtimage_variants[i].name)) > + return &smtimage_variants[i]; > + > + return NULL; > +} > + > +static void smtimage_init_header(struct smtimage_header *header, uint64_t > image_size, > + const struct smtimage_variant *variant) > +{ > + uint32_t crc; > + > + memcpy(header->magic, SMT_MAGIC, sizeof(header->magic)); > + > + header->image_size = cpu_to_le64(image_size); > + > + if (variant->flags & SMT_FLAG_CRC32) { > + crc = crc32(0, (uint8_t *)header, > + offsetof(struct smtimage_header, header_crc)); > + header->header_crc = cpu_to_le32(crc); > + } > +} > + > +static int smtimage_check_params(struct image_tool_params *params) > +{ > + if (!smtimage_get_variant(params->imagename)) { > + fprintf(stderr, "%s: SpacemiT SoC must be specified with -n\n", > + params->cmdname); > + return EXIT_FAILURE; > + } > + > + if (!params->dflag) > + return EXIT_FAILURE; > + > + return EXIT_SUCCESS; > +} > + > +static void smtimage_print_header(const void *buf, struct image_tool_params > *params) > +{ > + const struct smtimage_image_header *image_header = buf; > + const struct smtimage_header *header1 = &image_header->header1; > + uint64_t payload_size = le64_to_cpu(header1->image_size); > + > + printf("SpacemiT Boot Image (%s)\n", params->imagename); or make it slightly more readable printf("Spacemit Boot Image\n"); printf("SoC Variant: %s\n", params->imagename); > + printf("Total Size: %llu bytes\n", > + (unsigned long long)(sizeof(*image_header) + payload_size + > SMT_AUTH_SIZE)); > + printf("Payload Size: %llu bytes\n", (unsigned long long)payload_size); > +} > + > +static void smtimage_set_header(void *buf, struct stat *sbuf, int infd, > + struct image_tool_params *params) > +{ > + const struct smtimage_variant *variant = > smtimage_get_variant(params->imagename); > + struct smtimage_image_header *image_header = buf; > + struct smtimage_header *header0 = &image_header->header0; > + struct smtimage_header *header1 = &image_header->header1; > + size_t payload_size; > + uint32_t crc; > + > + payload_size = sbuf->st_size - sizeof(*image_header) - SMT_AUTH_SIZE; > + > + smtimage_init_header(header0, sizeof(*image_header), variant); > + smtimage_init_header(header1, payload_size, variant); > + > + if (variant->flags & SMT_FLAG_CRC32) { > + crc = crc32(0, (uint8_t *)(image_header + 1), payload_size); > + header1->image_crc = cpu_to_le32(crc); > + } > +} > + > +static int smtimage_check_image_type(uint8_t type) > +{ > + if (type == IH_TYPE_SMTIMAGE) > + return EXIT_SUCCESS; > + > + return EXIT_FAILURE; > +} > + > +static int smtimage_vrec_header(struct image_tool_params *params, > + struct image_type_params *tparams) > +{ > + size_t payload_size = params->file_size - tparams->header_size; > + > + tparams->hdr = calloc(tparams->header_size, 1); > + > + return ALIGN(payload_size, SMT_PAYLOAD_ALIGN) - payload_size + > SMT_AUTH_SIZE; > +} > + > +U_BOOT_IMAGE_TYPE( > + smtimage, > + "SpacemiT Boot Image support", > + sizeof(struct smtimage_image_header), > + NULL, > + smtimage_check_params, > + NULL, > + smtimage_print_header, > + smtimage_set_header, > + NULL, > + smtimage_check_image_type, > + NULL, > + smtimage_vrec_header > +); > > -- > 2.55.0 > -- Yixun Lan (dlan)
