AM43xx Suspend/Resume #1822

glneo · 2017-09-18T18:56:18Z

Hello all,

This is the AM43xx suspend/resume patch based on the common code in: #1729

Thanks,
Andrew

jforissier · 2017-09-18T21:14:48Z

core/arch/arm/plat-ti/sm_platform_handler_a9.c

 #include "api_monitor_index_a9.h"

+uint32_t suspend_regs[16];
+void sm_pm_cpu_do_suspend(uint32_t *ptr);


Remove this line and #include <sm/pm.h> instead.

jforissier · 2017-09-18T21:29:53Z

core/arch/arm/plat-ti/sub.mk

@@ -3,4 +3,4 @@ srcs-y += main.c
 srcs-$(CFG_PL310) += ti_pl310.c
 srcs-$(PLATFORM_FLAVOR_dra7xx) += sm_platform_handler_a15.c
 srcs-$(PLATFORM_FLAVOR_am57xx) += sm_platform_handler_a15.c
-srcs-$(PLATFORM_FLAVOR_am43xx) += sm_platform_handler_a9.c
+srcs-$(PLATFORM_FLAVOR_am43xx) += sm_platform_handler_a9.c a9_plat_init.S ../sm/pm_a32.S ../sm/pm.c


~~Are the sm files necessary? I would think they are already added, as long as CFG_WITH_ARM_TRUSTED_FIRMARE=n (see~~

optee_os/core/arch/arm/arm.mk

Lines 57 to 59 in 59ac392

ifneq ($(CFG_WITH_ARM_TRUSTED_FW),y)

core-platform-subdirs += $(arch-dir)/sm

endif

).

Ignore my previous comment, I did not read the file names correctly.

If you need to use only ../sm/pm_a32.S and ../sm/pm.c, you need to split the following line in core/arch/arm/sm/sub.mk and have them included by some CFG_ variable:

srcs-$(CFG_PSCI_ARM32) += std_smc.c psci.c pm.c psci-helper.S pm_a32.S

Indeed, it is not good practice to include files from an external directory.

I didn't like doing that either, but I wasn't sure how to split up CFG_PSCI_ARM32, so was looking for guidance on that. Maybe:

srcs-$(CFG_PM_ARM32) += pm.c pm_a32.S srcs-$(CFG_PSCI_ARM32) += std_smc.c psci.c psci-helper.S

But then how do we enforce the dependency from PSCI -> PM here without Kconfig, some kind of rule in a .mk file?

This should be enough:

ifeq ($(CFG_PSCI_ARM32),y) $(call force,CFG_PM_ARM32,y) endif

Since this pattern appears in several places, we could as well write a function to declare dependencies, but I'm a bit worried we may end up writing something uselessly complex...

MrVan · 2017-09-19T00:56:31Z

@glneo I have a common question, why not using psci in non-secure side?

etienne-lms · 2017-09-19T08:54:51Z

core/arch/arm/plat-ti/sm_platform_handler_a9.c

+			arm_cl2_cleaninvbypa(pl310_base(),
+					     virt_to_phys(suspend_regs),
+					     virt_to_phys(suspend_regs) +
+					     sizeof(suspend_regs));


l1+pl310 flush requires clean inner + flush outer + flush inner.
(http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0246h/Beicdhde.html)

Interesting, will fix.

By the way this was modeled after: core/arch/arm/sm/pm.c (lines 67-71), is this not correct then?

pm.c cleans the cache, hence clean L1 + clean L2.
Your code seems to clean & invalidate the caches, hence clean L1/flush L2/flush 1.

If we only need to clean the cache, would it be preferred then to only clean here?

Yes, i think so.

glneo · 2017-09-19T14:21:03Z

@MrVan This is a really good question that we have debated internally. We are attempting to leverage as much of the existing non-secure suspend/resume code as possible. If we went with a full PSCI path, we would have to duplicate a lot of code as it must still exist in-kernel for the non-secure devices.

I do not believe anything in this patch prevents a regular PSCI path from being implemented for our platforms at a later time should we need.

MrVan · 2017-09-20T07:42:49Z

@glneo The flow is ok, and could minimize changes to kernel which previous runs in secure world. Just wonder, could your implementation guarantee(could not be rooted by hacker) that when resumeing back, the entry will first runs into TEE, then tee back to linux?

glneo · 2017-09-21T18:28:42Z

@MrVan

It is true that a compromised non-secure side can decide to not re-start OP-TEE, but this does not present a security issue as the old ROM based secure side will still be present. It is up to the non-secure side to start OP-TEE in the first place (in U-Boot), so nothing is really lost here.

Also, OP-TEE is always in firewalled memory, even through suspend/resume, so before it is restarted we still have no risk of non-secure side tampering.

jforissier · 2017-09-25T22:40:24Z

For "core: arm: psci: Split PM config from PSCI": Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

I will actually pick up that patch and merge it now, independent of this PR, because if fixes an issue with current master:

$ make -s CFG_WITH_LPAE=y
core/arch/arm/sm/pm_a32.S:96:2: error: #error "Not supported"
 #error "Not supported"
  ^
core/arch/arm/sm/pm_a32.S:186:2: error: #error "Not supported -"
 #error "Not supported -"
  ^
mk/compile.mk:146: recipe for target 'out/arm-plat-vexpress/core/arch/arm/sm/pm_a32.o' failed
make: *** [out/arm-plat-vexpress/core/arch/arm/sm/pm_a32.o] Error 1

The issue was in fact triggered by 5402a9f ("qemu_virt: enable smp boot"), and that is the proper fix AFAICT.

jforissier · 2017-09-28T20:00:29Z

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

etienne-lms

A late comment about flat mapping constraint.
Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

etienne-lms · 2017-09-26T12:26:43Z

core/arch/arm/plat-ti/a9_plat_init.S

+	adr	lr, after_resume
+	push	{r4 - r12, lr}
+
+	ldr	r0, =suspend_regs


This expects suspend_regs is flat mapped: maybe add a comment about it.

Comment added.

etienne-lms · 2017-09-26T12:28:13Z

core/arch/arm/plat-ti/a9_plat_init.S

+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+ /*


very minor: extra space.

When the non-secure world is attempting to suspend it will call into the secure side using a platform service call. We implement this here in OP-TEE by saving the needed secure side registers. On resume the ROM will restore the secure side to its original configuration and OP-TEE will be re-entered from its boot reset vector. Add a check for the resume case and restore the secure registers if we are resuming. Signed-off-by: Andrew F. Davis <afd@ti.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

glneo force-pushed the am43xx-suspend branch from 3a5422d to 7c530ff Compare September 18, 2017 20:22

jforissier reviewed Sep 18, 2017

View reviewed changes

etienne-lms reviewed Sep 19, 2017

View reviewed changes

glneo force-pushed the am43xx-suspend branch from 7c530ff to 98871a1 Compare September 19, 2017 16:09

glneo force-pushed the am43xx-suspend branch from 98871a1 to dca661c Compare September 27, 2017 17:44

glneo force-pushed the am43xx-suspend branch from dca661c to a2f7011 Compare September 28, 2017 23:17

etienne-lms reviewed Sep 29, 2017

View reviewed changes

glneo force-pushed the am43xx-suspend branch from a2f7011 to 1ddafc1 Compare September 29, 2017 16:14

jforissier merged commit 0ec8746 into OP-TEE:master Sep 29, 2017

glneo deleted the am43xx-suspend branch September 30, 2017 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AM43xx Suspend/Resume #1822

AM43xx Suspend/Resume #1822

glneo commented Sep 18, 2017

jforissier Sep 18, 2017

jforissier Sep 18, 2017 •

edited

Loading

jforissier Sep 19, 2017

glneo Sep 19, 2017

jforissier Sep 19, 2017

MrVan commented Sep 19, 2017

etienne-lms Sep 19, 2017

glneo Sep 19, 2017

etienne-lms Sep 19, 2017

glneo Sep 19, 2017

etienne-lms Sep 19, 2017

glneo commented Sep 19, 2017

MrVan commented Sep 20, 2017

glneo commented Sep 21, 2017

jforissier commented Sep 25, 2017

jforissier commented Sep 28, 2017

etienne-lms left a comment

etienne-lms Sep 26, 2017

glneo Sep 29, 2017

etienne-lms Sep 26, 2017

glneo Sep 29, 2017

	ifneq ($(CFG_WITH_ARM_TRUSTED_FW),y)
	core-platform-subdirs += $(arch-dir)/sm
	endif

AM43xx Suspend/Resume #1822

AM43xx Suspend/Resume #1822

Conversation

glneo commented Sep 18, 2017

Choose a reason for hiding this comment

jforissier Sep 18, 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 Sep 19, 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

glneo commented Sep 19, 2017

MrVan commented Sep 20, 2017

glneo commented Sep 21, 2017

jforissier commented Sep 25, 2017

jforissier commented Sep 28, 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

jforissier Sep 18, 2017 •

edited

Loading