| Message ID | 1371114745-24710-2-git-send-email-acourbot@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Jun 13, 2013 at 06:12:23PM +0900, Alexandre Courbot wrote: > Add basic support for booting secondary processors on Tegra devices > using the Trusted Foundations secure monitor. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > --- > Documentation/devicetree/bindings/arm/tegra.txt | 11 +++++ > .../devicetree/bindings/vendor-prefixes.txt | 1 + > arch/arm/configs/tegra_defconfig | 1 + > arch/arm/mach-tegra/Kconfig | 14 ++++++ > arch/arm/mach-tegra/Makefile | 3 ++ > arch/arm/mach-tegra/common.c | 2 + > arch/arm/mach-tegra/firmware.c | 40 +++++++++++++++++ > arch/arm/mach-tegra/firmware.h | 19 ++++++++ > arch/arm/mach-tegra/trusted_foundations.c | 51 ++++++++++++++++++++++ > 9 files changed, 142 insertions(+) > create mode 100644 arch/arm/mach-tegra/firmware.c > create mode 100644 arch/arm/mach-tegra/firmware.h > create mode 100644 arch/arm/mach-tegra/trusted_foundations.c > > diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt > index ed9c853..d59bc19 100644 > --- a/Documentation/devicetree/bindings/arm/tegra.txt > +++ b/Documentation/devicetree/bindings/arm/tegra.txt > @@ -32,3 +32,14 @@ board-specific compatible values: > nvidia,whistler > toradex,colibri_t20-512 > toradex,iris > + > +Optional nodes > +------------------------------------------- > + > +Trusted Foundations: > +Boards using the Trusted Foundations secure monitor should signal its > +presence by declaring a node compatible with "tl,trusted-foundations": > + > + firmware { You need to make a clear statement about whether the node name is I think it shouldn't required to be exactly equal to "firmware", because that would lead to problems if there were multiple independent firmware APIs present (which is certainly possible, whether or not it is true on Tegra). > + compatible = "tl,trusted-foundations"; > + }; For now, it might make more sense to make this binding tegra-specific, and to interpret the node is only implying the presence of the low- level SoC functions you are using on Tegra, not TF as a whole. Otherwise, it feels too generic. There is no description of exactly what functionality will be available if this node it present: if this is going to be a generic binding for TF, then it needs to work for all deployments of TF, not just your specific case. For example, how to you find out what functionality is present? Will there be a standard probing ABI for all versions and deployments of TF, or would extra information need to be described in the DT? Partly, this depends on whether the TF_SET_CPU_BOOT_ADDR_SMC call will be present, working, and with compatible ABI and semantics, on every SoC where an implementation of TF is present. Someone from Trusted Logic, or someone with visibility of the relevant ABI/API specs would need to judge on that -- do you have that info? > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt > index 6931c43..c21e1e9 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt > @@ -58,6 +58,7 @@ st STMicroelectronics > ste ST-Ericsson > stericsson ST-Ericsson > ti Texas Instruments > +tl Trusted Logic > toshiba Toshiba Corporation > via VIA Technologies, Inc. > wlf Wolfson Microelectronics > diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig > index 4de3bce..3bf158a 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_TEGRA_TRUSTED_FOUNDATIONS=y > CONFIG_SMP=y > CONFIG_PREEMPT=y > CONFIG_AEABI=y > diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig > index 84d72fc..055f496 100644 > --- a/arch/arm/mach-tegra/Kconfig > +++ b/arch/arm/mach-tegra/Kconfig > @@ -87,4 +87,18 @@ config TEGRA_AHB > config TEGRA_EMC_SCALING_ENABLE > bool "Enable scaling the memory frequency" > > +config TEGRA_TRUSTED_FOUNDATIONS > + bool "Trusted Foundations secure monitor support" > + help > + Many Tegra devices 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 when > + required on devices using Trusted Foundations. > + > + Devices using Trusted Foundations should pass a device tree containing > + a node compatible with "tl,trusted-foundations" to signal the presence > + of the secure monitor. > + > endmenu > diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile > index d011f0a..9fdc004 100644 > --- a/arch/arm/mach-tegra/Makefile > +++ b/arch/arm/mach-tegra/Makefile > @@ -12,6 +12,7 @@ obj-y += pm.o > obj-y += reset.o > obj-y += reset-handler.o > obj-y += sleep.o > +obj-y += firmware.o > obj-y += tegra.o > obj-$(CONFIG_CPU_IDLE) += cpuidle.o > obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += tegra20_speedo.o > @@ -37,3 +38,5 @@ endif > obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += board-harmony-pcie.o > > obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += board-paz00.o > + > +obj-$(CONFIG_TEGRA_TRUSTED_FOUNDATIONS) += trusted_foundations.o > diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c > index 9f852c6..4796bb0 100644 > --- a/arch/arm/mach-tegra/common.c > +++ b/arch/arm/mach-tegra/common.c > @@ -37,6 +37,7 @@ > #include "sleep.h" > #include "pm.h" > #include "reset.h" > +#include "firmware.h" > > /* > * Storage for debug-macro.S's state. > @@ -97,6 +98,7 @@ static void __init tegra_init_cache(void) > > void __init tegra_init_early(void) > { > + tegra_init_firmware(); > tegra_cpu_reset_handler_init(); > tegra_apb_io_init(); > tegra_init_fuse(); > diff --git a/arch/arm/mach-tegra/firmware.c b/arch/arm/mach-tegra/firmware.c > new file mode 100644 > index 0000000..ea861ca > --- /dev/null > +++ b/arch/arm/mach-tegra/firmware.c > @@ -0,0 +1,40 @@ > +/* > + * SecureOS support for Tegra CPUs Should the name "SecureOS" change in these comment headers? (affects multiple files) > + * > + * 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/printk.h> > +#include <linux/kconfig.h> > +#include <linux/of.h> > +#include <asm/firmware.h> > + > +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS) > +extern const struct firmware_ops tegra_trusted_foundations_ops; > +#endif > + > +void __init tegra_init_firmware(void) > +{ > + struct device_node *node; > + > + if (!of_have_populated_dt()) > + return; > + > + node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations"); > + if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)) > + pr_warn("Trusted Foundations detected but support missing!\n"); Should this be more than just a warning? It looks to me like tegra_cpu_reset_handler_set() might either silently fail or trigger an external abort in this situation, depending on the hardware and on how TF sets things up. There seems to be no way to signal an error when attempting to set a CPU's reset address. > +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS) > + else if (node) > + register_firmware_ops(&tegra_trusted_foundations_ops); > +#endif > +} IS_ENABLED() allows #ifdefs to be avoided -- maybe this would be clearer as: node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations"); if (!node) return; if (WARN_ON(!IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))) { pr_warn("Trusted Foundations detected but support missing!\n"); return; } register_firmware_ops(&tegra_trusted_foundations_ops); > diff --git a/arch/arm/mach-tegra/firmware.h b/arch/arm/mach-tegra/firmware.h > new file mode 100644 > index 0000000..77c62fb > --- /dev/null > +++ b/arch/arm/mach-tegra/firmware.h > @@ -0,0 +1,19 @@ > +/* > + * Copyright (c) 2013, NVIDIA Corporation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope 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. > + */ > + > +#ifndef __TEGRA_FIRMWARE_H > +#define __TEGRA_FIRMWARE_H > + > +void tegra_init_firmware(void); > + > +#endif > diff --git a/arch/arm/mach-tegra/trusted_foundations.c b/arch/arm/mach-tegra/trusted_foundations.c > new file mode 100644 > index 0000000..411044f > --- /dev/null > +++ b/arch/arm/mach-tegra/trusted_foundations.c > @@ -0,0 +1,51 @@ > +/* > + * SecureOS support for Tegra 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> > + > +#define TF_SET_CPU_BOOT_ADDR_SMC 0xfffff200 > + > +static void __attribute__((naked)) tegra_tf_generic_smc(u32 type, u32 subtype, > + u32 arg) > +{ > + asm volatile( > + ".arch_extension sec\n\t" > + "stmfd sp!, {r3 - r11, lr}\n\t" I think you don't need to save r3: the ARM PCS makes r3 caller-save, so it shouldn't matter if the SMC call causes it to get trashed. > + __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!, {r3 - r11, pc}" > + : > + : "r" (type), "r" (subtype), "r" (arg) > + : "memory"); > +} > + > +static int tegra_tf_set_cpu_boot_addr(int cpu, unsigned long boot_addr) > +{ > + tegra_tf_generic_smc(TF_SET_CPU_BOOT_ADDR_SMC, boot_addr, 0); > + > + return 0; > +} > + > +const struct firmware_ops tegra_trusted_foundations_ops = { > + .set_cpu_boot_addr = tegra_tf_set_cpu_boot_addr, > +}; > -- > 1.8.3 > > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/13/2013 03:12 AM, Alexandre Courbot wrote: > Add basic support for booting secondary processors on Tegra devices > using the Trusted Foundations secure monitor. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > --- > Documentation/devicetree/bindings/arm/tegra.txt | 11 +++++ > .../devicetree/bindings/vendor-prefixes.txt | 1 + > arch/arm/configs/tegra_defconfig | 1 + The defconfig change should be a separate patch, so that I can squash it into any other defconfig updates separately from all the code changes. > diff --git a/arch/arm/mach-tegra/firmware.c b/arch/arm/mach-tegra/firmware.c > +void __init tegra_init_firmware(void) > +{ > + struct device_node *node; > + > + if (!of_have_populated_dt()) > + return; > + > + node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations"); > + if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)) > + pr_warn("Trusted Foundations detected but support missing!\n"); > +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS) > + else if (node) > + register_firmware_ops(&tegra_trusted_foundations_ops); > +#endif > +} Is it worth continuing on in the node && !IS_ENABLED case here? After all, we can be pretty certain that the write to the CPU reset vector is immediately going to trap... I suppose that perhaps without SMP, cpuidle, suspend, ... we could keep running, but that seems a little niche. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 14, 2013 at 4:19 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 06/13/2013 03:12 AM, Alexandre Courbot wrote: >> Add basic support for booting secondary processors on Tegra devices >> using the Trusted Foundations secure monitor. >> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> >> --- >> Documentation/devicetree/bindings/arm/tegra.txt | 11 +++++ >> .../devicetree/bindings/vendor-prefixes.txt | 1 + >> arch/arm/configs/tegra_defconfig | 1 + > > The defconfig change should be a separate patch, so that I can squash it > into any other defconfig updates separately from all the code changes. Ok, moved that part into its own patch. >> diff --git a/arch/arm/mach-tegra/firmware.c b/arch/arm/mach-tegra/firmware.c > >> +void __init tegra_init_firmware(void) >> +{ >> + struct device_node *node; >> + >> + if (!of_have_populated_dt()) >> + return; >> + >> + node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations"); >> + if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)) >> + pr_warn("Trusted Foundations detected but support missing!\n"); >> +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS) >> + else if (node) >> + register_firmware_ops(&tegra_trusted_foundations_ops); >> +#endif >> +} > > Is it worth continuing on in the node && !IS_ENABLED case here? After > all, we can be pretty certain that the write to the CPU reset vector is > immediately going to trap... That's what was happening until 3.9, but from 3.10 on the trap is apparently handled and the boot completes (although with only one processor). > I suppose that perhaps without SMP, cpuidle, suspend, ... we could keep > running, but that seems a little niche. If we can keep running, even in degraded mode, I see no reason to panic. The problem is well reported in the kernel log, and having a running system might be helpful to analyze the issue. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 13, 2013 at 11:35 PM, Dave Martin <Dave.Martin@arm.com> wrote: >> diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt >> index ed9c853..d59bc19 100644 >> --- a/Documentation/devicetree/bindings/arm/tegra.txt >> +++ b/Documentation/devicetree/bindings/arm/tegra.txt >> @@ -32,3 +32,14 @@ board-specific compatible values: >> nvidia,whistler >> toradex,colibri_t20-512 >> toradex,iris >> + >> +Optional nodes >> +------------------------------------------- >> + >> +Trusted Foundations: >> +Boards using the Trusted Foundations secure monitor should signal its >> +presence by declaring a node compatible with "tl,trusted-foundations": >> + >> + firmware { > > You need to make a clear statement about whether the node name is > > I think it shouldn't required to be exactly equal to "firmware", because > that would lead to problems if there were multiple independent firmware > APIs present (which is certainly possible, whether or not it is true > on Tegra). Yes, the name of the node is not fixed (doing so would make its lookup faster, but the gain is not obvious). Will make it explicit in the doc. >> + compatible = "tl,trusted-foundations"; >> + }; > > For now, it might make more sense to make this binding tegra-specific, > and to interpret the node is only implying the presence of the low- > level SoC functions you are using on Tegra, not TF as a whole. Do you mean the vendor should be changed from "tl" to "nvidia" here? > Otherwise, it feels too generic. There is no description of exactly > what functionality will be available if this node it present: if > this is going to be a generic binding for TF, then it needs to work > for all deployments of TF, not just your specific case. For example, > how to you find out what functionality is present? Will there be > a standard probing ABI for all versions and deployments of TF, or > would extra information need to be described in the DT? > > Partly, this depends on whether the TF_SET_CPU_BOOT_ADDR_SMC call will > be present, working, and with compatible ABI and semantics, on every SoC > where an implementation of TF is present. Someone from Trusted Logic, or > someone with visibility of the relevant ABI/API specs would need to > judge on that -- do you have that info? I'm currently looking into that. This patch makes the assumption that all TF implementations have the same features and the same interface - if this is the case, do you agree this binding is ok as it is? If indeed TF's functionality and ABI is the same no matter whether we are on Tegra on not, then its support should even be moved outside of mach-tegra. >> --- /dev/null >> +++ b/arch/arm/mach-tegra/firmware.c >> @@ -0,0 +1,40 @@ >> +/* >> + * SecureOS support for Tegra CPUs > > Should the name "SecureOS" change in these comment headers? (affects > multiple files) Yes, I missed these ones, thanks. Another missed opportunity to use git grep... >> + node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations"); >> + if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)) >> + pr_warn("Trusted Foundations detected but support missing!\n"); > > Should this be more than just a warning? > > It looks to me like tegra_cpu_reset_handler_set() might either silently > fail or trigger an external abort in this situation, depending on the > hardware and on how TF sets things up. What will happen (from 3.10) is that tegra_cpu_reset_handler_set() will output a "CPUX: failed to come online" for each secondary CPU and boot will continue (albeit on one CPU). The system's features are degraded, but it is not fatal, so I think it is reasonable to continue here. > There seems to be no way to signal an error when attempting to set a > CPU's reset address. Indeed. But even if that fails the system can still survive, at least on Tegra. >> +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS) >> + else if (node) >> + register_firmware_ops(&tegra_trusted_foundations_ops); >> +#endif >> +} > > > IS_ENABLED() allows #ifdefs to be avoided -- maybe this would be clearer > as: > > node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations"); > if (!node) > return; > > if (WARN_ON(!IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))) { > pr_warn("Trusted Foundations detected but support missing!\n"); > return; > } > > register_firmware_ops(&tegra_trusted_foundations_ops); But then you will get a link error when TF support is not compiled in because tegra_trusted_foundations_ops will not be defined. That's why I used a #ifdef here - I agree it is terribly inelegant though. >> + asm volatile( >> + ".arch_extension sec\n\t" >> + "stmfd sp!, {r3 - r11, lr}\n\t" > > I think you don't need to save r3: the ARM PCS makes r3 caller-save, so > it shouldn't matter if the SMC call causes it to get trashed. One less register to save, I take it! :) Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/14/2013 02:27 AM, Alexandre Courbot wrote: > On Fri, Jun 14, 2013 at 4:19 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 06/13/2013 03:12 AM, Alexandre Courbot wrote: >>> Add basic support for booting secondary processors on Tegra devices >>> using the Trusted Foundations secure monitor. >>> diff --git a/arch/arm/mach-tegra/firmware.c b/arch/arm/mach-tegra/firmware.c >> >>> +void __init tegra_init_firmware(void) ... >>> + node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations"); >>> + if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)) >>> + pr_warn("Trusted Foundations detected but support missing!\n"); >>> +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS) >>> + else if (node) >>> + register_firmware_ops(&tegra_trusted_foundations_ops); >>> +#endif >>> +} >> >> Is it worth continuing on in the node && !IS_ENABLED case here? After >> all, we can be pretty certain that the write to the CPU reset vector is >> immediately going to trap... > > That's what was happening until 3.9, but from 3.10 on the trap is > apparently handled and the boot completes (although with only one > processor). Why does that happen; surely the kernel shouldn't be ignoring failures? How does it recover? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/14/2013 02:43 AM, Alexandre Courbot wrote: > On Thu, Jun 13, 2013 at 11:35 PM, Dave Martin <Dave.Martin@arm.com> wrote: ... >>> + compatible = "tl,trusted-foundations"; >>> + }; >> >> For now, it might make more sense to make this binding tegra-specific, >> and to interpret the node is only implying the presence of the low- >> level SoC functions you are using on Tegra, not TF as a whole. > > Do you mean the vendor should be changed from "tl" to "nvidia" here? > >> Otherwise, it feels too generic. There is no description of exactly >> what functionality will be available if this node it present: if >> this is going to be a generic binding for TF, then it needs to work >> for all deployments of TF, not just your specific case. For example, >> how to you find out what functionality is present? Will there be >> a standard probing ABI for all versions and deployments of TF, or >> would extra information need to be described in the DT? >> >> Partly, this depends on whether the TF_SET_CPU_BOOT_ADDR_SMC call will >> be present, working, and with compatible ABI and semantics, on every SoC >> where an implementation of TF is present. Someone from Trusted Logic, or >> someone with visibility of the relevant ABI/API specs would need to >> judge on that -- do you have that info? > > I'm currently looking into that. This patch makes the assumption that > all TF implementations have the same features and the same interface - > if this is the case, do you agree this binding is ok as it is? > > If indeed TF's functionality and ABI is the same no matter whether we > are on Tegra on not, then its support should even be moved outside of > mach-tegra. I expect we at least need a version number in the compatible value, even if we don't need a representation of the SoC or vendor for which the ABI was built. >>> + node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations"); >>> + if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)) >>> + pr_warn("Trusted Foundations detected but support missing!\n"); >> >> Should this be more than just a warning? >> >> It looks to me like tegra_cpu_reset_handler_set() might either silently >> fail or trigger an external abort in this situation, depending on the >> hardware and on how TF sets things up. > > What will happen (from 3.10) is that tegra_cpu_reset_handler_set() > will output a "CPUX: failed to come online" for each secondary CPU and > boot will continue (albeit on one CPU). The system's features are > degraded, but it is not fatal, so I think it is reasonable to continue > here. > >> There seems to be no way to signal an error when attempting to set a >> CPU's reset address. > > Indeed. But even if that fails the system can still survive, at least on Tegra. One more thought: Setting the CPU reset address isn't the only operation that'll be performed via firmware_ops; we'd also need to e.g. disable CPU power-gating and perhaps other things. Can that all be done at run-time? I guess it shouldn't be hard to fix that if we can't yet. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 14, 2013 at 05:43:03PM +0900, Alexandre Courbot wrote: > On Thu, Jun 13, 2013 at 11:35 PM, Dave Martin <Dave.Martin@arm.com> wrote: > >> diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt > >> index ed9c853..d59bc19 100644 > >> --- a/Documentation/devicetree/bindings/arm/tegra.txt > >> +++ b/Documentation/devicetree/bindings/arm/tegra.txt > >> @@ -32,3 +32,14 @@ board-specific compatible values: > >> nvidia,whistler > >> toradex,colibri_t20-512 > >> toradex,iris > >> + > >> +Optional nodes > >> +------------------------------------------- > >> + > >> +Trusted Foundations: > >> +Boards using the Trusted Foundations secure monitor should signal its > >> +presence by declaring a node compatible with "tl,trusted-foundations": > >> + > >> + firmware { > > > > You need to make a clear statement about whether the node name is > > > > I think it shouldn't required to be exactly equal to "firmware", because > > that would lead to problems if there were multiple independent firmware > > APIs present (which is certainly possible, whether or not it is true > > on Tegra). > > Yes, the name of the node is not fixed (doing so would make its lookup > faster, but the gain is not obvious). Will make it explicit in the > doc. > > >> + compatible = "tl,trusted-foundations"; > >> + }; > > > > For now, it might make more sense to make this binding tegra-specific, > > and to interpret the node is only implying the presence of the low- > > level SoC functions you are using on Tegra, not TF as a whole. > > Do you mean the vendor should be changed from "tl" to "nvidia" here? Since this is a Tegra-specific integration, I think that would be wise, unless you can confirm by looking at the API specs that the functionality you are trying to describe and use it truly intended to be generic across all deployments of TF (either always present, or at least as a well- specified optional feature). > > Otherwise, it feels too generic. There is no description of exactly > > what functionality will be available if this node it present: if > > this is going to be a generic binding for TF, then it needs to work > > for all deployments of TF, not just your specific case. For example, > > how to you find out what functionality is present? Will there be > > a standard probing ABI for all versions and deployments of TF, or > > would extra information need to be described in the DT? > > > > Partly, this depends on whether the TF_SET_CPU_BOOT_ADDR_SMC call will > > be present, working, and with compatible ABI and semantics, on every SoC > > where an implementation of TF is present. Someone from Trusted Logic, or > > someone with visibility of the relevant ABI/API specs would need to > > judge on that -- do you have that info? > > I'm currently looking into that. This patch makes the assumption that > all TF implementations have the same features and the same interface - > if this is the case, do you agree this binding is ok as it is? It's a judgement call, but if the SMC you use is a mandatory part of TF after a certain version, and if it's guaranteed to have a fixed ABI (including function ID) and behaviour, then that binding works. If the function is an optional or SoC-specific feature, then it's not sufficient to say that TF firmware is present -- we need to describe something about the SoC-specific and optional features present, unless there's a well-defined generic way to probe for them. > If indeed TF's functionality and ABI is the same no matter whether we > are on Tegra on not, then its support should even be moved outside of > mach-tegra. Agreed, that's the ideal outcome. I'm just not convinced we're ready for that just now (but I'm happy to be corrected). > >> --- /dev/null > >> +++ b/arch/arm/mach-tegra/firmware.c > >> @@ -0,0 +1,40 @@ > >> +/* > >> + * SecureOS support for Tegra CPUs > > > > Should the name "SecureOS" change in these comment headers? (affects > > multiple files) > > Yes, I missed these ones, thanks. Another missed opportunity to use git grep... > > >> + node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations"); > >> + if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)) > >> + pr_warn("Trusted Foundations detected but support missing!\n"); > > > > Should this be more than just a warning? > > > > It looks to me like tegra_cpu_reset_handler_set() might either silently > > fail or trigger an external abort in this situation, depending on the > > hardware and on how TF sets things up. > > What will happen (from 3.10) is that tegra_cpu_reset_handler_set() > will output a "CPUX: failed to come online" for each secondary CPU and > boot will continue (albeit on one CPU). The system's features are > degraded, but it is not fatal, so I think it is reasonable to continue > here. I don't think we should rely on ignoring in imprecise external abort, because such aborts indicate serious or potentially fatal system errors or bugs. If you're getting a precise abort (so that we know the faulted address) or no abort at all, that's less harmful, though it's hard to guarantee across all SoCs, because the ARM Architecture doesn't guarantee synchronous aborts for slave errors on writes. What was heppening differently on 3.9 compared with 3.10? > > There seems to be no way to signal an error when attempting to set a > > CPU's reset address. > > Indeed. But even if that fails the system can still survive, at least on Tegra. > > >> +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS) > >> + else if (node) > >> + register_firmware_ops(&tegra_trusted_foundations_ops); > >> +#endif > >> +} > > > > > > IS_ENABLED() allows #ifdefs to be avoided -- maybe this would be clearer > > as: > > > > node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations"); > > if (!node) > > return; > > > > if (WARN_ON(!IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))) { > > pr_warn("Trusted Foundations detected but support missing!\n"); > > return; > > } > > > > register_firmware_ops(&tegra_trusted_foundations_ops); > > But then you will get a link error when TF support is not compiled in > because tegra_trusted_foundations_ops will not be defined. That's why > I used a #ifdef here - I agree it is terribly inelegant though. Hmmm, fair point. Dead code elimination in the compiler may solve that, but I've never been too keen on relying on that. Cheers ---Dave -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt index ed9c853..d59bc19 100644 --- a/Documentation/devicetree/bindings/arm/tegra.txt +++ b/Documentation/devicetree/bindings/arm/tegra.txt @@ -32,3 +32,14 @@ board-specific compatible values: nvidia,whistler toradex,colibri_t20-512 toradex,iris + +Optional nodes +------------------------------------------- + +Trusted Foundations: +Boards using the Trusted Foundations secure monitor should signal its +presence by declaring a node compatible with "tl,trusted-foundations": + + firmware { + compatible = "tl,trusted-foundations"; + }; diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt index 6931c43..c21e1e9 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt @@ -58,6 +58,7 @@ st STMicroelectronics ste ST-Ericsson stericsson ST-Ericsson ti Texas Instruments +tl Trusted Logic toshiba Toshiba Corporation via VIA Technologies, Inc. wlf Wolfson Microelectronics diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig index 4de3bce..3bf158a 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_TEGRA_TRUSTED_FOUNDATIONS=y CONFIG_SMP=y CONFIG_PREEMPT=y CONFIG_AEABI=y diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig index 84d72fc..055f496 100644 --- a/arch/arm/mach-tegra/Kconfig +++ b/arch/arm/mach-tegra/Kconfig @@ -87,4 +87,18 @@ config TEGRA_AHB config TEGRA_EMC_SCALING_ENABLE bool "Enable scaling the memory frequency" +config TEGRA_TRUSTED_FOUNDATIONS + bool "Trusted Foundations secure monitor support" + help + Many Tegra devices 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 when + required on devices using Trusted Foundations. + + Devices using Trusted Foundations should pass a device tree containing + a node compatible with "tl,trusted-foundations" to signal the presence + of the secure monitor. + endmenu diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile index d011f0a..9fdc004 100644 --- a/arch/arm/mach-tegra/Makefile +++ b/arch/arm/mach-tegra/Makefile @@ -12,6 +12,7 @@ obj-y += pm.o obj-y += reset.o obj-y += reset-handler.o obj-y += sleep.o +obj-y += firmware.o obj-y += tegra.o obj-$(CONFIG_CPU_IDLE) += cpuidle.o obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += tegra20_speedo.o @@ -37,3 +38,5 @@ endif obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += board-harmony-pcie.o obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += board-paz00.o + +obj-$(CONFIG_TEGRA_TRUSTED_FOUNDATIONS) += trusted_foundations.o diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c index 9f852c6..4796bb0 100644 --- a/arch/arm/mach-tegra/common.c +++ b/arch/arm/mach-tegra/common.c @@ -37,6 +37,7 @@ #include "sleep.h" #include "pm.h" #include "reset.h" +#include "firmware.h" /* * Storage for debug-macro.S's state. @@ -97,6 +98,7 @@ static void __init tegra_init_cache(void) void __init tegra_init_early(void) { + tegra_init_firmware(); tegra_cpu_reset_handler_init(); tegra_apb_io_init(); tegra_init_fuse(); diff --git a/arch/arm/mach-tegra/firmware.c b/arch/arm/mach-tegra/firmware.c new file mode 100644 index 0000000..ea861ca --- /dev/null +++ b/arch/arm/mach-tegra/firmware.c @@ -0,0 +1,40 @@ +/* + * SecureOS support for Tegra 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/printk.h> +#include <linux/kconfig.h> +#include <linux/of.h> +#include <asm/firmware.h> + +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS) +extern const struct firmware_ops tegra_trusted_foundations_ops; +#endif + +void __init tegra_init_firmware(void) +{ + struct device_node *node; + + if (!of_have_populated_dt()) + return; + + node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations"); + if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)) + pr_warn("Trusted Foundations detected but support missing!\n"); +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS) + else if (node) + register_firmware_ops(&tegra_trusted_foundations_ops); +#endif +} diff --git a/arch/arm/mach-tegra/firmware.h b/arch/arm/mach-tegra/firmware.h new file mode 100644 index 0000000..77c62fb --- /dev/null +++ b/arch/arm/mach-tegra/firmware.h @@ -0,0 +1,19 @@ +/* + * Copyright (c) 2013, NVIDIA Corporation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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. + */ + +#ifndef __TEGRA_FIRMWARE_H +#define __TEGRA_FIRMWARE_H + +void tegra_init_firmware(void); + +#endif diff --git a/arch/arm/mach-tegra/trusted_foundations.c b/arch/arm/mach-tegra/trusted_foundations.c new file mode 100644 index 0000000..411044f --- /dev/null +++ b/arch/arm/mach-tegra/trusted_foundations.c @@ -0,0 +1,51 @@ +/* + * SecureOS support for Tegra 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> + +#define TF_SET_CPU_BOOT_ADDR_SMC 0xfffff200 + +static void __attribute__((naked)) tegra_tf_generic_smc(u32 type, u32 subtype, + u32 arg) +{ + asm volatile( + ".arch_extension sec\n\t" + "stmfd sp!, {r3 - 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!, {r3 - r11, pc}" + : + : "r" (type), "r" (subtype), "r" (arg) + : "memory"); +} + +static int tegra_tf_set_cpu_boot_addr(int cpu, unsigned long boot_addr) +{ + tegra_tf_generic_smc(TF_SET_CPU_BOOT_ADDR_SMC, boot_addr, 0); + + return 0; +} + +const struct firmware_ops tegra_trusted_foundations_ops = { + .set_cpu_boot_addr = tegra_tf_set_cpu_boot_addr, +};
Add basic support for booting secondary processors on Tegra devices using the Trusted Foundations secure monitor. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- Documentation/devicetree/bindings/arm/tegra.txt | 11 +++++ .../devicetree/bindings/vendor-prefixes.txt | 1 + arch/arm/configs/tegra_defconfig | 1 + arch/arm/mach-tegra/Kconfig | 14 ++++++ arch/arm/mach-tegra/Makefile | 3 ++ arch/arm/mach-tegra/common.c | 2 + arch/arm/mach-tegra/firmware.c | 40 +++++++++++++++++ arch/arm/mach-tegra/firmware.h | 19 ++++++++ arch/arm/mach-tegra/trusted_foundations.c | 51 ++++++++++++++++++++++ 9 files changed, 142 insertions(+) create mode 100644 arch/arm/mach-tegra/firmware.c create mode 100644 arch/arm/mach-tegra/firmware.h create mode 100644 arch/arm/mach-tegra/trusted_foundations.c