| Message ID | 1370503687-17767-1-git-send-email-acourbot@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 06, 2013 at 04:28:07PM +0900, Alexandre Courbot wrote: > +static int __attribute__((used)) __tegra_smc_stack[10]; > + > +/* > + * With EABI, subtype and arg already end up in r0, r1 and r2 as they are > + * function arguments, but we prefer to play safe here and explicitly move > + * these values into the expected registers anyway. mov instructions without > + * any side-effect are turned into nops by the assembler, which limits > + * overhead. No they aren't. They will be preserved as: mov r0, r0 mov r1, r1 mov r2, r2 Moreover, things will go wrong if the compiler decides for whatever reason to move 'arg' into r0 before calling your code sequence. So really this is quite fragile. There's also no point in mentioning EABI in the above paragraph; all ARM ABIs under Linux have always placed the arguments in r0..r3 and then on the stack. You can assert that this is always true by using the __asmeq() macro. Also... > + */ > +static void tegra_generic_smc(u32 type, u32 subtype, u32 arg) > +{ > + asm volatile( > + ".arch_extension sec\n\t" > + "ldr r3, =__tegra_smc_stack\n\t" > + "stmia r3, {r4-r12, lr}\n\t" using a statically allocated area to save the registers in this way is probably a bad move; although the hotplug code guarantees that there will be no concurrency between CPU hotplug operations, this sets a precident for it being used in places where no such protection exists. What is wrong with stacking r4-r12, lr onto the SVC stack? You don't save the SVC stack pointer, so one can only assume that your SMC implmentation preserves this (if it doesn't, that's yet another bug with this assembly code.) Combining these two issues, you're probably far better off using an __attribute__((naked)) function for this - which means you get to write the entire function in assembly code without any compiler interference: static void __attribute__((naked)) tegra_generic_smc(u32 type, u32 subtype, u32 arg) { asm volatile( ".arch_extension sec\n\t" "stmfd sp!, {r4 - r12, lr}\n\t" __asmeq("%0", "r0") __asmeq("%1", "r1") __asmeq("%2", "r2") "mov r3, #0\n\t" "mov r4, #0\n\t" "dsb\n\t" "smc #0\n\t" "ldmfd sp!, {r4 - r12, pc}" : : "r" (type), "r" (subtype), "r" (arg) : "memory"); }
Hi Alex, On Thursday 06 of June 2013 16:28:07 Alexandre Courbot wrote: > Boot loaders on some Tegra devices can be unlocked but do not let the > system operate without SecureOS. SecureOS prevents access to some > registers and requires the operating system to perform certain > operations through Secure Monitor Calls instead of directly accessing > the hardware. > > This patch introduces basic SecureOS support for Tegra. SecureOS support > can be enabled by adding a "nvidia,secure-os" property to the "chosen" > node of the device tree. > > Currently, only the bringup of secondary CPUs is performed by SMCs, but > more operations will be added later. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > --- > Documentation/devicetree/bindings/arm/tegra.txt | 8 +++ > arch/arm/configs/tegra_defconfig | 1 + > arch/arm/mach-tegra/Kconfig | 11 ++++ > arch/arm/mach-tegra/Makefile | 2 + > arch/arm/mach-tegra/common.c | 2 + > arch/arm/mach-tegra/reset.c | 30 +++++++---- > arch/arm/mach-tegra/secureos.c | 70 > +++++++++++++++++++++++++ arch/arm/mach-tegra/secureos.h > | 31 +++++++++++ 8 files changed, 145 insertions(+), 10 deletions(-) > create mode 100644 arch/arm/mach-tegra/secureos.c > create mode 100644 arch/arm/mach-tegra/secureos.h > > diff --git a/Documentation/devicetree/bindings/arm/tegra.txt > b/Documentation/devicetree/bindings/arm/tegra.txt index > ed9c853..b543091 100644 > --- a/Documentation/devicetree/bindings/arm/tegra.txt > +++ b/Documentation/devicetree/bindings/arm/tegra.txt > @@ -32,3 +32,11 @@ board-specific compatible values: > nvidia,whistler > toradex,colibri_t20-512 > toradex,iris > + > +Global properties > +------------------------------------------- > + > +The following properties can be specified into the "chosen" root > +node: > + > + nvidia,secure-os: enable SecureOS. Hmm, on Exynos we had something like firmware@0203F000 { compatible = "samsung,secure-firmware"; reg = <0x0203F000 0x1000>; }; but your solution might be actually the proper one, since firmware is not a hardware block. (The address in reg property is pointing to SYSRAM memory, which is an additional communication channel with the firmware.) > diff --git a/arch/arm/configs/tegra_defconfig > b/arch/arm/configs/tegra_defconfig index f7ba3161..f6ed0f5 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_SECUREOS=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..acb5d0a 100644 > --- a/arch/arm/mach-tegra/Kconfig > +++ b/arch/arm/mach-tegra/Kconfig > @@ -87,4 +87,15 @@ config TEGRA_AHB > config TEGRA_EMC_SCALING_ENABLE > bool "Enable scaling the memory frequency" > > +config TEGRA_SECUREOS > + bool "Enable SecureOS support" > + help > + Support for Tegra devices which bootloader sets up a > + SecureOS environment. This will use Secure Monitor Calls > + instead of directly accessing the hardware for some protected > + operations. > + > + SecureOS support is enabled by declaring a "nvidia,secure-os" > + property into the "chosen" node of the device tree. > + > endmenu > diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile > index d011f0a..3adafe6 100644 > --- a/arch/arm/mach-tegra/Makefile > +++ b/arch/arm/mach-tegra/Makefile > @@ -37,3 +37,5 @@ endif > obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += board-harmony-pcie.o > > obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += board-paz00.o > + > +obj-$(CONFIG_TEGRA_SECUREOS) += secureos.o > diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c > index 9f852c6..b7eea02 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 "secureos.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_secureos(); > tegra_cpu_reset_handler_init(); > tegra_apb_io_init(); > tegra_init_fuse(); > diff --git a/arch/arm/mach-tegra/reset.c b/arch/arm/mach-tegra/reset.c > index 1ac434e..4b9ebf9 100644 > --- a/arch/arm/mach-tegra/reset.c > +++ b/arch/arm/mach-tegra/reset.c > @@ -21,38 +21,32 @@ > > #include <asm/cacheflush.h> > #include <asm/hardware/cache-l2x0.h> > +#include <asm/firmware.h> > > #include "iomap.h" > #include "irammap.h" > #include "reset.h" > #include "sleep.h" > #include "fuse.h" > +#include "secureos.h" > > #define TEGRA_IRAM_RESET_BASE (TEGRA_IRAM_BASE + \ > TEGRA_IRAM_RESET_HANDLER_OFFSET) > > 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 +60,22 @@ 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); > + > + if (call_firmware_op(set_cpu_boot_addr, 0, reset_address) == - ENOSYS) > + tegra_cpu_reset_handler_set(reset_address); > > is_enabled = true; > } I think this patch could be split into several patches: - add support for firmware - split reset function - add reset support using firmware. > diff --git a/arch/arm/mach-tegra/secureos.c > b/arch/arm/mach-tegra/secureos.c new file mode 100644 > index 0000000..44c3514 > --- /dev/null > +++ b/arch/arm/mach-tegra/secureos.c > @@ -0,0 +1,70 @@ > +/* > + * 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> > + > +static int __attribute__((used)) __tegra_smc_stack[10]; > + > +/* > + * With EABI, subtype and arg already end up in r0, r1 and r2 as they > are + * function arguments, but we prefer to play safe here and > explicitly move + * these values into the expected registers anyway. > mov instructions without + * any side-effect are turned into nops by > the assembler, which limits + * overhead. > + */ > +static void tegra_generic_smc(u32 type, u32 subtype, u32 arg) > +{ > + asm volatile( > + ".arch_extension sec\n\t" > + "ldr r3, =__tegra_smc_stack\n\t" > + "stmia r3, {r4-r12, lr}\n\t" > + "mov r0, %[type]\n\t" > + "mov r1, %[subtype]\n\t" > + "mov r2, %[arg]\n\t" > + "mov r3, #0\n\t" > + "mov r4, #0\n\t" > + "dsb\n\t" > + "smc #0\n\t" > + "ldr r3, =__tegra_smc_stack\n\t" > + "ldmia r3, {r4-r12, lr}" > + : > + : [type] "r" (type), > + [subtype] "r" (subtype), > + [arg] "r" (arg) > + : "r0", "r1", "r2", "r3", "r4", "memory"); > +} Hmm, I wonder if you need all this complexity here. Have a look at our exynos_smc function https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-exynos/exynos-smc.S?id=refs/tags/next-20130606 and feel free to uncover any problems of it ;) . > + > +static int tegra_set_cpu_boot_addr(int cpu, unsigned long boot_addr) > +{ > + tegra_generic_smc(0xfffff200, boot_addr, 0); > + > + return 0; > +} > + > +static const struct firmware_ops tegra_firmware_ops = { > + .set_cpu_boot_addr = tegra_set_cpu_boot_addr, > +}; It's good that this interface is finally getting some user besides Exynos. Best regards, Tomasz > +void __init tegra_init_secureos(void) > +{ > + struct device_node *node = of_find_node_by_path("/chosen"); > + > + if (node && of_property_read_bool(node, "nvidia,secure-os")) > + register_firmware_ops(&tegra_firmware_ops); > +} > diff --git a/arch/arm/mach-tegra/secureos.h > b/arch/arm/mach-tegra/secureos.h new file mode 100644 > index 0000000..5388cc5 > --- /dev/null > +++ b/arch/arm/mach-tegra/secureos.h > @@ -0,0 +1,31 @@ > +/* > + * 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_SECUREOS_H > +#define __TEGRA_SECUREOS_H > + > +#ifdef CONFIG_TEGRA_SECUREOS > + > +#include <linux/types.h> > + > +void tegra_init_secureos(void); > + > +#else > + > +static inline void tegra_init_secureos(void) > +{ > +} > + > +#endif > + > +#endif
On 06/06/2013 06:35 PM, Russell King - ARM Linux wrote: > On Thu, Jun 06, 2013 at 04:28:07PM +0900, Alexandre Courbot wrote: >> +static int __attribute__((used)) __tegra_smc_stack[10]; >> + >> +/* >> + * With EABI, subtype and arg already end up in r0, r1 and r2 as they are >> + * function arguments, but we prefer to play safe here and explicitly move >> + * these values into the expected registers anyway. mov instructions without >> + * any side-effect are turned into nops by the assembler, which limits >> + * overhead. > > No they aren't. They will be preserved as: > mov r0, r0 > mov r1, r1 > mov r2, r2 I'm pretty sure I checked with objdump and saw these replaced by nops at some point, but for some reason I cannot get that behavior again. So simply put, my statement is wrong indeed. > Moreover, things will go wrong if the compiler decides for whatever reason > to move 'arg' into r0 before calling your code sequence. So really this > is quite fragile. > > There's also no point in mentioning EABI in the above paragraph; all ARM > ABIs under Linux have always placed the arguments in r0..r3 and then on > the stack. You can assert that this is always true by using the __asmeq() > macro. Good to know, thanks. > Also... > >> + */ >> +static void tegra_generic_smc(u32 type, u32 subtype, u32 arg) >> +{ >> + asm volatile( >> + ".arch_extension sec\n\t" >> + "ldr r3, =__tegra_smc_stack\n\t" >> + "stmia r3, {r4-r12, lr}\n\t" > > using a statically allocated area to save the registers in this way is > probably a bad move; although the hotplug code guarantees that there > will be no concurrency between CPU hotplug operations, this sets a > precident for it being used in places where no such protection exists. Indeed. This function will be called from other places in the future, and for these we cannot assume there will be no concurrency. > What is wrong with stacking r4-r12, lr onto the SVC stack? Nothing, actually. /o\ > You don't > save the SVC stack pointer, so one can only assume that your SMC > implmentation preserves this (if it doesn't, that's yet another bug > with this assembly code.) SVC stack pointer is ok AFAICT. > Combining these two issues, you're probably far better off using an > __attribute__((naked)) function for this - which means you get to > write the entire function in assembly code without any compiler > interference: > > static void __attribute__((naked)) tegra_generic_smc(u32 type, u32 subtype, u32 arg) > { > asm volatile( > ".arch_extension sec\n\t" > "stmfd sp!, {r4 - r12, lr}\n\t" > __asmeq("%0", "r0") > __asmeq("%1", "r1") > __asmeq("%2", "r2") > "mov r3, #0\n\t" > "mov r4, #0\n\t" > "dsb\n\t" > "smc #0\n\t" > "ldmfd sp!, {r4 - r12, pc}" > : > : "r" (type), "r" (subtype), "r" (arg) > : "memory"); > } Well, that works beautifully indeed, and results in a much smaller function that does nothing more beyond what's needed. On top of that, I feel enlightened. Thanks, I will resubmit a fixed version soon. Alex.
Hi Tomasz, On 06/06/2013 07:17 PM, Tomasz Figa wrote: >> +Global properties >> +------------------------------------------- >> + >> +The following properties can be specified into the "chosen" root >> +node: >> + >> + nvidia,secure-os: enable SecureOS. > > Hmm, on Exynos we had something like > > firmware@0203F000 { > compatible = "samsung,secure-firmware"; > reg = <0x0203F000 0x1000>; > }; > > but your solution might be actually the proper one, since firmware is not > a hardware block. (The address in reg property is pointing to SYSRAM > memory, which is an additional communication channel with the firmware.) Yes, I saw your implementation but decided to do it through the chosen node anyway, since that's what it seems to be designed and we don't need any reg parameter. > I think this patch could be split into several patches: > - add support for firmware > - split reset function > - add reset support using firmware. Mmm possibly yes, but I wonder if that would not be too much splitting. Stephen? > Hmm, I wonder if you need all this complexity here. Have a look at our > exynos_smc function > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-exynos/exynos-smc.S?id=refs/tags/next-20130606 Yes, I just embarrassed myself showing my ignorance of ARM assembler. ;) The fix Russel proposed is pretty close to your version. >> +static const struct firmware_ops tegra_firmware_ops = { >> + .set_cpu_boot_addr = tegra_set_cpu_boot_addr, >> +}; > > It's good that this interface is finally getting some user besides Exynos. I didn't know about it first but Joseph kindly pointed it out to me and it indeed makes it easier to implement this. Thanks, Alex.
On Thu, Jun 06, 2013 at 04:28:07PM +0900, Alexandre Courbot wrote: > Boot loaders on some Tegra devices can be unlocked but do not let the > system operate without SecureOS. SecureOS prevents access to some > registers and requires the operating system to perform certain > operations through Secure Monitor Calls instead of directly accessing > the hardware. > > This patch introduces basic SecureOS support for Tegra. SecureOS support > can be enabled by adding a "nvidia,secure-os" property to the "chosen" > node of the device tree. > > Currently, only the bringup of secondary CPUs is performed by SMCs, but > more operations will be added later. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > --- > Documentation/devicetree/bindings/arm/tegra.txt | 8 +++ > arch/arm/configs/tegra_defconfig | 1 + > arch/arm/mach-tegra/Kconfig | 11 ++++ > arch/arm/mach-tegra/Makefile | 2 + > arch/arm/mach-tegra/common.c | 2 + > arch/arm/mach-tegra/reset.c | 30 +++++++---- > arch/arm/mach-tegra/secureos.c | 70 +++++++++++++++++++++++++ > arch/arm/mach-tegra/secureos.h | 31 +++++++++++ > 8 files changed, 145 insertions(+), 10 deletions(-) > create mode 100644 arch/arm/mach-tegra/secureos.c > create mode 100644 arch/arm/mach-tegra/secureos.h > > diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt > index ed9c853..b543091 100644 > --- a/Documentation/devicetree/bindings/arm/tegra.txt > +++ b/Documentation/devicetree/bindings/arm/tegra.txt > @@ -32,3 +32,11 @@ board-specific compatible values: > nvidia,whistler > toradex,colibri_t20-512 > toradex,iris > + > +Global properties > +------------------------------------------- > + > +The following properties can be specified into the "chosen" root > +node: > + > + nvidia,secure-os: enable SecureOS. > diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig > index f7ba3161..f6ed0f5 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_SECUREOS=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..acb5d0a 100644 > --- a/arch/arm/mach-tegra/Kconfig > +++ b/arch/arm/mach-tegra/Kconfig > @@ -87,4 +87,15 @@ config TEGRA_AHB > config TEGRA_EMC_SCALING_ENABLE > bool "Enable scaling the memory frequency" > > +config TEGRA_SECUREOS > + bool "Enable SecureOS support" > + help > + Support for Tegra devices which bootloader sets up a > + SecureOS environment. This will use Secure Monitor Calls > + instead of directly accessing the hardware for some protected > + operations. > + > + SecureOS support is enabled by declaring a "nvidia,secure-os" > + property into the "chosen" node of the device tree. > + > endmenu > diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile > index d011f0a..3adafe6 100644 > --- a/arch/arm/mach-tegra/Makefile > +++ b/arch/arm/mach-tegra/Makefile > @@ -37,3 +37,5 @@ endif > obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += board-harmony-pcie.o > > obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += board-paz00.o > + > +obj-$(CONFIG_TEGRA_SECUREOS) += secureos.o > diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c > index 9f852c6..b7eea02 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 "secureos.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_secureos(); > tegra_cpu_reset_handler_init(); > tegra_apb_io_init(); > tegra_init_fuse(); > diff --git a/arch/arm/mach-tegra/reset.c b/arch/arm/mach-tegra/reset.c > index 1ac434e..4b9ebf9 100644 > --- a/arch/arm/mach-tegra/reset.c > +++ b/arch/arm/mach-tegra/reset.c > @@ -21,38 +21,32 @@ > > #include <asm/cacheflush.h> > #include <asm/hardware/cache-l2x0.h> > +#include <asm/firmware.h> > > #include "iomap.h" > #include "irammap.h" > #include "reset.h" > #include "sleep.h" > #include "fuse.h" > +#include "secureos.h" > > #define TEGRA_IRAM_RESET_BASE (TEGRA_IRAM_BASE + \ > TEGRA_IRAM_RESET_HANDLER_OFFSET) > > 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 +60,22 @@ 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); > + > + if (call_firmware_op(set_cpu_boot_addr, 0, reset_address) == -ENOSYS) > + tegra_cpu_reset_handler_set(reset_address); > > is_enabled = true; > } > diff --git a/arch/arm/mach-tegra/secureos.c b/arch/arm/mach-tegra/secureos.c > new file mode 100644 > index 0000000..44c3514 > --- /dev/null > +++ b/arch/arm/mach-tegra/secureos.c > @@ -0,0 +1,70 @@ > +/* > + * 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> > + > +static int __attribute__((used)) __tegra_smc_stack[10]; Use __used instead of using GCC attributes directly. > + > +/* > + * With EABI, subtype and arg already end up in r0, r1 and r2 as they are > + * function arguments, but we prefer to play safe here and explicitly move > + * these values into the expected registers anyway. mov instructions without > + * any side-effect are turned into nops by the assembler, which limits > + * overhead. > + */ > +static void tegra_generic_smc(u32 type, u32 subtype, u32 arg) > +{ > + asm volatile( > + ".arch_extension sec\n\t" > + "ldr r3, =__tegra_smc_stack\n\t" ldr= should be avoided in inline asm, because GCC can't guess it's size, and can't guarantee that the literal pool word is close enough to be addressable (though for small compilation units it's unlikely to be a problem). If you follow Russell's approach and define a naked function for this, you can put an explicit literal word after the return: ldr r3, 1f /* ... */ /* return instruction */ .align 2 1: .long __tegra_smc_stack > + "stmia r3, {r4-r12, lr}\n\t" > + "mov r0, %[type]\n\t" > + "mov r1, %[subtype]\n\t" > + "mov r2, %[arg]\n\t" > + "mov r3, #0\n\t" > + "mov r4, #0\n\t" > + "dsb\n\t" Can you explain what this DSB is for? > + "smc #0\n\t" > + "ldr r3, =__tegra_smc_stack\n\t" > + "ldmia r3, {r4-r12, lr}" > + : > + : [type] "r" (type), > + [subtype] "r" (subtype), > + [arg] "r" (arg) > + : "r0", "r1", "r2", "r3", "r4", "memory"); If r5-r12 are not clobbered, why do you save and restore them? In the ARM ABI, r12 is a caller-save register, so you probably don't need to save/restore that even if it is clobbered. Cheers ---Dave
On Thu, Jun 06, 2013 at 12:17:02PM +0200, Tomasz Figa wrote: > Hi Alex, > > On Thursday 06 of June 2013 16:28:07 Alexandre Courbot wrote: > > Boot loaders on some Tegra devices can be unlocked but do not let the > > system operate without SecureOS. SecureOS prevents access to some > > registers and requires the operating system to perform certain > > operations through Secure Monitor Calls instead of directly accessing > > the hardware. > > > > This patch introduces basic SecureOS support for Tegra. SecureOS support > > can be enabled by adding a "nvidia,secure-os" property to the "chosen" > > node of the device tree. > > > > Currently, only the bringup of secondary CPUs is performed by SMCs, but > > more operations will be added later. > > > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > > --- > > Documentation/devicetree/bindings/arm/tegra.txt | 8 +++ > > arch/arm/configs/tegra_defconfig | 1 + > > arch/arm/mach-tegra/Kconfig | 11 ++++ > > arch/arm/mach-tegra/Makefile | 2 + > > arch/arm/mach-tegra/common.c | 2 + > > arch/arm/mach-tegra/reset.c | 30 +++++++---- > > arch/arm/mach-tegra/secureos.c | 70 > > +++++++++++++++++++++++++ arch/arm/mach-tegra/secureos.h > > | 31 +++++++++++ 8 files changed, 145 insertions(+), 10 deletions(-) > > create mode 100644 arch/arm/mach-tegra/secureos.c > > create mode 100644 arch/arm/mach-tegra/secureos.h > > > > diff --git a/Documentation/devicetree/bindings/arm/tegra.txt > > b/Documentation/devicetree/bindings/arm/tegra.txt index > > ed9c853..b543091 100644 > > --- a/Documentation/devicetree/bindings/arm/tegra.txt > > +++ b/Documentation/devicetree/bindings/arm/tegra.txt > > @@ -32,3 +32,11 @@ board-specific compatible values: > > nvidia,whistler > > toradex,colibri_t20-512 > > toradex,iris > > + > > +Global properties > > +------------------------------------------- > > + > > +The following properties can be specified into the "chosen" root > > +node: > > + > > + nvidia,secure-os: enable SecureOS. > > Hmm, on Exynos we had something like > > firmware@0203F000 { > compatible = "samsung,secure-firmware"; > reg = <0x0203F000 0x1000>; > }; > > but your solution might be actually the proper one, since firmware is not > a hardware block. (The address in reg property is pointing to SYSRAM > memory, which is an additional communication channel with the firmware.) Calling to SecureOS doesn't sound like a "choice" to me. It's part of the platform, and if it's present then you have to use it, otherwise major functionality (i.e., SMP) broken. Having a proper node still makes sense, because you can put a "compatible" property on it to track interface compatibility etc. This was the view taken for PSCI (though in practice, we do have additional information to put in the DT for that anyway). Cheers ---Dave
On Thu, Jun 6, 2013 at 12:58 PM, Alexandre Courbot <acourbot@nvidia.com> wrote: > Boot loaders on some Tegra devices can be unlocked but do not let the > system operate without SecureOS. SecureOS prevents access to some > registers and requires the operating system to perform certain > operations through Secure Monitor Calls instead of directly accessing > the hardware. > IOW, some critical h/w controls on Tegra are accessible only from Secure mode (not unusual). So if we(Linux) run in NS mode we need to make calls to the SecureOS, over SMC, to do things for us? > This patch introduces basic SecureOS support for Tegra. SecureOS support > can be enabled by adding a "nvidia,secure-os" property to the "chosen" > node of the device tree. > Probably just a nit, but shouldn't it be "nvidia,nonsecure-os" instead, denoting the mode Linux is going to run? (and then I wonder if we could detect the mode (S or NS) at runtime and avoid this flag at all). Cheers, -Jassi
On 06/06/2013 04:37 AM, Alex Courbot wrote: > Hi Tomasz, > > On 06/06/2013 07:17 PM, Tomasz Figa wrote: ... >> I think this patch could be split into several patches: >> - add support for firmware >> - split reset function >> - add reset support using firmware. > > Mmm possibly yes, but I wonder if that would not be too much splitting. > Stephen? That's probably fine. It might make bisection easier if there are regressions.
On 06/06/2013 01:28 AM, Alexandre Courbot wrote: > Boot loaders on some Tegra devices can be unlocked but do not let the > system operate without SecureOS. SecureOS prevents access to some > registers and requires the operating system to perform certain > operations through Secure Monitor Calls instead of directly accessing > the hardware. > > This patch introduces basic SecureOS support for Tegra. SecureOS support > can be enabled by adding a "nvidia,secure-os" property to the "chosen" > node of the device tree. I suspect "SecureOS" here is the name of a specific implementation of a secure monitor, right? It's certainly a very unfortunate name, since it sounds like a generic concept rather than a specific implementation:-( There certainly could be (and I believe are in practice?) multiple implementation of a secure monitor for Tegra. Presumably, each of those implementations has (or could have) a different definition for what SVC calls it supports, their parameters, etc. I think we need to separate the concept of support for *a* secure monitor, from support for a *particular* secure monitor. > diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt > +node: > + > + nvidia,secure-os: enable SecureOS. ... as such, I think some work is needed here to allow specification of which secure monitor is present and/or which secure monitor ABI it implements. The suggestion to use a specific DT node, and hence use compatible values for this, seems reasonable. Alternatively, perhaps: nvidia,secure-monitor = "name_of_vendor_or_SMC_ABI"; might be reasonable, although using a node would allow ABI-specific additional properties to be defined should they be needed, so I guess I would lean towards that. Similar comments may apply to the CONFIG_ option and description, filenames, etc. > diff --git a/arch/arm/mach-tegra/secureos.c b/arch/arm/mach-tegra/secureos.c > +void __init tegra_init_secureos(void) > +{ > + struct device_node *node = of_find_node_by_path("/chosen"); > + > + if (node && of_property_read_bool(node, "nvidia,secure-os")) > + register_firmware_ops(&tegra_firmware_ops); > +} I'm tempted to think that function should /always/ exist an be called (so no dummy inline in secureos.h). In particular, what happens when a kernel without CONFIG_SECUREOS enabled is booted under a secure monitor. Presumably we want the init code to explicitly check for this condition, and either BUG(), or do something to disable any features (like SMP) that require SVCs? So, the algorithm here might be more like: find node if node exists if (!IS_ENABLED(SECURE_OS)) BUG/WARN/... else register_firmware_ops() ?
On Thu, Jun 06, 2013 at 10:44:48AM -0600, Stephen Warren wrote: > On 06/06/2013 01:28 AM, Alexandre Courbot wrote: > > Boot loaders on some Tegra devices can be unlocked but do not let the > > system operate without SecureOS. SecureOS prevents access to some > > registers and requires the operating system to perform certain > > operations through Secure Monitor Calls instead of directly accessing > > the hardware. > > > > This patch introduces basic SecureOS support for Tegra. SecureOS support > > can be enabled by adding a "nvidia,secure-os" property to the "chosen" > > node of the device tree. > > I suspect "SecureOS" here is the name of a specific implementation of a > secure monitor, right? It's certainly a very unfortunate name, since it > sounds like a generic concept rather than a specific implementation:-( > > There certainly could be (and I believe are in practice?) multiple > implementation of a secure monitor for Tegra. Presumably, each of those > implementations has (or could have) a different definition for what SVC > calls it supports, their parameters, etc. > > I think we need to separate the concept of support for *a* secure > monitor, from support for a *particular* secure monitor. There is no fixed set of functionality implemented by these interfaces, so it might be better to think in terms of a generic "firmware" concept. Come to think of it... One option could be to have some standard baseline firmware calling conventions, so that we could have a few specific backends -- perhaps this could be built on the "method" notion used by PSCI (see Documentation/devicetree/bindings/arm/psci.tst; this is probably the most developed firmware interface binding we have today) There, method = "smc" means: populate registers in a certain way SMC #0 return results from register to caller in a certain way and method = "hvc" means: populate registers in a certain way HVC #0 return results from register to caller in a certain way The backend method arch/arm/kernel/psci.c:__invoke_psci_fn_smc() is probably close to what's needed for the tegra secureos case, so in theory it could be common, along with some of the DT binding conventions. The backends, and the convention for binding a firmware interface to the appropriate backend, could then theoretically be handled by a common framework. Firmwares with strange calling conventions which don't fit the standard backend could still add another one, within the framework. > > > diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt > > > +node: > > + > > + nvidia,secure-os: enable SecureOS. > > ... as such, I think some work is needed here to allow specification of > which secure monitor is present and/or which secure monitor ABI it > implements. The suggestion to use a specific DT node, and hence use > compatible values for this, seems reasonable. Alternatively, perhaps: > > nvidia,secure-monitor = "name_of_vendor_or_SMC_ABI"; So, something like foo-firmware { compatible = "vendor1,foo-firmware-1.0", "vendor1,foo-firmware"; method = "smc"; // ... }; bar-firmware { compatible = "vendor2,bar-firmware-1.6", "vendor2,bar-firmware"; method = "smc"; // ... }; Could make sense. ...where we would require all new firmware interface bindings to include the "-firmware" in their node names and compatible strings (with a version suffix encouraged for the compatible strings, where relevant). This could be a good opportunity to make things a bit more consistent, otherwise we will just reinvent this over and over. (Unfortunately the node for PSCI offers no clue that it describes a firmware interface, unless you go and read the binding documentation. It may be too late to fix that, but we can at least avoid repeating the mistake.) > might be reasonable, although using a node would allow ABI-specific > additional properties to be defined should they be needed, so I guess I > would lean towards that. > > Similar comments may apply to the CONFIG_ option and description, > filenames, etc. > > > diff --git a/arch/arm/mach-tegra/secureos.c b/arch/arm/mach-tegra/secureos.c > > > +void __init tegra_init_secureos(void) > > +{ > > + struct device_node *node = of_find_node_by_path("/chosen"); > > + > > + if (node && of_property_read_bool(node, "nvidia,secure-os")) > > + register_firmware_ops(&tegra_firmware_ops); > > +} > > I'm tempted to think that function should /always/ exist an be called > (so no dummy inline in secureos.h). > > In particular, what happens when a kernel without CONFIG_SECUREOS > enabled is booted under a secure monitor. Presumably we want the init > code to explicitly check for this condition, and either BUG(), or do > something to disable any features (like SMP) that require SVCs? > > So, the algorithm here might be more like: > > find node > if node exists > if (!IS_ENABLED(SECURE_OS)) > BUG/WARN/... > else > register_firmware_ops() Agreed, something like that would be needed, but it depends on the firmware interface being described. Cheers ---Dave
On 06/06/2013 12:08 PM, Dave Martin wrote: > On Thu, Jun 06, 2013 at 10:44:48AM -0600, Stephen Warren wrote: >> On 06/06/2013 01:28 AM, Alexandre Courbot wrote: >>> Boot loaders on some Tegra devices can be unlocked but do not let the >>> system operate without SecureOS. SecureOS prevents access to some >>> registers and requires the operating system to perform certain >>> operations through Secure Monitor Calls instead of directly accessing >>> the hardware. >>> >>> This patch introduces basic SecureOS support for Tegra. SecureOS support >>> can be enabled by adding a "nvidia,secure-os" property to the "chosen" >>> node of the device tree. ... > So, something like > > foo-firmware { > compatible = "vendor1,foo-firmware-1.0", "vendor1,foo-firmware"; > method = "smc"; > > // ... > }; > > bar-firmware { > compatible = "vendor2,bar-firmware-1.6", "vendor2,bar-firmware"; > method = "smc"; > > // ... > }; > > Could make sense. I'm not sure if the method property is useful in most cases. For generic ABIs such as PSCI that actually support multiple communication paths, yes, it makes sense. However, it seems like for most custom ABIs, such as is presumably implemented by whatever "SecureOS" is on Tegra, the firmware type (or compatible value) directly implies that it operates over SMC not HVC. After all, presumably if the kernel was virtualized, it wouldn't have access to the raw secure monitor? I suppose you could argue that the hypervisor might end up emulating the same ABI over HVC instead? I'm not sure how likely that is. A compromise might be to assume SMC if no property was present, which would allow it to be optionally added later if it turned out to be useful. > ...where we would require all new firmware interface bindings to > include the "-firmware" in their node names and compatible strings > (with a version suffix encouraged for the compatible strings, where > relevant). That's probably a good convention, but I'm not sure it should be required (or at least validated by code). Requiring this by code review might be OK. Node names aren't meant to be interpreted semantically.
On Thu, Jun 6, 2013 at 9:26 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote: > On Thu, Jun 6, 2013 at 12:58 PM, Alexandre Courbot <acourbot@nvidia.com> wrote: >> Boot loaders on some Tegra devices can be unlocked but do not let the >> system operate without SecureOS. SecureOS prevents access to some >> registers and requires the operating system to perform certain >> operations through Secure Monitor Calls instead of directly accessing >> the hardware. >> > IOW, some critical h/w controls on Tegra are accessible only from > Secure mode (not unusual). So if we(Linux) run in NS mode we need to > make calls to the SecureOS, over SMC, to do things for us? Exactly. >> This patch introduces basic SecureOS support for Tegra. SecureOS support >> can be enabled by adding a "nvidia,secure-os" property to the "chosen" >> node of the device tree. >> > Probably just a nit, but shouldn't it be "nvidia,nonsecure-os" > instead, denoting the mode Linux is going to run? (and then I wonder > if we could detect the mode (S or NS) at runtime and avoid this flag > at all). Detection of the secure mode at runtime would only solve half of the issue: we would know that we are running in non-secure mode, but we would still not know what monitor is operating. Detecting that part is impossible AFAIK, so I'm afraid we need to pass that information through the DT here. Alex.
On Thu, Jun 6, 2013 at 8:02 PM, Dave Martin <Dave.Martin@arm.com> wrote: >> +static int __attribute__((used)) __tegra_smc_stack[10]; > > Use __used instead of using GCC attributes directly. > >> + >> +/* >> + * With EABI, subtype and arg already end up in r0, r1 and r2 as they are >> + * function arguments, but we prefer to play safe here and explicitly move >> + * these values into the expected registers anyway. mov instructions without >> + * any side-effect are turned into nops by the assembler, which limits >> + * overhead. >> + */ >> +static void tegra_generic_smc(u32 type, u32 subtype, u32 arg) >> +{ >> + asm volatile( >> + ".arch_extension sec\n\t" >> + "ldr r3, =__tegra_smc_stack\n\t" > > ldr= should be avoided in inline asm, because GCC can't guess it's size, > and can't guarantee that the literal pool word is close enough to be > addressable (though for small compilation units it's unlikely to be a > problem). With Russel's suggested changes this will go away, but that's good to know anyway. >> + "dsb\n\t" > > Can you explain what this DSB is for? Just a safety measure to make sure all memory operations are done before we enter the secure monitor. Is it unnecessary? >> + "smc #0\n\t" >> + "ldr r3, =__tegra_smc_stack\n\t" >> + "ldmia r3, {r4-r12, lr}" >> + : >> + : [type] "r" (type), >> + [subtype] "r" (subtype), >> + [arg] "r" (arg) >> + : "r0", "r1", "r2", "r3", "r4", "memory"); > > If r5-r12 are not clobbered, why do you save and restore them? The secure monitor might change them. > In the ARM ABI, r12 is a caller-save register, so you probably > don't need to save/restore that even if it is clobbered. Right, thanks. Didn't know r12 could be used as a scratch register. Alex.
On Fri, Jun 7, 2013 at 1:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 06/06/2013 01:28 AM, Alexandre Courbot wrote: >> Boot loaders on some Tegra devices can be unlocked but do not let the >> system operate without SecureOS. SecureOS prevents access to some >> registers and requires the operating system to perform certain >> operations through Secure Monitor Calls instead of directly accessing >> the hardware. >> >> This patch introduces basic SecureOS support for Tegra. SecureOS support >> can be enabled by adding a "nvidia,secure-os" property to the "chosen" >> node of the device tree. > > I suspect "SecureOS" here is the name of a specific implementation of a > secure monitor, right? It's certainly a very unfortunate name, since it > sounds like a generic concept rather than a specific implementation:-( Right. Using the actual name (Trusted Foundations) is probably better. I don't think the SecureOS denomination is used by anyone else but NVIDIA. > There certainly could be (and I believe are in practice?) multiple > implementation of a secure monitor for Tegra. Presumably, each of those > implementations has (or could have) a different definition for what SVC > calls it supports, their parameters, etc. > > I think we need to separate the concept of support for *a* secure > monitor, from support for a *particular* secure monitor. Agreed. In this case, can we assume that support for a specific secure monitor is not arch-specific, and that this patch should be moved outside of arch-tegra and down to arch/arm? In other words, the ABI of a particular secure monitor should be the same no matter the chip, shouldn't it? >> +node: >> + >> + nvidia,secure-os: enable SecureOS. > > ... as such, I think some work is needed here to allow specification of > which secure monitor is present and/or which secure monitor ABI it > implements. The suggestion to use a specific DT node, and hence use > compatible values for this, seems reasonable. Alternatively, perhaps: > > nvidia,secure-monitor = "name_of_vendor_or_SMC_ABI"; > > might be reasonable, although using a node would allow ABI-specific > additional properties to be defined should they be needed, so I guess I > would lean towards that. Considering your previous comment, I agree this seems to make the most sense. > Similar comments may apply to the CONFIG_ option and description, > filenames, etc. > >> diff --git a/arch/arm/mach-tegra/secureos.c b/arch/arm/mach-tegra/secureos.c > >> +void __init tegra_init_secureos(void) >> +{ >> + struct device_node *node = of_find_node_by_path("/chosen"); >> + >> + if (node && of_property_read_bool(node, "nvidia,secure-os")) >> + register_firmware_ops(&tegra_firmware_ops); >> +} > > I'm tempted to think that function should /always/ exist an be called > (so no dummy inline in secureos.h). > > In particular, what happens when a kernel without CONFIG_SECUREOS > enabled is booted under a secure monitor. Presumably we want the init > code to explicitly check for this condition, and either BUG(), or do > something to disable any features (like SMP) that require SVCs? > > So, the algorithm here might be more like: > > find node > if node exists > if (!IS_ENABLED(SECURE_OS)) > BUG/WARN/... > else > register_firmware_ops() > > ? Yep, let's do it this way. Alex.
On Fri, Jun 7, 2013 at 12:43 PM, Alexandre Courbot <gnurou@gmail.com> wrote: > On Thu, Jun 6, 2013 at 9:26 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote: >> On Thu, Jun 6, 2013 at 12:58 PM, Alexandre Courbot <acourbot@nvidia.com> wrote: >>> Boot loaders on some Tegra devices can be unlocked but do not let the >>> system operate without SecureOS. SecureOS prevents access to some >>> registers and requires the operating system to perform certain >>> operations through Secure Monitor Calls instead of directly accessing >>> the hardware. >>> >> IOW, some critical h/w controls on Tegra are accessible only from >> Secure mode (not unusual). So if we(Linux) run in NS mode we need to >> make calls to the SecureOS, over SMC, to do things for us? > > Exactly. > OK so this just an instance of a situation that is going to be more common in future - most modern cpus (CortexA based at least) support TrustZone and it's a matter of device functionality before a board designer chucks the Linux kernel (that we usually run in Secure mode while development) into NS mode and have some SecureOS/TrustedOS/Hypervisor et al control access to the secure peripherals from Secure Mode. >>> This patch introduces basic SecureOS support for Tegra. SecureOS support >>> can be enabled by adding a "nvidia,secure-os" property to the "chosen" >>> node of the device tree. >>> >> Probably just a nit, but shouldn't it be "nvidia,nonsecure-os" >> instead, denoting the mode Linux is going to run? (and then I wonder >> if we could detect the mode (S or NS) at runtime and avoid this flag >> at all). > > Detection of the secure mode at runtime would only solve half of the > issue: we would know that we are running in non-secure mode, but we > would still not know what monitor is operating. Detecting that part is > impossible AFAIK, so I'm afraid we need to pass that information > through the DT here. > Yes, Stephen's suggestion is far more generally applicable. We should go for it, just bearing in mind that ABI need and type is going to be board specific. Cheers, -Jassi
On Fri, Jun 7, 2013 at 3:08 AM, Dave Martin <Dave.Martin@arm.com> wrote: >> I think we need to separate the concept of support for *a* secure >> monitor, from support for a *particular* secure monitor. > > There is no fixed set of functionality implemented by these interfaces, > so it might be better to think in terms of a generic "firmware" concept. > > > Come to think of it... > > One option could be to have some standard baseline firmware calling > conventions, so that we could have a few specific backends -- perhaps > this could be built on the "method" notion used by PSCI > > (see Documentation/devicetree/bindings/arm/psci.tst; this is probably > the most developed firmware interface binding we have today) > > There, method = "smc" means: > > populate registers in a certain way > SMC #0 > return results from register to caller in a certain way > > and method = "hvc" means: > > populate registers in a certain way > HVC #0 > return results from register to caller in a certain way > > > The backend method arch/arm/kernel/psci.c:__invoke_psci_fn_smc() > is probably close to what's needed for the tegra secureos case, > so in theory it could be common, along with some of the DT binding > conventions. > > The backends, and the convention for binding a firmware interface > to the appropriate backend, could then theoretically be handled > by a common framework. I'm not sure whether we could use the same backend for many different firmwares. If I understand you correctly, you propose to have a backend to the "smc" call that would cover the needs of all firmwares that rely on the smc instruction to invoke the firmware/secure monitor. I can understand the logic, but I'm not sure this is needed or even possible. For instance, the implementation you have in __invoke_psci_fn_smc assumes 4 arguments, while Tegra's only needs 3. Also (and although I have to confess I am not very knowledgeable about the "SecureOS" covered by this patch and need to double-check what follows), in Tegra's case registers r3-r11 can be altered by the secure monitor and need to be preserved - something you don't need to do with PSCI. Another example is the function that Tomasz shown (https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-exynos/exynos-smc.S?id=refs/tags/next-20130606 ), which preserves r4-r11 but also assumes r3 is an argument - that's again another slightly different convention. All in all the needs of the various firmwares might end up being just different enough that we need to have a different backend for each of them. The firmware_ops defined in arch/arm/include/asm/firmware.h perform the abstraction at a higher level, which seems more fit here IMHO. Alex.
On 06/07/2013 02:11 AM, Alexandre Courbot wrote: > On Fri, Jun 7, 2013 at 1:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 06/06/2013 01:28 AM, Alexandre Courbot wrote: >>> Boot loaders on some Tegra devices can be unlocked but do not let the >>> system operate without SecureOS. SecureOS prevents access to some >>> registers and requires the operating system to perform certain >>> operations through Secure Monitor Calls instead of directly accessing >>> the hardware. >>> >>> This patch introduces basic SecureOS support for Tegra. SecureOS support >>> can be enabled by adding a "nvidia,secure-os" property to the "chosen" >>> node of the device tree. >> >> I suspect "SecureOS" here is the name of a specific implementation of a >> secure monitor, right? It's certainly a very unfortunate name, since it >> sounds like a generic concept rather than a specific implementation:-( > > Right. Using the actual name (Trusted Foundations) is probably better. > I don't think the SecureOS denomination is used by anyone else but > NVIDIA. > >> There certainly could be (and I believe are in practice?) multiple >> implementation of a secure monitor for Tegra. Presumably, each of those >> implementations has (or could have) a different definition for what SVC >> calls it supports, their parameters, etc. >> >> I think we need to separate the concept of support for *a* secure >> monitor, from support for a *particular* secure monitor. > > Agreed. In this case, can we assume that support for a specific secure > monitor is not arch-specific, and that this patch should be moved > outside of arch-tegra and down to arch/arm? In other words, the ABI of > a particular secure monitor should be the same no matter the chip, > shouldn't it? I would like to believe that the Trusted Foundations monitor had the same ABI irrespective of which Soc it was running on. However, I have absolutely no idea at all if that's true. Even if there's some common subset of the ABI that is identical across all SoCs, I wouldn't be too surprised if there were custom extensions for each different SoC, or just perhaps even each product. Can you research this and find out the answer? What we can always do is make a compatible property that lists everything[1], and have the driver match on the most specific value for now, but relax the driver's matching later if it turns out that the ABI is indeed common. [1] That'd need to be at least secure OS name, and secure OS version. Perhaps the SoC and board data can be deduced from the DT's top-level compatible properties; nvidia,tegra114-shield, nvidia,tegra114?
On Fri, Jun 07, 2013 at 04:25:07PM +0900, Alexandre Courbot wrote: > On Thu, Jun 6, 2013 at 8:02 PM, Dave Martin <Dave.Martin@arm.com> wrote: > > >> +static int __attribute__((used)) __tegra_smc_stack[10]; > > > > Use __used instead of using GCC attributes directly. > > > >> + > >> +/* > >> + * With EABI, subtype and arg already end up in r0, r1 and r2 as they are > >> + * function arguments, but we prefer to play safe here and explicitly move > >> + * these values into the expected registers anyway. mov instructions without > >> + * any side-effect are turned into nops by the assembler, which limits > >> + * overhead. > >> + */ > >> +static void tegra_generic_smc(u32 type, u32 subtype, u32 arg) > >> +{ > >> + asm volatile( > >> + ".arch_extension sec\n\t" > >> + "ldr r3, =__tegra_smc_stack\n\t" > > > > ldr= should be avoided in inline asm, because GCC can't guess it's size, > > and can't guarantee that the literal pool word is close enough to be > > addressable (though for small compilation units it's unlikely to be a > > problem). > > With Russel's suggested changes this will go away, but that's good to > know anyway. > > >> + "dsb\n\t" > > > > Can you explain what this DSB is for? > > Just a safety measure to make sure all memory operations are done > before we enter the secure monitor. Is it unnecessary? Most likely it's either unnecessary, or insufficient. Just for entering call SMC properly, it's not needed. If the Secure World has its MMU on and maps Normal World memory using the same memory types as Linux, then the Normal World and Secure World access the memory coherently with no extra barrier needed. It the Secure World's MMU is off, or if it maps Normal World memory as Secure (pagetable NS bit = 0), or if it maps Normal World memory with memory types incompatible with the ones Linux is using then you will get coherency problems: the Secure and Normal Worlds won't necessarily see the same data. In the latter case, you must do explicit cache maintenance around SMC for the data you want to exchange. This might also be needed in the Secure World if caches are enabled over there. DSB isn't enough by itself. If the Secure World doesn't access Normal World memory at all, you don't need to do anything special and can remove the DSB. Otherwise, for performance reasons, it is definitely preferable to have the Secure World MMU on if possible, though it's a bit more complex to set up. > >> + "smc #0\n\t" > >> + "ldr r3, =__tegra_smc_stack\n\t" > >> + "ldmia r3, {r4-r12, lr}" > >> + : > >> + : [type] "r" (type), > >> + [subtype] "r" (subtype), > >> + [arg] "r" (arg) > >> + : "r0", "r1", "r2", "r3", "r4", "memory"); > > > > If r5-r12 are not clobbered, why do you save and restore them? > > The secure monitor might change them. Sure, but you could just add all the registers to the clobber list, and then the compiler would save and restore them for you in the function entry/exit sequences, which may be a bit more efficient. Since you are going to replace this code anyway, it's academic though. > > > In the ARM ABI, r12 is a caller-save register, so you probably > > don't need to save/restore that even if it is clobbered. > > Right, thanks. Didn't know r12 could be used as a scratch register. What I said is a bit wrong actually: for the naked version of the function you can go ahead and corrupt r12, because naked functions are not inlinable and follow the ABI. Adding "r12" in the clobber list would be harmless in this case, but I don't think it's necessary. asm() in any other context needs to declare all registers that it might clobber. Cheers ---Dave
On Thu, Jun 06, 2013 at 12:29:14PM -0600, Stephen Warren wrote: > On 06/06/2013 12:08 PM, Dave Martin wrote: > > On Thu, Jun 06, 2013 at 10:44:48AM -0600, Stephen Warren wrote: > >> On 06/06/2013 01:28 AM, Alexandre Courbot wrote: > >>> Boot loaders on some Tegra devices can be unlocked but do not let the > >>> system operate without SecureOS. SecureOS prevents access to some > >>> registers and requires the operating system to perform certain > >>> operations through Secure Monitor Calls instead of directly accessing > >>> the hardware. > >>> > >>> This patch introduces basic SecureOS support for Tegra. SecureOS support > >>> can be enabled by adding a "nvidia,secure-os" property to the "chosen" > >>> node of the device tree. > ... > > So, something like > > > > foo-firmware { > > compatible = "vendor1,foo-firmware-1.0", "vendor1,foo-firmware"; > > method = "smc"; > > > > // ... > > }; > > > > bar-firmware { > > compatible = "vendor2,bar-firmware-1.6", "vendor2,bar-firmware"; > > method = "smc"; > > > > // ... > > }; > > > > Could make sense. > > I'm not sure if the method property is useful in most cases. For generic I was mostly illustrating. It's not useful in most cases, unless we have standard code parsing these bindings (probably overkill). > ABIs such as PSCI that actually support multiple communication paths, > yes, it makes sense. However, it seems like for most custom ABIs, such > as is presumably implemented by whatever "SecureOS" is on Tegra, the > firmware type (or compatible value) directly implies that it operates > over SMC not HVC. After all, presumably if the kernel was virtualized, > it wouldn't have access to the raw secure monitor? I suppose you could > argue that the hypervisor might end up emulating the same ABI over HVC > instead? I'm not sure how likely that is. A compromise might be to > assume SMC if no property was present, which would allow it to be > optionally added later if it turned out to be useful. Sure. For most firmware interface bindings, the backend could be implied. The binding could be extended later if an alternative backend were needed. If an SMC-called firmware interface is visible to a guest running under a hypervisor at all, it is probably still callable using SMC, since hypervisors can trap and proxy/emulate it instead of the call bypassing the hypervisor and going straight to the Secure World. If the interface is not visible, the hypervisor shouldn't put the relevant node in the DT supplied to the guest (just as any physical hardware not accessible to the guest shouldn't be described in the DT). Firmware interfaces which don't specifically depend on the Security Extensions need multiple backends, because hypervisors can't trap SMC on ARMv8 platforms which don't implement the Security Extensions -- this was an issue for PSCI, and it's why HVC is used instead by KVM etc. For "Secure OS" interfaces this won't be an issue since those interfaces don't make sense if the Security Extensions aren't there. > > ...where we would require all new firmware interface bindings to > > include the "-firmware" in their node names and compatible strings > > (with a version suffix encouraged for the compatible strings, where > > relevant). > > That's probably a good convention, but I'm not sure it should be > required (or at least validated by code). Requiring this by code review > might be OK. Node names aren't meant to be interpreted semantically. Agreed: I don't think this makes sense as a formal binding, but rather it's a convention we can follow which avoids DTs being unnecessarily cryptic. Cheers ---Dave
On Fri, Jun 07, 2013 at 06:03:54PM +0900, Alexandre Courbot wrote: > On Fri, Jun 7, 2013 at 3:08 AM, Dave Martin <Dave.Martin@arm.com> wrote: > >> I think we need to separate the concept of support for *a* secure > >> monitor, from support for a *particular* secure monitor. > > > > There is no fixed set of functionality implemented by these interfaces, > > so it might be better to think in terms of a generic "firmware" concept. > > > > > > Come to think of it... > > > > One option could be to have some standard baseline firmware calling > > conventions, so that we could have a few specific backends -- perhaps > > this could be built on the "method" notion used by PSCI > > > > (see Documentation/devicetree/bindings/arm/psci.tst; this is probably > > the most developed firmware interface binding we have today) > > > > There, method = "smc" means: > > > > populate registers in a certain way > > SMC #0 > > return results from register to caller in a certain way > > > > and method = "hvc" means: > > > > populate registers in a certain way > > HVC #0 > > return results from register to caller in a certain way > > > > > > The backend method arch/arm/kernel/psci.c:__invoke_psci_fn_smc() > > is probably close to what's needed for the tegra secureos case, > > so in theory it could be common, along with some of the DT binding > > conventions. > > > > The backends, and the convention for binding a firmware interface > > to the appropriate backend, could then theoretically be handled > > by a common framework. > > I'm not sure whether we could use the same backend for many different > firmwares. If I understand you correctly, you propose to have a > backend to the "smc" call that would cover the needs of all firmwares > that rely on the smc instruction to invoke the firmware/secure > monitor. > > I can understand the logic, but I'm not sure this is needed or even > possible. For instance, the implementation you have in > __invoke_psci_fn_smc assumes 4 arguments, while Tegra's only needs 3. > Also (and although I have to confess I am not very knowledgeable about > the "SecureOS" covered by this patch and need to double-check what > follows), in Tegra's case registers r3-r11 can be altered by the > secure monitor and need to be preserved - something you don't need to > do with PSCI. One way to make the backend generic would be to have something like one of the following (some syntax omitted due to laziness): u32 __naked __call_smc(u32 r0, ...) { asm volatile ( stmfd sp!, {r4-r11,lr} smc #0 ldmfd sp!, {r4-r11,pc} ::: "memory" ); } /* The above works for up to 4 u32 arguments */ u32 __naked __call_smc(u32 r0, ...) { asm volatile ( mov ip, sp stmfd sp!, {r4-r11,lr} ldmia ip, {r4-r11} smc #0 ldmfd sp!, {r4-r11,pc} ::: "memory" ); } /* * Works for up to 13 u32 arguments, provided the stack is deep * enough to provide suitable garbage data to fill the registers * if the caller supplied fewer arguments (a bit of a hack) */ u32 __naked __call_smc(struct pt_regs *regs) { asm( stmfd sp!, {r4-r11,lr} /* load regs from <regs> */ smc #0 /* save regs back to <regs> */ ldmfd sp!, {r4-r11,pc} ); } /* * Most generic, least-efficient version. * Can return up to 13 u32 results instead of just one. * For convenience, the r0 value returned by the SMC could be * left in r0 so that it also determines the return value of the * function. * * Most of the time, SMC shouldn't be called on any hot path, * otherwise the performance battle is already lost -- so it may * not be crucial to reach the maximum possible efficiency for * these calls. */ A particular firmware's Linux glue code might have to put extra stuff around calls to generic_smc, but at least generic_smc itself wouldn't need to be reinvented, and the firmware-specific glue code could usually avoid asm. > Another example is the function that Tomasz shown > (https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-exynos/exynos-smc.S?id=refs/tags/next-20130606 > ), which preserves r4-r11 but also assumes r3 is an argument - that's > again another slightly different convention. ... for which the above implementations of __call_smc() should work too. > All in all the needs of the various firmwares might end up being just > different enough that we need to have a different backend for each of > them. The firmware_ops defined in arch/arm/include/asm/firmware.h > perform the abstraction at a higher level, which seems more fit here > IMHO. > > Alex. Indeed. If you think you could work with one of the above generics, we could try it and see what it looks like though. If it's an awkward fit, I might be being too optimistic. Cheers ---Dave
On Sat, Jun 8, 2013 at 2:30 AM, Dave Martin <Dave.Martin@arm.com> wrote: > Most likely it's either unnecessary, or insufficient. > > Just for entering call SMC properly, it's not needed. If the Secure > World has its MMU on and maps Normal World memory using the same memory > types as Linux, then the Normal World and Secure World access the memory > coherently with no extra barrier needed. > > It the Secure World's MMU is off, or if it maps Normal World memory > as Secure (pagetable NS bit = 0), or if it maps Normal World memory with > memory types incompatible with the ones Linux is using then you will get > coherency problems: the Secure and Normal Worlds won't necessarily see > the same data. > > In the latter case, you must do explicit cache maintenance around SMC > for the data you want to exchange. This might also be needed in the > Secure World if caches are enabled over there. DSB isn't enough by > itself. > > > If the Secure World doesn't access Normal World memory at all, you > don't need to do anything special and can remove the DSB. > > Otherwise, for performance reasons, it is definitely preferable to have > the Secure World MMU on if possible, though it's a bit more complex to > set up. Thanks for the information. I will try to understand why we put it here in the first place. >> >> + "smc #0\n\t" >> >> + "ldr r3, =__tegra_smc_stack\n\t" >> >> + "ldmia r3, {r4-r12, lr}" >> >> + : >> >> + : [type] "r" (type), >> >> + [subtype] "r" (subtype), >> >> + [arg] "r" (arg) >> >> + : "r0", "r1", "r2", "r3", "r4", "memory"); >> > >> > If r5-r12 are not clobbered, why do you save and restore them? >> >> The secure monitor might change them. > > Sure, but you could just add all the registers to the clobber list, > and then the compiler would save and restore them for you in the > function entry/exit sequences, which may be a bit more efficient. > > Since you are going to replace this code anyway, it's academic > though. Right. I suppose you mentioned this in the context of my initial code - however if I understand how inline asm works (in case it wasn't clear already, I probably don't), turning this function into a naked function will make it impossible for the compiler to generate the entry/exit sequences since the assembler code will be responsible for returning from the function. One could remove the naked attribute and put there registers into the clobber list, but then the function will be inlined and we will have to ensure the parameters end up in the right register (and having a function that cannot be inlined is convenient in that respect). So as far as I can tell, having the function naked and saving the registers ourselves seems to be the most convenient way around this. Thanks, Alex.
On Sat, Jun 8, 2013 at 3:13 AM, Dave Martin <Dave.Martin@arm.com> wrote: > One way to make the backend generic would be to have something like > one of the following (some syntax omitted due to laziness): > > u32 __naked __call_smc(u32 r0, ...) > { > asm volatile ( > stmfd sp!, {r4-r11,lr} > smc #0 > ldmfd sp!, {r4-r11,pc} > ::: "memory" > ); > } > > /* The above works for up to 4 u32 arguments */ > > u32 __naked __call_smc(u32 r0, ...) > { > asm volatile ( > mov ip, sp > stmfd sp!, {r4-r11,lr} > ldmia ip, {r4-r11} > smc #0 > ldmfd sp!, {r4-r11,pc} > ::: "memory" > ); > } > > /* > * Works for up to 13 u32 arguments, provided the stack is deep > * enough to provide suitable garbage data to fill the registers > * if the caller supplied fewer arguments (a bit of a hack) > */ > > u32 __naked __call_smc(struct pt_regs *regs) { > > asm( > stmfd sp!, {r4-r11,lr} > /* load regs from <regs> */ > smc #0 > /* save regs back to <regs> */ > ldmfd sp!, {r4-r11,pc} > ); > } > > /* > * Most generic, least-efficient version. > * Can return up to 13 u32 results instead of just one. > * For convenience, the r0 value returned by the SMC could be > * left in r0 so that it also determines the return value of the > * function. > * > * Most of the time, SMC shouldn't be called on any hot path, > * otherwise the performance battle is already lost -- so it may > * not be crucial to reach the maximum possible efficiency for > * these calls. > */ > > > A particular firmware's Linux glue code might have to put extra stuff > around calls to generic_smc, but at least generic_smc itself wouldn't > need to be reinvented, and the firmware-specific glue code could usually > avoid asm. > >> Another example is the function that Tomasz shown >> (https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-exynos/exynos-smc.S?id=refs/tags/next-20130606 >> ), which preserves r4-r11 but also assumes r3 is an argument - that's >> again another slightly different convention. > > ... for which the above implementations of __call_smc() should work too. > >> All in all the needs of the various firmwares might end up being just >> different enough that we need to have a different backend for each of >> them. The firmware_ops defined in arch/arm/include/asm/firmware.h >> perform the abstraction at a higher level, which seems more fit here >> IMHO. >> >> Alex. > > Indeed. If you think you could work with one of the above generics, we > could try it and see what it looks like though. > > If it's an awkward fit, I might be being too optimistic. I agree that your versions would most likely work in our (and probably many others) case. But I wonder if individual platforms will not prefer to sacrifice the ease of use of a generic version for the ability to write faster code that will just preserve what is needed (whether that performance gain is justified or not is of course subject to debate). I don't have enough hindsight to decide which approach is the best, but until we have more examples of firmwares that would justify such a factorization, I think I'd like to go with our own version first - for there is already more than enough to fix in this patch. :) Thanks, Alex.
On Sat, Jun 8, 2013 at 1:33 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> I think we need to separate the concept of support for *a* secure >>> monitor, from support for a *particular* secure monitor. >> >> Agreed. In this case, can we assume that support for a specific secure >> monitor is not arch-specific, and that this patch should be moved >> outside of arch-tegra and down to arch/arm? In other words, the ABI of >> a particular secure monitor should be the same no matter the chip, >> shouldn't it? > > I would like to believe that the Trusted Foundations monitor had the > same ABI irrespective of which Soc it was running on. However, I have > absolutely no idea at all if that's true. Even if there's some common > subset of the ABI that is identical across all SoCs, I wouldn't be too > surprised if there were custom extensions for each different SoC, or > just perhaps even each product. > > Can you research this and find out the answer? Will do. Information about TF is scarce unfortunately. > What we can always do is make a compatible property that lists > everything[1], and have the driver match on the most specific value for > now, but relax the driver's matching later if it turns out that the ABI > is indeed common. > > [1] That'd need to be at least secure OS name, and secure OS version. > Perhaps the SoC and board data can be deduced from the DT's top-level > compatible properties; nvidia,tegra114-shield, nvidia,tegra114? They can probably, but in theory nothing prevents a board from coming with different secure monitors (or none at all). In this case, just having the board name might not be enough. Having a proper node for the firmware like David and Tomasz suggested seems to be the best way to make sure we cover all cases - I think I will try to do it this way for the next version, and hopefully come with a binding that is useful for everyone. Thanks, Alex.
On Mon, Jun 10, 2013 at 04:47:22PM +0900, Alexandre Courbot wrote: > One could remove the naked attribute and put there registers into the > clobber list, but then the function will be inlined and we will have > to ensure the parameters end up in the right register (and having a > function that cannot be inlined is convenient in that respect). So as > far as I can tell, having the function naked and saving the registers > ourselves seems to be the most convenient way around this. If you use such a large clobber list, you risk the compiler barfing on you that it's run out of registers.
On Mon, Jun 10, 2013 at 05:11:15PM +0900, Alexandre Courbot wrote: > On Sat, Jun 8, 2013 at 1:33 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > >>> I think we need to separate the concept of support for *a* secure > >>> monitor, from support for a *particular* secure monitor. > >> > >> Agreed. In this case, can we assume that support for a specific secure > >> monitor is not arch-specific, and that this patch should be moved > >> outside of arch-tegra and down to arch/arm? In other words, the ABI of > >> a particular secure monitor should be the same no matter the chip, > >> shouldn't it? > > > > I would like to believe that the Trusted Foundations monitor had the > > same ABI irrespective of which Soc it was running on. However, I have > > absolutely no idea at all if that's true. Even if there's some common > > subset of the ABI that is identical across all SoCs, I wouldn't be too > > surprised if there were custom extensions for each different SoC, or > > just perhaps even each product. > > > > Can you research this and find out the answer? > > Will do. Information about TF is scarce unfortunately. The answer is... there isn't a common ABI. That is something I pressed ARM Ltd for when this stuff first appeared and they were adamant that they were not going to specify any kind of ABI for this interface. The net result is that everyone has invented their own interfaces around it. Some pass all arguments in registers, some pass arguments in memory and pass pointers to those memory locations, and those memory locations have to be flushed in some way.
On Mon, Jun 10, 2013 at 05:11:15PM +0900, Alexandre Courbot wrote: > On Sat, Jun 8, 2013 at 1:33 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > >>> I think we need to separate the concept of support for *a* secure > >>> monitor, from support for a *particular* secure monitor. > >> > >> Agreed. In this case, can we assume that support for a specific secure > >> monitor is not arch-specific, and that this patch should be moved > >> outside of arch-tegra and down to arch/arm? In other words, the ABI of > >> a particular secure monitor should be the same no matter the chip, > >> shouldn't it? > > > > I would like to believe that the Trusted Foundations monitor had the > > same ABI irrespective of which Soc it was running on. However, I have > > absolutely no idea at all if that's true. Even if there's some common > > subset of the ABI that is identical across all SoCs, I wouldn't be too > > surprised if there were custom extensions for each different SoC, or > > just perhaps even each product. > > > > Can you research this and find out the answer? > > Will do. Information about TF is scarce unfortunately. I don't have full information on this topic, but there is certainly no common standard ABI. Every example I've seen is different so far, though some will be less different than others. ARM are baking some proposabls for that, but that doesn't change the existing software, and it's impossible to predict how rapidly a new standards proposal would be adopted. So unfortunately, diversity is something we will have to cope with for the foreseeable future. > > What we can always do is make a compatible property that lists > > everything[1], and have the driver match on the most specific value for > > now, but relax the driver's matching later if it turns out that the ABI > > is indeed common. > > > > [1] That'd need to be at least secure OS name, and secure OS version. > > Perhaps the SoC and board data can be deduced from the DT's top-level > > compatible properties; nvidia,tegra114-shield, nvidia,tegra114? > > They can probably, but in theory nothing prevents a board from coming > with different secure monitors (or none at all). In this case, just > having the board name might not be enough. > > Having a proper node for the firmware like David and Tomasz suggested > seems to be the best way to make sure we cover all cases - I think I > will try to do it this way for the next version, and hopefully come > with a binding that is useful for everyone. Since existing SMC based firmwares are not safely probeable, describing what's there using DT feels like the best bet for now. Cheers ---Dave
On Mon, Jun 10, 2013 at 05:05:04PM +0900, Alexandre Courbot wrote: > On Sat, Jun 8, 2013 at 3:13 AM, Dave Martin <Dave.Martin@arm.com> wrote: > > One way to make the backend generic would be to have something like > > one of the following (some syntax omitted due to laziness): > > > > u32 __naked __call_smc(u32 r0, ...) > > { > > asm volatile ( > > stmfd sp!, {r4-r11,lr} > > smc #0 > > ldmfd sp!, {r4-r11,pc} > > ::: "memory" > > ); > > } > > > > /* The above works for up to 4 u32 arguments */ > > > > u32 __naked __call_smc(u32 r0, ...) > > { > > asm volatile ( > > mov ip, sp > > stmfd sp!, {r4-r11,lr} > > ldmia ip, {r4-r11} > > smc #0 > > ldmfd sp!, {r4-r11,pc} > > ::: "memory" > > ); > > } > > > > /* > > * Works for up to 13 u32 arguments, provided the stack is deep > > * enough to provide suitable garbage data to fill the registers > > * if the caller supplied fewer arguments (a bit of a hack) > > */ > > > > u32 __naked __call_smc(struct pt_regs *regs) { > > > > asm( > > stmfd sp!, {r4-r11,lr} > > /* load regs from <regs> */ > > smc #0 > > /* save regs back to <regs> */ > > ldmfd sp!, {r4-r11,pc} > > ); > > } > > > > /* > > * Most generic, least-efficient version. > > * Can return up to 13 u32 results instead of just one. > > * For convenience, the r0 value returned by the SMC could be > > * left in r0 so that it also determines the return value of the > > * function. > > * > > * Most of the time, SMC shouldn't be called on any hot path, > > * otherwise the performance battle is already lost -- so it may > > * not be crucial to reach the maximum possible efficiency for > > * these calls. > > */ > > > > > > A particular firmware's Linux glue code might have to put extra stuff > > around calls to generic_smc, but at least generic_smc itself wouldn't > > need to be reinvented, and the firmware-specific glue code could usually > > avoid asm. > > > >> Another example is the function that Tomasz shown > >> (https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-exynos/exynos-smc.S?id=refs/tags/next-20130606 > >> ), which preserves r4-r11 but also assumes r3 is an argument - that's > >> again another slightly different convention. > > > > ... for which the above implementations of __call_smc() should work too. > > > >> All in all the needs of the various firmwares might end up being just > >> different enough that we need to have a different backend for each of > >> them. The firmware_ops defined in arch/arm/include/asm/firmware.h > >> perform the abstraction at a higher level, which seems more fit here > >> IMHO. > >> > >> Alex. > > > > Indeed. If you think you could work with one of the above generics, we > > could try it and see what it looks like though. > > > > If it's an awkward fit, I might be being too optimistic. > > I agree that your versions would most likely work in our (and probably > many others) case. But I wonder if individual platforms will not > prefer to sacrifice the ease of use of a generic version for the > ability to write faster code that will just preserve what is needed > (whether that performance gain is justified or not is of course > subject to debate). I don't have enough hindsight to decide which > approach is the best, but until we have more examples of firmwares > that would justify such a factorization, I think I'd like to go with > our own version first - for there is already more than enough to fix > in this patch. :) Sure, I'll have another think based on your repost. Cheers ---Dave
On 06/10/2013 03:14 AM, Russell King - ARM Linux wrote: > On Mon, Jun 10, 2013 at 05:11:15PM +0900, Alexandre Courbot wrote: >> On Sat, Jun 8, 2013 at 1:33 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>> I think we need to separate the concept of support for *a* secure >>>>> monitor, from support for a *particular* secure monitor. >>>> >>>> Agreed. In this case, can we assume that support for a specific secure >>>> monitor is not arch-specific, and that this patch should be moved >>>> outside of arch-tegra and down to arch/arm? In other words, the ABI of >>>> a particular secure monitor should be the same no matter the chip, >>>> shouldn't it? >>> >>> I would like to believe that the Trusted Foundations monitor had the >>> same ABI irrespective of which Soc it was running on. However, I have >>> absolutely no idea at all if that's true. Even if there's some common >>> subset of the ABI that is identical across all SoCs, I wouldn't be too >>> surprised if there were custom extensions for each different SoC, or >>> just perhaps even each product. >>> >>> Can you research this and find out the answer? >> >> Will do. Information about TF is scarce unfortunately. > > The answer is... there isn't a common ABI. That is something I pressed > ARM Ltd for when this stuff first appeared and they were adamant that > they were not going to specify any kind of ABI for this interface. Right, there certainly isn't a common ABI across all secure monitors, but in this case I was wondering something more specific: whether for this specific implementation/provider of a secure monitor, if they had a consistent ABI across all SoCs (or even boards) that they implemented it on.
diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt index ed9c853..b543091 100644 --- a/Documentation/devicetree/bindings/arm/tegra.txt +++ b/Documentation/devicetree/bindings/arm/tegra.txt @@ -32,3 +32,11 @@ board-specific compatible values: nvidia,whistler toradex,colibri_t20-512 toradex,iris + +Global properties +------------------------------------------- + +The following properties can be specified into the "chosen" root +node: + + nvidia,secure-os: enable SecureOS. diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig index f7ba3161..f6ed0f5 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_SECUREOS=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..acb5d0a 100644 --- a/arch/arm/mach-tegra/Kconfig +++ b/arch/arm/mach-tegra/Kconfig @@ -87,4 +87,15 @@ config TEGRA_AHB config TEGRA_EMC_SCALING_ENABLE bool "Enable scaling the memory frequency" +config TEGRA_SECUREOS + bool "Enable SecureOS support" + help + Support for Tegra devices which bootloader sets up a + SecureOS environment. This will use Secure Monitor Calls + instead of directly accessing the hardware for some protected + operations. + + SecureOS support is enabled by declaring a "nvidia,secure-os" + property into the "chosen" node of the device tree. + endmenu diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile index d011f0a..3adafe6 100644 --- a/arch/arm/mach-tegra/Makefile +++ b/arch/arm/mach-tegra/Makefile @@ -37,3 +37,5 @@ endif obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += board-harmony-pcie.o obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += board-paz00.o + +obj-$(CONFIG_TEGRA_SECUREOS) += secureos.o diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c index 9f852c6..b7eea02 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 "secureos.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_secureos(); tegra_cpu_reset_handler_init(); tegra_apb_io_init(); tegra_init_fuse(); diff --git a/arch/arm/mach-tegra/reset.c b/arch/arm/mach-tegra/reset.c index 1ac434e..4b9ebf9 100644 --- a/arch/arm/mach-tegra/reset.c +++ b/arch/arm/mach-tegra/reset.c @@ -21,38 +21,32 @@ #include <asm/cacheflush.h> #include <asm/hardware/cache-l2x0.h> +#include <asm/firmware.h> #include "iomap.h" #include "irammap.h" #include "reset.h" #include "sleep.h" #include "fuse.h" +#include "secureos.h" #define TEGRA_IRAM_RESET_BASE (TEGRA_IRAM_BASE + \ TEGRA_IRAM_RESET_HANDLER_OFFSET) 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 +60,22 @@ 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); + + if (call_firmware_op(set_cpu_boot_addr, 0, reset_address) == -ENOSYS) + tegra_cpu_reset_handler_set(reset_address); is_enabled = true; } diff --git a/arch/arm/mach-tegra/secureos.c b/arch/arm/mach-tegra/secureos.c new file mode 100644 index 0000000..44c3514 --- /dev/null +++ b/arch/arm/mach-tegra/secureos.c @@ -0,0 +1,70 @@ +/* + * 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> + +static int __attribute__((used)) __tegra_smc_stack[10]; + +/* + * With EABI, subtype and arg already end up in r0, r1 and r2 as they are + * function arguments, but we prefer to play safe here and explicitly move + * these values into the expected registers anyway. mov instructions without + * any side-effect are turned into nops by the assembler, which limits + * overhead. + */ +static void tegra_generic_smc(u32 type, u32 subtype, u32 arg) +{ + asm volatile( + ".arch_extension sec\n\t" + "ldr r3, =__tegra_smc_stack\n\t" + "stmia r3, {r4-r12, lr}\n\t" + "mov r0, %[type]\n\t" + "mov r1, %[subtype]\n\t" + "mov r2, %[arg]\n\t" + "mov r3, #0\n\t" + "mov r4, #0\n\t" + "dsb\n\t" + "smc #0\n\t" + "ldr r3, =__tegra_smc_stack\n\t" + "ldmia r3, {r4-r12, lr}" + : + : [type] "r" (type), + [subtype] "r" (subtype), + [arg] "r" (arg) + : "r0", "r1", "r2", "r3", "r4", "memory"); +} + +static int tegra_set_cpu_boot_addr(int cpu, unsigned long boot_addr) +{ + tegra_generic_smc(0xfffff200, boot_addr, 0); + + return 0; +} + +static const struct firmware_ops tegra_firmware_ops = { + .set_cpu_boot_addr = tegra_set_cpu_boot_addr, +}; + +void __init tegra_init_secureos(void) +{ + struct device_node *node = of_find_node_by_path("/chosen"); + + if (node && of_property_read_bool(node, "nvidia,secure-os")) + register_firmware_ops(&tegra_firmware_ops); +} diff --git a/arch/arm/mach-tegra/secureos.h b/arch/arm/mach-tegra/secureos.h new file mode 100644 index 0000000..5388cc5 --- /dev/null +++ b/arch/arm/mach-tegra/secureos.h @@ -0,0 +1,31 @@ +/* + * 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_SECUREOS_H +#define __TEGRA_SECUREOS_H + +#ifdef CONFIG_TEGRA_SECUREOS + +#include <linux/types.h> + +void tegra_init_secureos(void); + +#else + +static inline void tegra_init_secureos(void) +{ +} + +#endif + +#endif
Boot loaders on some Tegra devices can be unlocked but do not let the system operate without SecureOS. SecureOS prevents access to some registers and requires the operating system to perform certain operations through Secure Monitor Calls instead of directly accessing the hardware. This patch introduces basic SecureOS support for Tegra. SecureOS support can be enabled by adding a "nvidia,secure-os" property to the "chosen" node of the device tree. Currently, only the bringup of secondary CPUs is performed by SMCs, but more operations will be added later. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- Documentation/devicetree/bindings/arm/tegra.txt | 8 +++ arch/arm/configs/tegra_defconfig | 1 + arch/arm/mach-tegra/Kconfig | 11 ++++ arch/arm/mach-tegra/Makefile | 2 + arch/arm/mach-tegra/common.c | 2 + arch/arm/mach-tegra/reset.c | 30 +++++++---- arch/arm/mach-tegra/secureos.c | 70 +++++++++++++++++++++++++ arch/arm/mach-tegra/secureos.h | 31 +++++++++++ 8 files changed, 145 insertions(+), 10 deletions(-) create mode 100644 arch/arm/mach-tegra/secureos.c create mode 100644 arch/arm/mach-tegra/secureos.h