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

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

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

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