Sixth attempt at the support for Trusted Foundations, which allows many
Tegra-based (and quite possibly other) devices sold on the market to boot and
be used with an upstream kernel.
v5 did not receive many comments on the code itself, so I hope this means
the patch set is converging well. Most remarks were about the lack of clarity
about what Trusted Foundations is and how it fits with respect to SMC calling
conventions and the PSCI standard (spoiler: it does not fit).
Change against v5:
- added a no-op of_register_trusted_foundations() for the case where neither
Trusted Foundations nor device tree support are enabled
- made it clear in the commit message and source file that TF does not follow
the SMC calling convention, is totally independant of PSIC and only matters
to devices that come with a bootloader that enables it.
Alexandre Courbot (5):
ARM: add basic support for Trusted Foundations
ARM: tegra: add support for Trusted Foundations
ARM: tegra: split setting of CPU reset handler
ARM: tegra: set CPU reset handler with firmware op
ARM: tegra: support Trusted Foundations by default
.../arm/firmware/tl,trusted-foundations.txt | 17 +++++
Documentation/devicetree/bindings/arm/tegra.txt | 5 ++
.../devicetree/bindings/vendor-prefixes.txt | 1 +
arch/arm/Kconfig | 2 +
arch/arm/Makefile | 1 +
arch/arm/configs/tegra_defconfig | 1 +
arch/arm/firmware/Kconfig | 28 ++++++++
arch/arm/firmware/Makefile | 1 +
arch/arm/firmware/trusted_foundations.c | 79 ++++++++++++++++++++++
arch/arm/include/asm/trusted_foundations.h | 70 +++++++++++++++++++
arch/arm/mach-tegra/Kconfig | 1 +
arch/arm/mach-tegra/common.c | 2 +
arch/arm/mach-tegra/reset.c | 40 ++++++++---
13 files changed, 237 insertions(+), 11 deletions(-)
create mode 100644 Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundations.txt
create mode 100644 arch/arm/firmware/Kconfig
create mode 100644 arch/arm/firmware/Makefile
create mode 100644 arch/arm/firmware/trusted_foundations.c
create mode 100644 arch/arm/include/asm/trusted_foundations.h
--
1.8.4
Not all Tegra devices can set the CPU reset handler in the same way.
In particular, devices using a TrustZone secure monitor cannot set the
reset handler directly and need to do it through a firmware operation.
This patch separates the act of setting the reset handler from its
preparation, so the former can be implemented in a different way.
Signed-off-by: Alexandre Courbot <[email protected]>
Reviewed-by: Tomasz Figa <[email protected]>
Reviewed-by: Stephen Warren <[email protected]>
---
arch/arm/mach-tegra/reset.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/arch/arm/mach-tegra/reset.c b/arch/arm/mach-tegra/reset.c
index 1ac434e..6964117 100644
--- a/arch/arm/mach-tegra/reset.c
+++ b/arch/arm/mach-tegra/reset.c
@@ -33,26 +33,18 @@
static bool is_enabled;
-static void __init tegra_cpu_reset_handler_enable(void)
+static void __init tegra_cpu_reset_handler_set(const u32 reset_address)
{
- void __iomem *iram_base = IO_ADDRESS(TEGRA_IRAM_RESET_BASE);
void __iomem *evp_cpu_reset =
IO_ADDRESS(TEGRA_EXCEPTION_VECTORS_BASE + 0x100);
void __iomem *sb_ctrl = IO_ADDRESS(TEGRA_SB_BASE);
u32 reg;
- BUG_ON(is_enabled);
- BUG_ON(tegra_cpu_reset_handler_size > TEGRA_IRAM_RESET_HANDLER_SIZE);
-
- memcpy(iram_base, (void *)__tegra_cpu_reset_handler_start,
- tegra_cpu_reset_handler_size);
-
/*
* NOTE: This must be the one and only write to the EVP CPU reset
* vector in the entire system.
*/
- writel(TEGRA_IRAM_RESET_BASE + tegra_cpu_reset_handler_offset,
- evp_cpu_reset);
+ writel(reset_address, evp_cpu_reset);
wmb();
reg = readl(evp_cpu_reset);
@@ -66,6 +58,21 @@ static void __init tegra_cpu_reset_handler_enable(void)
writel(reg, sb_ctrl);
wmb();
}
+}
+
+static void __init tegra_cpu_reset_handler_enable(void)
+{
+ void __iomem *iram_base = IO_ADDRESS(TEGRA_IRAM_RESET_BASE);
+ const u32 reset_address = TEGRA_IRAM_RESET_BASE +
+ tegra_cpu_reset_handler_offset;
+
+ BUG_ON(is_enabled);
+ BUG_ON(tegra_cpu_reset_handler_size > TEGRA_IRAM_RESET_HANDLER_SIZE);
+
+ memcpy(iram_base, (void *)__tegra_cpu_reset_handler_start,
+ tegra_cpu_reset_handler_size);
+
+ tegra_cpu_reset_handler_set(reset_address);
is_enabled = true;
}
--
1.8.4
Use a firmware operation to set the CPU reset handler and only resort to
doing it ourselves if there is none defined.
This supports the booting of secondary CPUs on devices using a TrustZone
secure monitor.
Signed-off-by: Alexandre Courbot <[email protected]>
Reviewed-by: Tomasz Figa <[email protected]>
Reviewed-by: Stephen Warren <[email protected]>
---
arch/arm/mach-tegra/reset.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-tegra/reset.c b/arch/arm/mach-tegra/reset.c
index 6964117..fc9cf03 100644
--- a/arch/arm/mach-tegra/reset.c
+++ b/arch/arm/mach-tegra/reset.c
@@ -21,6 +21,7 @@
#include <asm/cacheflush.h>
#include <asm/hardware/cache-l2x0.h>
+#include <asm/firmware.h>
#include "iomap.h"
#include "irammap.h"
@@ -65,6 +66,7 @@ static void __init tegra_cpu_reset_handler_enable(void)
void __iomem *iram_base = IO_ADDRESS(TEGRA_IRAM_RESET_BASE);
const u32 reset_address = TEGRA_IRAM_RESET_BASE +
tegra_cpu_reset_handler_offset;
+ int err;
BUG_ON(is_enabled);
BUG_ON(tegra_cpu_reset_handler_size > TEGRA_IRAM_RESET_HANDLER_SIZE);
@@ -72,9 +74,18 @@ static void __init tegra_cpu_reset_handler_enable(void)
memcpy(iram_base, (void *)__tegra_cpu_reset_handler_start,
tegra_cpu_reset_handler_size);
- tegra_cpu_reset_handler_set(reset_address);
-
- is_enabled = true;
+ err = call_firmware_op(set_cpu_boot_addr, 0, reset_address);
+ switch (err) {
+ case -ENOSYS:
+ tegra_cpu_reset_handler_set(reset_address);
+ /* pass-through */
+ case 0:
+ is_enabled = true;
+ break;
+ default:
+ pr_crit("Cannot set CPU reset handler: %d\n", err);
+ BUG();
+ }
}
void __init tegra_cpu_reset_handler_init(void)
--
1.8.4
Trusted Foundations is a TrustZone-based secure monitor for ARM that
can be invoked using the same SMC-based API on all supported
platforms. This patch adds initial basic support for Trusted
Foundations using the ARM firmware API. Current features are limited
to the ability to boot secondary processors.
Note: The API followed by Trusted Foundations does *not* follow the SMC
calling conventions. It has nothing to do with PSCI neither and is only
relevant to devices that use Trusted Foundations (like most Tegra-based
retail devices).
Signed-off-by: Alexandre Courbot <[email protected]>
Reviewed-by: Tomasz Figa <[email protected]>
Reviewed-by: Stephen Warren <[email protected]>
---
.../arm/firmware/tl,trusted-foundations.txt | 17 +++++
.../devicetree/bindings/vendor-prefixes.txt | 1 +
arch/arm/Kconfig | 2 +
arch/arm/Makefile | 1 +
arch/arm/firmware/Kconfig | 28 ++++++++
arch/arm/firmware/Makefile | 1 +
arch/arm/firmware/trusted_foundations.c | 79 ++++++++++++++++++++++
arch/arm/include/asm/trusted_foundations.h | 70 +++++++++++++++++++
8 files changed, 199 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundations.txt
create mode 100644 arch/arm/firmware/Kconfig
create mode 100644 arch/arm/firmware/Makefile
create mode 100644 arch/arm/firmware/trusted_foundations.c
create mode 100644 arch/arm/include/asm/trusted_foundations.h
diff --git a/Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundations.txt b/Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundations.txt
new file mode 100644
index 0000000..3954bbd
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundations.txt
@@ -0,0 +1,17 @@
+Trusted Foundations
+
+Boards that use the Trusted Foundations secure monitor can signal its
+presence by declaring a node compatible with "tl,trusted-foundations"
+under the root node.
+
+Required properties:
+- compatible : "tl,trusted-foundations"
+- version-major : major version number of Trusted Foundations firmware
+- version-minor: minor version number of Trusted Foundations firmware
+
+Example:
+ firmware {
+ compatible = "tl,trusted-foundations";
+ version-major = <2>;
+ version-minor = <8>;
+ };
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 366ce9b..20d61f3 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -63,6 +63,7 @@ ste ST-Ericsson
stericsson ST-Ericsson
toumaz Toumaz
ti Texas Instruments
+tl Trusted Logic
toshiba Toshiba Corporation
v3 V3 Semiconductor
via VIA Technologies, Inc.
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 43594d5..7fbe800 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1074,6 +1074,8 @@ config ARM_TIMER_SP804
select CLKSRC_MMIO
select CLKSRC_OF if OF
+source "arch/arm/firmware/Kconfig"
+
source arch/arm/mm/Kconfig
config ARM_NR_BANKS
diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 6fd2cea..a48de17 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -267,6 +267,7 @@ core-$(CONFIG_KVM_ARM_HOST) += arch/arm/kvm/
core-y += arch/arm/kernel/ arch/arm/mm/ arch/arm/common/
core-y += arch/arm/net/
core-y += arch/arm/crypto/
+core-y += arch/arm/firmware/
core-y += $(machdirs) $(platdirs)
drivers-$(CONFIG_OPROFILE) += arch/arm/oprofile/
diff --git a/arch/arm/firmware/Kconfig b/arch/arm/firmware/Kconfig
new file mode 100644
index 0000000..bb00ccf
--- /dev/null
+++ b/arch/arm/firmware/Kconfig
@@ -0,0 +1,28 @@
+config ARCH_SUPPORTS_FIRMWARE
+ bool
+
+config ARCH_SUPPORTS_TRUSTED_FOUNDATIONS
+ bool
+ select ARCH_SUPPORTS_FIRMWARE
+
+menu "Firmware options"
+ depends on ARCH_SUPPORTS_FIRMWARE
+
+config TRUSTED_FOUNDATIONS
+ bool "Trusted Foundations secure monitor support"
+ depends on ARCH_SUPPORTS_TRUSTED_FOUNDATIONS
+ help
+ Some devices (including most Tegra-based consumer devices on the
+ market) are booted with the Trusted Foundations secure monitor
+ active, requiring some core operations to be performed by the secure
+ monitor instead of the kernel.
+
+ This option allows the kernel to invoke the secure monitor whenever
+ required on devices using Trusted Foundations. See
+ arch/arm/include/asm/trusted_foundations.h or the
+ tl,trusted-foundations device tree binding documentation for details
+ on how to use it.
+
+ Say n if you don't know what this is about.
+
+endmenu
diff --git a/arch/arm/firmware/Makefile b/arch/arm/firmware/Makefile
new file mode 100644
index 0000000..a71f165
--- /dev/null
+++ b/arch/arm/firmware/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c
new file mode 100644
index 0000000..9382270
--- /dev/null
+++ b/arch/arm/firmware/trusted_foundations.c
@@ -0,0 +1,79 @@
+/*
+ * Trusted Foundations support for ARM CPUs
+ *
+ * Copyright (c) 2013, NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/of.h>
+#include <asm/firmware.h>
+#include <asm/trusted_foundations.h>
+
+#define TF_SET_CPU_BOOT_ADDR_SMC 0xfffff200
+
+static void __naked tf_generic_smc(u32 type, u32 subtype, u32 arg)
+{
+ asm volatile(
+ ".arch_extension sec\n\t"
+ "stmfd sp!, {r4 - r11, lr}\n\t"
+ __asmeq("%0", "r0")
+ __asmeq("%1", "r1")
+ __asmeq("%2", "r2")
+ "mov r3, #0\n\t"
+ "mov r4, #0\n\t"
+ "smc #0\n\t"
+ "ldmfd sp!, {r4 - r11, pc}"
+ :
+ : "r" (type), "r" (subtype), "r" (arg)
+ : "memory");
+}
+
+static int tf_set_cpu_boot_addr(int cpu, unsigned long boot_addr)
+{
+ tf_generic_smc(TF_SET_CPU_BOOT_ADDR_SMC, boot_addr, 0);
+
+ return 0;
+}
+
+static const struct firmware_ops trusted_foundations_ops = {
+ .set_cpu_boot_addr = tf_set_cpu_boot_addr,
+};
+
+void register_trusted_foundations(struct trusted_foundations_platform_data *pd)
+{
+ /* we are not using version information for now since currently
+ * supported SMCs are compatible with all TF releases */
+ register_firmware_ops(&trusted_foundations_ops);
+}
+
+#if IS_ENABLED(CONFIG_OF)
+void of_register_trusted_foundations(void)
+{
+ struct device_node *node;
+ struct trusted_foundations_platform_data pdata;
+ int err;
+
+ node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
+ if (!node)
+ return;
+
+ err = of_property_read_u32(node, "version-major", &pdata.version_major);
+ if (err != 0)
+ panic("Trusted Foundation: missing version-major property\n");
+ err = of_property_read_u32(node, "version-minor", &pdata.version_minor);
+ if (err != 0)
+ panic("Trusted Foundation: missing version-minor property\n");
+ register_trusted_foundations(&pdata);
+}
+#endif /* CONFIG_OF */
diff --git a/arch/arm/include/asm/trusted_foundations.h b/arch/arm/include/asm/trusted_foundations.h
new file mode 100644
index 0000000..921da58
--- /dev/null
+++ b/arch/arm/include/asm/trusted_foundations.h
@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2013, NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+/*
+ * Support for the Trusted Foundations secure monitor.
+ *
+ * Trusted Foundation comes active on some ARM consumer devices (most
+ * Tegra-based devices sold on the market are concerned). Such devices can only
+ * perform some basic operations, like setting the CPU reset vector, through
+ * SMC calls to the secure monitor. The calls are completely specific to
+ * Trusted Foundations, and do *not* follow the SMC calling convention or the
+ * PSCI standard.
+ */
+
+#ifndef __ASM_ARM_TRUSTED_FOUNDATIONS_H
+#define __ASM_ARM_TRUSTED_FOUNDATIONS_H
+
+#include <linux/kconfig.h>
+
+struct trusted_foundations_platform_data {
+ unsigned int version_major;
+ unsigned int version_minor;
+};
+
+#if IS_ENABLED(CONFIG_TRUSTED_FOUNDATIONS)
+void register_trusted_foundations(struct trusted_foundations_platform_data *pd);
+#if IS_ENABLED(CONFIG_OF)
+void of_register_trusted_foundations(void);
+#endif
+
+#else /* CONFIG_TRUSTED_FOUNDATIONS */
+
+#include <linux/printk.h>
+#include <linux/of.h>
+#include <asm/bug.h>
+
+static inline void register_trusted_foundations(
+ struct trusted_foundations_platform_data *pd)
+{
+ panic("No support for Trusted Foundations, stopping...\n");
+}
+
+#if IS_ENABLED(CONFIG_OF)
+static inline void of_register_trusted_foundations(void)
+{
+ /* If we find the target should enable TF but does not support it,
+ * fail as the system won't be able to do much anyway */
+ if (of_find_compatible_node(NULL, NULL, "tl,trusted-foundations"))
+ register_trusted_foundations(NULL);
+}
+#else
+static inline void of_register_trusted_foundations(void)
+{
+}
+#endif /* CONFIG_OF */
+
+#endif /* CONFIG_TRUSTED_FOUNDATIONS */
+
+#endif
--
1.8.4
Support for Trusted Foundations is light and allows the kernel to run on
a wider range of devices, so enable it by default.
Signed-off-by: Alexandre Courbot <[email protected]>
Reviewed-by: Tomasz Figa <[email protected]>
Reviewed-by: Stephen Warren <[email protected]>
---
arch/arm/configs/tegra_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index 1effb43..90c90d9 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig
@@ -28,6 +28,7 @@ CONFIG_ARCH_TEGRA_3x_SOC=y
CONFIG_ARCH_TEGRA_114_SOC=y
CONFIG_TEGRA_PCI=y
CONFIG_TEGRA_EMC_SCALING_ENABLE=y
+CONFIG_TRUSTED_FOUNDATIONS=y
CONFIG_SMP=y
CONFIG_PREEMPT=y
CONFIG_AEABI=y
--
1.8.4
Register the firmware operations for Trusted Foundations if the device
tree indicates it is active on the device.
Signed-off-by: Alexandre Courbot <[email protected]>
Reviewed-by: Tomasz Figa <[email protected]>
Reviewed-by: Stephen Warren <[email protected]>
---
Documentation/devicetree/bindings/arm/tegra.txt | 5 +++++
arch/arm/mach-tegra/Kconfig | 1 +
arch/arm/mach-tegra/common.c | 2 ++
3 files changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt
index ed9c853..8e51884 100644
--- a/Documentation/devicetree/bindings/arm/tegra.txt
+++ b/Documentation/devicetree/bindings/arm/tegra.txt
@@ -32,3 +32,8 @@ board-specific compatible values:
nvidia,whistler
toradex,colibri_t20-512
toradex,iris
+
+Trusted Foundations
+-------------------------------------------
+Tegra supports the Trusted Foundation secure monitor. See the
+"tl,trusted-foundations" binding's documentation for more details.
diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
index ef3a8da..b6b7c44 100644
--- a/arch/arm/mach-tegra/Kconfig
+++ b/arch/arm/mach-tegra/Kconfig
@@ -2,6 +2,7 @@ config ARCH_TEGRA
bool "NVIDIA Tegra" if ARCH_MULTI_V7
select ARCH_HAS_CPUFREQ
select ARCH_REQUIRE_GPIOLIB
+ select ARCH_SUPPORTS_TRUSTED_FOUNDATIONS
select CLKDEV_LOOKUP
select CLKSRC_MMIO
select CLKSRC_OF
diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c
index 94a119a..b405e45 100644
--- a/arch/arm/mach-tegra/common.c
+++ b/arch/arm/mach-tegra/common.c
@@ -27,6 +27,7 @@
#include <linux/clk-provider.h>
#include <asm/hardware/cache-l2x0.h>
+#include <asm/trusted_foundations.h>
#include "board.h"
#include "common.h"
@@ -99,6 +100,7 @@ static void __init tegra_init_cache(void)
void __init tegra_init_early(void)
{
+ of_register_trusted_foundations();
tegra_cpu_reset_handler_init();
tegra_apb_io_init();
tegra_init_fuse();
--
1.8.4
On 09/15/2013 03:40 AM, Alexandre Courbot wrote:
> Trusted Foundations is a TrustZone-based secure monitor for ARM that
> can be invoked using the same SMC-based API on all supported
> platforms. This patch adds initial basic support for Trusted
> Foundations using the ARM firmware API. Current features are limited
> to the ability to boot secondary processors.
>
> Note: The API followed by Trusted Foundations does *not* follow the SMC
> calling conventions. It has nothing to do with PSCI neither and is only
> relevant to devices that use Trusted Foundations (like most Tegra-based
> retail devices).
> diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c
> +#if IS_ENABLED(CONFIG_OF)
> +void of_register_trusted_foundations(void)
> +{
> + struct device_node *node;
> + struct trusted_foundations_platform_data pdata;
> + int err;
> +
> + node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
> + if (!node)
> + return;
...
> diff --git a/arch/arm/include/asm/trusted_foundations.h b/arch/arm/include/asm/trusted_foundations.h
> +#if IS_ENABLED(CONFIG_TRUSTED_FOUNDATIONS)
> +void register_trusted_foundations(struct trusted_foundations_platform_data *pd);
> +#if IS_ENABLED(CONFIG_OF)
> +void of_register_trusted_foundations(void);
> +#endif
I still don't think that's correct.
If TF support is enabled, yet DT support is not enabled, then there is
no prototype, implementation, or dummy implementation for
of_register_trusted_foundations(). I think there should be a dummy
implementation in this case, shouldn't there?
> +
> +#else /* CONFIG_TRUSTED_FOUNDATIONS */
> +
> +#include <linux/printk.h>
> +#include <linux/of.h>
> +#include <asm/bug.h>
> +
> +static inline void register_trusted_foundations(
> + struct trusted_foundations_platform_data *pd)
> +{
> + panic("No support for Trusted Foundations, stopping...\n");
> +}
> +
> +#if IS_ENABLED(CONFIG_OF)
> +static inline void of_register_trusted_foundations(void)
> +{
> + /* If we find the target should enable TF but does not support it,
> + * fail as the system won't be able to do much anyway */
> + if (of_find_compatible_node(NULL, NULL, "tl,trusted-foundations"))
> + register_trusted_foundations(NULL);
> +}
> +#else
> +static inline void of_register_trusted_foundations(void)
> +{
> +}
> +#endif /* CONFIG_OF */
That's more complex than it needs to be; there is a dummy
of_find_compatible_node() in the !OF case, so you don't need to ifdef
the implementation of of_register_trusted_foundations(); you just need
the first implementation here.
> +#endif /* CONFIG_TRUSTED_FOUNDATIONS */
In summary, I think you need:
If TF is enabled, always implement of_register_trusted_foundations() in
the C file, and rely on of_find_compatible_node() to return NULL if
!CONFIG_OF.
If TF is not enabled, implement the inline version in the header file,
and again rely on of_find_compatible_node() to return NULL if !CONFIG_OF.
Unless I'm missing something!
On Tue, Sep 17, 2013 at 1:50 AM, Stephen Warren <[email protected]> wrote:
> On 09/15/2013 03:40 AM, Alexandre Courbot wrote:
>> Trusted Foundations is a TrustZone-based secure monitor for ARM that
>> can be invoked using the same SMC-based API on all supported
>> platforms. This patch adds initial basic support for Trusted
>> Foundations using the ARM firmware API. Current features are limited
>> to the ability to boot secondary processors.
>>
>> Note: The API followed by Trusted Foundations does *not* follow the SMC
>> calling conventions. It has nothing to do with PSCI neither and is only
>> relevant to devices that use Trusted Foundations (like most Tegra-based
>> retail devices).
>
>> diff --git a/arch/arm/firmware/trusted_foundations.c b/arch/arm/firmware/trusted_foundations.c
>
>> +#if IS_ENABLED(CONFIG_OF)
>> +void of_register_trusted_foundations(void)
>> +{
>> + struct device_node *node;
>> + struct trusted_foundations_platform_data pdata;
>> + int err;
>> +
>> + node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
>> + if (!node)
>> + return;
> ...
>
>> diff --git a/arch/arm/include/asm/trusted_foundations.h b/arch/arm/include/asm/trusted_foundations.h
>
>> +#if IS_ENABLED(CONFIG_TRUSTED_FOUNDATIONS)
>> +void register_trusted_foundations(struct trusted_foundations_platform_data *pd);
>> +#if IS_ENABLED(CONFIG_OF)
>> +void of_register_trusted_foundations(void);
>> +#endif
>
> I still don't think that's correct.
>
> If TF support is enabled, yet DT support is not enabled, then there is
> no prototype, implementation, or dummy implementation for
> of_register_trusted_foundations(). I think there should be a dummy
> implementation in this case, shouldn't there?
>
>> +
>> +#else /* CONFIG_TRUSTED_FOUNDATIONS */
>> +
>> +#include <linux/printk.h>
>> +#include <linux/of.h>
>> +#include <asm/bug.h>
>> +
>> +static inline void register_trusted_foundations(
>> + struct trusted_foundations_platform_data *pd)
>> +{
>> + panic("No support for Trusted Foundations, stopping...\n");
>> +}
>> +
>> +#if IS_ENABLED(CONFIG_OF)
>> +static inline void of_register_trusted_foundations(void)
>> +{
>> + /* If we find the target should enable TF but does not support it,
>> + * fail as the system won't be able to do much anyway */
>> + if (of_find_compatible_node(NULL, NULL, "tl,trusted-foundations"))
>> + register_trusted_foundations(NULL);
>> +}
>> +#else
>> +static inline void of_register_trusted_foundations(void)
>> +{
>> +}
>> +#endif /* CONFIG_OF */
>
> That's more complex than it needs to be; there is a dummy
> of_find_compatible_node() in the !OF case, so you don't need to ifdef
> the implementation of of_register_trusted_foundations(); you just need
> the first implementation here.
>
>> +#endif /* CONFIG_TRUSTED_FOUNDATIONS */
>
> In summary, I think you need:
>
> If TF is enabled, always implement of_register_trusted_foundations() in
> the C file, and rely on of_find_compatible_node() to return NULL if
> !CONFIG_OF.
>
> If TF is not enabled, implement the inline version in the header file,
> and again rely on of_find_compatible_node() to return NULL if !CONFIG_OF.
Indeed, all your points are correct, and I've clearly been negligent
here. Sorry about that.
Will have to submit a v7, but hopefully we can get the acks that we
need (Russel?) on this version.
Alex.
Alexandre Courbot <[email protected]> writes:
> Trusted Foundations is a TrustZone-based secure monitor for ARM that
> can be invoked using the same SMC-based API on all supported
> platforms. This patch adds initial basic support for Trusted
> Foundations using the ARM firmware API. Current features are limited
> to the ability to boot secondary processors.
[...]
> +#if IS_ENABLED(CONFIG_OF)
> +void of_register_trusted_foundations(void)
> +{
> + struct device_node *node;
> + struct trusted_foundations_platform_data pdata;
> + int err;
> +
> + node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
> + if (!node)
> + return;
> +
> + err = of_property_read_u32(node, "version-major", &pdata.version_major);
> + if (err != 0)
> + panic("Trusted Foundation: missing version-major property\n");
Do really need to panic() the whole kernel for a missing property?
Surely this can more gracefully recover, or assume some defaults?
Same comment for the other uses of panic() in this patch.
Kevin
On Fri, Sep 20, 2013 at 5:49 AM, Kevin Hilman <[email protected]> wrote:
> Alexandre Courbot <[email protected]> writes:
>
>> Trusted Foundations is a TrustZone-based secure monitor for ARM that
>> can be invoked using the same SMC-based API on all supported
>> platforms. This patch adds initial basic support for Trusted
>> Foundations using the ARM firmware API. Current features are limited
>> to the ability to boot secondary processors.
>
> [...]
>
>> +#if IS_ENABLED(CONFIG_OF)
>> +void of_register_trusted_foundations(void)
>> +{
>> + struct device_node *node;
>> + struct trusted_foundations_platform_data pdata;
>> + int err;
>> +
>> + node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
>> + if (!node)
>> + return;
>> +
>> + err = of_property_read_u32(node, "version-major", &pdata.version_major);
>> + if (err != 0)
>> + panic("Trusted Foundation: missing version-major property\n");
>
> Do really need to panic() the whole kernel for a missing property?
> Surely this can more gracefully recover, or assume some defaults?
>
> Same comment for the other uses of panic() in this patch.
For something as essential as the secure monitor (for which there
cannot be "safe" defaults) it makes sense to have the version
explicitly mentioned. Version is a mandatory property in the bindings,
its implementors just need to follow it.
As for the other uses of panic(), that's because you just cannot
reasonably continue booting the system if trusted foundations is
required but not support for it is built into the kernel. Once again,
I don't feel this is out of place.
Alex.