On 27/03/2020 14:32, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Thomas Monjalon <tho...@monjalon.net>
>> Sent: Friday, March 27, 2020 2:16 PM
>> To: Neil Horman <nhor...@tuxdriver.com>; Dodji Seketeli <do...@redhat.com>;
>> m...@ashroe.eu
>> Cc: David Marchand <david.march...@redhat.com>; Laatz, Kevin
>> <kevin.la...@intel.com>; dev <dev@dpdk.org>; Richardson, Bruce
>> <bruce.richard...@intel.com>; Van Haaren, Harry <harry.van.haa...@intel.com>;
>> Honnappa Nagarahalli <honnappa.nagaraha...@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.la...@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.
