Re: [dpdk-ci] [dpdk-dev] [PATCH v2 0/7] Removal of PCI bus ABIs

["Xia, Chenbo" <chenbo.xia@intel.com>]

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
>