Re: [dpdk-dev] [EXT] [PATCH 5/6] build: add option for armv8 crypto extension

From: Yongseok Koh <yskoh@mellanox.com>
To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
Cc: "jerinj@marvell.com" <jerinj@marvell.com>,
	"bruce.richardson@intel.com" <bruce.richardson@intel.com>,
	Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>,
	Shahaf Shuler <shahafs@mellanox.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	Thomas Monjalon <thomas@monjalon.net>,
	"Gavin Hu (Arm Technology China)" <Gavin.Hu@arm.com>,
	dpdk-on-arm <dpdk-on-arm@arm.com>, nd <nd@arm.com>
Subject: Re: [dpdk-dev] [EXT] [PATCH 5/6] build: add option for armv8 crypto extension
Date: Wed, 17 Apr 2019 16:28:15 +0000	[thread overview]
Message-ID: <3ACFB177-32B1-4AF9-BC60-DE1EBB4EC9C7@mellanox.com> (raw)
In-Reply-To: <VE1PR08MB514978C5F96EC8FA0934C79F982B0@VE1PR08MB5149.eurprd08.prod.outlook.com>

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 