Re: [PATCH v4 1/5] ARM: add basic Trusted Foundations support

[Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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 <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../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 <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/of.h>
> +#include <asm/firmware.h>
> +#include <asm/trusted_foundations.h>
> +
> +#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 <linux/kconfig.h>
> +#include <linux/printk.h>
> +#include <asm/bug.h>
> +
> +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