[5/6] build: add option for armv8 crypto extension

Message ID 20190412232451.30197-6-yskoh@mellanox.com
State New
Delegated to: Thomas Monjalon
Headers show
Series
  • build: fix build for arm64
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Yongseok Koh April 12, 2019, 11:24 p.m.
Per armv8 crypto extension support, make build always enable it by default
as long as compiler supports the feature while meson build only enables it
for 'default' machine of generic armv8 architecture. For example,
specifying '-mcpu=cortex-a72' doesn't enable it but '+crypto' is required
in order to enable the feature.

It is also known that not all the armv8 platforms have the crypto
extension. For example, Mellanox BlueField has a variant which doesn't have
it. If crypto enabled binary runs on such a platform, rte_eal_init() fails.

Therefore, an option to control this feature is necessary. It is still
enabled by default but can be selectively disabled by vendors.

Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
---
 config/arm/meson.build        | 16 +++++++++-------
 config/common_armv8a_linux    |  1 +
 drivers/crypto/armv8/Makefile |  4 ++++
 meson_options.txt             |  2 ++
 mk/machine/armv8a/rte.vars.mk |  4 ++++
 5 files changed, 20 insertions(+), 7 deletions(-)

Comments

Jerin Jacob Kollanukkaran April 13, 2019, 7:22 a.m. | #1
> -----Original Message-----
> From: Yongseok Koh <yskoh@mellanox.com>
> Sent: Saturday, April 13, 2019 4:55 AM
> To: bruce.richardson@intel.com; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>; shahafs@mellanox.com
> Cc: dev@dpdk.org; thomas@monjalon.net; gavin.hu@arm.com;
> Honnappa.Nagarahalli@arm.com
> Subject: [EXT] [PATCH 5/6] build: add option for armv8 crypto extension
> 
>  CONFIG_RTE_MACHINE="armv8a"
> +CONFIG_RTE_ENABLE_ARMV8_CRYPTO=y

This approach is not scalable. Even, it is not good for BlueField as you 
you need to maintain two images.

Unlike other CPU flags, arm64's crypto cpu flag is really _optional_.
Access to crypto instructions is always at under runtime check.
See the following in rte_armv8_pmd.c


	/* Check CPU for support for AES instruction set */
	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_AES)) {
		ARMV8_CRYPTO_LOG_ERR(
			"AES instructions not supported by CPU");
		return -EFAULT;
	}

	/* Check CPU for support for SHA instruction set */
	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SHA1) ||
	    !rte_cpu_get_flag_enabled(RTE_CPUFLAG_SHA2)) {
		ARMV8_CRYPTO_LOG_ERR(
			"SHA1/SHA2 instructions not supported by CPU");
		return -EFAULT;
	}

So In order to avoid one more config flags specific to armv8 in meson and makefile build infra
And avoid the need for 6/6 patch. IMO,
# Introduce optional CPU flag scheme in eal. Treat armv8 crypto as optional flag
# Skip the eal init check for optional flag.

Do you see any issues with that approach?
Honnappa Nagarahalli April 15, 2019, 4:52 a.m. | #2
> >
> >  CONFIG_RTE_MACHINE="armv8a"
> > +CONFIG_RTE_ENABLE_ARMV8_CRYPTO=y
> 
> This approach is not scalable. Even, it is not good for BlueField as you you
> need to maintain two images.
> 
> Unlike other CPU flags, arm64's crypto cpu flag is really _optional_.
> Access to crypto instructions is always at under runtime check.
> See the following in rte_armv8_pmd.c
> 
> 
> 	/* Check CPU for support for AES instruction set */
> 	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_AES)) {
> 		ARMV8_CRYPTO_LOG_ERR(
> 			"AES instructions not supported by CPU");
> 		return -EFAULT;
> 	}
> 
> 	/* Check CPU for support for SHA instruction set */
> 	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SHA1) ||
> 	    !rte_cpu_get_flag_enabled(RTE_CPUFLAG_SHA2)) {
> 		ARMV8_CRYPTO_LOG_ERR(
> 			"SHA1/SHA2 instructions not supported by CPU");
> 		return -EFAULT;
> 	}
> 
> So In order to avoid one more config flags specific to armv8 in meson and
> makefile build infra And avoid the need for 6/6 patch. IMO, # Introduce
> optional CPU flag scheme in eal. Treat armv8 crypto as optional flag # Skip
> the eal init check for optional flag.
> 
> Do you see any issues with that approach?
> 
+1

> 
> 
> 
> 
>
Yongseok Koh April 15, 2019, 6:43 p.m. | #3
> On Apr 13, 2019, at 12:22 AM, Jerin Jacob Kollanukkaran <jerinj@marvell.com> wrote:
> 
>> -----Original Message-----
>> From: Yongseok Koh <yskoh@mellanox.com>
>> Sent: Saturday, April 13, 2019 4:55 AM
>> To: bruce.richardson@intel.com; Jerin Jacob Kollanukkaran
>> <jerinj@marvell.com>; Pavan Nikhilesh Bhagavatula
>> <pbhagavatula@marvell.com>; shahafs@mellanox.com
>> Cc: dev@dpdk.org; thomas@monjalon.net; gavin.hu@arm.com;
>> Honnappa.Nagarahalli@arm.com
>> Subject: [EXT] [PATCH 5/6] build: add option for armv8 crypto extension
>> 
>> CONFIG_RTE_MACHINE="armv8a"
>> +CONFIG_RTE_ENABLE_ARMV8_CRYPTO=y
> 
> This approach is not scalable. Even, it is not good for BlueField as you 
> you need to maintain two images.
> 
> Unlike other CPU flags, arm64's crypto cpu flag is really _optional_.
> Access to crypto instructions is always at under runtime check.
> See the following in rte_armv8_pmd.c
> 
> 
> 	/* Check CPU for support for AES instruction set */
> 	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_AES)) {
> 		ARMV8_CRYPTO_LOG_ERR(
> 			"AES instructions not supported by CPU");
> 		return -EFAULT;
> 	}
> 
> 	/* Check CPU for support for SHA instruction set */
> 	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SHA1) ||
> 	    !rte_cpu_get_flag_enabled(RTE_CPUFLAG_SHA2)) {
> 		ARMV8_CRYPTO_LOG_ERR(
> 			"SHA1/SHA2 instructions not supported by CPU");
> 		return -EFAULT;
> 	}
> 
> So In order to avoid one more config flags specific to armv8 in meson and makefile build infra
> And avoid the need for 6/6 patch. IMO,
> # Introduce optional CPU flag scheme in eal. Treat armv8 crypto as optional flag
> # Skip the eal init check for optional flag.
> 
> Do you see any issues with that approach?

I also thought about that approach and that was my number 1 priority. But, I had
one question came to my mind. Maybe, arm people can confirm it. Is it 100%
guaranteed that compiler never makes use of any of crypto instructions even if
there's no specific asm/intrinsic code?  The crypto extension has aes, pmull,
sha1 and sha2. In case of rte_memcpy() for x86, for example, compiler may
optimize code using avx512f instructions even though it is written specifically
with avx2 intrinsics (__mm256_*) unless avx512f is disabled.

If a complier expert in arm (or anyone else) confirm it is completely
**optional**, then I'd love to take that approach for sure.

Copied dpdk-on-arm ML.


Thanks,
Yongseok
Honnappa Nagarahalli April 15, 2019, 8:13 p.m. | #4
> >> Subject: [EXT] [PATCH 5/6] build: add option for armv8 crypto
> >> extension
> >>
> >> CONFIG_RTE_MACHINE="armv8a"
> >> +CONFIG_RTE_ENABLE_ARMV8_CRYPTO=y
> >
> > This approach is not scalable. Even, it is not good for BlueField as
> > you you need to maintain two images.
> >
> > Unlike other CPU flags, arm64's crypto cpu flag is really _optional_.
> > Access to crypto instructions is always at under runtime check.
> > See the following in rte_armv8_pmd.c
> >
> >
> > 	/* Check CPU for support for AES instruction set */
> > 	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_AES)) {
> > 		ARMV8_CRYPTO_LOG_ERR(
> > 			"AES instructions not supported by CPU");
> > 		return -EFAULT;
> > 	}
> >
> > 	/* Check CPU for support for SHA instruction set */
> > 	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SHA1) ||
> > 	    !rte_cpu_get_flag_enabled(RTE_CPUFLAG_SHA2)) {
> > 		ARMV8_CRYPTO_LOG_ERR(
> > 			"SHA1/SHA2 instructions not supported by CPU");
> > 		return -EFAULT;
> > 	}
> >
> > So In order to avoid one more config flags specific to armv8 in meson
> > and makefile build infra And avoid the need for 6/6 patch. IMO, #
> > Introduce optional CPU flag scheme in eal. Treat armv8 crypto as
> > optional flag # Skip the eal init check for optional flag.
> >
> > Do you see any issues with that approach?
> 
> I also thought about that approach and that was my number 1 priority.
> But, I had one question came to my mind. Maybe, arm people can confirm
> it. Is it 100% guaranteed that compiler never makes use of any of crypto
> instructions even if there's no specific asm/intrinsic code?  The crypto
> extension has aes, pmull,
> sha1 and sha2. In case of rte_memcpy() for x86, for example, compiler may
> optimize code using avx512f instructions even though it is written
> specifically with avx2 intrinsics (__mm256_*) unless avx512f is disabled.
> 
> If a complier expert in arm (or anyone else) confirm it is completely
> **optional**, then I'd love to take that approach for sure.
> 
> Copied dpdk-on-arm ML.
> 
I do not know the answer, will have to check with the compiler team. I will get back on this.

> 
> Thanks,
> Yongseok
>
Yongseok Koh April 17, 2019, 4:28 p.m. | #5
On Apr 15, 2019, at 1:13 PM, Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:

>>>> Subject: [EXT] [PATCH 5/6] build: add option for armv8 crypto
>>>> extension
>>>> 
>>>> CONFIG_RTE_MACHINE="armv8a"
>>>> +CONFIG_RTE_ENABLE_ARMV8_CRYPTO=y
>>> 
>>> This approach is not scalable. Even, it is not good for BlueField as
>>> you you need to maintain two images.
>>> 
>>> Unlike other CPU flags, arm64's crypto cpu flag is really _optional_.
>>> Access to crypto instructions is always at under runtime check.
>>> See the following in rte_armv8_pmd.c
>>> 
>>> 
>>>    /* Check CPU for support for AES instruction set */
>>>    if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_AES)) {
>>>        ARMV8_CRYPTO_LOG_ERR(
>>>            "AES instructions not supported by CPU");
>>>        return -EFAULT;
>>>    }
>>> 
>>>    /* Check CPU for support for SHA instruction set */
>>>    if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SHA1) ||
>>>        !rte_cpu_get_flag_enabled(RTE_CPUFLAG_SHA2)) {
>>>        ARMV8_CRYPTO_LOG_ERR(
>>>            "SHA1/SHA2 instructions not supported by CPU");
>>>        return -EFAULT;
>>>    }
>>> 
>>> So In order to avoid one more config flags specific to armv8 in meson
>>> and makefile build infra And avoid the need for 6/6 patch. IMO, #
>>> Introduce optional CPU flag scheme in eal. Treat armv8 crypto as
>>> optional flag # Skip the eal init check for optional flag.
>>> 
>>> Do you see any issues with that approach?
>> 
>> I also thought about that approach and that was my number 1 priority.
>> But, I had one question came to my mind. Maybe, arm people can confirm
>> it. Is it 100% guaranteed that compiler never makes use of any of crypto
>> instructions even if there's no specific asm/intrinsic code?  The crypto
>> extension has aes, pmull,
>> sha1 and sha2. In case of rte_memcpy() for x86, for example, compiler may
>> optimize code using avx512f instructions even though it is written
>> specifically with avx2 intrinsics (__mm256_*) unless avx512f is disabled.
>> 
>> If a complier expert in arm (or anyone else) confirm it is completely
>> **optional**, then I'd love to take that approach for sure.
>> 
>> Copied dpdk-on-arm ML.
>> 
> I do not know the answer, will have to check with the compiler team. I will get back on this.

Any update yet?

Thanks 
Yongseok

Patch

diff --git a/config/arm/meson.build b/config/arm/meson.build
index 73c581948c..762d222ed5 100644
--- a/config/arm/meson.build
+++ b/config/arm/meson.build
@@ -7,6 +7,8 @@  march_opt = '-march=@0@'.format(machine)
 
 arm_force_native_march = false
 
+crypto_flag = get_option('enable_armv8_crypto') ? '+crypto' : ''
+
 flags_common_default = [
 	# Accelarate rte_memcpy. Be sure to run unit test (memcpy_perf_autotest)
 	# to determine the best threshold in code. Refer to notes in source file
@@ -70,14 +72,14 @@  flags_octeontx2_extra = [
 	['RTE_USE_C11_MEM_MODEL', true]]
 
 machine_args_generic = [
-	['default', ['-march=armv8-a+crc+crypto']],
+	['default', ['-march=armv8-a+crc' + crypto_flag]],
 	['native', ['-march=native']],
-	['0xd03', ['-mcpu=cortex-a53']],
-	['0xd04', ['-mcpu=cortex-a35']],
-	['0xd07', ['-mcpu=cortex-a57']],
-	['0xd08', ['-mcpu=cortex-a72'], flags_cortex_a72_extra],
-	['0xd09', ['-mcpu=cortex-a73']],
-	['0xd0a', ['-mcpu=cortex-a75']]]
+	['0xd03', ['-mcpu=cortex-a53' + crypto_flag]],
+	['0xd04', ['-mcpu=cortex-a35' + crypto_flag]],
+	['0xd07', ['-mcpu=cortex-a57' + crypto_flag]],
+	['0xd08', ['-mcpu=cortex-a72' + crypto_flag], flags_cortex_a72_extra],
+	['0xd09', ['-mcpu=cortex-a73' + crypto_flag]],
+	['0xd0a', ['-mcpu=cortex-a75' + crypto_flag]]]
 
 machine_args_cavium = [
 	['default', ['-march=armv8-a+crc+crypto','-mcpu=thunderx']],
diff --git a/config/common_armv8a_linux b/config/common_armv8a_linux
index 72091de1c7..0efa3e2eb2 100644
--- a/config/common_armv8a_linux
+++ b/config/common_armv8a_linux
@@ -5,6 +5,7 @@ 
 #include "common_linux"
 
 CONFIG_RTE_MACHINE="armv8a"
+CONFIG_RTE_ENABLE_ARMV8_CRYPTO=y
 
 CONFIG_RTE_ARCH="arm64"
 CONFIG_RTE_ARCH_ARM64=y
diff --git a/drivers/crypto/armv8/Makefile b/drivers/crypto/armv8/Makefile
index f71f6b14a4..867a5206cf 100644
--- a/drivers/crypto/armv8/Makefile
+++ b/drivers/crypto/armv8/Makefile
@@ -4,6 +4,10 @@ 
 
 include $(RTE_SDK)/mk/rte.vars.mk
 
+ifneq ($(CONFIG_RTE_ENABLE_ARMV8_CRYPTO),y)
+$(error "Please enable CONFIG_RTE_ENABLE_ARMV8_CRYPTO")
+endif
+
 ifneq ($(MAKECMDGOALS),clean)
 ifneq ($(MAKECMDGOALS),config)
 ifeq ($(ARMV8_CRYPTO_LIB_PATH),)
diff --git a/meson_options.txt b/meson_options.txt
index 16d9f92c65..4ca09771de 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -4,6 +4,8 @@  option('allow_invalid_socket_id', type: 'boolean', value: false,
 	description: 'allow out-of-range NUMA socket id\'s for platforms that don\'t report the value correctly')
 option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VERSION>',
 	description: 'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.')
+option('enable_armv8_crypto', type: 'boolean', value: true,
+	description: 'enable armv8 crypto extension')
 option('enable_docs', type: 'boolean', value: false,
 	description: 'build documentation')
 option('enable_kmods', type: 'boolean', value: true,
diff --git a/mk/machine/armv8a/rte.vars.mk b/mk/machine/armv8a/rte.vars.mk
index 8252efbb7b..4893d01a2d 100644
--- a/mk/machine/armv8a/rte.vars.mk
+++ b/mk/machine/armv8a/rte.vars.mk
@@ -28,4 +28,8 @@ 
 # CPU_LDFLAGS =
 # CPU_ASFLAGS =
 
+ifeq ($(CONFIG_RTE_ENABLE_ARMV8_CRYPTO),y)
 MACHINE_CFLAGS += -march=armv8-a+crc+crypto
+else
+MACHINE_CFLAGS += -march=armv8-a+crc
+endif