* [U-Boot] [RFC 0/1] Add TEE loading support to FIT image
@ 2016-11-14 19:49 Andrew F. Davis
2016-11-14 19:49 ` [U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing Andrew F. Davis
0 siblings, 1 reply; 9+ messages in thread
From: Andrew F. Davis @ 2016-11-14 19:49 UTC (permalink / raw)
To: u-boot
Hello all,
Internally[0] we use the a hook in the FIT loadable section to install
our trusted execution environment solution (OPTEE) which is packaged
in the FIT image with the rest of the platform's images. I would like
to get some feedback if this will be an acceptable solution?
This patch is the first step where we allow a platform to add a
custom hook for the "tee" type loadable. The rest of the work
is in the platform specific call and so not relevant to this RFC.
The resulting FIT .its looks something like this:
...
am57xx-evm.optee {
description = "OPTEE OS Image";
data = /incbin/("am57xx-evm.optee");
type = "tee";
arch = "arm";
compression = "none";
};
...
configurations {
am57xx-evm {
description = "Linux kernel, FDT blob, and OPTEE OS for the AM57xx EVM";
kernel = "kernel at 1";
fdt = "am57xx-evm.dtb";
loadables = "am57xx-evm.optee";
};
...
Thanks,
Andrew
[0] git://git.ti.com/ti-u-boot/ti-u-boot.git
Andrew F. Davis (1):
image: Add TEE loading to FIT loadable processing
Kconfig | 10 ++++++++++
common/image.c | 18 ++++++++++++++++++
include/image.h | 15 +++++++++++++++
3 files changed, 43 insertions(+)
--
2.10.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing
2016-11-14 19:49 [U-Boot] [RFC 0/1] Add TEE loading support to FIT image Andrew F. Davis
@ 2016-11-14 19:49 ` Andrew F. Davis
2016-11-14 20:44 ` Simon Glass
2016-11-15 7:55 ` Michal Simek
0 siblings, 2 replies; 9+ messages in thread
From: Andrew F. Davis @ 2016-11-14 19:49 UTC (permalink / raw)
To: u-boot
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_DEVICE
+ 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",},
{ -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
}
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
+/**
+ * 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.
+ *
+ * @tee_image: pointer to the image
+ * @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);
+#endif /* CONFIG_FIT_IMAGE_TEE_PROCESS */
+
#endif /* __IMAGE_H__ */
--
2.10.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing
2016-11-14 19:49 ` [U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing Andrew F. Davis
@ 2016-11-14 20:44 ` Simon Glass
2016-11-14 21:55 ` Andrew F. Davis
2016-11-15 7:55 ` Michal Simek
1 sibling, 1 reply; 9+ messages in thread
From: Simon Glass @ 2016-11-14 20:44 UTC (permalink / raw)
To: u-boot
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_DEVICE
This is a generic option so I don't think it should depend on TI.
> + 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.
> { -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().
It is too ugly, I think, to check the image type in the 'load'
function, and do special things.
> }
> 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.
> +/**
> + * 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?
> + * @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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing
2016-11-14 20:44 ` Simon Glass
@ 2016-11-14 21:55 ` Andrew F. Davis
2016-11-15 0:34 ` Simon Glass
0 siblings, 1 reply; 9+ messages in thread
From: Andrew F. Davis @ 2016-11-14 21:55 UTC (permalink / raw)
To: u-boot
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_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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing
2016-11-14 21:55 ` Andrew F. Davis
@ 2016-11-15 0:34 ` Simon Glass
2016-11-15 17:07 ` Andrew F. Davis
0 siblings, 1 reply; 9+ messages in thread
From: Simon Glass @ 2016-11-15 0:34 UTC (permalink / raw)
To: u-boot
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_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.
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.
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.
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...
>
> 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.
>
>> 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.
>
>>> +/**
>>> + * 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.
OK - can you please add that info here?
>
>>> + * @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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing
2016-11-14 19:49 ` [U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing Andrew F. Davis
2016-11-14 20:44 ` Simon Glass
@ 2016-11-15 7:55 ` Michal Simek
2016-11-15 16:43 ` Andrew F. Davis
1 sibling, 1 reply; 9+ messages in thread
From: Michal Simek @ 2016-11-15 7:55 UTC (permalink / raw)
To: u-boot
Hi Andrew,
On 14.11.2016 20:49, Andrew F. Davis 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_DEVICE
> + 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",},
> { -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
> }
> 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
> +/**
> + * 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.
> + *
> + * @tee_image: pointer to the image
> + * @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);
> +#endif /* CONFIG_FIT_IMAGE_TEE_PROCESS */
> +
> #endif /* __IMAGE_H__ */
>
is this arm32 or arm64?
The reason why I am asking is that this option could be useful for
Xilinx's arm64 but this needs to be done before ATF is loaded because
ATF need to run it. That's why extending FIT format and handling in SPL
can be useful.
It has also connection to extending SPL to handle these cases for arm64.
Please keep me in the loop on this because I was adding that fpga
support which can be done by full u-boot later but there could be some
other cases where this needs to be done earlier in SPL too.
Thanks,
Michal
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing
2016-11-15 7:55 ` Michal Simek
@ 2016-11-15 16:43 ` Andrew F. Davis
0 siblings, 0 replies; 9+ messages in thread
From: Andrew F. Davis @ 2016-11-15 16:43 UTC (permalink / raw)
To: u-boot
On 11/15/2016 01:55 AM, Michal Simek wrote:
> Hi Andrew,
>
> On 14.11.2016 20:49, Andrew F. Davis 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_DEVICE
>> + 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",},
>> { -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
>> }
>> 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
>> +/**
>> + * 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.
>> + *
>> + * @tee_image: pointer to the image
>> + * @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);
>> +#endif /* CONFIG_FIT_IMAGE_TEE_PROCESS */
>> +
>> #endif /* __IMAGE_H__ */
>>
>
> is this arm32 or arm64?
> The reason why I am asking is that this option could be useful for
> Xilinx's arm64 but this needs to be done before ATF is loaded because
> ATF need to run it. That's why extending FIT format and handling in SPL
> can be useful.
> It has also connection to extending SPL to handle these cases for arm64.
> Please keep me in the loop on this because I was adding that fpga
> support which can be done by full u-boot later but there could be some
> other cases where this needs to be done earlier in SPL too.
>
I agree, I'm intentionally leaving this platform agnostic as we are
using across platforms that need different TEE types. If needed we can
add CONFIG_SPL_FIT_IMAGE_TEE_PROCESS, then the ugly specifics can be
handled in platform code.
Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing
2016-11-15 0:34 ` Simon Glass
@ 2016-11-15 17:07 ` Andrew F. Davis
2016-11-16 0:18 ` Simon Glass
0 siblings, 1 reply; 9+ messages in thread
From: Andrew F. Davis @ 2016-11-15 17:07 UTC (permalink / raw)
To: u-boot
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_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.
>
> 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 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.
>>
>>> 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.
>>
>>>> +/**
>>>> + * 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.
>
> OK - can you please add that info here?
>
Will do.
>>
>>>> + * @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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing
2016-11-15 17:07 ` Andrew F. Davis
@ 2016-11-16 0:18 ` Simon Glass
0 siblings, 0 replies; 9+ messages in thread
From: Simon Glass @ 2016-11-16 0:18 UTC (permalink / raw)
To: u-boot
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_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.
> >
> > 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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-11-16 0:18 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-14 19:49 [U-Boot] [RFC 0/1] Add TEE loading support to FIT image Andrew F. Davis
2016-11-14 19:49 ` [U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing Andrew F. Davis
2016-11-14 20:44 ` Simon Glass
2016-11-14 21:55 ` Andrew F. Davis
2016-11-15 0:34 ` Simon Glass
2016-11-15 17:07 ` Andrew F. Davis
2016-11-16 0:18 ` Simon Glass
2016-11-15 7:55 ` Michal Simek
2016-11-15 16:43 ` Andrew F. Davis
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.