Hi Ahmad, El mar., 11 feb. 2020 a las 17:53, Ahmad Fatoum (<a.fat...@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.fat...@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.fat...@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....@gmail.com> > >>>> Signed-off-by: Rouven Czerwinski <rou...@czerwinskis.de> > >>>> Signed-off-by: Ahmad Fatoum <a.fat...@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.czerwin...@pengutronix.de> > >>>> +# 2019 by Ahmad Fatoum <a.fat...@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.rodrig...@gmail.com _______________________________________________ ptxdist mailing list ptxdist@pengutronix.de
