On 11/14/2016 02:44 PM, Simon Glass wrote:
> Hi Andrew,
> 
> On 14 November 2016 at 12:49, Andrew F. Davis <a...@ti.com> wrote:
>> To help automate the loading of a TEE image during the boot we add a new
>> FIT section type 'tee', when we see this type while loading the loadable
>> sections we automatically call the platforms TEE processing function on
>> this image section.
>>
>> Signed-off-by: Andrew F. Davis <a...@ti.com>
>> ---
>>  Kconfig         | 10 ++++++++++
>>  common/image.c  | 18 ++++++++++++++++++
>>  include/image.h | 15 +++++++++++++++
>>  3 files changed, 43 insertions(+)
>>
>> diff --git a/Kconfig b/Kconfig
>> index 1263d0b..97cf7c8 100644
>> --- a/Kconfig
>> +++ b/Kconfig
>> @@ -291,6 +291,16 @@ config FIT_IMAGE_POST_PROCESS
>>           injected into the FIT creation (i.e. the blobs would have been pre-
>>           processed before being added to the FIT image).
>>
>> +config FIT_IMAGE_TEE_PROCESS
>> +       bool "Enable processing of TEE images during FIT loading by U-Boot"
>> +       depends on FIT && TI_SECURE_DEVICE
> 
> This is a generic option so I don't think it should depend on TI.
> 

This was based on FIT_IMAGE_POST_PROCESS which is also generic but
depends on TI as we currently are the only users.

I think it should be removed from both, so I'll remove it here at least.

>> +       help
>> +         Allows platforms to perform processing, such as authentication and
>> +         installation, on TEE images extracted from FIT images in a platform
>> +         or board specific way. In order to use this feature a platform or
>> +         board-specific implementation of board_tee_image_process() must be
>> +         provided.
>> +
>>  config SPL_DFU_SUPPORT
>>         bool "Enable SPL with DFU to load binaries to memory device"
>>         depends on USB
>> diff --git a/common/image.c b/common/image.c
>> index 7604494..4552ca5 100644
>> --- a/common/image.c
>> +++ b/common/image.c
>> @@ -165,6 +165,7 @@ static const table_entry_t uimage_type[] = {
>>         {       IH_TYPE_ZYNQIMAGE,  "zynqimage",  "Xilinx Zynq Boot Image" },
>>         {       IH_TYPE_ZYNQMPIMAGE, "zynqmpimage", "Xilinx ZynqMP Boot 
>> Image" },
>>         {       IH_TYPE_FPGA,       "fpga",       "FPGA Image" },
>> +       {       IH_TYPE_TEE,        "tee",        "TEE OS Image",},
> 
> Perhaps write out TEE in full? It's a bit cryptic.
> 

Will do.

>>         {       -1,                 "",           "",                   },
>>  };
>>
>> @@ -1408,6 +1409,8 @@ int boot_get_loadable(int argc, char * const argv[], 
>> bootm_headers_t *images,
>>         int fit_img_result;
>>         const char *uname;
>>
>> +       uint8_t img_type;
>> +
>>         /* Check to see if the images struct has a FIT configuration */
>>         if (!genimg_has_config(images)) {
>>                 debug("## FIT configuration was not specified\n");
>> @@ -1447,6 +1450,21 @@ int boot_get_loadable(int argc, char * const argv[], 
>> bootm_headers_t *images,
>>                                 /* Something went wrong! */
>>                                 return fit_img_result;
>>                         }
>> +
>> +                       fit_img_result = fit_image_get_node(buf, uname);
>> +                       if (fit_img_result < 0) {
>> +                               /* Something went wrong! */
>> +                               return fit_img_result;
>> +                       }
>> +                       fit_img_result = fit_image_get_type(buf, 
>> fit_img_result, &img_type);
>> +                       if (fit_img_result < 0) {
>> +                               /* Something went wrong! */
>> +                               return fit_img_result;
>> +                       }
>> +#if defined(CONFIG_FIT_IMAGE_TEE_PROCESS)
>> +                       if (img_type == IH_TYPE_TEE)
>> +                               board_tee_image_process(img_data, img_len);
>> +#endif
> 
> Instead of putting this here, I think it would be better for
> boot_get_loadable() to return the correct values for ld_start and
> ld_len. Perhaps you need to pass it the loadable index to load, so it
> is called multiple times? The only caller is bootm_find_images().
> 

Something like how boot_get_fpga() does it? This seems like a lot of
code duplication between boot_get_fpga() and boot_get_loadable(), and
both ignore ld_start and ld_len.

Does it not make more sense to have a single function for loadable
components like we have now? The loadables themselves have a type we can
switch on and then call a platform specific hook for that loadable type.

I don't want a big TI specific function in common/image.c, but if this
is okay I'll move it out of platform and into here.

> It is too ugly, I think, to check the image type in the 'load'
> function, and do special things.
> 

The alternative is a boot_get_<type> function for each type. All called
from bootm_find_images().

>>                 }
>>                 break;
>>         default:
>> diff --git a/include/image.h b/include/image.h
>> index 2b1296c..57084c8 100644
>> --- a/include/image.h
>> +++ b/include/image.h
>> @@ -279,6 +279,7 @@ enum {
>>         IH_TYPE_ZYNQMPIMAGE,            /* Xilinx ZynqMP Boot Image */
>>         IH_TYPE_FPGA,                   /* FPGA Image */
>>         IH_TYPE_VYBRIDIMAGE,    /* VYBRID .vyb Image */
>> +       IH_TYPE_TEE,            /* Trusted Execution Environment OS Image */
>>
>>         IH_TYPE_COUNT,                  /* Number of image types */
>>  };
>> @@ -1263,4 +1264,18 @@ int board_fit_config_name_match(const char *name);
>>  void board_fit_image_post_process(void **p_image, size_t *p_size);
>>  #endif /* CONFIG_SPL_FIT_IMAGE_POST_PROCESS */
>>
>> +#ifdef CONFIG_FIT_IMAGE_TEE_PROCESS
> 
> I don't think you should have this #ifdef in the header file.
> 

Then board_tee_image_process would always have a deceleration, even on
boards without a definition of it.

>> +/**
>> + * board_fit_tee_process() - Do any needed processing on a loaded TEE image
>> + *
>> + * This is used to verify, decrypt, and/or install a TEE in a platform or
>> + * board specific way.
> 
> nit: board-specific
> 
> 
>> + *
>> + * @tee_image: pointer to the image
> 
> What format is the image?
> 

Platform specific. Different TEEs have different header types and our
platforms require different signing headers.

>> + * @tee_size: the image size
>> + * @return no return value (failure should be handled internally)
>> + */
>> +void board_tee_image_process(void *tee_image, size_t tee_size);
> 
> I think it's a good idea to return an error code here, since the
> function may fail.
> 
>> +#endif /* CONFIG_FIT_IMAGE_TEE_PROCESS */
>> +
>>  #endif /* __IMAGE_H__ */
>> --
>> 2.10.1
>>
> 
> Regards,
> SImon
> 
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to