Re: [isar-cip-core][PATCH 1/7] add recipe for optee qemu arm64

From: Jan Kiszka <jan.kiszka@siemens.com>
To: <sven.schultschik@siemens.com>, <cip-dev@lists.cip-project.org>
Subject: Re: [isar-cip-core][PATCH 1/7] add recipe for optee qemu arm64
Date: Wed, 19 Oct 2022 12:44:16 +0200	[thread overview]
Message-ID: <c020dd31-5cc6-4be8-bee1-f313e816a817@siemens.com> (raw)
In-Reply-To: <20221019092117.5291-1-sven.schultschik@siemens.com>

On 19.10.22 11:21, sven.schultschik@siemens.com wrote:
> From: Sven Schultschik <sven.schultschik@siemens.com>
> 
> The recipe provides the possibility to create optee-os binaries for use inside of an qemu secureboot setup with edk2, rpmb, u-boot and uefi
> 
> Signed-off-by: Sven Schultschik <sven.schultschik@siemens.com>
> ---
>  .../op-tee/optee-os-qemu-arm64_3.17.0.bb      | 57 +++++++++++++++++++
>  recipes-bsp/u-boot/files/secure-boot.cfg.tmpl |  7 +++
>  recipes-bsp/u-boot/u-boot-common.inc          |  6 +-
>  3 files changed, 67 insertions(+), 3 deletions(-)
>  create mode 100644 recipes-bsp/op-tee/optee-os-qemu-arm64_3.17.0.bb
> 
> diff --git a/recipes-bsp/op-tee/optee-os-qemu-arm64_3.17.0.bb b/recipes-bsp/op-tee/optee-os-qemu-arm64_3.17.0.bb
> new file mode 100644
> index 000000000..5e60041af
> --- /dev/null
> +++ b/recipes-bsp/op-tee/optee-os-qemu-arm64_3.17.0.bb
> @@ -0,0 +1,57 @@
> +#
> +# CIP Core, generic profile
> +#
> +# Copyright (c) Siemens AG, 2022
> +#
> +# Authors:
> +#  Sven Schultschik <sven.schultschik@siemens.com>
> +#
> +# SPDX-License-Identifier: MIT
> +#
> +
> +HOMEPAGE = "https://github.com/OP-TEE/optee_os"
> +MAINTAINER = "Sven Schultschik <sven.schultschik@siemens.com>"
> +LICENSE = "BSD-2-Clause"
> +
> +require recipes-bsp/optee-os/optee-os-custom.inc
> +
> +SRC_URI += " \
> +            gitsm://github.com/OP-TEE/optee_os.git;branch=master;protocol=https;destsuffix=git;rev=${PV}"

Do we really need the second-class supported gitsm fetcher here? Also,
destsuffix and rev are both redundant (they are defaults).

Oh, indention by 4 spaces would be sufficient.

> +
> +S = "${WORKDIR}/git"
> +
> +OPTEE_PLATFORM = "vexpress-qemu_armv8a"
> +
> +OPTEE_BINARIES = "tee-header_v2.bin \
> +    tee-pager_v2.bin \
> +    tee-pageable_v2.bin"
> +
> +DEPENDS = "edk2"
> +DEBIAN_BUILD_DEPENDS += " ,\
> +    debhelper(>= 11~), \
> +    build-essential, \

Redundant.

> +    cpio, \
> +    python3-cryptography, \
> +    python3-pycryptodome, \
> +    python3-serial, \
> +    device-tree-compiler, \
> +    edk2, \

Do we need Debian's edk2 here?

> +    gcc-arm-linux-gnueabihf,"
> +
> +OPTEE_EXTRA_BUILDARGS = "CFG_STMM_PATH=/usr/lib/edk2/BL32_AP_MM.fd CFG_RPMB_FS=y \
> +        CFG_RPMB_FS_DEV_ID=0 CFG_CORE_HEAP_SIZE=524288 CFG_RPMB_WRITE_KEY=1 \
> +        CFG_CORE_DYN_SHM=y CFG_RPMB_TESTKEY=y \
> +        CFG_REE_FS=n\
> +        CFG_TEE_CORE_LOG_LEVEL=1 CFG_TEE_TA_LOG_LEVEL=1 CFG_SCTLR_ALIGNMENT_CHECK=n \
> +        CFG_ARM64_core=y CFG_CORE_ARM64_PA_BITS=48"
> +
> +ISAR_CROSS_COMPILE = "0"

This looks wrong, specifically as you are installing a 32-bit
cross-compiler and calling a 64-bit one as well below.

> +
> +dpkg_runbuild_prepend() {
> +    # $(ARCH) is the CPU architecture to be built. 
> +    # Currently, the only supported value is arm for 32-bit or 64-bit Armv7-A or Armv8-A. 
> +    # Please note that contrary to the Linux kernel, $(ARCH) should not be set to arm64 for 64-bit builds. 
> +    export ARCH="arm"
> +    export CROSS_COMPILE32=arm-linux-gnueabihf-
> +    export CROSS_COMPILE64=aarch64-linux-gnu-

That is a deprecated style, and sbuild will complain. Move into the
rules file.

> +}
> \ No newline at end of file

Please have a "newline at the end of file".

> diff --git a/recipes-bsp/u-boot/files/secure-boot.cfg.tmpl b/recipes-bsp/u-boot/files/secure-boot.cfg.tmpl
> index 956dcbfed..8e6428238 100644
> --- a/recipes-bsp/u-boot/files/secure-boot.cfg.tmpl
> +++ b/recipes-bsp/u-boot/files/secure-boot.cfg.tmpl
> @@ -4,3 +4,10 @@ CONFIG_USE_BOOTCOMMAND=y
>  CONFIG_BOOTCOMMAND="setenv scan_dev_for_boot 'if test -e ${devtype} ${devnum}:${distro_bootpart} efi/boot/boot${EFI_ARCH}.efi; then load ${devtype} ${devnum}:${distro_bootpart} ${kernel_addr_r} efi/boot/boot${EFI_ARCH}.efi; bootefi ${kernel_addr_r} ${fdtcontroladdr}; fi'; run distro_bootcmd; echo 'EFI Boot failed!'; sleep 1000; reset"
>  CONFIG_EFI_VARIABLES_PRESEED=y
>  CONFIG_EFI_SECURE_BOOT=y
> +### OPTEE config
> +CONFIG_CMD_OPTEE_RPMB=y
> +CONFIG_MMC=y
> +CONFIG_SUPPORT_EMMC_RPMB=y
> +CONFIG_TEE=y
> +CONFIG_OPTEE=y
> +CONFIG_EFI_MM_COMM_TEE=y
> diff --git a/recipes-bsp/u-boot/u-boot-common.inc b/recipes-bsp/u-boot/u-boot-common.inc
> index 60f0da361..7fe4d3fad 100644
> --- a/recipes-bsp/u-boot/u-boot-common.inc
> +++ b/recipes-bsp/u-boot/u-boot-common.inc
> @@ -25,12 +25,12 @@ DEBIAN_BUILD_DEPENDS += ", libssl-dev:native, libssl-dev:${DISTRO_ARCH}"
>  
>  DEBIAN_BUILD_DEPENDS_append_secureboot = ", \
>      openssl, pesign, secure-boot-secrets, python3-openssl:native"
> -DEPENDS_append_secureboot = " secure-boot-secrets"
> +DEPENDS_append_secureboot = " secure-boot-secrets optee-os-${MACHINE}"

Nope, this must go into qemu-specific U-Boot bits. We don't have
optee-os for the rest yet.

>  
>  TEMPLATE_FILES_append_secureboot = " secure-boot.cfg.tmpl"
>  TEMPLATE_VARS_append_secureboot = " EFI_ARCH"
>  
>  do_prepare_build_append_secureboot() {
>      sed -ni '/### Secure boot config/q;p' ${S}/configs/${U_BOOT_CONFIG}
> -    cat ${WORKDIR}/secure-boot.cfg >> ${S}/configs/${U_BOOT_CONFIG}
> -}
> +    cat ${WORKDIR}/secure-boot.cfg >> ${S}/configs/${U_BOOT_CONFIG}    
> +}
> \ No newline at end of file

Spurious change. Please cross-read your diffs for such things.

When you resend, I would suggest to split up the optee recipe
introduction and the U-Boot changes.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux