[v2] eal/cpuflags: add x86 based cpu flags
diff mbox series

Message ID 20200325111016.29163-1-kevin.laatz@intel.com
State Superseded
Delegated to: David Marchand
Headers show
Series
  • [v2] eal/cpuflags: add x86 based cpu flags
Related show

Checks

Context Check Description
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/checkpatch warning coding style issues

Commit Message

Kevin Laatz March 25, 2020, 11:10 a.m. UTC
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(+)

Comments

David Marchand March 27, 2020, 12:24 p.m. UTC | #1
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
Van Haaren, Harry March 27, 2020, 1:18 p.m. UTC | #2
> -----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.

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!

Regards, -Harry
Neil Horman March 27, 2020, 1:44 p.m. UTC | #3
On Fri, Mar 27, 2020 at 01:24:12PM +0100, David Marchand wrote:
> 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
> 
It shouldn't be, as the only API calls we expose that use rte_cpu_flag_t accept
it as an integer parameter to see if the flag is enabled.  Theres no use of the
enum in a public array or any struct that is sized based on the number of flags,
so you should be good to go
Neil

> 
> -- 
> David Marchand
> 
>
Thomas Monjalon March 27, 2020, 2:15 p.m. UTC | #4
27/03/2020 14:44, Neil Horman:
> On Fri, Mar 27, 2020 at 01:24:12PM +0100, David Marchand wrote:
> > On Wed, Mar 25, 2020 at 12:11 PM Kevin Laatz <kevin.laatz@intel.com> wrote:
> > > --- 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
> > 
> It shouldn't be, as the only API calls we expose that use rte_cpu_flag_t accept
> it as an integer parameter to see if the flag is enabled.  Theres no use of the
> enum in a public array or any struct that is sized based on the number of flags,
> so you should be good to go

Indeed I cannot imagine an ABI incompatibility in this case.
The only behaviour change is to accept new (higher) RTE_CPUFLAG values
in functions rte_cpu_get_flag_enabled() and rte_cpu_get_flag_name().
Is changing the range of valid values an ABI break?
Why is it flagged by libabigail?
Van Haaren, Harry March 27, 2020, 2:32 p.m. UTC | #5
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, March 27, 2020 2:16 PM
> To: Neil Horman <nhorman@tuxdriver.com>; Dodji Seketeli <dodji@redhat.com>;
> mdr@ashroe.eu
> Cc: David Marchand <david.marchand@redhat.com>; Laatz, Kevin
> <kevin.laatz@intel.com>; dev <dev@dpdk.org>; Richardson, Bruce
> <bruce.richardson@intel.com>; Van Haaren, Harry <harry.van.haaren@intel.com>;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v2] eal/cpuflags: add x86 based cpu flags
> 
> 27/03/2020 14:44, Neil Horman:
> > On Fri, Mar 27, 2020 at 01:24:12PM +0100, David Marchand wrote:
> > > On Wed, Mar 25, 2020 at 12:11 PM Kevin Laatz <kevin.laatz@intel.com>
> wrote:
> > > > --- 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
> > >
> > It shouldn't be, as the only API calls we expose that use rte_cpu_flag_t
> accept
> > it as an integer parameter to see if the flag is enabled.  Theres no use of
> the
> > enum in a public array or any struct that is sized based on the number of
> flags,
> > so you should be good to go
> 
> Indeed I cannot imagine an ABI incompatibility in this case.
> The only behaviour change is to accept new (higher) RTE_CPUFLAG values
> in functions rte_cpu_get_flag_enabled() and rte_cpu_get_flag_name().
> Is changing the range of valid values an ABI break?
> Why is it flagged by libabigail?

If this enum _MAX value was used by the application to allocate an array, that later our DPDK code would write to it could cause out-of-bounds array accesses of the application supplied array. Abigail doesn't know what applications could use the value for, so it flags it.

IMO Abigail is right to flag it to us - a manual review to understand what that _MAX enum value is used for, and then decide on a case by case basis seems the best way forward to me.

Thanks Neil/Thomas for reviewing, as reply in this thread, I also believe this is not going to break ABI.
Ray Kinsella March 27, 2020, 2:36 p.m. UTC | #6
On 27/03/2020 14:32, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Thomas Monjalon <thomas@monjalon.net>
>> Sent: Friday, March 27, 2020 2:16 PM
>> To: Neil Horman <nhorman@tuxdriver.com>; Dodji Seketeli <dodji@redhat.com>;
>> mdr@ashroe.eu
>> Cc: David Marchand <david.marchand@redhat.com>; Laatz, Kevin
>> <kevin.laatz@intel.com>; dev <dev@dpdk.org>; Richardson, Bruce
>> <bruce.richardson@intel.com>; Van Haaren, Harry <harry.van.haaren@intel.com>;
>> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
>> Subject: Re: [dpdk-dev] [PATCH v2] eal/cpuflags: add x86 based cpu flags
>>
>> 27/03/2020 14:44, Neil Horman:
>>> On Fri, Mar 27, 2020 at 01:24:12PM +0100, David Marchand wrote:
>>>> On Wed, Mar 25, 2020 at 12:11 PM Kevin Laatz <kevin.laatz@intel.com>
>> wrote:
>>>>> --- 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
>>>>
>>> It shouldn't be, as the only API calls we expose that use rte_cpu_flag_t
>> accept
>>> it as an integer parameter to see if the flag is enabled.  Theres no use of
>> the
>>> enum in a public array or any struct that is sized based on the number of
>> flags,
>>> so you should be good to go
>>
>> Indeed I cannot imagine an ABI incompatibility in this case.
>> The only behaviour change is to accept new (higher) RTE_CPUFLAG values
>> in functions rte_cpu_get_flag_enabled() and rte_cpu_get_flag_name().
>> Is changing the range of valid values an ABI break?
>> Why is it flagged by libabigail?
> 
> If this enum _MAX value was used by the application to allocate an array, that later our DPDK code would write to it could cause out-of-bounds array accesses of the application supplied array. Abigail doesn't know what applications could use the value for, so it flags it.
> 
> IMO Abigail is right to flag it to us - a manual review to understand what that _MAX enum value is used for, and then decide on a case by case basis seems the best way forward to me.
> 
> Thanks Neil/Thomas for reviewing, as reply in this thread, I also believe this is not going to break ABI.
> 
 
+1 to Harrry's comments. 
I can't immediately see how this might break the ABI either.
David Marchand March 27, 2020, 3:04 p.m. UTC | #7
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.
Thomas Monjalon March 27, 2020, 3:19 p.m. UTC | #8
27/03/2020 15:36, Ray Kinsella:
> On 27/03/2020 14:32, Van Haaren, Harry wrote:
> > From: Thomas Monjalon <thomas@monjalon.net>
> >> 27/03/2020 14:44, Neil Horman:
> >>> On Fri, Mar 27, 2020 at 01:24:12PM +0100, David Marchand wrote:
> >>>> On Wed, Mar 25, 2020 at 12:11 PM Kevin Laatz <kevin.laatz@intel.com>
> >> wrote:
> >>>>> --- 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
> >>>>
> >>> It shouldn't be, as the only API calls we expose that use rte_cpu_flag_t
> >> accept
> >>> it as an integer parameter to see if the flag is enabled.  Theres no use of
> >> the
> >>> enum in a public array or any struct that is sized based on the number of
> >> flags,
> >>> so you should be good to go
> >>
> >> Indeed I cannot imagine an ABI incompatibility in this case.
> >> The only behaviour change is to accept new (higher) RTE_CPUFLAG values
> >> in functions rte_cpu_get_flag_enabled() and rte_cpu_get_flag_name().
> >> Is changing the range of valid values an ABI break?
> >> Why is it flagged by libabigail?
> > 
> > If this enum _MAX value was used by the application to allocate an array, that later our DPDK code would write to it could cause out-of-bounds array accesses of the application supplied array. Abigail doesn't know what applications could use the value for, so it flags it.
> > 
> > IMO Abigail is right to flag it to us - a manual review to understand what that _MAX enum value is used for, and then decide on a case by case basis seems the best way forward to me.
> > 
> > Thanks Neil/Thomas for reviewing, as reply in this thread, I also believe this is not going to break ABI.
> > 
>  
> +1 to Harrry's comments. 
> I can't immediately see how this might break the ABI either. 


So we all agree that increasing the range of valid rte_cpu_flag_t values
is OK for ABI compatibility.
This conclusion must be written as a libabigail exception in this patch.

Patch
diff mbox series

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! */
 };