Hi Andrew,
On 15 November 2016 at 10:07, Andrew F. Davis afd@ti.com wrote:
On 11/14/2016 06:34 PM, Simon Glass wrote:
Hi Andrew,
On 14 November 2016 at 14:55, Andrew F. Davis afd@ti.com wrote:
On 11/14/2016 02:44 PM, Simon Glass wrote:
Hi Andrew,
On 14 November 2016 at 12:49, Andrew F. Davis afd@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 afd@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_DEVICEThis 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.
helpAllows platforms to perform processing, such as authentication andinstallation, on TEE images extracted from FIT images in a platformor board specific way. In order to use this feature a platform orboard-specific implementation of board_tee_image_process() must beprovided.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.
Yes it is. In fact I think we could rationalise these to some extent. But it's not a big deal and could be done later.
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.
So with your patch we have two special processing things, right? One for FPGA and one for TEE.
My plan would be to have only the basic image types handled in common/image.c (ramdisk, dtb, loadables, etc..), then we have a switch of optional callbacks for different "loadable" types. FPGA, DSP, TEE, are all images that need to be "loaded" with platform specific handlers.
So my proposal with this patch is to *not* add a new image type loader, but to use the existing "loadable" type.
Right, but you are wanting to add board-specific handlers for each type. I think that it is a good use case for a linker list and a function that (given the type) returns the function to call to process that type (a bit like spl_ll_find_loader()).
Right now a given FIT configuration can have
kernel= ramdisk= dtb= loadables=
When for instance when kernel type is parsed we look at what type of kernel it is by looking at the node it points to (we also get the data/arch/etc.. from that node). When we parse loadables we should do the same, when the loadable type is a recognized type, we load in the data and call a platform hook to further process it.
I wonder if we should have a linker list with handlers for each type. We can search that list and call the provided function if there is one. We could have handlers for FPGA and TEE, for now.
This is almost what I have now, but with an ifdef'able switch block over the types.
I spent a while refactoring the loading code. It's a tricky thing, but I hope we can avoid filling it up with special cases again...
My hope as well, what I'm trying to avoid is doing it like this new Xilinx FPGA loader where we have a custom type loader in common code.
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.
Agreed we don't want that, and in fact the xilinux stuff in image.c isn't nice.
So moving forward, is this a better solution, should I un-RFC this patch? Then we can move over the Xilinx loader to this style.
I think so.
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.
Right. But if someone uses it when they should not, they'll get an error.
A hard to track down link-time error. With this ifdef'd off, they will also get an error, but the compiler will complain and point them right to the offending call.
It works the same either way, at least for me. A link-time error should point to the offending C line. [...]
Regards, Simon