Re: [U-Boot] [PATCHv6 2/6] ARMv8: add the secure monitor firmware framework
Hi York,
Thanks for your comments!
> -Original Message-
> From: york sun
> Sent: 2016年6月23日 0:11
> To: Zhiqiang Hou ; u-boot@lists.denx.de;
> albert.u.b...@aribaud.net; scottw...@freescale.com;
> mingkai...@freescale.com; york...@freescale.com; le...@freescale.com;
> prabha...@freescale.com; bhupesh.sha...@freescale.com
> Subject: Re: [PATCHv6 2/6] ARMv8: add the secure monitor firmware framework
>
> On 06/21/2016 08:42 PM, Zhiqiang Hou wrote:
> > From: Hou Zhiqiang
> >
> > This framework is introduced for ARMv8 secure monitor mode firmware.
> > The main functions of the framework are, on EL3, verify the firmware,
> > load it to the secure memory and jump into it, and while it returned
> > to U-Boot, do some necessary setups at the 'target exception level'
> > that is determined by the respective secure firmware.
> >
> > So far, the framework support only FIT format image, and need to
> > define the name of which config node should be used in
> > 'configurations' and the name of property for the raw secure firmware image
> > in
> that config.
> > The FIT image should be stored in Byte accessing memory, such as NOR
> > Flash, or else it should be copied to main memory to use this framework.
> >
> > Signed-off-by: Hou Zhiqiang
> > ---
> > V6:
> > - Abstracted more code from PPA to this framework.
> > - Introduced gd->sec_firmware to hold the load address.
> > - Refactor the func sec_firmware_support_psci_version().
>
> A lot of change in this version.
Yes, take a lot time to refactor the code, just hope more code can be reused.
> >
> > V5:
> > - Added c file sec_firmware.c.
> > - Added declaration of sec_firmware_init().
> > - Renamed the func sec_firmware_validate().
> >
> > V4:
> > - Reordered this patch.
> > - Removed the FSL PPA related items.
> >
> > arch/arm/cpu/armv8/Makefile | 1 +
> > arch/arm/cpu/armv8/sec_firmware.c | 262
> ++
> > arch/arm/cpu/armv8/sec_firmware_asm.S | 53 ++
> > arch/arm/include/asm/armv8/sec_firmware.h | 18 ++
> > include/asm-generic/global_data.h | 11 ++
> > 5 files changed, 346 insertions(+)
> > create mode 100644 arch/arm/cpu/armv8/sec_firmware.c
> > create mode 100644 arch/arm/cpu/armv8/sec_firmware_asm.S
> > create mode 100644 arch/arm/include/asm/armv8/sec_firmware.h
> >
> > diff --git a/arch/arm/cpu/armv8/Makefile b/arch/arm/cpu/armv8/Makefile
> > index bf8644c..ee9e009 100644
> > --- a/arch/arm/cpu/armv8/Makefile
> > +++ b/arch/arm/cpu/armv8/Makefile
> > @@ -15,6 +15,7 @@ obj-y += cache.o
> > obj-y += tlb.o
> > obj-y += transition.o
> > obj-y += fwcall.o
> > +obj-$(CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT) += sec_firmware.o
> > +sec_firmware_asm.o
> >
> > obj-$(CONFIG_FSL_LAYERSCAPE) += fsl-layerscape/
> > obj-$(CONFIG_S32V234) += s32v234/
> > diff --git a/arch/arm/cpu/armv8/sec_firmware.c
> > b/arch/arm/cpu/armv8/sec_firmware.c
> > new file mode 100644
> > index 000..986df48
> > --- /dev/null
> > +++ b/arch/arm/cpu/armv8/sec_firmware.c
> > @@ -0,0 +1,266 @@
> > +/*
> > + * Copyright 2016 NXP Semiconductor, Inc.
> > + *
> > + * SPDX-License-Identifier:GPL-2.0+
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +extern void c_runtime_cpu_setup(void);
> > +
> > +static int sec_firmware_get_data(void *sec_firmware_img,
> > + const void **data, size_t *size)
>
> Throughout this patch, sec_firmware_img is used as read-only. How about add
> "const" to it?
Yes, will add it next version.
>
> > +{
> > + void *fit_hdr;
>
> Variable fit_hdr doesn't serve more purpose. You can use sec_firmware_img
> directly.
Yes, will fix it next version.
>
> > + int conf_node_off, fw_node_off;
> > + char *conf_node_name = NULL;
> > + char *desc;
> > + int ret;
> > +
> > + fit_hdr = sec_firmware_img;
> > + conf_node_name = SEC_FIRMEWARE_FIT_CNF_NAME;
> > +
> > + conf_node_off = fit_conf_get_node(fit_hdr, conf_node_name);
> > + if (conf_node_off < 0) {
> > + printf("SEC Firmware: %s: no such config\n", conf_node_name);
> > + return -ENOENT;
> > + }
> > +
> > + fw_node_off = fit_conf_get_prop_node(fit_hdr, conf_node_off,
> > + SEC_FIRMWARE_FIT_IMAGE);
> > + if (fw_node_off < 0) {
> > + printf("SEC Firmware: No '%s' in config\n",
> > + SEC_FIRMWARE_FIT_IMAGE);
>
> You have many of this alignment issues throughout this patch.
>
Will fix the alignment issues next version.
Thanks,
Zhiqiang
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv6 2/6] ARMv8: add the secure monitor firmware framework
Hi York,
Thanks for your comments!
> -Original Message-
> From: york sun
> Sent: 2016年6月23日 0:20
> To: Zhiqiang Hou ; u-boot@lists.denx.de;
> albert.u.b...@aribaud.net; scottw...@freescale.com;
> mingkai...@freescale.com; york...@freescale.com; le...@freescale.com;
> prabha...@freescale.com; bhupesh.sha...@freescale.com
> Subject: Re: [PATCHv6 2/6] ARMv8: add the secure monitor firmware framework
>
> On 06/21/2016 08:42 PM, Zhiqiang Hou wrote:
>
>
>
> > +
> > +#ifdef CONFIG_ARMV8_PSCI
> > +/*
> > + * The PSCI_VERSION function is added from PSCI v0.2. When the PSCI
> > + * v0.1 received this function, the NOT_SUPPORTED (0x_) error
> > + * number will be returned according to SMC Calling Conventions. But
> > + * when getting the NOT_SUPPORTED error number, we cannot ensure if
> > + * the PSCI version is v0.1 or other error occurred. So, PSCI v0.1
> > + * won't be supported by this framework.
> > + * And if the secure firmware isn't running, return NOT_SUPPORTED.
> > + *
> > + * The return value on success is PSCI version in format
> > + * major[31:16]:minor[15:0].
> > + */
> > +unsigned int sec_firmware_support_psci_version(void)
> > +{
> > + if (gd->sec_firmware & SEC_FIRMWARE_RUNNING)
> > + return _sec_firmware_support_psci_version();
> > +
> > + return 0x;
> > +}
> > +#endif
>
> Does _sec_firmware_support_psci_version() always return version numbers?
> Any chance it returns an error code?
If the PSCI_VERSION was supported in current PSCI version, it will return the
version,
otherwise, the SMC will return the value 0x_ to indicate the
PSCI_VERSION isn't
supported.
There isn't any description for returning error code in the PSCI spec.
Thanks,
Zhiqiang
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv6 2/6] ARMv8: add the secure monitor firmware framework
On 06/21/2016 08:42 PM, Zhiqiang Hou wrote:
> +
> +#ifdef CONFIG_ARMV8_PSCI
> +/*
> + * The PSCI_VERSION function is added from PSCI v0.2. When the PSCI
> + * v0.1 received this function, the NOT_SUPPORTED (0x_) error
> + * number will be returned according to SMC Calling Conventions. But
> + * when getting the NOT_SUPPORTED error number, we cannot ensure if
> + * the PSCI version is v0.1 or other error occurred. So, PSCI v0.1
> + * won't be supported by this framework.
> + * And if the secure firmware isn't running, return NOT_SUPPORTED.
> + *
> + * The return value on success is PSCI version in format
> + * major[31:16]:minor[15:0].
> + */
> +unsigned int sec_firmware_support_psci_version(void)
> +{
> + if (gd->sec_firmware & SEC_FIRMWARE_RUNNING)
> + return _sec_firmware_support_psci_version();
> +
> + return 0x;
> +}
> +#endif
Does _sec_firmware_support_psci_version() always return version numbers?
Any chance it returns an error code?
York
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv6 2/6] ARMv8: add the secure monitor firmware framework
On 06/21/2016 08:42 PM, Zhiqiang Hou wrote:
> From: Hou Zhiqiang
>
> This framework is introduced for ARMv8 secure monitor mode firmware.
> The main functions of the framework are, on EL3, verify the firmware,
> load it to the secure memory and jump into it, and while it returned
> to U-Boot, do some necessary setups at the 'target exception level'
> that is determined by the respective secure firmware.
>
> So far, the framework support only FIT format image, and need to define
> the name of which config node should be used in 'configurations' and
> the name of property for the raw secure firmware image in that config.
> The FIT image should be stored in Byte accessing memory, such as NOR
> Flash, or else it should be copied to main memory to use this framework.
>
> Signed-off-by: Hou Zhiqiang
> ---
> V6:
> - Abstracted more code from PPA to this framework.
> - Introduced gd->sec_firmware to hold the load address.
> - Refactor the func sec_firmware_support_psci_version().
A lot of change in this version.
>
> V5:
> - Added c file sec_firmware.c.
> - Added declaration of sec_firmware_init().
> - Renamed the func sec_firmware_validate().
>
> V4:
> - Reordered this patch.
> - Removed the FSL PPA related items.
>
> arch/arm/cpu/armv8/Makefile | 1 +
> arch/arm/cpu/armv8/sec_firmware.c | 262
> ++
> arch/arm/cpu/armv8/sec_firmware_asm.S | 53 ++
> arch/arm/include/asm/armv8/sec_firmware.h | 18 ++
> include/asm-generic/global_data.h | 11 ++
> 5 files changed, 346 insertions(+)
> create mode 100644 arch/arm/cpu/armv8/sec_firmware.c
> create mode 100644 arch/arm/cpu/armv8/sec_firmware_asm.S
> create mode 100644 arch/arm/include/asm/armv8/sec_firmware.h
>
> diff --git a/arch/arm/cpu/armv8/Makefile b/arch/arm/cpu/armv8/Makefile
> index bf8644c..ee9e009 100644
> --- a/arch/arm/cpu/armv8/Makefile
> +++ b/arch/arm/cpu/armv8/Makefile
> @@ -15,6 +15,7 @@ obj-y += cache.o
> obj-y += tlb.o
> obj-y += transition.o
> obj-y += fwcall.o
> +obj-$(CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT) += sec_firmware.o sec_firmware_asm.o
>
> obj-$(CONFIG_FSL_LAYERSCAPE) += fsl-layerscape/
> obj-$(CONFIG_S32V234) += s32v234/
> diff --git a/arch/arm/cpu/armv8/sec_firmware.c
> b/arch/arm/cpu/armv8/sec_firmware.c
> new file mode 100644
> index 000..986df48
> --- /dev/null
> +++ b/arch/arm/cpu/armv8/sec_firmware.c
> @@ -0,0 +1,266 @@
> +/*
> + * Copyright 2016 NXP Semiconductor, Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +extern void c_runtime_cpu_setup(void);
> +
> +static int sec_firmware_get_data(void *sec_firmware_img,
> + const void **data, size_t *size)
Throughout this patch, sec_firmware_img is used as read-only. How about
add "const" to it?
> +{
> + void *fit_hdr;
Variable fit_hdr doesn't serve more purpose. You can use
sec_firmware_img directly.
> + int conf_node_off, fw_node_off;
> + char *conf_node_name = NULL;
> + char *desc;
> + int ret;
> +
> + fit_hdr = sec_firmware_img;
> + conf_node_name = SEC_FIRMEWARE_FIT_CNF_NAME;
> +
> + conf_node_off = fit_conf_get_node(fit_hdr, conf_node_name);
> + if (conf_node_off < 0) {
> + printf("SEC Firmware: %s: no such config\n", conf_node_name);
> + return -ENOENT;
> + }
> +
> + fw_node_off = fit_conf_get_prop_node(fit_hdr, conf_node_off,
> + SEC_FIRMWARE_FIT_IMAGE);
> + if (fw_node_off < 0) {
> + printf("SEC Firmware: No '%s' in config\n",
> + SEC_FIRMWARE_FIT_IMAGE);
You have many of this alignment issues throughout this patch.
York
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
