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

  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(-)
  

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

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

> 
> 
> 
> 
>
  

> 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
  

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

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
  

> 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?
Currently, enabling 'crypto' flag will generate the crypto instructions only when crypto intrinsics are used. However, when 'sha3' (part of 8.2 crypto) flag is enabled, compiler can generate 3-way exclusive OR instructions beyond the intrinsics. Compiler team cannot provide a guarantee that other crypto instructions will not be used beyond the intrinsics.

The current suggestion is to use GNU indirect function [1] or similar. I am not sure on GNU indirect function portability.

[1] https://willnewton.name/2013/07/02/using-gnu-indirect-functions/

> 
> Thanks
> Yongseok
  

> On Apr 29, 2019, at 8:33 PM, Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:
> 
>> 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?
> Currently, enabling 'crypto' flag will generate the crypto instructions only when crypto intrinsics are used. However, when 'sha3' (part of 8.2 crypto) flag is enabled, compiler can generate 3-way exclusive OR instructions beyond the intrinsics. Compiler team cannot provide a guarantee that other crypto instructions will not be used beyond the intrinsics.
> 
> The current suggestion is to use GNU indirect function [1] or similar. I am not sure on GNU indirect function portability.
> 
> [1] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwillnewton.name%2F2013%2F07%2F02%2Fusing-gnu-indirect-functions%2F&amp;data=02%7C01%7Cyskoh%40mellanox.com%7Ce8738c4f725a4ca608ea08d6cd1cac03%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636921920373635167&amp;sdata=kuq6dbpTBfRgokrv2L%2FV4BIM0q1k%2FiL1JaMqCHUc2c0%3D&amp;reserved=0

Thanks for the update,

Then, I think the original patch to have build config is currently okay.
Will submit it again.

thanks
Yongseok
  

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Tuesday, April 30, 2019 9:04 AM
> To: yskoh@mellanox.com
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> bruce.richardson@intel.com; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>; Shahaf Shuler <shahafs@mellanox.com>;
> dev@dpdk.org; thomas@monjalon.net; Gavin Hu (Arm Technology China)
> <Gavin.Hu@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>
> Subject: RE: [EXT] [PATCH 5/6] build: add option for armv8 crypto extension
> 
> > 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?
> Currently, enabling 'crypto' flag will generate the crypto instructions only when
> crypto intrinsics are used. However, when 'sha3' (part of 8.2 crypto) flag is

The default image is 8.1 spec and except octeontx2 every other SoC is 8.1 and
For octeotx2 crypto is supported. If so, Should we worry this case?

> enabled, compiler can generate 3-way exclusive OR instructions beyond the
> intrinsics.

The very same problem will be applicable for Linux kernel too for distribution binary case.
If the above statement is true about 8.2 crypto and crypto generation without
Intrinsics then we need to see how linux kernel handling that and align our solution
based on that.

> Compiler team cannot provide a guarantee that other crypto
> instructions will not be used beyond the intrinsics.
> 
> The current suggestion is to use GNU indirect function [1] or similar. I am not

Not sure how it helps? If we know the compiler is generating a specific function
With crypto instruction then we can generate _alternative_ function for the same
With hwcap?.How do we know which function compiler using compiler instructions?


> sure on GNU indirect function portability.

We are using HWCAP scheme, So we may not need the very exact GNU indirect
scheme to fix the issue.

> 
> [1] https://willnewton.name/2013/07/02/using-gnu-indirect-functions/
> 
> >
> > Thanks
> > Yongseok
  

> On May 2, 2019, at 3:13 AM, Jerin Jacob Kollanukkaran <jerinj@marvell.com> wrote:
> 
>> -----Original Message-----
>> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
>> Sent: Tuesday, April 30, 2019 9:04 AM
>> To: yskoh@mellanox.com
>> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
>> bruce.richardson@intel.com; Pavan Nikhilesh Bhagavatula
>> <pbhagavatula@marvell.com>; Shahaf Shuler <shahafs@mellanox.com>;
>> dev@dpdk.org; thomas@monjalon.net; Gavin Hu (Arm Technology China)
>> <Gavin.Hu@arm.com>; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>
>> Subject: RE: [EXT] [PATCH 5/6] build: add option for armv8 crypto extension
>> 
>>> 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?
>> Currently, enabling 'crypto' flag will generate the crypto instructions only when
>> crypto intrinsics are used. However, when 'sha3' (part of 8.2 crypto) flag is
> 
> The default image is 8.1 spec and except octeontx2 every other SoC is 8.1 and
> For octeotx2 crypto is supported. If so, Should we worry this case?

Right, it sounds to me that we can disable the option without having the new
config flag until such instructions get needed. According to gcc-8 release note
[1], currently '+crypto' implies '+aes' and '+sha2' while '+sha3' and '+sm4' are
newly introduced. Given that armv8 crypto PMD uses external binary of Marvell. I
don't see any reason to enable '+crypto'. How about simply disable it from armv8
build configs?

diff --git a/config/arm/meson.build b/config/arm/meson.build
index 7fa6ed3105..abc8cf346c 100644
--- a/config/arm/meson.build
+++ b/config/arm/meson.build
@@ -74,7 +74,7 @@ flags_octeontx2_extra = [
        ['RTE_USE_C11_MEM_MODEL', true]]

 machine_args_generic = [
-       ['default', ['-march=armv8-a+crc+crypto']],
+       ['default', ['-march=armv8-a+crc']],
        ['native', ['-march=native']],
        ['0xd03', ['-mcpu=cortex-a53']],
        ['0xd04', ['-mcpu=cortex-a35']],
diff --git a/mk/machine/armv8a/rte.vars.mk b/mk/machine/armv8a/rte.vars.mk
index 8252efbb7b..5e3ffc3adf 100644
--- a/mk/machine/armv8a/rte.vars.mk
+++ b/mk/machine/armv8a/rte.vars.mk
@@ -28,4 +28,4 @@
 # CPU_LDFLAGS =
 # CPU_ASFLAGS =

-MACHINE_CFLAGS += -march=armv8-a+crc+crypto
+MACHINE_CFLAGS += -march=armv8-a+crc


[1] https://gcc.gnu.org/gcc-8/changes.html

Thanks,
Yongseok

>> enabled, compiler can generate 3-way exclusive OR instructions beyond the
>> intrinsics.
> 
> The very same problem will be applicable for Linux kernel too for distribution binary case.
> If the above statement is true about 8.2 crypto and crypto generation without
> Intrinsics then we need to see how linux kernel handling that and align our solution
> based on that.
> 
>> Compiler team cannot provide a guarantee that other crypto
>> instructions will not be used beyond the intrinsics.
>> 
>> The current suggestion is to use GNU indirect function [1] or similar. I am not
> 
> Not sure how it helps? If we know the compiler is generating a specific function
> With crypto instruction then we can generate _alternative_ function for the same
> With hwcap?.How do we know which function compiler using compiler instructions?
> 
> 
>> sure on GNU indirect function portability.
> 
> We are using HWCAP scheme, So we may not need the very exact GNU indirect
> scheme to fix the issue.
> 
>> 
>> [1] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwillnewton.name%2F2013%2F07%2F02%2Fusing-gnu-indirect-functions%2F&amp;data=02%7C01%7Cyskoh%40mellanox.com%7Cda8fb7ed03e7406ded8908d6cee6d759%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636923888189316743&amp;sdata=x5XNd5WZ3EtiprPMiFzaskvigX8K0AoXA2w%2BKiN156c%3D&amp;reserved=0
>> 
>>> 
>>> Thanks
>>> Yongseok
  

> On May 2, 2019, at 4:08 PM, Yongseok Koh <yskoh@mellanox.com> wrote:
> 
>> 
>> On May 2, 2019, at 3:13 AM, Jerin Jacob Kollanukkaran <jerinj@marvell.com> wrote:
>> 
>>> -----Original Message-----
>>> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
>>> Sent: Tuesday, April 30, 2019 9:04 AM
>>> To: yskoh@mellanox.com
>>> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
>>> bruce.richardson@intel.com; Pavan Nikhilesh Bhagavatula
>>> <pbhagavatula@marvell.com>; Shahaf Shuler <shahafs@mellanox.com>;
>>> dev@dpdk.org; thomas@monjalon.net; Gavin Hu (Arm Technology China)
>>> <Gavin.Hu@arm.com>; Honnappa Nagarahalli
>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>
>>> Subject: RE: [EXT] [PATCH 5/6] build: add option for armv8 crypto extension
>>> 
>>>> 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?
>>> Currently, enabling 'crypto' flag will generate the crypto instructions only when
>>> crypto intrinsics are used. However, when 'sha3' (part of 8.2 crypto) flag is
>> 
>> The default image is 8.1 spec and except octeontx2 every other SoC is 8.1 and
>> For octeotx2 crypto is supported. If so, Should we worry this case?
> 
> Right, it sounds to me that we can disable the option without having the new
> config flag until such instructions get needed. According to gcc-8 release note
> [1], currently '+crypto' implies '+aes' and '+sha2' while '+sha3' and '+sm4' are
> newly introduced. Given that armv8 crypto PMD uses external binary of Marvell. I
> don't see any reason to enable '+crypto'. How about simply disable it from armv8
> build configs?
> 
> diff --git a/config/arm/meson.build b/config/arm/meson.build
> index 7fa6ed3105..abc8cf346c 100644
> --- a/config/arm/meson.build
> +++ b/config/arm/meson.build
> @@ -74,7 +74,7 @@ flags_octeontx2_extra = [
>        ['RTE_USE_C11_MEM_MODEL', true]]
> 
> machine_args_generic = [
> -       ['default', ['-march=armv8-a+crc+crypto']],
> +       ['default', ['-march=armv8-a+crc']],
>        ['native', ['-march=native']],
>        ['0xd03', ['-mcpu=cortex-a53']],
>        ['0xd04', ['-mcpu=cortex-a35']],
> diff --git a/mk/machine/armv8a/rte.vars.mk b/mk/machine/armv8a/rte.vars.mk
> index 8252efbb7b..5e3ffc3adf 100644
> --- a/mk/machine/armv8a/rte.vars.mk
> +++ b/mk/machine/armv8a/rte.vars.mk
> @@ -28,4 +28,4 @@
> # CPU_LDFLAGS =
> # CPU_ASFLAGS =
> 
> -MACHINE_CFLAGS += -march=armv8-a+crc+crypto
> +MACHINE_CFLAGS += -march=armv8-a+crc
> 
> 
> [1] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fgcc-8%2Fchanges.html&amp;data=02%7C01%7Cyskoh%40mellanox.com%7C8a0d60c82a11498bf65608d6cf5327c3%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636924353391308162&amp;sdata=cuueiNi%2FdBfEJDKa8IFstwctBIrOkfZn0J7xojxgfvI%3D&amp;reserved=0

Just to make sure, I've run examples/ipsec-secgw on BlueField and it ran well as expected.

>>> enabled, compiler can generate 3-way exclusive OR instructions beyond the
>>> intrinsics.
>> 
>> The very same problem will be applicable for Linux kernel too for distribution binary case.
>> If the above statement is true about 8.2 crypto and crypto generation without
>> Intrinsics then we need to see how linux kernel handling that and align our solution
>> based on that.
>> 
>>> Compiler team cannot provide a guarantee that other crypto
>>> instructions will not be used beyond the intrinsics.
>>> 
>>> The current suggestion is to use GNU indirect function [1] or similar. I am not
>> 
>> Not sure how it helps? If we know the compiler is generating a specific function
>> With crypto instruction then we can generate _alternative_ function for the same
>> With hwcap?.How do we know which function compiler using compiler instructions?
>> 
>> 
>>> sure on GNU indirect function portability.
>> 
>> We are using HWCAP scheme, So we may not need the very exact GNU indirect
>> scheme to fix the issue.
>> 
>>> 
>>> [1] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwillnewton.name%2F2013%2F07%2F02%2Fusing-gnu-indirect-functions%2F&amp;data=02%7C01%7Cyskoh%40mellanox.com%7C8a0d60c82a11498bf65608d6cf5327c3%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636924353391308162&amp;sdata=WcRHom7k1MFmHzK1LYJEaI5ruMzCvvMxlFo7Ivl%2BOh4%3D&amp;reserved=0
>>> 
>>>> 
>>>> Thanks
>>>> Yongseok
  

> >>> 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?
> >> Currently, enabling 'crypto' flag will generate the crypto
> >> instructions only when crypto intrinsics are used. However, when
> >> 'sha3' (part of 8.2 crypto) flag is
> >
> > The default image is 8.1 spec and except octeontx2 every other SoC is
I am not following this. I think the default image is 8.0.

> > 8.1 and For octeotx2 crypto is supported. If so, Should we worry this case?
I assume we all are talking about the distro/binary portable build. IMO, we should not just look at the existing SoCs.
The CPU specific builds have the freedom to compile as per their corresponding support.

> 
> Right, it sounds to me that we can disable the option without having the new
> config flag until such instructions get needed. According to gcc-8 release note
> [1], currently '+crypto' implies '+aes' and '+sha2' while '+sha3' and '+sm4' are
> newly introduced. Given that armv8 crypto PMD uses external binary of
> Marvell. I don't see any reason to enable '+crypto'. How about simply disable
> it from armv8 build configs?
I think it should be fine. But, this alone is not enough. The run time detection of the crypto feature and hooking up the correct pointers needs to be added.

> 
> diff --git a/config/arm/meson.build b/config/arm/meson.build index
> 7fa6ed3105..abc8cf346c 100644
> --- a/config/arm/meson.build
> +++ b/config/arm/meson.build
> @@ -74,7 +74,7 @@ flags_octeontx2_extra = [
>         ['RTE_USE_C11_MEM_MODEL', true]]
> 
>  machine_args_generic = [
> -       ['default', ['-march=armv8-a+crc+crypto']],
> +       ['default', ['-march=armv8-a+crc']],
>         ['native', ['-march=native']],
>         ['0xd03', ['-mcpu=cortex-a53']],
>         ['0xd04', ['-mcpu=cortex-a35']], diff --git
> a/mk/machine/armv8a/rte.vars.mk b/mk/machine/armv8a/rte.vars.mk index
> 8252efbb7b..5e3ffc3adf 100644
> --- a/mk/machine/armv8a/rte.vars.mk
> +++ b/mk/machine/armv8a/rte.vars.mk
> @@ -28,4 +28,4 @@
>  # CPU_LDFLAGS =
>  # CPU_ASFLAGS =
> 
> -MACHINE_CFLAGS += -march=armv8-a+crc+crypto
> +MACHINE_CFLAGS += -march=armv8-a+crc
> 
> 
> [1] https://gcc.gnu.org/gcc-8/changes.html
> 
> Thanks,
> Yongseok
> 
> >> enabled, compiler can generate 3-way exclusive OR instructions beyond
> >> the intrinsics.
> >
> > The very same problem will be applicable for Linux kernel too for
> distribution binary case.
> > If the above statement is true about 8.2 crypto and crypto generation
> > without Intrinsics then we need to see how linux kernel handling that
> > and align our solution based on that.
Yes, the compiler team cited Linux kernel example, I have not verified it myself.

> >
> >> Compiler team cannot provide a guarantee that other crypto
> >> instructions will not be used beyond the intrinsics.
> >>
> >> The current suggestion is to use GNU indirect function [1] or
> >> similar. I am not
> >
> > Not sure how it helps? If we know the compiler is generating a
> > specific function With crypto instruction then we can generate
> > _alternative_ function for the same With hwcap?.How do we know which
> function compiler using compiler instructions?
This feature is similar to using function pointers and choosing which function pointer to use at run time. If this feature is used, the function pointer to use is decided during dynamic linking stage.
Either ways, we need to have 2 sets of crypto PMD drivers. One that implements the actual functionality using crypto intrinsics/assembly. Only, this code needs to be compiled with '+crypto'. Second driver that implements just stubs and returns error. This code will be compiled without '+crypto'. At run time, depending on the HWCAP, the correct driver/function pointers need to be hooked up.

> >
> >
> >> sure on GNU indirect function portability.
> >
> > We are using HWCAP scheme, So we may not need the very exact GNU
> > indirect scheme to fix the issue.
Agree, using indirect functions is not a must.

> >
> >>
> >> [1]
> >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwil
> >> lnewton.name%2F2013%2F07%2F02%2Fusing-gnu-indirect-
> functions%2F&amp;d
> >>
> ata=02%7C01%7Cyskoh%40mellanox.com%7Cda8fb7ed03e7406ded8908d6c
> ee6d759
> >> %7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C63692388818
> 9316743&amp;
> >>
> sdata=x5XNd5WZ3EtiprPMiFzaskvigX8K0AoXA2w%2BKiN156c%3D&amp;res
> erved=0
> >>
> >>>
> >>> Thanks
> >>> Yongseok
  

On Fri, May 03, 2019 at 03:54:09AM +0000, Honnappa Nagarahalli wrote:
> > >>> 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?
> > >> Currently, enabling 'crypto' flag will generate the crypto
> > >> instructions only when crypto intrinsics are used. However, when
> > >> 'sha3' (part of 8.2 crypto) flag is
> > >
> > > The default image is 8.1 spec and except octeontx2 every other SoC is
> I am not following this. I think the default image is 8.0.
> 
> > > 8.1 and For octeotx2 crypto is supported. If so, Should we worry this case?
> I assume we all are talking about the distro/binary portable build. IMO, we should not just look at the existing SoCs.
> The CPU specific builds have the freedom to compile as per their corresponding support.
> 
> > 
> > Right, it sounds to me that we can disable the option without having the new
> > config flag until such instructions get needed. According to gcc-8 release note
> > [1], currently '+crypto' implies '+aes' and '+sha2' while '+sha3' and '+sm4' are
> > newly introduced. Given that armv8 crypto PMD uses external binary of
> > Marvell. I don't see any reason to enable '+crypto'. How about simply disable
> > it from armv8 build configs?
> I think it should be fine. But, this alone is not enough. The run time
> detection of the crypto feature and hooking up the correct pointers needs to
> be added.

Like Jerin pointed out above, armv8 cryptodev already has runtime check of
cpuflags. If there's no support, it returns error. Unless we need a fallback
function with non-crypto instructions instead of returning error, I don't think
such hookup of func pointers are needed.

> > diff --git a/config/arm/meson.build b/config/arm/meson.build index
> > 7fa6ed3105..abc8cf346c 100644
> > --- a/config/arm/meson.build
> > +++ b/config/arm/meson.build
> > @@ -74,7 +74,7 @@ flags_octeontx2_extra = [
> >         ['RTE_USE_C11_MEM_MODEL', true]]
> > 
> >  machine_args_generic = [
> > -       ['default', ['-march=armv8-a+crc+crypto']],
> > +       ['default', ['-march=armv8-a+crc']],
> >         ['native', ['-march=native']],
> >         ['0xd03', ['-mcpu=cortex-a53']],
> >         ['0xd04', ['-mcpu=cortex-a35']], diff --git
> > a/mk/machine/armv8a/rte.vars.mk b/mk/machine/armv8a/rte.vars.mk index
> > 8252efbb7b..5e3ffc3adf 100644
> > --- a/mk/machine/armv8a/rte.vars.mk
> > +++ b/mk/machine/armv8a/rte.vars.mk
> > @@ -28,4 +28,4 @@
> >  # CPU_LDFLAGS =
> >  # CPU_ASFLAGS =
> > 
> > -MACHINE_CFLAGS += -march=armv8-a+crc+crypto
> > +MACHINE_CFLAGS += -march=armv8-a+crc
> > 
> > 
> > [1] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fgcc-8%2Fchanges.html&amp;data=02%7C01%7Cyskoh%40mellanox.com%7C5cd398e4cf1e45c1755a08d6cf7b0091%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636924524543262594&amp;sdata=4m4S2VQUVBMLYqpxmeLoAPqAcKGm9u1Wo5R7oE2CK94%3D&amp;reserved=0
> > 
> > Thanks,
> > Yongseok
> > 
> > >> enabled, compiler can generate 3-way exclusive OR instructions beyond
> > >> the intrinsics.
> > >
> > > The very same problem will be applicable for Linux kernel too for
> > distribution binary case.
> > > If the above statement is true about 8.2 crypto and crypto generation
> > > without Intrinsics then we need to see how linux kernel handling that
> > > and align our solution based on that.
> Yes, the compiler team cited Linux kernel example, I have not verified it myself.
> 
> > >
> > >> Compiler team cannot provide a guarantee that other crypto
> > >> instructions will not be used beyond the intrinsics.
> > >>
> > >> The current suggestion is to use GNU indirect function [1] or
> > >> similar. I am not
> > >
> > > Not sure how it helps? If we know the compiler is generating a
> > > specific function With crypto instruction then we can generate
> > > _alternative_ function for the same With hwcap?.How do we know which
> > > function compiler using compiler instructions?
> This feature is similar to using function pointers and choosing which function
> pointer to use at run time. If this feature is used, the function pointer to
> use is decided during dynamic linking stage.

I think what Jerin meant was about the case where compiler can generate crypto
instructions beyond intrinsics/asm like sha3 for 3-way exclusive OR
instructions. In this case, such function pointer can't help as we can't know
how compiler generates such instructions.

> Either ways, we need to have 2 sets of crypto PMD drivers. One that implements
> the actual functionality using crypto intrinsics/assembly. Only, this code
> needs to be compiled with '+crypto'. Second driver that implements just stubs
> and returns error. This code will be compiled without '+crypto'. At run time,
> depending on the HWCAP, the correct driver/function pointers need to be hooked
> up.

Like I mentioned above, it may not be necessary. armv8 cryptodev links external
library, which is compiled separately (out of dpdk) with crypto support and we
don't have/need a fallback but returns error if no crypto support in runtime.

> > >> sure on GNU indirect function portability.
> > >
> > > We are using HWCAP scheme, So we may not need the very exact GNU
> > > indirect scheme to fix the issue.
> Agree, using indirect functions is not a must.
> 
> > >
> > >>
> > >> [1]
> > >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwil
> > >> lnewton.name%2F2013%2F07%2F02%2Fusing-gnu-indirect-
> > functions%2F&amp;d
> > >>
> > ata=02%7C01%7Cyskoh%40mellanox.com%7Cda8fb7ed03e7406ded8908d6c
> > ee6d759
> > >> %7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C63692388818
> > 9316743&amp;
> > >>
> > sdata=x5XNd5WZ3EtiprPMiFzaskvigX8K0AoXA2w%2BKiN156c%3D&amp;res
> > erved=0
  

> -----Original Message-----
> From: Yongseok Koh <yskoh@mellanox.com>
> Sent: Friday, May 3, 2019 5:03 AM
> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> Cc: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> bruce.richardson@intel.com; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>; Shahaf Shuler <shahafs@mellanox.com>;
> dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Gavin Hu (Arm
> Technology China) <Gavin.Hu@arm.com>; nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [EXT] [PATCH 5/6] build: add option for armv8 crypto
> extension
> 
> 
> > On May 2, 2019, at 4:08 PM, Yongseok Koh <yskoh@mellanox.com> wrote:
> >
> >>
> >> On May 2, 2019, at 3:13 AM, Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> wrote:
> >>
> >>> -----Original Message-----
> >>> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> >>> Sent: Tuesday, April 30, 2019 9:04 AM
> >>> To: yskoh@mellanox.com
> >>> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> >>> bruce.richardson@intel.com; Pavan Nikhilesh Bhagavatula
> >>> <pbhagavatula@marvell.com>; Shahaf Shuler <shahafs@mellanox.com>;
> >>> dev@dpdk.org; thomas@monjalon.net; Gavin Hu (Arm Technology China)
> >>> <Gavin.Hu@arm.com>; Honnappa Nagarahalli
> >>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>; nd
> <nd@arm.com>
> >>> Subject: RE: [EXT] [PATCH 5/6] build: add option for armv8 crypto
> >>> extension
> >>>
> >>>> 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?
> >>> Currently, enabling 'crypto' flag will generate the crypto
> >>> instructions only when crypto intrinsics are used. However, when
> >>> 'sha3' (part of 8.2 crypto) flag is
> >>
> >> The default image is 8.1 spec and except octeontx2 every other SoC is
> >> 8.1 and For octeotx2 crypto is supported. If so, Should we worry this case?
> >
> > Right, it sounds to me that we can disable the option without having
> > the new config flag until such instructions get needed. According to
> > gcc-8 release note [1], currently '+crypto' implies '+aes' and '+sha2'
> > while '+sha3' and '+sm4' are newly introduced. Given that armv8 crypto
> > PMD uses external binary of Marvell. I don't see any reason to enable
> > '+crypto'. How about simply disable it from armv8 
build configs?
+1
Yes. Simply disable crypto would be enough for DPDK.
  

> On Fri, May 03, 2019 at 03:54:09AM +0000, Honnappa Nagarahalli wrote:
> > > >>> 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?
> > > >> Currently, enabling 'crypto' flag will generate the crypto
> > > >> instructions only when crypto intrinsics are used. However, when
> > > >> 'sha3' (part of 8.2 crypto) flag is
> > > >
> > > > The default image is 8.1 spec and except octeontx2 every other SoC
> > > > is
> > I am not following this. I think the default image is 8.0.
> >
> > > > 8.1 and For octeotx2 crypto is supported. If so, Should we worry this
> case?
> > I assume we all are talking about the distro/binary portable build. IMO, we
> should not just look at the existing SoCs.
> > The CPU specific builds have the freedom to compile as per their
> corresponding support.
> >
> > >
> > > Right, it sounds to me that we can disable the option without having
> > > the new config flag until such instructions get needed. According to
> > > gcc-8 release note [1], currently '+crypto' implies '+aes' and
> > > '+sha2' while '+sha3' and '+sm4' are newly introduced. Given that
> > > armv8 crypto PMD uses external binary of Marvell. I don't see any
> > > reason to enable '+crypto'. How about simply disable it from armv8 build
> configs?
> > I think it should be fine. But, this alone is not enough. The run time
> > detection of the crypto feature and hooking up the correct pointers
> > needs to be added.
> 
> Like Jerin pointed out above, armv8 cryptodev already has runtime check of
> cpuflags. If there's no support, it returns error. Unless we need a fallback
> function with non-crypto instructions instead of returning error, I don't think
> such hookup of func pointers are needed.
> 
> > > diff --git a/config/arm/meson.build b/config/arm/meson.build index
> > > 7fa6ed3105..abc8cf346c 100644
> > > --- a/config/arm/meson.build
> > > +++ b/config/arm/meson.build
> > > @@ -74,7 +74,7 @@ flags_octeontx2_extra = [
> > >         ['RTE_USE_C11_MEM_MODEL', true]]
> > >
> > >  machine_args_generic = [
> > > -       ['default', ['-march=armv8-a+crc+crypto']],
> > > +       ['default', ['-march=armv8-a+crc']],
> > >         ['native', ['-march=native']],
> > >         ['0xd03', ['-mcpu=cortex-a53']],
> > >         ['0xd04', ['-mcpu=cortex-a35']], diff --git
> > > a/mk/machine/armv8a/rte.vars.mk b/mk/machine/armv8a/rte.vars.mk
> > > index 8252efbb7b..5e3ffc3adf 100644
> > > --- a/mk/machine/armv8a/rte.vars.mk
> > > +++ b/mk/machine/armv8a/rte.vars.mk
> > > @@ -28,4 +28,4 @@
> > >  # CPU_LDFLAGS =
> > >  # CPU_ASFLAGS =
> > >
> > > -MACHINE_CFLAGS += -march=armv8-a+crc+crypto
> > > +MACHINE_CFLAGS += -march=armv8-a+crc
> > >
> > >
> > > [1]
> > > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgc
> > > c.gnu.org%2Fgcc-
> 8%2Fchanges.html&amp;data=02%7C01%7Cyskoh%40mellanox
> > > .com%7C5cd398e4cf1e45c1755a08d6cf7b0091%7Ca652971c7d2e4d9ba
> 6a4d14925
> > >
> 6f461b%7C0%7C0%7C636924524543262594&amp;sdata=4m4S2VQUVBML
> YqpxmeLoAP
> > > qAcKGm9u1Wo5R7oE2CK94%3D&amp;reserved=0
> > >
> > > Thanks,
> > > Yongseok
> > >
> > > >> enabled, compiler can generate 3-way exclusive OR instructions
> > > >> beyond the intrinsics.
> > > >
> > > > The very same problem will be applicable for Linux kernel too for
> > > distribution binary case.
> > > > If the above statement is true about 8.2 crypto and crypto
> > > > generation without Intrinsics then we need to see how linux kernel
> > > > handling that and align our solution based on that.
> > Yes, the compiler team cited Linux kernel example, I have not verified it
> myself.
> >
> > > >
> > > >> Compiler team cannot provide a guarantee that other crypto
> > > >> instructions will not be used beyond the intrinsics.
> > > >>
> > > >> The current suggestion is to use GNU indirect function [1] or
> > > >> similar. I am not
> > > >
> > > > Not sure how it helps? If we know the compiler is generating a
> > > > specific function With crypto instruction then we can generate
> > > > _alternative_ function for the same With hwcap?.How do we know
> > > > which function compiler using compiler instructions?
> > This feature is similar to using function pointers and choosing which
> > function pointer to use at run time. If this feature is used, the
> > function pointer to use is decided during dynamic linking stage.
> 
> I think what Jerin meant was about the case where compiler can generate
> crypto instructions beyond intrinsics/asm like sha3 for 3-way exclusive OR
> instructions. In this case, such function pointer can't help as we can't know
> how compiler generates such instructions.
> 
> > Either ways, we need to have 2 sets of crypto PMD drivers. One that
> > implements the actual functionality using crypto intrinsics/assembly.
> > Only, this code needs to be compiled with '+crypto'. Second driver
> > that implements just stubs and returns error. This code will be
> > compiled without '+crypto'. At run time, depending on the HWCAP, the
> > correct driver/function pointers need to be hooked up.
> 
> Like I mentioned above, it may not be necessary. armv8 cryptodev links
> external library, which is compiled separately (out of dpdk) with crypto
> support and we don't have/need a fallback but returns error if no crypto
> support in runtime.
Ok, got it (did not realize crypto library is external to DPDK).

> 
> > > >> sure on GNU indirect function portability.
> > > >
> > > > We are using HWCAP scheme, So we may not need the very exact GNU
> > > > indirect scheme to fix the issue.
> > Agree, using indirect functions is not a must.
> >
> > > >
> > > >>
> > > >> [1]
> > > >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2
> > > >> Fwil
> > > >> lnewton.name%2F2013%2F07%2F02%2Fusing-gnu-indirect-
> > > functions%2F&amp;d
> > > >>
> > >
> ata=02%7C01%7Cyskoh%40mellanox.com%7Cda8fb7ed03e7406ded8908d6c
> > > ee6d759
> > > >> %7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C63692388
> 818
> > > 9316743&amp;
> > > >>
> > >
> sdata=x5XNd5WZ3EtiprPMiFzaskvigX8K0AoXA2w%2BKiN156c%3D&amp;res
> > > erved=0