ARM: tegra: add basic SecureOS support

PATCH In review Alexandre Courbot 7 participants 29 messages Jun 6, 2013 – Jun 10, 2013

Subsystems (4)

Device tree bindings Arm (32-bit) Arm sub-architectures Tegra architecture support

lore

Share & tools

lore

Alexandre Courbot

author

2013-06-06 07:28 UTC #0 lore raw

PATCH 1/1 ARM: tegra: add basic SecureOS support

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 <[email protected]>

---
 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

Documentation/devicetree/bindings/arm/tegra.txt +8 -0

index ed9c853..b543091 100644
@@ -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.

arch/arm/configs/tegra_defconfig +1 -0

index f7ba3161..f6ed0f5 100644
@@ -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

arch/arm/mach-tegra/Kconfig +11 -0

index 84d72fc..acb5d0a 100644
@@ -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

arch/arm/mach-tegra/Makefile +2 -0

index d011f0a..3adafe6 100644
@@ -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

arch/arm/mach-tegra/common.c +2 -0

index 9f852c6..b7eea02 100644
@@ -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();

arch/arm/mach-tegra/reset.c +20 ~~-10~~

index 1ac434e..4b9ebf9 100644
@@ -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;
 }

arch/arm/mach-tegra/secureos.c new file +70 -0

70 lines not on this page · raw message

arch/arm/mach-tegra/secureos.h new file +31 -0

new file mode 100644
index 0000000..5388cc5
@@ -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

Signature · 1 line

1.8.3

Russell King - ARM Linux

2013-06-06 09:35 UTC #1 lore raw

arch/arm/mach-tegra/secureos.c line 29

Quoted 8 lines · Alexandre Courbot

arch/arm/mach-tegra/secureos.c new file +8 -0

+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...

arch/arm/mach-tegra/secureos.c line 36

Quoted 7 lines

+ */

arch/arm/mach-tegra/secureos.c new file +6 -0

+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");
}

Alex Courbot

author

2013-06-06 10:23 UTC #2 lore raw

Quoted 11 lines · Russell King - ARM Linux

arch/arm/mach-tegra/secureos.c line 29

Quoted 8 lines · Alexandre Courbot

arch/arm/mach-tegra/secureos.c new file +8 -0

+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:

Quoted 3 lines

	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.

Quoted 8 lines

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.

Quoted 14 lines

Also...
arch/arm/mach-tegra/secureos.c line 36
Quoted 7 lines
+ */
arch/arm/mach-tegra/secureos.c new file +6 -0
+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.

Quoted 1 line

What is wrong with stacking r4-r12, lr onto the SVC stack?

Nothing, actually. /o\

Quoted 4 lines

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.

Quoted 6 lines

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)

Quoted 16 lines

{
	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.

Tomasz Figa

2013-06-06 10:17 UTC #3 lore raw

In reply to Alexandre Courbot

Hi Alex,

Quoted 34 lines · 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 <[email protected]>
---
 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
(quoted diff) +0 -0
b/Documentation/devicetree/bindings/arm/tegra.txt index ed9c853..b543091 100644
Documentation/devicetree/bindings/arm/tegra.txt +0 -0
@@ -32,3 +32,11 @@ board-specific compatible values:
   nvidia,whistler

Documentation/devicetree/bindings/arm/tegra.txt board-specific compatible values: · line 42

Quoted 10 lines

Documentation/devicetree/bindings/arm/tegra.txt +8 -0

   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 at 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.)

Quoted 85 lines

(quoted diff) +0 -0

b/arch/arm/configs/tegra_defconfig index f7ba3161..f6ed0f5 100644

arch/arm/configs/tegra_defconfig +1 -0

@@ -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

arch/arm/mach-tegra/Kconfig +11 -0

index 84d72fc..acb5d0a 100644
@@ -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

arch/arm/mach-tegra/Makefile +2 -0

index d011f0a..3adafe6 100644
@@ -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

arch/arm/mach-tegra/common.c +2 -0

index 9f852c6..b7eea02 100644
@@ -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();

arch/arm/mach-tegra/reset.c +2 -1

index 1ac434e..4b9ebf9 100644
@@ -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)

arch/arm/mach-tegra/reset.c line 45

Quoted 10 lines

arch/arm/mach-tegra/reset.c +1 -3

+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);

Quoted 15 lines

arch/arm/mach-tegra/reset.c +1 -6

-
-	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);

arch/arm/mach-tegra/reset.c static void __init · line 69

Quoted 10 lines

arch/arm/mach-tegra/reset.c +7 -0

@@ -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;

arch/arm/mach-tegra/reset.c static void __init tegra_cpu_reset_handler_enable(void)

Quoted 3 lines

arch/arm/mach-tegra/reset.c +3 -0

+
+	BUG_ON(is_enabled);
+	BUG_ON(tegra_cpu_reset_handler_size >

TEGRA_IRAM_RESET_HANDLER_SIZE);

arch/arm/mach-tegra/reset.c static void __init tegra_cpu_reset_handler_enable(void)

Quoted 5 lines

arch/arm/mach-tegra/reset.c +5 -0

+
+	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)

arch/arm/mach-tegra/reset.c static void __init tegra_cpu_reset_handler_enable(void) · line 81

Quoted 4 lines

arch/arm/mach-tegra/reset.c +1 -0

+		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.

Quoted 47 lines

(quoted diff) +0 -0
b/arch/arm/mach-tegra/secureos.c new file mode 100644 index 0000000..44c3514
arch/arm/mach-tegra/secureos.c new file +7 -0
@@ -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.
+ */
arch/arm/mach-tegra/secureos.c new file +10 -0
+
+#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.
+ */
arch/arm/mach-tegra/secureos.c new file +11 -0
+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"

arch/arm/mach-tegra/secureos.c line 51

Quoted 10 lines

arch/arm/mach-tegra/secureos.c new file +10 -0

+		"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 ;) .

arch/arm/mach-tegra/secureos.c line 62

Quoted 11 lines

arch/arm/mach-tegra/secureos.c new file +11 -0

+
+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

Quoted 44 lines

arch/arm/mach-tegra/secureos.c new file +7 -0
+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);
+}
(quoted diff) +0 -0
b/arch/arm/mach-tegra/secureos.h new file mode 100644 index 0000000..5388cc5
arch/arm/mach-tegra/secureos.h new file +4 -0
@@ -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.
+ */
arch/arm/mach-tegra/secureos.h new file +19 -0
+
+#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

Alex Courbot

author

2013-06-06 10:37 UTC #4 lore raw

Hi Tomasz,

Quoted 10 lines · Tomasz Figa

Documentation/devicetree/bindings/arm/tegra.txt board-specific compatible values: · line 42
Quoted 7 lines
Documentation/devicetree/bindings/arm/tegra.txt +7 -0
+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

Quoted 8 lines

         firmware at 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.

Quoted 4 lines

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?

Quoted 4 lines

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.

Quoted 5 lines

arch/arm/mach-tegra/secureos.c line 62
Quoted 3 lines
arch/arm/mach-tegra/secureos.c new file +3 -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.

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.

Stephen Warren

2013-06-06 16:28 UTC #5 lore raw

Quoted 3 lines · Alex Courbot

Hi Tomasz,
On 06/06/2013 07:17 PM, Tomasz Figa wrote:

...

Quoted 7 lines

Quoted 4 lines
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.

Dave Martin

2013-06-06 11:11 UTC #6 lore raw

In reply to Tomasz Figa

Quoted 50 lines · Tomasz Figa

Hi Alex,
Quoted 34 lines · 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 <[email protected]>
---
 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
(quoted diff) +0 -0
b/Documentation/devicetree/bindings/arm/tegra.txt index ed9c853..b543091 100644
Documentation/devicetree/bindings/arm/tegra.txt +0 -0
@@ -32,3 +32,11 @@ board-specific compatible values:
   nvidia,whistler
Documentation/devicetree/bindings/arm/tegra.txt board-specific compatible values: · line 42
Quoted 10 lines
Documentation/devicetree/bindings/arm/tegra.txt +8 -0
   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

Quoted 8 lines

	firmware at 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

Dave Martin

2013-06-06 11:02 UTC #7 lore raw

In reply to Alexandre Courbot

Quoted 193 lines · Alexandre Courbot

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 <[email protected]>

---
 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

Documentation/devicetree/bindings/arm/tegra.txt +8 -0

index ed9c853..b543091 100644
@@ -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.

arch/arm/configs/tegra_defconfig +1 -0

index f7ba3161..f6ed0f5 100644
@@ -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

arch/arm/mach-tegra/Kconfig +11 -0

index 84d72fc..acb5d0a 100644
@@ -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

arch/arm/mach-tegra/Makefile +2 -0

index d011f0a..3adafe6 100644
@@ -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

arch/arm/mach-tegra/common.c +2 -0

index 9f852c6..b7eea02 100644
@@ -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();

arch/arm/mach-tegra/reset.c +20 ~~-10~~

index 1ac434e..4b9ebf9 100644
@@ -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;
 }

arch/arm/mach-tegra/secureos.c new file +12 -0

new file mode 100644
index 0000000..44c3514
@@ -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

arch/arm/mach-tegra/secureos.c line 22

Quoted 10 lines

arch/arm/mach-tegra/secureos.c new file +10 -0

+ * 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.

arch/arm/mach-tegra/secureos.c line 35

Quoted 13 lines

arch/arm/mach-tegra/secureos.c new file +13 -0

+
+/*
+ * 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

arch/arm/mach-tegra/secureos.c line 42

Quoted 7 lines

arch/arm/mach-tegra/secureos.c new file +7 -0

+		"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?

arch/arm/mach-tegra/secureos.c line 50

Quoted 8 lines

arch/arm/mach-tegra/secureos.c new file +8 -0

+		"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

Alexandre Courbot

2013-06-07 07:25 UTC #8 lore raw

Quoted 14 lines · Dave Martin

Quoted 1 line

+static int __attribute__((used)) __tegra_smc_stack[10];

Use __used instead of using GCC attributes directly.

Quoted 10 lines

arch/arm/mach-tegra/secureos.c new file +10 -0

+
+/*
+ * 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)
+{

Quoted 8 lines

arch/arm/mach-tegra/secureos.c line 35
Quoted 3 lines
arch/arm/mach-tegra/secureos.c new file +3 -0
+     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.

Quoted 3 lines

Quoted 1 line
+ "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?

Quoted 10 lines

arch/arm/mach-tegra/secureos.c line 50

Quoted 8 lines

arch/arm/mach-tegra/secureos.c new file +8 -0

+             "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.

Quoted 2 lines

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.

Dave Martin

2013-06-07 17:30 UTC #9 lore raw

Quoted 28 lines · Alexandre Courbot

Quoted 14 lines · Dave Martin
Quoted 1 line
+static int __attribute__((used)) __tegra_smc_stack[10];
Use __used instead of using GCC attributes directly.
Quoted 10 lines
arch/arm/mach-tegra/secureos.c new file +10 -0
+
+/*
+ * 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)
+{
Quoted 8 lines
arch/arm/mach-tegra/secureos.c line 35
Quoted 3 lines
arch/arm/mach-tegra/secureos.c new file +3 -0
+     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.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
+ "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.

Quoted 9 lines

Quoted 8 lines

arch/arm/mach-tegra/secureos.c new file +8 -0

+             "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");

Quoted 3 lines

Quoted 1 line
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.

Quoted 5 lines

Quoted 3 lines
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

Alexandre Courbot

2013-06-10 07:47 UTC #10 lore raw

Quoted 19 lines · Dave Martin

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.

Quoted 6 lines

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.

Quoted 11 lines

Quoted 10 lines

arch/arm/mach-tegra/secureos.c line 50

Quoted 8 lines

arch/arm/mach-tegra/secureos.c new file +8 -0

+             "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?

Quoted 8 lines

Quoted 1 line
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.

Russell King - ARM Linux

2013-06-10 09:10 UTC #11 lore raw

Quoted 6 lines · Alexandre Courbot

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.

Jassi Brar

2013-06-06 12:26 UTC #12 lore raw

In reply to Alexandre Courbot

Quoted 6 lines · Alexandre Courbot

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?

Quoted 4 lines

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

Alexandre Courbot

2013-06-07 07:13 UTC #13 lore raw

Quoted 10 lines · Jassi Brar

Quoted 6 lines · Alexandre Courbot
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.

Quoted 8 lines

Quoted 4 lines
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.

Jassi Brar

2013-06-07 08:52 UTC #14 lore raw

Quoted 8 lines · Alexandre Courbot

Quoted 7 lines · Jassi Brar
Quoted 6 lines · Alexandre Courbot
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.

Quoted 6 lines

Quoted 3 lines
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.

Quoted 9 lines

Quoted 8 lines
Quoted 4 lines
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).

Quoted 6 lines

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

Stephen Warren

2013-06-06 16:44 UTC #15 lore raw

In reply to Alexandre Courbot

Quoted 9 lines · Alexandre Courbot

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.

Documentation/devicetree/bindings/arm/tegra.txt board-specific compatible values: · line 42

Quoted 5 lines

Documentation/devicetree/bindings/arm/tegra.txt +3 -0
 
+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.

arch/arm/mach-tegra/secureos.c line 70

Quoted 9 lines

arch/arm/mach-tegra/secureos.c +7 -0

 
+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()

Dave Martin

2013-06-06 18:08 UTC #16 lore raw

Quoted 15 lines · Stephen Warren

Quoted 9 lines · Alexandre Courbot
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:-(

Quoted 7 lines

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.

Quoted 12 lines

Documentation/devicetree/bindings/arm/tegra.txt board-specific compatible values: · line 42
Quoted 6 lines
Documentation/devicetree/bindings/arm/tegra.txt +3 -0
 
+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:

Quoted 1 line

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.)

Quoted 25 lines

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.
arch/arm/mach-tegra/secureos.c line 70
Quoted 9 lines
arch/arm/mach-tegra/secureos.c +7 -0
 
+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?

Quoted 8 lines

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

Stephen Warren

2013-06-06 18:29 UTC #17 lore raw

Quoted 11 lines · Dave Martin

Quoted 10 lines · Stephen Warren
Quoted 9 lines · Alexandre Courbot
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.

...

Quoted 16 lines

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";

		// ...
	};

Quoted 1 line

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.

Quoted 4 lines

...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.

Dave Martin

2013-06-07 17:47 UTC #18 lore raw

Quoted 26 lines · Stephen Warren

Quoted 11 lines · Dave Martin
Quoted 10 lines · Stephen Warren
Quoted 9 lines · Alexandre Courbot
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.
...
Quoted 13 lines
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";

Quoted 6 lines

Quoted 4 lines
		// ...
	};
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).

Quoted 10 lines

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.

Quoted 8 lines

Quoted 4 lines
...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

Alexandre Courbot

2013-06-07 09:03 UTC #19 lore raw

In reply to Dave Martin

Quoted 29 lines · Dave Martin

Quoted 2 lines
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

Quoted 8 lines

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.

Dave Martin

2013-06-07 18:13 UTC #20 lore raw

Quoted 45 lines · Alexandre Courbot

Quoted 29 lines · Dave Martin
Quoted 2 lines
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
Quoted 8 lines
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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 7 lines

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

Alexandre Courbot

2013-06-10 08:05 UTC #21 lore raw

Quoted 76 lines · Dave Martin

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.
Quoted 4 lines
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.
Quoted 6 lines
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.

Quoted 6 lines

Quoted 1 line
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.

Dave Martin

2013-06-10 11:20 UTC #22 lore raw

Quoted 84 lines · Alexandre Courbot

Quoted 76 lines · Dave Martin
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.
Quoted 4 lines
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.
Quoted 6 lines
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.
Quoted 6 lines
Quoted 1 line
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.

Quoted 10 lines

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

Alexandre Courbot

2013-06-07 08:11 UTC #23 lore raw

In reply to Stephen Warren

Quoted 7 lines · Stephen Warren

Quoted 6 lines · Alexandre Courbot
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.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 7 lines

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?

Quoted 9 lines

Documentation/devicetree/bindings/arm/tegra.txt board-specific compatible values: · line 42
Quoted 3 lines
Documentation/devicetree/bindings/arm/tegra.txt +3 -0
+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:

Quoted 5 lines

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.

Quoted 23 lines

Similar comments may apply to the CONFIG_ option and description, filenames, etc.
arch/arm/mach-tegra/secureos.c line 70
Quoted 9 lines
arch/arm/mach-tegra/secureos.c +7 -0
 
+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:

Quoted 8 lines

find node
if node exists

    if (!IS_ENABLED(SECURE_OS))
        BUG/WARN/...
    else
        register_firmware_ops()

Yep, let's do it this way.

Alex.

Stephen Warren

2013-06-07 16:33 UTC #24 lore raw

Quoted 25 lines · Alexandre Courbot

Quoted 7 lines · Stephen Warren
Quoted 6 lines · Alexandre Courbot
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 5 lines
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.

Quoted 8 lines

Quoted 2 lines
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?

Alexandre Courbot

2013-06-10 08:11 UTC #25 lore raw

Quoted 9 lines · Stephen Warren

Quoted 8 lines
Quoted 2 lines
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?

Quoted 8 lines

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.

Quoted 8 lines

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.

Russell King - ARM Linux

2013-06-10 09:14 UTC #26 lore raw

Quoted 17 lines · Alexandre Courbot

Quoted 16 lines · Stephen Warren
Quoted 8 lines
Quoted 2 lines
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.

Quoted 3 lines

Quoted 1 line
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.

Stephen Warren

2013-06-10 16:35 UTC #27 lore raw

Quoted 18 lines · Russell King - ARM Linux

Quoted 17 lines · Alexandre Courbot
Quoted 16 lines · Stephen Warren
Quoted 8 lines
Quoted 2 lines
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.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
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.

Dave Martin

2013-06-10 11:16 UTC #28 lore raw

In reply to Alexandre Courbot

Quoted 17 lines · Alexandre Courbot

Quoted 16 lines · Stephen Warren
Quoted 8 lines
Quoted 2 lines
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.

Quoted 3 lines

Quoted 1 line
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.

Quoted 9 lines

Quoted 8 lines
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?

Quoted 8 lines

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