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 E0A8DA04A3; Mon, 15 Jun 2020 23:00:51 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E7BFC2B94; Mon, 15 Jun 2020 23:00:50 +0200 (CEST) Received: from relay12.mail.gandi.net (relay12.mail.gandi.net [217.70.178.232]) by dpdk.org (Postfix) with ESMTP id 1DF112B87 for ; Mon, 15 Jun 2020 23:00:50 +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 relay12.mail.gandi.net (Postfix) with ESMTPSA id 9BA73200004; Mon, 15 Jun 2020 21:00:48 +0000 (UTC) Date: Mon, 15 Jun 2020 23:00:42 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Parav Pandit Cc: dev@dpdk.org, ferruh.yigit@intel.com, thomasm@mellanox.com, orika@mellanox.com, matan@mellanox.com Message-ID: <20200615210042.gblrdp6rus6exooy@u256.net> References: <20200610171728.89-1-parav@mellanox.com> <20200610171728.89-5-parav@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200610171728.89-5-parav@mellanox.com> Subject: Re: [dpdk-dev] [RFC PATCH 4/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" On 10/06/20 17:17 +0000, Parav Pandit wrote: > Add mlx5 PCI bus which enables multiple mlx5 drivers to bind to single > pci device. > This is a little quick to explain the architecture here. First it should be clear that this is not, in fact, a bus. You only define a PCI driver, that will demux PCI ops towards several sub-drivers. We can call it a bus in the sense that it will support multiple devices and carry on some control, but this should be made clear here that no rte_bus is being added. > Signed-off-by: Parav Pandit > --- > config/common_base | 6 ++ > config/defconfig_arm64-bluefield-linuxapp-gcc | 6 ++ > drivers/bus/meson.build | 2 +- > drivers/bus/mlx5_pci/Makefile | 48 ++++++++++++++ > 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 | 65 +++++++++++++++++++ > .../bus/mlx5_pci/rte_bus_mlx5_pci_version.map | 5 ++ > 8 files changed, 151 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'm not a fan of having yet another CONFIG_ toggle to enable in build systems. Ideally, this should be enabled implicitly by enabling any of its dependents (MLX5 PMD, MLX5_VDPA_PMD, REGEX I guess, etc). You can find such similar constructs already in some makefiles: mk/rte.app.mk:204:ifeq ($(findstring y,$(CONFIG_RTE_LIBRTE_MLX5_PMD)$(CONFIG_RTE_LIBRTE_MLX5_VDPA_PMD)),y) Actually, reading further commits, you already use this construct when you enable the build for VDPA and MLX5 PMDs, I think this option is not needed then? > # > # 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@' > diff --git a/drivers/bus/mlx5_pci/Makefile b/drivers/bus/mlx5_pci/Makefile > new file mode 100644 > index 000000000..b36916e52 > --- /dev/null > +++ b/drivers/bus/mlx5_pci/Makefile > @@ -0,0 +1,48 @@ > +# 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 > +CFLAGS += -Wno-strict-prototypes Why no-strict-prototypes by the way? > +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 > + At this point why not define some $(RTE_SDK)/drivers/common/mlx5/mlx5_common.mk That should be included by vdpa, mlx5, this one? This would force-align flag behavior, this is becoming untidy. (Make is disappearing soon I heard, but still.) > +# > +# 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..b0423f99e > --- /dev/null > +++ b/drivers/bus/mlx5_pci/rte_bus_mlx5_pci.h > @@ -0,0 +1,65 @@ > +/* 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) > + * 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. > + */ I think it would be better to explain that this bus is mostly a PCI driver demuxing to several device classes (you could copy here the explanation you'd write in the commit log). > + > +#ifdef __cplusplus > +extern "C" { > +#endif /* __cplusplus */ > + > +#include > +#include > + > +#include > + > +/** > + * A structure describing a mlx5 pci driver. > + */ > +struct rte_mlx5_pci_driver { A note on the namespace: rte_mlx5_pci seems heavy. Do you expect other types of "super-driver", other than PCI? Wouldn't rte_mlx5_driver be ok for example? > + enum mlx5_class dev_class; /**< Class of this driver */ > + struct rte_driver driver; /**< Inherit core driver. */ > + pci_probe_t *probe; /**< Class device probe function. */ > + pci_remove_t *remove; /**< Class device remove function. */ > + pci_dma_map_t *dma_map; /**< Class device dma map function. */ > + pci_dma_unmap_t *dma_unmap; /**< Class device dma unmap function. */ > + TAILQ_ENTRY(rte_mlx5_pci_driver) next; > + const struct rte_pci_id *id_table; /**< ID table, NULL terminated. */ At this point, why not inherit an rte_pci_driver instead of the core rte_driver? > +}; > + > +/** > + * 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_ */ I'm not sure something is gained by cutting this definition here. You added the build system but this is an empty shell. Why not merge this commit and the next? > 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