| Message ID | 1516391006-22483-7-git-send-email-bryan.odonoghue@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add new OPTEE bootm support to u-boot | expand |
On 01/19/2018 01:43 PM, Bryan O'Donoghue wrote: > This patch adds support for bootable OPTEE images to mkimage. Currently > there is a (Trusted Execution Environment) TEE image type, the TEE image > type is installed to a memory location with u-boot continuing to own the > boot process whereas the OPTEE image type defined here is a bootable image, > which typically wants to live at a defined location in memory. Defining a > new image type allows us to pull out the load address and entry point > defined in the OPTEE header and having a separate image type lays the > foundation for a subsequent patch to validate the OPTEE memory defined in a > board-port matches the link location specified in the OPTEE bootable > image. > > example usage: > > mkimage -A arm -T optee -C none -d ./out/arm-plat-imx/core/tee.bin > uTee.optee > > making a separate image type means you don't need to specify things like > entry point and load address as you would if you were defining the OPTEE > image as a linux image. > I'm still not getting the reasoning for this all, you can have the load address checks and relocation stuff inside your loadable handler: U_BOOT_FIT_LOADABLE_HANDLER(IH_TYPE_TEE, board_tee_image_process); Then define 'board_tee_image_process' to do whatever you want to your image. > mkimage -A arm -O linux -C none -a 0x9c0fffe4 -e 0x9c100000 -d > ./out/arm-plat-imx/core/tee.bin uTee > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Cc: Harinarayan Bhatta <harinarayan@ti.com> > Cc: Andrew F. Davis <afd@ti.com> > Cc: Tom Rini <trini@konsulko.com> > Cc: Kever Yang <kever.yang@rock-chips.com> > Cc: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > Cc: Peng Fan <peng.fan@nxp.com> > Tested-by: Peng Fan <peng.fan@nxp.com> > --- > common/image.c | 1 + > include/image.h | 1 + > tools/default_image.c | 25 +++++++++++++++++++------ > 3 files changed, 21 insertions(+), 6 deletions(-) > > diff --git a/common/image.c b/common/image.c > index e9609cd..14e738b 100644 > --- a/common/image.c > +++ b/common/image.c > @@ -161,6 +161,7 @@ static const table_entry_t uimage_type[] = { > { IH_TYPE_TEE, "tee", "Trusted Execution Environment Image",}, > { IH_TYPE_FIRMWARE_IVT, "firmware_ivt", "Firmware with HABv4 IVT" }, > { IH_TYPE_PMMC, "pmmc", "TI Power Management Micro-Controller Firmware",}, > + { IH_TYPE_OPTEE, "optee", "OPTEE Boot Image",}, > { -1, "", "", }, > }; > > diff --git a/include/image.h b/include/image.h > index a128a62..9175624 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -271,6 +271,7 @@ enum { > IH_TYPE_TEE, /* Trusted Execution Environment OS Image */ > IH_TYPE_FIRMWARE_IVT, /* Firmware Image with HABv4 IVT */ > IH_TYPE_PMMC, /* TI Power Management Micro-Controller Firmware */ > + IH_TYPE_OPTEE, /* OPTEE Boot Image */ > > IH_TYPE_COUNT, /* Number of image types */ > }; > diff --git a/tools/default_image.c b/tools/default_image.c > index 4e5568e..5653933 100644 > --- a/tools/default_image.c > +++ b/tools/default_image.c > @@ -18,6 +18,7 @@ > #include "mkimage.h" > > #include <image.h> > +#include <tee/optee.h> > #include <u-boot/crc.h> > > static image_header_t header; > @@ -25,7 +26,8 @@ static image_header_t header; > static int image_check_image_types(uint8_t type) > { > if (((type > IH_TYPE_INVALID) && (type < IH_TYPE_FLATDT)) || > - (type == IH_TYPE_KERNEL_NOLOAD) || (type == IH_TYPE_FIRMWARE_IVT)) > + (type == IH_TYPE_KERNEL_NOLOAD) || (type == IH_TYPE_FIRMWARE_IVT) || > + (type == IH_TYPE_OPTEE)) > return EXIT_SUCCESS; > else > return EXIT_FAILURE; > @@ -90,6 +92,8 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd, > uint32_t checksum; > time_t time; > uint32_t imagesize; > + uint32_t ep; > + uint32_t addr; > > image_header_t * hdr = (image_header_t *)ptr; > > @@ -99,18 +103,27 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd, > sbuf->st_size - sizeof(image_header_t)); > > time = imagetool_get_source_date(params, sbuf->st_mtime); > - if (params->type == IH_TYPE_FIRMWARE_IVT) > + ep = params->ep; > + addr = params->addr; > + imagesize = sbuf->st_size - sizeof(image_header_t); > + > + switch (params->type) { > + case IH_TYPE_FIRMWARE_IVT: > /* Add size of CSF minus IVT */ > imagesize = sbuf->st_size - sizeof(image_header_t) + 0x1FE0; > - else > - imagesize = sbuf->st_size - sizeof(image_header_t); > + break; > + case IH_TYPE_OPTEE: > + addr = optee_image_get_load_addr(hdr); > + ep = optee_image_get_entry_point(hdr); > + break; > + } > > /* Build new header */ > image_set_magic(hdr, IH_MAGIC); > image_set_time(hdr, time); > image_set_size(hdr, imagesize); > - image_set_load(hdr, params->addr); > - image_set_ep(hdr, params->ep); > + image_set_load(hdr, addr); > + image_set_ep(hdr, ep); > image_set_dcrc(hdr, checksum); > image_set_os(hdr, params->os); > image_set_arch(hdr, params->arch); >
On 19/01/18 20:14, Andrew F. Davis wrote: > On 01/19/2018 01:43 PM, Bryan O'Donoghue wrote: >> This patch adds support for bootable OPTEE images to mkimage. Currently >> there is a (Trusted Execution Environment) TEE image type, the TEE image >> type is installed to a memory location with u-boot continuing to own the >> boot process whereas the OPTEE image type defined here is a bootable image, >> which typically wants to live at a defined location in memory. Defining a >> new image type allows us to pull out the load address and entry point >> defined in the OPTEE header and having a separate image type lays the >> foundation for a subsequent patch to validate the OPTEE memory defined in a >> board-port matches the link location specified in the OPTEE bootable >> image. >> >> example usage: >> >> uTee.optee >> >> making a separate image type means you don't need to specify things like >> entry point and load address as you would if you were defining the OPTEE >> image as a linux image. >> > > I'm still not getting the reasoning for this all, you can have the load > address checks and relocation stuff inside your loadable handler: > > U_BOOT_FIT_LOADABLE_HANDLER(IH_TYPE_TEE, board_tee_image_process Hi Andrew, As I understand it, that's a board-specific method, which wants to install a TEE (jump into a TEE and return to u-boot), whereas the aim with this patch-set is to chain-load and boot via TEE - OPTEE in this case. I should make that clearer in the patch text description. The example from Peng Fang >> mkimage -A arm -O linux -C none -a 0x9c0fffe4 -e 0x9c100000 -d >> ./out/arm-plat-imx/core/tee.bin uTee is then simplified down to this >> mkimage -A arm -T optee -C none -d ./out/arm-plat-imx/core/tee.bin and remove the requirement to pass load and entry point on the command line. The boot-flow looks like this BootROM -> u-boot {load kernel, dtb, optee} -> OPTEE -> Kernel Whereas - as I understand it the flow on the TI examples you have is BootROM -> u-boot {load kernel, dtb, TEE} -> TEE -> u-boot -> Kernel Note: I do believe IH_TYPE_TEE is the right type to use for Kever's patch with the SPL - because again as I understand it - the model is BootROM -> SPL -> Install TEE -> u-boot -> etc --- bod
On 01/19/2018 05:59 PM, Bryan O'Donoghue wrote: > > > On 19/01/18 20:14, Andrew F. Davis wrote: >> On 01/19/2018 01:43 PM, Bryan O'Donoghue wrote: >>> This patch adds support for bootable OPTEE images to mkimage. Currently >>> there is a (Trusted Execution Environment) TEE image type, the TEE image >>> type is installed to a memory location with u-boot continuing to own the >>> boot process whereas the OPTEE image type defined here is a bootable >>> image, >>> which typically wants to live at a defined location in memory. >>> Defining a >>> new image type allows us to pull out the load address and entry point >>> defined in the OPTEE header and having a separate image type lays the >>> foundation for a subsequent patch to validate the OPTEE memory >>> defined in a >>> board-port matches the link location specified in the OPTEE bootable >>> image. >>> >>> example usage: >>> > >>> uTee.optee >>> >>> making a separate image type means you don't need to specify things like >>> entry point and load address as you would if you were defining the OPTEE >>> image as a linux image. >>> >> >> I'm still not getting the reasoning for this all, you can have the load >> address checks and relocation stuff inside your loadable handler: >> >> U_BOOT_FIT_LOADABLE_HANDLER(IH_TYPE_TEE, board_tee_image_process > Hi Andrew, > > As I understand it, that's a board-specific method, which wants to > install a TEE (jump into a TEE and return to u-boot), whereas the aim > with this patch-set is to chain-load and boot via TEE - OPTEE in this case. > This is not board-specific, this is the flow all ARM boards I know of use (except i.MX 6). It is certainly possible to use the same flow on TI boards if we wanted, we just don't as in my mind OP-TEE logically should jump back to the non-secure side were it was called from so non-secure boot can continue. Not act as another boot loader stage. Is there some technical reason I am missing as to why you want to use this alternate flow? > I should make that clearer in the patch text description. > That would be great. > The example from Peng Fang > >>> mkimage -A arm -O linux -C none -a 0x9c0fffe4 -e 0x9c100000 -d >>> ./out/arm-plat-imx/core/tee.bin uTee > I haven't used mkimage in a while, but how is this any different than what we do with the kernel image? Why do we need to pull this info out of the header when we don't do the same for Linux? > is then simplified down to this > >>> mkimage -A arm -T optee -C none -d ./out/arm-plat-imx/core/tee.bin > > and remove the requirement to pass load and entry point on the command > line. > To me the save in this command (which should be handled automatically during the build process), doesn't justify the additional complexity in the boot flow. > The boot-flow looks like this > > BootROM -> u-boot {load kernel, dtb, optee} -> OPTEE -> Kernel > > Whereas - as I understand it the flow on the TI examples you have is > > BootROM -> u-boot {load kernel, dtb, TEE} -> TEE -> u-boot -> Kernel > This is correct, we also on some platforms load additional firmware, so returning to u-boot after each image is deployed is a must. BootROM -> (u-boot u-boot u-boot u-boot) -> kernel | | | | | | v | v | v | TEE -> PMMC -> Something else, etc.. You seem to be locking yourself in with your flow, as now TEE *must* be the last item you load. What happens when you want to make secure calls into OP-TEE from U-Boot? You won't be able to move it back in the stages. (We are now doing this to handle some new Android secure-boot requirements, so it is not that far-fetched) > Note: I do believe IH_TYPE_TEE is the right type to use for Kever's > patch with the SPL - because again as I understand it - the model is > > BootROM -> SPL -> Install TEE -> u-boot -> etc > > --- > bod
On 22/01/18 14:46, Andrew F. Davis wrote: >> As I understand it, that's a board-specific method, which wants to >> install a TEE (jump into a TEE and return to u-boot), whereas the aim >> with this patch-set is to chain-load and boot via TEE - OPTEE in this case. >> > > This is not board-specific, this is the flow all ARM boards I know of > use (except i.MX 6). The OPTEE port I'm working with is i.MX 7, which chain-loads in this same way. > Is there some technical reason I am missing as to why you want to use > this alternate flow? The reason is the upstream OPTEE port we are working with already uses this bootflow. >> The example from Peng Fang >> >>>> mkimage -A arm -O linux -C none -a 0x9c0fffe4 -e 0x9c100000 -d >>>> ./out/arm-plat-imx/core/tee.bin uTee >> > > I haven't used mkimage in a while, but how is this any different than > what we do with the kernel image? Why do we need to pull this info out > of the header when we don't do the same for Linux? So for a kernel you are typically making a uImage of a compressed kernel image and therefore you have to pass load-address and entry point. mkimage -A arm -O linux -T kernel -C none -a 0x80008000 -e 0x80008000 -n "Linux kernel" -d arch/arm/boot/zImage uImage For the bootable OPTEE image case I'm proposing 1. A distinct image type 2. Based on that image type we validate the OPTEE header MAGIC, version, etc 3. Based on the OPTEE header we can validate the location the OPTEE binary gets loaded to. Having a distinct image type makes it more robust. >>>> mkimage -A arm -T optee -C none -d ./out/arm-plat-imx/core/tee.bin >>> and remove the requirement to pass load and entry point on the command >> line. >> > To me the save in this command (which should be handled automatically > during the build process), As above, it's about image generation, validation and load-address sanity checking. I apologize for not making that clearer upfront - my bad, I'll attempt to flesh-out the patch descriptions to make the logic clearer. --- bod
diff --git a/common/image.c b/common/image.c index e9609cd..14e738b 100644 --- a/common/image.c +++ b/common/image.c @@ -161,6 +161,7 @@ static const table_entry_t uimage_type[] = { { IH_TYPE_TEE, "tee", "Trusted Execution Environment Image",}, { IH_TYPE_FIRMWARE_IVT, "firmware_ivt", "Firmware with HABv4 IVT" }, { IH_TYPE_PMMC, "pmmc", "TI Power Management Micro-Controller Firmware",}, + { IH_TYPE_OPTEE, "optee", "OPTEE Boot Image",}, { -1, "", "", }, }; diff --git a/include/image.h b/include/image.h index a128a62..9175624 100644 --- a/include/image.h +++ b/include/image.h @@ -271,6 +271,7 @@ enum { IH_TYPE_TEE, /* Trusted Execution Environment OS Image */ IH_TYPE_FIRMWARE_IVT, /* Firmware Image with HABv4 IVT */ IH_TYPE_PMMC, /* TI Power Management Micro-Controller Firmware */ + IH_TYPE_OPTEE, /* OPTEE Boot Image */ IH_TYPE_COUNT, /* Number of image types */ }; diff --git a/tools/default_image.c b/tools/default_image.c index 4e5568e..5653933 100644 --- a/tools/default_image.c +++ b/tools/default_image.c @@ -18,6 +18,7 @@ #include "mkimage.h" #include <image.h> +#include <tee/optee.h> #include <u-boot/crc.h> static image_header_t header; @@ -25,7 +26,8 @@ static image_header_t header; static int image_check_image_types(uint8_t type) { if (((type > IH_TYPE_INVALID) && (type < IH_TYPE_FLATDT)) || - (type == IH_TYPE_KERNEL_NOLOAD) || (type == IH_TYPE_FIRMWARE_IVT)) + (type == IH_TYPE_KERNEL_NOLOAD) || (type == IH_TYPE_FIRMWARE_IVT) || + (type == IH_TYPE_OPTEE)) return EXIT_SUCCESS; else return EXIT_FAILURE; @@ -90,6 +92,8 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd, uint32_t checksum; time_t time; uint32_t imagesize; + uint32_t ep; + uint32_t addr; image_header_t * hdr = (image_header_t *)ptr; @@ -99,18 +103,27 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd, sbuf->st_size - sizeof(image_header_t)); time = imagetool_get_source_date(params, sbuf->st_mtime); - if (params->type == IH_TYPE_FIRMWARE_IVT) + ep = params->ep; + addr = params->addr; + imagesize = sbuf->st_size - sizeof(image_header_t); + + switch (params->type) { + case IH_TYPE_FIRMWARE_IVT: /* Add size of CSF minus IVT */ imagesize = sbuf->st_size - sizeof(image_header_t) + 0x1FE0; - else - imagesize = sbuf->st_size - sizeof(image_header_t); + break; + case IH_TYPE_OPTEE: + addr = optee_image_get_load_addr(hdr); + ep = optee_image_get_entry_point(hdr); + break; + } /* Build new header */ image_set_magic(hdr, IH_MAGIC); image_set_time(hdr, time); image_set_size(hdr, imagesize); - image_set_load(hdr, params->addr); - image_set_ep(hdr, params->ep); + image_set_load(hdr, addr); + image_set_ep(hdr, ep); image_set_dcrc(hdr, checksum); image_set_os(hdr, params->os); image_set_arch(hdr, params->arch);