Re: [PATCH] ARM: tegra: add basic SecureOS support

From: Tomasz Figa
Date: Thu Jun 06 2013 - 06:17:16 EST

Next message: Mark Brown: "Re: [PATCH 1/2] spi: spi-omap2-mcspi.c: Add dts for slave deviceconfiguration."
Previous message: Miao Xie: "Re: [PATCH] btrfs: Drop inode if inode root is NULL"
In reply to: Alex Courbot: "Re: [PATCH] ARM: tegra: add basic SecureOS support"
Next in thread: Alex Courbot: "Re: [PATCH] ARM: tegra: add basic SecureOS support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Alex,

On Thursday 06 of June 2013 16:28:07 Alexandre Courbot wrote:
> Boot loaders on some Tegra devices can be unlocked but do not let the
> system operate without SecureOS. SecureOS prevents access to some
> registers and requires the operating system to perform certain
> operations through Secure Monitor Calls instead of directly accessing
> the hardware.
>
> This patch introduces basic SecureOS support for Tegra. SecureOS support
> can be enabled by adding a "nvidia,secure-os" property to the "chosen"
> node of the device tree.
>
> Currently, only the bringup of secondary CPUs is performed by SMCs, but
> more operations will be added later.
>
> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
> ---
> Documentation/devicetree/bindings/arm/tegra.txt | 8 +++
> arch/arm/configs/tegra_defconfig | 1 +
> arch/arm/mach-tegra/Kconfig | 11 ++++
> arch/arm/mach-tegra/Makefile | 2 +
> arch/arm/mach-tegra/common.c | 2 +
> arch/arm/mach-tegra/reset.c | 30 +++++++----
> arch/arm/mach-tegra/secureos.c | 70
> +++++++++++++++++++++++++ arch/arm/mach-tegra/secureos.h
> | 31 +++++++++++ 8 files changed, 145 insertions(+), 10 deletions(-)
> create mode 100644 arch/arm/mach-tegra/secureos.c
> create mode 100644 arch/arm/mach-tegra/secureos.h
>
> diff --git a/Documentation/devicetree/bindings/arm/tegra.txt
> b/Documentation/devicetree/bindings/arm/tegra.txt index
> ed9c853..b543091 100644
> --- a/Documentation/devicetree/bindings/arm/tegra.txt
> +++ b/Documentation/devicetree/bindings/arm/tegra.txt
> @@ -32,3 +32,11 @@ board-specific compatible values:
> nvidia,whistler
> toradex,colibri_t20-512
> toradex,iris
> +
> +Global properties
> +-------------------------------------------
> +
> +The following properties can be specified into the "chosen" root
> +node:
> +
> + nvidia,secure-os: enable SecureOS.

Hmm, on Exynos we had something like

firmware@0203F000 {
compatible = "samsung,secure-firmware";
reg = <0x0203F000 0x1000>;
};

but your solution might be actually the proper one, since firmware is not
a hardware block. (The address in reg property is pointing to SYSRAM
memory, which is an additional communication channel with the firmware.)

> diff --git a/arch/arm/configs/tegra_defconfig
> b/arch/arm/configs/tegra_defconfig index f7ba3161..f6ed0f5 100644
> --- a/arch/arm/configs/tegra_defconfig
> +++ b/arch/arm/configs/tegra_defconfig
> @@ -28,6 +28,7 @@ CONFIG_ARCH_TEGRA_3x_SOC=y
> CONFIG_ARCH_TEGRA_114_SOC=y
> CONFIG_TEGRA_PCI=y
> CONFIG_TEGRA_EMC_SCALING_ENABLE=y
> +CONFIG_TEGRA_SECUREOS=y
> CONFIG_SMP=y
> CONFIG_PREEMPT=y
> CONFIG_AEABI=y
> diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
> index 84d72fc..acb5d0a 100644
> --- a/arch/arm/mach-tegra/Kconfig
> +++ b/arch/arm/mach-tegra/Kconfig
> @@ -87,4 +87,15 @@ config TEGRA_AHB
> config TEGRA_EMC_SCALING_ENABLE
> bool "Enable scaling the memory frequency"
>
> +config TEGRA_SECUREOS
> + bool "Enable SecureOS support"
> + help
> + Support for Tegra devices which bootloader sets up a
> + SecureOS environment. This will use Secure Monitor Calls
> + instead of directly accessing the hardware for some protected
> + operations.
> +
> + SecureOS support is enabled by declaring a "nvidia,secure-os"
> + property into the "chosen" node of the device tree.
> +
> endmenu
> diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile
> index d011f0a..3adafe6 100644
> --- a/arch/arm/mach-tegra/Makefile
> +++ b/arch/arm/mach-tegra/Makefile
> @@ -37,3 +37,5 @@ endif
> obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += board-harmony-pcie.o
>
> obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += board-paz00.o
> +
> +obj-$(CONFIG_TEGRA_SECUREOS) += secureos.o
> diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c
> index 9f852c6..b7eea02 100644
> --- a/arch/arm/mach-tegra/common.c
> +++ b/arch/arm/mach-tegra/common.c
> @@ -37,6 +37,7 @@
> #include "sleep.h"
> #include "pm.h"
> #include "reset.h"
> +#include "secureos.h"
>
> /*
> * Storage for debug-macro.S's state.
> @@ -97,6 +98,7 @@ static void __init tegra_init_cache(void)
>
> void __init tegra_init_early(void)
> {
> + tegra_init_secureos();
> tegra_cpu_reset_handler_init();
> tegra_apb_io_init();
> tegra_init_fuse();
> diff --git a/arch/arm/mach-tegra/reset.c b/arch/arm/mach-tegra/reset.c
> index 1ac434e..4b9ebf9 100644
> --- a/arch/arm/mach-tegra/reset.c
> +++ b/arch/arm/mach-tegra/reset.c
> @@ -21,38 +21,32 @@
>
> #include <asm/cacheflush.h>
> #include <asm/hardware/cache-l2x0.h>
> +#include <asm/firmware.h>
>
> #include "iomap.h"
> #include "irammap.h"
> #include "reset.h"
> #include "sleep.h"
> #include "fuse.h"
> +#include "secureos.h"
>
> #define TEGRA_IRAM_RESET_BASE (TEGRA_IRAM_BASE + \
> TEGRA_IRAM_RESET_HANDLER_OFFSET)
>
> static bool is_enabled;
>
> -static void __init tegra_cpu_reset_handler_enable(void)
> +static void __init tegra_cpu_reset_handler_set(const u32 reset_address)
> {
> - void __iomem *iram_base = IO_ADDRESS(TEGRA_IRAM_RESET_BASE);
> void __iomem *evp_cpu_reset =
> IO_ADDRESS(TEGRA_EXCEPTION_VECTORS_BASE + 0x100);
> void __iomem *sb_ctrl = IO_ADDRESS(TEGRA_SB_BASE);
> u32 reg;
>
> - BUG_ON(is_enabled);
> - BUG_ON(tegra_cpu_reset_handler_size >
TEGRA_IRAM_RESET_HANDLER_SIZE);
> -
> - memcpy(iram_base, (void *)__tegra_cpu_reset_handler_start,
> - tegra_cpu_reset_handler_size);
> -
> /*
> * NOTE: This must be the one and only write to the EVP CPU reset
> * vector in the entire system.
> */
> - writel(TEGRA_IRAM_RESET_BASE + tegra_cpu_reset_handler_offset,
> - evp_cpu_reset);
> + writel(reset_address, evp_cpu_reset);
> wmb();
> reg = readl(evp_cpu_reset);
>
> @@ -66,6 +60,22 @@ static void __init
> tegra_cpu_reset_handler_enable(void) writel(reg, sb_ctrl);
> wmb();
> }
> +}
> +
> +static void __init tegra_cpu_reset_handler_enable(void)
> +{
> + void __iomem *iram_base = IO_ADDRESS(TEGRA_IRAM_RESET_BASE);
> + const u32 reset_address = TEGRA_IRAM_RESET_BASE +
> +
tegra_cpu_reset_handler_offset;
> +
> + BUG_ON(is_enabled);
> + BUG_ON(tegra_cpu_reset_handler_size >
TEGRA_IRAM_RESET_HANDLER_SIZE);
> +
> + memcpy(iram_base, (void *)__tegra_cpu_reset_handler_start,
> + tegra_cpu_reset_handler_size);
> +
> + if (call_firmware_op(set_cpu_boot_addr, 0, reset_address) == -
ENOSYS)
> + tegra_cpu_reset_handler_set(reset_address);
>
> is_enabled = true;
> }

I think this patch could be split into several patches:
- add support for firmware
- split reset function
- add reset support using firmware.

> diff --git a/arch/arm/mach-tegra/secureos.c
> b/arch/arm/mach-tegra/secureos.c new file mode 100644
> index 0000000..44c3514
> --- /dev/null
> +++ b/arch/arm/mach-tegra/secureos.c
> @@ -0,0 +1,70 @@
> +/*
> + * SecureOS support for Tegra CPUs
> + *
> + * Copyright (c) 2013, NVIDIA Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published
> by + * the Free Software Foundation; either version 2 of the License,
> or + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> WITHOUT + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> General Public License for + * more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/of.h>
> +#include <asm/firmware.h>
> +
> +static int __attribute__((used)) __tegra_smc_stack[10];
> +
> +/*
> + * With EABI, subtype and arg already end up in r0, r1 and r2 as they
> are + * function arguments, but we prefer to play safe here and
> explicitly move + * these values into the expected registers anyway.
> mov instructions without + * any side-effect are turned into nops by
> the assembler, which limits + * overhead.
> + */
> +static void tegra_generic_smc(u32 type, u32 subtype, u32 arg)
> +{
> + asm volatile(
> + ".arch_extension sec\n\t"
> + "ldr r3, =__tegra_smc_stack\n\t"
> + "stmia r3, {r4-r12, lr}\n\t"
> + "mov r0, %[type]\n\t"
> + "mov r1, %[subtype]\n\t"
> + "mov r2, %[arg]\n\t"
> + "mov r3, #0\n\t"
> + "mov r4, #0\n\t"
> + "dsb\n\t"
> + "smc #0\n\t"
> + "ldr r3, =__tegra_smc_stack\n\t"
> + "ldmia r3, {r4-r12, lr}"
> + :
> + : [type] "r" (type),
> + [subtype] "r" (subtype),
> + [arg] "r" (arg)
> + : "r0", "r1", "r2", "r3", "r4", "memory");
> +}

Hmm, I wonder if you need all this complexity here. Have a look at our
exynos_smc function

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-exynos/exynos-smc.S?id=refs/tags/next-20130606

and feel free to uncover any problems of it ;) .

> +
> +static int tegra_set_cpu_boot_addr(int cpu, unsigned long boot_addr)
> +{
> + tegra_generic_smc(0xfffff200, boot_addr, 0);
> +
> + return 0;
> +}
> +
> +static const struct firmware_ops tegra_firmware_ops = {
> + .set_cpu_boot_addr = tegra_set_cpu_boot_addr,
> +};

It's good that this interface is finally getting some user besides Exynos.

Best regards,
Tomasz

> +void __init tegra_init_secureos(void)
> +{
> + struct device_node *node = of_find_node_by_path("/chosen");
> +
> + if (node && of_property_read_bool(node, "nvidia,secure-os"))
> + register_firmware_ops(&tegra_firmware_ops);
> +}
> diff --git a/arch/arm/mach-tegra/secureos.h
> b/arch/arm/mach-tegra/secureos.h new file mode 100644
> index 0000000..5388cc5
> --- /dev/null
> +++ b/arch/arm/mach-tegra/secureos.h
> @@ -0,0 +1,31 @@
> +/*
> + * Copyright (c) 2013, NVIDIA Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> it + * under the terms and conditions of the GNU General Public
> License, + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but
> WITHOUT + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> General Public License for + * more details.
> + */
> +
> +#ifndef __TEGRA_SECUREOS_H
> +#define __TEGRA_SECUREOS_H
> +
> +#ifdef CONFIG_TEGRA_SECUREOS
> +
> +#include <linux/types.h>
> +
> +void tegra_init_secureos(void);
> +
> +#else
> +
> +static inline void tegra_init_secureos(void)
> +{
> +}
> +
> +#endif
> +
> +#endif
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Mark Brown: "Re: [PATCH 1/2] spi: spi-omap2-mcspi.c: Add dts for slave deviceconfiguration."
Previous message: Miao Xie: "Re: [PATCH] btrfs: Drop inode if inode root is NULL"
In reply to: Alex Courbot: "Re: [PATCH] ARM: tegra: add basic SecureOS support"
Next in thread: Alex Courbot: "Re: [PATCH] ARM: tegra: add basic SecureOS support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]