Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754702Ab3H3IUx (ORCPT ); Fri, 30 Aug 2013 04:20:53 -0400 Received: from mail-ea0-f171.google.com ([209.85.215.171]:60111 "EHLO mail-ea0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752749Ab3H3IUs (ORCPT ); Fri, 30 Aug 2013 04:20:48 -0400 From: Tomasz Figa To: Alexandre Courbot Cc: Stephen Warren , Russell King - ARM Linux , Dave Martin , gnurou@gmail.com, linux-arm-kernel@lists.infradead.org, linux-tegra@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 1/5] ARM: add basic Trusted Foundations support Date: Fri, 30 Aug 2013 10:20:42 +0200 Message-ID: <2081838.eZHRxlDf7s@flatron> User-Agent: KMail/4.11 (Linux/3.10.9-gentoo; KDE/4.11.0; x86_64; ; ) In-Reply-To: <1377770268-14014-2-git-send-email-acourbot@nvidia.com> References: <1377770268-14014-1-git-send-email-acourbot@nvidia.com> <1377770268-14014-2-git-send-email-acourbot@nvidia.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9863 Lines: 306 Hi Alexandre, Overall this looks good to me, but I have some minor comments. On Thursday 29 of August 2013 18:57:44 Alexandre Courbot wrote: > Trusted Foundations is a TrustZone-based secure monitor for ARM that > can be invoked using a consistent smc-based API on all supported > platforms. This patch adds initial basic support for Trusted > Foundations using the ARM firmware API. Current features are limited > to the ability to boot secondary processors. > > Signed-off-by: Alexandre Courbot > --- > .../arm/firmware/tl,trusted-foundations.txt | 17 +++++ > .../devicetree/bindings/vendor-prefixes.txt | 1 + > arch/arm/Kconfig | 2 + > arch/arm/Makefile | 1 + > arch/arm/firmware/Kconfig | 26 +++++++ > arch/arm/firmware/Makefile | 1 + > arch/arm/firmware/trusted_foundations.c | 83 > ++++++++++++++++++++++ arch/arm/include/asm/trusted_foundations.h > | 48 +++++++++++++ 8 files changed, 179 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundations.t > xt create mode 100644 arch/arm/firmware/Kconfig > create mode 100644 arch/arm/firmware/Makefile > create mode 100644 arch/arm/firmware/trusted_foundations.c > create mode 100644 arch/arm/include/asm/trusted_foundations.h > > diff --git > a/Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundations > .txt > b/Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundations > .txt new file mode 100644 > index 0000000..3954bbd > --- /dev/null > +++ > b/Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundations > .txt @@ -0,0 +1,17 @@ > +Trusted Foundations > + > +Boards that use the Trusted Foundations secure monitor can signal its > +presence by declaring a node compatible with "tl,trusted-foundations" > +under the root node. > + > +Required properties: > +- compatible : "tl,trusted-foundations" > +- version-major : major version number of Trusted Foundations firmware > +- version-minor: minor version number of Trusted Foundations firmware Hmm, maybe you could simply define a single version property that could have multiple cells? Like: firmware { compatible = "tl,trusted-foundations"; version = <2 8>; }; > +Example: > + firmware { > + compatible = "tl,trusted-foundations"; > + version-major = <2>; > + version-minor = <8>; > + }; > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt > b/Documentation/devicetree/bindings/vendor-prefixes.txt index > 366ce9b..20d61f3 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt > @@ -63,6 +63,7 @@ ste ST-Ericsson > stericsson ST-Ericsson > toumaz Toumaz > ti Texas Instruments > +tl Trusted Logic > toshiba Toshiba Corporation > v3 V3 Semiconductor > via VIA Technologies, Inc. > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index 43594d5..7fbe800 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -1074,6 +1074,8 @@ config ARM_TIMER_SP804 > select CLKSRC_MMIO > select CLKSRC_OF if OF > > +source "arch/arm/firmware/Kconfig" > + > source arch/arm/mm/Kconfig > > config ARM_NR_BANKS > diff --git a/arch/arm/Makefile b/arch/arm/Makefile > index 6fd2cea..a48de17 100644 > --- a/arch/arm/Makefile > +++ b/arch/arm/Makefile > @@ -267,6 +267,7 @@ core-$(CONFIG_KVM_ARM_HOST) += arch/arm/kvm/ > core-y += arch/arm/kernel/ arch/arm/mm/ arch/arm/common/ > core-y += arch/arm/net/ > core-y += arch/arm/crypto/ > +core-y += arch/arm/firmware/ > core-y += $(machdirs) $(platdirs) > > drivers-$(CONFIG_OPROFILE) += arch/arm/oprofile/ > diff --git a/arch/arm/firmware/Kconfig b/arch/arm/firmware/Kconfig > new file mode 100644 > index 0000000..fde21d0 > --- /dev/null > +++ b/arch/arm/firmware/Kconfig > @@ -0,0 +1,26 @@ > +config ARCH_SUPPORTS_FIRMWARE > + bool > + > +config ARCH_SUPPORTS_TRUSTED_FOUNDATIONS > + bool > + select ARCH_SUPPORTS_FIRMWARE > + > +menu "Firmware options" > + depends on ARCH_SUPPORTS_FIRMWARE > + > +config TRUSTED_FOUNDATIONS > + bool "Trusted Foundations secure monitor support" > + depends on ARCH_SUPPORTS_TRUSTED_FOUNDATIONS > + help > + Some devices are booted with the Trusted Foundations secure monitor > + active, requiring some core operations to be performed by the secure > + monitor instead of the kernel. > + > + This option allows the kernel to invoke the secure monitor whenever > + required on devices using Trusted Foundations. > + > + Devices using Trusted Foundations should pass a device tree > containing + a node compatible with "tl,trusted-foundations" to > signal the presence + of the secure monitor. What about pointing to the documentation file instead? > + > +endmenu > diff --git a/arch/arm/firmware/Makefile b/arch/arm/firmware/Makefile > new file mode 100644 > index 0000000..a71f165 > --- /dev/null > +++ b/arch/arm/firmware/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o > diff --git a/arch/arm/firmware/trusted_foundations.c > b/arch/arm/firmware/trusted_foundations.c new file mode 100644 > index 0000000..181f348 > --- /dev/null > +++ b/arch/arm/firmware/trusted_foundations.c > @@ -0,0 +1,83 @@ > +/* > + * Trusted Foundations support for ARM CPUs > + * > + * Copyright (c) 2013, NVIDIA Corporation. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published > by + * the Free Software Foundation; either version 2 of the License, > or + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, but > WITHOUT + * ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU > General Public License for + * more details. > + */ > + > +#include > +#include > +#include > +#include > +#include > + > +#define TF_SET_CPU_BOOT_ADDR_SMC 0xfffff200 > + > +static void __naked tf_generic_smc(u32 type, u32 subtype, u32 arg) > +{ > + asm volatile( > + ".arch_extension sec\n\t" > + "stmfd sp!, {r4 - r11, lr}\n\t" > + __asmeq("%0", "r0") > + __asmeq("%1", "r1") > + __asmeq("%2", "r2") > + "mov r3, #0\n\t" > + "mov r4, #0\n\t" > + "smc #0\n\t" > + "ldmfd sp!, {r4 - r11, pc}" > + : > + : "r" (type), "r" (subtype), "r" (arg) > + : "memory"); > +} > + > +static int tf_set_cpu_boot_addr(int cpu, unsigned long boot_addr) > +{ > + tf_generic_smc(TF_SET_CPU_BOOT_ADDR_SMC, boot_addr, 0); > + > + return 0; > +} > + > +static const struct firmware_ops trusted_foundations_ops = { > + .set_cpu_boot_addr = tf_set_cpu_boot_addr, > +}; > + > +void register_trusted_foundations(struct > trusted_foundations_platform_data *pd) +{ > + /* we are not using version information for now since currently > + * supported SMCs are compatible with all TF releases */ > + register_firmware_ops(&trusted_foundations_ops); > +} > + > +void of_register_trusted_foundations(void) > +{ > + struct device_node *node; > + > + node = of_find_compatible_node(NULL, NULL, "tl,trusted- foundations"); nit: if (!node) return; > + if (node) { > + struct trusted_foundations_platform_data pdata; > + int err; > + > + err = of_property_read_u32(node, "version-major", > + &pdata.version_major); > + if (err != 0) { > + pr_crit("Trusted Foundation: missing version- major\n"); > + BUG(); > + } > + err = of_property_read_u32(node, "version-minor", > + &pdata.version_minor); > + if (err != 0) { > + pr_crit("Trusted Foundation: missing version- minor\n"); > + BUG(); > + } If version was stored in multiple cells of a single property, then it would be parsed by just one call to of_property_read_u32_array(). > + register_trusted_foundations(&pdata); > + } > +} > diff --git a/arch/arm/include/asm/trusted_foundations.h > b/arch/arm/include/asm/trusted_foundations.h new file mode 100644 > index 0000000..77a4961 > --- /dev/null > +++ b/arch/arm/include/asm/trusted_foundations.h > @@ -0,0 +1,48 @@ > +/* > + * 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. > + */ > + > +#ifndef __ASM_ARM_TRUSTED_FOUNDATIONS_H > +#define __ASM_ARM_TRUSTED_FOUNDATIONS_H > + > +#include > +#include > +#include > + > +struct trusted_foundations_platform_data { > + unsigned int version_major; > + unsigned int version_minor; > +}; > + > +#if IS_ENABLED(CONFIG_TRUSTED_FOUNDATIONS) > + > +void register_trusted_foundations(struct > trusted_foundations_platform_data *pd); +void > of_register_trusted_foundations(void); > + > +#else > + > +static inline void register_trusted_foundations( > + struct trusted_foundations_platform_data *pd) > +{ > + pr_crit("No support for Trusted Foundations, stopping...\n"); > + BUG(); Hmm, why not simply panic()? Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/