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

[Thomas Monjalon <thomas@monjalon.net>]

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.