[U-Boot] [PATCHv6 2/6] ARMv8: add the secure monitor firmware framework

[Zhiqiang Hou]

Hi York,

Thanks for your comments!

> -----Original Message-----
> From: york sun
> Sent: 2016年6月23日 0:11
> To: Zhiqiang Hou <zhiqiang.hou at nxp.com>; u-boot at lists.denx.de;
> albert.u.boot at aribaud.net; scottwood at freescale.com;
> Mingkai.hu at freescale.com; yorksun at freescale.com; leoli at freescale.com;
> prabhakar at freescale.com; bhupesh.sharma at 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 <Zhiqiang.Hou at nxp.com>
> >
> > 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 <Zhiqiang.Hou at nxp.com>
> > ---
> > 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 0000000..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 <common.h>
> > +#include <errno.h>
> > +#include <linux/kernel.h>
> > +#include <asm/io.h>
> > +#include <asm/system.h>
> > +#include <asm/types.h>
> > +#include <asm/macro.h>
> > +#include <asm/armv8/sec_firmware.h>
> > +
> > +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