psci reset with wdog #1849

MrVan · 2017-09-29T02:06:34Z

This is to support psci reset on i.MX. The wdog is shared between normal world and secure world. The secure world needs to parse the wdog node and check whether fsl,ext-reset-output exists or not, then decide to how to trigger wdog reset. imx_wdog_restart is exposed to imx psci reset.

Introduced get_dt_blob, dt_read_bool,CFG_MMAP_REGIONS.

CFG_MMAP_REGIONS maybe not a good way here, but the easiest way to fix my issue. core_mmu_add_mapping may fail, because RES_VASPACE entry in static memmap maybe used by others, such as device tree.

A few typo fixes.

etienne-lms

sorry, I have not been through all of it. Yet here's some feedback.

etienne-lms · 2017-09-29T17:32:49Z

core/include/kernel/dt.h

@@ -89,6 +89,16 @@ const struct dt_driver *__dt_driver_end(void);
 int dt_map_dev(const void *fdt, int offs, vaddr_t *base, size_t *size);

 /*
+ * Check whether the node at @offs contains the proerty with propname or not.


s/proerty/property/

etienne-lms · 2017-09-29T17:33:27Z

core/include/kernel/dt.h

+ * @offs is the offset of the node that describes the device in @fdt.
+ * @propname is the property that need to check
+ *
+ * Returs true on success or false if no propname.


s/Returs/Returns/

etienne-lms · 2017-09-29T17:34:23Z

core/kernel/dt.c

+
+	prop = fdt_getprop(fdt, offs, propname, NULL);
+
+	return prop ? true : false;


minor: return prop;

I would like to use true/false as return value, this is to check whether a property exist or not.

etienne-lms · 2017-09-29T17:35:03Z

core/kernel/dt.c

@@ -144,14 +144,17 @@ static paddr_t _fdt_read_paddr(const uint32_t *cell, int n)
 paddr_t _fdt_reg_base_address(const void *fdt, int offs)
 {
 	const void *reg;
-	int ncells;
-	int len;
+	int parent, ncells, len;


minor: split over 3 lines.

etienne-lms · 2017-09-29T17:38:39Z

core/arch/arm/kernel/generic_boot.c

@@ -684,6 +696,8 @@ static void init_fdt(unsigned long phys_fdt)
 	void *fdt;
 	int ret;

+	dt_blob_addr = NULL;


minor: not needed.

The dt_blob_addr is not a static variable. So init it with value NULL in init_fdt first.

ok. Will fix it.

etienne-lms · 2017-09-29T17:41:00Z

core/arch/arm/mm/core_mmu.c

@@ -51,7 +51,7 @@

 #include "core_mmu_private.h"

-#define MAX_MMAP_REGIONS	13
+#define MAX_MMAP_REGIONS	CFG_MMAP_REGIONS


I think the platform_config.h could set MAX_MMAP_REGIONS and change here:

+#ifndef MAX_MMAP_REGIONS #define MAX_MMAP_REGIONS 13 +#endif

Main point is that this config information does not really need to be CFG_xxx format which are mainly used for config settings required in makefile scripts and eventually also source files.

ok, fix in next version

etienne-lms · 2017-09-29T17:45:45Z

core/drivers/imx_wdog.c

+	write16(val, wdog_base + WCR_OFF);
+	write16(val, wdog_base + WCR_OFF);
+
+	asm volatile ("b .\r\n");


etienne-lms · 2017-09-29T17:47:06Z

core/arch/arm/plat-imx/conf.mk

@@ -82,4 +82,6 @@ ifeq ($(filter y, $(CFG_PSCI_ARM32)), y)
 CFG_HWSUPP_MEM_PERM_WXN = n
 endif

+CFG_MMAP_REGIONS ?= 24


see comment on MAX_MMAP_REGIONS.

jenswi-linaro · 2017-10-02T05:54:29Z

core/include/kernel/dt.h

+ *
+ * Returs true on success or false if no propname.
+ */
+bool dt_read_bool(const void *fdt, int offs, const char *propname);


I'd prefer dt_have_prop() or dt_prop_exists().

Fix in next version

jenswi-linaro · 2017-10-02T07:48:10Z

core/arch/arm/kernel/generic_boot.c

@@ -726,6 +740,8 @@ static void init_fdt(unsigned long phys_fdt)
 		     phys_fdt, ret);
 		panic();
 	}
+
+	dt_blob_addr = fdt;


This pointer isn't valid forever. It should be cleared when OP-TEE has initialized and exits the first time.

I need to parse the dtb to get fsl,ext-reset-output in wdog node. In this patchset, I use driver_init(xxx_wdog_xx) to handle that. If not expose dtb here, I am not able to parse dtb for the drivers. Could we expose fdt in init_fdt, and destroy the pointer when first back to normal world? Or we create a copy of the dtb in optee secure side?

If you clear it before init_primary_helper() (after calling init_teecore()) returns it should be OK.

Thanks. I'll clear it as you suggested.

jenswi-linaro · 2017-10-02T07:51:03Z

core/drivers/imx_wdog.c

+	uint32_t i;
+
+#ifdef CFG_MX7
+	const char *wdog_path[4] = {


Could have used [] instead of [4]

Fix in next version

MrVan · 2017-10-03T04:58:11Z

Updated:
proerty->property
Returs->Returns
Spilt varaiable definition into different lines.
Allow platform set MAX_MMAP_REGIONS.
asm volatile("b .") -> while(1);
dt_read_bool->dt_have_prop
set dt_blob_addr with NULL after init_teecore
const char *wdog_path[4] -> static const char * const wdog_path[];

jenswi-linaro · 2017-10-03T05:55:01Z

core/arch/arm/kernel/generic_boot.c

@@ -90,6 +90,11 @@ uint32_t sem_cpu_sync[CFG_TEE_CORE_NB_CORE];
 KEEP_PAGER(sem_cpu_sync);
 #endif

+#ifdef CFG_DT
+void *dt_blob_addr;
+KEEP_PAGER(dt_blob_addr);


This isn't needed, all non-const global variable are unpaged.

ok. Will remove it.

jenswi-linaro · 2017-10-03T05:56:23Z

core/arch/arm/include/kernel/generic_boot.h

@@ -61,4 +61,7 @@ int generic_boot_core_release(size_t core_idx, paddr_t entry);
 paddr_t generic_boot_core_hpen(void);
 #endif

+extern void *dt_blob_addr;


This could be just a static variable instead as there's a get function for it.

jenswi-linaro · 2017-10-03T05:59:37Z

core/arch/arm/kernel/generic_boot.c

+	if (cpu_mmu_enabled())
+		return dt_blob_addr;
+
+	return (void *)virt_to_phys(dt_blob_addr);


Given the limited time where dt_blob_addr is valid we can guarantee that MMU is always enabled while the variable is valid.

ok. Then let the function directly return dt_blob_addr is ok here.

add assert(cpu_mmu_enabled());

ok. Will add it.

jenswi-linaro · 2017-10-03T06:00:59Z

core/arch/arm/plat-imx/conf.mk

 endif

+MAX_MMAP_REGIONS ?= 24


This variable should have a CFG_ prefix, also the config magic is only applied to CFG_ variables if I'm not misstaken.

Per @etienne-lms 's comments:

I think the platform_config.h could set MAX_MMAP_REGIONS and change here: +#ifndef MAX_MMAP_REGIONS #define MAX_MMAP_REGIONS 13 +#endif Main point is that this config information does not really need to be CFG_xxx format which are mainly used for config settings required in makefile scripts and eventually also source files.

I changed to use MAX_MMAP_REGIONS.

So @jenswi-linaro @jforissier, you agree to use the following, add platform could define CFG_MMAP_REGIONS?

#ifndef CFG_MMAP_REGIONS #define CFG_MMAP_REGIONS 13 #endif #define MAX_MMAP_REGIONS CFG_MMAP_REGIONS

First thing: generally speaking, #ifndef CFG_X / #define CFG_X / #endif is not good. CFG_x ?= some_value in some .mk file should always be preferred.

Second thing: whether or not the variable should be a CFG_ is easy to decide. If it makes sense to choose the value a compile time, use CFG_. Otherwise, hard-code the value in a .h and do NOT use #ifndef.

Third thing: I don't understand why you would need two variables (MAX_MMAP_REGIONS and CFG_MMAP_REGIONS).

It seems to me CFG_MMAP_REGIONS ?= some_value is all we need. It may be set globally in core/config.mk and a platform-specific value may be given in core/arch/arm/plat-imx/conf.mk. A compile-time assert may be in order to make sure the value is acceptable in the end.

Thanks @jforissier . Replacing MAX_MMAP_REGIONS with CFG_MMAP_REGIONS is simpler. How about the following patch?

diff --git a/core/arch/arm/arm.mk b/core/arch/arm/arm.mk index bb65f77..6e52deb 100644 --- a/core/arch/arm/arm.mk +++ b/core/arch/arm/arm.mk @@ -4,6 +4,8 @@ CFG_LTC_OPTEE_THREAD ?= y CFG_CORE_TZSRAM_EMUL_SIZE ?= 458752 CFG_LPAE_ADDR_SPACE_SIZE ?= (1ull << 32) +CFG_MMAP_REGIONS ?= 13 + ifeq ($(CFG_ARM64_core),y) CFG_KERN_LINKER_FORMAT ?= elf64-littleaarch64 CFG_KERN_LINKER_ARCH ?= aarch64 diff --git a/core/arch/arm/mm/core_mmu.c b/core/arch/arm/mm/core_mmu.c index f2d728d..7c5fb3d 100644 --- a/core/arch/arm/mm/core_mmu.c +++ b/core/arch/arm/mm/core_mmu.c @@ -51,7 +51,6 @@ #include "core_mmu_private.h" -#define MAX_MMAP_REGIONS 13 #define RES_VASPACE_SIZE (CORE_MMU_PGDIR_SIZE * 10) #define SHM_VASPACE_SIZE (1024 * 1024 * 32) @@ -66,7 +65,7 @@ unsigned long default_nsec_shm_size; unsigned long default_nsec_shm_paddr; static struct tee_mmap_region - static_memory_map[MAX_MMAP_REGIONS + 1]; + static_memory_map[CFG_MMAP_REGIONS + 1]; /* Define the platform's memory layout. */ struct memaccess_area { @@ -911,6 +910,7 @@ void core_init_mmu_map(void) panic("Invalid memory access config: sec/nsec"); } + COMPILE_TIME_ASSERT(CFG_MMAP_REGIONS < 13); init_mem_map(static_memory_map, ARRAY_SIZE(static_memory_map)); map = static_memory_map;

@MrVan looks good to me!

I think MAX_MMAP_REGION does not need to reach make script.
This value allocated static memory so it's a good thing to have it tunable. But the amount of memory is quite low. Maybe we would need a debug trace trace to get the number of entry used against max defined value.

In this scope, we should rather look at MAX_XLAT_TABLES which cover bigger areas.

Minor comment: why restrict to 13. If no valid value, will panic. Ok 13 to be a default value, but why the lowest supported ?

@etienne-lms I have no idea about the default value 13, 13 is not large enough in my case. The patch I listed above is the easy way to fix the issue I met.

Covering bigger areas, do you mean XLAT_TABLE_ENTRIES?

jenswi-linaro · 2017-10-03T06:05:11Z

core/kernel/dt.c

+
+	prop = fdt_getprop(fdt, offs, propname, NULL);
+
+	return prop ? true : false;


There's no need for the ternary operator here. Since dt_have_prop() returns bool the compiler is required to normalize the value to return either true or false.

I am not sure, but let compiler to normalize a value to true or false maybe not good, different compilers may have different behaviors? gcc/llvm or else. I checked linux and freebsd code, they not use compiler to normalize the value. freebsd not use bool, see https://github.com/freebsd/freebsd/blob/master/sys/dev/ofw/openfirm.c#L371
Linux kernel

static inline bool of_property_read_bool(const struct device_node *np, const char *propname) { struct property *prop = of_find_property(np, propname, NULL); return prop ? true : false; }

So, could we keep prop ? true : false here?

? true : false is clearly useless here because the bool type in optee_os/core is always defined as _Bool, which is C99 standard.
There are reaons why the links you provide have the normalization to true/false (or 0/1):

The FreeBSD function returns int, which is not _Bool,

The Linux function is part of libfdt, which makes no assumption on the actual type behind bool.

From the C99 standard:
6.3.1.2 Boolean type
When any scalar value is converted to _Bool, the result is 0 if the value compares equal to 0; otherwise, the result is 1.

Thanks @jforissier for the explaination. I'll fix it in next version.

etienne-lms · 2017-10-02T13:09:45Z

core/drivers/imx_wdog.c

+		if (!core_mmu_add_mapping(mtype, pbase, sz)) {
+			EMSG("Failed to map %zu bytes at PA 0x%"PRIxPA,
+			     (size_t)sz, pbase);
+		return TEE_ERROR_GENERIC;


minor: indentation

etienne-lms · 2017-10-03T19:12:41Z

core/arch/arm/kernel/generic_boot.c

+	if (cpu_mmu_enabled())
+		return dt_blob_addr;
+
+	return (void *)virt_to_phys(dt_blob_addr);


add assert(cpu_mmu_enabled());

etienne-lms · 2017-10-03T19:14:59Z

core/arch/arm/kernel/generic_boot.c

@@ -760,6 +777,9 @@ static void init_primary_helper(unsigned long pageable_part,
 	init_vfp_nsec();
 	if (init_teecore() != TEE_SUCCESS)
 		panic();
+#ifdef CFG_DT
+	dt_blob_addr = NULL;


Proposal to add comment /* dt no more reached, reset pointer to invalid */. It is not obvious why this is cleared.
Even add a reset_dt_references(); to remove #ifdef CFG_xxx.

ok. I'll fix it.

etienne-lms · 2017-10-03T19:19:30Z

core/arch/arm/plat-imx/conf.mk

 endif

+MAX_MMAP_REGIONS ?= 24


I think MAX_MMAP_REGION does not need to reach make script.
This value allocated static memory so it's a good thing to have it tunable. But the amount of memory is quite low. Maybe we would need a debug trace trace to get the number of entry used against max defined value.

In this scope, we should rather look at MAX_XLAT_TABLES which cover bigger areas.

Minor comment: why restrict to 13. If no valid value, will panic. Ok 13 to be a default value, but why the lowest supported ?

etienne-lms · 2017-10-03T19:21:26Z

core/drivers/imx_wdog.c

+#include <io.h>
+#include <keep.h>
+#include <kernel/generic_boot.h>
+#include <kernel/dt.h>


minor: 2 sorting error.

etienne-lms · 2017-10-03T19:22:43Z

core/drivers/imx_wdog.c

+	paddr_t pbase;
+	vaddr_t vbase;
+	ssize_t sz;
+	int off, st;


minor: split in 2 lines.

MrVan · 2017-10-06T01:42:35Z

Updated:
Remove KEEP_PAGER(dt_blob_addr)
Add static to dt_blob_addr
add "assert(cpu_mmu_enabled())" to get_dt_blob
Use CFG_MMAP_REGIONS to replace MAX_MMAP_REGIONS
Change "return prop ? true: fase" to "return prop"
Fix header sorting error
Split "int off, st" to two lines
Fix indention
Add a reset_dt_references to clear dt_blob_addr and remove ifdef CFG_XXX

jenswi-linaro

With my comment addressed:
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro · 2017-10-06T06:43:56Z

core/arch/arm/kernel/generic_boot.c

@@ -90,6 +90,10 @@ uint32_t sem_cpu_sync[CFG_TEE_CORE_NB_CORE];
 KEEP_PAGER(sem_cpu_sync);
 #endif

+#ifdef CFG_DT
+void *dt_blob_addr;


This should be static

Typo fix: inseart -> insert Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

1. According to ePAPR spec, status should be okay/disabled/fail/fail-sss. To Linux device tree, "okay" and "ok" are both used. Function is_okay also use "okay" and "ok". But "ok" is not defined in spec. Here only correct comments 2. size -> sz Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Introudce dt_have_prop Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

According to ePAPR spec. " The #address-cells and #size-cells properties may be used in any device node that has children in the device tree hierarchy and describes how child device nodes should be addressed. The #address-cells property defines the number of <u32> cells used to encode the address field in a child node’s reg property. The #size-cells property defines the number of <u32> cells used to encode the size field in a child node’s reg property. The #address-cells and #size-cells properties are not inherited from ancestors in the device tree. They shall be explicitly defined. An ePAPR-compliant boot program shall supply #address-cells and #size-cells on all nodes that have children. If missing, a client program should assume a default value of 2 for An ePAPR-compliant boot program shall supply #address-cells and #size-cells on all nodes that have children. " So need to use the parent's address-cells and size-cells property. Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Introduce get_dt_blob. This could allow drivers to use device tree. Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Introduce CFG_MMAP_REGIONS to replace MAX_MMAP_REGIONS to allow platform specific value. Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Introducing the wdog support is for psci reset usage. To i.MX6/7, when `reboot`, need wdog to trigger soc reset or send out signal to pmic through wdog pin to trigger pmic reset. In linux device tree, there is a "fsl,ext-reset-output" property, this driver is to check whether the wdog node contains the property or not, then decide how to trigger reset. We still rely on normal world to initialize wdog and configure pinmux when need to trigger pmic reset. Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Set CFG_MMAP_REGIONS to 24. Signed-off-by: Peng Fan <peng.fan@nxp.com>

Implement psci reset support. Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

MrVan · 2017-10-07T11:12:45Z

R-b Tags applied with changing"void *dt_blob_addr;" -> "static void *dt_blob_addr".

etienne-lms suggested changes Oct 1, 2017

View reviewed changes

jenswi-linaro reviewed Oct 2, 2017

View reviewed changes

MrVan force-pushed the wdog-psci branch from 5703326 to 7aa622a Compare October 3, 2017 04:56

jenswi-linaro reviewed Oct 3, 2017

View reviewed changes

etienne-lms reviewed Oct 3, 2017

View reviewed changes

MrVan force-pushed the wdog-psci branch from 7aa622a to 0ea4820 Compare October 6, 2017 01:37

MrVan force-pushed the wdog-psci branch from 0ea4820 to 43f60bd Compare October 6, 2017 03:17

jenswi-linaro reviewed Oct 6, 2017

View reviewed changes

MrVan added 9 commits October 7, 2017 19:05

core: mmu: typo fix

0625a54

Typo fix: inseart -> insert Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

core: dt: introduce dt_have_prop

e7296f8

Introudce dt_have_prop Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

core: arm: introduce get_dt_blob

12a2a40

Introduce get_dt_blob. This could allow drivers to use device tree. Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

core: mmu: introduce CFG_MMAP_REGIONS

25bc79f

Introduce CFG_MMAP_REGIONS to replace MAX_MMAP_REGIONS to allow platform specific value. Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

core: arm: imx set CFG_MMAP_REGIONS

f65725a

Set CFG_MMAP_REGIONS to 24. Signed-off-by: Peng Fan <peng.fan@nxp.com>

core: imx: implement psci reset

d4f4bc2

Implement psci reset support. Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

MrVan force-pushed the wdog-psci branch from 43f60bd to d4f4bc2 Compare October 7, 2017 11:10

jforissier merged commit 093fb9c into OP-TEE:master Oct 10, 2017

MrVan deleted the wdog-psci branch October 10, 2017 08:35


		prop = fdt_getprop(fdt, offs, propname, NULL);

		return prop ? true : false;

psci reset with wdog #1849

psci reset with wdog #1849

Conversation

MrVan commented Sep 29, 2017

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrVan commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrVan commented Oct 6, 2017

jenswi-linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrVan commented Oct 7, 2017

jenswi-linaro Oct 2, 2017 •

edited

Loading