mbox series

[v2,0/7] Removal of PCI bus ABIs

Message ID 20210918022443.12719-1-chenbo.xia@intel.com (mailing list archive)
Headers
Series Removal of PCI bus ABIs |

Message

Chenbo Xia Sept. 18, 2021, 2:24 a.m. UTC 
  As announced in the deprecation notice, most ABIs in PCI bus will be removed.

As there exist some applications that want to access PCI memory resource,
two new APIs are defined in Patch 1 and corresponding changes are applied
to testpmd in Patch 2.

Patch 3-4 clean up the unnecessary usage of PCI bus header in examples.

Patch 5-6 clean up the unused PCI related structure in kni library and related
tests and examples.

Patch 7 finally removes most of ABIs in PCI bus.

---
v2: 
 - Add check on call of port_id_pci_reg_write (Xiaoyun)
 - Combine two clean-up patches in test and example, and backport (David)

Chenbo Xia (7):
  bus/pci: add new memory resource access APIs
  app/testpmd: use PCI memory resource access APIs
  examples/ethtool: use PCI library API to get PCI address
  examples/kni: remove unused PCI bus header
  kni: remove unused PCI info from test and example
  kni: replace unused variable definition with reserved bytes
  bus/pci: remove ABIs in PCI bus

 app/test-pmd/config.c                         |  50 +--
 app/test-pmd/testpmd.h                        |  54 +--
 app/test/test_kni.c                           |  57 ---
 app/test/virtual_pmd.c                        |   2 +-
 doc/guides/rel_notes/release_21_11.rst        |   8 +
 drivers/baseband/acc100/rte_acc100_pmd.c      |   2 +-
 .../fpga_5gnr_fec/rte_fpga_5gnr_fec.c         |   2 +-
 drivers/baseband/fpga_lte_fec/fpga_lte_fec.c  |   2 +-
 drivers/bus/pci/bsd/pci.c                     |   1 -
 drivers/bus/pci/linux/pci.c                   |   1 -
 drivers/bus/pci/linux/pci_uio.c               |   1 -
 drivers/bus/pci/linux/pci_vfio.c              |   1 -
 drivers/bus/pci/meson.build                   |   4 +
 drivers/bus/pci/pci_common.c                  |  78 ++++
 drivers/bus/pci/pci_common_uio.c              |   1 -
 drivers/bus/pci/pci_driver.h                  | 402 ++++++++++++++++++
 drivers/bus/pci/pci_params.c                  |   1 -
 drivers/bus/pci/private.h                     |   3 +-
 drivers/bus/pci/rte_bus_pci.h                 | 387 ++---------------
 drivers/bus/pci/version.map                   |  28 +-
 drivers/common/cnxk/roc_platform.h            |   2 +-
 drivers/common/mlx5/linux/mlx5_common_verbs.c |   2 +-
 drivers/common/mlx5/mlx5_common_pci.c         |   2 +-
 drivers/common/octeontx2/otx2_dev.h           |   2 +-
 drivers/common/octeontx2/otx2_sec_idev.c      |   2 +-
 drivers/common/qat/qat_device.h               |   2 +-
 drivers/common/qat/qat_qp.c                   |   2 +-
 drivers/common/sfc_efx/sfc_efx.h              |   2 +-
 drivers/compress/mlx5/mlx5_compress.c         |   2 +-
 drivers/compress/octeontx/otx_zip.h           |   2 +-
 drivers/compress/qat/qat_comp.c               |   2 +-
 drivers/crypto/ccp/ccp_dev.h                  |   2 +-
 drivers/crypto/ccp/ccp_pci.h                  |   2 +-
 drivers/crypto/ccp/rte_ccp_pmd.c              |   2 +-
 drivers/crypto/cnxk/cn10k_cryptodev.c         |   2 +-
 drivers/crypto/cnxk/cn9k_cryptodev.c          |   2 +-
 drivers/crypto/mlx5/mlx5_crypto.c             |   2 +-
 drivers/crypto/nitrox/nitrox_device.h         |   2 +-
 drivers/crypto/octeontx/otx_cryptodev.c       |   2 +-
 drivers/crypto/octeontx/otx_cryptodev_ops.c   |   2 +-
 drivers/crypto/octeontx2/otx2_cryptodev.c     |   2 +-
 drivers/crypto/qat/qat_sym.c                  |   2 +-
 drivers/crypto/qat/qat_sym_pmd.c              |   2 +-
 drivers/crypto/virtio/virtio_cryptodev.c      |   2 +-
 drivers/crypto/virtio/virtio_pci.h            |   2 +-
 drivers/event/dlb2/pf/dlb2_main.h             |   2 +-
 drivers/event/dlb2/pf/dlb2_pf.c               |   2 +-
 drivers/event/octeontx/ssovf_probe.c          |   2 +-
 drivers/event/octeontx/timvf_probe.c          |   2 +-
 drivers/event/octeontx2/otx2_evdev.c          |   2 +-
 drivers/mempool/cnxk/cnxk_mempool.c           |   2 +-
 drivers/mempool/octeontx/octeontx_fpavf.c     |   2 +-
 drivers/mempool/octeontx2/otx2_mempool.c      |   2 +-
 drivers/mempool/octeontx2/otx2_mempool.h      |   2 +-
 drivers/mempool/octeontx2/otx2_mempool_irq.c  |   2 +-
 drivers/meson.build                           |   4 +
 drivers/net/ark/ark_ethdev.c                  |   2 +-
 drivers/net/avp/avp_ethdev.c                  |   2 +-
 drivers/net/bnx2x/bnx2x.h                     |   2 +-
 drivers/net/bnxt/bnxt.h                       |   2 +-
 drivers/net/bonding/rte_eth_bond_args.c       |   2 +-
 drivers/net/cxgbe/base/adapter.h              |   2 +-
 drivers/net/cxgbe/cxgbe_ethdev.c              |   2 +-
 drivers/net/e1000/em_ethdev.c                 |   2 +-
 drivers/net/e1000/em_rxtx.c                   |   2 +-
 drivers/net/e1000/igb_ethdev.c                |   2 +-
 drivers/net/e1000/igb_pf.c                    |   2 +-
 drivers/net/ena/ena_ethdev.h                  |   2 +-
 drivers/net/enic/base/vnic_dev.h              |   2 +-
 drivers/net/enic/enic_ethdev.c                |   2 +-
 drivers/net/enic/enic_main.c                  |   2 +-
 drivers/net/enic/enic_vf_representor.c        |   2 +-
 drivers/net/hinic/base/hinic_pmd_hwdev.c      |   2 +-
 drivers/net/hinic/base/hinic_pmd_hwif.c       |   2 +-
 drivers/net/hinic/base/hinic_pmd_nicio.c      |   2 +-
 drivers/net/hinic/hinic_pmd_ethdev.c          |   2 +-
 drivers/net/hns3/hns3_ethdev.c                |   2 +-
 drivers/net/hns3/hns3_rxtx.c                  |   2 +-
 drivers/net/i40e/i40e_ethdev.c                |   2 +-
 drivers/net/i40e/i40e_ethdev_vf.c             |   2 +-
 drivers/net/i40e/i40e_vf_representor.c        |   2 +-
 drivers/net/igc/igc_ethdev.c                  |   2 +-
 drivers/net/ionic/ionic.h                     |   2 +-
 drivers/net/ionic/ionic_ethdev.c              |   2 +-
 drivers/net/ipn3ke/ipn3ke_ethdev.c            |   2 +-
 drivers/net/ipn3ke/ipn3ke_representor.c       |   2 +-
 drivers/net/ipn3ke/ipn3ke_tm.c                |   2 +-
 drivers/net/ixgbe/ixgbe_ethdev.c              |   2 +-
 drivers/net/ixgbe/ixgbe_ethdev.h              |   2 +-
 drivers/net/mlx4/mlx4_ethdev.c                |   2 +-
 drivers/net/mlx5/linux/mlx5_ethdev_os.c       |   2 +-
 drivers/net/mlx5/linux/mlx5_os.c              |   2 +-
 drivers/net/mlx5/mlx5.c                       |   2 +-
 drivers/net/mlx5/mlx5_ethdev.c                |   2 +-
 drivers/net/mlx5/mlx5_txq.c                   |   2 +-
 drivers/net/netvsc/hn_vf.c                    |   2 +-
 drivers/net/octeontx/base/octeontx_pkivf.c    |   2 +-
 drivers/net/octeontx/base/octeontx_pkovf.c    |   2 +-
 drivers/net/octeontx2/otx2_ethdev_irq.c       |   2 +-
 drivers/net/qede/base/bcm_osal.h              |   2 +-
 drivers/net/sfc/sfc.h                         |   2 +-
 drivers/net/sfc/sfc_ethdev.c                  |   2 +-
 drivers/net/sfc/sfc_sriov.c                   |   2 +-
 drivers/net/thunderx/nicvf_ethdev.c           |   2 +-
 drivers/net/txgbe/txgbe_ethdev.h              |   2 +-
 drivers/net/txgbe/txgbe_flow.c                |   2 +-
 drivers/net/txgbe/txgbe_pf.c                  |   2 +-
 drivers/net/virtio/virtio_pci.h               |   2 +-
 drivers/net/virtio/virtio_pci_ethdev.c        |   2 +-
 drivers/net/vmxnet3/vmxnet3_ethdev.c          |   2 +-
 drivers/raw/cnxk_bphy/cnxk_bphy.c             |   2 +-
 drivers/raw/cnxk_bphy/cnxk_bphy_cgx.c         |   2 +-
 drivers/raw/cnxk_bphy/cnxk_bphy_irq.c         |   2 +-
 drivers/raw/cnxk_bphy/cnxk_bphy_irq.h         |   2 +-
 drivers/raw/ifpga/ifpga_rawdev.c              |   2 +-
 drivers/raw/ifpga/rte_pmd_ifpga.c             |   2 +-
 drivers/raw/ioat/idxd_pci.c                   |   2 +-
 drivers/raw/ioat/ioat_rawdev.c                |   2 +-
 drivers/raw/ntb/ntb.c                         |   2 +-
 drivers/raw/ntb/ntb_hw_intel.c                |   2 +-
 drivers/raw/octeontx2_dma/otx2_dpi_rawdev.c   |   2 +-
 drivers/raw/octeontx2_ep/otx2_ep_enqdeq.c     |   2 +-
 drivers/raw/octeontx2_ep/otx2_ep_rawdev.c     |   2 +-
 drivers/regex/mlx5/mlx5_regex.c               |   2 +-
 drivers/regex/mlx5/mlx5_regex_fastpath.c      |   2 +-
 drivers/vdpa/ifc/base/ifcvf_osdep.h           |   2 +-
 drivers/vdpa/ifc/ifcvf_vdpa.c                 |   2 +-
 drivers/vdpa/mlx5/mlx5_vdpa.c                 |   2 +-
 examples/ethtool/lib/rte_ethtool.c            |  14 +-
 examples/ethtool/meson.build                  |   2 +-
 examples/ip_pipeline/kni.c                    |  16 -
 examples/kni/main.c                           |   1 -
 lib/ethdev/ethdev_pci.h                       |   2 +-
 lib/eventdev/eventdev_pmd_pci.h               |   2 +-
 lib/kni/rte_kni.h                             |   4 +-
 135 files changed, 711 insertions(+), 633 deletions(-)
 create mode 100644 drivers/bus/pci/pci_driver.h

Comments

Chenbo Xia Sept. 29, 2021, 7:38 a.m. UTC | #1 
Gentle ping for comments..

@David, could you help me understand what is the compile error in Fedora 31?
DPDK_compile_spdk failure is expected as the header name for SPDK is changed,
I am not sure if it's the same error...

Thanks,
Chenbo

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Chenbo Xia
> Sent: Saturday, September 18, 2021 10:25 AM
> To: dev@dpdk.org; david.marchand@redhat.com
> Subject: [dpdk-dev] [PATCH v2 0/7] Removal of PCI bus ABIs
> 
> As announced in the deprecation notice, most ABIs in PCI bus will be removed.
> 
> As there exist some applications that want to access PCI memory resource,
> two new APIs are defined in Patch 1 and corresponding changes are applied
> to testpmd in Patch 2.
> 
> Patch 3-4 clean up the unnecessary usage of PCI bus header in examples.
> 
> Patch 5-6 clean up the unused PCI related structure in kni library and related
> tests and examples.
> 
> Patch 7 finally removes most of ABIs in PCI bus.
> 
> ---
> v2:
>  - Add check on call of port_id_pci_reg_write (Xiaoyun)
>  - Combine two clean-up patches in test and example, and backport (David)
> 
> Chenbo Xia (7):
>   bus/pci: add new memory resource access APIs
>   app/testpmd: use PCI memory resource access APIs
>   examples/ethtool: use PCI library API to get PCI address
>   examples/kni: remove unused PCI bus header
>   kni: remove unused PCI info from test and example
>   kni: replace unused variable definition with reserved bytes
>   bus/pci: remove ABIs in PCI bus
> 
>  app/test-pmd/config.c                         |  50 +--
>  app/test-pmd/testpmd.h                        |  54 +--
>  app/test/test_kni.c                           |  57 ---
>  app/test/virtual_pmd.c                        |   2 +-
>  doc/guides/rel_notes/release_21_11.rst        |   8 +
>  drivers/baseband/acc100/rte_acc100_pmd.c      |   2 +-
>  .../fpga_5gnr_fec/rte_fpga_5gnr_fec.c         |   2 +-
>  drivers/baseband/fpga_lte_fec/fpga_lte_fec.c  |   2 +-
>  drivers/bus/pci/bsd/pci.c                     |   1 -
>  drivers/bus/pci/linux/pci.c                   |   1 -
>  drivers/bus/pci/linux/pci_uio.c               |   1 -
>  drivers/bus/pci/linux/pci_vfio.c              |   1 -
>  drivers/bus/pci/meson.build                   |   4 +
>  drivers/bus/pci/pci_common.c                  |  78 ++++
>  drivers/bus/pci/pci_common_uio.c              |   1 -
>  drivers/bus/pci/pci_driver.h                  | 402 ++++++++++++++++++
>  drivers/bus/pci/pci_params.c                  |   1 -
>  drivers/bus/pci/private.h                     |   3 +-
>  drivers/bus/pci/rte_bus_pci.h                 | 387 ++---------------
>  drivers/bus/pci/version.map                   |  28 +-
>  drivers/common/cnxk/roc_platform.h            |   2 +-
>  drivers/common/mlx5/linux/mlx5_common_verbs.c |   2 +-
>  drivers/common/mlx5/mlx5_common_pci.c         |   2 +-
>  drivers/common/octeontx2/otx2_dev.h           |   2 +-
>  drivers/common/octeontx2/otx2_sec_idev.c      |   2 +-
>  drivers/common/qat/qat_device.h               |   2 +-
>  drivers/common/qat/qat_qp.c                   |   2 +-
>  drivers/common/sfc_efx/sfc_efx.h              |   2 +-
>  drivers/compress/mlx5/mlx5_compress.c         |   2 +-
>  drivers/compress/octeontx/otx_zip.h           |   2 +-
>  drivers/compress/qat/qat_comp.c               |   2 +-
>  drivers/crypto/ccp/ccp_dev.h                  |   2 +-
>  drivers/crypto/ccp/ccp_pci.h                  |   2 +-
>  drivers/crypto/ccp/rte_ccp_pmd.c              |   2 +-
>  drivers/crypto/cnxk/cn10k_cryptodev.c         |   2 +-
>  drivers/crypto/cnxk/cn9k_cryptodev.c          |   2 +-
>  drivers/crypto/mlx5/mlx5_crypto.c             |   2 +-
>  drivers/crypto/nitrox/nitrox_device.h         |   2 +-
>  drivers/crypto/octeontx/otx_cryptodev.c       |   2 +-
>  drivers/crypto/octeontx/otx_cryptodev_ops.c   |   2 +-
>  drivers/crypto/octeontx2/otx2_cryptodev.c     |   2 +-
>  drivers/crypto/qat/qat_sym.c                  |   2 +-
>  drivers/crypto/qat/qat_sym_pmd.c              |   2 +-
>  drivers/crypto/virtio/virtio_cryptodev.c      |   2 +-
>  drivers/crypto/virtio/virtio_pci.h            |   2 +-
>  drivers/event/dlb2/pf/dlb2_main.h             |   2 +-
>  drivers/event/dlb2/pf/dlb2_pf.c               |   2 +-
>  drivers/event/octeontx/ssovf_probe.c          |   2 +-
>  drivers/event/octeontx/timvf_probe.c          |   2 +-
>  drivers/event/octeontx2/otx2_evdev.c          |   2 +-
>  drivers/mempool/cnxk/cnxk_mempool.c           |   2 +-
>  drivers/mempool/octeontx/octeontx_fpavf.c     |   2 +-
>  drivers/mempool/octeontx2/otx2_mempool.c      |   2 +-
>  drivers/mempool/octeontx2/otx2_mempool.h      |   2 +-
>  drivers/mempool/octeontx2/otx2_mempool_irq.c  |   2 +-
>  drivers/meson.build                           |   4 +
>  drivers/net/ark/ark_ethdev.c                  |   2 +-
>  drivers/net/avp/avp_ethdev.c                  |   2 +-
>  drivers/net/bnx2x/bnx2x.h                     |   2 +-
>  drivers/net/bnxt/bnxt.h                       |   2 +-
>  drivers/net/bonding/rte_eth_bond_args.c       |   2 +-
>  drivers/net/cxgbe/base/adapter.h              |   2 +-
>  drivers/net/cxgbe/cxgbe_ethdev.c              |   2 +-
>  drivers/net/e1000/em_ethdev.c                 |   2 +-
>  drivers/net/e1000/em_rxtx.c                   |   2 +-
>  drivers/net/e1000/igb_ethdev.c                |   2 +-
>  drivers/net/e1000/igb_pf.c                    |   2 +-
>  drivers/net/ena/ena_ethdev.h                  |   2 +-
>  drivers/net/enic/base/vnic_dev.h              |   2 +-
>  drivers/net/enic/enic_ethdev.c                |   2 +-
>  drivers/net/enic/enic_main.c                  |   2 +-
>  drivers/net/enic/enic_vf_representor.c        |   2 +-
>  drivers/net/hinic/base/hinic_pmd_hwdev.c      |   2 +-
>  drivers/net/hinic/base/hinic_pmd_hwif.c       |   2 +-
>  drivers/net/hinic/base/hinic_pmd_nicio.c      |   2 +-
>  drivers/net/hinic/hinic_pmd_ethdev.c          |   2 +-
>  drivers/net/hns3/hns3_ethdev.c                |   2 +-
>  drivers/net/hns3/hns3_rxtx.c                  |   2 +-
>  drivers/net/i40e/i40e_ethdev.c                |   2 +-
>  drivers/net/i40e/i40e_ethdev_vf.c             |   2 +-
>  drivers/net/i40e/i40e_vf_representor.c        |   2 +-
>  drivers/net/igc/igc_ethdev.c                  |   2 +-
>  drivers/net/ionic/ionic.h                     |   2 +-
>  drivers/net/ionic/ionic_ethdev.c              |   2 +-
>  drivers/net/ipn3ke/ipn3ke_ethdev.c            |   2 +-
>  drivers/net/ipn3ke/ipn3ke_representor.c       |   2 +-
>  drivers/net/ipn3ke/ipn3ke_tm.c                |   2 +-
>  drivers/net/ixgbe/ixgbe_ethdev.c              |   2 +-
>  drivers/net/ixgbe/ixgbe_ethdev.h              |   2 +-
>  drivers/net/mlx4/mlx4_ethdev.c                |   2 +-
>  drivers/net/mlx5/linux/mlx5_ethdev_os.c       |   2 +-
>  drivers/net/mlx5/linux/mlx5_os.c              |   2 +-
>  drivers/net/mlx5/mlx5.c                       |   2 +-
>  drivers/net/mlx5/mlx5_ethdev.c                |   2 +-
>  drivers/net/mlx5/mlx5_txq.c                   |   2 +-
>  drivers/net/netvsc/hn_vf.c                    |   2 +-
>  drivers/net/octeontx/base/octeontx_pkivf.c    |   2 +-
>  drivers/net/octeontx/base/octeontx_pkovf.c    |   2 +-
>  drivers/net/octeontx2/otx2_ethdev_irq.c       |   2 +-
>  drivers/net/qede/base/bcm_osal.h              |   2 +-
>  drivers/net/sfc/sfc.h                         |   2 +-
>  drivers/net/sfc/sfc_ethdev.c                  |   2 +-
>  drivers/net/sfc/sfc_sriov.c                   |   2 +-
>  drivers/net/thunderx/nicvf_ethdev.c           |   2 +-
>  drivers/net/txgbe/txgbe_ethdev.h              |   2 +-
>  drivers/net/txgbe/txgbe_flow.c                |   2 +-
>  drivers/net/txgbe/txgbe_pf.c                  |   2 +-
>  drivers/net/virtio/virtio_pci.h               |   2 +-
>  drivers/net/virtio/virtio_pci_ethdev.c        |   2 +-
>  drivers/net/vmxnet3/vmxnet3_ethdev.c          |   2 +-
>  drivers/raw/cnxk_bphy/cnxk_bphy.c             |   2 +-
>  drivers/raw/cnxk_bphy/cnxk_bphy_cgx.c         |   2 +-
>  drivers/raw/cnxk_bphy/cnxk_bphy_irq.c         |   2 +-
>  drivers/raw/cnxk_bphy/cnxk_bphy_irq.h         |   2 +-
>  drivers/raw/ifpga/ifpga_rawdev.c              |   2 +-
>  drivers/raw/ifpga/rte_pmd_ifpga.c             |   2 +-
>  drivers/raw/ioat/idxd_pci.c                   |   2 +-
>  drivers/raw/ioat/ioat_rawdev.c                |   2 +-
>  drivers/raw/ntb/ntb.c                         |   2 +-
>  drivers/raw/ntb/ntb_hw_intel.c                |   2 +-
>  drivers/raw/octeontx2_dma/otx2_dpi_rawdev.c   |   2 +-
>  drivers/raw/octeontx2_ep/otx2_ep_enqdeq.c     |   2 +-
>  drivers/raw/octeontx2_ep/otx2_ep_rawdev.c     |   2 +-
>  drivers/regex/mlx5/mlx5_regex.c               |   2 +-
>  drivers/regex/mlx5/mlx5_regex_fastpath.c      |   2 +-
>  drivers/vdpa/ifc/base/ifcvf_osdep.h           |   2 +-
>  drivers/vdpa/ifc/ifcvf_vdpa.c                 |   2 +-
>  drivers/vdpa/mlx5/mlx5_vdpa.c                 |   2 +-
>  examples/ethtool/lib/rte_ethtool.c            |  14 +-
>  examples/ethtool/meson.build                  |   2 +-
>  examples/ip_pipeline/kni.c                    |  16 -
>  examples/kni/main.c                           |   1 -
>  lib/ethdev/ethdev_pci.h                       |   2 +-
>  lib/eventdev/eventdev_pmd_pci.h               |   2 +-
>  lib/kni/rte_kni.h                             |   4 +-
>  135 files changed, 711 insertions(+), 633 deletions(-)
>  create mode 100644 drivers/bus/pci/pci_driver.h
> 
> --
> 2.17.1
David Marchand Sept. 30, 2021, 8:45 a.m. UTC | #2 
Hello Chenbo,

On Wed, Sep 29, 2021 at 9:38 AM Xia, Chenbo <chenbo.xia@intel.com> wrote:
>
> Gentle ping for comments..

Sorry, I'll try to look at it.

>
> @David, could you help me understand what is the compile error in Fedora 31?
> DPDK_compile_spdk failure is expected as the header name for SPDK is changed,
> I am not sure if it's the same error...

The error log is odd (no compilation "backtrace").
You'll need to test spdk manually I guess.
David Marchand Oct. 4, 2021, 1:37 p.m. UTC | #3 
On Thu, Sep 30, 2021 at 10:45 AM David Marchand
<david.marchand@redhat.com> wrote:
> On Wed, Sep 29, 2021 at 9:38 AM Xia, Chenbo <chenbo.xia@intel.com> wrote:
> > @David, could you help me understand what is the compile error in Fedora 31?
> > DPDK_compile_spdk failure is expected as the header name for SPDK is changed,
> > I am not sure if it's the same error...
>
> The error log is odd (no compilation "backtrace").
> You'll need to test spdk manually I guess.

Tried your series with SPDK (w/o and w/ enable_driver_sdk).
I think the same, and the error is likely due to the file rename.

$ make
  CC lib/env_dpdk/env.o
In file included from env.c:39:0:
env_internal.h:64:25: error: field ‘driver’ has incomplete type
  struct rte_pci_driver  driver;
                         ^
env_internal.h:75:59: warning: ‘struct rte_pci_device’ declared inside
parameter list [enabled by default]
 int pci_device_init(struct rte_pci_driver *driver, struct
rte_pci_device *device);
                                                           ^
env_internal.h:75:59: warning: its scope is only this definition or
declaration, which is probably not what you want [enabled by default]
env_internal.h:76:28: warning: ‘struct rte_pci_device’ declared inside
parameter list [enabled by default]
 int pci_device_fini(struct rte_pci_device *device);
                            ^
env_internal.h:89:38: warning: ‘struct rte_pci_device’ declared inside
parameter list [enabled by default]
 void vtophys_pci_device_added(struct rte_pci_device *pci_device);
                                      ^
env_internal.h:96:40: warning: ‘struct rte_pci_device’ declared inside
parameter list [enabled by default]
 void vtophys_pci_device_removed(struct rte_pci_device *pci_device);
                                        ^
make[2]: *** [env.o] Error 1
make[1]: *** [env_dpdk] Error 2
make: *** [lib] Error 2



So basically, SPDK needs some updates since it has its own pci drivers.
I copied some SPDK folks for info.

*Disclaimer* I only checked it links fine against my 21.11 dpdk env,
and did not test the other cases:

diff --git a/dpdkbuild/Makefile b/dpdkbuild/Makefile
index d51b1a6e5..0e666735d 100644
--- a/dpdkbuild/Makefile
+++ b/dpdkbuild/Makefile
@@ -166,6 +166,7 @@ all: $(SPDK_ROOT_DIR)/dpdk/build-tmp
 $(SPDK_ROOT_DIR)/dpdk/build-tmp: $(SPDK_ROOT_DIR)/mk/cc.mk
$(SPDK_ROOT_DIR)/include/spdk/config.h
        $(Q)rm -rf $(SPDK_ROOT_DIR)/dpdk/build $(SPDK_ROOT_DIR)/dpdk/build-tmp
        $(Q)cd "$(SPDK_ROOT_DIR)/dpdk"; CC="$(SUB_CC)" meson
--prefix="$(MESON_PREFIX)" --libdir lib -Dc_args="$(DPDK_CFLAGS)"
-Dc_link_args="$(DPDK_LDFLAGS)" $(DPDK_OPTS)
-Ddisable_drivers="$(shell echo $(DPDK_DISABLED_DRVERS) | sed -E "s/
+/,/g")" build-tmp
+       $(Q)! meson configure build-tmp | grep -qw enable_driver_sdk
|| meson configure build-tmp -Denable_driver_sdk=true
        $(Q)sed $(SED_INPLACE_FLAG) 's/#define RTE_EAL_PMD_PATH
.*/#define RTE_EAL_PMD_PATH ""/g'
$(SPDK_ROOT_DIR)/dpdk/build-tmp/rte_build_config.h
        $(Q) \
        # TODO Meson build adds libbsd dependency when it's available.
This means any app will be \
diff --git a/lib/env_dpdk/env.mk b/lib/env_dpdk/env.mk
index cc7db8aab..e24c6942f 100644bits with an embedded dpdk
--- a/lib/env_dpdk/env.mk
+++ b/lib/env_dpdk/env.mk
@@ -172,6 +172,12 @@ DPDK_PRIVATE_LINKER_ARGS += -lnuma
 endif
 endif

+ifneq (,$(wildcard $(DPDK_INC_DIR)/rte_build_config.h))
+ifneq (,$(shell grep -e "define RTE_HAS_LIBARCHIVE 1"
$(DPDK_INC_DIR)/rte_build_config.h))
+DPDK_PRIVATE_LINKER_ARGS += -larchive
+endif
+endif
+
 ifeq ($(OS),Linux)
 DPDK_PRIVATE_LINKER_ARGS += -ldl
 endif
diff --git a/lib/env_dpdk/env_internal.h b/lib/env_dpdk/env_internal.h
index 2303f432c..24b377545 100644
--- a/lib/env_dpdk/env_internal.h
+++ b/lib/env_dpdk/env_internal.h
@@ -43,13 +43,18 @@
 #include <rte_eal.h>
 #include <rte_bus.h>
 #include <rte_pci.h>
-#include <rte_bus_pci.h>
 #include <rte_dev.h>

 #if RTE_VERSION < RTE_VERSION_NUM(19, 11, 0, 0)
 #error RTE_VERSION is too old! Minimum 19.11 is required.
 #endif

+#if RTE_VERSION < RTE_VERSION_NUM(21, 11, 0, 0)
+#include <rte_bus_pci.h>
+#else
+#include <pci_driver.h>
+#endif
+
 /* x86-64 and ARM userspace virtual addresses use only the low 48 bits [0..47],
  * which is enough to cover 256 TB.
  */
Harris, James R Oct. 4, 2021, 3:56 p.m. UTC | #4 
Adding Changpeng Liu from SPDK side.

On 10/4/21, 6:48 AM, "David Marchand" <david.marchand@redhat.com> wrote:

    On Thu, Sep 30, 2021 at 10:45 AM David Marchand
    <david.marchand@redhat.com> wrote:
    > On Wed, Sep 29, 2021 at 9:38 AM Xia, Chenbo <chenbo.xia@intel.com> wrote:
    > > @David, could you help me understand what is the compile error in Fedora 31?
    > > DPDK_compile_spdk failure is expected as the header name for SPDK is changed,
    > > I am not sure if it's the same error...
    >
    > The error log is odd (no compilation "backtrace").
    > You'll need to test spdk manually I guess.

    Tried your series with SPDK (w/o and w/ enable_driver_sdk).
    I think the same, and the error is likely due to the file rename.

    $ make
      CC lib/env_dpdk/env.o
    In file included from env.c:39:0:
    env_internal.h:64:25: error: field ‘driver’ has incomplete type
      struct rte_pci_driver  driver;
                             ^
    env_internal.h:75:59: warning: ‘struct rte_pci_device’ declared inside
    parameter list [enabled by default]
     int pci_device_init(struct rte_pci_driver *driver, struct
    rte_pci_device *device);
                                                               ^
    env_internal.h:75:59: warning: its scope is only this definition or
    declaration, which is probably not what you want [enabled by default]
    env_internal.h:76:28: warning: ‘struct rte_pci_device’ declared inside
    parameter list [enabled by default]
     int pci_device_fini(struct rte_pci_device *device);
                                ^
    env_internal.h:89:38: warning: ‘struct rte_pci_device’ declared inside
    parameter list [enabled by default]
     void vtophys_pci_device_added(struct rte_pci_device *pci_device);
                                          ^
    env_internal.h:96:40: warning: ‘struct rte_pci_device’ declared inside
    parameter list [enabled by default]
     void vtophys_pci_device_removed(struct rte_pci_device *pci_device);
                                            ^
    make[2]: *** [env.o] Error 1
    make[1]: *** [env_dpdk] Error 2
    make: *** [lib] Error 2



    So basically, SPDK needs some updates since it has its own pci drivers.
    I copied some SPDK folks for info.

    *Disclaimer* I only checked it links fine against my 21.11 dpdk env,
    and did not test the other cases:

    diff --git a/dpdkbuild/Makefile b/dpdkbuild/Makefile
    index d51b1a6e5..0e666735d 100644
    --- a/dpdkbuild/Makefile
    +++ b/dpdkbuild/Makefile
    @@ -166,6 +166,7 @@ all: $(SPDK_ROOT_DIR)/dpdk/build-tmp
     $(SPDK_ROOT_DIR)/dpdk/build-tmp: $(SPDK_ROOT_DIR)/mk/cc.mk
    $(SPDK_ROOT_DIR)/include/spdk/config.h
            $(Q)rm -rf $(SPDK_ROOT_DIR)/dpdk/build $(SPDK_ROOT_DIR)/dpdk/build-tmp
            $(Q)cd "$(SPDK_ROOT_DIR)/dpdk"; CC="$(SUB_CC)" meson
    --prefix="$(MESON_PREFIX)" --libdir lib -Dc_args="$(DPDK_CFLAGS)"
    -Dc_link_args="$(DPDK_LDFLAGS)" $(DPDK_OPTS)
    -Ddisable_drivers="$(shell echo $(DPDK_DISABLED_DRVERS) | sed -E "s/
    +/,/g")" build-tmp
    +       $(Q)! meson configure build-tmp | grep -qw enable_driver_sdk
    || meson configure build-tmp -Denable_driver_sdk=true
            $(Q)sed $(SED_INPLACE_FLAG) 's/#define RTE_EAL_PMD_PATH
    .*/#define RTE_EAL_PMD_PATH ""/g'
    $(SPDK_ROOT_DIR)/dpdk/build-tmp/rte_build_config.h
            $(Q) \
            # TODO Meson build adds libbsd dependency when it's available.
    This means any app will be \
    diff --git a/lib/env_dpdk/env.mk b/lib/env_dpdk/env.mk
    index cc7db8aab..e24c6942f 100644bits with an embedded dpdk
    --- a/lib/env_dpdk/env.mk
    +++ b/lib/env_dpdk/env.mk
    @@ -172,6 +172,12 @@ DPDK_PRIVATE_LINKER_ARGS += -lnuma
     endif
     endif

    +ifneq (,$(wildcard $(DPDK_INC_DIR)/rte_build_config.h))
    +ifneq (,$(shell grep -e "define RTE_HAS_LIBARCHIVE 1"
    $(DPDK_INC_DIR)/rte_build_config.h))
    +DPDK_PRIVATE_LINKER_ARGS += -larchive
    +endif
    +endif
    +
     ifeq ($(OS),Linux)
     DPDK_PRIVATE_LINKER_ARGS += -ldl
     endif
    diff --git a/lib/env_dpdk/env_internal.h b/lib/env_dpdk/env_internal.h
    index 2303f432c..24b377545 100644
    --- a/lib/env_dpdk/env_internal.h
    +++ b/lib/env_dpdk/env_internal.h
    @@ -43,13 +43,18 @@
     #include <rte_eal.h>
     #include <rte_bus.h>
     #include <rte_pci.h>
    -#include <rte_bus_pci.h>
     #include <rte_dev.h>

     #if RTE_VERSION < RTE_VERSION_NUM(19, 11, 0, 0)
     #error RTE_VERSION is too old! Minimum 19.11 is required.
     #endif

    +#if RTE_VERSION < RTE_VERSION_NUM(21, 11, 0, 0)
    +#include <rte_bus_pci.h>
    +#else
    +#include <pci_driver.h>
    +#endif
    +
     /* x86-64 and ARM userspace virtual addresses use only the low 48 bits [0..47],
      * which is enough to cover 256 TB.
      */



    -- 
    David Marchand
Chenbo Xia Oct. 6, 2021, 4:25 a.m. UTC | #5 
Thanks David for helping check this and including SPDK folks!

Hi Changpeng,

Although we have synced about this during last release's deprecation notice,
I’d like to summarize two points for SPDK to change if this patchset applied.

1. The pci bus header for drivers will only be exposed if meson option
'enable_driver_sdk' is added, so SPDK need this DPDK meson option to build.

2. As some functions in pci bus is needed for apps and the rest for drivers,
the header for driver is renamed to pci_driver.h (header for app is rte_bus_pci.h).
So SPDK drivers will need pci_driver.h instead of rte_bus_pci.h starting from DPDK
21.11. David showed some tests he did below.

Could you help check above two updates are fine to SPDK?

Thanks,
Chenbo

> -----Original Message-----
> From: Harris, James R <james.r.harris@intel.com>
> Sent: Monday, October 4, 2021 11:56 PM
> To: David Marchand <david.marchand@redhat.com>; Xia, Chenbo
> <chenbo.xia@intel.com>; Liu, Changpeng <changpeng.liu@intel.com>
> Cc: dev@dpdk.org; ci@dpdk.org; Aaron Conole <aconole@redhat.com>; dpdklab
> <dpdklab@iol.unh.edu>; Zawadzki, Tomasz <tomasz.zawadzki@intel.com>;
> alexeymar@mellanox.com
> Subject: Re: [dpdk-dev] [PATCH v2 0/7] Removal of PCI bus ABIs
> 
> Adding Changpeng Liu from SPDK side.
> 
> On 10/4/21, 6:48 AM, "David Marchand" <david.marchand@redhat.com> wrote:
> 
>     On Thu, Sep 30, 2021 at 10:45 AM David Marchand
>     <david.marchand@redhat.com> wrote:
>     > On Wed, Sep 29, 2021 at 9:38 AM Xia, Chenbo <chenbo.xia@intel.com>
> wrote:
>     > > @David, could you help me understand what is the compile error in
> Fedora 31?
>     > > DPDK_compile_spdk failure is expected as the header name for SPDK
> is changed,
>     > > I am not sure if it's the same error...
>     >
>     > The error log is odd (no compilation "backtrace").
>     > You'll need to test spdk manually I guess.
> 
>     Tried your series with SPDK (w/o and w/ enable_driver_sdk).
>     I think the same, and the error is likely due to the file rename.
> 
>     $ make
>       CC lib/env_dpdk/env.o
>     In file included from env.c:39:0:
>     env_internal.h:64:25: error: field ‘driver’ has incomplete type
>       struct rte_pci_driver  driver;
>                              ^
>     env_internal.h:75:59: warning: ‘struct rte_pci_device’ declared inside
>     parameter list [enabled by default]
>      int pci_device_init(struct rte_pci_driver *driver, struct
>     rte_pci_device *device);
>                                                                ^
>     env_internal.h:75:59: warning: its scope is only this definition or
>     declaration, which is probably not what you want [enabled by default]
>     env_internal.h:76:28: warning: ‘struct rte_pci_device’ declared inside
>     parameter list [enabled by default]
>      int pci_device_fini(struct rte_pci_device *device);
>                                 ^
>     env_internal.h:89:38: warning: ‘struct rte_pci_device’ declared inside
>     parameter list [enabled by default]
>      void vtophys_pci_device_added(struct rte_pci_device *pci_device);
>                                           ^
>     env_internal.h:96:40: warning: ‘struct rte_pci_device’ declared inside
>     parameter list [enabled by default]
>      void vtophys_pci_device_removed(struct rte_pci_device *pci_device);
>                                             ^
>     make[2]: *** [env.o] Error 1
>     make[1]: *** [env_dpdk] Error 2
>     make: *** [lib] Error 2
> 
> 
> 
>     So basically, SPDK needs some updates since it has its own pci drivers.
>     I copied some SPDK folks for info.
> 
>     *Disclaimer* I only checked it links fine against my 21.11 dpdk env,
>     and did not test the other cases:
> 
>     diff --git a/dpdkbuild/Makefile b/dpdkbuild/Makefile
>     index d51b1a6e5..0e666735d 100644
>     --- a/dpdkbuild/Makefile
>     +++ b/dpdkbuild/Makefile
>     @@ -166,6 +166,7 @@ all: $(SPDK_ROOT_DIR)/dpdk/build-tmp
>      $(SPDK_ROOT_DIR)/dpdk/build-tmp: $(SPDK_ROOT_DIR)/mk/cc.mk
>     $(SPDK_ROOT_DIR)/include/spdk/config.h
>             $(Q)rm -rf $(SPDK_ROOT_DIR)/dpdk/build
> $(SPDK_ROOT_DIR)/dpdk/build-tmp
>             $(Q)cd "$(SPDK_ROOT_DIR)/dpdk"; CC="$(SUB_CC)" meson
>     --prefix="$(MESON_PREFIX)" --libdir lib -Dc_args="$(DPDK_CFLAGS)"
>     -Dc_link_args="$(DPDK_LDFLAGS)" $(DPDK_OPTS)
>     -Ddisable_drivers="$(shell echo $(DPDK_DISABLED_DRVERS) | sed -E "s/
>     +/,/g")" build-tmp
>     +       $(Q)! meson configure build-tmp | grep -qw enable_driver_sdk
>     || meson configure build-tmp -Denable_driver_sdk=true
>             $(Q)sed $(SED_INPLACE_FLAG) 's/#define RTE_EAL_PMD_PATH
>     .*/#define RTE_EAL_PMD_PATH ""/g'
>     $(SPDK_ROOT_DIR)/dpdk/build-tmp/rte_build_config.h
>             $(Q) \
>             # TODO Meson build adds libbsd dependency when it's available.
>     This means any app will be \
>     diff --git a/lib/env_dpdk/env.mk b/lib/env_dpdk/env.mk
>     index cc7db8aab..e24c6942f 100644bits with an embedded dpdk
>     --- a/lib/env_dpdk/env.mk
>     +++ b/lib/env_dpdk/env.mk
>     @@ -172,6 +172,12 @@ DPDK_PRIVATE_LINKER_ARGS += -lnuma
>      endif
>      endif
> 
>     +ifneq (,$(wildcard $(DPDK_INC_DIR)/rte_build_config.h))
>     +ifneq (,$(shell grep -e "define RTE_HAS_LIBARCHIVE 1"
>     $(DPDK_INC_DIR)/rte_build_config.h))
>     +DPDK_PRIVATE_LINKER_ARGS += -larchive
>     +endif
>     +endif
>     +
>      ifeq ($(OS),Linux)
>      DPDK_PRIVATE_LINKER_ARGS += -ldl
>      endif
>     diff --git a/lib/env_dpdk/env_internal.h b/lib/env_dpdk/env_internal.h
>     index 2303f432c..24b377545 100644
>     --- a/lib/env_dpdk/env_internal.h
>     +++ b/lib/env_dpdk/env_internal.h
>     @@ -43,13 +43,18 @@
>      #include <rte_eal.h>
>      #include <rte_bus.h>
>      #include <rte_pci.h>
>     -#include <rte_bus_pci.h>
>      #include <rte_dev.h>
> 
>      #if RTE_VERSION < RTE_VERSION_NUM(19, 11, 0, 0)
>      #error RTE_VERSION is too old! Minimum 19.11 is required.
>      #endif
> 
>     +#if RTE_VERSION < RTE_VERSION_NUM(21, 11, 0, 0)
>     +#include <rte_bus_pci.h>
>     +#else
>     +#include <pci_driver.h>
>     +#endif
>     +
>      /* x86-64 and ARM userspace virtual addresses use only the low 48
> bits [0..47],
>       * which is enough to cover 256 TB.
>       */
> 
> 
> 
>     --
>     David Marchand
>
Liu, Changpeng Oct. 8, 2021, 6:15 a.m. UTC | #6 
I tried the above DPDK patches, and got the following errors:

pci.c:115:7: error: call to ‘rte_pci_read_config’ declared with attribute error: Symbol is not public ABI
  115 |  rc = rte_pci_read_config(dev->dev_handle, value, len, offset);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pci.c: In function ‘cfg_write_rte’:
pci.c:125:7: error: call to ‘rte_pci_write_config’ declared with attribute error: Symbol is not public ABI
  125 |  rc = rte_pci_write_config(dev->dev_handle, value, len, offset);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pci.c: In function ‘register_rte_driver’:
pci.c:375:2: error: call to ‘rte_pci_register’ declared with attribute error: Symbol is not public ABI
  375 |  rte_pci_register(&driver->driver); 

We may use the new added API to replace rte_pci_write_config and rte_pci_read_config, but SPDK
do require rte_pci_register().


> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Wednesday, October 6, 2021 12:26 PM
> To: Harris, James R <james.r.harris@intel.com>; David Marchand
> <david.marchand@redhat.com>; Liu, Changpeng <changpeng.liu@intel.com>
> Cc: dev@dpdk.org; ci@dpdk.org; Aaron Conole <aconole@redhat.com>; dpdklab
> <dpdklab@iol.unh.edu>; Zawadzki, Tomasz <tomasz.zawadzki@intel.com>;
> alexeymar@mellanox.com
> Subject: RE: [dpdk-dev] [PATCH v2 0/7] Removal of PCI bus ABIs
> 
> Thanks David for helping check this and including SPDK folks!
> 
> Hi Changpeng,
> 
> Although we have synced about this during last release's deprecation notice,
> I’d like to summarize two points for SPDK to change if this patchset applied.
> 
> 1. The pci bus header for drivers will only be exposed if meson option
> 'enable_driver_sdk' is added, so SPDK need this DPDK meson option to build.
> 
> 2. As some functions in pci bus is needed for apps and the rest for drivers,
> the header for driver is renamed to pci_driver.h (header for app is rte_bus_pci.h).
> So SPDK drivers will need pci_driver.h instead of rte_bus_pci.h starting from
> DPDK
> 21.11. David showed some tests he did below.
> 
> Could you help check above two updates are fine to SPDK?
> 
> Thanks,
> Chenbo
> 
> > -----Original Message-----
> > From: Harris, James R <james.r.harris@intel.com>
> > Sent: Monday, October 4, 2021 11:56 PM
> > To: David Marchand <david.marchand@redhat.com>; Xia, Chenbo
> > <chenbo.xia@intel.com>; Liu, Changpeng <changpeng.liu@intel.com>
> > Cc: dev@dpdk.org; ci@dpdk.org; Aaron Conole <aconole@redhat.com>;
> dpdklab
> > <dpdklab@iol.unh.edu>; Zawadzki, Tomasz <tomasz.zawadzki@intel.com>;
> > alexeymar@mellanox.com
> > Subject: Re: [dpdk-dev] [PATCH v2 0/7] Removal of PCI bus ABIs
> >
> > Adding Changpeng Liu from SPDK side.
> >
> > On 10/4/21, 6:48 AM, "David Marchand" <david.marchand@redhat.com>
> wrote:
> >
> >     On Thu, Sep 30, 2021 at 10:45 AM David Marchand
> >     <david.marchand@redhat.com> wrote:
> >     > On Wed, Sep 29, 2021 at 9:38 AM Xia, Chenbo <chenbo.xia@intel.com>
> > wrote:
> >     > > @David, could you help me understand what is the compile error in
> > Fedora 31?
> >     > > DPDK_compile_spdk failure is expected as the header name for SPDK
> > is changed,
> >     > > I am not sure if it's the same error...
> >     >
> >     > The error log is odd (no compilation "backtrace").
> >     > You'll need to test spdk manually I guess.
> >
> >     Tried your series with SPDK (w/o and w/ enable_driver_sdk).
> >     I think the same, and the error is likely due to the file rename.
> >
> >     $ make
> >       CC lib/env_dpdk/env.o
> >     In file included from env.c:39:0:
> >     env_internal.h:64:25: error: field ‘driver’ has incomplete type
> >       struct rte_pci_driver  driver;
> >                              ^
> >     env_internal.h:75:59: warning: ‘struct rte_pci_device’ declared inside
> >     parameter list [enabled by default]
> >      int pci_device_init(struct rte_pci_driver *driver, struct
> >     rte_pci_device *device);
> >                                                                ^
> >     env_internal.h:75:59: warning: its scope is only this definition or
> >     declaration, which is probably not what you want [enabled by default]
> >     env_internal.h:76:28: warning: ‘struct rte_pci_device’ declared inside
> >     parameter list [enabled by default]
> >      int pci_device_fini(struct rte_pci_device *device);
> >                                 ^
> >     env_internal.h:89:38: warning: ‘struct rte_pci_device’ declared inside
> >     parameter list [enabled by default]
> >      void vtophys_pci_device_added(struct rte_pci_device *pci_device);
> >                                           ^
> >     env_internal.h:96:40: warning: ‘struct rte_pci_device’ declared inside
> >     parameter list [enabled by default]
> >      void vtophys_pci_device_removed(struct rte_pci_device *pci_device);
> >                                             ^
> >     make[2]: *** [env.o] Error 1
> >     make[1]: *** [env_dpdk] Error 2
> >     make: *** [lib] Error 2
> >
> >
> >
> >     So basically, SPDK needs some updates since it has its own pci drivers.
> >     I copied some SPDK folks for info.
> >
> >     *Disclaimer* I only checked it links fine against my 21.11 dpdk env,
> >     and did not test the other cases:
> >
> >     diff --git a/dpdkbuild/Makefile b/dpdkbuild/Makefile
> >     index d51b1a6e5..0e666735d 100644
> >     --- a/dpdkbuild/Makefile
> >     +++ b/dpdkbuild/Makefile
> >     @@ -166,6 +166,7 @@ all: $(SPDK_ROOT_DIR)/dpdk/build-tmp
> >      $(SPDK_ROOT_DIR)/dpdk/build-tmp: $(SPDK_ROOT_DIR)/mk/cc.mk
> >     $(SPDK_ROOT_DIR)/include/spdk/config.h
> >             $(Q)rm -rf $(SPDK_ROOT_DIR)/dpdk/build
> > $(SPDK_ROOT_DIR)/dpdk/build-tmp
> >             $(Q)cd "$(SPDK_ROOT_DIR)/dpdk"; CC="$(SUB_CC)" meson
> >     --prefix="$(MESON_PREFIX)" --libdir lib -Dc_args="$(DPDK_CFLAGS)"
> >     -Dc_link_args="$(DPDK_LDFLAGS)" $(DPDK_OPTS)
> >     -Ddisable_drivers="$(shell echo $(DPDK_DISABLED_DRVERS) | sed -E "s/
> >     +/,/g")" build-tmp
> >     +       $(Q)! meson configure build-tmp | grep -qw enable_driver_sdk
> >     || meson configure build-tmp -Denable_driver_sdk=true
> >             $(Q)sed $(SED_INPLACE_FLAG) 's/#define RTE_EAL_PMD_PATH
> >     .*/#define RTE_EAL_PMD_PATH ""/g'
> >     $(SPDK_ROOT_DIR)/dpdk/build-tmp/rte_build_config.h
> >             $(Q) \
> >             # TODO Meson build adds libbsd dependency when it's available.
> >     This means any app will be \
> >     diff --git a/lib/env_dpdk/env.mk b/lib/env_dpdk/env.mk
> >     index cc7db8aab..e24c6942f 100644bits with an embedded dpdk
> >     --- a/lib/env_dpdk/env.mk
> >     +++ b/lib/env_dpdk/env.mk
> >     @@ -172,6 +172,12 @@ DPDK_PRIVATE_LINKER_ARGS += -lnuma
> >      endif
> >      endif
> >
> >     +ifneq (,$(wildcard $(DPDK_INC_DIR)/rte_build_config.h))
> >     +ifneq (,$(shell grep -e "define RTE_HAS_LIBARCHIVE 1"
> >     $(DPDK_INC_DIR)/rte_build_config.h))
> >     +DPDK_PRIVATE_LINKER_ARGS += -larchive
> >     +endif
> >     +endif
> >     +
> >      ifeq ($(OS),Linux)
> >      DPDK_PRIVATE_LINKER_ARGS += -ldl
> >      endif
> >     diff --git a/lib/env_dpdk/env_internal.h b/lib/env_dpdk/env_internal.h
> >     index 2303f432c..24b377545 100644
> >     --- a/lib/env_dpdk/env_internal.h
> >     +++ b/lib/env_dpdk/env_internal.h
> >     @@ -43,13 +43,18 @@
> >      #include <rte_eal.h>
> >      #include <rte_bus.h>
> >      #include <rte_pci.h>
> >     -#include <rte_bus_pci.h>
> >      #include <rte_dev.h>
> >
> >      #if RTE_VERSION < RTE_VERSION_NUM(19, 11, 0, 0)
> >      #error RTE_VERSION is too old! Minimum 19.11 is required.
> >      #endif
> >
> >     +#if RTE_VERSION < RTE_VERSION_NUM(21, 11, 0, 0)
> >     +#include <rte_bus_pci.h>
> >     +#else
> >     +#include <pci_driver.h>
> >     +#endif
> >     +
> >      /* x86-64 and ARM userspace virtual addresses use only the low 48
> > bits [0..47],
> >       * which is enough to cover 256 TB.
> >       */
> >
> >
> >
> >     --
> >     David Marchand
> >
David Marchand Oct. 8, 2021, 7:08 a.m. UTC | #7 
Hello,

On Fri, Oct 8, 2021 at 8:15 AM Liu, Changpeng <changpeng.liu@intel.com> wrote:
>
> I tried the above DPDK patches, and got the following errors:
>
> pci.c:115:7: error: call to ‘rte_pci_read_config’ declared with attribute error: Symbol is not public ABI
>   115 |  rc = rte_pci_read_config(dev->dev_handle, value, len, offset);
>       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> pci.c: In function ‘cfg_write_rte’:
> pci.c:125:7: error: call to ‘rte_pci_write_config’ declared with attribute error: Symbol is not public ABI
>   125 |  rc = rte_pci_write_config(dev->dev_handle, value, len, offset);
>       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> pci.c: In function ‘register_rte_driver’:
> pci.c:375:2: error: call to ‘rte_pci_register’ declared with attribute error: Symbol is not public ABI
>   375 |  rte_pci_register(&driver->driver);

I should have got this warning... but compilation passed fine for me.
Happy you tested it.

>
> We may use the new added API to replace rte_pci_write_config and rte_pci_read_config, but SPDK
> do require rte_pci_register().

Since SPDK has a PCI driver, you'll need to compile code that calls
those PCI driver internal API with ALLOW_INTERNAL_API defined.
You can probably add a #define ALLOW_INTERNAL_API first thing (it's
important to have it defined before including any dpdk header) in
pci.c

Another option, is to add it to lib/env_dpdk/env.mk:ENV_CFLAGS =
$(DPDK_INC) -DALLOW_EXPERIMENTAL_API.

Can someone from SPDK take over this and sync with Chenbo?


Thanks.
Liu, Changpeng Oct. 8, 2021, 7:44 a.m. UTC | #8 
Thanks, I have worked with Chenbo to address this issue before.  After enable the `ALLOW_INTERNAL_API` option, it works now with SPDK.

Another issue raised by Jim Harris is that for distro packaged DPDK, since this option isn't enabled by default, this will not allow SPDK
to use the distro packaged DPDK after this release.

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, October 8, 2021 3:08 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Harris, James R
> <james.r.harris@intel.com>; dev@dpdk.org; ci@dpdk.org; Aaron Conole
> <aconole@redhat.com>; dpdklab <dpdklab@iol.unh.edu>; Zawadzki, Tomasz
> <tomasz.zawadzki@intel.com>; alexeymar@mellanox.com
> Subject: Re: [dpdk-dev] [PATCH v2 0/7] Removal of PCI bus ABIs
> 
> Hello,
> 
> On Fri, Oct 8, 2021 at 8:15 AM Liu, Changpeng <changpeng.liu@intel.com> wrote:
> >
> > I tried the above DPDK patches, and got the following errors:
> >
> > pci.c:115:7: error: call to ‘rte_pci_read_config’ declared with attribute error:
> Symbol is not public ABI
> >   115 |  rc = rte_pci_read_config(dev->dev_handle, value, len, offset);
> >       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > pci.c: In function ‘cfg_write_rte’:
> > pci.c:125:7: error: call to ‘rte_pci_write_config’ declared with attribute error:
> Symbol is not public ABI
> >   125 |  rc = rte_pci_write_config(dev->dev_handle, value, len, offset);
> >       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > pci.c: In function ‘register_rte_driver’:
> > pci.c:375:2: error: call to ‘rte_pci_register’ declared with attribute error:
> Symbol is not public ABI
> >   375 |  rte_pci_register(&driver->driver);
> 
> I should have got this warning... but compilation passed fine for me.
> Happy you tested it.
> 
> >
> > We may use the new added API to replace rte_pci_write_config and
> rte_pci_read_config, but SPDK
> > do require rte_pci_register().
> 
> Since SPDK has a PCI driver, you'll need to compile code that calls
> those PCI driver internal API with ALLOW_INTERNAL_API defined.
> You can probably add a #define ALLOW_INTERNAL_API first thing (it's
> important to have it defined before including any dpdk header) in
> pci.c
> 
> Another option, is to add it to lib/env_dpdk/env.mk:ENV_CFLAGS =
> $(DPDK_INC) -DALLOW_EXPERIMENTAL_API.
> 
> Can someone from SPDK take over this and sync with Chenbo?
> 
> 
> Thanks.
> 
> --
> David Marchand
Chenbo Xia Oct. 11, 2021, 6:58 a.m. UTC | #9 
Hi David & Changpeng,

> -----Original Message-----
> From: Liu, Changpeng <changpeng.liu@intel.com>
> Sent: Friday, October 8, 2021 3:45 PM
> To: David Marchand <david.marchand@redhat.com>; Harris, James R
> <james.r.harris@intel.com>
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org; ci@dpdk.org; Aaron
> Conole <aconole@redhat.com>; dpdklab <dpdklab@iol.unh.edu>; Zawadzki, Tomasz
> <tomasz.zawadzki@intel.com>; alexeymar@mellanox.com
> Subject: RE: [dpdk-dev] [PATCH v2 0/7] Removal of PCI bus ABIs
> 
> Thanks, I have worked with Chenbo to address this issue before.  After enable
> the `ALLOW_INTERNAL_API` option, it works now with SPDK.
> 
> Another issue raised by Jim Harris is that for distro packaged DPDK, since
> this option isn't enabled by default, this will not allow SPDK
> to use the distro packaged DPDK after this release.

I think for this problem, we have two options: enable driver sdk by default or
let OSV configure the option when building distros. I'm fine with either option.

@David, What do you think?

Thanks,
Chenbo

> 
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Friday, October 8, 2021 3:08 PM
> > To: Liu, Changpeng <changpeng.liu@intel.com>
> > Cc: Xia, Chenbo <chenbo.xia@intel.com>; Harris, James R
> > <james.r.harris@intel.com>; dev@dpdk.org; ci@dpdk.org; Aaron Conole
> > <aconole@redhat.com>; dpdklab <dpdklab@iol.unh.edu>; Zawadzki, Tomasz
> > <tomasz.zawadzki@intel.com>; alexeymar@mellanox.com
> > Subject: Re: [dpdk-dev] [PATCH v2 0/7] Removal of PCI bus ABIs
> >
> > Hello,
> >
> > On Fri, Oct 8, 2021 at 8:15 AM Liu, Changpeng <changpeng.liu@intel.com>
> wrote:
> > >
> > > I tried the above DPDK patches, and got the following errors:
> > >
> > > pci.c:115:7: error: call to ‘rte_pci_read_config’ declared with attribute
> error:
> > Symbol is not public ABI
> > >   115 |  rc = rte_pci_read_config(dev->dev_handle, value, len, offset);
> > >       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > pci.c: In function ‘cfg_write_rte’:
> > > pci.c:125:7: error: call to ‘rte_pci_write_config’ declared with attribute
> error:
> > Symbol is not public ABI
> > >   125 |  rc = rte_pci_write_config(dev->dev_handle, value, len, offset);
> > >       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > pci.c: In function ‘register_rte_driver’:
> > > pci.c:375:2: error: call to ‘rte_pci_register’ declared with attribute
> error:
> > Symbol is not public ABI
> > >   375 |  rte_pci_register(&driver->driver);
> >
> > I should have got this warning... but compilation passed fine for me.
> > Happy you tested it.
> >
> > >
> > > We may use the new added API to replace rte_pci_write_config and
> > rte_pci_read_config, but SPDK
> > > do require rte_pci_register().
> >
> > Since SPDK has a PCI driver, you'll need to compile code that calls
> > those PCI driver internal API with ALLOW_INTERNAL_API defined.
> > You can probably add a #define ALLOW_INTERNAL_API first thing (it's
> > important to have it defined before including any dpdk header) in
> > pci.c
> >
> > Another option, is to add it to lib/env_dpdk/env.mk:ENV_CFLAGS =
> > $(DPDK_INC) -DALLOW_EXPERIMENTAL_API.
> >
> > Can someone from SPDK take over this and sync with Chenbo?
> >
> >
> > Thanks.
> >
> > --
> > David Marchand
Thomas Monjalon Oct. 11, 2021, 12:55 p.m. UTC | #10 
11/10/2021 08:58, Xia, Chenbo:
> From: Liu, Changpeng <changpeng.liu@intel.com>
> > Another issue raised by Jim Harris is that for distro packaged DPDK, since
> > this option isn't enabled by default, this will not allow SPDK
> > to use the distro packaged DPDK after this release.
> 
> I think for this problem, we have two options: enable driver sdk by default or
> let OSV configure the option when building distros. I'm fine with either option.

The meson option enable_driver_sdk is described as "Install headers to build drivers."
Standard development packages should provide headers to build an application.
This option is for projects extending DPDK drivers out of the tree.
The preferred option is to develop drivers inside DPDK.

If a project needs the special option enable_driver_sdk,
1/ it is not following the recommended approach,
2/ it has to manage the burden of driver compatibility with DPDK,
3/ it can compile DPDK itself.

So I think we neither need to make it a default, nor force distros to enable it.
Am I missing something?
Harris, James R Oct. 12, 2021, 12:35 a.m. UTC | #11 
On 10/11/21, 5:55 AM, "Thomas Monjalon" <thomas@monjalon.net> wrote:

    11/10/2021 08:58, Xia, Chenbo:
    > From: Liu, Changpeng <changpeng.liu@intel.com>
    > > Another issue raised by Jim Harris is that for distro packaged DPDK, since
    > > this option isn't enabled by default, this will not allow SPDK
    > > to use the distro packaged DPDK after this release.
    > 
    > I think for this problem, we have two options: enable driver sdk by default or
    > let OSV configure the option when building distros. I'm fine with either option.

    The meson option enable_driver_sdk is described as "Install headers to build drivers."
    Standard development packages should provide headers to build an application.
    This option is for projects extending DPDK drivers out of the tree.
    The preferred option is to develop drivers inside DPDK.

    If a project needs the special option enable_driver_sdk,
    1/ it is not following the recommended approach,
    2/ it has to manage the burden of driver compatibility with DPDK,
    3/ it can compile DPDK itself.

    So I think we neither need to make it a default, nor force distros to enable it.
    Am I missing something?

Hi Thomas,

This preference to develop PCI drivers inside of DPDK seems to be a very recent preference.  enable_driver_sdk was just added in DPDK 21.05, and for building out-of-tree ethdev drivers. But DPDK has always enabled building out-of-tree PCI drivers with its default build configuration - SPDK has relied on these APIs since its inception.

We have always viewed DPDK as being a very useful toolkit for building userspace drivers (especially storage drivers!) that aren't part of DPDK itself.  We hope that continues to be the case.

All of that being said, SPDK already compiles DPDK itself as the default configuration. We maintain a DPDK fork for patches that have not yet hit DPDK upstream. If this gets merged we can document that users building DPDK themselves must set enable_driver_sdk. We can also document to our users that SPDK may not build against distro DPDK packages, once distros pick up these changes.

-Jim
Thomas Monjalon Oct. 12, 2021, 7:04 a.m. UTC | #12 
12/10/2021 02:35, Harris, James R:
> On 10/11/21, 5:55 AM, "Thomas Monjalon" <thomas@monjalon.net> wrote:
> 
>     11/10/2021 08:58, Xia, Chenbo:
>     > From: Liu, Changpeng <changpeng.liu@intel.com>
>     > > Another issue raised by Jim Harris is that for distro packaged DPDK, since
>     > > this option isn't enabled by default, this will not allow SPDK
>     > > to use the distro packaged DPDK after this release.
>     > 
>     > I think for this problem, we have two options: enable driver sdk by default or
>     > let OSV configure the option when building distros. I'm fine with either option.
> 
>     The meson option enable_driver_sdk is described as "Install headers to build drivers."
>     Standard development packages should provide headers to build an application.
>     This option is for projects extending DPDK drivers out of the tree.
>     The preferred option is to develop drivers inside DPDK.
> 
>     If a project needs the special option enable_driver_sdk,
>     1/ it is not following the recommended approach,
>     2/ it has to manage the burden of driver compatibility with DPDK,
>     3/ it can compile DPDK itself.
> 
>     So I think we neither need to make it a default, nor force distros to enable it.
>     Am I missing something?
> 
> Hi Thomas,
> 
> This preference to develop PCI drivers inside of DPDK seems to be a very recent preference.  enable_driver_sdk was just added in DPDK 21.05, and for building out-of-tree ethdev drivers. But DPDK has always enabled building out-of-tree PCI drivers with its default build configuration - SPDK has relied on these APIs since its inception.

Yes DPDK allows out-of-tree drivers, but it has never been recommended.
We have introduced enable_driver_sdk option recently to keep allowing out-of-tree drivers.

> We have always viewed DPDK as being a very useful toolkit for building userspace drivers (especially storage drivers!) that aren't part of DPDK itself.  We hope that continues to be the case.

Yes, there is no plan to stop that, but also no plan to make it easier.

> All of that being said, SPDK already compiles DPDK itself as the default configuration. We maintain a DPDK fork for patches that have not yet hit DPDK upstream. If this gets merged we can document that users building DPDK themselves must set enable_driver_sdk. We can also document to our users that SPDK may not build against distro DPDK packages, once distros pick up these changes.

Yes I think that's the right thing to do.

Note: I don't remember the reason to keep your drivers out of DPDK?
Walker, Benjamin Oct. 12, 2021, 4:59 p.m. UTC | #13 
> From: dev <dev-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> 
> 12/10/2021 02:35, Harris, James R:
> > On 10/11/21, 5:55 AM, "Thomas Monjalon" <thomas@monjalon.net> wrote:
> >
> >     The meson option enable_driver_sdk is described as "Install headers to build
> drivers."
> >     Standard development packages should provide headers to build an
> application.
> >     This option is for projects extending DPDK drivers out of the tree.
> >     The preferred option is to develop drivers inside DPDK.
> >
> >     If a project needs the special option enable_driver_sdk,
> >     1/ it is not following the recommended approach,
> >     2/ it has to manage the burden of driver compatibility with DPDK,
> >     3/ it can compile DPDK itself.
> >
> >     So I think we neither need to make it a default, nor force distros to enable it.
> >     Am I missing something?
> >
> > Hi Thomas,
> >
> > This preference to develop PCI drivers inside of DPDK seems to be a very
> recent preference.  enable_driver_sdk was just added in DPDK 21.05, and for
> building out-of-tree ethdev drivers. But DPDK has always enabled building out-
> of-tree PCI drivers with its default build configuration - SPDK has relied on these
> APIs since its inception.
> 
> Yes DPDK allows out-of-tree drivers, but it has never been recommended.

For networking drivers, maybe. But certainly years and years ago when SPDK was started no one recommended putting an nvme driver into DPDK.

> We have introduced enable_driver_sdk option recently to keep allowing out-of-
> tree drivers.
> 
> > We have always viewed DPDK as being a very useful toolkit for building
> userspace drivers (especially storage drivers!) that aren't part of DPDK itself.  We
> hope that continues to be the case.
> 
> Yes, there is no plan to stop that, but also no plan to make it easier.

To be clear, this change actively makes it harder. DPDK has changed the longstanding status quo.

> 
> > All of that being said, SPDK already compiles DPDK itself as the default
> configuration. We maintain a DPDK fork for patches that have not yet hit DPDK
> upstream. If this gets merged we can document that users building DPDK
> themselves must set enable_driver_sdk. We can also document to our users that
> SPDK may not build against distro DPDK packages, once distros pick up these
> changes.
> 
> Yes I think that's the right thing to do.
> 

This means that a distro-packaged SPDK cannot exist, because it cannot use a distro-packaged DPDK as a dependency. While using a distro-packaged SPDK is not the common case (people just build it themselves), my personal view is that we need to be able to support this and this change from DPDK is unacceptable.

> Note: I don't remember the reason to keep your drivers out of DPDK?

SPDK uses DPDK as a framework for writing user space drivers only - scanning the PCI bus, allocating DMA-safe memory, etc. This functionality is hidden behind an abstraction layer that can be reimplemented by our users to remove the DPDK dependency entirely, and real production users have elected to do this. The reasons they do this are varied, but the shortest way to say it is that DPDK is a framework that requires their application to buy-in across the board, whereas SPDK is a set of libraries that integrates into their existing application more easily.

SPDK simply uses DPDK as the default implementation for this functionality. We cannot port our drivers into DPDK or it would break this use case. 

Thanks,
Ben
Thomas Monjalon Oct. 12, 2021, 6:43 p.m. UTC | #14 
12/10/2021 18:59, Walker, Benjamin:
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Thomas Monjalon
> > 12/10/2021 02:35, Harris, James R:
> > > On 10/11/21, 5:55 AM, "Thomas Monjalon" <thomas@monjalon.net> wrote:
> > >
> > >     The meson option enable_driver_sdk is described as "Install headers to build
> > drivers."
> > >     Standard development packages should provide headers to build an
> > application.
> > >     This option is for projects extending DPDK drivers out of the tree.
> > >     The preferred option is to develop drivers inside DPDK.
> > >
> > >     If a project needs the special option enable_driver_sdk,
> > >     1/ it is not following the recommended approach,
> > >     2/ it has to manage the burden of driver compatibility with DPDK,
> > >     3/ it can compile DPDK itself.
> > >
> > >     So I think we neither need to make it a default, nor force distros to enable it.
> > >     Am I missing something?
> > >
> > > Hi Thomas,
> > >
> > > This preference to develop PCI drivers inside of DPDK seems to be a very
> > recent preference.  enable_driver_sdk was just added in DPDK 21.05, and for
> > building out-of-tree ethdev drivers. But DPDK has always enabled building out-
> > of-tree PCI drivers with its default build configuration - SPDK has relied on these
> > APIs since its inception.
> > 
> > Yes DPDK allows out-of-tree drivers, but it has never been recommended.
> 
> For networking drivers, maybe. But certainly years and years ago when SPDK was started no one recommended putting an nvme driver into DPDK.

No one from SPDK project proposed such thing.
I asked several times in person why that,
and even the DPDK techboard asked for such a merge:
https://mails.dpdk.org/archives/dev/2018-December/120706.html
The reply:
http://inbox.dpdk.org/dev/20181217141030.bhe5pwlqnzb3w3i7@platinum/
Even older question in 2015:
http://inbox.dpdk.org/dev/6421280.5XkMhqyP4M@xps13/


> > We have introduced enable_driver_sdk option recently to keep allowing out-of-
> > tree drivers.
> > 
> > > We have always viewed DPDK as being a very useful toolkit for building
> > userspace drivers (especially storage drivers!) that aren't part of DPDK itself.  We
> > hope that continues to be the case.
> > 
> > Yes, there is no plan to stop that, but also no plan to make it easier.
> 
> To be clear, this change actively makes it harder. DPDK has changed the longstanding status quo.

Yes it requires a compilation option.


> > > All of that being said, SPDK already compiles DPDK itself as the default
> > configuration. We maintain a DPDK fork for patches that have not yet hit DPDK
> > upstream. If this gets merged we can document that users building DPDK
> > themselves must set enable_driver_sdk. We can also document to our users that
> > SPDK may not build against distro DPDK packages, once distros pick up these
> > changes.
> > 
> > Yes I think that's the right thing to do.
> 
> This means that a distro-packaged SPDK cannot exist, because it cannot use a distro-packaged DPDK as a dependency.

I don't think so.
Once SPDK is packaged, what do you need from DPDK?
I think you need only .so files for some libs like EAL and PCI,
so that's available in the DPDK package, right?

> While using a distro-packaged SPDK is not the common case (people just build it themselves),
> my personal view is that we need to be able to support this and this change from DPDK is unacceptable.

I agree you should be able to package SPDK.


> > Note: I don't remember the reason to keep your drivers out of DPDK?
> 
> SPDK uses DPDK as a framework for writing user space drivers only - scanning the PCI bus, allocating DMA-safe memory, etc. This functionality is hidden behind an abstraction layer that can be reimplemented by our users to remove the DPDK dependency entirely, and real production users have elected to do this. The reasons they do this are varied, but the shortest way to say it is that DPDK is a framework that requires their application to buy-in across the board, whereas SPDK is a set of libraries that integrates into their existing application more easily.
> 
> SPDK simply uses DPDK as the default implementation for this functionality. We cannot port our drivers into DPDK or it would break this use case.
> 
> Thanks,
> Ben
Walker, Benjamin Oct. 12, 2021, 7:26 p.m. UTC | #15 
> From: Thomas Monjalon <thomas@monjalon.net>
> 12/10/2021 18:59, Walker, Benjamin:
> > For networking drivers, maybe. But certainly years and years ago when SPDK
> was started no one recommended putting an nvme driver into DPDK.
> 
> No one from SPDK project proposed such thing.
> I asked several times in person why that, and even the DPDK techboard asked for
> such a merge:
> https://mails.dpdk.org/archives/dev/2018-December/120706.html
> The reply:
> http://inbox.dpdk.org/dev/20181217141030.bhe5pwlqnzb3w3i7@platinum/
> Even older question in 2015:
> http://inbox.dpdk.org/dev/6421280.5XkMhqyP4M@xps13/
> 

For my part in these discussions, it was always about merging the governance of the projects rather than the code. I don't think a merger even occurred to anyone I spoke with during that - certainly it didn't to me. SPDK is huge and beyond its use of EAL/PCI doesn't share much in common with the rest of DPDK (SPDK uses lightweight green threading, all virtual addresses, etc.). Anyway, as I pointed out one of our key use cases for several users is the ability to replace DPDK entirely, so merging isn't an option.

> > This means that a distro-packaged SPDK cannot exist, because it cannot use a
> distro-packaged DPDK as a dependency.
> 
> I don't think so.
> Once SPDK is packaged, what do you need from DPDK?
> I think you need only .so files for some libs like EAL and PCI, so that's available in
> the DPDK package, right?
> 

So is DPDK committed to maintaining the existing ABI, such that the necessary symbols are still exported even when enable_driver_sdk is off? This option will, into the foreseeable future, only impact the installation of those header files? If that's the case, we can just copy the header file into SPDK, as could anyone else that wants to continue using DPDK to implement out of tree drivers. Can you clarify if something like this scheme would be considered a supported use of DPDK?

Thanks,
Ben
Thomas Monjalon Oct. 12, 2021, 9:50 p.m. UTC | #16 
12/10/2021 21:26, Walker, Benjamin:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > 12/10/2021 18:59, Walker, Benjamin:
> > > For networking drivers, maybe. But certainly years and years ago when SPDK
> > was started no one recommended putting an nvme driver into DPDK.
> > 
> > No one from SPDK project proposed such thing.
> > I asked several times in person why that, and even the DPDK techboard asked for
> > such a merge:
> > https://mails.dpdk.org/archives/dev/2018-December/120706.html
> > The reply:
> > http://inbox.dpdk.org/dev/20181217141030.bhe5pwlqnzb3w3i7@platinum/
> > Even older question in 2015:
> > http://inbox.dpdk.org/dev/6421280.5XkMhqyP4M@xps13/
> > 
> 
> For my part in these discussions, it was always about merging the governance of the projects rather than the code. I don't think a merger even occurred to anyone I spoke with during that - certainly it didn't to me. SPDK is huge and beyond its use of EAL/PCI doesn't share much in common with the rest of DPDK (SPDK uses lightweight green threading, all virtual addresses, etc.). Anyway, as I pointed out one of our key use cases for several users is the ability to replace DPDK entirely, so merging isn't an option.

OK I understand, that's clear.
I would be interesting to know if the NVMe drivers could be split in two parts:
one part in DPDK, and the other part in SPDK for the non-DPDK case.
I ask because it may ease things for DPDK integration in SPDK.
There is probably a cost for the SPDK project, so it could be interesting
to compare pros and cons, if possible at all.


> > > This means that a distro-packaged SPDK cannot exist, because it cannot use a
> > distro-packaged DPDK as a dependency.
> > 
> > I don't think so.
> > Once SPDK is packaged, what do you need from DPDK?
> > I think you need only .so files for some libs like EAL and PCI, so that's available in
> > the DPDK package, right?
> > 
> 
> So is DPDK committed to maintaining the existing ABI,
> such that the necessary symbols are still exported
> even when enable_driver_sdk is off?

Symbols required by drivers are necessarily exported.
Do you think I am missing something?
Do you need EAL internal functions?
We should check which functions are called by SPDK,
because there is a trend to export less functions if not needed.

> This option will, into the foreseeable future,
> only impact the installation of those header files?

I don't see what else it could impact.

> If that's the case, we can just copy the header file into SPDK,
> as could anyone else that wants to continue using DPDK
> to implement out of tree drivers.
> Can you clarify if something like this scheme would be considered a supported use of DPDK?

DPDK can be used by anybody as far as the (permissive) license is respected.
I consider copying files as a source of sync issues, but you are free.

In order to be perfectly clear, all the changes done
around this option enable_driver_sdk share the goal of tidying stuff
in DPDK so that ABI becomes better manageable.
I think that nobody want to annoy the SPDK project.
I understand that the changes effectively add troubles, and I am sorry
about that. If SPDK and other projects can manage with this change, good.
If there is a real blocker, we should discuss what are the options.

Thanks for your understanding
Walker, Benjamin Oct. 13, 2021, 5:56 p.m. UTC | #17 
> From: Thomas Monjalon <thomas@monjalon.net>
> 
> In order to be perfectly clear, all the changes done around this option
> enable_driver_sdk share the goal of tidying stuff in DPDK so that ABI becomes
> better manageable.
> I think that nobody want to annoy the SPDK project.
> I understand that the changes effectively add troubles, and I am sorry about
> that. If SPDK and other projects can manage with this change, good.
> If there is a real blocker, we should discuss what are the options.
> 
> Thanks for your understanding

I completely understand the desire to make the ABI manageable. If I were in your shoes, I'd be doing the same exact thing. What I don't currently understand is the motivation behind this enable_driver_sdk option. My guess is that it's one of two things.

\1 ABI manageability: You say that's the purpose above, and that was my initial assumption. But wouldn't that necessarily mean, over time, no longer considering the symbols that were defined by the header files as part of the stable ABI? If you still consider these symbols as part of the ABI in shared library builds, then the enable_driver_sdk option does absolutely nothing to improve the ABI situation, so why bother to have it at all? We can't have packaged SPDK relying on symbols in a packaged DPDK that are not part of the official ABI.
\2 Not supporting out-of-tree drivers: Another option is that you just don't want people writing out of tree drivers. You can't just drop it outright because people already do it, but you'd like to not support it for shared library builds at least.

So I'd like to really understand which of these two motivated the enable_driver_sdk option . Maybe it's not even one of the two above. If it is #1, then I think maybe we can work with DPDK to define a very small set of out-of-tree driver APIs/ABIs that need to continue to exist in the shared libraries by default. I do think SPDK needs only a very small number. If it's #2, then that's the entire SPDK use case and I'd ask you to reconsider the direction.

Thanks,
Ben
Thomas Monjalon Oct. 13, 2021, 6:59 p.m. UTC | #18 
13/10/2021 19:56, Walker, Benjamin:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > 
> > In order to be perfectly clear, all the changes done around this option
> > enable_driver_sdk share the goal of tidying stuff in DPDK so that ABI becomes
> > better manageable.
> > I think that nobody want to annoy the SPDK project.
> > I understand that the changes effectively add troubles, and I am sorry about
> > that. If SPDK and other projects can manage with this change, good.
> > If there is a real blocker, we should discuss what are the options.
> > 
> > Thanks for your understanding
> 
> I completely understand the desire to make the ABI manageable. If I were in your shoes, I'd be doing the same exact thing. What I don't currently understand is the motivation behind this enable_driver_sdk option. My guess is that it's one of two things.
> 
> \1 ABI manageability: You say that's the purpose above, and that was my initial assumption. But wouldn't that necessarily mean, over time, no longer considering the symbols that were defined by the header files as part of the stable ABI?

Absolutely. The idea is that we don't guarantee ABI for the drivers.

> If you still consider these symbols as part of the ABI in shared library builds, then the enable_driver_sdk option does absolutely nothing to improve the ABI situation, so why bother to have it at all? We can't have packaged SPDK relying on symbols in a packaged DPDK that are not part of the official ABI.

> \2 Not supporting out-of-tree drivers: Another option is that you just don't want people writing out of tree drivers.

We don't want complications due to support of out-of-tree drivers,
but we don't want to forbid them.

> You can't just drop it outright because people already do it,
> but you'd like to not support it for shared library builds at least.

I didn't think about it in these terms.
But saying we don't offer compatibility for shared library drivers
is not too far of "no support" indeed.

> So I'd like to really understand which of these two motivated the enable_driver_sdk option . Maybe it's not even one of the two above. If it is #1, then I think maybe we can work with DPDK to define a very small set of out-of-tree driver APIs/ABIs that need to continue to exist in the shared libraries by default. I do think SPDK needs only a very small number. If it's #2, then that's the entire SPDK use case and I'd ask you to reconsider the direction.

Yes I think we need to agree on functions to keep as-is for compatibility.
Waiting for your input please.
Walker, Benjamin Oct. 13, 2021, 10:48 p.m. UTC | #19 
> From: Thomas Monjalon <thomas@monjalon.net>
 
> Yes I think we need to agree on functions to keep as-is for compatibility.
> Waiting for your input please.

We've added a task to our backlog to propose a stable ABI for out of tree drivers here. It's not as simple as just keeping a couple of the existing functions - we're currently manipulating structures directly. We'll need to work a bit to design the simplest possible set of functions that we need.
Chenbo Xia Oct. 14, 2021, 2:21 a.m. UTC | #20 
Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, October 14, 2021 3:00 AM
> To: Harris, James R <james.r.harris@intel.com>; Walker, Benjamin
> <benjamin.walker@intel.com>
> Cc: Liu, Changpeng <changpeng.liu@intel.com>; Xia, Chenbo
> <chenbo.xia@intel.com>; David Marchand <david.marchand@redhat.com>;
> dev@dpdk.org; Aaron Conole <aconole@redhat.com>; Zawadzki, Tomasz
> <tomasz.zawadzki@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 0/7] Removal of PCI bus ABIs
> 
> 13/10/2021 19:56, Walker, Benjamin:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > >
> > > In order to be perfectly clear, all the changes done around this option
> > > enable_driver_sdk share the goal of tidying stuff in DPDK so that ABI
> becomes
> > > better manageable.
> > > I think that nobody want to annoy the SPDK project.
> > > I understand that the changes effectively add troubles, and I am sorry
> about
> > > that. If SPDK and other projects can manage with this change, good.
> > > If there is a real blocker, we should discuss what are the options.
> > >
> > > Thanks for your understanding
> >
> > I completely understand the desire to make the ABI manageable. If I were in
> your shoes, I'd be doing the same exact thing. What I don't currently
> understand is the motivation behind this enable_driver_sdk option. My guess is
> that it's one of two things.
> >
> > \1 ABI manageability: You say that's the purpose above, and that was my
> initial assumption. But wouldn't that necessarily mean, over time, no longer
> considering the symbols that were defined by the header files as part of the
> stable ABI?
> 
> Absolutely. The idea is that we don't guarantee ABI for the drivers.
> 
> > If you still consider these symbols as part of the ABI in shared library
> builds, then the enable_driver_sdk option does absolutely nothing to improve
> the ABI situation, so why bother to have it at all? We can't have packaged
> SPDK relying on symbols in a packaged DPDK that are not part of the official
> ABI.
> 
> > \2 Not supporting out-of-tree drivers: Another option is that you just don't
> want people writing out of tree drivers.
> 
> We don't want complications due to support of out-of-tree drivers,
> but we don't want to forbid them.
> 
> > You can't just drop it outright because people already do it,
> > but you'd like to not support it for shared library builds at least.
> 
> I didn't think about it in these terms.
> But saying we don't offer compatibility for shared library drivers
> is not too far of "no support" indeed.
> 
> > So I'd like to really understand which of these two motivated the
> enable_driver_sdk option . Maybe it's not even one of the two above. If it is
> #1, then I think maybe we can work with DPDK to define a very small set of
> out-of-tree driver APIs/ABIs that need to continue to exist in the shared
> libraries by default. I do think SPDK needs only a very small number. If it's
> #2, then that's the entire SPDK use case and I'd ask you to reconsider the
> direction.
> 
> Yes I think we need to agree on functions to keep as-is for compatibility.
> Waiting for your input please.

So, do you mean currently DPDK doesn't guarantee ABI for drivers but could have
driver ABI in the future?

Thanks,
Chenbo

>
Thomas Monjalon Oct. 14, 2021, 6:41 a.m. UTC | #21 
14/10/2021 04:21, Xia, Chenbo:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 13/10/2021 19:56, Walker, Benjamin:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > >
> > > > In order to be perfectly clear, all the changes done around this option
> > > > enable_driver_sdk share the goal of tidying stuff in DPDK so that ABI
> > becomes
> > > > better manageable.
> > > > I think that nobody want to annoy the SPDK project.
> > > > I understand that the changes effectively add troubles, and I am sorry
> > about
> > > > that. If SPDK and other projects can manage with this change, good.
> > > > If there is a real blocker, we should discuss what are the options.
> > > >
> > > > Thanks for your understanding
> > >
> > > I completely understand the desire to make the ABI manageable. If I were in
> > your shoes, I'd be doing the same exact thing. What I don't currently
> > understand is the motivation behind this enable_driver_sdk option. My guess is
> > that it's one of two things.
> > >
> > > \1 ABI manageability: You say that's the purpose above, and that was my
> > initial assumption. But wouldn't that necessarily mean, over time, no longer
> > considering the symbols that were defined by the header files as part of the
> > stable ABI?
> > 
> > Absolutely. The idea is that we don't guarantee ABI for the drivers.
> > 
> > > If you still consider these symbols as part of the ABI in shared library
> > builds, then the enable_driver_sdk option does absolutely nothing to improve
> > the ABI situation, so why bother to have it at all? We can't have packaged
> > SPDK relying on symbols in a packaged DPDK that are not part of the official
> > ABI.
> > 
> > > \2 Not supporting out-of-tree drivers: Another option is that you just don't
> > want people writing out of tree drivers.
> > 
> > We don't want complications due to support of out-of-tree drivers,
> > but we don't want to forbid them.
> > 
> > > You can't just drop it outright because people already do it,
> > > but you'd like to not support it for shared library builds at least.
> > 
> > I didn't think about it in these terms.
> > But saying we don't offer compatibility for shared library drivers
> > is not too far of "no support" indeed.
> > 
> > > So I'd like to really understand which of these two motivated the
> > enable_driver_sdk option . Maybe it's not even one of the two above. If it is
> > #1, then I think maybe we can work with DPDK to define a very small set of
> > out-of-tree driver APIs/ABIs that need to continue to exist in the shared
> > libraries by default. I do think SPDK needs only a very small number. If it's
> > #2, then that's the entire SPDK use case and I'd ask you to reconsider the
> > direction.
> > 
> > Yes I think we need to agree on functions to keep as-is for compatibility.
> > Waiting for your input please.
> 
> So, do you mean currently DPDK doesn't guarantee ABI for drivers

Yes

> but could have driver ABI in the future?

I don't think so, not general compatibility,
but we can think about a way to avoid breaking SPDK specifically,
which has less requirements.
Thomas Monjalon Oct. 14, 2021, 6:41 a.m. UTC | #22 
14/10/2021 00:48, Walker, Benjamin:
> > From: Thomas Monjalon <thomas@monjalon.net>
>  
> > Yes I think we need to agree on functions to keep as-is for compatibility.
> > Waiting for your input please.
> 
> We've added a task to our backlog to propose a stable ABI for out of tree drivers here. It's not as simple as just keeping a couple of the existing functions - we're currently manipulating structures directly. We'll need to work a bit to design the simplest possible set of functions that we need.

OK thanks
Chenbo Xia Oct. 14, 2021, 7 a.m. UTC | #23 
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, October 14, 2021 2:42 PM
> To: Harris, James R <james.r.harris@intel.com>; Walker, Benjamin
> <benjamin.walker@intel.com>; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: Liu, Changpeng <changpeng.liu@intel.com>; David Marchand
> <david.marchand@redhat.com>; dev@dpdk.org; Aaron Conole <aconole@redhat.com>;
> Zawadzki, Tomasz <tomasz.zawadzki@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 0/7] Removal of PCI bus ABIs
> 
> 14/10/2021 04:21, Xia, Chenbo:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 13/10/2021 19:56, Walker, Benjamin:
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > >
> > > > > In order to be perfectly clear, all the changes done around this
> option
> > > > > enable_driver_sdk share the goal of tidying stuff in DPDK so that ABI
> > > becomes
> > > > > better manageable.
> > > > > I think that nobody want to annoy the SPDK project.
> > > > > I understand that the changes effectively add troubles, and I am sorry
> > > about
> > > > > that. If SPDK and other projects can manage with this change, good.
> > > > > If there is a real blocker, we should discuss what are the options.
> > > > >
> > > > > Thanks for your understanding
> > > >
> > > > I completely understand the desire to make the ABI manageable. If I were
> in
> > > your shoes, I'd be doing the same exact thing. What I don't currently
> > > understand is the motivation behind this enable_driver_sdk option. My
> guess is
> > > that it's one of two things.
> > > >
> > > > \1 ABI manageability: You say that's the purpose above, and that was my
> > > initial assumption. But wouldn't that necessarily mean, over time, no
> longer
> > > considering the symbols that were defined by the header files as part of
> the
> > > stable ABI?
> > >
> > > Absolutely. The idea is that we don't guarantee ABI for the drivers.
> > >
> > > > If you still consider these symbols as part of the ABI in shared library
> > > builds, then the enable_driver_sdk option does absolutely nothing to
> improve
> > > the ABI situation, so why bother to have it at all? We can't have packaged
> > > SPDK relying on symbols in a packaged DPDK that are not part of the
> official
> > > ABI.
> > >
> > > > \2 Not supporting out-of-tree drivers: Another option is that you just
> don't
> > > want people writing out of tree drivers.
> > >
> > > We don't want complications due to support of out-of-tree drivers,
> > > but we don't want to forbid them.
> > >
> > > > You can't just drop it outright because people already do it,
> > > > but you'd like to not support it for shared library builds at least.
> > >
> > > I didn't think about it in these terms.
> > > But saying we don't offer compatibility for shared library drivers
> > > is not too far of "no support" indeed.
> > >
> > > > So I'd like to really understand which of these two motivated the
> > > enable_driver_sdk option . Maybe it's not even one of the two above. If it
> is
> > > #1, then I think maybe we can work with DPDK to define a very small set of
> > > out-of-tree driver APIs/ABIs that need to continue to exist in the shared
> > > libraries by default. I do think SPDK needs only a very small number. If
> it's
> > > #2, then that's the entire SPDK use case and I'd ask you to reconsider the
> > > direction.
> > >
> > > Yes I think we need to agree on functions to keep as-is for compatibility.
> > > Waiting for your input please.
> >
> > So, do you mean currently DPDK doesn't guarantee ABI for drivers
> 
> Yes
> 
> > but could have driver ABI in the future?
> 
> I don't think so, not general compatibility,
> but we can think about a way to avoid breaking SPDK specifically,
> which has less requirements.

So the problem here is exposing some APIs to SPDK directly? Without the 'enable_driver_sdk'
option, I don't see a solution of both exposed and not-ABI. Any idea in your mind?

Thanks,
Chenbo

> 
>
Thomas Monjalon Oct. 14, 2021, 7:07 a.m. UTC | #24 
14/10/2021 09:00, Xia, Chenbo:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 14/10/2021 04:21, Xia, Chenbo:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > Yes I think we need to agree on functions to keep as-is for compatibility.
> > > > Waiting for your input please.
> > >
> > > So, do you mean currently DPDK doesn't guarantee ABI for drivers
> > 
> > Yes
> > 
> > > but could have driver ABI in the future?
> > 
> > I don't think so, not general compatibility,
> > but we can think about a way to avoid breaking SPDK specifically,
> > which has less requirements.
> 
> So the problem here is exposing some APIs to SPDK directly? Without the 'enable_driver_sdk'
> option, I don't see a solution of both exposed and not-ABI. Any idea in your mind?

No the idea is to keep using enable_driver_sdk.
But so far, there is no compatibility guarantee for driver SDK.
The discussion is about which basic compatibility requirement is needed for SPDK.
Chenbo Xia Oct. 14, 2021, 8:07 a.m. UTC | #25 
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, October 14, 2021 3:08 PM
> To: Harris, James R <james.r.harris@intel.com>; Walker, Benjamin
> <benjamin.walker@intel.com>; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: Liu, Changpeng <changpeng.liu@intel.com>; David Marchand
> <david.marchand@redhat.com>; dev@dpdk.org; Aaron Conole <aconole@redhat.com>;
> Zawadzki, Tomasz <tomasz.zawadzki@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 0/7] Removal of PCI bus ABIs
> 
> 14/10/2021 09:00, Xia, Chenbo:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 14/10/2021 04:21, Xia, Chenbo:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > Yes I think we need to agree on functions to keep as-is for
> compatibility.
> > > > > Waiting for your input please.
> > > >
> > > > So, do you mean currently DPDK doesn't guarantee ABI for drivers
> > >
> > > Yes
> > >
> > > > but could have driver ABI in the future?
> > >
> > > I don't think so, not general compatibility,
> > > but we can think about a way to avoid breaking SPDK specifically,
> > > which has less requirements.
> >
> > So the problem here is exposing some APIs to SPDK directly? Without the
> 'enable_driver_sdk'
> > option, I don't see a solution of both exposed and not-ABI. Any idea in your
> mind?
> 
> No the idea is to keep using enable_driver_sdk.
> But so far, there is no compatibility guarantee for driver SDK.
> The discussion is about which basic compatibility requirement is needed for
> SPDK.

Sorry for not understanding your point quickly, but what's the difference of
'general compatibility' and 'basic compatibility'? Because in my mind, one
struct or function should either be ABI-compatible or not. Could you help explain
it a bit?

Thanks,
Chenbo

> 
>
Thomas Monjalon Oct. 14, 2021, 8:25 a.m. UTC | #26 
14/10/2021 10:07, Xia, Chenbo:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 14/10/2021 09:00, Xia, Chenbo:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 14/10/2021 04:21, Xia, Chenbo:
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > Yes I think we need to agree on functions to keep as-is for
> > compatibility.
> > > > > > Waiting for your input please.
> > > > >
> > > > > So, do you mean currently DPDK doesn't guarantee ABI for drivers
> > > >
> > > > Yes
> > > >
> > > > > but could have driver ABI in the future?
> > > >
> > > > I don't think so, not general compatibility,
> > > > but we can think about a way to avoid breaking SPDK specifically,
> > > > which has less requirements.
> > >
> > > So the problem here is exposing some APIs to SPDK directly? Without the
> > 'enable_driver_sdk'
> > > option, I don't see a solution of both exposed and not-ABI. Any idea in your
> > mind?
> > 
> > No the idea is to keep using enable_driver_sdk.
> > But so far, there is no compatibility guarantee for driver SDK.
> > The discussion is about which basic compatibility requirement is needed for
> > SPDK.
> 
> Sorry for not understanding your point quickly, but what's the difference of
> 'general compatibility' and 'basic compatibility'? Because in my mind, one
> struct or function should either be ABI-compatible or not. Could you help explain
> it a bit?

I wonder whether we could have a guarantee for a subset of structs and functions.
Anyway, this is just opening the discussion to collect some inputs first.
Then we'll have to check what is possible and get a techboard approval.
Chenbo Xia Oct. 27, 2021, 12:03 p.m. UTC | #27 
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, October 14, 2021 4:26 PM
> To: Harris, James R <james.r.harris@intel.com>; Walker, Benjamin
> <benjamin.walker@intel.com>; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: Liu, Changpeng <changpeng.liu@intel.com>; David Marchand
> <david.marchand@redhat.com>; dev@dpdk.org; Aaron Conole <aconole@redhat.com>;
> Zawadzki, Tomasz <tomasz.zawadzki@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 0/7] Removal of PCI bus ABIs
> 
> 14/10/2021 10:07, Xia, Chenbo:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 14/10/2021 09:00, Xia, Chenbo:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 14/10/2021 04:21, Xia, Chenbo:
> > > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > Yes I think we need to agree on functions to keep as-is for
> > > compatibility.
> > > > > > > Waiting for your input please.
> > > > > >
> > > > > > So, do you mean currently DPDK doesn't guarantee ABI for drivers
> > > > >
> > > > > Yes
> > > > >
> > > > > > but could have driver ABI in the future?
> > > > >
> > > > > I don't think so, not general compatibility,
> > > > > but we can think about a way to avoid breaking SPDK specifically,
> > > > > which has less requirements.
> > > >
> > > > So the problem here is exposing some APIs to SPDK directly? Without the
> > > 'enable_driver_sdk'
> > > > option, I don't see a solution of both exposed and not-ABI. Any idea in
> your
> > > mind?
> > >
> > > No the idea is to keep using enable_driver_sdk.
> > > But so far, there is no compatibility guarantee for driver SDK.
> > > The discussion is about which basic compatibility requirement is needed
> for
> > > SPDK.
> >
> > Sorry for not understanding your point quickly, but what's the difference of
> > 'general compatibility' and 'basic compatibility'? Because in my mind, one
> > struct or function should either be ABI-compatible or not. Could you help
> explain
> > it a bit?
> 
> I wonder whether we could have a guarantee for a subset of structs and
> functions.
> Anyway, this is just opening the discussion to collect some inputs first.
> Then we'll have to check what is possible and get a techboard approval.
> 

After going through related code in SPDK, I think we can add some new functions and keep
some macros in the exposed header (i.e., rte_bus_pci.h) for SPDK to register pci driver
and get needed info.

Most structs/marocs will be hided and SPDK can use the new proposed APIs and small set
of macros/structs to build. In this way, the problem of SPDK building with DPDK distros
and ABI issue can both be solved. APIs like struct rte_pci_device and struct rte_pci_driver
can be hided to minimize pci bus ABI.

Thomas & SPDK folks, please share your opinions of above.

Thanks,
Chenbo
Thomas Monjalon July 11, 2022, 12:11 p.m. UTC | #28 
9 months have passed. Do you have any news from SPDK side?


14/10/2021 08:41, Thomas Monjalon:
> 14/10/2021 00:48, Walker, Benjamin:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> >  
> > > Yes I think we need to agree on functions to keep as-is for compatibility.
> > > Waiting for your input please.
> > 
> > We've added a task to our backlog to propose a stable ABI for out of tree drivers here. It's not as simple as just keeping a couple of the existing functions - we're currently manipulating structures directly. We'll need to work a bit to design the simplest possible set of functions that we need.
> 
> OK thanks