* [PATCH 0/3] STM32 Extended TrustZone Protection driver
@ 2018-02-27 14:09 Benjamin Gaignard
2018-02-27 14:09 ` [PATCH 1/3] driver core: check notifier_call_chain return value Benjamin Gaignard
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Benjamin Gaignard @ 2018-02-27 14:09 UTC (permalink / raw)
To: gregkh, robh+dt, mark.rutland, mcoquelin.stm32, alexandre.torgue
Cc: devicetree, linux-arm-kernel, linux-kernel, 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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] driver core: check notifier_call_chain return value 2018-02-27 14:09 [PATCH 0/3] STM32 Extended TrustZone Protection driver Benjamin Gaignard @ 2018-02-27 14:09 ` Benjamin Gaignard 2018-03-15 17:10 ` Greg KH 2018-02-27 14:09 ` [PATCH 2/3] dt-bindings: stm32: Add bindings for Extended TrustZone Protection Benjamin Gaignard ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Benjamin Gaignard @ 2018-02-27 14:09 UTC (permalink / raw) To: gregkh, robh+dt, mark.rutland, mcoquelin.stm32, alexandre.torgue Cc: devicetree, linux-arm-kernel, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] driver core: check notifier_call_chain return value 2018-02-27 14:09 ` [PATCH 1/3] driver core: check notifier_call_chain return value Benjamin Gaignard @ 2018-03-15 17:10 ` Greg KH 2018-03-16 8:53 ` Benjamin Gaignard 0 siblings, 1 reply; 14+ messages in thread From: Greg KH @ 2018-03-15 17:10 UTC (permalink / raw) To: Benjamin Gaignard Cc: robh+dt, mark.rutland, mcoquelin.stm32, alexandre.torgue, devicetree, linux-arm-kernel, linux-kernel, Benjamin Gaignard 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] driver core: check notifier_call_chain return value 2018-03-15 17:10 ` Greg KH @ 2018-03-16 8:53 ` Benjamin Gaignard 0 siblings, 0 replies; 14+ messages in thread From: Benjamin Gaignard @ 2018-03-16 8:53 UTC (permalink / raw) To: Greg KH Cc: Rob Herring, Mark Rutland, Maxime Coquelin, Alexandre Torgue, devicetree, Linux ARM, Linux Kernel Mailing List, 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] dt-bindings: stm32: Add bindings for Extended TrustZone Protection 2018-02-27 14:09 [PATCH 0/3] STM32 Extended TrustZone Protection driver Benjamin Gaignard 2018-02-27 14:09 ` [PATCH 1/3] driver core: check notifier_call_chain return value Benjamin Gaignard @ 2018-02-27 14:09 ` Benjamin Gaignard 2018-02-27 14:09 ` [PATCH 3/3] ARM: mach-stm32: Add Extended TrustZone Protection driver Benjamin Gaignard 2018-02-27 17:11 ` [PATCH 0/3] STM32 " Mark Rutland 3 siblings, 0 replies; 14+ messages in thread From: Benjamin Gaignard @ 2018-02-27 14:09 UTC (permalink / raw) To: gregkh, robh+dt, mark.rutland, mcoquelin.stm32, alexandre.torgue Cc: devicetree, linux-arm-kernel, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] ARM: mach-stm32: Add Extended TrustZone Protection driver 2018-02-27 14:09 [PATCH 0/3] STM32 Extended TrustZone Protection driver Benjamin Gaignard 2018-02-27 14:09 ` [PATCH 1/3] driver core: check notifier_call_chain return value Benjamin Gaignard 2018-02-27 14:09 ` [PATCH 2/3] dt-bindings: stm32: Add bindings for Extended TrustZone Protection Benjamin Gaignard @ 2018-02-27 14:09 ` Benjamin Gaignard 2018-02-27 17:14 ` Mark Rutland 2018-02-27 17:11 ` [PATCH 0/3] STM32 " Mark Rutland 3 siblings, 1 reply; 14+ messages in thread From: Benjamin Gaignard @ 2018-02-27 14:09 UTC (permalink / raw) To: gregkh, robh+dt, mark.rutland, mcoquelin.stm32, alexandre.torgue Cc: devicetree, linux-arm-kernel, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] ARM: mach-stm32: Add Extended TrustZone Protection driver 2018-02-27 14:09 ` [PATCH 3/3] ARM: mach-stm32: Add Extended TrustZone Protection driver Benjamin Gaignard @ 2018-02-27 17:14 ` Mark Rutland 2018-02-27 19:23 ` Benjamin Gaignard 0 siblings, 1 reply; 14+ messages in thread From: Mark Rutland @ 2018-02-27 17:14 UTC (permalink / raw) To: Benjamin Gaignard Cc: gregkh, robh+dt, mcoquelin.stm32, alexandre.torgue, devicetree, linux-arm-kernel, linux-kernel, Benjamin Gaignard 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] ARM: mach-stm32: Add Extended TrustZone Protection driver 2018-02-27 17:14 ` Mark Rutland @ 2018-02-27 19:23 ` Benjamin Gaignard 0 siblings, 0 replies; 14+ messages in thread From: Benjamin Gaignard @ 2018-02-27 19:23 UTC (permalink / raw) To: Mark Rutland Cc: Greg Kroah-Hartman, Rob Herring, Maxime Coquelin, Alexandre Torgue, devicetree, Linux ARM, Linux Kernel Mailing List, 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] STM32 Extended TrustZone Protection driver 2018-02-27 14:09 [PATCH 0/3] STM32 Extended TrustZone Protection driver Benjamin Gaignard ` (2 preceding siblings ...) 2018-02-27 14:09 ` [PATCH 3/3] ARM: mach-stm32: Add Extended TrustZone Protection driver Benjamin Gaignard @ 2018-02-27 17:11 ` Mark Rutland 2018-02-27 19:16 ` Benjamin Gaignard 3 siblings, 1 reply; 14+ messages in thread From: Mark Rutland @ 2018-02-27 17:11 UTC (permalink / raw) To: Benjamin Gaignard Cc: gregkh, robh+dt, mcoquelin.stm32, alexandre.torgue, devicetree, linux-arm-kernel, linux-kernel, Benjamin Gaignard 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] STM32 Extended TrustZone Protection driver 2018-02-27 17:11 ` [PATCH 0/3] STM32 " Mark Rutland @ 2018-02-27 19:16 ` Benjamin Gaignard 2018-02-27 19:46 ` Robin Murphy 0 siblings, 1 reply; 14+ messages in thread From: Benjamin Gaignard @ 2018-02-27 19:16 UTC (permalink / raw) To: Mark Rutland Cc: Greg Kroah-Hartman, Rob Herring, Maxime Coquelin, Alexandre Torgue, devicetree, Linux ARM, Linux Kernel Mailing List, 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 >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] STM32 Extended TrustZone Protection driver 2018-02-27 19:16 ` Benjamin Gaignard @ 2018-02-27 19:46 ` Robin Murphy 2018-02-28 7:53 ` Benjamin Gaignard 0 siblings, 1 reply; 14+ messages in thread From: Robin Murphy @ 2018-02-27 19:46 UTC (permalink / raw) To: Benjamin Gaignard, Mark Rutland Cc: devicetree, Alexandre Torgue, Greg Kroah-Hartman, Linux Kernel Mailing List, Rob Herring, Maxime Coquelin, Linux ARM, Benjamin Gaignard 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] STM32 Extended TrustZone Protection driver 2018-02-27 19:46 ` Robin Murphy @ 2018-02-28 7:53 ` Benjamin Gaignard 2018-02-28 17:53 ` Mark Rutland 0 siblings, 1 reply; 14+ messages in thread From: Benjamin Gaignard @ 2018-02-28 7:53 UTC (permalink / raw) To: Robin Murphy Cc: Mark Rutland, devicetree, Alexandre Torgue, Greg Kroah-Hartman, Linux Kernel Mailing List, Rob Herring, Maxime Coquelin, Linux ARM, 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] STM32 Extended TrustZone Protection driver 2018-02-28 7:53 ` Benjamin Gaignard @ 2018-02-28 17:53 ` Mark Rutland 2018-02-28 18:32 ` Robin Murphy 0 siblings, 1 reply; 14+ messages in thread From: Mark Rutland @ 2018-02-28 17:53 UTC (permalink / raw) To: Benjamin Gaignard Cc: Robin Murphy, devicetree, Alexandre Torgue, Greg Kroah-Hartman, Linux Kernel Mailing List, Rob Herring, Maxime Coquelin, Linux ARM, Benjamin Gaignard 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] STM32 Extended TrustZone Protection driver 2018-02-28 17:53 ` Mark Rutland @ 2018-02-28 18:32 ` Robin Murphy 0 siblings, 0 replies; 14+ messages in thread From: Robin Murphy @ 2018-02-28 18:32 UTC (permalink / raw) To: Benjamin Gaignard Cc: Mark Rutland, devicetree, Alexandre Torgue, Greg Kroah-Hartman, Linux Kernel Mailing List, Rob Herring, Maxime Coquelin, Linux ARM, Benjamin Gaignard 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-03-16 8:53 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-27 14:09 [PATCH 0/3] STM32 Extended TrustZone Protection driver Benjamin Gaignard 2018-02-27 14:09 ` [PATCH 1/3] driver core: check notifier_call_chain return value Benjamin Gaignard 2018-03-15 17:10 ` Greg KH 2018-03-16 8:53 ` Benjamin Gaignard 2018-02-27 14:09 ` [PATCH 2/3] dt-bindings: stm32: Add bindings for Extended TrustZone Protection Benjamin Gaignard 2018-02-27 14:09 ` [PATCH 3/3] ARM: mach-stm32: Add Extended TrustZone Protection driver Benjamin Gaignard 2018-02-27 17:14 ` Mark Rutland 2018-02-27 19:23 ` Benjamin Gaignard 2018-02-27 17:11 ` [PATCH 0/3] STM32 " Mark Rutland 2018-02-27 19:16 ` Benjamin Gaignard 2018-02-27 19:46 ` Robin Murphy 2018-02-28 7:53 ` Benjamin Gaignard 2018-02-28 17:53 ` Mark Rutland 2018-02-28 18:32 ` Robin Murphy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).