[PATCH v2 2/2] mkimage: Add support for bundling TEE in mkimage -f auto

[Marek Vasut]

On 11/25/25 12:37 PM, Quentin Schulz wrote:

Hello Quentin,

>> +++ b/doc/mkimage.1
>> @@ -251,6 +251,18 @@ Append TFA BL31 file to the image.
>>   .B \-\-tfa-bl31-addr
>>   Set TFA BL31 file load and entry point address.
>>   .
>> +.TP
>> +.B \-z
>> +.TQ
>> +.B \-\-tee-file
>> +Append TEE file to the image.
> 
> Please specify which format is supported here if only select ones are.
> 
>> +.
>> +.TP
>> +.B \-Z
>> +.TQ
>> +.B \-\-tee-addr
>> +Set TEE file load and entry point address, in hexadecimal.
> 
> s/address/addresses/ ?

This is a single address here, so no plural.

> [...]
>> @@ -239,3 +251,42 @@ def test_fit_auto_signed(ubman):
>>       generate_and_check_fit_image(' -fauto-conf' + b_args + s_args + 
>> " " + fit_file,
>>                                    scfgs=True, bl31present=True,
>>                                    key_name=key_name, 
>> sign_algo=sign_algo)
>> +
>> +    # Run the same tests as 1/2/3 above, but this time with TEE
>> +    # options -z tee.bin -Z 0x56780000 to cover both mkimage with
>> +    # and without TEE use cases.
>> +    b_args = " -d" + kernel_file + " -b" + dt1_file + " -b" + 
>> dt2_file + " -z" + tee_file + " -Z 0x56780000"
>> +
> 
> General remark but using f-string may improve readability:
> 
> b_args = f'-d {kernel_file} -b {dt1_file} -b {dt2_file} -z {tee_file} -Z 
> 0x56780000'
> 
> [...]
> 
>> @@ -473,10 +504,20 @@ static void fit_write_configs(struct 
>> image_tool_params *params, char *fdt)
>>           len = strlen(str);
>>           fdt_property_string(fdt, typename, str);
>> -        if (params->fit_tfa_bl31) {
>> +        if (params->fit_tfa_bl31 && params->fit_tee) {
>> +            snprintf(str, sizeof(str), "%s-1." FIT_TFA_BL31_PROP 
>> "-1." FIT_TEE_PROP "-1", typename);
>> +            str[len] = 0;
>> +            len += strlen(FIT_TFA_BL31_PROP "-1") + 1;
>> +            str[len] = 0;
>> +            len += strlen(FIT_TEE_PROP "-1") + 1;
>> +        } else if (params->fit_tfa_bl31) {
>>               snprintf(str, sizeof(str), "%s-1." FIT_TFA_BL31_PROP 
>> "-1", typename);
>>               str[len] = 0;
>>               len += strlen(FIT_TFA_BL31_PROP "-1") + 1;
>> +        } else if (params->fit_tee) {
>> +            snprintf(str, sizeof(str), "%s-1." FIT_TEE_PROP "-1", 
>> typename);
>> +            str[len] = 0;
>> +            len += strlen(FIT_TEE_PROP "-1") + 1;
> 
> It actually took me a while to figure out that we are reusing len from 
> outside of the if block because the loadables property starts with 
> kernel-1.
> 
> I'm wondering if it would make it more readable to do the following
> 
> if (params->fit_tfa_bl31) {
>      snprintf(&str[len + 1], sizeof(str) - (len + 1), FIT_TFA_BL31_PROP 
> "-1");
>      len += strlen(&str[len + 1]) + 1;
> }
> 
> if (params->fit_tee) {
>      snprintf(&str[len + 1], sizeof(str) - (len + 1), FIT_TEE_PROP "-1");
>      len += strlen(&str[len + 1]) + 1;
> }
> 
> (NOT TESTED!)

Works. and this is much better than the current code, thanks !