Re: [dpdk-dev] [PATCH v2] eal/cpuflags: add x86 based cpu flags

[David Marchand <david.marchand@redhat.com>]

On Fri, Mar 27, 2020 at 2:18 PM Van Haaren, Harry
<harry.van.haaren@intel.com> wrote:
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Friday, March 27, 2020 12:24 PM
> > To: Laatz, Kevin <kevin.laatz@intel.com>
> > Cc: dev <dev@dpdk.org>; Richardson, Bruce <bruce.richardson@intel.com>; Van
> > Haaren, Harry <harry.van.haaren@intel.com>; Neil Horman
> > <nhorman@tuxdriver.com>; Thomas Monjalon <thomas@monjalon.net>; Honnappa
> > Nagarahalli <Honnappa.Nagarahalli@arm.com>; Dodji Seketeli <dodji@redhat.com>
> > Subject: Re: [dpdk-dev] [PATCH v2] eal/cpuflags: add x86 based cpu flags
> >
> > On Wed, Mar 25, 2020 at 12:11 PM Kevin Laatz <kevin.laatz@intel.com> wrote:
> > >
> > > This patch adds CPU flags which will enable the detection of ISA
> > > features available on more recent x86 based CPUs.
> > >
> > > The CPUID leaf information can be found in Section 1.7 of this
> > > document:
> > > https://software.intel.com/sites/default/files/managed/c5/15/architecture-
> > instruction-set-extensions-programming-reference.pdf
> > >
> > > The following CPU flags are added in this patch:
> > >     - AVX-512 doubleword and quadword instructions.
> > >     - AVX-512 integer fused multiply-add instructions.
> > >     - AVX-512 conflict detection instructions.
> > >     - AVX-512 byte and word instructions.
> > >     - AVX-512 vector length instructions.
> > >     - AVX-512 vector bit manipulation instructions.
> > >     - AVX-512 vector bit manipulation 2 instructions.
> > >     - Galois field new instructions.
> > >     - Vector AES instructions.
> > >     - Vector carry-less multiply instructions.
> > >     - AVX-512 vector neural network instructions.
> > >     - AVX-512 for bit algorithm instructions.
> > >     - AVX-512 vector popcount instructions.
> > >     - Cache line demote instructions.
> > >     - Direct store instructions.
> > >     - Direct store 64B instructions.
> > >     - AVX-512 two register intersection instructions.
> > >
> > > Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> > > ---
> > >  lib/librte_eal/common/arch/x86/rte_cpuflags.c  | 18 ++++++++++++++++++
> > >  .../common/include/arch/x86/rte_cpuflags.h     | 18 ++++++++++++++++++
> > >  2 files changed, 36 insertions(+)
> > >
> > > diff --git a/lib/librte_eal/common/arch/x86/rte_cpuflags.c
> > b/lib/librte_eal/common/arch/x86/rte_cpuflags.c
> > > index 6492df556..30439e795 100644
> > > --- a/lib/librte_eal/common/arch/x86/rte_cpuflags.c
> > > +++ b/lib/librte_eal/common/arch/x86/rte_cpuflags.c
> > > @@ -120,6 +120,24 @@ const struct feature_entry rte_cpu_feature_table[] = {
> > >         FEAT_DEF(EM64T, 0x80000001, 0, RTE_REG_EDX, 29)
> > >
> > >         FEAT_DEF(INVTSC, 0x80000007, 0, RTE_REG_EDX,  8)
> > > +
> > > +       FEAT_DEF(AVX512DQ, 0x00000007, 0, RTE_REG_EBX, 17)
> > > +       FEAT_DEF(AVX512IFMA, 0x00000007, 0, RTE_REG_EBX, 21)
> > > +       FEAT_DEF(AVX512CD, 0x00000007, 0, RTE_REG_EBX, 28)
> > > +       FEAT_DEF(AVX512BW, 0x00000007, 0, RTE_REG_EBX, 30)
> > > +       FEAT_DEF(AVX512VL, 0x00000007, 0, RTE_REG_EBX, 31)
> > > +       FEAT_DEF(AVX512VBMI, 0x00000007, 0, RTE_REG_ECX, 1)
> > > +       FEAT_DEF(AVX512VBMI2, 0x00000007, 0, RTE_REG_ECX, 6)
> > > +       FEAT_DEF(GFNI, 0x00000007, 0, RTE_REG_ECX, 8)
> > > +       FEAT_DEF(VAES, 0x00000007, 0, RTE_REG_ECX, 9)
> > > +       FEAT_DEF(VPCLMULQDQ, 0x00000007, 0, RTE_REG_ECX, 10)
> > > +       FEAT_DEF(AVX512VNNI, 0x00000007, 0, RTE_REG_ECX, 11)
> > > +       FEAT_DEF(AVX512BITALG, 0x00000007, 0, RTE_REG_ECX, 12)
> > > +       FEAT_DEF(AVX512VPOPCNTDQ, 0x00000007, 0, RTE_REG_ECX,  14)
> > > +       FEAT_DEF(CLDEMOTE, 0x00000007, 0, RTE_REG_ECX, 25)
> > > +       FEAT_DEF(MOVDIRI, 0x00000007, 0, RTE_REG_ECX, 27)
> > > +       FEAT_DEF(MOVDIR64B, 0x00000007, 0, RTE_REG_ECX, 28)
> > > +       FEAT_DEF(AVX512VP2INTERSECT, 0x00000007, 0, RTE_REG_EDX, 8)
> > >  };
> > >
> > >  int
> > > diff --git a/lib/librte_eal/common/include/arch/x86/rte_cpuflags.h
> > b/lib/librte_eal/common/include/arch/x86/rte_cpuflags.h
> > > index 25ba47b96..f8f73b19f 100644
> > > --- a/lib/librte_eal/common/include/arch/x86/rte_cpuflags.h
> > > +++ b/lib/librte_eal/common/include/arch/x86/rte_cpuflags.h
> > > @@ -113,6 +113,24 @@ enum rte_cpu_flag_t {
> > >         /* (EAX 80000007h) EDX features */
> > >         RTE_CPUFLAG_INVTSC,                 /**< INVTSC */
> > >
> > > +       RTE_CPUFLAG_AVX512DQ,               /**< AVX512 Doubleword and
> > Quadword */
> > > +       RTE_CPUFLAG_AVX512IFMA,             /**< AVX512 Integer Fused
> > Multiply-Add */
> > > +       RTE_CPUFLAG_AVX512CD,               /**< AVX512 Conflict Detection*/
> > > +       RTE_CPUFLAG_AVX512BW,               /**< AVX512 Byte and Word */
> > > +       RTE_CPUFLAG_AVX512VL,               /**< AVX512 Vector Length */
> > > +       RTE_CPUFLAG_AVX512VBMI,             /**< AVX512 Vector Bit
> > Manipulation */
> > > +       RTE_CPUFLAG_AVX512VBMI2,            /**< AVX512 Vector Bit
> > Manipulation 2 */
> > > +       RTE_CPUFLAG_GFNI,                   /**< Galois Field New
> > Instructions */
> > > +       RTE_CPUFLAG_VAES,                   /**< Vector AES */
> > > +       RTE_CPUFLAG_VPCLMULQDQ,             /**< Vector Carry-less Multiply
> > */
> > > +       RTE_CPUFLAG_AVX512VNNI,             /**< AVX512 Vector Neural
> > Network Instructions */
> > > +       RTE_CPUFLAG_AVX512BITALG,           /**< AVX512 Bit Algorithms */
> > > +       RTE_CPUFLAG_AVX512VPOPCNTDQ,        /**< AVX512 Vector Popcount */
> > > +       RTE_CPUFLAG_CLDEMOTE,               /**< Cache Line Demote */
> > > +       RTE_CPUFLAG_MOVDIRI,                /**< Direct Store Instructions
> > */
> > > +       RTE_CPUFLAG_MOVDIR64B,              /**< Direct Store Instructions
> > 64B */
> > > +       RTE_CPUFLAG_AVX512VP2INTERSECT,     /**< AVX512 Two Register
> > Intersection */
> > > +
> > >         /* The last item */
> > >         RTE_CPUFLAG_NUMFLAGS,               /**< This should always be the
> > last! */
> >
> > This is seen as an ABI break because of the change on _NUMFLAGS:
> > https://travis-ci.com/github/ovsrobot/dpdk/jobs/302524264#L2351
>
> Correct a publicly exposed enum max value has changed - I don't believe this is an ABI break, but a backward compatible ABI change was expected with this patchset.

A change is that if an application was passing incorrect values to the
functions taking this enum as input, then now it would succeed.
I don't really see the point in doing this :-).

>
> Code compiled against eg 19.11 or 20.02 is expected to continue operating correctly.
> The new flags were only added at the end of the enum, ensuring to not change the meaning of any existing flags which would be compiled-in constants to the application binary.
>
> The actual size of the CPU flags array is a DPDK internal structure (in a .c file), and is hidden from the application, and never allocated by an application - so no possible mismatch in ABI there? Applications compiled against the older ABI will just not know about the newer flags - but suffer no breakage.
>
> @ABI compatibility folks, please review too - but to the best of my understanding this is not an ABI break, but a backwards compatible update of CPU flag lists?
>
> Thanks for flagging the CI results David!

I'd like people to look at this by themselves, not wait for Thomas,
Aaron or me to check.

When a failure is caught by the robot, a mail is sent to the submitter
afaiu (I suppose with Travis instability wrt ARM jobs, the mail might
not have been sent this time).
It is then the responsibility of the submitter to either discuss the
report on the mailing or/and waive it in devtools/libabigail.abignore.

Thanks.


-- 
David Marchand