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

[Rouven Czerwinski <r.czerwinski@pengutronix.de>]

On Mon, 2020-02-10 at 17:50 +0100, Guillermo Rodriguez Garcia wrote:
> El lun., 10 feb. 2020 a las 17:33, Ahmad Fatoum
> (<a.fatoum@pengutronix.de>) escribió:
> > Hello Guillermo,
> > 
> > On 2/10/20 5:29 PM, Guillermo Rodriguez Garcia wrote:
> > > Hello,
> > > 
> > > 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
> > > > +
> > > > +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
> > > > +
> > > > +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
> > > 
> > > I am missing here a way to pass additional parameters to TF-A (as
> > > in
> > > TFA_ADDITIONAL_PARAMETERS in Alejandro's version) as there are
> > > many
> > > other options (some of which may be vendor specific) that are not
> > > explicitly
> > > defined in the .in file.
> > 
> > I am not too keen on sidestepping the Kconfig by allowing free form
> > command line parameters.. Do we do this for other rules?
> 
> In most cases I agree that there are better ways, but in some cases
> it's
> the best option. See for example the "extra CFLAGS" & friends in the
> toolchain configuration.
> 
> For TF-A you need a way to pass vendor-specific parameters, as these
> may
> be different for each platform.
> 
> The buildroot rule files also added an "additional parameters" thing
> for TF-A.
> See: 
> https://git.buildroot.org/buildroot/tree/boot/arm-trusted-firmware/Config.in?id=9dbc934217e170578d4cbfdf524bc1b3988d0b9e#n103
> 
> Thanks,
> 
> Guillermo

I agree here as well, since TF-A can introduce arbitrary new parameters
which would create a lot of churn to keep the KConfig flags updated.
We also introduced this for OP-TEE, where it works well so far.

Regards,
Rouven

> > I can add it to v2 though.
> > 
> > > > 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)
> > > > +# TF_A_DEBUG=yes
> > > > +
> > > > +
> > > > +$(STATEDIR)/tf-a.prepare:
> > > > +       @$(call targetinfo)
> > > > +       @$(call touch)
> > > > +
> > > > +# ------------------------------------------------------------
> > > > ----------------
> > > > +# 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_STRIN
> > > > G)
> > > > +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)
> > > > +
> > > > +# ------------------------------------------------------------
> > > > ----------------
> > > > +# 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
> > > > +
> > > > +       @$(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
> > > 
> > > This should probably remove whatever was installed in
> > > targetinstall
> > > (TF_A_ARTIFACTS)
> > > instead of "bl1.bin" and "fip.bin".
> > 
> > Thanks. Will be fixed.
> > 
> > > Guillermo
> > > 
> > 
> > --
> > Pengutronix
> > e.K.                           |                             |
> > Steuerwalder Str. 21                       | 
> > https://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