[Buildroot] [PATCH 1/6] boot/optee-os: install trusted shared libraries

[Thomas Petazzoni]

Hello Etienne,

Thanks for your series of patches on OP-TEE. A couple of comments below.

On Mon, 13 May 2019 22:15:57 +0200
Etienne Carriere <etienne.carriere at linaro.org> wrote:

>  config BR2_TARGET_OPTEE_OS_SERVICES
> -	bool "Build service TAs"
> +	bool "Service trusted application and shared libraries"

I am not too happy with the change of wording, because all other
options are worded as "Build XYZ", and your change moves away from this
consistency.

Can we keep:

	bool "Build service TAs and libs"

for example ?

> diff --git a/boot/optee-os/optee-os.mk b/boot/optee-os/optee-os.mk
> index 6da20a9f3e..811bbbc6ea 100644
> --- a/boot/optee-os/optee-os.mk
> +++ b/boot/optee-os/optee-os.mk
> @@ -77,8 +77,12 @@ endif # BR2_TARGET_OPTEE_OS_CORE
>  ifeq ($(BR2_TARGET_OPTEE_OS_SERVICES),y)
>  define OPTEE_OS_INSTALL_IMAGES_SERVICES
>  	mkdir -p $(TARGET_DIR)/lib/optee_armtz
> -	$(INSTALL) -D -m 444 -t $(TARGET_DIR)/lib/optee_armtz \
> -		$(@D)/$(OPTEE_OS_BUILDDIR_OUT)/ta/*/*.ta
> +	[ -z "$(wildcard $(@D)/$(OPTEE_OS_BUILDDIR_OUT)/ta/*/*.ta)" ] || \
> +		$(INSTALL) -D -m 444 -t $(TARGET_DIR)/lib/optee_armtz \
> +			$(@D)/$(OPTEE_OS_BUILDDIR_OUT)/ta/*/*.ta
> +	[ -z "$(wildcard $(OPTEE_OS_SDK)/lib/*.ta)" ] || \
> +		$(INSTALL) -D -m 444 -t $(TARGET_DIR)/lib/optee_armtz \
> +			$(OPTEE_OS_SDK)/lib/*.ta

I am not a big fan of the conditionals here.

The first one should I guess not be needed, because it was already
non-conditional, so I don't see why it should become conditional.

The second one is less clear. In which cases do we have .ta files in
OPTEE_OS_SDK/lib ? I did a build for the Qemu vexpress platform and
didn't get any lib/*.ta file.

If we really need a test, we could do it in make:

	$(if $(wildcard $(OPTEE_OS_SDK)/lib/*.ta),
		...)

Also, and it should be fixed by a separate patch:
OPTEE_OS_INSTALL_IMAGES_SERVICES contrary to what its name says
installs stuff to $(TARGET_DIR), but is called from
OPTEE_OS_INSTALL_IMAGES_CMDS. It should be called from
OPTEE_OS_INSTALL_TARGET_CMDS instead, and probably renamed to
OPTEE_OS_INSTALL_SERVICES or something like that.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com