| Message ID | 1516033778-31557-1-git-send-email-sumit.garg@nxp.com |
|---|---|
| State | Superseded |
| Delegated to: | York Sun |
| Headers | show |
| Series | [U-Boot] armv8: sec_firmware: Add support for multiple loadables | expand |
On 01/14/2018 08:55 PM, Sumit Garg wrote: > Enable support for multiple loadable images in SEC firmware FIT image. > > Signed-off-by: Sumit Garg <sumit.garg@nxp.com> > --- > arch/arm/cpu/armv8/sec_firmware.c | 51 +++++++++++++++++++++++++++++++-------- > 1 file changed, 41 insertions(+), 10 deletions(-) > > diff --git a/arch/arm/cpu/armv8/sec_firmware.c b/arch/arm/cpu/armv8/sec_firmware.c > index 927eae4..28de81c 100644 > --- a/arch/arm/cpu/armv8/sec_firmware.c > +++ b/arch/arm/cpu/armv8/sec_firmware.c > @@ -116,11 +116,13 @@ static int sec_firmware_check_copy_loadable(const void *sec_firmware_img, > u32 *loadable_l, u32 *loadable_h) > { > phys_addr_t sec_firmware_loadable_addr = 0; > - int conf_node_off, ld_node_off; > + int conf_node_off, ld_node_off, images; > char *conf_node_name = NULL; > const void *data; > size_t size; > ulong load; > + const char *name, *str, *type; > + int len; > > conf_node_name = SEC_FIRMEWARE_FIT_CNF_NAME; > > @@ -130,11 +132,32 @@ static int sec_firmware_check_copy_loadable(const void *sec_firmware_img, > return -ENOENT; > } > > - ld_node_off = fit_conf_get_prop_node(sec_firmware_img, conf_node_off, > - FIT_LOADABLE_PROP); > - if (ld_node_off >= 0) { > - printf("SEC Firmware: '%s' present in config\n", > - FIT_LOADABLE_PROP); > + /* find the node holding the images information */ > + images = fdt_path_offset(sec_firmware_img, FIT_IMAGES_PATH); > + if (images < 0) { > + debug("%s: Cannot find /images node: %d\n", __func__, images); Do you expect this often? If it should happen, change debug to printf. > + return -1; > + } > + > + type = FIT_LOADABLE_PROP; > + > + name = fdt_getprop(sec_firmware_img, conf_node_off, type, &len); > + if (!name) { > + /* Loadables not present */ > + return 0; > + } > + > + printf("SEC Firmware: '%s' present in config\n", type); How about replace printf() with debug()? > + > + for (str = name; str && ((str - name) < len); > + str = strchr(str, '\0') + 1) { > + printf("%s: '%s'\n", type, str); > + ld_node_off = fdt_subnode_offset(sec_firmware_img, images, str); > + if (ld_node_off < 0) { > + printf("cannot find image node '%s': %d\n", str, > + ld_node_off); > + return -EINVAL; > + } > > /* Verify secure firmware image */ > if (!(fit_image_verify(sec_firmware_img, ld_node_off))) { > @@ -164,11 +187,19 @@ static int sec_firmware_check_copy_loadable(const void *sec_firmware_img, > memcpy((void *)sec_firmware_loadable_addr, data, size); > flush_dcache_range(sec_firmware_loadable_addr, > sec_firmware_loadable_addr + size); > - } > > - /* Populate address ptrs for loadable image with loadbale addr */ > - out_le32(loadable_l, (sec_firmware_loadable_addr & WORD_MASK)); > - out_le32(loadable_h, (sec_firmware_loadable_addr >> WORD_SHIFT)); > + /* Populate loadable address only for Trusted OS */ > + if (!strcmp(str, "trustedOS@1")) { > + /* > + * Populate address ptrs for loadable image with > + * loadbale addr > + */ > + out_le32(loadable_l, (sec_firmware_loadable_addr & > + WORD_MASK)); > + out_le32(loadable_h, (sec_firmware_loadable_addr >> > + WORD_SHIFT)); > + } > + } > > return 0; > } > It is unclear that you are looking for "trustedOS@1" to set the address. Please add some document to describe the FIT image you are expecting. York
> From: York Sun > Sent: Monday, January 15, 2018 10:16 PM > > On 01/14/2018 08:55 PM, Sumit Garg wrote: > > Enable support for multiple loadable images in SEC firmware FIT image. > > > > Signed-off-by: Sumit Garg <sumit.garg@nxp.com> > > --- > > arch/arm/cpu/armv8/sec_firmware.c | 51 > > +++++++++++++++++++++++++++++++-------- > > 1 file changed, 41 insertions(+), 10 deletions(-) > > > > diff --git a/arch/arm/cpu/armv8/sec_firmware.c > > b/arch/arm/cpu/armv8/sec_firmware.c > > index 927eae4..28de81c 100644 > > --- a/arch/arm/cpu/armv8/sec_firmware.c > > +++ b/arch/arm/cpu/armv8/sec_firmware.c > > @@ -116,11 +116,13 @@ static int sec_firmware_check_copy_loadable(const > void *sec_firmware_img, > > u32 *loadable_l, u32 *loadable_h) { > > phys_addr_t sec_firmware_loadable_addr = 0; > > - int conf_node_off, ld_node_off; > > + int conf_node_off, ld_node_off, images; > > char *conf_node_name = NULL; > > const void *data; > > size_t size; > > ulong load; > > + const char *name, *str, *type; > > + int len; > > > > conf_node_name = SEC_FIRMEWARE_FIT_CNF_NAME; > > > > @@ -130,11 +132,32 @@ static int sec_firmware_check_copy_loadable(const > void *sec_firmware_img, > > return -ENOENT; > > } > > > > - ld_node_off = fit_conf_get_prop_node(sec_firmware_img, > conf_node_off, > > - FIT_LOADABLE_PROP); > > - if (ld_node_off >= 0) { > > - printf("SEC Firmware: '%s' present in config\n", > > - FIT_LOADABLE_PROP); > > + /* find the node holding the images information */ > > + images = fdt_path_offset(sec_firmware_img, FIT_IMAGES_PATH); > > + if (images < 0) { > > + debug("%s: Cannot find /images node: %d\n", __func__, > images); > > Do you expect this often? If it should happen, change debug to printf. No this isn't expected as fit image should at least have ppa monitor image. > > > + return -1; > > + } > > + > > + type = FIT_LOADABLE_PROP; > > + > > + name = fdt_getprop(sec_firmware_img, conf_node_off, type, &len); > > + if (!name) { > > + /* Loadables not present */ > > + return 0; > > + } > > + > > + printf("SEC Firmware: '%s' present in config\n", type); > > How about replace printf() with debug()? Actually I wanted to differentiate from u-boot logs whether there were any "loadables" in ppa fit image or not. > > > + > > + for (str = name; str && ((str - name) < len); > > + str = strchr(str, '\0') + 1) { > > + printf("%s: '%s'\n", type, str); > > + ld_node_off = fdt_subnode_offset(sec_firmware_img, > images, str); > > + if (ld_node_off < 0) { > > + printf("cannot find image node '%s': %d\n", str, > > + ld_node_off); > > + return -EINVAL; > > + } > > > > /* Verify secure firmware image */ > > if (!(fit_image_verify(sec_firmware_img, ld_node_off))) { @@ > > -164,11 +187,19 @@ static int sec_firmware_check_copy_loadable(const void > *sec_firmware_img, > > memcpy((void *)sec_firmware_loadable_addr, data, size); > > flush_dcache_range(sec_firmware_loadable_addr, > > sec_firmware_loadable_addr + size); > > - } > > > > - /* Populate address ptrs for loadable image with loadbale addr */ > > - out_le32(loadable_l, (sec_firmware_loadable_addr & WORD_MASK)); > > - out_le32(loadable_h, (sec_firmware_loadable_addr >> > WORD_SHIFT)); > > + /* Populate loadable address only for Trusted OS */ > > + if (!strcmp(str, "trustedOS@1")) { > > + /* > > + * Populate address ptrs for loadable image with > > + * loadbale addr > > + */ > > + out_le32(loadable_l, (sec_firmware_loadable_addr & > > + WORD_MASK)); > > + out_le32(loadable_h, (sec_firmware_loadable_addr >> > > + WORD_SHIFT)); > > + } > > + } > > > > return 0; > > } > > > > It is unclear that you are looking for "trustedOS@1" to set the address. Actually this patch is meant to load "fuse provisioning script" used by PPA to provision fuses on our SoC's in PPA DDR space (2MB). And this allocation of PPA DDR space for this script will be fixed and hardcoded in PPA. But in case of trusted OS with own DDR space (64MB), we have to communicate load address of trusted OS to PPA to initialize it. > Please add some document to describe the FIT image you are expecting. Sure I will add a document to describe the FIT image. -Sumit
On 01/15/2018 09:09 AM, Sumit Garg wrote: >> From: York Sun >> Sent: Monday, January 15, 2018 10:16 PM >> >> On 01/14/2018 08:55 PM, Sumit Garg wrote: >>> Enable support for multiple loadable images in SEC firmware FIT image. >>> >>> Signed-off-by: Sumit Garg <sumit.garg@nxp.com> >>> --- >>> arch/arm/cpu/armv8/sec_firmware.c | 51 >>> +++++++++++++++++++++++++++++++-------- >>> 1 file changed, 41 insertions(+), 10 deletions(-) >>> >>> diff --git a/arch/arm/cpu/armv8/sec_firmware.c >>> b/arch/arm/cpu/armv8/sec_firmware.c >>> index 927eae4..28de81c 100644 >>> --- a/arch/arm/cpu/armv8/sec_firmware.c >>> +++ b/arch/arm/cpu/armv8/sec_firmware.c >>> @@ -116,11 +116,13 @@ static int sec_firmware_check_copy_loadable(const >> void *sec_firmware_img, >>> u32 *loadable_l, u32 *loadable_h) { >>> phys_addr_t sec_firmware_loadable_addr = 0; >>> - int conf_node_off, ld_node_off; >>> + int conf_node_off, ld_node_off, images; >>> char *conf_node_name = NULL; >>> const void *data; >>> size_t size; >>> ulong load; >>> + const char *name, *str, *type; >>> + int len; >>> >>> conf_node_name = SEC_FIRMEWARE_FIT_CNF_NAME; >>> >>> @@ -130,11 +132,32 @@ static int sec_firmware_check_copy_loadable(const >> void *sec_firmware_img, >>> return -ENOENT; >>> } >>> >>> - ld_node_off = fit_conf_get_prop_node(sec_firmware_img, >> conf_node_off, >>> - FIT_LOADABLE_PROP); >>> - if (ld_node_off >= 0) { >>> - printf("SEC Firmware: '%s' present in config\n", >>> - FIT_LOADABLE_PROP); >>> + /* find the node holding the images information */ >>> + images = fdt_path_offset(sec_firmware_img, FIT_IMAGES_PATH); >>> + if (images < 0) { >>> + debug("%s: Cannot find /images node: %d\n", __func__, >> images); >> >> Do you expect this often? If it should happen, change debug to printf. > > No this isn't expected as fit image should at least have ppa monitor image. The idea is to reduce unnecessary messages and make sure the error is raised. In this case, if the image is not found, you cannot proceed with secure booting. So a visible error is needed. York
> -----Original Message----- > From: York Sun > Sent: Monday, January 15, 2018 10:43 PM > > On 01/15/2018 09:09 AM, Sumit Garg wrote: > >> From: York Sun > >> Sent: Monday, January 15, 2018 10:16 PM > >> > >> On 01/14/2018 08:55 PM, Sumit Garg wrote: > >>> Enable support for multiple loadable images in SEC firmware FIT image. > >>> > >>> Signed-off-by: Sumit Garg <sumit.garg@nxp.com> > >>> --- > >>> arch/arm/cpu/armv8/sec_firmware.c | 51 > >>> +++++++++++++++++++++++++++++++-------- > >>> 1 file changed, 41 insertions(+), 10 deletions(-) > >>> > >>> diff --git a/arch/arm/cpu/armv8/sec_firmware.c > >>> b/arch/arm/cpu/armv8/sec_firmware.c > >>> index 927eae4..28de81c 100644 > >>> --- a/arch/arm/cpu/armv8/sec_firmware.c > >>> +++ b/arch/arm/cpu/armv8/sec_firmware.c > >>> @@ -116,11 +116,13 @@ static int > >>> sec_firmware_check_copy_loadable(const > >> void *sec_firmware_img, > >>> u32 *loadable_l, u32 *loadable_h) { > >>> phys_addr_t sec_firmware_loadable_addr = 0; > >>> - int conf_node_off, ld_node_off; > >>> + int conf_node_off, ld_node_off, images; > >>> char *conf_node_name = NULL; > >>> const void *data; > >>> size_t size; > >>> ulong load; > >>> + const char *name, *str, *type; > >>> + int len; > >>> > >>> conf_node_name = SEC_FIRMEWARE_FIT_CNF_NAME; > >>> > >>> @@ -130,11 +132,32 @@ static int > >>> sec_firmware_check_copy_loadable(const > >> void *sec_firmware_img, > >>> return -ENOENT; > >>> } > >>> > >>> - ld_node_off = fit_conf_get_prop_node(sec_firmware_img, > >> conf_node_off, > >>> - FIT_LOADABLE_PROP); > >>> - if (ld_node_off >= 0) { > >>> - printf("SEC Firmware: '%s' present in config\n", > >>> - FIT_LOADABLE_PROP); > >>> + /* find the node holding the images information */ > >>> + images = fdt_path_offset(sec_firmware_img, FIT_IMAGES_PATH); > >>> + if (images < 0) { > >>> + debug("%s: Cannot find /images node: %d\n", __func__, > >> images); > >> > >> Do you expect this often? If it should happen, change debug to printf. > > > > No this isn't expected as fit image should at least have ppa monitor image. > > The idea is to reduce unnecessary messages and make sure the error is raised. > In this case, if the image is not found, you cannot proceed with secure booting. > So a visible error is needed. > > York Sure I will put up a printf instead of debug here. -Sumit
diff --git a/arch/arm/cpu/armv8/sec_firmware.c b/arch/arm/cpu/armv8/sec_firmware.c index 927eae4..28de81c 100644 --- a/arch/arm/cpu/armv8/sec_firmware.c +++ b/arch/arm/cpu/armv8/sec_firmware.c @@ -116,11 +116,13 @@ static int sec_firmware_check_copy_loadable(const void *sec_firmware_img, u32 *loadable_l, u32 *loadable_h) { phys_addr_t sec_firmware_loadable_addr = 0; - int conf_node_off, ld_node_off; + int conf_node_off, ld_node_off, images; char *conf_node_name = NULL; const void *data; size_t size; ulong load; + const char *name, *str, *type; + int len; conf_node_name = SEC_FIRMEWARE_FIT_CNF_NAME; @@ -130,11 +132,32 @@ static int sec_firmware_check_copy_loadable(const void *sec_firmware_img, return -ENOENT; } - ld_node_off = fit_conf_get_prop_node(sec_firmware_img, conf_node_off, - FIT_LOADABLE_PROP); - if (ld_node_off >= 0) { - printf("SEC Firmware: '%s' present in config\n", - FIT_LOADABLE_PROP); + /* find the node holding the images information */ + images = fdt_path_offset(sec_firmware_img, FIT_IMAGES_PATH); + if (images < 0) { + debug("%s: Cannot find /images node: %d\n", __func__, images); + return -1; + } + + type = FIT_LOADABLE_PROP; + + name = fdt_getprop(sec_firmware_img, conf_node_off, type, &len); + if (!name) { + /* Loadables not present */ + return 0; + } + + printf("SEC Firmware: '%s' present in config\n", type); + + for (str = name; str && ((str - name) < len); + str = strchr(str, '\0') + 1) { + printf("%s: '%s'\n", type, str); + ld_node_off = fdt_subnode_offset(sec_firmware_img, images, str); + if (ld_node_off < 0) { + printf("cannot find image node '%s': %d\n", str, + ld_node_off); + return -EINVAL; + } /* Verify secure firmware image */ if (!(fit_image_verify(sec_firmware_img, ld_node_off))) { @@ -164,11 +187,19 @@ static int sec_firmware_check_copy_loadable(const void *sec_firmware_img, memcpy((void *)sec_firmware_loadable_addr, data, size); flush_dcache_range(sec_firmware_loadable_addr, sec_firmware_loadable_addr + size); - } - /* Populate address ptrs for loadable image with loadbale addr */ - out_le32(loadable_l, (sec_firmware_loadable_addr & WORD_MASK)); - out_le32(loadable_h, (sec_firmware_loadable_addr >> WORD_SHIFT)); + /* Populate loadable address only for Trusted OS */ + if (!strcmp(str, "trustedOS@1")) { + /* + * Populate address ptrs for loadable image with + * loadbale addr + */ + out_le32(loadable_l, (sec_firmware_loadable_addr & + WORD_MASK)); + out_le32(loadable_h, (sec_firmware_loadable_addr >> + WORD_SHIFT)); + } + } return 0; }
Enable support for multiple loadable images in SEC firmware FIT image. Signed-off-by: Sumit Garg <sumit.garg@nxp.com> --- arch/arm/cpu/armv8/sec_firmware.c | 51 +++++++++++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 10 deletions(-)