From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id A6DECA0350; Mon, 29 Jun 2020 16:01:47 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1495B1BED0; Mon, 29 Jun 2020 16:01:47 +0200 (CEST) Received: from relay10.mail.gandi.net (relay10.mail.gandi.net [217.70.178.230]) by dpdk.org (Postfix) with ESMTP id 7CCB11BECB for ; Mon, 29 Jun 2020 16:01:46 +0200 (CEST) Received: from u256.net (lfbn-idf2-1-566-132.w86-246.abo.wanadoo.fr [86.246.31.132]) (Authenticated sender: grive@u256.net) by relay10.mail.gandi.net (Postfix) with ESMTPSA id DD166240023; Mon, 29 Jun 2020 14:01:44 +0000 (UTC) Date: Mon, 29 Jun 2020 16:01:39 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Parav Pandit Cc: ferruh.yigit@intel.com, thomas@monjalon.net, dev@dpdk.org, orika@mellanox.com, matan@mellanox.com Message-ID: <20200629140139.3bkqwbfrpho7u664@u256.net> References: <20200610171728.89-2-parav@mellanox.com> <20200621191200.28120-1-parav@mellanox.com> <20200621191200.28120-4-parav@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200621191200.28120-4-parav@mellanox.com> Subject: Re: [dpdk-dev] [PATCH v2 3/6] bus/mlx5_pci: add mlx5 PCI bus X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Parav, On 21/06/20 19:11 +0000, Parav Pandit wrote: > Add mlx5 PCI bus which enables multiple mlx5 drivers to bind to single > pci device. > > Signed-off-by: Parav Pandit > --- > Changelog: > v1->v2: > - Address comments from Thomas and Gaetan > - Inheriting ret_pci_driver instead of rte_driver > - Added design and description of the mlx5_pci bus > --- > config/common_base | 6 ++ > config/defconfig_arm64-bluefield-linuxapp-gcc | 6 ++ > drivers/bus/meson.build | 2 +- > drivers/bus/mlx5_pci/Makefile | 47 +++++++++++ > drivers/bus/mlx5_pci/meson.build | 6 ++ > drivers/bus/mlx5_pci/mlx5_pci_bus.c | 14 ++++ > drivers/bus/mlx5_pci/rte_bus_mlx5_pci.h | 84 +++++++++++++++++++ > .../bus/mlx5_pci/rte_bus_mlx5_pci_version.map | 5 ++ > 8 files changed, 169 insertions(+), 1 deletion(-) > create mode 100644 drivers/bus/mlx5_pci/Makefile > create mode 100644 drivers/bus/mlx5_pci/meson.build > create mode 100644 drivers/bus/mlx5_pci/mlx5_pci_bus.c > create mode 100644 drivers/bus/mlx5_pci/rte_bus_mlx5_pci.h > create mode 100644 drivers/bus/mlx5_pci/rte_bus_mlx5_pci_version.map > > diff --git a/config/common_base b/config/common_base > index c7d5c7321..f75b333f9 100644 > --- a/config/common_base > +++ b/config/common_base > @@ -366,6 +366,12 @@ CONFIG_RTE_LIBRTE_IGC_DEBUG_TX=n > CONFIG_RTE_LIBRTE_MLX4_PMD=n > CONFIG_RTE_LIBRTE_MLX4_DEBUG=n > > +# > +# Compile Mellanox PCI BUS for ConnectX-4, ConnectX-5, > +# ConnectX-6 & BlueField (MLX5) PMD > +# > +CONFIG_RTE_LIBRTE_MLX5_PCI_BUS=n > + I told you that you were already using a construct in Makefiles that was making this config useless, but you insisted to add it. Meson does not need it and you can write your makefiles in such a way, that this bus will be enabled automatically if any of its dependents is enabled. This is the way to go, please remove these config lines. > # > # Compile burst-oriented Mellanox ConnectX-4, ConnectX-5, > # ConnectX-6 & BlueField (MLX5) PMD > diff --git a/config/defconfig_arm64-bluefield-linuxapp-gcc b/config/defconfig_arm64-bluefield-linuxapp-gcc > index b49653881..15ade7ebc 100644 > --- a/config/defconfig_arm64-bluefield-linuxapp-gcc > +++ b/config/defconfig_arm64-bluefield-linuxapp-gcc > @@ -14,5 +14,11 @@ CONFIG_RTE_CACHE_LINE_SIZE=64 > CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n > CONFIG_RTE_LIBRTE_VHOST_NUMA=n > > +# > +# Compile Mellanox PCI BUS for ConnectX-4, ConnectX-5, > +# ConnectX-6 & BlueField (MLX5) PMD > +# > +CONFIG_RTE_LIBRTE_MLX5_PCI_BUS=n > + > # PMD for ConnectX-5 > CONFIG_RTE_LIBRTE_MLX5_PMD=y > diff --git a/drivers/bus/meson.build b/drivers/bus/meson.build > index 80de2d91d..b1381838d 100644 > --- a/drivers/bus/meson.build > +++ b/drivers/bus/meson.build > @@ -1,7 +1,7 @@ > # SPDX-License-Identifier: BSD-3-Clause > # Copyright(c) 2017 Intel Corporation > > -drivers = ['dpaa', 'fslmc', 'ifpga', 'pci', 'vdev', 'vmbus'] > +drivers = ['dpaa', 'fslmc', 'ifpga', 'pci', 'mlx5_pci', 'vdev', 'vmbus'] > std_deps = ['eal'] > config_flag_fmt = 'RTE_LIBRTE_@0@_BUS' > driver_name_fmt = 'rte_bus_@0@' It seems you only added your bus to Meson. I see no modification to drivers/bus/Makefile > diff --git a/drivers/bus/mlx5_pci/Makefile b/drivers/bus/mlx5_pci/Makefile > new file mode 100644 > index 000000000..7db977ba8 > --- /dev/null > +++ b/drivers/bus/mlx5_pci/Makefile > @@ -0,0 +1,47 @@ > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright 2020 Mellanox Technologies, Ltd > + > +include $(RTE_SDK)/mk/rte.vars.mk > + > +# > +# library name > +# > +LIB = librte_bus_mlx5_pci.a > + > +CFLAGS += -O3 > +CFLAGS += $(WERROR_FLAGS) > +CFLAGS += -I$(RTE_SDK)/drivers/common/mlx5 > +CFLAGS += -I$(BUILDDIR)/drivers/common/mlx5 > +CFLAGS += -I$(RTE_SDK)/drivers/bus/pci > +LDLIBS += -lrte_eal > +LDLIBS += -lrte_common_mlx5 > +LDLIBS += -lrte_pci -lrte_bus_pci > + > +# versioning export map > +EXPORT_MAP := rte_bus_mlx5_pci_version.map > + > +SRCS-y += mlx5_pci_bus.c > + > +# DEBUG which is usually provided on the command-line may enable > +# CONFIG_RTE_LIBRTE_MLX5_DEBUG. > +ifeq ($(DEBUG),1) > +CONFIG_RTE_LIBRTE_MLX5_DEBUG := y > +endif > + > +# User-defined CFLAGS. > +ifeq ($(CONFIG_RTE_LIBRTE_MLX5_DEBUG),y) > +CFLAGS += -pedantic > +ifneq ($(CONFIG_RTE_TOOLCHAIN_ICC),y) > +CFLAGS += -DPEDANTIC > +endif > +AUTO_CONFIG_CFLAGS += -Wno-pedantic > +else > +CFLAGS += -UPEDANTIC > +endif > + > +# > +# Export include files > +# > +SYMLINK-y-include += rte_bus_mlx5_pci.h > + > +include $(RTE_SDK)/mk/rte.lib.mk > diff --git a/drivers/bus/mlx5_pci/meson.build b/drivers/bus/mlx5_pci/meson.build > new file mode 100644 > index 000000000..cc4a84e23 > --- /dev/null > +++ b/drivers/bus/mlx5_pci/meson.build > @@ -0,0 +1,6 @@ > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright(c) 2020 Mellanox Technologies Ltd > + > +deps += ['pci', 'bus_pci', 'common_mlx5'] > +install_headers('rte_bus_mlx5_pci.h') > +sources = files('mlx5_pci_bus.c') > diff --git a/drivers/bus/mlx5_pci/mlx5_pci_bus.c b/drivers/bus/mlx5_pci/mlx5_pci_bus.c > new file mode 100644 > index 000000000..66db3c7b0 > --- /dev/null > +++ b/drivers/bus/mlx5_pci/mlx5_pci_bus.c > @@ -0,0 +1,14 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright 2020 Mellanox Technologies, Ltd > + */ > + > +#include "rte_bus_mlx5_pci.h" > + > +static TAILQ_HEAD(mlx5_pci_bus_drv_head, rte_mlx5_pci_driver) drv_list = > + TAILQ_HEAD_INITIALIZER(drv_list); > + > +void > +rte_mlx5_pci_driver_register(struct rte_mlx5_pci_driver *driver) > +{ > + TAILQ_INSERT_TAIL(&drv_list, driver, next); > +} > diff --git a/drivers/bus/mlx5_pci/rte_bus_mlx5_pci.h b/drivers/bus/mlx5_pci/rte_bus_mlx5_pci.h > new file mode 100644 > index 000000000..571f7dfd6 > --- /dev/null > +++ b/drivers/bus/mlx5_pci/rte_bus_mlx5_pci.h > @@ -0,0 +1,84 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright 2020 Mellanox Technologies, Ltd > + */ > + > +#ifndef _RTE_BUS_MLX5_PCI_H_ > +#define _RTE_BUS_MLX5_PCI_H_ > + > +/** > + * @file > + * > + * RTE Mellanox PCI Bus Interface > + * Mellanox ConnectX PCI device supports multiple class (net/vdpa/regex) ^ this should only appear in doc once supported > + * devices. This bus enables creating such multiple class of devices on a > + * single PCI device by allowing to bind multiple class specific device > + * driver to attach to mlx5_pci bus driver. Formulation is not great here. Here's a fix: Mellanox ConnectX PCI devices can be used in several ways, each represented by a device class (net, VDPA). This bus allows using multiple device classes with a single PCI device. Currently only net and VDPA classes exists and no combination of the two is allowed. New classes are expected to be introduced, this bus is the groundwork for their support. This last paragraph is to be removed once regex dev are supported, when combinations becomes valid and it's not future anymore. It should then be replaced by a description of the rules to combine several classes. > + * > + * ----------- ------------ ----------------- > + * | mlx5 | | mlx5 | | mlx5 | > + * | net pmd | | vdpa pmd | | new class pmd | > + * ----------- ------------ ----------------- > + * \ | / > + * \ | / > + * \ ------------- / > + * \______| mlx5 |_____ / > + * | pci bus | > + * ------------- ^^^-- trailing spaces here > + * | > + * ----------- < > + * | mlx5 | < > + * | pci dev | < > + * ----------- <--- trailing spaces here > + * > + * - mlx5 pci bus driver binds to mlx5 PCI devices defined by PCI > + * ID table of all related mlx5 PCI devices. > + * - mlx5 class driver such as net, vdpa, regex PMD defines its > + * specific PCI ID table and mlx5 bus driver probes matching > + * class drivers. > + * - mlx5 pci bus driver is cental place that validates supported > + * class combinations. Multiple formulation issues, simpler for me to rewrite: (please leave the empty newlines between each points.) - The mlx5 PCI bus driver binds to each PCI devices matching a PCI ID within the mlx5 table of supported PCI devices. - mlx5 class drivers such as net or VDPA PMD define their supported PCI ID table. The mlx5 bus driver will probe each matching class driver. - The mlx5 PCI bus driver is the central place validating the rules to combine and match multiple device classes. > + */ > + > +#ifdef __cplusplus > +extern "C" { > +#endif /* __cplusplus */ > + > +#include > +#include > + > +#include > + > +/** > + * A structure describing a mlx5 pci driver. > + */ > +struct rte_mlx5_pci_driver { > + enum mlx5_class dev_class; /**< Class of this driver */ ^^^^^^^^^^^^^^^ ^ ^ missing period here. \ \ \ Please do not align doc elements in your structs, \ they will be awkward to edit later when adding long-name fields. \ enums width is implementation defined, I thinks it's better to use uint32_t instead. Especially considering you use offsetof-based stuff after (TAILQ). > + struct rte_pci_driver pci_driver; /**< Inherit core pci driver. */ > + TAILQ_ENTRY(rte_mlx5_pci_driver) next; > +}; > + > +/** > + * Register a mlx5_pci device driver. > + * > + * @param driver > + * A pointer to a rte_mlx5_pci_driver structure describing the driver > + * to be registered. > + */ > +__rte_internal > +void > +rte_mlx5_pci_driver_register(struct rte_mlx5_pci_driver *driver); > + > +#define RTE_PMD_REGISTER_MLX5_PCI(nm, drv) \ > +static const char *mlx5_pci_drvinit_fn_ ## nm; \ > +RTE_INIT(mlx5_pci_drvinit_fn_ ##drv) \ > +{ \ > + (drv).driver.name = RTE_STR(nm); \ > + rte_mlx5_pci_driver_register(&drv); \ > +} \ > +RTE_PMD_EXPORT_NAME(nm, __COUNTER__) > + > +#ifdef __cplusplus > +} > +#endif /* __cplusplus */ > + > +#endif /* _RTE_BUS_MLX5_PCI_H_ */ > diff --git a/drivers/bus/mlx5_pci/rte_bus_mlx5_pci_version.map b/drivers/bus/mlx5_pci/rte_bus_mlx5_pci_version.map > new file mode 100644 > index 000000000..4cfd3db10 > --- /dev/null > +++ b/drivers/bus/mlx5_pci/rte_bus_mlx5_pci_version.map > @@ -0,0 +1,5 @@ > +INTERNAL { > + global: > + > + rte_mlx5_pci_driver_register; > +}; > -- > 2.25.4 > -- Gaëtan