[PATCH v2 0/2] STM32 Extended TrustZone Protection driver

[PATCH v2 0/2] STM32 Extended TrustZone Protection driver

[Benjamin Gaignard]

On early boot stages STM32MP1 platform is able to dedicate some hardware blocks
to a secure OS running in TrustZone.
We need to avoid using those hardware blocks on non-secure context (i.e. kernel)
because read/write accesses could generate illegale access exceptions.

Extended TrustZone Protection driver make sure that device is disabled if
non-secure world can't acces to it.

version 2:
- do not use notifier anymore
- change status property value in device-tree if needed
- use a list of phandle instead of hard coded array

NOTE: Those patches should be applied only on
git://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next
until this patch: https://lkml.org/lkml/2018/2/26/386
find it way to mainline because KBuild will complain about them.

Benjamin Gaignard (2):
  dt-bindings: stm32: Add bindings for Extended TrustZone Protection
  ARM: mach-stm32: Add Extended TrustZone Protection driver

 .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       |  25 +++++
 arch/arm/mach-stm32/Kconfig                        |   7 ++
 arch/arm/mach-stm32/Makefile                       |   1 +
 arch/arm/mach-stm32/stm32-etzpc.c                  | 116 +++++++++++++++++++++
 4 files changed, 149 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
 create mode 100644 arch/arm/mach-stm32/stm32-etzpc.c

-- 
2.15.0

[PATCH v2 0/2] STM32 Extended TrustZone Protection driver

[Benjamin Gaignard]

On early boot stages STM32MP1 platform is able to dedicate some hardware blocks
to a secure OS running in TrustZone.
We need to avoid using those hardware blocks on non-secure context (i.e. kernel)
because read/write accesses could generate illegale access exceptions.

Extended TrustZone Protection driver make sure that device is disabled if
non-secure world can't acces to it.

version 2:
- do not use notifier anymore
- change status property value in device-tree if needed
- use a list of phandle instead of hard coded array

NOTE: Those patches should be applied only on
git://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next
until this patch: https://lkml.org/lkml/2018/2/26/386
find it way to mainline because KBuild will complain about them.

Benjamin Gaignard (2):
  dt-bindings: stm32: Add bindings for Extended TrustZone Protection
  ARM: mach-stm32: Add Extended TrustZone Protection driver

 .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       |  25 +++++
 arch/arm/mach-stm32/Kconfig                        |   7 ++
 arch/arm/mach-stm32/Makefile                       |   1 +
 arch/arm/mach-stm32/stm32-etzpc.c                  | 116 +++++++++++++++++++++
 4 files changed, 149 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
 create mode 100644 arch/arm/mach-stm32/stm32-etzpc.c

-- 
2.15.0

[PATCH v2 1/2] dt-bindings: stm32: Add bindings for Extended TrustZone Protection

[Benjamin Gaignard]

Extended TrustZone Protection driver is very basic and only needs
to know where are the registers (no clock, no interrupt)

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt

diff --git a/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt b/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
new file mode 100644
index 000000000000..9407e37f7d15
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
@@ -0,0 +1,25 @@
+STMicroelectronics STM32 Extended TrustZone Protection driver
+
+Required properties:
+ - compatible : value should be "st,stm32mp1-etzpc"
+ - reg : physical base address of the IP registers and length of memory
+	 mapped region.
+ - protected-devices: list of phandle of devices protected by etzpc.
+		      Because etzpc driver rely on the phandle index in
+		      the list, holes must be filled with a disabled node.
+
+Example for stm32mp1:
+
+reserved: disabled_node {
+	status = "disabled";
+};
+
+etzpc: etzpc@5c007000 {
+	compatible = "st,stm32mp1-etzpc";
+	reg = <0x5c007000 0x400>;
+		protected-devices = <&usart1>,
+				    <&spi6>,
+				    <&i2c4>,
+				    <&reserved>,
+				    <&rng1>;
+};
-- 
2.15.0

[PATCH v2 1/2] dt-bindings: stm32: Add bindings for Extended TrustZone Protection

[Benjamin Gaignard]

Extended TrustZone Protection driver is very basic and only needs
to know where are the registers (no clock, no interrupt)

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt

diff --git a/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt b/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
new file mode 100644
index 000000000000..9407e37f7d15
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
@@ -0,0 +1,25 @@
+STMicroelectronics STM32 Extended TrustZone Protection driver
+
+Required properties:
+ - compatible : value should be "st,stm32mp1-etzpc"
+ - reg : physical base address of the IP registers and length of memory
+	 mapped region.
+ - protected-devices: list of phandle of devices protected by etzpc.
+		      Because etzpc driver rely on the phandle index in
+		      the list, holes must be filled with a disabled node.
+
+Example for stm32mp1:
+
+reserved: disabled_node {
+	status = "disabled";
+};
+
+etzpc: etzpc at 5c007000 {
+	compatible = "st,stm32mp1-etzpc";
+	reg = <0x5c007000 0x400>;
+		protected-devices = <&usart1>,
+				    <&spi6>,
+				    <&i2c4>,
+				    <&reserved>,
+				    <&rng1>;
+};
-- 
2.15.0

[PATCH v2 2/2] ARM: mach-stm32: Add Extended TrustZone Protection driver

[Benjamin Gaignard]

Extended TrustZone Protection (ETZPC) driver checks that
the hardware block is accessible to non-secure world.
If not it will disable the device tree node by updated it
status property.

Split between secure and non-secure hardware blocks is
done at early boot stage so the driver only needs to read
the status (2 bits) for each of the block.

Hardware blocks status bits location in the registers is
computed from the index of the device phandle in the list.

To avoid to bind a device which will not be accessible ETZPC driver
must be probed early, at least before platform driver, so just after
core initialisation.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 arch/arm/mach-stm32/Kconfig       |   7 +++
 arch/arm/mach-stm32/Makefile      |   1 +
 arch/arm/mach-stm32/stm32-etzpc.c | 116 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+)
 create mode 100644 arch/arm/mach-stm32/stm32-etzpc.c

diff --git a/arch/arm/mach-stm32/Kconfig b/arch/arm/mach-stm32/Kconfig
index 5bc7f5ab61cd..a3ef308642be 100644
--- a/arch/arm/mach-stm32/Kconfig
+++ b/arch/arm/mach-stm32/Kconfig
@@ -44,6 +44,13 @@ config MACH_STM32MP157
 	bool "STMicroelectronics STM32MP157"
 	default y
 
+config STM32_ETZPC
+	bool "STM32 Extended TrustZone Protection"
+	depends on MACH_STM32MP157
+	help
+	  Select y to enable STM32 Extended TrustZone Protection
+	  Controller (ETZPC)
+
 endif # ARMv7-A
 
 endif
diff --git a/arch/arm/mach-stm32/Makefile b/arch/arm/mach-stm32/Makefile
index bd0b7b5d6e9d..2e1e729a68c9 100644
--- a/arch/arm/mach-stm32/Makefile
+++ b/arch/arm/mach-stm32/Makefile
@@ -1 +1,2 @@
 obj-y += board-dt.o
+obj-$(CONFIG_STM32_ETZPC) += stm32-etzpc.o
diff --git a/arch/arm/mach-stm32/stm32-etzpc.c b/arch/arm/mach-stm32/stm32-etzpc.c
new file mode 100644
index 000000000000..ea966b7d519a
--- /dev/null
+++ b/arch/arm/mach-stm32/stm32-etzpc.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) STMicroelectronics 2018 - All Rights Reserved
+ * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
+ */
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define ETZPC_DECPROT0		0x10
+#define ETZPC_IP_VER		0x3F4
+
+#define IP_VER_MP1		0x00000020
+
+#define DECPROT_MASK		0x03
+#define NB_PROT_PER_REG		0x10
+#define DECPROT_NB_BITS		2
+
+static void __init stm32_etzpc_update_status(struct device_node *np)
+{
+	struct property *prop;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop)
+		return;
+
+	prop->name = "status";
+	prop->value = "disabled";
+	prop->length = strlen((char *)prop->value)+1;
+
+	of_update_property(np, prop);
+
+	pr_err("%s status doesn't match ETZPC status\n", of_node_full_name(np));
+}
+
+static bool __init stm32_etzpc_is_secured(void __iomem *base, int index)
+{
+	u32 status;
+	int offset = (index / NB_PROT_PER_REG) * sizeof(u32);
+	int shift = (index % NB_PROT_PER_REG) * DECPROT_NB_BITS;
+
+	status = readl(base + ETZPC_DECPROT0 + offset);
+	status &= DECPROT_MASK << shift;
+
+	return (status != DECPROT_MASK << shift);
+}
+
+static const struct of_device_id stm32_etzpc_of_match[] = {
+	{
+		.compatible = "st,stm32mp1-etzpc",
+	},
+	{ /* end node */ },
+};
+MODULE_DEVICE_TABLE(of, stm32_etzpc_of_match);
+
+static int __init stm32_etzpc_probe(struct device_node *np,
+			     const struct of_device_id *match)
+{
+	struct of_phandle_iterator it;
+	void __iomem *base;
+	int version, index = 0, ret = 0;
+
+	base = of_iomap(np, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	version = readl(base + ETZPC_IP_VER);
+	if (version != IP_VER_MP1) {
+		pr_err("Wrong ETZPC version\n");
+		ret = -EINVAL;
+		goto failed;
+	}
+
+	of_for_each_phandle(&it, ret, np, "protected-devices", NULL, 0) {
+		if (of_device_is_available(it.node) &&
+		    stm32_etzpc_is_secured(base, index))
+			stm32_etzpc_update_status(it.node);
+
+		index++;
+	}
+
+failed:
+	iounmap(base);
+	return ret;
+}
+
+/*
+ * stm32_etzpc_init need to be called before starting to probe
+ * platform drivers to be able check the status of each protected devices
+ * that's why it is tagged as postcore_initcall
+ */
+static int __init stm32_etzpc_init(void)
+{
+	struct device_node *np;
+	const struct of_device_id *m;
+	int ret;
+
+	np = of_find_matching_node_and_match(NULL, stm32_etzpc_of_match, &m);
+
+	if (!np)
+		return -ENODEV;
+
+	if (!of_device_is_available(np)) {
+		of_node_put(np);
+		return -ENODEV;
+	}
+
+	ret = stm32_etzpc_probe(np, m);
+
+	of_node_put(np);
+
+	return ret;
+}
+postcore_initcall(stm32_etzpc_init);
-- 
2.15.0

[PATCH v2 2/2] ARM: mach-stm32: Add Extended TrustZone Protection driver

[Benjamin Gaignard]

Extended TrustZone Protection (ETZPC) driver checks that
the hardware block is accessible to non-secure world.
If not it will disable the device tree node by updated it
status property.

Split between secure and non-secure hardware blocks is
done at early boot stage so the driver only needs to read
the status (2 bits) for each of the block.

Hardware blocks status bits location in the registers is
computed from the index of the device phandle in the list.

To avoid to bind a device which will not be accessible ETZPC driver
must be probed early, at least before platform driver, so just after
core initialisation.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 arch/arm/mach-stm32/Kconfig       |   7 +++
 arch/arm/mach-stm32/Makefile      |   1 +
 arch/arm/mach-stm32/stm32-etzpc.c | 116 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+)
 create mode 100644 arch/arm/mach-stm32/stm32-etzpc.c

diff --git a/arch/arm/mach-stm32/Kconfig b/arch/arm/mach-stm32/Kconfig
index 5bc7f5ab61cd..a3ef308642be 100644
--- a/arch/arm/mach-stm32/Kconfig
+++ b/arch/arm/mach-stm32/Kconfig
@@ -44,6 +44,13 @@ config MACH_STM32MP157
 	bool "STMicroelectronics STM32MP157"
 	default y
 
+config STM32_ETZPC
+	bool "STM32 Extended TrustZone Protection"
+	depends on MACH_STM32MP157
+	help
+	  Select y to enable STM32 Extended TrustZone Protection
+	  Controller (ETZPC)
+
 endif # ARMv7-A
 
 endif
diff --git a/arch/arm/mach-stm32/Makefile b/arch/arm/mach-stm32/Makefile
index bd0b7b5d6e9d..2e1e729a68c9 100644
--- a/arch/arm/mach-stm32/Makefile
+++ b/arch/arm/mach-stm32/Makefile
@@ -1 +1,2 @@
 obj-y += board-dt.o
+obj-$(CONFIG_STM32_ETZPC) += stm32-etzpc.o
diff --git a/arch/arm/mach-stm32/stm32-etzpc.c b/arch/arm/mach-stm32/stm32-etzpc.c
new file mode 100644
index 000000000000..ea966b7d519a
--- /dev/null
+++ b/arch/arm/mach-stm32/stm32-etzpc.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) STMicroelectronics 2018 - All Rights Reserved
+ * Author: Benjamin Gaignard <benjamin.gaignard@st.com> for STMicroelectronics.
+ */
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define ETZPC_DECPROT0		0x10
+#define ETZPC_IP_VER		0x3F4
+
+#define IP_VER_MP1		0x00000020
+
+#define DECPROT_MASK		0x03
+#define NB_PROT_PER_REG		0x10
+#define DECPROT_NB_BITS		2
+
+static void __init stm32_etzpc_update_status(struct device_node *np)
+{
+	struct property *prop;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop)
+		return;
+
+	prop->name = "status";
+	prop->value = "disabled";
+	prop->length = strlen((char *)prop->value)+1;
+
+	of_update_property(np, prop);
+
+	pr_err("%s status doesn't match ETZPC status\n", of_node_full_name(np));
+}
+
+static bool __init stm32_etzpc_is_secured(void __iomem *base, int index)
+{
+	u32 status;
+	int offset = (index / NB_PROT_PER_REG) * sizeof(u32);
+	int shift = (index % NB_PROT_PER_REG) * DECPROT_NB_BITS;
+
+	status = readl(base + ETZPC_DECPROT0 + offset);
+	status &= DECPROT_MASK << shift;
+
+	return (status != DECPROT_MASK << shift);
+}
+
+static const struct of_device_id stm32_etzpc_of_match[] = {
+	{
+		.compatible = "st,stm32mp1-etzpc",
+	},
+	{ /* end node */ },
+};
+MODULE_DEVICE_TABLE(of, stm32_etzpc_of_match);
+
+static int __init stm32_etzpc_probe(struct device_node *np,
+			     const struct of_device_id *match)
+{
+	struct of_phandle_iterator it;
+	void __iomem *base;
+	int version, index = 0, ret = 0;
+
+	base = of_iomap(np, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	version = readl(base + ETZPC_IP_VER);
+	if (version != IP_VER_MP1) {
+		pr_err("Wrong ETZPC version\n");
+		ret = -EINVAL;
+		goto failed;
+	}
+
+	of_for_each_phandle(&it, ret, np, "protected-devices", NULL, 0) {
+		if (of_device_is_available(it.node) &&
+		    stm32_etzpc_is_secured(base, index))
+			stm32_etzpc_update_status(it.node);
+
+		index++;
+	}
+
+failed:
+	iounmap(base);
+	return ret;
+}
+
+/*
+ * stm32_etzpc_init need to be called before starting to probe
+ * platform drivers to be able check the status of each protected devices
+ * that's why it is tagged as postcore_initcall
+ */
+static int __init stm32_etzpc_init(void)
+{
+	struct device_node *np;
+	const struct of_device_id *m;
+	int ret;
+
+	np = of_find_matching_node_and_match(NULL, stm32_etzpc_of_match, &m);
+
+	if (!np)
+		return -ENODEV;
+
+	if (!of_device_is_available(np)) {
+		of_node_put(np);
+		return -ENODEV;
+	}
+
+	ret = stm32_etzpc_probe(np, m);
+
+	of_node_put(np);
+
+	return ret;
+}
+postcore_initcall(stm32_etzpc_init);
-- 
2.15.0

Re: [PATCH v2 0/2] STM32 Extended TrustZone Protection driver

[Mark Rutland]

On Thu, Mar 01, 2018 at 02:58:04PM +0100, Benjamin Gaignard wrote:
> On early boot stages STM32MP1 platform is able to dedicate some hardware blocks
> to a secure OS running in TrustZone.
> We need to avoid using those hardware blocks on non-secure context (i.e. kernel)
> because read/write accesses could generate illegale access exceptions.
> 
> Extended TrustZone Protection driver make sure that device is disabled if
> non-secure world can't acces to it.
> 
> version 2:
> - do not use notifier anymore
> - change status property value in device-tree if needed
> - use a list of phandle instead of hard coded array

As mentioned on v1, I don't think this should be done in Linux at all.

If you wish to handle this dynamically, please fixup the DT *before*
entering Linux.

If you want a sane default in the dts file, put status = "disabled" on
all nodes which the secure world might take ownership of.

Thanks,
Mark.

> NOTE: Those patches should be applied only on
> git://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next
> until this patch: https://lkml.org/lkml/2018/2/26/386
> find it way to mainline because KBuild will complain about them.
> 
> Benjamin Gaignard (2):
>   dt-bindings: stm32: Add bindings for Extended TrustZone Protection
>   ARM: mach-stm32: Add Extended TrustZone Protection driver
> 
>  .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       |  25 +++++
>  arch/arm/mach-stm32/Kconfig                        |   7 ++
>  arch/arm/mach-stm32/Makefile                       |   1 +
>  arch/arm/mach-stm32/stm32-etzpc.c                  | 116 +++++++++++++++++++++
>  4 files changed, 149 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
>  create mode 100644 arch/arm/mach-stm32/stm32-etzpc.c
> 
> -- 
> 2.15.0
> 

[PATCH v2 0/2] STM32 Extended TrustZone Protection driver

[Mark Rutland]

On Thu, Mar 01, 2018 at 02:58:04PM +0100, Benjamin Gaignard wrote:
> On early boot stages STM32MP1 platform is able to dedicate some hardware blocks
> to a secure OS running in TrustZone.
> We need to avoid using those hardware blocks on non-secure context (i.e. kernel)
> because read/write accesses could generate illegale access exceptions.
> 
> Extended TrustZone Protection driver make sure that device is disabled if
> non-secure world can't acces to it.
> 
> version 2:
> - do not use notifier anymore
> - change status property value in device-tree if needed
> - use a list of phandle instead of hard coded array

As mentioned on v1, I don't think this should be done in Linux at all.

If you wish to handle this dynamically, please fixup the DT *before*
entering Linux.

If you want a sane default in the dts file, put status = "disabled" on
all nodes which the secure world might take ownership of.

Thanks,
Mark.

> NOTE: Those patches should be applied only on
> git://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next
> until this patch: https://lkml.org/lkml/2018/2/26/386
> find it way to mainline because KBuild will complain about them.
> 
> Benjamin Gaignard (2):
>   dt-bindings: stm32: Add bindings for Extended TrustZone Protection
>   ARM: mach-stm32: Add Extended TrustZone Protection driver
> 
>  .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       |  25 +++++
>  arch/arm/mach-stm32/Kconfig                        |   7 ++
>  arch/arm/mach-stm32/Makefile                       |   1 +
>  arch/arm/mach-stm32/stm32-etzpc.c                  | 116 +++++++++++++++++++++
>  4 files changed, 149 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
>  create mode 100644 arch/arm/mach-stm32/stm32-etzpc.c
> 
> -- 
> 2.15.0
> 

Re: [PATCH v2 1/2] dt-bindings: stm32: Add bindings for Extended TrustZone Protection

[Mark Rutland]

On Thu, Mar 01, 2018 at 02:58:05PM +0100, Benjamin Gaignard wrote:
> Extended TrustZone Protection driver is very basic and only needs
> to know where are the registers (no clock, no interrupt)
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt b/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
> new file mode 100644
> index 000000000000..9407e37f7d15
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
> @@ -0,0 +1,25 @@
> +STMicroelectronics STM32 Extended TrustZone Protection driver
> +
> +Required properties:
> + - compatible : value should be "st,stm32mp1-etzpc"
> + - reg : physical base address of the IP registers and length of memory
> +	 mapped region.
> + - protected-devices: list of phandle of devices protected by etzpc.
> +		      Because etzpc driver rely on the phandle index in
> +		      the list, holes must be filled with a disabled node.

... where the index corresponds to what, exactly?

Padding with a disabled node seems very hacky.

Thanks,
Mark.

> +
> +Example for stm32mp1:
> +
> +reserved: disabled_node {
> +	status = "disabled";
> +};
> +
> +etzpc: etzpc@5c007000 {
> +	compatible = "st,stm32mp1-etzpc";
> +	reg = <0x5c007000 0x400>;
> +		protected-devices = <&usart1>,
> +				    <&spi6>,
> +				    <&i2c4>,
> +				    <&reserved>,
> +				    <&rng1>;
> +};
> -- 
> 2.15.0
> 

[PATCH v2 1/2] dt-bindings: stm32: Add bindings for Extended TrustZone Protection

[Mark Rutland]

On Thu, Mar 01, 2018 at 02:58:05PM +0100, Benjamin Gaignard wrote:
> Extended TrustZone Protection driver is very basic and only needs
> to know where are the registers (no clock, no interrupt)
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt b/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
> new file mode 100644
> index 000000000000..9407e37f7d15
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
> @@ -0,0 +1,25 @@
> +STMicroelectronics STM32 Extended TrustZone Protection driver
> +
> +Required properties:
> + - compatible : value should be "st,stm32mp1-etzpc"
> + - reg : physical base address of the IP registers and length of memory
> +	 mapped region.
> + - protected-devices: list of phandle of devices protected by etzpc.
> +		      Because etzpc driver rely on the phandle index in
> +		      the list, holes must be filled with a disabled node.

... where the index corresponds to what, exactly?

Padding with a disabled node seems very hacky.

Thanks,
Mark.

> +
> +Example for stm32mp1:
> +
> +reserved: disabled_node {
> +	status = "disabled";
> +};
> +
> +etzpc: etzpc at 5c007000 {
> +	compatible = "st,stm32mp1-etzpc";
> +	reg = <0x5c007000 0x400>;
> +		protected-devices = <&usart1>,
> +				    <&spi6>,
> +				    <&i2c4>,
> +				    <&reserved>,
> +				    <&rng1>;
> +};
> -- 
> 2.15.0
> 

Re: [PATCH v2 1/2] dt-bindings: stm32: Add bindings for Extended TrustZone Protection

[Benjamin Gaignard]

2018-03-01 15:03 GMT+01:00 Mark Rutland <mark.rutland@arm.com>:
> On Thu, Mar 01, 2018 at 02:58:05PM +0100, Benjamin Gaignard wrote:
>> Extended TrustZone Protection driver is very basic and only needs
>> to know where are the registers (no clock, no interrupt)
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>>  .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       | 25 ++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt b/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
>> new file mode 100644
>> index 000000000000..9407e37f7d15
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
>> @@ -0,0 +1,25 @@
>> +STMicroelectronics STM32 Extended TrustZone Protection driver
>> +
>> +Required properties:
>> + - compatible : value should be "st,stm32mp1-etzpc"
>> + - reg : physical base address of the IP registers and length of memory
>> +      mapped region.
>> + - protected-devices: list of phandle of devices protected by etzpc.
>> +                   Because etzpc driver rely on the phandle index in
>> +                   the list, holes must be filled with a disabled node.
>
> ... where the index corresponds to what, exactly?

to the offset of the status bits in the register

>
> Padding with a disabled node seems very hacky.

If a device node doesn't exist in the DT I need to fill the hole by something
to keep the index valid.

>
> Thanks,
> Mark.
>
>> +
>> +Example for stm32mp1:
>> +
>> +reserved: disabled_node {
>> +     status = "disabled";
>> +};
>> +
>> +etzpc: etzpc@5c007000 {
>> +     compatible = "st,stm32mp1-etzpc";
>> +     reg = <0x5c007000 0x400>;
>> +             protected-devices = <&usart1>,
>> +                                 <&spi6>,
>> +                                 <&i2c4>,
>> +                                 <&reserved>,
>> +                                 <&rng1>;
>> +};
>> --
>> 2.15.0
>>

[PATCH v2 1/2] dt-bindings: stm32: Add bindings for Extended TrustZone Protection

[Benjamin Gaignard]

2018-03-01 15:03 GMT+01:00 Mark Rutland <mark.rutland@arm.com>:
> On Thu, Mar 01, 2018 at 02:58:05PM +0100, Benjamin Gaignard wrote:
>> Extended TrustZone Protection driver is very basic and only needs
>> to know where are the registers (no clock, no interrupt)
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>>  .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       | 25 ++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt b/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
>> new file mode 100644
>> index 000000000000..9407e37f7d15
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
>> @@ -0,0 +1,25 @@
>> +STMicroelectronics STM32 Extended TrustZone Protection driver
>> +
>> +Required properties:
>> + - compatible : value should be "st,stm32mp1-etzpc"
>> + - reg : physical base address of the IP registers and length of memory
>> +      mapped region.
>> + - protected-devices: list of phandle of devices protected by etzpc.
>> +                   Because etzpc driver rely on the phandle index in
>> +                   the list, holes must be filled with a disabled node.
>
> ... where the index corresponds to what, exactly?

to the offset of the status bits in the register

>
> Padding with a disabled node seems very hacky.

If a device node doesn't exist in the DT I need to fill the hole by something
to keep the index valid.

>
> Thanks,
> Mark.
>
>> +
>> +Example for stm32mp1:
>> +
>> +reserved: disabled_node {
>> +     status = "disabled";
>> +};
>> +
>> +etzpc: etzpc at 5c007000 {
>> +     compatible = "st,stm32mp1-etzpc";
>> +     reg = <0x5c007000 0x400>;
>> +             protected-devices = <&usart1>,
>> +                                 <&spi6>,
>> +                                 <&i2c4>,
>> +                                 <&reserved>,
>> +                                 <&rng1>;
>> +};
>> --
>> 2.15.0
>>

Re: [PATCH v2 0/2] STM32 Extended TrustZone Protection driver

[Benjamin Gaignard]

2018-03-01 15:02 GMT+01:00 Mark Rutland <mark.rutland@arm.com>:
> On Thu, Mar 01, 2018 at 02:58:04PM +0100, Benjamin Gaignard wrote:
>> On early boot stages STM32MP1 platform is able to dedicate some hardware blocks
>> to a secure OS running in TrustZone.
>> We need to avoid using those hardware blocks on non-secure context (i.e. kernel)
>> because read/write accesses could generate illegale access exceptions.
>>
>> Extended TrustZone Protection driver make sure that device is disabled if
>> non-secure world can't acces to it.
>>
>> version 2:
>> - do not use notifier anymore
>> - change status property value in device-tree if needed
>> - use a list of phandle instead of hard coded array
>
> As mentioned on v1, I don't think this should be done in Linux at all.
>
> If you wish to handle this dynamically, please fixup the DT *before*
> entering Linux.
>
> If you want a sane default in the dts file, put status = "disabled" on
> all nodes which the secure world might take ownership of.

That is the case, nodes are disabled by ealier boot stages before entering
in Linux but, since mistakes and/or errors are always possible, fixup the DT
to avoid illegal access exceptions make sense for me.

Benjamin

>
> Thanks,
> Mark.
>
>> NOTE: Those patches should be applied only on
>> git://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next
>> until this patch: https://lkml.org/lkml/2018/2/26/386
>> find it way to mainline because KBuild will complain about them.
>>
>> Benjamin Gaignard (2):
>>   dt-bindings: stm32: Add bindings for Extended TrustZone Protection
>>   ARM: mach-stm32: Add Extended TrustZone Protection driver
>>
>>  .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       |  25 +++++
>>  arch/arm/mach-stm32/Kconfig                        |   7 ++
>>  arch/arm/mach-stm32/Makefile                       |   1 +
>>  arch/arm/mach-stm32/stm32-etzpc.c                  | 116 +++++++++++++++++++++
>>  4 files changed, 149 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
>>  create mode 100644 arch/arm/mach-stm32/stm32-etzpc.c
>>
>> --
>> 2.15.0
>>

[PATCH v2 0/2] STM32 Extended TrustZone Protection driver

[Benjamin Gaignard]

2018-03-01 15:02 GMT+01:00 Mark Rutland <mark.rutland@arm.com>:
> On Thu, Mar 01, 2018 at 02:58:04PM +0100, Benjamin Gaignard wrote:
>> On early boot stages STM32MP1 platform is able to dedicate some hardware blocks
>> to a secure OS running in TrustZone.
>> We need to avoid using those hardware blocks on non-secure context (i.e. kernel)
>> because read/write accesses could generate illegale access exceptions.
>>
>> Extended TrustZone Protection driver make sure that device is disabled if
>> non-secure world can't acces to it.
>>
>> version 2:
>> - do not use notifier anymore
>> - change status property value in device-tree if needed
>> - use a list of phandle instead of hard coded array
>
> As mentioned on v1, I don't think this should be done in Linux at all.
>
> If you wish to handle this dynamically, please fixup the DT *before*
> entering Linux.
>
> If you want a sane default in the dts file, put status = "disabled" on
> all nodes which the secure world might take ownership of.

That is the case, nodes are disabled by ealier boot stages before entering
in Linux but, since mistakes and/or errors are always possible, fixup the DT
to avoid illegal access exceptions make sense for me.

Benjamin

>
> Thanks,
> Mark.
>
>> NOTE: Those patches should be applied only on
>> git://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next
>> until this patch: https://lkml.org/lkml/2018/2/26/386
>> find it way to mainline because KBuild will complain about them.
>>
>> Benjamin Gaignard (2):
>>   dt-bindings: stm32: Add bindings for Extended TrustZone Protection
>>   ARM: mach-stm32: Add Extended TrustZone Protection driver
>>
>>  .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       |  25 +++++
>>  arch/arm/mach-stm32/Kconfig                        |   7 ++
>>  arch/arm/mach-stm32/Makefile                       |   1 +
>>  arch/arm/mach-stm32/stm32-etzpc.c                  | 116 +++++++++++++++++++++
>>  4 files changed, 149 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
>>  create mode 100644 arch/arm/mach-stm32/stm32-etzpc.c
>>
>> --
>> 2.15.0
>>

Re: [PATCH v2 0/2] STM32 Extended TrustZone Protection driver

[Robin Murphy]

On 01/03/18 14:15, Benjamin Gaignard wrote:
> 2018-03-01 15:02 GMT+01:00 Mark Rutland <mark.rutland@arm.com>:
>> On Thu, Mar 01, 2018 at 02:58:04PM +0100, Benjamin Gaignard wrote:
>>> On early boot stages STM32MP1 platform is able to dedicate some hardware blocks
>>> to a secure OS running in TrustZone.
>>> We need to avoid using those hardware blocks on non-secure context (i.e. kernel)
>>> because read/write accesses could generate illegale access exceptions.
>>>
>>> Extended TrustZone Protection driver make sure that device is disabled if
>>> non-secure world can't acces to it.
>>>
>>> version 2:
>>> - do not use notifier anymore
>>> - change status property value in device-tree if needed
>>> - use a list of phandle instead of hard coded array
>>
>> As mentioned on v1, I don't think this should be done in Linux at all.
>>
>> If you wish to handle this dynamically, please fixup the DT *before*
>> entering Linux.
>>
>> If you want a sane default in the dts file, put status = "disabled" on
>> all nodes which the secure world might take ownership of.
> 
> That is the case, nodes are disabled by ealier boot stages before entering
> in Linux but, since mistakes and/or errors are always possible, fixup the DT
> to avoid illegal access exceptions make sense for me.

So why not also run a test on the memory controller in case the 
bootloader made a mistake in the memory node too? As I mentioned before, 
if you can't trust the DT to describe your hardware correctly you've 
already lost.

Robin.

[PATCH v2 0/2] STM32 Extended TrustZone Protection driver

[Robin Murphy]

On 01/03/18 14:15, Benjamin Gaignard wrote:
> 2018-03-01 15:02 GMT+01:00 Mark Rutland <mark.rutland@arm.com>:
>> On Thu, Mar 01, 2018 at 02:58:04PM +0100, Benjamin Gaignard wrote:
>>> On early boot stages STM32MP1 platform is able to dedicate some hardware blocks
>>> to a secure OS running in TrustZone.
>>> We need to avoid using those hardware blocks on non-secure context (i.e. kernel)
>>> because read/write accesses could generate illegale access exceptions.
>>>
>>> Extended TrustZone Protection driver make sure that device is disabled if
>>> non-secure world can't acces to it.
>>>
>>> version 2:
>>> - do not use notifier anymore
>>> - change status property value in device-tree if needed
>>> - use a list of phandle instead of hard coded array
>>
>> As mentioned on v1, I don't think this should be done in Linux at all.
>>
>> If you wish to handle this dynamically, please fixup the DT *before*
>> entering Linux.
>>
>> If you want a sane default in the dts file, put status = "disabled" on
>> all nodes which the secure world might take ownership of.
> 
> That is the case, nodes are disabled by ealier boot stages before entering
> in Linux but, since mistakes and/or errors are always possible, fixup the DT
> to avoid illegal access exceptions make sense for me.

So why not also run a test on the memory controller in case the 
bootloader made a mistake in the memory node too? As I mentioned before, 
if you can't trust the DT to describe your hardware correctly you've 
already lost.

Robin.