[v4] eal/cpuflags: add x86 based cpu flags

Message ID 20200416110040.42819-1-kevin.laatz@intel.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series [v4] eal/cpuflags: add x86 based cpu flags |

Checks

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

Commit Message

Kevin Laatz April 16, 2020, 11 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
Table 1-2. "Information Returned by CPUID Instruction" 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>
Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

---
v2:
  - Squashed patch set into single patch.

v3:
  - Add abignore entry for 'rte_cpu_flag_t'.

v4:
  - Updated commit message to reflect updated ISA doc linked.
  - Fixed line wrap for VNNI comment.
  - Rebased on master.
---
 devtools/libabigail.abignore              |  5 +++++
 lib/librte_eal/x86/include/rte_cpuflags.h | 19 +++++++++++++++++++
 lib/librte_eal/x86/rte_cpuflags.c         | 18 ++++++++++++++++++
 3 files changed, 42 insertions(+)
  

Comments

Thomas Monjalon April 25, 2020, 4:04 p.m. UTC | #1
16/04/2020 13:00, Kevin Laatz:
> This patch adds CPU flags which will enable the detection of ISA
> features available on more recent x86 based CPUs.
[...]
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> +; Ignore this enum update as it should not be allocated by the application
> +[suppress_type]
> +	type_kind = enum
> +	name = rte_cpu_flag_t
> +	changed_enumerators = RTE_CPUFLAG_NUMFLAGS

The justification is not correct.
The application is allowed to use RTE_CPUFLAG_NUMFLAGS in array allocation.
But no API is returning a CPU flag, so the new flags will remain unknown
to the application.

However, there is a behaviour change:
The functions rte_cpu_get_flag_name() and rte_cpu_get_flag_enabled()
will now accept new values, which were previously considered as an error.
Is it an ABI breakage? I would say no.


PS: Who is REALLY maintaining the ABI?
We really miss someone who carefully check all these things,
and take care of the doc and tooling.
  
Kinsella, Ray April 27, 2020, 9:22 a.m. UTC | #2
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Saturday 25 April 2020 17:04
> To: Kinsella, Ray <ray.kinsella@intel.com>; nhorman@tuxdriver.com;
> Laatz, Kevin <kevin.laatz@intel.com>
> Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Van
> Haaren, Harry <harry.van.haaren@intel.com>; david.marchand@redhat.com
> Subject: Re: [dpdk-dev] [PATCH v4] eal/cpuflags: add x86 based cpu
> flags
> 
> 16/04/2020 13:00, Kevin Laatz:
> > This patch adds CPU flags which will enable the detection of ISA
> > features available on more recent x86 based CPUs.
> [...]
> > --- a/devtools/libabigail.abignore
> > +++ b/devtools/libabigail.abignore
> > +; Ignore this enum update as it should not be allocated by the
> > +application [suppress_type]
> > +	type_kind = enum
> > +	name = rte_cpu_flag_t
> > +	changed_enumerators = RTE_CPUFLAG_NUMFLAGS
> 
> The justification is not correct.
> The application is allowed to use RTE_CPUFLAG_NUMFLAGS in array
> allocation.
> But no API is returning a CPU flag, so the new flags will remain
> unknown to the application.
> 
> However, there is a behaviour change:
> The functions rte_cpu_get_flag_name() and rte_cpu_get_flag_enabled()
> will now accept new values, which were previously considered as an
> error.
> Is it an ABI breakage? I would say no.

We saw something similar with the Cryptodev's rte_crypto_sym_xform_type also.
Libabigail appears to be particularly sensitive to changes to enumerations. 
Leaving it to the user to decide if there is a problem. 

I am seeing a bit of weirdness though between versions of libabigail.
1.7.1 seems to fine with the change, however 1.2 is reporting an issue. 

Kevin - what version are you using?

> 
> PS: Who is REALLY maintaining the ABI?
> We really miss someone who carefully check all these things, and take
> care of the doc and tooling.

I would say that I am missing these changes to libabigail.ignore, which would be useful. 
Should we consolidate the ABI Policy and ABI Versioning sections of the MAINTAINERS file?
  
Ray Kinsella April 27, 2020, 9:27 a.m. UTC | #3
(replying this time to the list)

On 25/04/2020 17:04, Thomas Monjalon wrote:
> 16/04/2020 13:00, Kevin Laatz:
>> This patch adds CPU flags which will enable the detection of ISA
>> features available on more recent x86 based CPUs.
> [...]
>> --- a/devtools/libabigail.abignore
>> +++ b/devtools/libabigail.abignore
>> +; Ignore this enum update as it should not be allocated by the application
>> +[suppress_type]
>> +	type_kind = enum
>> +	name = rte_cpu_flag_t
>> +	changed_enumerators = RTE_CPUFLAG_NUMFLAGS
> 
> The justification is not correct.
> The application is allowed to use RTE_CPUFLAG_NUMFLAGS in array allocation.
> But no API is returning a CPU flag, so the new flags will remain unknown
> to the application.
> 
> However, there is a behaviour change:
> The functions rte_cpu_get_flag_name() and rte_cpu_get_flag_enabled()
> will now accept new values, which were previously considered as an error.
> Is it an ABI breakage? I would say no.

We saw something similar with the Cryptodev's rte_crypto_sym_xform_type also.
Libabigail appears to be particularly sensitive to changes to enumerations. 
Leaving it to the user to decide if there is a problem. 

I am seeing a bit of weirdness though between versions of libabigail.
1.7.1 seems to fine with the change, however 1.2 is reporting an issue. 

Kevin - what version are you using?

> 
> PS: Who is REALLY maintaining the ABI?
> We really miss someone who carefully check all these things,
> and take care of the doc and tooling.
> 
> 

I would say that I am missing these changes to libabigail.ignore, which would be useful. 
Should we consolidate the ABI Policy and ABI Versioning sections of the MAINTAINERS file?
  
Kevin Laatz April 27, 2020, 9:31 a.m. UTC | #4
> (replying this time to the list)
> 
> On 25/04/2020 17:04, Thomas Monjalon wrote:
> > 16/04/2020 13:00, Kevin Laatz:
> >> This patch adds CPU flags which will enable the detection of ISA
> >> features available on more recent x86 based CPUs.
> > [...]
> >> --- a/devtools/libabigail.abignore
> >> +++ b/devtools/libabigail.abignore
> >> +; Ignore this enum update as it should not be allocated by the
> >> +application [suppress_type]
> >> +	type_kind = enum
> >> +	name = rte_cpu_flag_t
> >> +	changed_enumerators = RTE_CPUFLAG_NUMFLAGS
> >
> > The justification is not correct.
> > The application is allowed to use RTE_CPUFLAG_NUMFLAGS in array
> allocation.
> > But no API is returning a CPU flag, so the new flags will remain
> > unknown to the application.
> >
> > However, there is a behaviour change:
> > The functions rte_cpu_get_flag_name() and rte_cpu_get_flag_enabled()
> > will now accept new values, which were previously considered as an error.
> > Is it an ABI breakage? I would say no.
> 
> We saw something similar with the Cryptodev's rte_crypto_sym_xform_type
> also.
> Libabigail appears to be particularly sensitive to changes to enumerations.
> Leaving it to the user to decide if there is a problem.
> 
> I am seeing a bit of weirdness though between versions of libabigail.
> 1.7.1 seems to fine with the change, however 1.2 is reporting an issue.
> 
> Kevin - what version are you using?

I'm using version 1.6.0

> 
> >
> > PS: Who is REALLY maintaining the ABI?
> > We really miss someone who carefully check all these things, and take
> > care of the doc and tooling.
> >
> >
> 
> I would say that I am missing these changes to libabigail.ignore, which would
> be useful.
> Should we consolidate the ABI Policy and ABI Versioning sections of the
> MAINTAINERS file?
  
Ray Kinsella April 27, 2020, 9:35 a.m. UTC | #5
On 27/04/2020 10:31, Laatz, Kevin wrote:
>  
>> (replying this time to the list)
>>
>> On 25/04/2020 17:04, Thomas Monjalon wrote:
>>> 16/04/2020 13:00, Kevin Laatz:
>>>> This patch adds CPU flags which will enable the detection of ISA
>>>> features available on more recent x86 based CPUs.
>>> [...]
>>>> --- a/devtools/libabigail.abignore
>>>> +++ b/devtools/libabigail.abignore
>>>> +; Ignore this enum update as it should not be allocated by the
>>>> +application [suppress_type]
>>>> +	type_kind = enum
>>>> +	name = rte_cpu_flag_t
>>>> +	changed_enumerators = RTE_CPUFLAG_NUMFLAGS
>>>
>>> The justification is not correct.
>>> The application is allowed to use RTE_CPUFLAG_NUMFLAGS in array
>> allocation.
>>> But no API is returning a CPU flag, so the new flags will remain
>>> unknown to the application.
>>>
>>> However, there is a behaviour change:
>>> The functions rte_cpu_get_flag_name() and rte_cpu_get_flag_enabled()
>>> will now accept new values, which were previously considered as an error.
>>> Is it an ABI breakage? I would say no.
>>
>> We saw something similar with the Cryptodev's rte_crypto_sym_xform_type
>> also.
>> Libabigail appears to be particularly sensitive to changes to enumerations.
>> Leaving it to the user to decide if there is a problem.
>>
>> I am seeing a bit of weirdness though between versions of libabigail.
>> 1.7.1 seems to fine with the change, however 1.2 is reporting an issue.
>>
>> Kevin - what version are you using?
> 
> I'm using version 1.6.0

right you are either on Fedora 31 or some Ubuntu v19.xx, right?

> 
>>
>>>
>>> PS: Who is REALLY maintaining the ABI?
>>> We really miss someone who carefully check all these things, and take
>>> care of the doc and tooling.
>>>
>>>
>>
>> I would say that I am missing these changes to libabigail.ignore, which would
>> be useful.
>> Should we consolidate the ABI Policy and ABI Versioning sections of the
>> MAINTAINERS file?
>
  
Kevin Laatz April 27, 2020, 10:08 a.m. UTC | #6
> On 27/04/2020 10:31, Laatz, Kevin wrote:
> >
> >> (replying this time to the list)
> >>
> >> On 25/04/2020 17:04, Thomas Monjalon wrote:
> >>> 16/04/2020 13:00, Kevin Laatz:
> >>>> This patch adds CPU flags which will enable the detection of ISA
> >>>> features available on more recent x86 based CPUs.
> >>> [...]
> >>>> --- a/devtools/libabigail.abignore
> >>>> +++ b/devtools/libabigail.abignore
> >>>> +; Ignore this enum update as it should not be allocated by the
> >>>> +application [suppress_type]
> >>>> +	type_kind = enum
> >>>> +	name = rte_cpu_flag_t
> >>>> +	changed_enumerators = RTE_CPUFLAG_NUMFLAGS
> >>>
> >>> The justification is not correct.
> >>> The application is allowed to use RTE_CPUFLAG_NUMFLAGS in array
> >> allocation.
> >>> But no API is returning a CPU flag, so the new flags will remain
> >>> unknown to the application.
> >>>
> >>> However, there is a behaviour change:
> >>> The functions rte_cpu_get_flag_name() and
> rte_cpu_get_flag_enabled()
> >>> will now accept new values, which were previously considered as an
> error.
> >>> Is it an ABI breakage? I would say no.
> >>
> >> We saw something similar with the Cryptodev's
> >> rte_crypto_sym_xform_type also.
> >> Libabigail appears to be particularly sensitive to changes to enumerations.
> >> Leaving it to the user to decide if there is a problem.
> >>
> >> I am seeing a bit of weirdness though between versions of libabigail.
> >> 1.7.1 seems to fine with the change, however 1.2 is reporting an issue.
> >>
> >> Kevin - what version are you using?
> >
> > I'm using version 1.6.0
> 
> right you are either on Fedora 31 or some Ubuntu v19.xx, right?

At the time of making the patch: Ubuntu 18.04 with a manually upgraded libabigail
Currently Ubuntu 20.04 (beta). 

> 
> >
> >>
> >>>
> >>> PS: Who is REALLY maintaining the ABI?
> >>> We really miss someone who carefully check all these things, and
> >>> take care of the doc and tooling.
> >>>
> >>>
> >>
> >> I would say that I am missing these changes to libabigail.ignore,
> >> which would be useful.
> >> Should we consolidate the ABI Policy and ABI Versioning sections of
> >> the MAINTAINERS file?
> >
  
Thomas Monjalon April 27, 2020, 12:31 p.m. UTC | #7
27/04/2020 11:27, Ray Kinsella:
> On 25/04/2020 17:04, Thomas Monjalon wrote:
> > PS: Who is REALLY maintaining the ABI?
> > We really miss someone who carefully check all these things,
> > and take care of the doc and tooling.
> 
> I would say that I am missing these changes to libabigail.ignore, which would be useful. 
> Should we consolidate the ABI Policy and ABI Versioning sections of the MAINTAINERS file?

Yes, I think it does not make sense spliting ABI topic in 2 sections
in MAINTAINERS file.
We need to have a clear ownership covering policy, libs, tooling and doc.
Let's agree to merge all in one section please.
  
Ray Kinsella April 27, 2020, 1:58 p.m. UTC | #8
On 27/04/2020 13:31, Thomas Monjalon wrote:
> 27/04/2020 11:27, Ray Kinsella:
>> On 25/04/2020 17:04, Thomas Monjalon wrote:
>>> PS: Who is REALLY maintaining the ABI?
>>> We really miss someone who carefully check all these things,
>>> and take care of the doc and tooling.
>>
>> I would say that I am missing these changes to libabigail.ignore, which would be useful. 
>> Should we consolidate the ABI Policy and ABI Versioning sections of the MAINTAINERS file?
> 
> Yes, I think it does not make sense spliting ABI topic in 2 sections
> in MAINTAINERS file.
> We need to have a clear ownership covering policy, libs, tooling and doc.
> Let's agree to merge all in one section please.
> 

I would suggest merging and listing myself and Neil as maintainers?
Unless you are aware of another potential owner?

Ray K
  
Neil Horman April 29, 2020, 11:22 a.m. UTC | #9
On Mon, Apr 27, 2020 at 02:58:07PM +0100, Ray Kinsella wrote:
> 
> 
> On 27/04/2020 13:31, Thomas Monjalon wrote:
> > 27/04/2020 11:27, Ray Kinsella:
> >> On 25/04/2020 17:04, Thomas Monjalon wrote:
> >>> PS: Who is REALLY maintaining the ABI?
> >>> We really miss someone who carefully check all these things,
> >>> and take care of the doc and tooling.
> >>
> >> I would say that I am missing these changes to libabigail.ignore, which would be useful. 
> >> Should we consolidate the ABI Policy and ABI Versioning sections of the MAINTAINERS file?
> > 
> > Yes, I think it does not make sense spliting ABI topic in 2 sections
> > in MAINTAINERS file.
> > We need to have a clear ownership covering policy, libs, tooling and doc.
> > Let's agree to merge all in one section please.
> > 
> 
> I would suggest merging and listing myself and Neil as maintainers?
> Unless you are aware of another potential owner?
> 
I'm ok with this
Neil

> Ray K
>  
>
  
Ray Kinsella April 30, 2020, 7:59 a.m. UTC | #10
On 29/04/2020 12:22, Neil Horman wrote:
> On Mon, Apr 27, 2020 at 02:58:07PM +0100, Ray Kinsella wrote:
>>
>>
>> On 27/04/2020 13:31, Thomas Monjalon wrote:
>>> 27/04/2020 11:27, Ray Kinsella:
>>>> On 25/04/2020 17:04, Thomas Monjalon wrote:
>>>>> PS: Who is REALLY maintaining the ABI?
>>>>> We really miss someone who carefully check all these things,
>>>>> and take care of the doc and tooling.
>>>>
>>>> I would say that I am missing these changes to libabigail.ignore, which would be useful. 
>>>> Should we consolidate the ABI Policy and ABI Versioning sections of the MAINTAINERS file?
>>>
>>> Yes, I think it does not make sense spliting ABI topic in 2 sections
>>> in MAINTAINERS file.
>>> We need to have a clear ownership covering policy, libs, tooling and doc.
>>> Let's agree to merge all in one section please.
>>>
>>
>> I would suggest merging and listing myself and Neil as maintainers?
>> Unless you are aware of another potential owner?
>>
> I'm ok with this
> Neil

ok I will take care of it in the next rev of the 

"[PATCH] abi: change references to abi 20.0.1 to abi v21"
 
>> Ray K
>>  
>>
  

Patch

diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index a59df8f13..eb252ae0e 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -11,3 +11,8 @@ 
         type_kind = enum
         name = rte_crypto_asym_xform_type
         changed_enumerators = RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
+; Ignore this enum update as it should not be allocated by the application
+[suppress_type]
+	type_kind = enum
+	name = rte_cpu_flag_t
+	changed_enumerators = RTE_CPUFLAG_NUMFLAGS
diff --git a/lib/librte_eal/x86/include/rte_cpuflags.h b/lib/librte_eal/x86/include/rte_cpuflags.h
index 25ba47b96..c1d20364d 100644
--- a/lib/librte_eal/x86/include/rte_cpuflags.h
+++ b/lib/librte_eal/x86/include/rte_cpuflags.h
@@ -113,6 +113,25 @@  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! */
 };
diff --git a/lib/librte_eal/x86/rte_cpuflags.c b/lib/librte_eal/x86/rte_cpuflags.c
index 6492df556..30439e795 100644
--- a/lib/librte_eal/x86/rte_cpuflags.c
+++ b/lib/librte_eal/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