core: dt: Make it possible to alter device mapping #5603

vesajaaskelainen · 2022-10-23T12:44:50Z

In case where IP core device is TrustZone aware and is used by both REE
and TEE dt_map_dev() would normally cause non-secure mapping for the
device.

When selected registers in IP core are only accessible by TrustZone device
needs to be mapped with MEM_AREA_IO_SEC to cause actual AXI memory access
be made with AWPROT[1] and ARPROT[1] bits configured properly.

This adds new argument for dt_map_dev() to enable forcing mapping to be
secure or non-secure.

Signed-off-by: Vesa Jääskeläinen vesa.jaaskelainen@vaisala.com

vesajaaskelainen · 2022-10-23T12:45:45Z

This is split from PR #5350 as we could not re-open that.

Also decided to split the commits to own PRs.

jenswi-linaro · 2022-10-24T10:45:27Z

core/kernel/dt.c

@@ -125,6 +125,39 @@ int dt_map_dev(const void *fdt, int offs, vaddr_t *base, size_t *size)
 	return 0;
 }

+int dt_map_secure_dev(const void *fdt, int offs, vaddr_t *base, size_t *size)


This duplicates most of the code in dt_map_dev(). Would it be possible to refactor the two functions to use a common helper function or something?

Maybe add a hint argument to dt_map_dev():
enum map_dev_directive { MAP_FROM_NODE_STATUS, MAP_SECURE, MAP_NON_SECURE };

Sounds good to me.

I can try the proposed method.

xiaoxuZeng · 2022-10-25T02:34:40Z

core/kernel/dt.c

+		return -1;
+	sz = _fdt_reg_size(fdt, offs);
+	if (sz == DT_INFO_INVALID_REG_SIZE)
+		return -1;


The err_code(-1) returned by different exception branches. Maybe inconvenient for debug.

But returning to -1 seems to be the norm in this file. :)

Right. This is legacy and could move to TEE_Result return value.

etienne-lms

dt_map_dev() maps with ns=0 when secure-status = "okay" & status = "disabled". @vesajaaskelainen, you don't want to rely on this scheme? You need an API function that enforces the secure state?

etienne-lms · 2022-10-25T06:23:36Z

core/kernel/dt.c

+		return -1;
+	sz = _fdt_reg_size(fdt, offs);
+	if (sz == DT_INFO_INVALID_REG_SIZE)
+		return -1;


Right. This is legacy and could move to TEE_Result return value.

vesajaaskelainen · 2022-10-25T08:22:29Z

dt_map_dev() maps with ns=0 when secure-status = "okay" & status = "disabled". @vesajaaskelainen, you don't want to rely on this scheme? You need an API function that enforces the secure state?

The problem here is that the real device tree would be:

status = "enabled"
secure-status = "enabled"

Eg. part of the registers are accessible for Linux kernel drivers and full set is available for OP-TEE driver.

If you do this the MMU mapping will be non-secure resulting non-secure access in FPGA side and we cannot access the TZ protected registers.

vesajaaskelainen · 2022-10-25T08:52:30Z

Alternative for this function would be I suppose:

invent new device tree keyword
re-work logic when both statuses are enabled so that actual mapping is secure. But don't know what that would break.

etienne-lms

invent new device tree keyword

I don't think it is the preferred way...

re-work logic when both statuses are enabled so that actual mapping is secure. But don't know what that would break.

That would be sowhat more consistent IMO. There are impacted drivers, we should get an Acked-by from them.

etienne-lms · 2022-10-25T12:53:49Z

core/kernel/dt.c

@@ -125,6 +125,39 @@ int dt_map_dev(const void *fdt, int offs, vaddr_t *base, size_t *size)
 	return 0;
 }

+int dt_map_secure_dev(const void *fdt, int offs, vaddr_t *base, size_t *size)


Maybe add a hint argument to dt_map_dev():
enum map_dev_directive { MAP_FROM_NODE_STATUS, MAP_SECURE, MAP_NON_SECURE };

vesajaaskelainen · 2022-10-28T09:20:22Z

core/include/kernel/dt.h

+ * DT_MAP_NON_SECURE: Force mapping for device to be non-secure.
+ */
+enum dt_map_dev_directive {
+	DT_MAP_FROM_NODE_STATUS,


Was thinking should this be DT_MAP_AUTO or so.

Yes, that's a good name.

I wait a bit if there are other comments and then go with this (needs a change in quite a many places 😓)

FWIW... DT_MAP_AUTO sounds good to me too

vesajaaskelainen · 2022-10-28T09:21:18Z

Now I have updated the PR based on comments.

Please check it out if it matches your ideas.

jenswi-linaro · 2022-10-31T07:12:10Z

Please change the prefix in the commit subject from kernel to core.

vesajaaskelainen · 2022-10-31T12:14:03Z

Please change the prefix in the commit subject from kernel to core.

OK

vesajaaskelainen · 2022-10-31T16:34:12Z

Updated the PR based on comments.

jenswi-linaro · 2022-11-01T07:44:56Z

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

vesajaaskelainen · 2022-11-01T11:44:18Z

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Thanks for the review! Applied tag.

etienne-lms

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

In case where IP core device is TrustZone aware and is used by both REE and TEE dt_map_dev() would normally cause non-secure mapping for the device. When selected registers in IP core are only accessible by TrustZone device needs to be mapped with MEM_AREA_IO_SEC to cause actual AXI memory access be made with AWPROT[1] and ARPROT[1] bits configured properly. This adds new argument for dt_map_dev() to enable forcing mapping to be secure or non-secure. Signed-off-by: Vesa Jääskeläinen <vesa.jaaskelainen@vaisala.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

vesajaaskelainen · 2022-11-02T14:20:45Z

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

Thanks for the review! Applied tags.

vesajaaskelainen · 2022-11-02T15:28:58Z

About failed checks. These passed nicely on my github fork's actions.

So some transient failure somewhere else?

jforissier · 2022-11-02T15:41:47Z

About failed checks. These passed nicely on my github fork's actions.

So some transient failure somewhere else?

Possible. Or perhaps there is a race condition somewhere -- 2002.1 is a multi-threaded TCP socket test. Unfortunately CI doesn't produce detailed logs and even with those it might be difficult to understand what happened. I have restarted the job. Update: it completed successfully.

vesajaaskelainen mentioned this pull request Oct 23, 2022

Improving Xilinx Zynq-7000 support #5605

Open

jenswi-linaro reviewed Oct 24, 2022

View reviewed changes

xiaoxuZeng reviewed Oct 25, 2022

View reviewed changes

etienne-lms reviewed Oct 25, 2022

View reviewed changes

vesajaaskelainen force-pushed the map-device-as-secure branch from 996ad84 to a951b4a Compare October 28, 2022 09:19

vesajaaskelainen commented Oct 28, 2022

View reviewed changes

vesajaaskelainen changed the title ~~kernel: dt: Add new method to map device as secure~~ kernel: dt: Make it possible to alter device mapping Oct 28, 2022

vesajaaskelainen force-pushed the map-device-as-secure branch from a951b4a to 3834a1b Compare October 31, 2022 16:32

vesajaaskelainen changed the title ~~kernel: dt: Make it possible to alter device mapping~~ core: dt: Make it possible to alter device mapping Oct 31, 2022

vesajaaskelainen force-pushed the map-device-as-secure branch from 3834a1b to 1a927de Compare November 1, 2022 11:43

etienne-lms reviewed Nov 2, 2022

View reviewed changes

vesajaaskelainen force-pushed the map-device-as-secure branch from 1a927de to dee2f84 Compare November 2, 2022 14:20

jforissier merged commit a5d5bbc into OP-TEE:master Nov 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: dt: Make it possible to alter device mapping #5603

core: dt: Make it possible to alter device mapping #5603

vesajaaskelainen commented Oct 23, 2022 •

edited

vesajaaskelainen commented Oct 23, 2022

jenswi-linaro Oct 24, 2022

etienne-lms Oct 25, 2022

jenswi-linaro Oct 27, 2022

vesajaaskelainen Oct 28, 2022

xiaoxuZeng Oct 25, 2022 •

edited

etienne-lms Oct 25, 2022

etienne-lms left a comment

etienne-lms Oct 25, 2022

vesajaaskelainen commented Oct 25, 2022

vesajaaskelainen commented Oct 25, 2022 •

edited

etienne-lms left a comment •

edited

etienne-lms Oct 25, 2022

vesajaaskelainen Oct 28, 2022

jenswi-linaro Oct 31, 2022

vesajaaskelainen Oct 31, 2022

jforissier Oct 31, 2022

vesajaaskelainen commented Oct 28, 2022

jenswi-linaro commented Oct 31, 2022

vesajaaskelainen commented Oct 31, 2022

vesajaaskelainen commented Oct 31, 2022

jenswi-linaro commented Nov 1, 2022

vesajaaskelainen commented Nov 1, 2022

etienne-lms left a comment

vesajaaskelainen commented Nov 2, 2022

vesajaaskelainen commented Nov 2, 2022

jforissier commented Nov 2, 2022 •

edited

core: dt: Make it possible to alter device mapping #5603

core: dt: Make it possible to alter device mapping #5603

Conversation

vesajaaskelainen commented Oct 23, 2022 • edited

vesajaaskelainen commented Oct 23, 2022

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

xiaoxuZeng Oct 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vesajaaskelainen commented Oct 25, 2022

vesajaaskelainen commented Oct 25, 2022 • edited

etienne-lms left a comment • edited

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

vesajaaskelainen commented Oct 28, 2022

jenswi-linaro commented Oct 31, 2022

vesajaaskelainen commented Oct 31, 2022

vesajaaskelainen commented Oct 31, 2022

jenswi-linaro commented Nov 1, 2022

vesajaaskelainen commented Nov 1, 2022

etienne-lms left a comment

Choose a reason for hiding this comment

vesajaaskelainen commented Nov 2, 2022

vesajaaskelainen commented Nov 2, 2022

jforissier commented Nov 2, 2022 • edited

vesajaaskelainen commented Oct 23, 2022 •

edited

xiaoxuZeng Oct 25, 2022 •

edited

vesajaaskelainen commented Oct 25, 2022 •

edited

etienne-lms left a comment •

edited

jforissier commented Nov 2, 2022 •

edited