[PATCH 0/3] STM32 Extended TrustZone Protection driver

[PATCH 0/3] 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 access will all be discarded.

Extended TrustZone Protection driver register itself as listener of
BUS_NOTIFY_BIND_DRIVER and check, given the device address, if the hardware block
could be used in a Linux context. If not it returns NOTIFY_BAD to driver core
to stop driver probing.

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

Benjamin Gaignard (3):
  driver core: check notifier_call_chain return value
  dt-bindings: stm32: Add bindings for Extended TrustZone Protection
  ARM: mach-stm32: Add Extended TrustZone Protection driver

 .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       |  13 ++
 arch/arm/mach-stm32/Kconfig                        |   7 +
 arch/arm/mach-stm32/Makefile                       |   1 +
 arch/arm/mach-stm32/stm32-etzpc.c                  | 252 +++++++++++++++++++++
 drivers/base/dd.c                                  |   9 +-
 5 files changed, 279 insertions(+), 3 deletions(-)
 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 1/3] driver core: check notifier_call_chain return value

[Benjamin Gaignard]

When being notified that a driver is about to be bind a listener
could return NOTIFY_BAD.
Check the return to be sure that the driver could be bind.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 drivers/base/dd.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index de6fd092bf2f..9275f2c0fed2 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -304,9 +304,12 @@ static int driver_sysfs_add(struct device *dev)
 {
 	int ret;
 
-	if (dev->bus)
-		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
-					     BUS_NOTIFY_BIND_DRIVER, dev);
+	if (dev->bus) {
+		if (blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
+						 BUS_NOTIFY_BIND_DRIVER, dev) ==
+						 NOTIFY_BAD)
+			return -EINVAL;
+	}
 
 	ret = sysfs_create_link(&dev->driver->p->kobj, &dev->kobj,
 				kobject_name(&dev->kobj));
-- 
2.15.0

[PATCH 2/3] 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>
---
 .../devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt     | 13 +++++++++++++
 1 file changed, 13 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..6db093847a13
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/stm32/st,stm32mp1-etzpc.txt
@@ -0,0 +1,13 @@
+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.
+
+Example for stm32mp1:
+
+etzpc: etzpc@5c007000 {
+	compatible = "st,stm32mp1-etzpc";
+	reg = <0x5c007000 0x400>;
+};
-- 
2.15.0

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

[Benjamin Gaignard]

Before binding a driver Extended TrustZone Protection (ETZPC) driver
checks that the hardware block is accessible to non-secure world.
Hardware blocks split between secure and non-secure 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 device address index in the array.

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 | 252 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 260 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..b338d219d5a8
--- /dev/null
+++ b/arch/arm/mach-stm32/stm32-etzpc.c
@@ -0,0 +1,252 @@
+// 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/notifier.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
+
+struct stm32_etzpc_cfg {
+	const u32 *addr;
+	const int size;
+	const int version;
+};
+
+struct stm32_etzpc {
+	void __iomem *base;
+	const struct stm32_etzpc_cfg *cfg;
+	struct notifier_block nb;
+};
+
+static inline struct stm32_etzpc *to_stm32_etzpc(struct notifier_block *nb)
+{
+	return container_of(nb, struct stm32_etzpc, nb);
+}
+
+static const u32 stm32mp1_ip_addr[] = {
+	0x5c008000,	/* 00 stgenc */
+	0x54000000,	/* 01 bkpsram */
+	0x5c003000,	/* 02 iwdg1 */
+	0x5c000000,	/* 03 usart1 */
+	0x5c001000,	/* 04 spi6 */
+	0x5c002000,	/* 05 i2c4 */
+	0xffffffff,	/* 06 reserved */
+	0x54003000,	/* 07 rng1 */
+	0x54002000,	/* 08 hash1 */
+	0x54001000,	/* 09 cryp1 */
+	0x5a003000,	/* 0A ddrctrl */
+	0x5a004000,	/* 0B ddrphyc */
+	0x5c009000,	/* 0C i2c6 */
+	0xffffffff,	/* 0D reserved */
+	0xffffffff,	/* 0E reserved */
+	0xffffffff,	/* 0F reserved */
+	0x40000000,	/* 10 tim2 */
+	0x40001000,	/* 11 tim3 */
+	0x40002000,	/* 12 tim4 */
+	0x40003000,	/* 13 tim5 */
+	0x40004000,	/* 14 tim6 */
+	0x40005000,	/* 15 tim7 */
+	0x40006000,	/* 16 tim12 */
+	0x40007000,	/* 17 tim13 */
+	0x40008000,	/* 18 tim14 */
+	0x40009000,	/* 19 lptim1 */
+	0x4000a000,	/* 1A wwdg1 */
+	0x4000b000,	/* 1B spi2 */
+	0x4000c000,	/* 1C spi3 */
+	0x4000d000,	/* 1D spdifrx */
+	0x4000e000,	/* 1E usart2 */
+	0x4000f000,	/* 1F usart3 */
+	0x40010000,	/* 20 uart4 */
+	0x40011000,	/* 21 uart5 */
+	0x40012000,	/* 22 i2c1 */
+	0x40013000,	/* 23 i2c2 */
+	0x40014000,	/* 24 i2c3 */
+	0x40015000,	/* 25 i2c5 */
+	0x40016000,	/* 26 cec */
+	0x40017000,	/* 27 dac */
+	0x40018000,	/* 28 uart7 */
+	0x40019000,	/* 29 uart8 */
+	0xffffffff,	/* 2A reserved */
+	0xffffffff,	/* 2B reserved */
+	0x4001c000,	/* 2C mdios */
+	0xffffffff,	/* 2D reserved */
+	0xffffffff,	/* 2E reserved */
+	0xffffffff,	/* 2F reserved */
+	0x44000000,	/* 30 tim1 */
+	0x44001000,	/* 31 tim8 */
+	0xffffffff,	/* 32 reserved */
+	0x44003000,	/* 33 usart6 */
+	0x44004000,	/* 34 spi1 */
+	0x44005000,	/* 35 spi4 */
+	0x44006000,	/* 36 tim15 */
+	0x44007000,	/* 37 tim16 */
+	0x44008000,	/* 38 tim17 */
+	0x44009000,	/* 39 spi5 */
+	0x4400a000,	/* 3A sai1 */
+	0x4400b000,	/* 3B sai2 */
+	0x4400c000,	/* 3C sai3 */
+	0x4400d000,	/* 3D dfsdm */
+	0x4400e000,	/* 3E tt_fdcan */
+	0xffffffff,	/* 3F reserved */
+	0x50021000,	/* 40 lptim2 */
+	0x50022000,	/* 41 lptim3 */
+	0x50023000,	/* 42 lptim4 */
+	0x50024000,	/* 43 lptim5 */
+	0x50027000,	/* 44 sai4 */
+	0x50025000,	/* 45 vrefbuf */
+	0x4c006000,	/* 46 dcmi */
+	0x4c004000,	/* 47 crc2 */
+	0x48003000,	/* 48 adc */
+	0x4c002000,	/* 49 hash2 */
+	0x4c003000,	/* 4A rng2 */
+	0x4c005000,	/* 4B cryp2 */
+	0xffffffff,	/* 4C reserved */
+	0xffffffff,	/* 4D reserved */
+	0xffffffff,	/* 4E reserved */
+	0xffffffff,	/* 4F reserved */
+	0xffffffff,	/* 50 sram1 */
+	0xffffffff,	/* 51 sram2 */
+	0xffffffff,	/* 52 sram3 */
+	0xffffffff,	/* 53 sram4 */
+	0xffffffff,	/* 54 retram */
+	0x49000000,	/* 55 otg */
+	0x48004000,	/* 56 sdmmc3 */
+	0x48005000,	/* 57 dlybsd3 */
+	0x48000000,	/* 58 dma1 */
+	0x48001000,	/* 59 dma2 */
+	0x48002000,	/* 5A dmamux */
+	0x58002000,	/* 5B fmc */
+	0x58003000,	/* 5C qspi */
+	0x58004000,	/* 5D dlybq */
+	0x5800a000,	/* 5E eth */
+	0xffffffff,	/* 5F reserved */
+};
+
+static const struct stm32_etzpc_cfg stm32_etzpc_mp1_cfg = {
+	.addr = stm32mp1_ip_addr,
+	.size = ARRAY_SIZE(stm32mp1_ip_addr),
+	.version = IP_VER_MP1
+};
+
+static const struct of_device_id stm32_etzpc_of_match[] = {
+	{
+		.compatible = "st,stm32mp1-etzpc",
+		.data = (void *)&stm32_etzpc_mp1_cfg,
+	},
+	{ /* end node */ },
+};
+MODULE_DEVICE_TABLE(of, stm32_etzpc_of_match);
+
+static int stm32_etzpc_notifier_call(struct notifier_block *nb,
+				     unsigned long event, void *data)
+{
+	struct stm32_etzpc *etzpc = to_stm32_etzpc(nb);
+	struct device *dev = data;
+	struct resource res;
+	int i;
+
+	if (event != BUS_NOTIFY_BIND_DRIVER)
+		return NOTIFY_DONE;
+
+	if (of_address_to_resource(dev->of_node, 0, &res))
+		return NOTIFY_DONE;
+
+	for (i = 0; i < etzpc->cfg->size; i++) {
+		if (etzpc->cfg->addr[i] == res.start) {
+			/*
+			 * Each hardware block protection status is defined by
+			 * a 2 bits field and all of them are packed into
+			 * 32 bits registers. Do some computation to get
+			 * register offset and the shift.
+			 */
+			u32 status;
+			int offset = (i / NB_PROT_PER_REG) * sizeof(u32);
+			int shift = (i % NB_PROT_PER_REG) * DECPROT_NB_BITS;
+
+			status = readl(etzpc->base + ETZPC_DECPROT0 + offset);
+			status &= DECPROT_MASK << shift;
+
+			return (status == DECPROT_MASK << shift) ?
+				NOTIFY_DONE : NOTIFY_BAD;
+		}
+	}
+
+	return NOTIFY_DONE;
+}
+
+static int stm32_etzpc_probe(struct device_node *np,
+			     const struct of_device_id *match)
+{
+	struct stm32_etzpc *etzpc;
+	int version, ret;
+
+	etzpc = kzalloc(sizeof(*etzpc), GFP_KERNEL);
+	if (!etzpc)
+		return -ENOMEM;
+
+	etzpc->base = of_iomap(np, 0);
+	if (IS_ERR(etzpc->base)) {
+		ret = PTR_ERR(etzpc->base);
+		goto failed;
+	}
+
+	etzpc->cfg = (const struct stm32_etzpc_cfg *)match->data;
+
+	version = readl(etzpc->base + ETZPC_IP_VER);
+	if (version != etzpc->cfg->version) {
+		pr_err("Wrong ETZPC version\n");
+		ret = -EINVAL;
+		goto failed;
+	}
+
+	etzpc->nb.notifier_call = stm32_etzpc_notifier_call,
+	ret = bus_register_notifier(&platform_bus_type, &etzpc->nb);
+	if (!ret)
+		return 0;
+
+failed:
+	kfree(etzpc);
+	return ret;
+}
+
+/*
+ * stm32_etzpc_init needs to be called before starting to probe
+ * platform drivers to be able to catch all bind notifications
+ * 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 0/3] STM32 Extended TrustZone Protection driver

[Mark Rutland]

On Tue, Feb 27, 2018 at 03:09:23PM +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 access will all be discarded.
> 
> Extended TrustZone Protection driver register itself as listener of
> BUS_NOTIFY_BIND_DRIVER and check, given the device address, if the hardware block
> could be used in a Linux context. If not it returns NOTIFY_BAD to driver core
> to stop driver probing.

Huh?

If these devices are not usable from the non-secure side, why are they
not removed form the DT (or marked disabled)?

In other cases, where resources are carved out for the secure side (e.g.
DRAM carveouts), that's how we handle things.

Mark.

> 
> NOTE: patches 2 and 3 should be applied only on
> git://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next
> but until this patch: https://lkml.org/lkml/2018/2/26/386
> find it way to mailine KBuild will complain about them.
> 
> Benjamin Gaignard (3):
>   driver core: check notifier_call_chain return value
>   dt-bindings: stm32: Add bindings for Extended TrustZone Protection
>   ARM: mach-stm32: Add Extended TrustZone Protection driver
> 
>  .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       |  13 ++
>  arch/arm/mach-stm32/Kconfig                        |   7 +
>  arch/arm/mach-stm32/Makefile                       |   1 +
>  arch/arm/mach-stm32/stm32-etzpc.c                  | 252 +++++++++++++++++++++
>  drivers/base/dd.c                                  |   9 +-
>  5 files changed, 279 insertions(+), 3 deletions(-)
>  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 3/3] ARM: mach-stm32: Add Extended TrustZone Protection driver

[Mark Rutland]

On Tue, Feb 27, 2018 at 03:09:26PM +0100, Benjamin Gaignard wrote:
 +
> +static const u32 stm32mp1_ip_addr[] = {
> +	0x5c008000,	/* 00 stgenc */
> +	0x54000000,	/* 01 bkpsram */
> +	0x5c003000,	/* 02 iwdg1 */
> +	0x5c000000,	/* 03 usart1 */
> +	0x5c001000,	/* 04 spi6 */
> +	0x5c002000,	/* 05 i2c4 */

...

This duplicates information that is in the DT, which is unfortunate.

Why can these not be marked disabled inthe DT instead? 

If it's dynamic form boot-to-boot, then the FW can probe this prior to
entering Linux, and patch the DT appropriately.

Thanks,
Mark.

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

[Benjamin Gaignard]

2018-02-27 18:11 GMT+01:00 Mark Rutland <mark.rutland@arm.com>:
> On Tue, Feb 27, 2018 at 03:09:23PM +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 access will all be discarded.
>>
>> Extended TrustZone Protection driver register itself as listener of
>> BUS_NOTIFY_BIND_DRIVER and check, given the device address, if the hardware block
>> could be used in a Linux context. If not it returns NOTIFY_BAD to driver core
>> to stop driver probing.
>
> Huh?
>
> If these devices are not usable from the non-secure side, why are they
> not removed form the DT (or marked disabled)?
>
> In other cases, where resources are carved out for the secure side (e.g.
> DRAM carveouts), that's how we handle things.
>

That true you can parse and disable a device a boot time but if DT doesn't
exactly reflect etzpc status bits we will in trouble when try to get access to
the device.
Changing the DT is a software protection while etzpc is an hardware protection
so we need to check it anyway.

Benjamin


> Mark.
>
>>
>> NOTE: patches 2 and 3 should be applied only on
>> git://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next
>> but until this patch: https://lkml.org/lkml/2018/2/26/386
>> find it way to mailine KBuild will complain about them.
>>
>> Benjamin Gaignard (3):
>>   driver core: check notifier_call_chain return value
>>   dt-bindings: stm32: Add bindings for Extended TrustZone Protection
>>   ARM: mach-stm32: Add Extended TrustZone Protection driver
>>
>>  .../bindings/arm/stm32/st,stm32mp1-etzpc.txt       |  13 ++
>>  arch/arm/mach-stm32/Kconfig                        |   7 +
>>  arch/arm/mach-stm32/Makefile                       |   1 +
>>  arch/arm/mach-stm32/stm32-etzpc.c                  | 252 +++++++++++++++++++++
>>  drivers/base/dd.c                                  |   9 +-
>>  5 files changed, 279 insertions(+), 3 deletions(-)
>>  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 3/3] ARM: mach-stm32: Add Extended TrustZone Protection driver

[Benjamin Gaignard]

2018-02-27 18:14 GMT+01:00 Mark Rutland <mark.rutland@arm.com>:
> On Tue, Feb 27, 2018 at 03:09:26PM +0100, Benjamin Gaignard wrote:
>  +
>> +static const u32 stm32mp1_ip_addr[] = {
>> +     0x5c008000,     /* 00 stgenc */
>> +     0x54000000,     /* 01 bkpsram */
>> +     0x5c003000,     /* 02 iwdg1 */
>> +     0x5c000000,     /* 03 usart1 */
>> +     0x5c001000,     /* 04 spi6 */
>> +     0x5c002000,     /* 05 i2c4 */
>
> ...
>
> This duplicates information that is in the DT, which is unfortunate.

Yes I would have prefer to be able to get this information from etzpc
hardware itself
but that isn't the case so I need this table to found the status bits
of each device.

>
> Why can these not be marked disabled inthe DT instead?
>
> If it's dynamic form boot-to-boot, then the FW can probe this prior to
> entering Linux, and patch the DT appropriately.

I know that is one software way to do, but let discusted about that in
cover letter thread.

Thanks,
Benjamin

>
> Thanks,
> Mark.

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

[Robin Murphy]

On 27/02/18 19:16, Benjamin Gaignard wrote:
> 2018-02-27 18:11 GMT+01:00 Mark Rutland <mark.rutland@arm.com>:
>> On Tue, Feb 27, 2018 at 03:09:23PM +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 access will all be discarded.
>>>
>>> Extended TrustZone Protection driver register itself as listener of
>>> BUS_NOTIFY_BIND_DRIVER and check, given the device address, if the hardware block
>>> could be used in a Linux context. If not it returns NOTIFY_BAD to driver core
>>> to stop driver probing.
>>
>> Huh?
>>
>> If these devices are not usable from the non-secure side, why are they
>> not removed form the DT (or marked disabled)?
>>
>> In other cases, where resources are carved out for the secure side (e.g.
>> DRAM carveouts), that's how we handle things.
>>
> 
> That true you can parse and disable a device a boot time but if DT doesn't
> exactly reflect etzpc status bits we will in trouble when try to get access to
> the device.

Well, yes. If the DT doesn't correctly represent the hardware, things 
will probably go wrong; that's hardly a novel concept, and it's 
certainly not unique to this particular SoC.

> Changing the DT is a software protection while etzpc is an hardware protection
> so we need to check it anyway.

There are several in-tree DT and code examples where devices are marked 
as disabled on certain boards/SoC variants/etc. because attempting to 
access them can abort/lock up/trigger a secure watchdog reset/etc. The 
only "special" thing in this particular situation is apparently that 
this device even allows its secure configuration to be probed from the 
non-secure side at all.

Implementing a boardfile so that you can "check" the DT makes very 
little sense to me; Linux is not a firmware validation suite.

Robin.

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

[Benjamin Gaignard]

2018-02-27 20:46 GMT+01:00 Robin Murphy <robin.murphy@arm.com>:
> On 27/02/18 19:16, Benjamin Gaignard wrote:
>>
>> 2018-02-27 18:11 GMT+01:00 Mark Rutland <mark.rutland@arm.com>:
>>>
>>> On Tue, Feb 27, 2018 at 03:09:23PM +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 access will all be discarded.
>>>>
>>>> Extended TrustZone Protection driver register itself as listener of
>>>> BUS_NOTIFY_BIND_DRIVER and check, given the device address, if the
>>>> hardware block
>>>> could be used in a Linux context. If not it returns NOTIFY_BAD to driver
>>>> core
>>>> to stop driver probing.
>>>
>>>
>>> Huh?
>>>
>>> If these devices are not usable from the non-secure side, why are they
>>> not removed form the DT (or marked disabled)?
>>>
>>> In other cases, where resources are carved out for the secure side (e.g.
>>> DRAM carveouts), that's how we handle things.
>>>
>>
>> That true you can parse and disable a device a boot time but if DT doesn't
>> exactly reflect etzpc status bits we will in trouble when try to get
>> access to
>> the device.
>
>
> Well, yes. If the DT doesn't correctly represent the hardware, things will
> probably go wrong; that's hardly a novel concept, and it's certainly not
> unique to this particular SoC.
>
>> Changing the DT is a software protection while etzpc is an hardware
>> protection
>> so we need to check it anyway.
>
>
> There are several in-tree DT and code examples where devices are marked as
> disabled on certain boards/SoC variants/etc. because attempting to access
> them can abort/lock up/trigger a secure watchdog reset/etc. The only
> "special" thing in this particular situation is apparently that this device
> even allows its secure configuration to be probed from the non-secure side
> at all.
>
> Implementing a boardfile so that you can "check" the DT makes very little
> sense to me; Linux is not a firmware validation suite.

It is not about to "check" the DT but if Linux could get access to the hardware.
Hardware block assignment to secure or non-secure world could change at runtime
for example I2C block could be manage by secure OS for a trusted
application and when
it have finish "release" the it for Linux. I don't think that could be
done by changing DT.

I think that dhecking hardware blocks status bits before probe them is
also more robust than let
each driver discover at probe time that it hardware isn't responding.

Benjamin

>
> Robin.

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

[Mark Rutland]

On Wed, Feb 28, 2018 at 08:53:28AM +0100, Benjamin Gaignard wrote:
> 2018-02-27 20:46 GMT+01:00 Robin Murphy <robin.murphy@arm.com>:
> > On 27/02/18 19:16, Benjamin Gaignard wrote:
> >> 2018-02-27 18:11 GMT+01:00 Mark Rutland <mark.rutland@arm.com>:
> >>> On Tue, Feb 27, 2018 at 03:09:23PM +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 access will all be discarded.
> >>>>
> >>>> Extended TrustZone Protection driver register itself as listener
> >>>> of BUS_NOTIFY_BIND_DRIVER and check, given the device address, if
> >>>> the hardware block could be used in a Linux context. If not it
> >>>> returns NOTIFY_BAD to driver core to stop driver probing.

> >>> If these devices are not usable from the non-secure side, why are they
> >>> not removed form the DT (or marked disabled)?
> >>>
> >>> In other cases, where resources are carved out for the secure side (e.g.
> >>> DRAM carveouts), that's how we handle things.
> >>
> >> That true you can parse and disable a device a boot time but if DT
> >> doesn't exactly reflect etzpc status bits we will in trouble when
> >> try to get access to the device.
> >
> > Well, yes. If the DT doesn't correctly represent the hardware,
> > things will probably go wrong; that's hardly a novel concept, and
> > it's certainly not unique to this particular SoC.
> >
> >> Changing the DT is a software protection while etzpc is an hardware
> >> protection so we need to check it anyway.
> >
> > There are several in-tree DT and code examples where devices are marked as
> > disabled on certain boards/SoC variants/etc. because attempting to access
> > them can abort/lock up/trigger a secure watchdog reset/etc. The only
> > "special" thing in this particular situation is apparently that this device
> > even allows its secure configuration to be probed from the non-secure side
> > at all.
> >
> > Implementing a boardfile so that you can "check" the DT makes very little
> > sense to me; Linux is not a firmware validation suite.
> 
> It is not about to "check" the DT but if Linux could get access to the
> hardware.  Hardware block assignment to secure or non-secure world
> could change at runtime for example I2C block could be manage by
> secure OS for a trusted application and when it have finish "release"
> the it for Linux.

The driver does not do this today. It probe the HW once during early
boot, then aborts driver probes. It provides no provision for dynamic
assignment.

Is this something you plan to implement? How will the secure world
notify the non-secure world of its intent to manage a device, or
vice-versa?

> I don't think that could be done by changing DT.
> 
> I think that dhecking hardware blocks status bits before probe them is
> also more robust than let
> each driver discover at probe time that it hardware isn't responding.

I don't follow. Robin and I suggest that gets encoded in the DT, which
is *more* efficient than having each driver probe the DT, begin probing,
then abort via the notifier.

This really seems like something that should be done *prior* to entering
Linux.

Thanks,
Mark.

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

[Robin Murphy]

On 28/02/18 17:53, Mark Rutland wrote:
[...]
>> It is not about to "check" the DT but if Linux could get access to the
>> hardware.  Hardware block assignment to secure or non-secure world
>> could change at runtime for example I2C block could be manage by
>> secure OS for a trusted application and when it have finish "release"
>> the it for Linux.
> 
> The driver does not do this today. It probe the HW once during early
> boot, then aborts driver probes. It provides no provision for dynamic
> assignment.
> 
> Is this something you plan to implement? How will the secure world
> notify the non-secure world of its intent to manage a device, or
> vice-versa?

Note that this is another thing which in general already happens - 
control of (and correspondingly, hardware access to) things like display 
engines and video decoders can get transferred between Linux (well, 
Android at least) and a trusted OS. You need communication and 
cooperation between the two sides via channels like tee-supplicant to 
make it usable, though, at which point the fact that this ETZPC provides 
non-secure-visible status unlike other TZASCs is rather superfluous - if 
the secure side could just blindly take ownership of the I2C block in 
response to some event, while the Linux I2C driver is in the middle of 
its own transfer, a status bit in a register somewhere else isn't going 
to be much help overall.

>> I don't think that could be done by changing DT.
>>
>> I think that dhecking hardware blocks status bits before probe them is
>> also more robust than let
>> each driver discover at probe time that it hardware isn't responding.
> 
> I don't follow. Robin and I suggest that gets encoded in the DT, which
> is *more* efficient than having each driver probe the DT, begin probing,
> then abort via the notifier.
> 
> This really seems like something that should be done *prior* to entering
> Linux.

Indeed; the DT by nature describes the initial state of the system at 
the point that Linux takes control, and thus it really *should* reflect 
whatever the current ETZPC configuration is. Note what DTSpec actually says:

     "disabled" 	

     Indicates that the device is not presently operational, but it might
     become operational in the future (for example, something is not
     plugged in, or switched off).

     Refer to the device binding for details on what disabled means for a
     given device.

The fact that the current behaviour of our OF platform code doesn't 
really respect that last point and makes it tricky to bring 
initially-unavailable devices to life later is a separate issue.

Robin.

Re: [PATCH 1/3] driver core: check notifier_call_chain return value

[Greg KH]

On Tue, Feb 27, 2018 at 03:09:24PM +0100, Benjamin Gaignard wrote:
> When being notified that a driver is about to be bind a listener
> could return NOTIFY_BAD.
> Check the return to be sure that the driver could be bind.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  drivers/base/dd.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index de6fd092bf2f..9275f2c0fed2 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -304,9 +304,12 @@ static int driver_sysfs_add(struct device *dev)
>  {
>  	int ret;
>  
> -	if (dev->bus)
> -		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> -					     BUS_NOTIFY_BIND_DRIVER, dev);
> +	if (dev->bus) {
> +		if (blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> +						 BUS_NOTIFY_BIND_DRIVER, dev) ==
> +						 NOTIFY_BAD)
> +			return -EINVAL;

checkpatch does not complain about this?

And what is going to break when we enable this, as we have never checked
this before?

thanks,

greg k-h

Re: [PATCH 1/3] driver core: check notifier_call_chain return value

[Benjamin Gaignard]

2018-03-15 18:10 GMT+01:00 Greg KH <gregkh@linuxfoundation.org>:
> On Tue, Feb 27, 2018 at 03:09:24PM +0100, Benjamin Gaignard wrote:
>> When being notified that a driver is about to be bind a listener
>> could return NOTIFY_BAD.
>> Check the return to be sure that the driver could be bind.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>>  drivers/base/dd.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index de6fd092bf2f..9275f2c0fed2 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -304,9 +304,12 @@ static int driver_sysfs_add(struct device *dev)
>>  {
>>       int ret;
>>
>> -     if (dev->bus)
>> -             blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
>> -                                          BUS_NOTIFY_BIND_DRIVER, dev);
>> +     if (dev->bus) {
>> +             if (blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
>> +                                              BUS_NOTIFY_BIND_DRIVER, dev) ==
>> +                                              NOTIFY_BAD)
>> +                     return -EINVAL;
>
> checkpatch does not complain about this?

No (even with --strict), it should be indented with tabs

>
> And what is going to break when we enable this, as we have never checked
> this before?

I could have miss some occurences but when greping with BUS_NOTIFY_*
patern I haven't any problematic cases.
When notifiers don't care of the message they almost all return
NOTIFY_DONE, some return NOTIFY_OK but none
return NOTIFY_BAD. That I wrote the test like "== NOTIFY_BAD" and not
"!= NOTIFY_OK".

I have checked this list of files (I hope I haven't forgot any occurence)
arch/powerpc/kernel/isa-bridge.c
arch/powerpc/kernel/iommu.c
arch/powerpc/kernel/dma-swiotlb.c
arch/powerpc/platforms/pasemi/setup.c
arch/powerpc/platforms/512x/pdm360ng.c
arch/powerpc/platforms/cell/iommu.c
arch/arm/mach-mvebu/coherency.c
arch/arm/common/sa1111.c
arch/arm/mach-keystone/keystone.c -> this one return NOTIFY_BAD if dev
is NULL which is never the case.
arch/arm/mach-highbank/highbank.c
arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
arch/arm/mach-omap2/omap_device.c
drivers/base/power/clock_ops.c
drivers/platform/x86/silead_dmi.c
drivers/input/serio/i8042.c
drivers/input/mouse/psmouse-smbus.c
drivers/w1/w1.c
drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
drivers/i2c/i2c-dev.c drivers/acpi/acpi_lpss.c
drivers/gpu/vga/vgaarb.c
drivers/xen/pci.c drivers/xen/xen-pciback/pci_stub.c
drivers/xen/arm-device.c
drivers/iommu/s390-iommu.c
drivers/iommu/iommu.c
drivers/iommu/dmar.c
drivers/iommu/intel-iommu.c
drivers/usb/core/usb.c
drivers/s390/cio/ccwgroup.c
drivers/net/ethernet/ibm/emac/core.c

Benjamin

>
> thanks,
>
> greg k-h