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

[Ray Kinsella <mdr@ashroe.eu>]

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.