diff mbox series

[1/6] boot/optee-os: install trusted sharedlibraries

Message ID 20190513201602.2549-1-etienne.carriere@linaro.org
State Superseded
Headers show
Series [1/6] boot/optee-os: install trusted sharedlibraries | expand

Commit Message

Etienne Carriere May 13, 2019, 8:15 p.m. UTC 
Install generated trusted shared libraries in the target file
system next to the trusted applications.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 boot/optee-os/Config.in   | 14 +++++++-------
 boot/optee-os/optee-os.mk |  8 ++++++--
 2 files changed, 13 insertions(+), 9 deletions(-)

Comments

Thomas Petazzoni May 13, 2019, 8:59 p.m. UTC | #1 
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@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
Etienne Carriere May 14, 2019, 6:14 a.m. UTC | #2 
Hello Thomas,

On Mon, 13 May 2019 at 23:00, Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> 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@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 ?

Ok, i'll fix.

>
> > 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.

OP-TEE OS content may change and possibly no TA being built.
For example, maybe one day the package will not build TAs unless some
per in-tree TA configuration.
This change aims at making this build script to be more flexible.

>
> 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.

.ta file in lib/ build directory are generated when configuration
enabled trusted shared libraries,
which a platform may enable or not. The package default config current
does not enable shared trusted libs.

>
> If we really need a test, we could do it in make:
>
>         $(if $(wildcard $(OPTEE_OS_SDK)/lib/*.ta),
>                 ...)

ok.

>
> 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.

Fine. Thanks for the help.

Best regards,
etienne

>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Thomas Petazzoni May 14, 2019, 7:42 a.m. UTC | #3 
Hello Etienne,

On Tue, 14 May 2019 08:14:41 +0200
Etienne Carriere <etienne.carriere@linaro.org> wrote:


> > 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.  
> 
> OP-TEE OS content may change and possibly no TA being built.
> For example, maybe one day the package will not build TAs unless some
> per in-tree TA configuration.
> This change aims at making this build script to be more flexible.

Hm, OK. I generally don't like those conditionals, because then you
don't get any error if those TAs should have been built, but have not.
But if it's indeed dependent on the platform configuration, we don't
really have much choice. So let's keep those conditionals, but please
use make conditions with $(if ...) as I suggested in my previous e-mail.

Thanks!

Thomas
diff mbox series

Patch

diff --git a/boot/optee-os/Config.in b/boot/optee-os/Config.in
index a1f1b910ac..2f129bcc59 100644
--- a/boot/optee-os/Config.in
+++ b/boot/optee-os/Config.in
@@ -70,16 +70,16 @@  config BR2_TARGET_OPTEE_OS_SDK
 	  installed in the staging directory /lib/optee.
 
 config BR2_TARGET_OPTEE_OS_SERVICES
-	bool "Build service TAs"
+	bool "Service trusted application and shared libraries"
 	default y
 	select BR2_TARGET_OPTEE_OS_CORE
 	help
-	  This option installs the service trusted applications built
-	  from OP-TEE OS source tree. These are installed in the target
-	  /lib/optee_armtz directory as other trusted applications.
-	  At runtime OP-TEE OS can load trusted applications from this
-	  non-secure filesystem/directory into the secure world for
-	  execution.
+	  This option installs the service trusted applications and
+	  trusted shared libraries built from OP-TEE OS source tree.
+	  These are installed in target /lib/optee_armtz directory
+	  as other trusted applications. At runtime OP-TEE OS can
+	  load these from this non-secure filesystem/directory into
+	  the secure world for execution.
 
 config BR2_TARGET_OPTEE_OS_PLATFORM
 	string "Target platform (mandatory)"
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
 endef
 endif # BR2_TARGET_OPTEE_OS_SERVICES