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

[Jerin Jacob Kollanukkaran <jerinj@marvell.com>]

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