Hi Andrew,
On 15 November 2016 at 10:07, Andrew F. Davis <a...@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 <a...@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 <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.
> >
> > 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
