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

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

> -----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.