Patch Comment List
List patch comments
GET /api/patches/74698/comments/?format=api
[ { "id": 116615, "web_url": "https://patches.dpdk.org/comment/116615/", "msgid": "<CAJFAV8xT0Uo9b73SfpA3Tt15A1K5+0BgLnNMuehbx5jyjtNKwg@mail.gmail.com>", "list_archive_url": "https://inbox.dpdk.org/dev/CAJFAV8xT0Uo9b73SfpA3Tt15A1K5+0BgLnNMuehbx5jyjtNKwg@mail.gmail.com", "date": "2020-07-24T14:40:05", "subject": "Re: [dpdk-dev] [PATCH v8 08/10] common/mlx5: introduce layer to\n support multiple class drivers", "submitter": { "id": 1173, "url": "https://patches.dpdk.org/api/people/1173/?format=api", "name": "David Marchand", "email": "david.marchand@redhat.com" }, "content": "This is not a thorough review, just caught one issue for makefile and some nits.\n\n\nOn Thu, Jul 23, 2020 at 10:11 PM Parav Pandit <parav@mellanox.com> wrote:\n>\n> From: Thomas Monjalon <thomas@monjalon.net>\n>\n> Add generic mlx5 PCI PMD layer as part of existing common_mlx5\n> module. This enables multiple classes (net, regex, vdpa) PMDs\n> to be supported at same time.\n>\n> Signed-off-by: Parav Pandit <parav@mellanox.com>\n> Acked-by: Matan Azrad <matan@mellanox.com>\n> ---\n> drivers/Makefile | 10 +-\n> drivers/common/Makefile | 4 -\n> drivers/common/meson.build | 2 +-\n> drivers/common/mlx5/Makefile | 1 +\n> drivers/common/mlx5/meson.build | 9 +-\n> drivers/common/mlx5/mlx5_common.c | 2 +\n> drivers/common/mlx5/mlx5_common_pci.c | 541 ++++++++++++++++++\n> drivers/common/mlx5/mlx5_common_pci.h | 77 +++\n> .../common/mlx5/rte_common_mlx5_version.map | 2 +\n> drivers/meson.build | 1 +\n> 10 files changed, 638 insertions(+), 11 deletions(-)\n> create mode 100644 drivers/common/mlx5/mlx5_common_pci.c\n> create mode 100644 drivers/common/mlx5/mlx5_common_pci.h\n>\n> diff --git a/drivers/Makefile b/drivers/Makefile\n> index 1551272ef..7f06162dc 100644\n> --- a/drivers/Makefile\n> +++ b/drivers/Makefile\n> @@ -8,8 +8,12 @@ DIRS-y += bus\n> DEPDIRS-bus := common\n> DIRS-y += mempool\n> DEPDIRS-mempool := common bus\n> +ifeq ($(findstring y,$(CONFIG_RTE_LIBRTE_MLX5_PMD)$(CONFIG_RTE_LIBRTE_MLX5_VDPA_PMD)$(CONFIG_RTE_LIBRTE_MLX5_REGEX_PMD)),y)\n> +DIRS-y += common/mlx5\n> +DEPDIRS-common/mlx5 := bus\n> +endif\n> DIRS-y += net\n> -DEPDIRS-net := common bus mempool\n> +DEPDIRS-net := common bus mempool common/mlx5\n\nWill it still work for people not building with mlx5?\n\nI would have seen something like:\nDEPDIRS-net/mlx5 := xx xx xx common/mlx5\nOr maybe\nDEPDIRS-net/mlx5 += common/mlx5\n\n\n> DIRS-$(CONFIG_RTE_LIBRTE_BBDEV) += baseband\n> DEPDIRS-baseband := common bus mempool\n> DIRS-$(CONFIG_RTE_LIBRTE_CRYPTODEV) += crypto\n> @@ -19,9 +23,9 @@ DEPDIRS-common/qat := bus mempool\n> DIRS-$(CONFIG_RTE_LIBRTE_COMPRESSDEV) += compress\n> DEPDIRS-compress := bus mempool\n> DIRS-$(CONFIG_RTE_LIBRTE_REGEXDEV) += regex\n> -DEPDIRS-regex := common bus\n> +DEPDIRS-regex := common bus common/mlx5\n\nDEPDIRS-regex/mlx5 := xx xx xx common/mlx5\n\n> DIRS-$(CONFIG_RTE_LIBRTE_VHOST) += vdpa\n> -DEPDIRS-vdpa := common bus mempool\n> +DEPDIRS-vdpa := common bus mempool common/mlx5\n\nIdem.\n\n\n> DIRS-$(CONFIG_RTE_LIBRTE_EVENTDEV) += event\n> DEPDIRS-event := common bus mempool net crypto\n> DIRS-$(CONFIG_RTE_LIBRTE_RAWDEV) += raw\n> diff --git a/drivers/common/Makefile b/drivers/common/Makefile\n> index cbc71077c..cfb6b4dc8 100644\n> --- a/drivers/common/Makefile\n> +++ b/drivers/common/Makefile\n> @@ -36,8 +36,4 @@ ifneq (,$(findstring y,$(IAVF-y)))\n> DIRS-y += iavf\n> endif\n>\n> -ifeq ($(findstring y,$(CONFIG_RTE_LIBRTE_MLX5_PMD)$(CONFIG_RTE_LIBRTE_MLX5_VDPA_PMD)$(CONFIG_RTE_LIBRTE_MLX5_REGEX_PMD)),y)\n> -DIRS-y += mlx5\n> -endif\n> -\n> include $(RTE_SDK)/mk/rte.subdir.mk\n> diff --git a/drivers/common/meson.build b/drivers/common/meson.build\n> index 5db7e29b1..9ed4c04ba 100644\n> --- a/drivers/common/meson.build\n> +++ b/drivers/common/meson.build\n> @@ -6,6 +6,6 @@ if is_windows\n> endif\n>\n> std_deps = ['eal']\n> -drivers = ['cpt', 'dpaax', 'iavf', 'mlx5', 'mvep', 'octeontx', 'octeontx2', 'qat']\n> +drivers = ['cpt', 'dpaax', 'iavf', 'mvep', 'octeontx', 'octeontx2', 'qat']\n> config_flag_fmt = 'RTE_LIBRTE_@0@_COMMON'\n> driver_name_fmt = 'rte_common_@0@'\n> diff --git a/drivers/common/mlx5/Makefile b/drivers/common/mlx5/Makefile\n> index 6b89a6c85..eaabb8c10 100644\n> --- a/drivers/common/mlx5/Makefile\n> +++ b/drivers/common/mlx5/Makefile\n> @@ -22,6 +22,7 @@ SRCS-y += linux/mlx5_common_verbs.c\n> SRCS-y += mlx5_common_mp.c\n> SRCS-y += mlx5_common_mr.c\n> SRCS-y += mlx5_malloc.c\n> +SRCS-y += mlx5_common_pci.c\n\nAlphabetical order?\n\n\n> ifeq ($(CONFIG_RTE_IBVERBS_LINK_DLOPEN),y)\n> INSTALL-y-lib += $(LIB_GLUE)\n> endif\n> diff --git a/drivers/common/mlx5/meson.build b/drivers/common/mlx5/meson.build\n> index 70e2c1c32..8e5608703 100644\n> --- a/drivers/common/mlx5/meson.build\n> +++ b/drivers/common/mlx5/meson.build\n> @@ -1,19 +1,22 @@\n> # SPDX-License-Identifier: BSD-3-Clause\n> # Copyright 2019 Mellanox Technologies, Ltd\n>\n> -if not (is_linux or is_windows)\n> +if not is_linux\n> build = false\n> - reason = 'only supported on Linux and Windows'\n> + reason = 'only supported on Linux'\n> subdir_done()\n> endif\n>\n> -deps += ['hash', 'pci', 'net', 'eal', 'kvargs']\n> +config_flag_fmt = 'RTE_LIBRTE_@0@_COMMON'\n> +driver_name_fmt = 'rte_common_@0@'\n> +deps += ['hash', 'pci', 'bus_pci', 'net', 'eal', 'kvargs']\n> sources += files(\n> 'mlx5_devx_cmds.c',\n> 'mlx5_common.c',\n> 'mlx5_common_mp.c',\n> 'mlx5_common_mr.c',\n> 'mlx5_malloc.c',\n> + 'mlx5_common_pci.c',\n\nIdem order.\n\n\n> )\n>\n> cflags_options = [\n> diff --git a/drivers/common/mlx5/mlx5_common.c b/drivers/common/mlx5/mlx5_common.c\n> index 2b336bb2d..fd818ef24 100644\n> --- a/drivers/common/mlx5/mlx5_common.c\n> +++ b/drivers/common/mlx5/mlx5_common.c\n> @@ -14,6 +14,7 @@\n> #include \"mlx5_common_os.h\"\n> #include \"mlx5_common_utils.h\"\n> #include \"mlx5_malloc.h\"\n> +#include \"mlx5_common_pci.h\"\n\nIdem order.\n\n\n>\n> int mlx5_common_logtype;\n>\n> @@ -100,6 +101,7 @@ mlx5_common_init(void)\n> return;\n>\n> mlx5_glue_constructor();\n> + mlx5_common_pci_init();\n> mlx5_common_initialized = true;\n> }\n>\n> diff --git a/drivers/common/mlx5/mlx5_common_pci.c b/drivers/common/mlx5/mlx5_common_pci.c\n> new file mode 100644\n> index 000000000..b42f7469f\n> --- /dev/null\n> +++ b/drivers/common/mlx5/mlx5_common_pci.c\n> @@ -0,0 +1,541 @@\n> +/* SPDX-License-Identifier: BSD-3-Clause\n> + * Copyright 2020 Mellanox Technologies Ltd\n> + */\n> +\n> +#include <stdlib.h>\n\nNit: headers usually come in separate blocks, \"system headers\", \"dpdk\nheaders\", \"drivers headers\".\nSo missing an empty line here.\n\n> +#include <rte_malloc.h>\n\n\n> +#include \"mlx5_common_utils.h\"\n> +#include \"mlx5_common_pci.h\"\n> +\n> +struct mlx5_pci_device {\n> + struct rte_pci_device *pci_dev;\n> + TAILQ_ENTRY(mlx5_pci_device) next;\n> + uint32_t classes_loaded;\n> +};\n> +\n> +/* Head of list of drivers. */\n> +static TAILQ_HEAD(mlx5_pci_bus_drv_head, mlx5_pci_driver) drv_list =\n> + TAILQ_HEAD_INITIALIZER(drv_list);\n> +\n> +/* Head of mlx5 pci devices. */\n> +static TAILQ_HEAD(mlx5_pci_devices_head, mlx5_pci_device) devices_list =\n> + TAILQ_HEAD_INITIALIZER(devices_list);\n> +\n> +static const struct {\n> + const char *name;\n> + unsigned int driver_class;\n> +} mlx5_classes[] = {\n> + { .name = \"vdpa\", .driver_class = MLX5_CLASS_VDPA },\n> + { .name = \"net\", .driver_class = MLX5_CLASS_NET },\n> + { .name = \"regex\", .driver_class = MLX5_CLASS_REGEX },\n> +};\n> +\n> +static const unsigned int mlx5_class_combinations[] = {\n> + MLX5_CLASS_NET,\n> + MLX5_CLASS_VDPA,\n> + MLX5_CLASS_REGEX,\n> + /* New class combination should be added here.\n> + * For example a new multi class device combination\n> + * can be MLX5_CLASS_FOO | MLX5_CLASS_BAR.\n> + */\n> +};\n> +\n> +static int\n> +class_name_to_value(const char *class_name)\n> +{\n> + unsigned int i;\n> +\n> + for (i = 0; i < RTE_DIM(mlx5_classes); i++) {\n> + if (strcmp(class_name, mlx5_classes[i].name) == 0)\n> + return mlx5_classes[i].driver_class;\n> + }\n> + return -EINVAL;\n> +}\n> +\n> +static struct mlx5_pci_driver *\n> +driver_get(uint32_t class)\n> +{\n> + struct mlx5_pci_driver *driver;\n> +\n> + TAILQ_FOREACH(driver, &drv_list, next) {\n> + if (driver->driver_class == class)\n> + return driver;\n> + }\n> + return NULL;\n> +}\n> +\n> +static int\n> +bus_cmdline_options_handler(__rte_unused const char *key,\n> + const char *class_names, void *opaque)\n> +{\n> + int *ret = opaque;\n> + char *nstr_org;\n> + int class_val;\n> + char *found;\n> + char *nstr;\n> +\n> + *ret = 0;\n> + nstr = strdup(class_names);\n> + if (!nstr) {\n> + *ret = -ENOMEM;\n> + return *ret;\n> + }\n> + nstr_org = nstr;\n> + while (nstr) {\n> + /* Extract each individual class name. Multiple\n> + * class key,value is supplied as class=net:vdpa:foo:bar.\n> + */\n> + found = strsep(&nstr, \":\");\n> + if (!found)\n> + continue;\n> + /* Check if its a valid class. */\n> + class_val = class_name_to_value(found);\n> + if (class_val < 0) {\n> + *ret = -EINVAL;\n> + goto err;\n> + }\n> + *ret |= class_val;\n> + }\n> +err:\n> + free(nstr_org);\n> + if (*ret < 0)\n> + DRV_LOG(ERR, \"Invalid mlx5 class options %s.\"\n> + \" Maybe typo in device class argument setting?\",\n> + class_names);\n> + return *ret;\n> +}\n> +\n> +static int\n> +parse_class_options(const struct rte_devargs *devargs)\n> +{\n> + const char *key = MLX5_CLASS_ARG_NAME;\n> + struct rte_kvargs *kvlist;\n> + int ret = 0;\n> +\n> + if (devargs == NULL)\n> + return 0;\n> + kvlist = rte_kvargs_parse(devargs->args, NULL);\n> + if (kvlist == NULL)\n> + return 0;\n> + if (rte_kvargs_count(kvlist, key))\n> + rte_kvargs_process(kvlist, key, bus_cmdline_options_handler,\n> + &ret);\n> + rte_kvargs_free(kvlist);\n> + return ret;\n> +}\n> +\n> +static bool\n> +mlx5_bus_match(const struct mlx5_pci_driver *drv,\n> + const struct rte_pci_device *pci_dev)\n> +{\n> + const struct rte_pci_id *id_table;\n> +\n> + for (id_table = drv->pci_driver.id_table; id_table->vendor_id != 0;\n> + id_table++) {\n> + /* Check if device's ids match the class driver's ids. */\n> + if (id_table->vendor_id != pci_dev->id.vendor_id &&\n> + id_table->vendor_id != PCI_ANY_ID)\n> + continue;\n> + if (id_table->device_id != pci_dev->id.device_id &&\n> + id_table->device_id != PCI_ANY_ID)\n> + continue;\n> + if (id_table->subsystem_vendor_id !=\n> + pci_dev->id.subsystem_vendor_id &&\n> + id_table->subsystem_vendor_id != PCI_ANY_ID)\n> + continue;\n> + if (id_table->subsystem_device_id !=\n> + pci_dev->id.subsystem_device_id &&\n> + id_table->subsystem_device_id != PCI_ANY_ID)\n> + continue;\n> + if (id_table->class_id != pci_dev->id.class_id &&\n> + id_table->class_id != RTE_CLASS_ANY_ID)\n> + continue;\n> + return true;\n> + }\n> + return false;\n> +}\n> +\n> +static int\n> +is_valid_class_combination(uint32_t user_classes)\n> +{\n> + unsigned int i;\n> +\n> + /* Verify if user specified valid supported combination. */\n\nAre there invalid supported combinations? :-)\n\n/* Verify if the user specified a supported combination. */\n\n\n> + for (i = 0; i < RTE_DIM(mlx5_class_combinations); i++) {\n> + if (mlx5_class_combinations[i] == user_classes)\n> + return 0;\n> + }\n> + /* Not found any valid class combination. */\n> + return -EINVAL;\n> +}\n> +\n> +static struct mlx5_pci_device *\n> +pci_to_mlx5_device(const struct rte_pci_device *pci_dev)\n> +{\n> + struct mlx5_pci_device *dev;\n> +\n> + TAILQ_FOREACH(dev, &devices_list, next) {\n> + if (dev->pci_dev == pci_dev)\n> + return dev;\n> + }\n> + return NULL;\n> +}\n> +\n> +static bool\n> +device_class_enabled(const struct mlx5_pci_device *device, uint32_t class)\n> +{\n> + return (device->classes_loaded & class) ? true : false;\n> +}\n> +\n> +static void\n> +dev_release(struct mlx5_pci_device *dev)\n> +{\n> + TAILQ_REMOVE(&devices_list, dev, next);\n> + rte_free(dev);\n> +}\n> +\n> +static int\n> +drivers_remove(struct mlx5_pci_device *dev, uint32_t enabled_classes)\n> +{\n> + struct mlx5_pci_driver *driver;\n> + int local_ret = -ENODEV;\n> + unsigned int i = 0;\n> + int ret = 0;\n> +\n> + enabled_classes &= dev->classes_loaded;\n> + while (enabled_classes) {\n> + driver = driver_get(RTE_BIT64(i));\n> + if (driver) {\n> + local_ret = driver->pci_driver.remove(dev->pci_dev);\n> + if (!local_ret)\n> + dev->classes_loaded &= ~RTE_BIT64(i);\n> + else if (ret == 0)\n> + ret = local_ret;\n> + }\n> + enabled_classes &= ~RTE_BIT64(i);\n> + i++;\n> + }\n> + if (local_ret)\n> + ret = local_ret;\n> + return ret;\n> +}\n> +\n> +static int\n> +drivers_probe(struct mlx5_pci_device *dev, struct rte_pci_driver *pci_drv,\n> + struct rte_pci_device *pci_dev, uint32_t user_classes)\n> +{\n> + struct mlx5_pci_driver *driver;\n> + uint32_t enabled_classes = 0;\n> + bool already_loaded;\n> + int ret;\n> +\n> + TAILQ_FOREACH(driver, &drv_list, next) {\n> + if ((driver->driver_class & user_classes) == 0)\n> + continue;\n> + if (!mlx5_bus_match(driver, pci_dev))\n> + continue;\n> + already_loaded = dev->classes_loaded & driver->driver_class;\n> + if (already_loaded &&\n> + !(driver->pci_driver.drv_flags & RTE_PCI_DRV_PROBE_AGAIN)) {\n> + DRV_LOG(ERR, \"Device %s is already probed\\n\",\n> + pci_dev->device.name);\n> + ret = -EEXIST;\n> + goto probe_err;\n> + }\n> + ret = driver->pci_driver.probe(pci_drv, pci_dev);\n> + if (ret < 0) {\n> + DRV_LOG(ERR, \"Failed to load driver = %s.\\n\",\n> + driver->pci_driver.driver.name);\n> + goto probe_err;\n> + }\n> + enabled_classes |= driver->driver_class;\n> + }\n> + dev->classes_loaded |= enabled_classes;\n> + return 0;\n> +probe_err:\n> + /* Only unload drivers which are enabled which were enabled\n> + * in this probe instance.\n> + */\n> + drivers_remove(dev, enabled_classes);\n> + return ret;\n> +}\n> +\n> +/**\n> + * DPDK callback to register to probe multiple drivers for a PCI device.\n> + *\n> + * @param[in] pci_drv\n> + * PCI driver structure.\n> + * @param[in] dev\n> + * PCI device information.\n> + *\n> + * @return\n> + * 0 on success, a negative errno value otherwise and rte_errno is set.\n> + */\n> +static int\n> +mlx5_common_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,\n> + struct rte_pci_device *pci_dev)\n> +{\n> + struct mlx5_pci_device *dev;\n> + uint32_t user_classes = 0;\n> + bool new_device = false;\n> + int ret;\n> +\n> + ret = parse_class_options(pci_dev->device.devargs);\n> + if (ret < 0)\n> + return ret;\n> + user_classes = ret;\n> + if (user_classes) {\n> + /* Validate combination here. */\n> + ret = is_valid_class_combination(user_classes);\n> + if (ret) {\n> + DRV_LOG(ERR, \"Unsupported mlx5 classes supplied.\");\n> + return ret;\n> + }\n> + } else {\n> + /* Default to net class. */\n> + user_classes = MLX5_CLASS_NET;\n> + }\n> + dev = pci_to_mlx5_device(pci_dev);\n> + if (!dev) {\n> + dev = rte_zmalloc(\"mlx5_pci_device\", sizeof(*dev), 0);\n> + if (!dev)\n> + return -ENOMEM;\n> + dev->pci_dev = pci_dev;\n> + TAILQ_INSERT_HEAD(&devices_list, dev, next);\n> + new_device = true;\n> + }\n> + ret = drivers_probe(dev, pci_drv, pci_dev, user_classes);\n> + if (ret)\n> + goto class_err;\n> + return 0;\n> +class_err:\n> + if (new_device)\n> + dev_release(dev);\n> + return ret;\n> +}\n> +\n> +/**\n> + * DPDK callback to remove one or more drivers for a PCI device.\n> + *\n> + * This function removes all drivers probed for a given PCI device.\n> + *\n> + * @param[in] pci_dev\n> + * Pointer to the PCI device.\n> + *\n> + * @return\n> + * 0 on success, the function cannot fail.\n> + */\n> +static int\n> +mlx5_common_pci_remove(struct rte_pci_device *pci_dev)\n> +{\n> + struct mlx5_pci_device *dev;\n> + int ret;\n> +\n> + dev = pci_to_mlx5_device(pci_dev);\n> + if (!dev)\n> + return -ENODEV;\n> + /* Matching device found, cleanup and unload drivers. */\n> + ret = drivers_remove(dev, dev->classes_loaded);\n> + if (!ret)\n> + dev_release(dev);\n> + return ret;\n> +}\n> +\n> +static int\n> +mlx5_common_pci_dma_map(struct rte_pci_device *pci_dev, void *addr,\n> + uint64_t iova, size_t len)\n> +{\n> + struct mlx5_pci_driver *driver = NULL;\n> + struct mlx5_pci_driver *temp;\n> + struct mlx5_pci_device *dev;\n> + int ret = -EINVAL;\n> +\n> + dev = pci_to_mlx5_device(pci_dev);\n> + if (!dev)\n> + return -ENODEV;\n> + TAILQ_FOREACH(driver, &drv_list, next) {\n> + if (device_class_enabled(dev, driver->driver_class) &&\n> + driver->pci_driver.dma_map) {\n> + ret = driver->pci_driver.dma_map(pci_dev, addr,\n> + iova, len);\n> + if (ret)\n> + goto map_err;\n> + }\n> + }\n> + return ret;\n> +map_err:\n> + TAILQ_FOREACH(temp, &drv_list, next) {\n> + if (temp == driver)\n> + break;\n> + if (device_class_enabled(dev, temp->driver_class) &&\n> + temp->pci_driver.dma_map && temp->pci_driver.dma_unmap)\n> + temp->pci_driver.dma_unmap(pci_dev, addr, iova, len);\n> + }\n> + return ret;\n> +}\n> +\n> +static int\n> +mlx5_common_pci_dma_unmap(struct rte_pci_device *pci_dev, void *addr,\n> + uint64_t iova, size_t len)\n> +{\n> + struct mlx5_pci_driver *driver;\n> + struct mlx5_pci_device *dev;\n> + int local_ret = -EINVAL;\n> + int ret;\n> +\n> + dev = pci_to_mlx5_device(pci_dev);\n> + if (!dev)\n> + return -ENODEV;\n> + ret = 0;\n> + /* There is no unmap error recovery in current implementation. */\n> + TAILQ_FOREACH_REVERSE(driver, &drv_list, mlx5_pci_bus_drv_head, next) {\n> + if (device_class_enabled(dev, driver->driver_class) &&\n> + driver->pci_driver.dma_unmap) {\n> + local_ret = driver->pci_driver.dma_unmap(pci_dev, addr,\n> + iova, len);\n> + if (local_ret && (ret == 0))\n> + ret = local_ret;\n> + }\n> + }\n> + if (local_ret)\n> + ret = local_ret;\n> + return ret;\n> +}\n> +\n> +/* PCI ID table is build dynamically based on registered mlx5 drivers. */\n\nbuilt\n\n> +static struct rte_pci_id *mlx5_pci_id_table;\n> +\n> +static struct rte_pci_driver mlx5_pci_driver = {\n> + .driver = {\n> + .name = \"mlx5_pci\",\n> + },\n> + .probe = mlx5_common_pci_probe,\n> + .remove = mlx5_common_pci_remove,\n> + .dma_map = mlx5_common_pci_dma_map,\n> + .dma_unmap = mlx5_common_pci_dma_unmap,\n> +};\n> +\n> +static int\n> +pci_id_table_size_get(const struct rte_pci_id *id_table)\n> +{\n> + int table_size = 0;\n> +\n> + for (; id_table->vendor_id != 0; id_table++)\n> + table_size++;\n> + return table_size;\n> +}\n> +\n> +static bool\n> +pci_id_exists(const struct rte_pci_id *id, const struct rte_pci_id *table,\n> + int next_idx)\n> +{\n> + int current_size = next_idx - 1;\n> + int i;\n> +\n> + for (i = 0; i < current_size; i++) {\n> + if (id->device_id == table[i].device_id &&\n> + id->vendor_id == table[i].vendor_id &&\n> + id->subsystem_vendor_id == table[i].subsystem_vendor_id &&\n> + id->subsystem_device_id == table[i].subsystem_device_id)\n> + return true;\n> + }\n> + return false;\n> +}\n> +\n> +static void\n> +pci_id_insert(struct rte_pci_id *new_table, int *next_idx,\n> + const struct rte_pci_id *id_table)\n> +{\n> + /* Traverse the id_table, check if entry exists in new_table;\n> + * Add non duplicate entries to new table.\n> + */\n> + for (; id_table->vendor_id != 0; id_table++) {\n> + if (!pci_id_exists(id_table, new_table, *next_idx)) {\n> + /* New entry; add to the table. */\n> + new_table[*next_idx] = *id_table;\n> + (*next_idx)++;\n> + }\n> + }\n> +}\n> +\n> +static int\n> +pci_ids_table_update(const struct rte_pci_id *driver_id_table)\n> +{\n> + const struct rte_pci_id *id_iter;\n> + struct rte_pci_id *updated_table;\n> + struct rte_pci_id *old_table;\n> + int num_ids = 0;\n> + int i = 0;\n> +\n> + old_table = mlx5_pci_id_table;\n> + if (old_table)\n> + num_ids = pci_id_table_size_get(old_table);\n> + num_ids += pci_id_table_size_get(driver_id_table);\n> + /* Increase size by one for the termination entry of vendor_id = 0. */\n> + num_ids += 1;\n> + updated_table = calloc(num_ids, sizeof(*updated_table));\n> + if (!updated_table)\n> + return -ENOMEM;\n> + if (TAILQ_EMPTY(&drv_list)) {\n> + /* Copy the first driver's ID table. */\n> + for (id_iter = driver_id_table; id_iter->vendor_id != 0;\n> + id_iter++, i++)\n> + updated_table[i] = *id_iter;\n> + } else {\n> + /* First copy existing table entries. */\n> + for (id_iter = old_table; id_iter->vendor_id != 0;\n> + id_iter++, i++)\n> + updated_table[i] = *id_iter;\n> + /* New id to be added at the end of current ID table. */\n> + pci_id_insert(updated_table, &i, driver_id_table);\n> + }\n> + /* Terminate table with empty entry. */\n> + updated_table[i].vendor_id = 0;\n> + mlx5_pci_driver.id_table = updated_table;\n> + mlx5_pci_id_table = updated_table;\n> + if (old_table)\n> + free(old_table);\n> + return 0;\n> +}\n> +\n> +void\n> +mlx5_pci_driver_register(struct mlx5_pci_driver *driver)\n> +{\n> + int ret;\n> +\n> + ret = pci_ids_table_update(driver->pci_driver.id_table);\n> + if (ret)\n> + return;\n> + mlx5_pci_driver.drv_flags |= driver->pci_driver.drv_flags;\n> + TAILQ_INSERT_TAIL(&drv_list, driver, next);\n> +}\n> +\n> +void mlx5_common_pci_init(void)\n> +{\n> + const struct rte_pci_id empty_table[] = {\n> + {\n> + .vendor_id = 0\n> + },\n> + };\n> +\n> + /* All mlx5 PMDs constructor runs at same priority. So any of the PMD\n\n/* All mlx5 PMDs constructors run at the same priority. So any of the PMD\n\n\n> + * including this one can register the PCI table first. If any other\n> + * PMD(s) have registered the PCI ID table, No need to register an empty\n> + * default one.\n> + */\n> + if (mlx5_pci_id_table == NULL && pci_ids_table_update(empty_table))\n> + return;\n> + rte_pci_register(&mlx5_pci_driver);\n> +}\n> +\n> +RTE_FINI(mlx5_common_pci_finish)\n> +{\n> + if (mlx5_pci_id_table != NULL) {\n> + /* Constructor doesn't register with PCI bus if it failed\n> + * to build the table.\n> + */\n> + rte_pci_unregister(&mlx5_pci_driver);\n> + free(mlx5_pci_id_table);\n> + }\n> +}\n\nI don't like the asymmetry between multiple constructors calling a\ncommon init / and a single destructor in the common code.\nBut this is not really an issue.\n\n\n> +RTE_PMD_EXPORT_NAME(mlx5_common_pci, __COUNTER__);\n\nThe pci driver name is mlx5_pci iirc.\n\nBut if there is no info about this driver to export, you can remove\nRTE_PMD_EXPORT_NAME.\n\n\n> diff --git a/drivers/common/mlx5/mlx5_common_pci.h b/drivers/common/mlx5/mlx5_common_pci.h\n> new file mode 100644\n> index 000000000..41b73e17a\n> --- /dev/null\n> +++ b/drivers/common/mlx5/mlx5_common_pci.h\n> @@ -0,0 +1,77 @@\n> +/* SPDX-License-Identifier: BSD-3-Clause\n> + * Copyright 2020 Mellanox Technologies, Ltd\n> + */\n> +\n> +#ifndef _MLX5_COMMON_PCI_H_\n> +#define _MLX5_COMMON_PCI_H_\n> +\n> +/**\n> + * @file\n> + *\n> + * RTE Mellanox PCI Driver Interface\n> + * Mellanox ConnectX PCI device supports multiple class (net/vdpa/regex)\n> + * devices. This layer enables creating such multiple class of devices on a\n> + * single PCI device by allowing to bind multiple class specific device\n> + * driver to attach to mlx5_pci driver.\n> + *\n> + * ----------- ------------ -------------\n> + * | mlx5 | | mlx5 | | mlx5 |\n> + * | net pmd | | vdpa pmd | | regex pmd |\n> + * ----------- ------------ -------------\n> + * \\ | /\n> + * \\ | /\n> + * \\ -------------- /\n> + * \\______| mlx5 |_____ /\n> + * | pci common |\n> + * --------------\n> + * |\n> + * -----------\n> + * | mlx5 |\n> + * | pci dev |\n> + * -----------\n> + *\n> + * - mlx5 pci driver binds to mlx5 PCI devices defined by PCI\n> + * ID table of all related mlx5 PCI devices.\n> + * - mlx5 class driver such as net, vdpa, regex PMD defines its\n> + * specific PCI ID table and mlx5 bus driver probes matching\n> + * class drivers.\n> + * - mlx5 pci bus driver is cental place that validates supported\n\ncentral\n\n\n> + * class combinations.\n> + */\n> +\n> +#ifdef __cplusplus\n> +extern \"C\" {\n> +#endif /* __cplusplus */\n> +\n> +#include <rte_pci.h>\n> +#include <rte_bus_pci.h>\n> +\n> +#include <mlx5_common.h>\n> +\n> +void mlx5_common_pci_init(void);\n> +\n> +/**\n> + * A structure describing a mlx5 pci driver.\n> + */\n> +struct mlx5_pci_driver {\n> + struct rte_pci_driver pci_driver; /**< Inherit core pci driver. */\n> + uint32_t driver_class; /**< Class of this driver, enum mlx5_class */\n> + TAILQ_ENTRY(mlx5_pci_driver) next;\n> +};\n> +\n> +/**\n> + * Register a mlx5_pci device driver.\n> + *\n> + * @param driver\n> + * A pointer to a mlx5_pci_driver structure describing the driver\n> + * to be registered.\n> + */\n> +__rte_internal\n> +void\n> +mlx5_pci_driver_register(struct mlx5_pci_driver *driver);\n> +\n> +#ifdef __cplusplus\n> +}\n> +#endif /* __cplusplus */\n> +\n> +#endif /* _MLX5_COMMON_PCI_H_ */\n> diff --git a/drivers/common/mlx5/rte_common_mlx5_version.map b/drivers/common/mlx5/rte_common_mlx5_version.map\n> index 65f25252a..73cf72548 100644\n> --- a/drivers/common/mlx5/rte_common_mlx5_version.map\n> +++ b/drivers/common/mlx5/rte_common_mlx5_version.map\n> @@ -91,5 +91,7 @@ INTERNAL {\n> mlx5_malloc;\n> mlx5_realloc;\n> mlx5_free;\n> +\n> + mlx5_pci_driver_register;\n> };\n>\n> diff --git a/drivers/meson.build b/drivers/meson.build\n> index e6d0409aa..95df7ef1d 100644\n> --- a/drivers/meson.build\n> +++ b/drivers/meson.build\n> @@ -5,6 +5,7 @@\n> subdirs = [\n> 'common',\n> 'bus',\n> + 'common/mlx5', # depends on bus.\n> 'mempool', # depends on common and bus.\n> 'net', # depends on common, bus, mempool\n> 'raw', # depends on common, bus and net.\n> --\n> 2.25.4\n>\n\nReviewed-by: David Marchand <david.marchand@redhat.com>", "headers": { "Return-Path": "<dev-bounces@dpdk.org>", "X-Original-To": "patchwork@inbox.dpdk.org", "Delivered-To": "patchwork@inbox.dpdk.org", "Received": [ "from dpdk.org (dpdk.org [92.243.14.124])\n\tby inbox.dpdk.org (Postfix) with ESMTP id 03BF9A0527;\n\tFri, 24 Jul 2020 16:41:45 +0200 (CEST)", "from [92.243.14.124] (localhost [127.0.0.1])\n\tby dpdk.org (Postfix) with ESMTP id 255301C038;\n\tFri, 24 Jul 2020 16:40:24 +0200 (CEST)", "from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com\n [205.139.110.61]) by dpdk.org (Postfix) with ESMTP id EE1661C031\n for <dev@dpdk.org>; Fri, 24 Jul 2020 16:40:22 +0200 (CEST)", "from mail-vk1-f197.google.com (mail-vk1-f197.google.com\n [209.85.221.197]) (Using TLS) by relay.mimecast.com with ESMTP id\n us-mta-60-ZsNvW_ZkOtqOJQ7I22iSvA-1; Fri, 24 Jul 2020 10:40:18 -0400", "by mail-vk1-f197.google.com with SMTP id d132so2983117vke.16\n for <dev@dpdk.org>; Fri, 24 Jul 2020 07:40:18 -0700 (PDT)" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n s=mimecast20190719; t=1595601622;\n h=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n to:to:cc:cc:mime-version:mime-version:content-type:content-type:\n in-reply-to:in-reply-to:references:references;\n bh=QyPpHTPx7huOh96whO0X2tplTPjwuY3f9WkRf8BgwX0=;\n b=QUH7FXUhiKmczycQz5bOt6k6LyZfVwQhHtpx3XemYxhgCxFGzTBJ3BTHROwyNjXJkATowS\n 5t1Vp7n+CwU4eVSmHkqQMr+f+axzYH4Gz9+42rvF/EE/SmfuO0dlDa4CZOxEzOHweR9I2k\n 8HxMFprIMvMFerKj7AFCPTS6rxFOo0o=", "X-MC-Unique": "ZsNvW_ZkOtqOJQ7I22iSvA-1", "X-Google-DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=1e100.net; s=20161025;\n h=x-gm-message-state:mime-version:references:in-reply-to:from:date\n :message-id:subject:to:cc;\n bh=QyPpHTPx7huOh96whO0X2tplTPjwuY3f9WkRf8BgwX0=;\n b=c5/blMs6H3pbXi8VgL2vSmsVD3c/eZbob9KQzJZn9mJBa5upakagGwqfmwOsRvBd3G\n xVTeaftYCUYlLBcY0kPCr0UXhh4XrxoUhlzoG0edYd/TLUST++dbWMe8f+wmaQ5KFeCF\n HcPikaIZOUE1ZqUPZ3taN8agjoOn5kNhDWuMie4VcOOlAg7G5/5JG/bOzTt+VnLFXBRk\n K3KMbDxVDmoBp/EPjmPfY2PY3QLnWqth3X6cnVakAKmjEJLPw3s8VbF7Pnw1ZbeGVfnj\n sziD95NU4Ak+4DavhGhbh2o95lnBeasWwWBD4Tx1d9JJJUIP4j3ahOGOzgZEoQ9OGsD3\n y7gw==", "X-Gm-Message-State": "AOAM533n4IXJKBO9Adg7QcNLn49wtAIx8x4A2BWHrphQLiOe+u0gvfNK\n x8UKfO3OR1cyF9N8xrkYoPcblLgvi89DR7SxkbA3/BLwYDoX83urRPWf6lG3d9y4+6RWH8fdTIX\n wZDO/8e/LgAyAWdPk2ME=", "X-Received": [ "by 2002:a9f:2b8d:: with SMTP id y13mr8207153uai.126.1595601617462;\n Fri, 24 Jul 2020 07:40:17 -0700 (PDT)", "by 2002:a9f:2b8d:: with SMTP id y13mr8207120uai.126.1595601616773;\n Fri, 24 Jul 2020 07:40:16 -0700 (PDT)" ], "X-Google-Smtp-Source": "\n ABdhPJz3J5AdXSfQNSDxJ9XJNJBi5J8rN3hUjv7ESFHTIYk7gn6dIHrdjYuNndS0EJodVsgzW+nS/ZIZKP6i5G1lTsE=", "MIME-Version": "1.0", "References": "<20200610171728.89-2-parav@mellanox.com>\n <20200723200910.376581-1-parav@mellanox.com>\n <20200723200910.376581-9-parav@mellanox.com>", "In-Reply-To": "<20200723200910.376581-9-parav@mellanox.com>", "From": "David Marchand <david.marchand@redhat.com>", "Date": "Fri, 24 Jul 2020 16:40:05 +0200", "Message-ID": "\n <CAJFAV8xT0Uo9b73SfpA3Tt15A1K5+0BgLnNMuehbx5jyjtNKwg@mail.gmail.com>", "To": "Parav Pandit <parav@mellanox.com>", "Cc": "dev <dev@dpdk.org>, Gaetan Rivet <grive@u256.net>,\n \"Yigit, Ferruh\" <ferruh.yigit@intel.com>,\n Thomas Monjalon <thomas@monjalon.net>,\n Raslan <rasland@mellanox.com>, Ori Kam <orika@mellanox.com>,\n Matan Azrad <matan@mellanox.com>, Joyce Kong <joyce.kong@arm.com>", "Authentication-Results": "relay.mimecast.com;\n auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dmarchan@redhat.com", "X-Mimecast-Spam-Score": "0", "X-Mimecast-Originator": "redhat.com", "Content-Type": "text/plain; charset=\"UTF-8\"", "Subject": "Re: [dpdk-dev] [PATCH v8 08/10] common/mlx5: introduce layer to\n support multiple class drivers", "X-BeenThere": "dev@dpdk.org", "X-Mailman-Version": "2.1.15", "Precedence": "list", "List-Id": "DPDK patches and discussions <dev.dpdk.org>", "List-Unsubscribe": "<https://mails.dpdk.org/options/dev>,\n <mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://mails.dpdk.org/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<https://mails.dpdk.org/listinfo/dev>,\n <mailto:dev-request@dpdk.org?subject=subscribe>", "Errors-To": "dev-bounces@dpdk.org", "Sender": "\"dev\" <dev-bounces@dpdk.org>" }, "addressed": null } ]