Re: [ptxdist] [PATCH v2] tf-a: new package for ARM trusted firmware A

[Michael Olbrich <m.olbrich@pengutronix.de>]

On Wed, Feb 12, 2020 at 05:40:33PM +0100, Ahmad Fatoum wrote:
> Trusted Firmware-A (TF-A) is a reference implementation of secure world
> software for Arm A-Profile architectures (Armv8-A and Armv7-A).
> 
> Cc: Alejandro Vazquez <avazquez.dev@gmail.com>
> Signed-off-by: Rouven Czerwinski <rouven@czerwinskis.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> v1 -> v2:
>  - Made TF_A_ARCH_MAJOR configurable to support 32 bit ARMv8 (Guillermo)
>  - Replaces stm32mp-specific TF_A_DTB with TF_A_EXTRA_ARGS to contain
>    all board/vendor specific options
>  - removed reference to no longer existing CREDITS file
>  - removed TF_A_MAKE_OPT contents that are set elsewhere
>  - reduced uses of += in favor of directly appending to the string
>  - delete old build directory in prepare instead of compile
>  - use default compile stage (Guillermo)
>  - install artifacts to sysroot /usr/lib/firmware in install stage
>  - install artifacts to IMAGEDIR in targetinstall
>  - fix clean stage to delete proper artifacts
> ---
>  platforms/tf-a.in | 138 ++++++++++++++++++++++++++++++++++++++++++++++
>  rules/tf-a.make   | 114 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 252 insertions(+)
>  create mode 100644 platforms/tf-a.in
>  create mode 100644 rules/tf-a.make
> 
> diff --git a/platforms/tf-a.in b/platforms/tf-a.in
> new file mode 100644
> index 000000000000..f3971c4c2a3a
> --- /dev/null
> +++ b/platforms/tf-a.in
> @@ -0,0 +1,138 @@
> +## SECTION=bootloader
> +
> +menuconfig TF_A
> +	select BOOTLOADER
> +	prompt "ARM Trusted Firmware-A"

Spaces at the end to align the '-->'

> +	depends on ARCH_ARM || ARCH_ARM64
> +	bool
> +
> +if TF_A
> +
> +config TF_A_ARCH_STRING
> +        string
> +        default "aarch32" if ARCH_ARM
> +        default "aarch64" if ARCH_ARM64
> +
> +choice
> +	prompt "TF-A Architecture"
> +	default TF_A_ARM_ARCH_MAJOR_7 if ARCH_ARM
> +	default TF_A_ARM_ARCH_MAJOR_8 if ARCH_ARM64
> +	help
> +	  Architecture version major number
> +
> +	config TF_A_ARM_ARCH_MAJOR_7
> +		depends on ARCH_ARM
> +		prompt "ARMv7"
> +		bool
> +
> +	config TF_A_ARM_ARCH_MAJOR_8_32_BIT
> +		depends on ARCH_ARM
> +		prompt "ARMv8 32-bit"
> +		bool
> +
> +	config TF_A_ARM_ARCH_MAJOR_8
> +		depends on ARCH_ARM64
> +		prompt "ARMv8"
> +		bool
> +
> +endchoice
> +
> +config TF_A_ARM_ARCH_MAJOR
> +        int
> +        default 7 if TF_A_ARM_ARCH_MAJOR_7
> +        default 8 if TF_A_ARM_ARCH_MAJOR_8_32_BIT
> +        default 8 if TF_A_ARM_ARCH_MAJOR_8
> +
> +config TF_A_VERSION
> +	string
> +	default "v2.2"
> +	prompt "TF-A version"
> +	help
> +	  Enter the TF-A version you want to build. Usally something like "v2.2"

You should mention that this is the tag name from the git repository.

> +
> +config TF_A_MD5
> +	string
> +	default "bb300e5a62c911e189c80d935d497a4b"
> +	prompt "TF-A source md5"

TF_A_VERSION and TF_A_MD5 should be the first two sub options.

> +
> +config TF_A_PLATFORM
> +	string
> +	prompt "TF-A target platform"
> +	help
> +	  The TF-A target platform.
> +
> +config TF_A_ARM_ARCH_MINOR
> +	depends on TF_A_ARM_ARCH_MAJOR_8 || TF_A_ARM_ARCH_MAJOR_8_32_BIT
> +	int
> +	default 0
> +	prompt "TF-A target ARMv8.MINOR version"
> +	help
> +	  The minor version of the ARMv8 architecture targeted. Defaults to 0.
> +
> +config TF_A_EXTRA_ARGS
> +	string
> +	prompt "TF-A extra build arguments"
> +	help
> +	  Extra platform-specific build arguments to pass to the TF-A build
> +	  process, e.g. DTB_FILE_NAME= for the stm32mp1
> +
> +config TF_A_ARTIFACTS
> +	string
> +	prompt "TF-A artifact file names"
> +	default "bl2.bin"
> +	help
> +	  A space-separated list of artifacts to copy to the image directory.
> +	  All file names are relative to the appropriate TF-A platform build
> +	  directory.

Where does the default come from? When I tried this, the only file that
existed was 'bl31.bin'.

> +
> +comment "Payloads"
> +
> +choice
> +	prompt "BL32 Payload"
> +	default TF_A_BL32_NONE
> +	help
> +	  payload for BL32 (Secure World OS)
> +
> +	config TF_A_BL32_NONE
> +		prompt "None"
> +		bool
> +
> +	config TF_A_BL32_SP_MIN
> +		depends on ARCH_ARM
> +		prompt "sp_min"
> +		bool
> +
> +	config TF_A_BL32_TSP
> +		depends on ARCH_ARM64
> +		prompt "Test Secure Payload"
> +		bool
> +
> +endchoice
> +
> +if TF_A_BL32_TSP
> +choice TF_A_BL32_TSP_RAM_LOCATION
> +	prompt "TSP location"
> +	default TF_A_BL32_TSP_RAM_LOCATION_TSRAM
> +
> +	config TF_A_BL32_TSP_RAM_LOCATION_TSRAM
> +		prompt "Trusted SRAM"
> +		bool
> +
> +	config TF_A_BL32_TSP_RAM_LOCATION_TDRAM
> +		prompt "Trusted DRAM (if available)"
> +		bool
> +
> +	config TF_A_BL32_TSP_RAM_LOCATION_DRAM
> +		prompt "Secure DRAM region (configured by TrustZone controller)"
> +		bool
> +endchoice
> +
> +config TF_A_BL32_TSP_RAM_LOCATION_STRING
> +        string
> +        default "tsram" if TF_A_BL32_TSP_RAM_LOCATION_TSRAM
> +        default "tdram" if TF_A_BL32_TSP_RAM_LOCATION_TDRAM
> +        default "dram"  if TF_A_BL32_TSP_RAM_LOCATION_DRAM
> +
> +endif
> +
> +endif
> diff --git a/rules/tf-a.make b/rules/tf-a.make
> new file mode 100644
> index 000000000000..9f895d32518d
> --- /dev/null
> +++ b/rules/tf-a.make
> @@ -0,0 +1,114 @@
> +# -*-makefile-*-
> +#
> +# Copyright (C) 2018 by Rouven Czerwinski <r.czerwinski@pengutronix.de>
> +#               2019 by Ahmad Fatoum <a.fatoum@pengutronix.de>
> +#
> +# For further information about the PTXdist project and license conditions
> +# see the README file.
> +#
> +
> +#
> +# We provide this package
> +#
> +PACKAGES-$(PTXCONF_TF_A) += tf-a
> +
> +#
> +# Paths and names
> +#
> +TF_A_VERSION	:= $(call remove_quotes,$(PTXCONF_TF_A_VERSION))
> +TF_A_MD5	:= $(call remove_quotes,$(PTXCONF_TF_A_MD5))
> +TF_A		:= tf-a-$(TF_A_VERSION)
> +TF_A_SUFFIX	:= tar.gz
> +TF_A_URL	:= https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/snapshot/$(TF_A_VERSION).$(TF_A_SUFFIX)
> +TF_A_SOURCE	:= $(SRCDIR)/$(TF_A).$(TF_A_SUFFIX)
> +TF_A_DIR	:= $(BUILDDIR)/$(TF_A)
> +TF_A_LICENSE    := BSD-3-Clause
> +
> +# ----------------------------------------------------------------------------
> +# Prepare
> +# ----------------------------------------------------------------------------
> +
> +TF_A_WRAPPER_BLACKLIST := \
> +	TARGET_HARDEN_RELRO \
> +	TARGET_HARDEN_BINDNOW \
> +	TARGET_HARDEN_PIE \
> +	TARGET_DEBUG \
> +	TARGET_BUILD_ID
> +
> +TF_A_PATH	:= PATH=$(CROSS_PATH)
> +TF_A_MAKE_OPT	:= \
> +	CROSS_COMPILE=$(BOOTLOADER_CROSS_COMPILE) \
> +	HOSTCC=$(HOSTCC) \
> +	PLAT=$(PTXCONF_TF_A_PLATFORM) \
> +	DEBUG=$(call ptx/ifdef,TF_A_DEBUG,1,0) \

No, don't use variables that are never defined.
If the rule must be copied anyways, then editing this is simple.

> +	ARCH=$(PTXCONF_TF_A_ARCH_STRING) \
> +	ARM_ARCH_MAJOR=$(PTXCONF_TF_A_ARM_ARCH_MAJOR) \
> +	$(call remove_quotes,$(PTXCONF_TF_A_EXTRA_ARGS)) \
> +	all
> +
> +ifdef PTXCONF_TF_A_BL32_TSP
> +TF_A_MAKE_OPT += ARM_TSP_RAM_LOCATION=$(PTXCONF_TF_A_BL32_TSP_RAM_LOCATION_STRING)
> +endif
> +ifdef PTXCONF_TF_A_ARM_ARCH_MINOR
> +TF_A_MAKE_OPT += ARM_ARCH_MINOR=$(PTXCONF_TF_A_ARM_ARCH_MINOR)
> +endif
> +ifdef PTXCONF_TF_A_BL32_SP_MIN
> +TF_A_MAKE_OPT += AARCH32_SP=sp_min
> +endif
> +
> +TF_A_BUILD_OUTPUT_DIR := $(call ptx/ifdef,TF_A_DEBUG,debug,release)

Same here.

> +TF_A_ARTIFACTS_DEST := $(call remove_quotes,$(PTXCONF_TF_A_ARTIFACTS))
> +TF_A_ARTIFACTS_SRC := $(addprefix $(TF_A_DIR)/build/$(PTXCONF_TF_A_PLATFORM)/$(TF_A_BUILD_OUTPUT_DIR)/, \
> +		$(TF_A_ARTIFACTS_DEST))
> +TF_A_CONF_TOOL	:= NO
> +TF_A_MAKE_ENV	:= $(CROSS_ENV)

Move this variable below the prepare stage. That's for compile.

> +# TF_A_DEBUG=yes

remove. 

> +
> +# ----------------------------------------------------------------------------
> +# Prepare
> +# ----------------------------------------------------------------------------

This is all the prepare stage. Don't add a second header.

> +$(STATEDIR)/tf-a.prepare:
> +	@$(call targetinfo)
> +	@rm -rf $(TF_A_DIR)/build/
> +	@$(call touch)
> +
> +# ----------------------------------------------------------------------------
> +# Install
> +# ----------------------------------------------------------------------------
> +
> +$(STATEDIR)/tf-a.install:
> +	@$(call targetinfo)
> +ifeq ($(TF_A_ARTIFACTS_SRC),)
> +	$(warning TF_A_ARTIFACTS is empty. nothing to install.)
> +else
> +	@install -m644 -D \
> +		--target-directory=$(PTXCONF_SYSROOT_TARGET)/usr/lib/firmware \

Never use PTXCONF_SYSROOT_TARGET as a target. Install to TF_A_PKGDIR
instead. This will be synct to PTXCONF_SYSROOT_TARGET during install.post.

and use '-v' to make this visible.

> +		$(TF_A_ARTIFACTS_SRC)
> +endif
> +	@$(call touch)
> +
> +# ----------------------------------------------------------------------------
> +# Target-Install
> +# ----------------------------------------------------------------------------
> +
> +$(STATEDIR)/tf-a.targetinstall:
> +	@$(call targetinfo)
> +ifeq ($(TF_A_ARTIFACTS_SRC),)
> +	$(warning TF_A_ARTIFACTS is empty. nothing to install.)

Hmm, the install stage will fail if TF_A_ARTIFACTS_SRC is empty, right?
And I don't think no output is valid in any way, so just put an '$(error )'
below the definition of TF_A_ARTIFACTS_SRC (wrapped by a 'ifdef
PTXCONF_TF_A').

> +else
> +	@install -D -m644 $(TF_A_ARTIFACTS_SRC) $(IMAGEDIR)
> +endif
> +	@$(call touch)
> +
> +# ----------------------------------------------------------------------------
> +# Clean
> +# ----------------------------------------------------------------------------
> +
> +$(STATEDIR)/tf-a.clean:
> +	@$(call targetinfo)
> +	@$(call clean_pkg, TF_A)
> +	@rm -f $(addprefix $(PTXCONF_SYSROOT_TARGET)/usr/lib/firmware/, \
> +		TF_A_ARTIFACTS_DEST)

This is not necessary if you use pkgdir.

Michael

> +	@rm -f $(addprefix $(IMAGEDIR), TF_A_ARTIFACTS_DEST)
> +
> +# vim: syntax=make
> -- 
> 2.25.0
> 
> 
> _______________________________________________
> ptxdist mailing list
> ptxdist@pengutronix.de
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de