mbox series

[00/16] NXP DPAAx fixes and enhancements

Message ID 20200302145829.27808-1-hemant.agrawal@nxp.com
Headers show
Series NXP DPAAx fixes and enhancements | expand

Message

Hemant Agrawal March 2, 2020, 2:58 p.m. UTC
This patch series add various patches for enhancing and fixing NXP
fslmc bus, dpaa bus, and dpaax.

- the main change is support to allow thread migration across lcores
- improving the multi-process support


Apeksha Gupta (1):
  bus/fslmc: fix dereferencing null pointer

Gagandeep Singh (2):
  bus/fslmc: combine thread specific variables
  net/dpaa: enable Tx queue taildrop

Hemant Agrawal (1):
  bus/fslmc: support handle portal alloc failure

Nipun Gupta (8):
  bus/fslmc: rework portal allocation to a per thread basis
  bus/fslmc: limit pthread destructor called for dpaa2 only
  bus/fslmc: support portal migration
  drivers: enhance portal allocation failure log
  bus/fslmc: rename the cinh read functions used for ls1088
  net/dpaa: return error on multiple mp config
  net/dpaa: update process specific device info
  net/dpaa2: do not prefetch annotaion for physical mode

Rohit Raj (3):
  net/dpaa2: fix 10g port negotiation issue
  bus/dpaa: enable link state interrupt
  bus/dpaa: enable set link status

Sachin Saxena (1):
  net/dpaa: add 2.5G support

 drivers/bus/dpaa/base/fman/fman.c             |  10 +-
 drivers/bus/dpaa/base/fman/netcfg_layer.c     |   3 +-
 drivers/bus/dpaa/base/qbman/process.c         |  95 ++-
 drivers/bus/dpaa/base/qbman/qman.c            |  43 ++
 drivers/bus/dpaa/dpaa_bus.c                   |  28 +-
 drivers/bus/dpaa/include/fman.h               |   3 +
 drivers/bus/dpaa/include/fsl_qman.h           |  17 +
 drivers/bus/dpaa/include/process.h            |  23 +
 drivers/bus/dpaa/rte_bus_dpaa_version.map     |  11 +
 drivers/bus/dpaa/rte_dpaa_bus.h               |   6 +-
 drivers/bus/fslmc/Makefile                    |   1 +
 drivers/bus/fslmc/fslmc_bus.c                 |   2 -
 drivers/bus/fslmc/portal/dpaa2_hw_dpio.c      | 284 ++++-----
 drivers/bus/fslmc/portal/dpaa2_hw_dpio.h      |  10 +-
 drivers/bus/fslmc/portal/dpaa2_hw_pvt.h       |  14 +-
 .../fslmc/qbman/include/fsl_qbman_portal.h    |   8 +-
 drivers/bus/fslmc/qbman/qbman_debug.c         |   9 +-
 drivers/bus/fslmc/qbman/qbman_portal.c        | 580 +++++++++++++++++-
 drivers/bus/fslmc/qbman/qbman_portal.h        |  19 +-
 drivers/bus/fslmc/qbman/qbman_sys.h           | 135 +++-
 drivers/bus/fslmc/rte_fslmc.h                 |  18 -
 drivers/common/dpaax/compat.h                 |   5 +-
 drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c   |   8 +-
 drivers/event/dpaa2/dpaa2_eventdev.c          |   8 +-
 drivers/mempool/dpaa/dpaa_mempool.c           |   1 +
 drivers/mempool/dpaa2/dpaa2_hw_mempool.c      |  12 +-
 drivers/net/dpaa/dpaa_ethdev.c                | 417 +++++++++----
 drivers/net/dpaa/dpaa_ethdev.h                |   3 +-
 drivers/net/dpaa/dpaa_rxtx.c                  |  71 +++
 drivers/net/dpaa/dpaa_rxtx.h                  |   3 +
 drivers/net/dpaa2/dpaa2_ethdev.c              |   8 +-
 drivers/net/dpaa2/dpaa2_rxtx.c                |  56 +-
 drivers/raw/dpaa2_cmdif/dpaa2_cmdif.c         |   8 +-
 drivers/raw/dpaa2_qdma/dpaa2_qdma.c           |  12 +-
 34 files changed, 1566 insertions(+), 365 deletions(-)

-- 
2.17.1

Comments

David Marchand March 2, 2020, 1:01 p.m. UTC | #1
On Mon, Mar 2, 2020 at 10:26 AM Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
>

> This patch series add various patches for enhancing and fixing NXP

> fslmc bus, dpaa bus, and dpaax.

>

> - the main change is support to allow thread migration across lcores

> - improving the multi-process support


This series triggers an ABI warning that must be investigated.
https://travis-ci.com/ovsrobot/dpdk/jobs/292904119#L2233


-- 
David Marchand
Hemant Agrawal March 5, 2020, 9:06 a.m. UTC | #2
Hi David,
> On Mon, Mar 2, 2020 at 10:26 AM Hemant Agrawal

> <hemant.agrawal@nxp.com> wrote:

> >

> > This patch series add various patches for enhancing and fixing NXP

> > fslmc bus, dpaa bus, and dpaax.

> >

> > - the main change is support to allow thread migration across lcores

> > - improving the multi-process support

> 

> This series triggers an ABI warning that must be investigated.

>https://travis-ci.com/ovsrobot/dpdk/jobs/292904119#L2233


[Hemant] 
As per the logs:

Variables changes summary: 1 Removed, 2 Changed, 0 Added variables
1 Removed variable:
  'dpaa2_portal_dqrr per_lcore_dpaa2_held_bufs'    {per_lcore_dpaa2_held_bufs@@DPDK_20.0}
2 Changed variables:
  [C]'dpaa2_io_portal_t dpaa2_io_portal[128]' was changed at dpaa2_hw_dpio.h:40:1: size of symbol changed from 5120 to 2048
  [C]'dpaa2_io_portal_t per_lcore__dpaa2_io' was changed at dpaa2_hw_dpio.h:20:1: size of symbol changed from 40 to 16

Error: ABI issue reported for 'abidiff --suppr devtools/libabigail.abignore --no-added-syms --headers-dir1 reference/usr/local/include --headers-dir2 install/usr/local/include reference/dump/librte_bus_fslmc.dump install/dump/librte_bus_fslmc.dump'

---------------

These changes are w.r.t modifications in internal structures and variables. They may be ignored.
> 

> --

> David Marchand
David Marchand March 5, 2020, 9:09 a.m. UTC | #3
On Thu, Mar 5, 2020 at 10:06 AM Hemant Agrawal (OSS)
<hemant.agrawal@oss.nxp.com> wrote:
>

> Hi David,

> > On Mon, Mar 2, 2020 at 10:26 AM Hemant Agrawal

> > <hemant.agrawal@nxp.com> wrote:

> > >

> > > This patch series add various patches for enhancing and fixing NXP

> > > fslmc bus, dpaa bus, and dpaax.

> > >

> > > - the main change is support to allow thread migration across lcores

> > > - improving the multi-process support

> >

> > This series triggers an ABI warning that must be investigated.

> >https://travis-ci.com/ovsrobot/dpdk/jobs/292904119#L2233

>

> [Hemant]

> As per the logs:

>

> Variables changes summary: 1 Removed, 2 Changed, 0 Added variables

> 1 Removed variable:

>   'dpaa2_portal_dqrr per_lcore_dpaa2_held_bufs'    {per_lcore_dpaa2_held_bufs@@DPDK_20.0}

> 2 Changed variables:

>   [C]'dpaa2_io_portal_t dpaa2_io_portal[128]' was changed at dpaa2_hw_dpio.h:40:1: size of symbol changed from 5120 to 2048

>   [C]'dpaa2_io_portal_t per_lcore__dpaa2_io' was changed at dpaa2_hw_dpio.h:20:1: size of symbol changed from 40 to 16

>

> Error: ABI issue reported for 'abidiff --suppr devtools/libabigail.abignore --no-added-syms --headers-dir1 reference/usr/local/include --headers-dir2 install/usr/local/include reference/dump/librte_bus_fslmc.dump install/dump/librte_bus_fslmc.dump'

>

> ---------------

>

> These changes are w.r.t modifications in internal structures and variables. They may be ignored.


The ABI check considers symbol exposed in headers available to final users.
If those are internal, why are the headers public?


-- 
David Marchand
Hemant Agrawal March 5, 2020, 9:19 a.m. UTC | #4
Hi David,

> On Thu, Mar 5, 2020 at 10:06 AM Hemant Agrawal (OSS)

> <hemant.agrawal@oss.nxp.com> wrote:

> >

> > Hi David,

> > > On Mon, Mar 2, 2020 at 10:26 AM Hemant Agrawal

> > > <hemant.agrawal@nxp.com> wrote:

> > > >

> > > > This patch series add various patches for enhancing and fixing NXP

> > > > fslmc bus, dpaa bus, and dpaax.

> > > >

> > > > - the main change is support to allow thread migration across

> > > > lcores

> > > > - improving the multi-process support

> > >

> > > This series triggers an ABI warning that must be investigated.

> > >https://travis-ci.com/ovsrobot/dpdk/jobs/292904119#L2233

> >

> > [Hemant]

> > As per the logs:

> >

> > Variables changes summary: 1 Removed, 2 Changed, 0 Added variables

> > 1 Removed variable:

> >   'dpaa2_portal_dqrr per_lcore_dpaa2_held_bufs'

> {per_lcore_dpaa2_held_bufs@@DPDK_20.0}

> > 2 Changed variables:

> >   [C]'dpaa2_io_portal_t dpaa2_io_portal[128]' was changed at

> dpaa2_hw_dpio.h:40:1: size of symbol changed from 5120 to 2048

> >   [C]'dpaa2_io_portal_t per_lcore__dpaa2_io' was changed at

> > dpaa2_hw_dpio.h:20:1: size of symbol changed from 40 to 16

> >

> > Error: ABI issue reported for 'abidiff --suppr devtools/libabigail.abignore --

> no-added-syms --headers-dir1 reference/usr/local/include --headers-dir2

> install/usr/local/include reference/dump/librte_bus_fslmc.dump

> install/dump/librte_bus_fslmc.dump'

> >

> > ---------------

> >

> > These changes are w.r.t modifications in internal structures and variables.

> They may be ignored.

> 

> The ABI check considers symbol exposed in headers available to final users.

> If those are internal, why are the headers public?

> 


[Hemant] These symbols are not part of any public header files,  but they are part of *.map files to share them between different driver libs i.e bus_fslmc and net_dpaa2 

> 

> --

> David Marchand
David Marchand March 6, 2020, 10:12 a.m. UTC | #5
On Thu, Mar 5, 2020 at 10:19 AM Hemant Agrawal (OSS)
<hemant.agrawal@oss.nxp.com> wrote:
> > On Thu, Mar 5, 2020 at 10:06 AM Hemant Agrawal (OSS)

> > <hemant.agrawal@oss.nxp.com> wrote:

> > >

> > > Hi David,

> > > > On Mon, Mar 2, 2020 at 10:26 AM Hemant Agrawal

> > > > <hemant.agrawal@nxp.com> wrote:

> > > > >

> > > > > This patch series add various patches for enhancing and fixing NXP

> > > > > fslmc bus, dpaa bus, and dpaax.

> > > > >

> > > > > - the main change is support to allow thread migration across

> > > > > lcores

> > > > > - improving the multi-process support

> > > >

> > > > This series triggers an ABI warning that must be investigated.

> > > >https://travis-ci.com/ovsrobot/dpdk/jobs/292904119#L2233

> > >

> > > [Hemant]

> > > As per the logs:

> > >

> > > Variables changes summary: 1 Removed, 2 Changed, 0 Added variables

> > > 1 Removed variable:

> > >   'dpaa2_portal_dqrr per_lcore_dpaa2_held_bufs'

> > {per_lcore_dpaa2_held_bufs@@DPDK_20.0}

> > > 2 Changed variables:

> > >   [C]'dpaa2_io_portal_t dpaa2_io_portal[128]' was changed at

> > dpaa2_hw_dpio.h:40:1: size of symbol changed from 5120 to 2048

> > >   [C]'dpaa2_io_portal_t per_lcore__dpaa2_io' was changed at

> > > dpaa2_hw_dpio.h:20:1: size of symbol changed from 40 to 16

> > >

> > > Error: ABI issue reported for 'abidiff --suppr devtools/libabigail.abignore --

> > no-added-syms --headers-dir1 reference/usr/local/include --headers-dir2

> > install/usr/local/include reference/dump/librte_bus_fslmc.dump

> > install/dump/librte_bus_fslmc.dump'

> > >

> > > ---------------

> > >

> > > These changes are w.r.t modifications in internal structures and variables.

> > They may be ignored.

> >

> > The ABI check considers symbol exposed in headers available to final users.

> > If those are internal, why are the headers public?

> >

>

> [Hemant] These symbols are not part of any public header files,  but they are part of *.map files to share them between different driver libs i.e bus_fslmc and net_dpaa2


I would expect libabigail to skip those symbols, so there is something
I have missed in how --headers-dirX work.


Anyway, all of those symbols in dpaa are part of the driver ABI.
We are still missing a way to mark internal symbols.
Neil had posted a framework for this
http://patchwork.dpdk.org/project/dpdk/list/?series=5004.

In order to get this series passing the checks, I recommend NXP
rebasing Neil scripts (I will help reviewing this part), then mark all
those symbols as internal in its drivers.
Other vendor will convert their drivers later, as there is no need at
the moment.

Thanks.

-- 
David Marchand
Dodji Seketeli March 10, 2020, 10:36 a.m. UTC | #6
Hello,

David Marchand <david.marchand@redhat.com> writes:

> On Thu, Mar 5, 2020 at 10:19 AM Hemant Agrawal (OSS)

> <hemant.agrawal@oss.nxp.com> wrote:

>> > On Thu, Mar 5, 2020 at 10:06 AM Hemant Agrawal (OSS)

>> > <hemant.agrawal@oss.nxp.com> wrote:

>> > >

>> > > Hi David,

>> > > > On Mon, Mar 2, 2020 at 10:26 AM Hemant Agrawal

>> > > > <hemant.agrawal@nxp.com> wrote:

>> > > > >

>> > > > > This patch series add various patches for enhancing and fixing NXP

>> > > > > fslmc bus, dpaa bus, and dpaax.

>> > > > >

>> > > > > - the main change is support to allow thread migration across

>> > > > > lcores

>> > > > > - improving the multi-process support

>> > > >

>> > > > This series triggers an ABI warning that must be investigated.

>> > > >https://travis-ci.com/ovsrobot/dpdk/jobs/292904119#L2233

>> > >

>> > > [Hemant]

>> > > As per the logs:

>> > >

>> > > Variables changes summary: 1 Removed, 2 Changed, 0 Added variables

>> > > 1 Removed variable:

>> > >   'dpaa2_portal_dqrr per_lcore_dpaa2_held_bufs'

>> > {per_lcore_dpaa2_held_bufs@@DPDK_20.0}

>> > > 2 Changed variables:

>> > >   [C]'dpaa2_io_portal_t dpaa2_io_portal[128]' was changed at

>> > dpaa2_hw_dpio.h:40:1: size of symbol changed from 5120 to 2048

>> > >   [C]'dpaa2_io_portal_t per_lcore__dpaa2_io' was changed at

>> > > dpaa2_hw_dpio.h:20:1: size of symbol changed from 40 to 16

>> > >

>> > > Error: ABI issue reported for 'abidiff --suppr devtools/libabigail.abignore --

>> > no-added-syms --headers-dir1 reference/usr/local/include --headers-dir2

>> > install/usr/local/include reference/dump/librte_bus_fslmc.dump

>> > install/dump/librte_bus_fslmc.dump'

>> > >

>> > > ---------------

>> > >

>> > > These changes are w.r.t modifications in internal structures and variables.

>> > They may be ignored.

>> >

>> > The ABI check considers symbol exposed in headers available to final users.

>> > If those are internal, why are the headers public?

>> >

>>

>> [Hemant] These symbols are not part of any public header files, but

>> they are part of *.map files to share them between different driver

>> libs i.e bus_fslmc and net_dpaa2

>

> I would expect libabigail to skip those symbols, so there is something

> I have missed in how --headers-dirX work.


In libabigail speak, we make a difference between *ELF symbols* and
types.

--header-dirX is about telling the tool what the public *types* are.  As
you rightfully implied, types that are defined in files that are not
found in the directories specified by --header-dirX are considered to be
private types and are thus not shown in the ABI change report.

ELF symbols however are a different matter.  Header files don't usually
define ELF symbols, be they variable of function symbols.  Header files
can at most declare variables or functions that would be actually
defined elsewhere in source code, leading to the definition of ELF
variable or function symbols in the final binary.  At this point, we
aren't talking about types anymore, as the ELF format doesn't know what
types (in C or any other language) are.  So --header-dirX don't deal with
ELF symbols.

And from what I understand from the message quoted above, the changes we
are talking about have to do with EFL variable symbols which size have
changed.  So in practise, these are global arrays (exposed by at the
binary level as an ELF variable symbol of a given size) with public
visibility which size have changed.

So my guess would be that if you guys don't want these arrays to be part
the binary interface of this library, they should probably be declared
static at the C level and accessed through some accessor function or
something like that.  At least, that's my humble uninformed opinion.

In the mean time, the tooling can be tought to ignore changes to these
ELF symbols, as you you guys all know already.


> Anyway, all of those symbols in dpaa are part of the driver ABI.

> We are still missing a way to mark internal symbols.

> Neil had posted a framework for this

> http://patchwork.dpdk.org/project/dpdk/list/?series=5004.

>

> In order to get this series passing the checks, I recommend NXP

> rebasing Neil scripts (I will help reviewing this part), then mark all

> those symbols as internal in its drivers.

> Other vendor will convert their drivers later, as there is no need at

> the moment.

>

> Thanks.


Cheers,

-- 
		Dodji
Hemant Agrawal April 7, 2020, 10:25 a.m. UTC | #7
HI Dodji,
> 

> David Marchand <david.marchand@redhat.com> writes:

> 

> > On Thu, Mar 5, 2020 at 10:19 AM Hemant Agrawal (OSS)

> > <hemant.agrawal@oss.nxp.com> wrote:

> >> > On Thu, Mar 5, 2020 at 10:06 AM Hemant Agrawal (OSS)

> >> > <hemant.agrawal@oss.nxp.com> wrote:

> >> > >

> >> > > Hi David,

> >> > > > On Mon, Mar 2, 2020 at 10:26 AM Hemant Agrawal

> >> > > > <hemant.agrawal@nxp.com> wrote:

> >> > > > >

> >> > > > > This patch series add various patches for enhancing and

> >> > > > > fixing NXP fslmc bus, dpaa bus, and dpaax.

> >> > > > >

> >> > > > > - the main change is support to allow thread migration across

> >> > > > > lcores

> >> > > > > - improving the multi-process support

> >> > > >

> >> > > > This series triggers an ABI warning that must be investigated.

> >> > > >https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%

> >> > > >2Ftravis-

> ci.com%2Fovsrobot%2Fdpdk%2Fjobs%2F292904119%23L2233&amp

> >> > >

> >;data=02%7C01%7Chemant.agrawal%40nxp.com%7C91a3230cfd334c28bbd

> b0

> >> > >

> >8d7c4dee590%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6371

> 943

> >> > >

> >33920176015&amp;sdata=%2BViKwS2sNucwLFD9VtvwxOK1huq0g%2B6TfT6

> Fqp

> >> > > >Nyz5w%3D&amp;reserved=0

> >> > >

> >> > > [Hemant]

> >> > > As per the logs:

> >> > >

> >> > > Variables changes summary: 1 Removed, 2 Changed, 0 Added

> >> > > variables

> >> > > 1 Removed variable:

> >> > >   'dpaa2_portal_dqrr per_lcore_dpaa2_held_bufs'

> >> > {per_lcore_dpaa2_held_bufs@@DPDK_20.0}

> >> > > 2 Changed variables:

> >> > >   [C]'dpaa2_io_portal_t dpaa2_io_portal[128]' was changed at

> >> > dpaa2_hw_dpio.h:40:1: size of symbol changed from 5120 to 2048

> >> > >   [C]'dpaa2_io_portal_t per_lcore__dpaa2_io' was changed at

> >> > > dpaa2_hw_dpio.h:20:1: size of symbol changed from 40 to 16

> >> > >

> >> > > Error: ABI issue reported for 'abidiff --suppr

> >> > > devtools/libabigail.abignore --

> >> > no-added-syms --headers-dir1 reference/usr/local/include

> >> > --headers-dir2 install/usr/local/include

> >> > reference/dump/librte_bus_fslmc.dump

> >> > install/dump/librte_bus_fslmc.dump'

> >> > >

> >> > > ---------------

> >> > >

> >> > > These changes are w.r.t modifications in internal structures and

> variables.

> >> > They may be ignored.

> >> >

> >> > The ABI check considers symbol exposed in headers available to final

> users.

> >> > If those are internal, why are the headers public?

> >> >

> >>

> >> [Hemant] These symbols are not part of any public header files, but

> >> they are part of *.map files to share them between different driver

> >> libs i.e bus_fslmc and net_dpaa2

> >

> > I would expect libabigail to skip those symbols, so there is something

> > I have missed in how --headers-dirX work.

> 

> In libabigail speak, we make a difference between *ELF symbols* and types.

> 

> --header-dirX is about telling the tool what the public *types* are.  As you

> rightfully implied, types that are defined in files that are not found in the

> directories specified by --header-dirX are considered to be private types and

> are thus not shown in the ABI change report.

> 

> ELF symbols however are a different matter.  Header files don't usually define

> ELF symbols, be they variable of function symbols.  Header files can at most

> declare variables or functions that would be actually defined elsewhere in

> source code, leading to the definition of ELF variable or function symbols in the

> final binary.  At this point, we aren't talking about types anymore, as the ELF

> format doesn't know what types (in C or any other language) are.  So --header-

> dirX don't deal with ELF symbols.

> 

> And from what I understand from the message quoted above, the changes we

> are talking about have to do with EFL variable symbols which size have

> changed.  So in practise, these are global arrays (exposed by at the binary level

> as an ELF variable symbol of a given size) with public visibility which size have

> changed.

> 

> So my guess would be that if you guys don't want these arrays to be part the

> binary interface of this library, they should probably be declared static at the C

> level and accessed through some accessor function or something like that.  At

> least, that's my humble uninformed opinion.


[Hemant] Actually some of these are in datapath, there is a performance impact of accessing them via function calls. 

> 

> In the mean time, the tooling can be tought to ignore changes to these ELF

> symbols, as you you guys all know already.

> 

[Hemant] will you please help me about adding entry to libagigail.abignore 
I tried doing following, but it is not helping
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -2,10 +2,15 @@
         symbol_version = EXPERIMENTAL
 [suppress_variable]
         symbol_version = EXPERIMENTAL
+       name = per_lcore__dpaa2_io
+       name = dpaa2_io_portal

 ; Explicit ignore for driver-only ABI
 [suppress_type]
         name = rte_cryptodev_ops
+       name = dpaa2_io_portal_t
> 

> > Anyway, all of those symbols in dpaa are part of the driver ABI.

> > We are still missing a way to mark internal symbols.

> > Neil had posted a framework for this

> >

> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatchwo

> rk.dpdk.org%2Fproject%2Fdpdk%2Flist%2F%3Fseries%3D5004&amp;data=02

> %7C01%7Chemant.agrawal%40nxp.com%7C91a3230cfd334c28bbdb08d7c4d

> ee590%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6371943339

> 20186005&amp;sdata=1Is%2BqQwP%2Bn0QVJ2HYK2%2Bx7TJooEvry1sNUUN

> fWMygkM%3D&amp;reserved=0.

> >

> > In order to get this series passing the checks, I recommend NXP

> > rebasing Neil scripts (I will help reviewing this part), then mark all

> > those symbols as internal in its drivers.

> > Other vendor will convert their drivers later, as there is no need at

> > the moment.

> >

[Hemant] I have commented on Neil's series.  It needs more changes in existing code. An approach like __rte_experimental will work better.

> > Thanks.

> 


Regards,
Hemant
Thomas Monjalon April 7, 2020, 12:20 p.m. UTC | #8
07/04/2020 12:25, Hemant Agrawal:
> > In the mean time, the tooling can be tought to ignore changes to these ELF

> > symbols, as you you guys all know already.

> > 

> [Hemant] will you please help me about adding entry to libagigail.abignore 

> I tried doing following, but it is not helping

> 

> --- a/devtools/libabigail.abignore

> +++ b/devtools/libabigail.abignore

> @@ -2,10 +2,15 @@

>          symbol_version = EXPERIMENTAL

>  [suppress_variable]

>          symbol_version = EXPERIMENTAL

> +       name = per_lcore__dpaa2_io

> +       name = dpaa2_io_portal

> 

>  ; Explicit ignore for driver-only ABI

>  [suppress_type]

>          name = rte_cryptodev_ops

> +       name = dpaa2_io_portal_t

> > 

> > > Anyway, all of those symbols in dpaa are part of the driver ABI.

> > > We are still missing a way to mark internal symbols.

> > > Neil had posted a framework for this

> > >

> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatchwo

> > rk.dpdk.org%2Fproject%2Fdpdk%2Flist%2F%3Fseries%3D5004&amp;data=02

> > %7C01%7Chemant.agrawal%40nxp.com%7C91a3230cfd334c28bbdb08d7c4d

> > ee590%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6371943339

> > 20186005&amp;sdata=1Is%2BqQwP%2Bn0QVJ2HYK2%2Bx7TJooEvry1sNUUN

> > fWMygkM%3D&amp;reserved=0.

> > >

> > > In order to get this series passing the checks, I recommend NXP

> > > rebasing Neil scripts (I will help reviewing this part), then mark all

> > > those symbols as internal in its drivers.

> > > Other vendor will convert their drivers later, as there is no need at

> > > the moment.

> > >

> [Hemant] I have commented on Neil's series.

> It needs more changes in existing code.

> An approach like __rte_experimental will work better.


I guess you mean __rte_internal?

Please Hemant don't wait for someone else filling the gap.
If __rte_internal is the right approach, please complete and use it.
Dodji Seketeli April 8, 2020, 7:20 a.m. UTC | #9
Hello Hemant,

Hemant Agrawal <hemant.agrawal@nxp.com> writes:

[...]

>> >> > > [Hemant]

>> >> > > As per the logs:

>> >> > >

>> >> > > Variables changes summary: 1 Removed, 2 Changed, 0 Added

>> >> > > variables

>> >> > > 1 Removed variable:

>> >> > >   'dpaa2_portal_dqrr per_lcore_dpaa2_held_bufs'

>> >> > {per_lcore_dpaa2_held_bufs@@DPDK_20.0}

>> >> > > 2 Changed variables:

>> >> > >   [C]'dpaa2_io_portal_t dpaa2_io_portal[128]' was changed at

>> >> > dpaa2_hw_dpio.h:40:1: size of symbol changed from 5120 to 2048

>> >> > >   [C]'dpaa2_io_portal_t per_lcore__dpaa2_io' was changed at

>> >> > > dpaa2_hw_dpio.h:20:1: size of symbol changed from 40 to 16

>> >> > >

>> >> > > Error: ABI issue reported for 'abidiff --suppr

>> >> > > devtools/libabigail.abignore --

>> >> > no-added-syms --headers-dir1 reference/usr/local/include

>> >> > --headers-dir2 install/usr/local/include

>> >> > reference/dump/librte_bus_fslmc.dump

>> >> > install/dump/librte_bus_fslmc.dump'


[...]

>> In the mean time, the tooling can be tought to ignore changes to these ELF

>> symbols, as you you guys all know already.

>> 

> [Hemant] will you please help me about adding entry to libagigail.abignore 

> I tried doing following, but it is not helping

> --- a/devtools/libabigail.abignore

> +++ b/devtools/libabigail.abignore

> @@ -2,10 +2,15 @@

>          symbol_version = EXPERIMENTAL

>  [suppress_variable]

>          symbol_version = EXPERIMENTAL

> +       name = per_lcore__dpaa2_io

> +       name = dpaa2_io_portal

>

>  ; Explicit ignore for driver-only ABI

>  [suppress_type]

>          name = rte_cryptodev_ops

> +       name = dpaa2_io_portal_t


So, I understand you want the tooling to ignore changes to the global
arrays dpaa2_io_portal and per_lcore__dpaa2_io, right?

If that is correct, then here are the entries you should add to the
libabigail.abignore file (please make sure the comments I have added
there is accurate):

        [suppress_variable]
          # This global variable is exported by the binary but is not part of
          # the logical ABI.  In a perfect world, that variable should not be
          # global, and we should access it via an accessor function.  We do
          # that right now because of performance concerns.
          name = dpaa2_io_portal

        [suppress_variable]
          # This global variable is exported by the binary but is not part of
          # the logical ABI.  In a perfect world, that variable should not be
          # global, and we should access it via an accessor function.  We do
          # that right now because of performance concerns.
          name = per_lcore__dpaa2_io

I hope this helps.

Cheers,

-- 
		Dodji
Dodji Seketeli April 8, 2020, 7:52 a.m. UTC | #10
Hello Thomas, Hemant,

Thomas Monjalon <thomas@monjalon.net> writes:

> 07/04/2020 12:25, Hemant Agrawal:


[...]

>> [Hemant] I have commented on Neil's series.

>> It needs more changes in existing code.

>> An approach like __rte_experimental will work better.

>

> I guess you mean __rte_internal?

>

> Please Hemant don't wait for someone else filling the gap.

> If __rte_internal is the right approach, please complete and use it.


Just so that I understand, is __rte_internal an ELF version that the
symbols per_lcore_dpaa2_held_bufs, dpaa2_io_portal and
per_lcore__dpaa2_io should have in the binary?

If that is the case, then it seems to me that the __rte_internal
approach that you are suggesting would be a much better approach that
the one I replied to Hemant about below.

I didn't mean to tell Hemant what approach he should take :-) I was just
trying to help him get the syntax of a libabigail suppression
specification right.

Sorry for the confusion I might have induced.

Dodji Seketeli <dseketel@redhat.com> writes:

> Hello Hemant,

>

> Hemant Agrawal <hemant.agrawal@nxp.com> writes:

>

> [...]

>

>>> >> > > [Hemant]

>>> >> > > As per the logs:

>>> >> > >

>>> >> > > Variables changes summary: 1 Removed, 2 Changed, 0 Added

>>> >> > > variables

>>> >> > > 1 Removed variable:

>>> >> > >   'dpaa2_portal_dqrr per_lcore_dpaa2_held_bufs'

>>> >> > {per_lcore_dpaa2_held_bufs@@DPDK_20.0}

>>> >> > > 2 Changed variables:

>>> >> > >   [C]'dpaa2_io_portal_t dpaa2_io_portal[128]' was changed at

>>> >> > dpaa2_hw_dpio.h:40:1: size of symbol changed from 5120 to 2048

>>> >> > >   [C]'dpaa2_io_portal_t per_lcore__dpaa2_io' was changed at

>>> >> > > dpaa2_hw_dpio.h:20:1: size of symbol changed from 40 to 16

>>> >> > >

>>> >> > > Error: ABI issue reported for 'abidiff --suppr

>>> >> > > devtools/libabigail.abignore --

>>> >> > no-added-syms --headers-dir1 reference/usr/local/include

>>> >> > --headers-dir2 install/usr/local/include

>>> >> > reference/dump/librte_bus_fslmc.dump

>>> >> > install/dump/librte_bus_fslmc.dump'

>

> [...]

>

>>> In the mean time, the tooling can be tought to ignore changes to these ELF

>>> symbols, as you you guys all know already.

>>> 

>> [Hemant] will you please help me about adding entry to libagigail.abignore 

>> I tried doing following, but it is not helping

>> --- a/devtools/libabigail.abignore

>> +++ b/devtools/libabigail.abignore

>> @@ -2,10 +2,15 @@

>>          symbol_version = EXPERIMENTAL

>>  [suppress_variable]

>>          symbol_version = EXPERIMENTAL

>> +       name = per_lcore__dpaa2_io

>> +       name = dpaa2_io_portal

>>

>>  ; Explicit ignore for driver-only ABI

>>  [suppress_type]

>>          name = rte_cryptodev_ops

>> +       name = dpaa2_io_portal_t

>

> So, I understand you want the tooling to ignore changes to the global

> arrays dpaa2_io_portal and per_lcore__dpaa2_io, right?

>

> If that is correct, then here are the entries you should add to the

> libabigail.abignore file (please make sure the comments I have added

> there is accurate):

>

>         [suppress_variable]

>           # This global variable is exported by the binary but is not part of

>           # the logical ABI.  In a perfect world, that variable should not be

>           # global, and we should access it via an accessor function.  We do

>           # that right now because of performance concerns.

>           name = dpaa2_io_portal

>

>         [suppress_variable]

>           # This global variable is exported by the binary but is not part of

>           # the logical ABI.  In a perfect world, that variable should not be

>           # global, and we should access it via an accessor function.  We do

>           # that right now because of performance concerns.

>           name = per_lcore__dpaa2_io

>

> I hope this helps.

>

> Cheers,


-- 
		Dodji
Thomas Monjalon April 8, 2020, 12:35 p.m. UTC | #11
08/04/2020 09:52, Dodji Seketeli:
> Hello Thomas, Hemant,

> 

> Thomas Monjalon <thomas@monjalon.net> writes:

> > 07/04/2020 12:25, Hemant Agrawal:

> 

> [...]

> 

> >> [Hemant] I have commented on Neil's series.

> >> It needs more changes in existing code.

> >> An approach like __rte_experimental will work better.

> >

> > I guess you mean __rte_internal?

> >

> > Please Hemant don't wait for someone else filling the gap.

> > If __rte_internal is the right approach, please complete and use it.

> 

> Just so that I understand, is __rte_internal an ELF version that the

> symbols per_lcore_dpaa2_held_bufs, dpaa2_io_portal and

> per_lcore__dpaa2_io should have in the binary?


Correct

> If that is the case, then it seems to me that the __rte_internal

> approach that you are suggesting would be a much better approach that

> the one I replied to Hemant about below.


Yes I think we all agree, just waiting for the patch to be ready.

> I didn't mean to tell Hemant what approach he should take :-) I was just

> trying to help him get the syntax of a libabigail suppression

> specification right.

> 

> Sorry for the confusion I might have induced.


No problem, thanks for the help.