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