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

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

Hi Ahmad,

El mar., 11 feb. 2020 a las 17:53, Ahmad Fatoum
(<a.fatoum@pengutronix.de>) escribió:
>
> Hello Guillermo,
>
> On 2/11/20 5:27 PM, Guillermo Rodriguez Garcia wrote:
> > Hi Ahmad,
> >
> > El mar., 11 feb. 2020 a las 16:22, Ahmad Fatoum
> > (<a.fatoum@pengutronix.de>) escribió:
> >>
> >> Hi,
> >>
> >> On 2/11/20 9:37 AM, Guillermo Rodriguez Garcia wrote:
> >>> 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.
> >>
> >> Ah, indeed. Didn't know about cores like the Cortex-A32.
> >> I will make this configurable.
> >>
> >>>> +
> >>>> +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?
> >>
> >> I don't use any others at the moment, so no.
> >> If you want to use e.g. OP-TEE, you can amend the the tf-a.in.
> >> A string prompt won't save you the effort of doing that, because you
> >> would need to specify dependencies anyway.
> >>
> >>>
> >>>> +
> >>>> +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)
> >>
> >> Yes, u-boot's Kbuild is known to ptxdist, so we don't
> >> need to specify it explicitly.
> >
> > Uhm, here I didn't need to specify CROSS_ENV and things seemed to work
> > just fine.
>
> CROSS_ENV sets stuff like CC and LD. I took a look inside the v2.2 TF-A Makefile
> and on line 808 there is a reference to $(CC) before it's defined,

That would be a bug in the Makefile, and if this was the case,
shouldn't we add a
patch to fix the Makefile (and possibly submit it to upstream),
instead of trying to
work around the bug by setting CROSS_ENV ?

Anyway I am not sure this is actually a bug -- I see that CC is
defined in line 160
in v2.2 of the Makefile; that should happen before the usage in line 808.

> so in that case it
> would come from the environment. It's probably unintended, but to account for
> any other scripts that may reference LD, CC and the like without defining them
> first, I guess it's better to have CROSS_ENV in. Do you agree?

I guess keeping it can not do any harm. However it can also hide bugs,
so I am not
sure what is best.

Guillermo

>
> >
> >>
> >>>> +$(STATEDIR)/tf-a.prepare:
> >>>> +       @$(call targetinfo)
> >>>> +       @$(call touch)
> >>>
> >>> I think this does nothing and can be removed.
> >>
> >> Right. See tf-a.compile below
> >>
> >>>
> >>>> +
> >>>> +# ----------------------------------------------------------------------------
> >>>> +# 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
> >
> > I think this should not be hardcoded.
> >
> > (Sorry, didn't spot this one before)
>
> Will drop it.
>
> >
> >>>> +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?
> >>
> >> Yes. I will move the deletion of the build directory to the prepare stage.
> >> Seems like old versions of TF-A had problems when reconfiguring without a clean?
> >
> > Not sure; I have not seen such problems myself.
>
> I've sent a question to the colleague who first added the line.
> Will keep it in for now.
>
> >
> >>
> >>>> +
> >>>> +# ----------------------------------------------------------------------------
> >>>> +# 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.
> >>
> >> On some systems, the BootROM doesn't directly invoke the TF-A, but instead it is
> >> a payload embedded into another bootloader. For this to work, we need to do it
> >> in the install stage, so the bootloader rule can depend on this and have the
> >> artifact available.
> >>
> >> That was the original line of thought, but apparently, how I did it is not
> >> completely sound. I should instead have both an install and targetinstall:
> >>
> >> - install installs into sysroot for use by other non-image rules
> >> - targetinstall installs into IMAGEDIR for use by image rules
> >>
> >> I will incorporate these changes along with your other suggestions in a v2.
> >
> > Thank you,
> >
> > Guillermo
> >
> >>
> >> Thanks!
> >> Ahmad
> >>
> >>>
> >>> 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
> >>>
> >>>
> >>>
> >>
> >> --
> >> 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 |
> >
> >
> >
>
> --
> 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 |



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

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de