Re: [dpdk-dev] [PATCH v2 3/6] bus/mlx5_pci: add mlx5 PCI bus

["Gaëtan Rivet" <grive@u256.net>]

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 <parav@mellanox.com>
> ---
> 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 <rte_pci.h>
> +#include <rte_bus_pci.h>
> +
> +#include <mlx5_common.h>
> +
> +/**
> + * 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