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

[Guillermo Rodriguez Garcia <guille.rodriguez@gmail.com>]

Hi Ahmad,

Some other questions and comments, please see below.

El lun., 10 feb. 2020 a las 17:16, Ahmad Fatoum
(<a.fatoum@pengutronix.de>) escribió:
>
> 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>
> ---
>  platforms/tf-a.in | 113 ++++++++++++++++++++++++++++++++++++++++++
>  rules/tf-a.make   | 123 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 236 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..5aa4ca473c35
> --- /dev/null
> +++ b/platforms/tf-a.in
> @@ -0,0 +1,113 @@
> +## SECTION=bootloader
> +
> +menuconfig TF_A
> +       select BOOTLOADER
> +       prompt "ARM Trusted Firmware-A"
> +       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
> +
> +config TF_A_ARM_ARCH_MAJOR
> +        int
> +        default "7" if ARCH_ARM
> +        default "8" if ARCH_ARM64

Shouldn't this be configurable?
aarch64 is always v8, but for aarch32 you can have either v7 or v8.

> +
> +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"
> +
> +config TF_A_MD5
> +       string
> +       default "bb300e5a62c911e189c80d935d497a4b"
> +       prompt "TF-A source md5"
> +
> +config TF_A_PLATFORM
> +       string
> +       prompt "TF-A target platform"
> +       help
> +         The TF-A target platform.
> +
> +if ARCH_ARM64
> +config TF_A_ARM_ARCH_MINOR
> +       int
> +       default 0
> +       prompt "TF-A target ARMv8.MINOR version"
> +       help
> +         The minor version of the ARMv8 architecture targeted. Defaults to 0.
> +endif
> +
> +config TF_A_DTB
> +       string
> +       prompt "TF-A DTB file name"
> +       help
> +         Device Tree Binary file name
> +
> +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.
> +
> +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

No way to specify other payloads?

> +
> +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..9ee554476927
> --- /dev/null
> +++ b/rules/tf-a.make
> @@ -0,0 +1,123 @@
> +# -*-makefile-*-
> +#
> +# Copyright (C) 2018 by Rouven Czerwinski <r.czerwinski@pengutronix.de>
> +#               2019 by Ahmad Fatoum <a.fatoum@pengutronix.de>
> +#
> +# See CREDITS for details about who has contributed to this project.
> +#
> +# 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  := \
> +       V=$(PTXDIST_VERBOSE) \
> +       CROSS_COMPILE=$(BOOTLOADER_CROSS_COMPILE) \
> +       HOSTCC=$(HOSTCC)
> +TF_A_CONF_TOOL := NO
> +TF_A_MAKE_ENV  := $(CROSS_ENV)

Do you need $(CROSS_ENV) here? (not used in e.g. u-boot.make)

> +# TF_A_DEBUG=yes
> +
> +
> +$(STATEDIR)/tf-a.prepare:
> +       @$(call targetinfo)
> +       @$(call touch)

I think this does nothing and can be removed.

> +
> +# ----------------------------------------------------------------------------
> +# Compile
> +# ----------------------------------------------------------------------------
> +TF_A_MAKE_OPT += PLAT=$(PTXCONF_TF_A_PLATFORM)
> +
> +TF_A_MAKE_OPT += DEBUG=$(call ptx/ifdef,TF_A_DEBUG,1,0)
> +TF_A_BUILD_OUTPUT_DIR := $(call ptx/ifdef,TF_A_DEBUG,debug,release)
> +
> +TF_A_MAKE_OPT += ARCH=$(PTXCONF_TF_A_ARCH_STRING)
> +TF_A_MAKE_OPT += LOAD_IMAGE_V2=1
> +ifdef PTXCONF_TF_A_BL32_TSP
> +TF_A_MAKE_OPT += ARM_TSP_RAM_LOCATION=$(PTXCONF_TF_A_BL32_TSP_RAM_LOCATION_STRING)
> +endif
> +TF_A_MAKE_OPT += ARM_ARCH_MAJOR=$(PTXCONF_TF_A_ARM_ARCH_MAJOR)
> +ifdef PTXCONF_TF_A_ARM_ARCH_MINOR
> +TF_A_MAKE_OPT += ARM_ARCH_MINOR=$(PTXCONF_TF_A_ARM_ARCH_MINOR)
> +endif
> +
> +ifneq ($(PTXCONF_TF_A_DTB),)
> +TF_A_MAKE_OPT += DTB_FILE_NAME=$(PTXCONF_TF_A_DTB)
> +endif
> +
> +ifdef PTXCONF_TF_A_BL32_SP_MIN
> +TF_A_MAKE_OPT += AARCH32_SP=sp_min
> +endif
> +
> +TF_A_MAKE_OPT += all
> +
> +TF_A_ARTIFACTS = $(addprefix $(TF_A_DIR)/build/$(PTXCONF_TF_A_PLATFORM)/$(TF_A_BUILD_OUTPUT_DIR)/, \
> +       $(call remove_quotes,$(PTXCONF_TF_A_ARTIFACTS)))
> +
> +$(STATEDIR)/tf-a.compile:
> +       @$(call targetinfo)
> +       @rm -rf $(TF_A_DIR)/build/
> +       @$(call world/compile, TF_A)
> +       @$(call touch)

Can't you use the default compile rule here?

> +
> +# ----------------------------------------------------------------------------
> +# Install
> +# ----------------------------------------------------------------------------
> +
> +$(STATEDIR)/tf-a.install:
> +       @$(call targetinfo)
> +
> +ifeq ($(TF_A_ARTIFACTS),)
> +       $(warning TF_A_ARTIFACTS is empty. nothing to install.)
> +else
> +       @install -D -m644 $(TF_A_ARTIFACTS) $(IMAGEDIR)
> +endif

Shouldn't this (copying files to IMAGEDIR) happen in targetinstall
rather than install? Again based on what is being done for other
bootloaders.

Thanks,

Guillermo

> +
> +       @$(call touch)
> +
> +# ----------------------------------------------------------------------------
> +# Target-Install
> +# ----------------------------------------------------------------------------
> +
> +$(STATEDIR)/tf-a.targetinstall:
> +       @$(call targetinfo)
> +       @$(call touch)
> +
> +# ----------------------------------------------------------------------------
> +# Clean
> +# ----------------------------------------------------------------------------
> +
> +$(STATEDIR)/tf-a.clean:
> +       @$(call targetinfo)
> +       @$(call clean_pkg, TF_A)
> +       @rm -f $(IMAGEDIR)/bl1.bin $(IMAGEDIR)/fip.bin
> +
> +# vim: syntax=make
> --
> 2.25.0
>
>
> _______________________________________________
> ptxdist mailing list
> ptxdist@pengutronix.de



-- 
Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de