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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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, 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?

> 
>>
>>>> +$(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 |

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de