[Buildroot] [PATCH 0/3] Improve libopenssl target arch selection

[Buildroot] [PATCH 0/3] Improve libopenssl target arch selection

[Thomas Petazzoni]

Hello,

Reviewing commit 027c02660b8cd670eff0ba6fdd9e49253968517f
("package/libopenssl: set no-asm with generic architectures") from
Fabrice made me realize that the OpenSSL target architecture selection
logic was not very easy to read, and was missing a number of useful
cases.

So here is a small patch series that moves the target architecture
selection logic to Config.in, and then makes use of linux-generic64
and linux-x86 when appropriate.

Thomas

Thomas Petazzoni (3):
  package/libopenssl: move target arch selection to Config.in
  package/libopenssl: make use of linux-generic64 for 64-bit archs
  package/libopenssl: make use of linux-x86 for i386

 package/libopenssl/Config.in     | 31 ++++++++++++++++++++++++++++
 package/libopenssl/libopenssl.mk | 35 +-------------------------------
 package/openssl/Config.in        |  2 ++
 3 files changed, 34 insertions(+), 34 deletions(-)
 create mode 100644 package/libopenssl/Config.in

-- 
2.21.0

[Buildroot] [PATCH 1/3] package/libopenssl: move target arch selection to Config.in

[Thomas Petazzoni]

The logic to select the proper OpenSSL target arch in libopenssl.mk is
not easy to read, so let's move it to Config.in where we have some
nice constructs for that kind of value selection.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 package/libopenssl/Config.in     | 29 ++++++++++++++++++++++++++
 package/libopenssl/libopenssl.mk | 35 +-------------------------------
 package/openssl/Config.in        |  2 ++
 3 files changed, 32 insertions(+), 34 deletions(-)
 create mode 100644 package/libopenssl/Config.in

diff --git a/package/libopenssl/Config.in b/package/libopenssl/Config.in
new file mode 100644
index 0000000000..732da5b4ef
--- /dev/null
+++ b/package/libopenssl/Config.in
@@ -0,0 +1,29 @@
+# 4xx PowerPC cores seem to have trouble with openssl's ASM
+# optimizations
+config BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH_LINUX_PPC
+	bool
+	default y if BR2_powerpc
+	depends on !BR2_powerpc_401
+	depends on !BR2_powerpc_403
+	depends on !BR2_powerpc_405
+	depends on !BR2_powerpc_405fp
+	depends on !BR2_powerpc_440
+	depends on !BR2_powerpc_440fp
+
+config BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH
+	string
+	# Use "gcc" minimalistic target to disable DSO
+	# no-asm is needed with generic architectures such as gcc, see
+	# https://github.com/openssl/openssl/issues/9839
+	default "gcc no-asm"			if BR2_STATIC_LIBS
+	# Doesn't work for thumb-only (Cortex-M?)
+	default "linux-armv4"			if BR2_ARM_CPU_HAS_ARM
+	default "linux-aarch64"			if BR2_aarch64
+	default "linux-ppc"			if BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH_LINUX_PPC
+	default "linux-ppc64"			if BR2_powerpc64
+	default "linux-ppc64le"			if BR2_powerpc64le
+	default "linux-x86_64"			if BR2_x86_64
+	# no-asm is needed with generic architectures such as
+	# linux-generic32, see
+	# https://github.com/openssl/openssl/issues/9839
+	default "linux-generic32 no-asm"
diff --git a/package/libopenssl/libopenssl.mk b/package/libopenssl/libopenssl.mk
index a1bbf9a900..da4ae291c0 100644
--- a/package/libopenssl/libopenssl.mk
+++ b/package/libopenssl/libopenssl.mk
@@ -12,9 +12,7 @@ LIBOPENSSL_LICENSE_FILES = LICENSE
 LIBOPENSSL_INSTALL_STAGING = YES
 LIBOPENSSL_DEPENDENCIES = zlib
 HOST_LIBOPENSSL_DEPENDENCIES = host-zlib
-# no-asm is needed with generic architectures such as linux-generic32, see
-# https://github.com/openssl/openssl/issues/9839
-LIBOPENSSL_TARGET_ARCH = linux-generic32 no-asm
+LIBOPENSSL_TARGET_ARCH = $(call qstrip,$(BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH))
 LIBOPENSSL_CFLAGS = $(TARGET_CFLAGS)
 LIBOPENSSL_PROVIDES = openssl
 
@@ -55,37 +53,6 @@ ifeq ($(BR2_TOOLCHAIN_HAS_UCONTEXT),)
 LIBOPENSSL_CFLAGS += -DOPENSSL_NO_ASYNC
 endif
 
-ifeq ($(BR2_STATIC_LIBS),y)
-# Use "gcc" minimalistic target to disable DSO
-# no-asm is needed with generic architectures such as gcc, see
-# https://github.com/openssl/openssl/issues/9839
-LIBOPENSSL_TARGET_ARCH = gcc no-asm
-else
-# Some architectures are optimized in OpenSSL
-# Doesn't work for thumb-only (Cortex-M?)
-ifeq ($(BR2_ARM_CPU_HAS_ARM),y)
-LIBOPENSSL_TARGET_ARCH = linux-armv4
-endif
-ifeq ($(ARCH),aarch64)
-LIBOPENSSL_TARGET_ARCH = linux-aarch64
-endif
-ifeq ($(ARCH),powerpc)
-# 4xx cores seem to have trouble with openssl's ASM optimizations
-ifeq ($(BR2_powerpc_401)$(BR2_powerpc_403)$(BR2_powerpc_405)$(BR2_powerpc_405fp)$(BR2_powerpc_440)$(BR2_powerpc_440fp),)
-LIBOPENSSL_TARGET_ARCH = linux-ppc
-endif
-endif
-ifeq ($(ARCH),powerpc64)
-LIBOPENSSL_TARGET_ARCH = linux-ppc64
-endif
-ifeq ($(ARCH),powerpc64le)
-LIBOPENSSL_TARGET_ARCH = linux-ppc64le
-endif
-ifeq ($(ARCH),x86_64)
-LIBOPENSSL_TARGET_ARCH = linux-x86_64
-endif
-endif
-
 define HOST_LIBOPENSSL_CONFIGURE_CMDS
 	(cd $(@D); \
 		$(HOST_CONFIGURE_OPTS) \
diff --git a/package/openssl/Config.in b/package/openssl/Config.in
index a64660bea3..4d37a3ecf9 100644
--- a/package/openssl/Config.in
+++ b/package/openssl/Config.in
@@ -43,6 +43,8 @@ config BR2_PACKAGE_LIBOPENSSL_ENGINES
 	help
 	  Install additional encryption engine libraries.
 
+source "package/libopenssl/Config.in"
+
 endif
 
 config BR2_PACKAGE_LIBRESSL
-- 
2.21.0

[Buildroot] [PATCH 1/3] package/libopenssl: move target arch selection to Config.in

[Yann E. MORIN]

Thomas, All,

On 2019-10-27 11:24 +0100, Thomas Petazzoni spake thusly:
> The logic to select the proper OpenSSL target arch in libopenssl.mk is
> not easy to read, so let's move it to Config.in where we have some
> nice constructs for that kind of value selection.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
>  package/libopenssl/Config.in     | 29 ++++++++++++++++++++++++++
>  package/libopenssl/libopenssl.mk | 35 +-------------------------------
>  package/openssl/Config.in        |  2 ++
>  3 files changed, 32 insertions(+), 34 deletions(-)
>  create mode 100644 package/libopenssl/Config.in
> 
> diff --git a/package/libopenssl/Config.in b/package/libopenssl/Config.in
> new file mode 100644
> index 0000000000..732da5b4ef
> --- /dev/null
> +++ b/package/libopenssl/Config.in
> @@ -0,0 +1,29 @@
> +# 4xx PowerPC cores seem to have trouble with openssl's ASM
> +# optimizations
> +config BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH_LINUX_PPC

Why 'LINUX' in the option name?

> +	bool
> +	default y if BR2_powerpc
> +	depends on !BR2_powerpc_401
> +	depends on !BR2_powerpc_403
> +	depends on !BR2_powerpc_405
> +	depends on !BR2_powerpc_405fp
> +	depends on !BR2_powerpc_440
> +	depends on !BR2_powerpc_440fp
> +
> +config BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH
> +	string
> +	# Use "gcc" minimalistic target to disable DSO
> +	# no-asm is needed with generic architectures such as gcc, see
> +	# https://github.com/openssl/openssl/issues/9839
> +	default "gcc no-asm"			if BR2_STATIC_LIBS
> +	# Doesn't work for thumb-only (Cortex-M?)
> +	default "linux-armv4"			if BR2_ARM_CPU_HAS_ARM
> +	default "linux-aarch64"			if BR2_aarch64

So I know you just transposed the existing logic from Makefile to
Kconfig. Yet, I'd like to point at how fragile this ordering is.

The arm vs. aarch64 situation works because BR2_ARM_CPU_HAS_ARM is never
selected by an armv8 CPU when it works in 64bit mode.

I think a more reliable way would have been something like:

    config BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH_ARM
        bool
        default y if BR2_arm || BR2_armeb
        depends on BR2_ARM_CPU_HAS_ARM

and then:

    default "linux-armv4"  if BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH_ARM

But since that patch was just a transposition from Makefiel to Kconfig,
I've left it as-is and applied to master.

> +	default "linux-ppc"			if BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH_LINUX_PPC
> +	default "linux-ppc64"			if BR2_powerpc64
> +	default "linux-ppc64le"			if BR2_powerpc64le
> +	default "linux-x86_64"			if BR2_x86_64
> +	# no-asm is needed with generic architectures such as
> +	# linux-generic32, see
> +	# https://github.com/openssl/openssl/issues/9839
> +	default "linux-generic32 no-asm"
[--SNIP--]
> diff --git a/package/openssl/Config.in b/package/openssl/Config.in
> index a64660bea3..4d37a3ecf9 100644
> --- a/package/openssl/Config.in
> +++ b/package/openssl/Config.in
> @@ -43,6 +43,8 @@ config BR2_PACKAGE_LIBOPENSSL_ENGINES
>  	help
>  	  Install additional encryption engine libraries.
>  
> +source "package/libopenssl/Config.in"

Amybe it makes sense to move BR2_PACKAGE_LIBOPENSSL_BIN there too?

Regards,
Yann E. MORIN.

>  endif
>  
>  config BR2_PACKAGE_LIBRESSL
> -- 
> 2.21.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/3] package/libopenssl: move target arch selection to Config.in

[Thomas Petazzoni]

On Mon, 30 Dec 2019 13:54:58 +0100
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> > diff --git a/package/libopenssl/Config.in b/package/libopenssl/Config.in
> > new file mode 100644
> > index 0000000000..732da5b4ef
> > --- /dev/null
> > +++ b/package/libopenssl/Config.in
> > @@ -0,0 +1,29 @@
> > +# 4xx PowerPC cores seem to have trouble with openssl's ASM
> > +# optimizations
> > +config BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH_LINUX_PPC  
> 
> Why 'LINUX' in the option name?

I wrote the patch a while ago, but I guess my reasoning was that it was
related to the "linux-ppc" OpenSSL architecture name. I.e this option
says which "TARGET_ARCH" in Buildroot speak, are compatible with the
"linux-ppc" OpenSSL architecture support.

But I'm fine with the name being changed, it's really just because I
had to find a name :-)

> So I know you just transposed the existing logic from Makefile to
> Kconfig. Yet, I'd like to point at how fragile this ordering is.
> 
> The arm vs. aarch64 situation works because BR2_ARM_CPU_HAS_ARM is never
> selected by an armv8 CPU when it works in 64bit mode.
> 
> I think a more reliable way would have been something like:
> 
>     config BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH_ARM
>         bool
>         default y if BR2_arm || BR2_armeb
>         depends on BR2_ARM_CPU_HAS_ARM
> 
> and then:
> 
>     default "linux-armv4"  if BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH_ARM

True. Could then also be:

	default "linux-armv4" if (BR2_arm || BR2_armeb) && BR2_ARM_CPU_HAS_ARM

We will need to clarify the semantic of BR2_ARM_CPU_HAS_ARM I believe,
to indicate what it means in the context of 64-bit ARM platforms.

> > diff --git a/package/openssl/Config.in b/package/openssl/Config.in
> > index a64660bea3..4d37a3ecf9 100644
> > --- a/package/openssl/Config.in
> > +++ b/package/openssl/Config.in
> > @@ -43,6 +43,8 @@ config BR2_PACKAGE_LIBOPENSSL_ENGINES
> >  	help
> >  	  Install additional encryption engine libraries.
> >  
> > +source "package/libopenssl/Config.in"  
> 
> Amybe it makes sense to move BR2_PACKAGE_LIBOPENSSL_BIN there too?

Yes, it does.

Thanks!

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

[Buildroot] [PATCH 2/3] package/libopenssl: make use of linux-generic64 for 64-bit archs

[Thomas Petazzoni]

It was tested with:

BR2_mips64el=y
BR2_MIPS_NABI64=y
BR2_TOOLCHAIN_EXTERNAL=y
BR2_TOOLCHAIN_EXTERNAL_DOWNLOAD=y
BR2_TOOLCHAIN_EXTERNAL_URL="http://autobuild.buildroot.org/toolchains/tarballs/br-mips64-n64-full-2019.05.1.tar.bz2"
BR2_TOOLCHAIN_EXTERNAL_GCC_5=y
BR2_TOOLCHAIN_EXTERNAL_HEADERS_5_1=y
BR2_TOOLCHAIN_EXTERNAL_LOCALE=y
BR2_TOOLCHAIN_EXTERNAL_CXX=y
BR2_INIT_NONE=y
BR2_SYSTEM_BIN_SH_NONE=y
BR2_PACKAGE_OPENSSL=y

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 package/libopenssl/Config.in | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/package/libopenssl/Config.in b/package/libopenssl/Config.in
index 732da5b4ef..4bc32e06a9 100644
--- a/package/libopenssl/Config.in
+++ b/package/libopenssl/Config.in
@@ -24,6 +24,7 @@ config BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH
 	default "linux-ppc64le"			if BR2_powerpc64le
 	default "linux-x86_64"			if BR2_x86_64
 	# no-asm is needed with generic architectures such as
-	# linux-generic32, see
+	# linux-generic{32,64}, see
 	# https://github.com/openssl/openssl/issues/9839
+	default "linux-generic64 no-asm"	if BR2_ARCH_IS_64
 	default "linux-generic32 no-asm"
-- 
2.21.0

[Buildroot] [PATCH 3/3] package/libopenssl: make use of linux-x86 for i386

[Thomas Petazzoni]

Tested with:

BR2_x86_pentium4=y
BR2_TOOLCHAIN_EXTERNAL=y
BR2_TOOLCHAIN_EXTERNAL_DOWNLOAD=y
BR2_TOOLCHAIN_EXTERNAL_URL="http://autobuild.buildroot.org/toolchains/tarballs/br-i386-pentium4-full-2019.05.1.tar.bz2"
BR2_TOOLCHAIN_EXTERNAL_GCC_7=y
BR2_TOOLCHAIN_EXTERNAL_HEADERS_4_4=y
BR2_TOOLCHAIN_EXTERNAL_LOCALE=y
BR2_TOOLCHAIN_EXTERNAL_CXX=y
BR2_INIT_NONE=y
BR2_SYSTEM_BIN_SH_NONE=y
BR2_PACKAGE_OPENSSL=y

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 package/libopenssl/Config.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/package/libopenssl/Config.in b/package/libopenssl/Config.in
index 4bc32e06a9..5ca19b38f8 100644
--- a/package/libopenssl/Config.in
+++ b/package/libopenssl/Config.in
@@ -22,6 +22,7 @@ config BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH
 	default "linux-ppc"			if BR2_PACKAGE_LIBOPENSSL_TARGET_ARCH_LINUX_PPC
 	default "linux-ppc64"			if BR2_powerpc64
 	default "linux-ppc64le"			if BR2_powerpc64le
+	default "linux-x86"			if BR2_i386
 	default "linux-x86_64"			if BR2_x86_64
 	# no-asm is needed with generic architectures such as
 	# linux-generic{32,64}, see
-- 
2.21.0

[Buildroot] [PATCH 0/3] Improve libopenssl target arch selection

[Yann E. MORIN]

Thomas, All,

On 2019-10-27 11:24 +0100, Thomas Petazzoni spake thusly:
> Hello,
> 
> Reviewing commit 027c02660b8cd670eff0ba6fdd9e49253968517f
> ("package/libopenssl: set no-asm with generic architectures") from
> Fabrice made me realize that the OpenSSL target architecture selection
> logic was not very easy to read, and was missing a number of useful
> cases.
> 
> So here is a small patch series that moves the target architecture
> selection logic to Config.in, and then makes use of linux-generic64
> and linux-x86 when appropriate.
> 
> Thomas
> 
> Thomas Petazzoni (3):
>   package/libopenssl: move target arch selection to Config.in
>   package/libopenssl: make use of linux-generic64 for 64-bit archs
>   package/libopenssl: make use of linux-x86 for i386

I've applied all athree to master, thanks.

I'll reply further on patch 1, for some specific comments.

Regards,
Yann E. MORIN.

>  package/libopenssl/Config.in     | 31 ++++++++++++++++++++++++++++
>  package/libopenssl/libopenssl.mk | 35 +-------------------------------
>  package/openssl/Config.in        |  2 ++
>  3 files changed, 34 insertions(+), 34 deletions(-)
>  create mode 100644 package/libopenssl/Config.in
> 
> -- 
> 2.21.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'