DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 00/14] vDPA API and framework rework
@ 2020-06-11 21:37 Maxime Coquelin
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 01/14] bus/dpaa: fix null pointer dereference Maxime Coquelin
                   ` (14 more replies)
  0 siblings, 15 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin

This series aims to rework the vDPA framework and
its API to better fit into the DPDK device model
and also be more easily consumable by applications.

Main changes are creating a device class for vDPA,
which enables applications to iterate vDPA devices
in a generic way:

RTE_DEV_FOREACH(dev, "class=vdpa", it) {...}

Also the notion of vDPA device ID is replaced
with both application & drivers using the
rte_vdpa_device as reference. Doing that also
made possible to store devices references into
a linked list instead of a static array. Last
patch makes the rte_vdpa_device structure
content opaque to the applications, creating
a clear barrier between application visible
API and drivers visble ones.

The first two patches fixes issues in some
busses iterators, causing segmentation faults
when iterating only on a device class.

While reviewing, if you notice further possible
improvements, please let me know. Target is to
remove the experimental tag from vDPA APIs in
next LTS release.

Thanks to David for giving me some ideas of
improvements!

Maxime Coquelin (14):
  bus/dpaa: fix null pointer dereference
  bus/fslmc: fix null pointer dereference
  vhost: introduce vDPA devices class
  vhost: make vDPA framework bus agnostic
  vhost: replace device ID in vDPA ops
  vhost: replace vDPA device ID in Vhost
  vhost: replace device ID in applications
  vhost: remove useless vDPA API
  vhost: use linked-list for vDPA devices
  vhost: introduce wrappers for some vDPA ops
  examples/vdpa: use new wrappers instead of ops
  examples/vdpa: remove useless device count
  vhost: remove vDPA device count API
  vhost: split vDPA header file

 drivers/bus/dpaa/dpaa_bus.c            |   5 +
 drivers/bus/fslmc/fslmc_bus.c          |   5 +
 drivers/vdpa/ifc/ifcvf_vdpa.c          |  84 ++++-----
 drivers/vdpa/mlx5/mlx5_vdpa.c          |  85 +++++----
 drivers/vdpa/mlx5/mlx5_vdpa.h          |   5 +-
 examples/vdpa/main.c                   | 108 +++++------
 lib/librte_vhost/Makefile              |   3 +-
 lib/librte_vhost/meson.build           |   3 +-
 lib/librte_vhost/rte_vdpa.h            | 172 ++++--------------
 lib/librte_vhost/rte_vdpa_dev.h        | 135 ++++++++++++++
 lib/librte_vhost/rte_vhost.h           |  20 ++-
 lib/librte_vhost/rte_vhost_version.map |  11 +-
 lib/librte_vhost/socket.c              |  48 ++---
 lib/librte_vhost/vdpa.c                | 238 ++++++++++++++++---------
 lib/librte_vhost/vhost.c               |  19 +-
 lib/librte_vhost/vhost.h               |   9 +-
 lib/librte_vhost/vhost_user.c          |  28 +--
 17 files changed, 545 insertions(+), 433 deletions(-)
 create mode 100644 lib/librte_vhost/rte_vdpa_dev.h

-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH 01/14] bus/dpaa: fix null pointer dereference
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
@ 2020-06-11 21:37 ` Maxime Coquelin
  2020-06-12  9:48   ` Gaëtan Rivet
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 02/14] bus/fslmc: " Maxime Coquelin
                   ` (13 subsequent siblings)
  14 siblings, 1 reply; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin, stable

This patches fixes a null pointer derefencing that happens
when the device string passed to the iterator is NULL. This
situation can happen when iterating on a class type.
For example:

RTE_DEV_FOREACH(dev, "class=eth", &dev_iter) {
    ...
}

Fixes: e79df833d3f6 ("bus/dpaa: support hotplug ops")
Cc: stable@dpdk.org
Cc: shreyansh.jain@nxp.com

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/bus/dpaa/dpaa_bus.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/bus/dpaa/dpaa_bus.c b/drivers/bus/dpaa/dpaa_bus.c
index d53fe6083a..216f38acd4 100644
--- a/drivers/bus/dpaa/dpaa_bus.c
+++ b/drivers/bus/dpaa/dpaa_bus.c
@@ -703,6 +703,11 @@ dpaa_bus_dev_iterate(const void *start, const char *str,
 	struct rte_dpaa_device *dev;
 	char *dup, *dev_name = NULL;
 
+	if (str == NULL) {
+		DPAA_BUS_DEBUG("No device string\n");
+		return NULL;
+	}
+
 	/* Expectation is that device would be name=device_name */
 	if (strncmp(str, "name=", 5) != 0) {
 		DPAA_BUS_DEBUG("Invalid device string (%s)\n", str);
-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH 02/14] bus/fslmc: fix null pointer dereference
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 01/14] bus/dpaa: fix null pointer dereference Maxime Coquelin
@ 2020-06-11 21:37 ` Maxime Coquelin
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 03/14] vhost: introduce vDPA devices class Maxime Coquelin
                   ` (12 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin, stable

This patches fixes a null pointer derefencing that happens
when the device string passed to the iterator is NULL. This
situation can happen when iterating on a class type.
For example:

RTE_DEV_FOREACH(dev, "class=eth", &dev_iter) {
    ...
}

Fixes: e67a61614d0b ("bus/fslmc: support device iteration")
Cc: stable@dpdk.org
Cc: shreyansh.jain@nxp.com

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/bus/fslmc/fslmc_bus.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/bus/fslmc/fslmc_bus.c b/drivers/bus/fslmc/fslmc_bus.c
index afbd82e8db..75c15d6b67 100644
--- a/drivers/bus/fslmc/fslmc_bus.c
+++ b/drivers/bus/fslmc/fslmc_bus.c
@@ -603,6 +603,11 @@ fslmc_bus_dev_iterate(const void *start, const char *str,
 	struct rte_dpaa2_device *dev;
 	char *dup, *dev_name = NULL;
 
+	if (str == NULL) {
+		DPAA2_BUS_DEBUG("No device string\n");
+		return NULL;
+	}
+
 	/* Expectation is that device would be name=device_name */
 	if (strncmp(str, "name=", 5) != 0) {
 		DPAA2_BUS_DEBUG("Invalid device string (%s)\n", str);
-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH 03/14] vhost: introduce vDPA devices class
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 01/14] bus/dpaa: fix null pointer dereference Maxime Coquelin
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 02/14] bus/fslmc: " Maxime Coquelin
@ 2020-06-11 21:37 ` Maxime Coquelin
  2020-06-12 12:37   ` Gaëtan Rivet
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 04/14] vhost: make vDPA framework bus agnostic Maxime Coquelin
                   ` (11 subsequent siblings)
  14 siblings, 1 reply; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin

This patch introduces vDPA device class. It will enable
application to iterate over the vDPA devices.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vdpa.c | 116 +++++++++++++++++++++++++++++++++-------
 1 file changed, 98 insertions(+), 18 deletions(-)

diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
index b2b2a105f1..61ab9aadb4 100644
--- a/lib/librte_vhost/vdpa.c
+++ b/lib/librte_vhost/vdpa.c
@@ -10,11 +10,12 @@
 
 #include <stdbool.h>
 
+#include <rte_class.h>
 #include <rte_malloc.h>
 #include "rte_vdpa.h"
 #include "vhost.h"
 
-static struct rte_vdpa_device *vdpa_devices[MAX_VHOST_DEVICE];
+static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE];
 static uint32_t vdpa_device_num;
 
 static bool
@@ -46,35 +47,28 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
 		struct rte_vdpa_dev_ops *ops)
 {
 	struct rte_vdpa_device *dev;
-	char device_name[MAX_VDPA_NAME_LEN];
 	int i;
 
 	if (vdpa_device_num >= MAX_VHOST_DEVICE || addr == NULL || ops == NULL)
 		return -1;
 
 	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
-		dev = vdpa_devices[i];
-		if (dev && is_same_vdpa_device(&dev->addr, addr))
+		dev = &vdpa_devices[i];
+		if (dev->ops && is_same_vdpa_device(&dev->addr, addr))
 			return -1;
 	}
 
 	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
-		if (vdpa_devices[i] == NULL)
+		if (vdpa_devices[i].ops == NULL)
 			break;
 	}
 
 	if (i == MAX_VHOST_DEVICE)
 		return -1;
 
-	snprintf(device_name, sizeof(device_name), "vdpa-dev-%d", i);
-	dev = rte_zmalloc(device_name, sizeof(struct rte_vdpa_device),
-			RTE_CACHE_LINE_SIZE);
-	if (!dev)
-		return -1;
-
+	dev = &vdpa_devices[i];
 	memcpy(&dev->addr, addr, sizeof(struct rte_vdpa_dev_addr));
 	dev->ops = ops;
-	vdpa_devices[i] = dev;
 	vdpa_device_num++;
 
 	return i;
@@ -83,11 +77,10 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
 int
 rte_vdpa_unregister_device(int did)
 {
-	if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did] == NULL)
+	if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did].ops == NULL)
 		return -1;
 
-	rte_free(vdpa_devices[did]);
-	vdpa_devices[did] = NULL;
+	memset(&vdpa_devices[did], 0, sizeof(struct rte_vdpa_device));
 	vdpa_device_num--;
 
 	return did;
@@ -103,8 +96,11 @@ rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr)
 		return -1;
 
 	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
-		dev = vdpa_devices[i];
-		if (dev && is_same_vdpa_device(&dev->addr, addr))
+		dev = &vdpa_devices[i];
+		if (dev->ops == NULL)
+			continue;
+
+		if (is_same_vdpa_device(&dev->addr, addr))
 			return i;
 	}
 
@@ -117,7 +113,7 @@ rte_vdpa_get_device(int did)
 	if (did < 0 || did >= MAX_VHOST_DEVICE)
 		return NULL;
 
-	return vdpa_devices[did];
+	return &vdpa_devices[did];
 }
 
 int
@@ -227,3 +223,87 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m)
 		free_ind_table(idesc);
 	return -1;
 }
+
+static uint16_t
+vdpa_dev_to_id(const struct rte_vdpa_device *dev)
+{
+	if (dev == NULL)
+		return MAX_VHOST_DEVICE;
+	return dev - vdpa_devices;
+}
+
+static int
+vdpa_dev_match(struct rte_vdpa_device *dev,
+	      const struct rte_device *rte_dev)
+{
+	struct rte_vdpa_dev_addr addr;
+
+	/*  Only PCI bus supported for now */
+	if (strcmp(rte_dev->bus->name, "pci") != 0)
+		return -1;
+
+	addr.type = VDPA_ADDR_PCI;
+
+	if (rte_pci_addr_parse(rte_dev->name, &addr.pci_addr) != 0)
+		return -1;
+
+	if (!is_same_vdpa_device(&dev->addr, &addr))
+		return -1;
+
+	return 0;
+}
+
+/* Generic rte_vdpa_dev comparison function. */
+typedef int (*rte_vdpa_cmp_t)(struct rte_vdpa_device *,
+		const struct rte_device *rte_dev);
+
+static struct rte_vdpa_device *
+vdpa_find_device(const struct rte_vdpa_device *start, rte_vdpa_cmp_t cmp,
+		struct rte_device *rte_dev)
+{
+	struct rte_vdpa_device *dev;
+	ptrdiff_t idx;
+
+	/* Avoid Undefined Behaviour */
+	if (start != NULL &&
+	    (start < &vdpa_devices[0] ||
+	     start >= &vdpa_devices[MAX_VHOST_DEVICE]))
+		return NULL;
+
+	if (start != NULL)
+		idx = vdpa_dev_to_id(start) + 1;
+	else
+		idx = 0;
+	for (; idx < MAX_VHOST_DEVICE; idx++) {
+		dev = &vdpa_devices[idx];
+		/*
+		 * ToDo: Certainly better to introduce a state field,
+		 * but rely on ops being set for now.
+		 */
+		if (dev->ops == NULL)
+			continue;
+		if (cmp(dev, rte_dev) == 0)
+			return dev;
+	}
+	return NULL;
+}
+
+static void *
+vdpa_dev_iterate(const void *start,
+		const char *str,
+		const struct rte_dev_iterator *it)
+{
+	struct rte_vdpa_device *vdpa_dev = NULL;
+
+	RTE_SET_USED(str);
+
+	vdpa_dev = vdpa_find_device(start, vdpa_dev_match, it->device);
+
+	return vdpa_dev;
+}
+
+static struct rte_class rte_class_vdpa = {
+	.dev_iterate = vdpa_dev_iterate,
+};
+
+RTE_REGISTER_CLASS(vdpa, rte_class_vdpa);
-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH 04/14] vhost: make vDPA framework bus agnostic
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
                   ` (2 preceding siblings ...)
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 03/14] vhost: introduce vDPA devices class Maxime Coquelin
@ 2020-06-11 21:37 ` Maxime Coquelin
  2020-06-12 12:46   ` Gaëtan Rivet
  2020-06-19 11:14   ` Adrian Moreno
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 05/14] vhost: replace device ID in vDPA ops Maxime Coquelin
                   ` (10 subsequent siblings)
  14 siblings, 2 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin

This patch makes the vDPA framework to no more
support only PCI devices, but any devices by relying
on the generic device name as identifier.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/vdpa/ifc/ifcvf_vdpa.c          |  6 +-
 drivers/vdpa/mlx5/mlx5_vdpa.c          |  8 +--
 drivers/vdpa/mlx5/mlx5_vdpa.h          |  2 +-
 examples/vdpa/main.c                   | 49 ++++++++--------
 lib/librte_vhost/rte_vdpa.h            | 42 +++++++-------
 lib/librte_vhost/rte_vhost_version.map |  1 +
 lib/librte_vhost/vdpa.c                | 79 +++++++++++---------------
 7 files changed, 85 insertions(+), 102 deletions(-)

diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c b/drivers/vdpa/ifc/ifcvf_vdpa.c
index ec97178dcb..1fec1f1baf 100644
--- a/drivers/vdpa/ifc/ifcvf_vdpa.c
+++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
@@ -47,7 +47,6 @@ static const char * const ifcvf_valid_arguments[] = {
 static int ifcvf_vdpa_logtype;
 
 struct ifcvf_internal {
-	struct rte_vdpa_dev_addr dev_addr;
 	struct rte_pci_device *pdev;
 	struct ifcvf_hw hw;
 	int vfio_container_fd;
@@ -1176,8 +1175,6 @@ ifcvf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 		(1ULL << VHOST_USER_F_PROTOCOL_FEATURES) |
 		(1ULL << VHOST_F_LOG_ALL);
 
-	internal->dev_addr.pci_addr = pci_dev->addr;
-	internal->dev_addr.type = VDPA_ADDR_PCI;
 	list->internal = internal;
 
 	if (rte_kvargs_count(kvlist, IFCVF_SW_FALLBACK_LM)) {
@@ -1188,8 +1185,7 @@ ifcvf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	}
 	internal->sw_lm = sw_fallback_lm;
 
-	internal->did = rte_vdpa_register_device(&internal->dev_addr,
-				&ifcvf_ops);
+	internal->did = rte_vdpa_register_device(&pci_dev->device, &ifcvf_ops);
 	if (internal->did < 0) {
 		DRV_LOG(ERR, "failed to register device %s", pci_dev->name);
 		goto error;
diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c b/drivers/vdpa/mlx5/mlx5_vdpa.c
index 1113d6cef0..e8255c7d7e 100644
--- a/drivers/vdpa/mlx5/mlx5_vdpa.c
+++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
@@ -501,14 +501,13 @@ mlx5_vdpa_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	priv->caps = attr.vdpa;
 	priv->log_max_rqt_size = attr.log_max_rqt_size;
 	priv->ctx = ctx;
-	priv->dev_addr.pci_addr = pci_dev->addr;
-	priv->dev_addr.type = VDPA_ADDR_PCI;
+	priv->pci_dev = pci_dev;
 	priv->var = mlx5_glue->dv_alloc_var(ctx, 0);
 	if (!priv->var) {
 		DRV_LOG(ERR, "Failed to allocate VAR %u.\n", errno);
 		goto error;
 	}
-	priv->id = rte_vdpa_register_device(&priv->dev_addr, &mlx5_vdpa_ops);
+	priv->id = rte_vdpa_register_device(&pci_dev->device, &mlx5_vdpa_ops);
 	if (priv->id < 0) {
 		DRV_LOG(ERR, "Failed to register vDPA device.");
 		rte_errno = rte_errno ? rte_errno : EINVAL;
@@ -550,8 +549,7 @@ mlx5_vdpa_pci_remove(struct rte_pci_device *pci_dev)
 
 	pthread_mutex_lock(&priv_list_lock);
 	TAILQ_FOREACH(priv, &priv_list, next) {
-		if (memcmp(&priv->dev_addr.pci_addr, &pci_dev->addr,
-			   sizeof(pci_dev->addr)) == 0) {
+		if (priv->pci_dev == pci_dev) {
 			found = 1;
 			break;
 		}
diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.h b/drivers/vdpa/mlx5/mlx5_vdpa.h
index fcc216ac78..50ee3c5870 100644
--- a/drivers/vdpa/mlx5/mlx5_vdpa.h
+++ b/drivers/vdpa/mlx5/mlx5_vdpa.h
@@ -104,7 +104,7 @@ struct mlx5_vdpa_priv {
 	int id; /* vDPA device id. */
 	int vid; /* vhost device id. */
 	struct ibv_context *ctx; /* Device context. */
-	struct rte_vdpa_dev_addr dev_addr;
+	struct rte_pci_device *pci_dev;
 	struct mlx5_hca_vdpa_attr caps;
 	uint32_t pdn; /* Protection Domain number. */
 	struct ibv_pd *pd;
diff --git a/examples/vdpa/main.c b/examples/vdpa/main.c
index d9a9112b16..c12da69574 100644
--- a/examples/vdpa/main.c
+++ b/examples/vdpa/main.c
@@ -271,10 +271,14 @@ static void cmd_list_vdpa_devices_parsed(
 	uint32_t queue_num;
 	uint64_t features;
 	struct rte_vdpa_device *vdev;
-	struct rte_pci_addr addr;
+	struct rte_device *dev;
+	struct rte_dev_iterator dev_iter;
 
-	cmdline_printf(cl, "device id\tdevice address\tqueue num\tsupported features\n");
-	for (did = 0; did < dev_total; did++) {
+	cmdline_printf(cl, "device id\tdevice name\tqueue num\tsupported features\n");
+	RTE_DEV_FOREACH(dev, "class=vdpa", &dev_iter) {
+		did = rte_vdpa_find_device_id_by_name(dev->name);
+		if (did < 0)
+			continue;
 		vdev = rte_vdpa_get_device(did);
 		if (!vdev)
 			continue;
@@ -290,11 +294,8 @@ static void cmd_list_vdpa_devices_parsed(
 				"for device id %d.\n", did);
 			continue;
 		}
-		addr = vdev->addr.pci_addr;
-		cmdline_printf(cl,
-			"%d\t\t" PCI_PRI_FMT "\t%" PRIu32 "\t\t0x%" PRIx64 "\n",
-			did, addr.domain, addr.bus, addr.devid,
-			addr.function, queue_num, features);
+		cmdline_printf(cl, "%d\t\t%s\t\t%" PRIu32 "\t\t0x%" PRIx64 "\n",
+			did, dev->name, queue_num, features);
 	}
 }
 
@@ -324,17 +325,12 @@ static void cmd_create_vdpa_port_parsed(void *parsed_result,
 {
 	int did;
 	struct cmd_create_result *res = parsed_result;
-	struct rte_vdpa_dev_addr addr;
 
 	rte_strscpy(vports[devcnt].ifname, res->socket_path, MAX_PATH_LEN);
-	if (rte_pci_addr_parse(res->bdf, &addr.pci_addr) != 0) {
-		cmdline_printf(cl, "Unable to parse the given bdf.\n");
-		return;
-	}
-	addr.type = VDPA_ADDR_PCI;
-	did = rte_vdpa_find_device_id(&addr);
+	did = rte_vdpa_find_device_id_by_name(res->bdf);
 	if (did < 0) {
-		cmdline_printf(cl, "Unable to find vdpa device id.\n");
+		cmdline_printf(cl, "Unable to find vdpa device id for %s.\n",
+				res->bdf);
 		return;
 	}
 
@@ -400,9 +396,11 @@ int
 main(int argc, char *argv[])
 {
 	char ch;
-	int i;
+	int did;
 	int ret;
 	struct cmdline *cl;
+	struct rte_device *dev;
+	struct rte_dev_iterator dev_iter;
 
 	ret = rte_eal_init(argc, argv);
 	if (ret < 0)
@@ -428,13 +426,18 @@ main(int argc, char *argv[])
 		cmdline_interact(cl);
 		cmdline_stdin_exit(cl);
 	} else {
-		for (i = 0; i < RTE_MIN(MAX_VDPA_SAMPLE_PORTS, dev_total);
-				i++) {
-			vports[i].did = i;
-			snprintf(vports[i].ifname, MAX_PATH_LEN, "%s%d",
-					iface, i);
+		RTE_DEV_FOREACH(dev, "class=vdpa", &dev_iter) {
+			did = rte_vdpa_find_device_id_by_name(dev->name);
+			if (did < 0) {
+				rte_panic("Failed to find device id for %s\n",
+						dev->name);
+			}
+			vports[devcnt].did = did;
+			snprintf(vports[devcnt].ifname, MAX_PATH_LEN, "%s%d",
+					iface, devcnt);
 
-			start_vdpa(&vports[i]);
+			start_vdpa(&vports[devcnt]);
+			devcnt++;
 		}
 
 		printf("enter \'q\' to quit\n");
diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
index 3c400ee79b..33037d39ea 100644
--- a/lib/librte_vhost/rte_vdpa.h
+++ b/lib/librte_vhost/rte_vdpa.h
@@ -18,25 +18,6 @@
 
 #define MAX_VDPA_NAME_LEN 128
 
-enum vdpa_addr_type {
-	VDPA_ADDR_PCI,
-	VDPA_ADDR_MAX
-};
-
-/**
- * vdpa device address
- */
-struct rte_vdpa_dev_addr {
-	/** vdpa address type */
-	enum vdpa_addr_type type;
-
-	/** vdpa pci address */
-	union {
-		uint8_t __dummy[64];
-		struct rte_pci_addr pci_addr;
-	};
-};
-
 /**
  * vdpa device operations
  */
@@ -81,8 +62,8 @@ struct rte_vdpa_dev_ops {
  * vdpa device structure includes device address and device operations.
  */
 struct rte_vdpa_device {
-	/** vdpa device address */
-	struct rte_vdpa_dev_addr addr;
+	/** Generic device information */
+	struct rte_device *device;
 	/** vdpa device operations */
 	struct rte_vdpa_dev_ops *ops;
 } __rte_cache_aligned;
@@ -102,7 +83,7 @@ struct rte_vdpa_device {
  */
 __rte_experimental
 int
-rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
+rte_vdpa_register_device(struct rte_device *rte_dev,
 		struct rte_vdpa_dev_ops *ops);
 
 /**
@@ -120,6 +101,21 @@ __rte_experimental
 int
 rte_vdpa_unregister_device(int did);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Find the device id of a vdpa device from its name
+ *
+ * @param name
+ *  the vdpa device name
+ * @return
+ *  device id on success, -1 on failure
+ */
+__rte_experimental
+int
+rte_vdpa_find_device_id_by_name(const char *name);
+
 /**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
@@ -133,7 +129,7 @@ rte_vdpa_unregister_device(int did);
  */
 __rte_experimental
 int
-rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr);
+rte_vdpa_find_device_id(struct rte_vdpa_device *dev);
 
 /**
  * @warning
diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
index 051d08c120..1abfff8a0c 100644
--- a/lib/librte_vhost/rte_vhost_version.map
+++ b/lib/librte_vhost/rte_vhost_version.map
@@ -66,4 +66,5 @@ EXPERIMENTAL {
 	rte_vhost_get_vhost_ring_inflight;
 	rte_vhost_get_vring_base_from_inflight;
 	rte_vhost_slave_config_change;
+	rte_vdpa_find_device_id_by_name;
 };
diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
index 61ab9aadb4..5abc5a2a7c 100644
--- a/lib/librte_vhost/vdpa.c
+++ b/lib/librte_vhost/vdpa.c
@@ -18,43 +18,22 @@
 static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE];
 static uint32_t vdpa_device_num;
 
-static bool
-is_same_vdpa_device(struct rte_vdpa_dev_addr *a,
-		struct rte_vdpa_dev_addr *b)
-{
-	bool ret = true;
-
-	if (a->type != b->type)
-		return false;
-
-	switch (a->type) {
-	case VDPA_ADDR_PCI:
-		if (a->pci_addr.domain != b->pci_addr.domain ||
-				a->pci_addr.bus != b->pci_addr.bus ||
-				a->pci_addr.devid != b->pci_addr.devid ||
-				a->pci_addr.function != b->pci_addr.function)
-			ret = false;
-		break;
-	default:
-		break;
-	}
-
-	return ret;
-}
-
 int
-rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
+rte_vdpa_register_device(struct rte_device *rte_dev,
 		struct rte_vdpa_dev_ops *ops)
 {
 	struct rte_vdpa_device *dev;
 	int i;
 
-	if (vdpa_device_num >= MAX_VHOST_DEVICE || addr == NULL || ops == NULL)
+	if (vdpa_device_num >= MAX_VHOST_DEVICE || ops == NULL)
 		return -1;
 
 	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
 		dev = &vdpa_devices[i];
-		if (dev->ops && is_same_vdpa_device(&dev->addr, addr))
+		if (dev->ops == NULL)
+			continue;
+
+		if (dev->device == rte_dev)
 			return -1;
 	}
 
@@ -67,7 +46,7 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
 		return -1;
 
 	dev = &vdpa_devices[i];
-	memcpy(&dev->addr, addr, sizeof(struct rte_vdpa_dev_addr));
+	dev->device = rte_dev;
 	dev->ops = ops;
 	vdpa_device_num++;
 
@@ -87,12 +66,33 @@ rte_vdpa_unregister_device(int did)
 }
 
 int
-rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr)
+rte_vdpa_find_device_id(struct rte_vdpa_device *dev)
+{
+	struct rte_vdpa_device *tmp_dev;
+	int i;
+
+	if (dev == NULL)
+		return -1;
+
+	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
+		tmp_dev = &vdpa_devices[i];
+		if (tmp_dev->ops == NULL)
+			continue;
+
+		if (tmp_dev == dev)
+			return i;
+	}
+
+	return -1;
+}
+
+int
+rte_vdpa_find_device_id_by_name(const char *name)
 {
 	struct rte_vdpa_device *dev;
 	int i;
 
-	if (addr == NULL)
+	if (name == NULL)
 		return -1;
 
 	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
@@ -100,7 +100,7 @@ rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr)
 		if (dev->ops == NULL)
 			continue;
 
-		if (is_same_vdpa_device(&dev->addr, addr))
+		if (strcmp(dev->device->name, name) == 0)
 			return i;
 	}
 
@@ -236,21 +236,10 @@ static int
 vdpa_dev_match(struct rte_vdpa_device *dev,
 	      const struct rte_device *rte_dev)
 {
-	struct rte_vdpa_dev_addr addr;
+	if (dev->device == rte_dev)
+		return 0;
 
-	/*  Only PCI bus supported for now */
-	if (strcmp(rte_dev->bus->name, "pci") != 0)
-		return -1;
-
-	addr.type = VDPA_ADDR_PCI;
-
-	if (rte_pci_addr_parse(rte_dev->name, &addr.pci_addr) != 0)
-		return -1;
-
-	if (!is_same_vdpa_device(&dev->addr, &addr))
-		return -1;
-
-	return 0;
+	return -1;
 }
 
 /* Generic rte_vdpa_dev comparison function. */
-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH 05/14] vhost: replace device ID in vDPA ops
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
                   ` (3 preceding siblings ...)
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 04/14] vhost: make vDPA framework bus agnostic Maxime Coquelin
@ 2020-06-11 21:37 ` Maxime Coquelin
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 06/14] vhost: replace vDPA device ID in Vhost Maxime Coquelin
                   ` (9 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin

This patch is a preliminary step to get rid of the
vDPA device ID. It makes vDPA callbacks to use the
vDPA device struct as a reference instead of the ID.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/vdpa/ifc/ifcvf_vdpa.c | 79 +++++++++++++++--------------
 drivers/vdpa/mlx5/mlx5_vdpa.c | 85 ++++++++++++++++++-------------
 drivers/vdpa/mlx5/mlx5_vdpa.h |  2 +-
 examples/vdpa/main.c          |  4 +-
 lib/librte_vhost/rte_vdpa.h   | 17 ++++---
 lib/librte_vhost/socket.c     |  6 +--
 lib/librte_vhost/vdpa.c       | 95 ++++++++++++++++++-----------------
 7 files changed, 157 insertions(+), 131 deletions(-)

diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c b/drivers/vdpa/ifc/ifcvf_vdpa.c
index 1fec1f1baf..a04da16bbe 100644
--- a/drivers/vdpa/ifc/ifcvf_vdpa.c
+++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
@@ -55,7 +55,7 @@ struct ifcvf_internal {
 	pthread_t tid;	/* thread for notify relay */
 	int epfd;
 	int vid;
-	int did;
+	struct rte_vdpa_device *vdev;
 	uint16_t max_queues;
 	uint64_t features;
 	rte_atomic32_t started;
@@ -84,7 +84,7 @@ static pthread_mutex_t internal_list_lock = PTHREAD_MUTEX_INITIALIZER;
 static void update_used_ring(struct ifcvf_internal *internal, uint16_t qid);
 
 static struct internal_list *
-find_internal_resource_by_did(int did)
+find_internal_resource_by_vdev(struct rte_vdpa_device *vdev)
 {
 	int found = 0;
 	struct internal_list *list;
@@ -92,7 +92,7 @@ find_internal_resource_by_did(int did)
 	pthread_mutex_lock(&internal_list_lock);
 
 	TAILQ_FOREACH(list, &internal_list, next) {
-		if (did == list->internal->did) {
+		if (vdev == list->internal->vdev) {
 			found = 1;
 			break;
 		}
@@ -876,14 +876,14 @@ ifcvf_sw_fallback_switchover(struct ifcvf_internal *internal)
 static int
 ifcvf_dev_config(int vid)
 {
-	int did;
+	struct rte_vdpa_device *vdev;
 	struct internal_list *list;
 	struct ifcvf_internal *internal;
 
-	did = rte_vhost_get_vdpa_device_id(vid);
-	list = find_internal_resource_by_did(did);
+	vdev = rte_vdpa_get_device(rte_vhost_get_vdpa_device_id(vid));
+	list = find_internal_resource_by_vdev(vdev);
 	if (list == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
 		return -1;
 	}
 
@@ -893,7 +893,8 @@ ifcvf_dev_config(int vid)
 	update_datapath(internal);
 
 	if (rte_vhost_host_notifier_ctrl(vid, true) != 0)
-		DRV_LOG(NOTICE, "vDPA (%d): software relay is used.", did);
+		DRV_LOG(NOTICE, "vDPA (%s): software relay is used.",
+				vdev->device->name);
 
 	return 0;
 }
@@ -901,14 +902,14 @@ ifcvf_dev_config(int vid)
 static int
 ifcvf_dev_close(int vid)
 {
-	int did;
+	struct rte_vdpa_device *vdev;
 	struct internal_list *list;
 	struct ifcvf_internal *internal;
 
-	did = rte_vhost_get_vdpa_device_id(vid);
-	list = find_internal_resource_by_did(did);
+	vdev = rte_vdpa_get_device(rte_vhost_get_vdpa_device_id(vid));
+	list = find_internal_resource_by_vdev(vdev);
 	if (list == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
 		return -1;
 	}
 
@@ -940,15 +941,15 @@ static int
 ifcvf_set_features(int vid)
 {
 	uint64_t features = 0;
-	int did;
+	struct rte_vdpa_device *vdev;
 	struct internal_list *list;
 	struct ifcvf_internal *internal;
 	uint64_t log_base = 0, log_size = 0;
 
-	did = rte_vhost_get_vdpa_device_id(vid);
-	list = find_internal_resource_by_did(did);
+	vdev = rte_vdpa_get_device(rte_vhost_get_vdpa_device_id(vid));
+	list = find_internal_resource_by_vdev(vdev);
 	if (list == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
 		return -1;
 	}
 
@@ -973,13 +974,13 @@ ifcvf_set_features(int vid)
 static int
 ifcvf_get_vfio_group_fd(int vid)
 {
-	int did;
+	struct rte_vdpa_device *vdev;
 	struct internal_list *list;
 
-	did = rte_vhost_get_vdpa_device_id(vid);
-	list = find_internal_resource_by_did(did);
+	vdev = rte_vdpa_get_device(rte_vhost_get_vdpa_device_id(vid));
+	list = find_internal_resource_by_vdev(vdev);
 	if (list == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
 		return -1;
 	}
 
@@ -989,13 +990,13 @@ ifcvf_get_vfio_group_fd(int vid)
 static int
 ifcvf_get_vfio_device_fd(int vid)
 {
-	int did;
+	struct rte_vdpa_device *vdev;
 	struct internal_list *list;
 
-	did = rte_vhost_get_vdpa_device_id(vid);
-	list = find_internal_resource_by_did(did);
+	vdev = rte_vdpa_get_device(rte_vhost_get_vdpa_device_id(vid));
+	list = find_internal_resource_by_vdev(vdev);
 	if (list == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
 		return -1;
 	}
 
@@ -1005,16 +1006,16 @@ ifcvf_get_vfio_device_fd(int vid)
 static int
 ifcvf_get_notify_area(int vid, int qid, uint64_t *offset, uint64_t *size)
 {
-	int did;
+	struct rte_vdpa_device *vdev;
 	struct internal_list *list;
 	struct ifcvf_internal *internal;
 	struct vfio_region_info reg = { .argsz = sizeof(reg) };
 	int ret;
 
-	did = rte_vhost_get_vdpa_device_id(vid);
-	list = find_internal_resource_by_did(did);
+	vdev = rte_vdpa_get_device(rte_vhost_get_vdpa_device_id(vid));
+	list = find_internal_resource_by_vdev(vdev);
 	if (list == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
 		return -1;
 	}
 
@@ -1035,13 +1036,13 @@ ifcvf_get_notify_area(int vid, int qid, uint64_t *offset, uint64_t *size)
 }
 
 static int
-ifcvf_get_queue_num(int did, uint32_t *queue_num)
+ifcvf_get_queue_num(struct rte_vdpa_device *vdev, uint32_t *queue_num)
 {
 	struct internal_list *list;
 
-	list = find_internal_resource_by_did(did);
+	list = find_internal_resource_by_vdev(vdev);
 	if (list == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
 		return -1;
 	}
 
@@ -1051,13 +1052,13 @@ ifcvf_get_queue_num(int did, uint32_t *queue_num)
 }
 
 static int
-ifcvf_get_vdpa_features(int did, uint64_t *features)
+ifcvf_get_vdpa_features(struct rte_vdpa_device *vdev, uint64_t *features)
 {
 	struct internal_list *list;
 
-	list = find_internal_resource_by_did(did);
+	list = find_internal_resource_by_vdev(vdev);
 	if (list == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
 		return -1;
 	}
 
@@ -1073,8 +1074,10 @@ ifcvf_get_vdpa_features(int did, uint64_t *features)
 		 1ULL << VHOST_USER_PROTOCOL_F_HOST_NOTIFIER | \
 		 1ULL << VHOST_USER_PROTOCOL_F_LOG_SHMFD)
 static int
-ifcvf_get_protocol_features(int did __rte_unused, uint64_t *features)
+ifcvf_get_protocol_features(struct rte_vdpa_device *vdev, uint64_t *features)
 {
+	RTE_SET_USED(vdev);
+
 	*features = VDPA_SUPPORTED_PROTOCOL_FEATURES;
 	return 0;
 }
@@ -1185,8 +1188,8 @@ ifcvf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	}
 	internal->sw_lm = sw_fallback_lm;
 
-	internal->did = rte_vdpa_register_device(&pci_dev->device, &ifcvf_ops);
-	if (internal->did < 0) {
+	internal->vdev = rte_vdpa_register_device(&pci_dev->device, &ifcvf_ops);
+	if (internal->vdev == NULL) {
 		DRV_LOG(ERR, "failed to register device %s", pci_dev->name);
 		goto error;
 	}
@@ -1229,7 +1232,7 @@ ifcvf_pci_remove(struct rte_pci_device *pci_dev)
 
 	rte_pci_unmap_device(internal->pdev);
 	rte_vfio_container_destroy(internal->vfio_container_fd);
-	rte_vdpa_unregister_device(internal->did);
+	rte_vdpa_unregister_device(internal->vdev);
 
 	pthread_mutex_lock(&internal_list_lock);
 	TAILQ_REMOVE(&internal_list, list, next);
diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c b/drivers/vdpa/mlx5/mlx5_vdpa.c
index e8255c7d7e..cbf5c94c78 100644
--- a/drivers/vdpa/mlx5/mlx5_vdpa.c
+++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
@@ -39,21 +39,21 @@ static pthread_mutex_t priv_list_lock = PTHREAD_MUTEX_INITIALIZER;
 int mlx5_vdpa_logtype;
 
 static struct mlx5_vdpa_priv *
-mlx5_vdpa_find_priv_resource_by_did(int did)
+mlx5_vdpa_find_priv_resource_by_vdev(struct rte_vdpa_device *vdev)
 {
 	struct mlx5_vdpa_priv *priv;
 	int found = 0;
 
 	pthread_mutex_lock(&priv_list_lock);
 	TAILQ_FOREACH(priv, &priv_list, next) {
-		if (did == priv->id) {
+		if (vdev == priv->vdev) {
 			found = 1;
 			break;
 		}
 	}
 	pthread_mutex_unlock(&priv_list_lock);
 	if (!found) {
-		DRV_LOG(ERR, "Invalid device id: %d.", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p.", vdev);
 		rte_errno = EINVAL;
 		return NULL;
 	}
@@ -61,12 +61,13 @@ mlx5_vdpa_find_priv_resource_by_did(int did)
 }
 
 static int
-mlx5_vdpa_get_queue_num(int did, uint32_t *queue_num)
+mlx5_vdpa_get_queue_num(struct rte_vdpa_device *vdev, uint32_t *queue_num)
 {
-	struct mlx5_vdpa_priv *priv = mlx5_vdpa_find_priv_resource_by_did(did);
+	struct mlx5_vdpa_priv *priv =
+		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 
 	if (priv == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d.", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p.", vdev);
 		return -1;
 	}
 	*queue_num = priv->caps.max_num_virtio_queues;
@@ -74,12 +75,13 @@ mlx5_vdpa_get_queue_num(int did, uint32_t *queue_num)
 }
 
 static int
-mlx5_vdpa_get_vdpa_features(int did, uint64_t *features)
+mlx5_vdpa_get_vdpa_features(struct rte_vdpa_device *vdev, uint64_t *features)
 {
-	struct mlx5_vdpa_priv *priv = mlx5_vdpa_find_priv_resource_by_did(did);
+	struct mlx5_vdpa_priv *priv =
+		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 
 	if (priv == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d.", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p.", vdev);
 		return -1;
 	}
 	*features = MLX5_VDPA_DEFAULT_FEATURES;
@@ -99,12 +101,14 @@ mlx5_vdpa_get_vdpa_features(int did, uint64_t *features)
 }
 
 static int
-mlx5_vdpa_get_protocol_features(int did, uint64_t *features)
+mlx5_vdpa_get_protocol_features(struct rte_vdpa_device *vdev,
+		uint64_t *features)
 {
-	struct mlx5_vdpa_priv *priv = mlx5_vdpa_find_priv_resource_by_did(did);
+	struct mlx5_vdpa_priv *priv =
+		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 
 	if (priv == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d.", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p.", vdev);
 		return -1;
 	}
 	*features = MLX5_VDPA_PROTOCOL_FEATURES;
@@ -114,11 +118,13 @@ mlx5_vdpa_get_protocol_features(int did, uint64_t *features)
 static int
 mlx5_vdpa_set_vring_state(int vid, int vring, int state)
 {
-	int did = rte_vhost_get_vdpa_device_id(vid);
-	struct mlx5_vdpa_priv *priv = mlx5_vdpa_find_priv_resource_by_did(did);
+	struct rte_vdpa_device *vdev = rte_vdpa_get_device(
+			rte_vhost_get_vdpa_device_id(vid));
+	struct mlx5_vdpa_priv *priv =
+		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 
 	if (priv == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d.", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p.", vdev);
 		return -EINVAL;
 	}
 	if (vring >= (int)priv->caps.max_num_virtio_queues * 2) {
@@ -154,14 +160,16 @@ mlx5_vdpa_direct_db_prepare(struct mlx5_vdpa_priv *priv)
 static int
 mlx5_vdpa_features_set(int vid)
 {
-	int did = rte_vhost_get_vdpa_device_id(vid);
-	struct mlx5_vdpa_priv *priv = mlx5_vdpa_find_priv_resource_by_did(did);
+	struct rte_vdpa_device *vdev = rte_vdpa_get_device(
+			rte_vhost_get_vdpa_device_id(vid));
+	struct mlx5_vdpa_priv *priv =
+		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 	uint64_t log_base, log_size;
 	uint64_t features;
 	int ret;
 
 	if (priv == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d.", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p.", vdev);
 		return -EINVAL;
 	}
 	ret = rte_vhost_get_negotiated_features(vid, &features);
@@ -193,12 +201,14 @@ mlx5_vdpa_features_set(int vid)
 static int
 mlx5_vdpa_dev_close(int vid)
 {
-	int did = rte_vhost_get_vdpa_device_id(vid);
-	struct mlx5_vdpa_priv *priv = mlx5_vdpa_find_priv_resource_by_did(did);
+	struct rte_vdpa_device *vdev = rte_vdpa_get_device(
+			rte_vhost_get_vdpa_device_id(vid));
+	struct mlx5_vdpa_priv *priv =
+		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 	int ret = 0;
 
 	if (priv == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d.", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p.", vdev);
 		return -1;
 	}
 	if (priv->configured)
@@ -217,11 +227,13 @@ mlx5_vdpa_dev_close(int vid)
 static int
 mlx5_vdpa_dev_config(int vid)
 {
-	int did = rte_vhost_get_vdpa_device_id(vid);
-	struct mlx5_vdpa_priv *priv = mlx5_vdpa_find_priv_resource_by_did(did);
+	struct rte_vdpa_device *vdev = rte_vdpa_get_device(
+			rte_vhost_get_vdpa_device_id(vid));
+	struct mlx5_vdpa_priv *priv =
+		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 
 	if (priv == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d.", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p.", vdev);
 		return -EINVAL;
 	}
 	if (priv->configured && mlx5_vdpa_dev_close(vid)) {
@@ -243,11 +255,13 @@ mlx5_vdpa_dev_config(int vid)
 static int
 mlx5_vdpa_get_device_fd(int vid)
 {
-	int did = rte_vhost_get_vdpa_device_id(vid);
-	struct mlx5_vdpa_priv *priv = mlx5_vdpa_find_priv_resource_by_did(did);
+	struct rte_vdpa_device *vdev = rte_vdpa_get_device(
+			rte_vhost_get_vdpa_device_id(vid));
+	struct mlx5_vdpa_priv *priv =
+		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 
 	if (priv == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d.", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p.", vdev);
 		return -EINVAL;
 	}
 	return priv->ctx->cmd_fd;
@@ -256,17 +270,19 @@ mlx5_vdpa_get_device_fd(int vid)
 static int
 mlx5_vdpa_get_notify_area(int vid, int qid, uint64_t *offset, uint64_t *size)
 {
-	int did = rte_vhost_get_vdpa_device_id(vid);
-	struct mlx5_vdpa_priv *priv = mlx5_vdpa_find_priv_resource_by_did(did);
+	struct rte_vdpa_device *vdev = rte_vdpa_get_device(
+			rte_vhost_get_vdpa_device_id(vid));
+	struct mlx5_vdpa_priv *priv =
+		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 
 	RTE_SET_USED(qid);
 	if (priv == NULL) {
-		DRV_LOG(ERR, "Invalid device id: %d.", did);
+		DRV_LOG(ERR, "Invalid vDPA device: %p.", vdev);
 		return -EINVAL;
 	}
 	if (!priv->var) {
-		DRV_LOG(ERR, "VAR was not created for device %d, is the device"
-			" configured?.", did);
+		DRV_LOG(ERR, "VAR was not created for device %p, is the device"
+			" configured?.", vdev);
 		return -EINVAL;
 	}
 	*offset = priv->var->mmap_off;
@@ -507,8 +523,9 @@ mlx5_vdpa_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 		DRV_LOG(ERR, "Failed to allocate VAR %u.\n", errno);
 		goto error;
 	}
-	priv->id = rte_vdpa_register_device(&pci_dev->device, &mlx5_vdpa_ops);
-	if (priv->id < 0) {
+	priv->vdev = rte_vdpa_register_device(&pci_dev->device,
+			&mlx5_vdpa_ops);
+	if (priv->vdev == NULL) {
 		DRV_LOG(ERR, "Failed to register vDPA device.");
 		rte_errno = rte_errno ? rte_errno : EINVAL;
 		goto error;
diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.h b/drivers/vdpa/mlx5/mlx5_vdpa.h
index 50ee3c5870..ec8503f543 100644
--- a/drivers/vdpa/mlx5/mlx5_vdpa.h
+++ b/drivers/vdpa/mlx5/mlx5_vdpa.h
@@ -101,7 +101,7 @@ struct mlx5_vdpa_priv {
 	TAILQ_ENTRY(mlx5_vdpa_priv) next;
 	uint8_t configured;
 	uint8_t direct_notifier; /* Whether direct notifier is on or off. */
-	int id; /* vDPA device id. */
+	struct rte_vdpa_device *vdev; /* vDPA device. */
 	int vid; /* vhost device id. */
 	struct ibv_context *ctx; /* Device context. */
 	struct rte_pci_device *pci_dev;
diff --git a/examples/vdpa/main.c b/examples/vdpa/main.c
index c12da69574..6b92c712e8 100644
--- a/examples/vdpa/main.c
+++ b/examples/vdpa/main.c
@@ -282,13 +282,13 @@ static void cmd_list_vdpa_devices_parsed(
 		vdev = rte_vdpa_get_device(did);
 		if (!vdev)
 			continue;
-		if (vdev->ops->get_queue_num(did, &queue_num) < 0) {
+		if (vdev->ops->get_queue_num(vdev, &queue_num) < 0) {
 			RTE_LOG(ERR, VDPA,
 				"failed to get vdpa queue number "
 				"for device id %d.\n", did);
 			continue;
 		}
-		if (vdev->ops->get_features(did, &features) < 0) {
+		if (vdev->ops->get_features(vdev, &features) < 0) {
 			RTE_LOG(ERR, VDPA,
 				"failed to get vdpa features "
 				"for device id %d.\n", did);
diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
index 33037d39ea..e7c785102d 100644
--- a/lib/librte_vhost/rte_vdpa.h
+++ b/lib/librte_vhost/rte_vdpa.h
@@ -18,18 +18,21 @@
 
 #define MAX_VDPA_NAME_LEN 128
 
+struct rte_vdpa_device;
+
 /**
  * vdpa device operations
  */
 struct rte_vdpa_dev_ops {
 	/** Get capabilities of this device */
-	int (*get_queue_num)(int did, uint32_t *queue_num);
+	int (*get_queue_num)(struct rte_vdpa_device *dev, uint32_t *queue_num);
 
 	/** Get supported features of this device */
-	int (*get_features)(int did, uint64_t *features);
+	int (*get_features)(struct rte_vdpa_device *dev, uint64_t *features);
 
 	/** Get supported protocol features of this device */
-	int (*get_protocol_features)(int did, uint64_t *protocol_features);
+	int (*get_protocol_features)(struct rte_vdpa_device *dev,
+			uint64_t *protocol_features);
 
 	/** Driver configure/close the device */
 	int (*dev_conf)(int vid);
@@ -79,10 +82,10 @@ struct rte_vdpa_device {
  * @param ops
  *  the vdpa device operations
  * @return
- *  device id on success, -1 on failure
+ *  vDPA device pointer on success, NULL on failure
  */
 __rte_experimental
-int
+struct rte_vdpa_device *
 rte_vdpa_register_device(struct rte_device *rte_dev,
 		struct rte_vdpa_dev_ops *ops);
 
@@ -93,13 +96,13 @@ rte_vdpa_register_device(struct rte_device *rte_dev,
  * Unregister a vdpa device
  *
  * @param did
- *  vdpa device id
+ *  vDPA device pointer
  * @return
  *  device id on success, -1 on failure
  */
 __rte_experimental
 int
-rte_vdpa_unregister_device(int did);
+rte_vdpa_unregister_device(struct rte_vdpa_device *);
 
 /**
  * @warning
diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 0a66ef9767..da575b608c 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -712,7 +712,7 @@ rte_vhost_driver_get_features(const char *path, uint64_t *features)
 		goto unlock_exit;
 	}
 
-	if (vdpa_dev->ops->get_features(did, &vdpa_features) < 0) {
+	if (vdpa_dev->ops->get_features(vdpa_dev, &vdpa_features) < 0) {
 		VHOST_LOG_CONFIG(ERR,
 				"failed to get vdpa features "
 				"for socket file %s.\n", path);
@@ -767,7 +767,7 @@ rte_vhost_driver_get_protocol_features(const char *path,
 		goto unlock_exit;
 	}
 
-	if (vdpa_dev->ops->get_protocol_features(did,
+	if (vdpa_dev->ops->get_protocol_features(vdpa_dev,
 				&vdpa_protocol_features) < 0) {
 		VHOST_LOG_CONFIG(ERR,
 				"failed to get vdpa protocol features "
@@ -809,7 +809,7 @@ rte_vhost_driver_get_queue_num(const char *path, uint32_t *queue_num)
 		goto unlock_exit;
 	}
 
-	if (vdpa_dev->ops->get_queue_num(did, &vdpa_queue_num) < 0) {
+	if (vdpa_dev->ops->get_queue_num(vdpa_dev, &vdpa_queue_num) < 0) {
 		VHOST_LOG_CONFIG(ERR,
 				"failed to get vdpa queue number "
 				"for socket file %s.\n", path);
diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
index 5abc5a2a7c..5ad96fe8a6 100644
--- a/lib/librte_vhost/vdpa.c
+++ b/lib/librte_vhost/vdpa.c
@@ -18,52 +18,6 @@
 static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE];
 static uint32_t vdpa_device_num;
 
-int
-rte_vdpa_register_device(struct rte_device *rte_dev,
-		struct rte_vdpa_dev_ops *ops)
-{
-	struct rte_vdpa_device *dev;
-	int i;
-
-	if (vdpa_device_num >= MAX_VHOST_DEVICE || ops == NULL)
-		return -1;
-
-	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
-		dev = &vdpa_devices[i];
-		if (dev->ops == NULL)
-			continue;
-
-		if (dev->device == rte_dev)
-			return -1;
-	}
-
-	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
-		if (vdpa_devices[i].ops == NULL)
-			break;
-	}
-
-	if (i == MAX_VHOST_DEVICE)
-		return -1;
-
-	dev = &vdpa_devices[i];
-	dev->device = rte_dev;
-	dev->ops = ops;
-	vdpa_device_num++;
-
-	return i;
-}
-
-int
-rte_vdpa_unregister_device(int did)
-{
-	if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did].ops == NULL)
-		return -1;
-
-	memset(&vdpa_devices[did], 0, sizeof(struct rte_vdpa_device));
-	vdpa_device_num--;
-
-	return did;
-}
 
 int
 rte_vdpa_find_device_id(struct rte_vdpa_device *dev)
@@ -116,6 +70,55 @@ rte_vdpa_get_device(int did)
 	return &vdpa_devices[did];
 }
 
+struct rte_vdpa_device *
+rte_vdpa_register_device(struct rte_device *rte_dev,
+		struct rte_vdpa_dev_ops *ops)
+{
+	struct rte_vdpa_device *dev;
+	int i;
+
+	if (vdpa_device_num >= MAX_VHOST_DEVICE || ops == NULL)
+		return NULL;
+
+	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
+		dev = &vdpa_devices[i];
+		if (dev->ops == NULL)
+			continue;
+
+		if (dev->device == rte_dev)
+			return NULL;
+	}
+
+	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
+		if (vdpa_devices[i].ops == NULL)
+			break;
+	}
+
+	if (i == MAX_VHOST_DEVICE)
+		return NULL;
+
+	dev = &vdpa_devices[i];
+	dev->device = rte_dev;
+	dev->ops = ops;
+	vdpa_device_num++;
+
+	return dev;
+}
+
+int
+rte_vdpa_unregister_device(struct rte_vdpa_device *vdev)
+{
+	int did = rte_vdpa_find_device_id(vdev);
+
+	if (did < 0 || vdpa_devices[did].ops == NULL)
+		return -1;
+
+	memset(&vdpa_devices[did], 0, sizeof(struct rte_vdpa_device));
+	vdpa_device_num--;
+
+	return 0;
+}
+
 int
 rte_vdpa_get_device_num(void)
 {
-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH 06/14] vhost: replace vDPA device ID in Vhost
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
                   ` (4 preceding siblings ...)
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 05/14] vhost: replace device ID in vDPA ops Maxime Coquelin
@ 2020-06-11 21:37 ` Maxime Coquelin
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 07/14] vhost: replace device ID in applications Maxime Coquelin
                   ` (8 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin

This removes the notion of device ID in Vhost library
as a preliminary step to get rid of the vDPA device ID.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/vdpa/ifc/ifcvf_vdpa.c          | 12 ++++----
 drivers/vdpa/mlx5/mlx5_vdpa.c          | 18 ++++-------
 examples/vdpa/main.c                   |  9 +++++-
 lib/librte_vhost/rte_vhost.h           | 20 ++++++------
 lib/librte_vhost/rte_vhost_version.map |  4 +--
 lib/librte_vhost/socket.c              | 42 ++++++++++----------------
 lib/librte_vhost/vhost.c               | 19 +++++-------
 lib/librte_vhost/vhost.h               |  8 ++---
 lib/librte_vhost/vhost_user.c          | 28 +++++------------
 9 files changed, 66 insertions(+), 94 deletions(-)

diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c b/drivers/vdpa/ifc/ifcvf_vdpa.c
index a04da16bbe..a8fdedba06 100644
--- a/drivers/vdpa/ifc/ifcvf_vdpa.c
+++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
@@ -880,7 +880,7 @@ ifcvf_dev_config(int vid)
 	struct internal_list *list;
 	struct ifcvf_internal *internal;
 
-	vdev = rte_vdpa_get_device(rte_vhost_get_vdpa_device_id(vid));
+	vdev = rte_vhost_get_vdpa_device(vid);
 	list = find_internal_resource_by_vdev(vdev);
 	if (list == NULL) {
 		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
@@ -906,7 +906,7 @@ ifcvf_dev_close(int vid)
 	struct internal_list *list;
 	struct ifcvf_internal *internal;
 
-	vdev = rte_vdpa_get_device(rte_vhost_get_vdpa_device_id(vid));
+	vdev = rte_vhost_get_vdpa_device(vid);
 	list = find_internal_resource_by_vdev(vdev);
 	if (list == NULL) {
 		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
@@ -946,7 +946,7 @@ ifcvf_set_features(int vid)
 	struct ifcvf_internal *internal;
 	uint64_t log_base = 0, log_size = 0;
 
-	vdev = rte_vdpa_get_device(rte_vhost_get_vdpa_device_id(vid));
+	vdev = rte_vhost_get_vdpa_device(vid);
 	list = find_internal_resource_by_vdev(vdev);
 	if (list == NULL) {
 		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
@@ -977,7 +977,7 @@ ifcvf_get_vfio_group_fd(int vid)
 	struct rte_vdpa_device *vdev;
 	struct internal_list *list;
 
-	vdev = rte_vdpa_get_device(rte_vhost_get_vdpa_device_id(vid));
+	vdev = rte_vhost_get_vdpa_device(vid);
 	list = find_internal_resource_by_vdev(vdev);
 	if (list == NULL) {
 		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
@@ -993,7 +993,7 @@ ifcvf_get_vfio_device_fd(int vid)
 	struct rte_vdpa_device *vdev;
 	struct internal_list *list;
 
-	vdev = rte_vdpa_get_device(rte_vhost_get_vdpa_device_id(vid));
+	vdev = rte_vhost_get_vdpa_device(vid);
 	list = find_internal_resource_by_vdev(vdev);
 	if (list == NULL) {
 		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
@@ -1012,7 +1012,7 @@ ifcvf_get_notify_area(int vid, int qid, uint64_t *offset, uint64_t *size)
 	struct vfio_region_info reg = { .argsz = sizeof(reg) };
 	int ret;
 
-	vdev = rte_vdpa_get_device(rte_vhost_get_vdpa_device_id(vid));
+	vdev = rte_vhost_get_vdpa_device(vid);
 	list = find_internal_resource_by_vdev(vdev);
 	if (list == NULL) {
 		DRV_LOG(ERR, "Invalid vDPA device: %p", vdev);
diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c b/drivers/vdpa/mlx5/mlx5_vdpa.c
index cbf5c94c78..c1b9e68516 100644
--- a/drivers/vdpa/mlx5/mlx5_vdpa.c
+++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
@@ -118,8 +118,7 @@ mlx5_vdpa_get_protocol_features(struct rte_vdpa_device *vdev,
 static int
 mlx5_vdpa_set_vring_state(int vid, int vring, int state)
 {
-	struct rte_vdpa_device *vdev = rte_vdpa_get_device(
-			rte_vhost_get_vdpa_device_id(vid));
+	struct rte_vdpa_device *vdev = rte_vhost_get_vdpa_device(vid);
 	struct mlx5_vdpa_priv *priv =
 		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 
@@ -160,8 +159,7 @@ mlx5_vdpa_direct_db_prepare(struct mlx5_vdpa_priv *priv)
 static int
 mlx5_vdpa_features_set(int vid)
 {
-	struct rte_vdpa_device *vdev = rte_vdpa_get_device(
-			rte_vhost_get_vdpa_device_id(vid));
+	struct rte_vdpa_device *vdev = rte_vhost_get_vdpa_device(vid);
 	struct mlx5_vdpa_priv *priv =
 		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 	uint64_t log_base, log_size;
@@ -201,8 +199,7 @@ mlx5_vdpa_features_set(int vid)
 static int
 mlx5_vdpa_dev_close(int vid)
 {
-	struct rte_vdpa_device *vdev = rte_vdpa_get_device(
-			rte_vhost_get_vdpa_device_id(vid));
+	struct rte_vdpa_device *vdev = rte_vhost_get_vdpa_device(vid);
 	struct mlx5_vdpa_priv *priv =
 		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 	int ret = 0;
@@ -227,8 +224,7 @@ mlx5_vdpa_dev_close(int vid)
 static int
 mlx5_vdpa_dev_config(int vid)
 {
-	struct rte_vdpa_device *vdev = rte_vdpa_get_device(
-			rte_vhost_get_vdpa_device_id(vid));
+	struct rte_vdpa_device *vdev = rte_vhost_get_vdpa_device(vid);
 	struct mlx5_vdpa_priv *priv =
 		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 
@@ -255,8 +251,7 @@ mlx5_vdpa_dev_config(int vid)
 static int
 mlx5_vdpa_get_device_fd(int vid)
 {
-	struct rte_vdpa_device *vdev = rte_vdpa_get_device(
-			rte_vhost_get_vdpa_device_id(vid));
+	struct rte_vdpa_device *vdev = rte_vhost_get_vdpa_device(vid);
 	struct mlx5_vdpa_priv *priv =
 		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 
@@ -270,8 +265,7 @@ mlx5_vdpa_get_device_fd(int vid)
 static int
 mlx5_vdpa_get_notify_area(int vid, int qid, uint64_t *offset, uint64_t *size)
 {
-	struct rte_vdpa_device *vdev = rte_vdpa_get_device(
-			rte_vhost_get_vdpa_device_id(vid));
+	struct rte_vdpa_device *vdev = rte_vhost_get_vdpa_device(vid);
 	struct mlx5_vdpa_priv *priv =
 		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
 
diff --git a/examples/vdpa/main.c b/examples/vdpa/main.c
index 6b92c712e8..8d707c9e2d 100644
--- a/examples/vdpa/main.c
+++ b/examples/vdpa/main.c
@@ -145,6 +145,7 @@ start_vdpa(struct vdpa_port *vport)
 {
 	int ret;
 	char *socket_path = vport->ifname;
+	struct rte_vdpa_device *vdev;
 	int did = vport->did;
 
 	if (client_mode)
@@ -169,7 +170,13 @@ start_vdpa(struct vdpa_port *vport)
 			"register driver ops failed: %s\n",
 			socket_path);
 
-	ret = rte_vhost_driver_attach_vdpa_device(socket_path, did);
+	vdev = rte_vdpa_get_device(did);
+	if (!vdev)
+		rte_exit(EXIT_FAILURE,
+			"vDPA device retrieval failed: %p\n",
+			vdev);
+
+	ret = rte_vhost_driver_attach_vdpa_device(socket_path, vdev);
 	if (ret != 0)
 		rte_exit(EXIT_FAILURE,
 			"attach vdpa device failed: %s\n",
diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
index d43669f2c2..2fbc364643 100644
--- a/lib/librte_vhost/rte_vhost.h
+++ b/lib/librte_vhost/rte_vhost.h
@@ -90,6 +90,7 @@ extern "C" {
 #define VHOST_USER_F_PROTOCOL_FEATURES	30
 #endif
 
+struct rte_vdpa_device;
 
 /**
  * Information relating to memory regions including offsets to
@@ -402,14 +403,15 @@ int rte_vhost_driver_unregister(const char *path);
  *
  * @param path
  *  The vhost-user socket file path
- * @param did
- *  Device id
+ * @param dev
+ *  vDPA device pointer
  * @return
  *  0 on success, -1 on failure
  */
 __rte_experimental
 int
-rte_vhost_driver_attach_vdpa_device(const char *path, int did);
+rte_vhost_driver_attach_vdpa_device(const char *path,
+		struct rte_vdpa_device *dev);
 
 /**
  * Unset the vdpa device id
@@ -429,11 +431,11 @@ rte_vhost_driver_detach_vdpa_device(const char *path);
  * @param path
  *  The vhost-user socket file path
  * @return
- *  Device id, -1 on failure
+ *  vDPA device pointer, NULL on failure
  */
 __rte_experimental
-int
-rte_vhost_driver_get_vdpa_device_id(const char *path);
+struct rte_vdpa_device *
+rte_vhost_driver_get_vdpa_device(const char *path);
 
 /**
  * Set the feature bits the vhost-user driver supports.
@@ -977,11 +979,11 @@ rte_vhost_extern_callback_register(int vid,
  * @param vid
  *  vhost device id
  * @return
- *  device id
+ *  vDPA device pointer on success, NULL on failure
  */
 __rte_experimental
-int
-rte_vhost_get_vdpa_device_id(int vid);
+struct rte_vdpa_device *
+rte_vhost_get_vdpa_device(int vid);
 
 /**
  * Notify the guest that should get virtio configuration space from backend.
diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
index 1abfff8a0c..4872bc4f35 100644
--- a/lib/librte_vhost/rte_vhost_version.map
+++ b/lib/librte_vhost/rte_vhost_version.map
@@ -40,8 +40,8 @@ EXPERIMENTAL {
 	rte_vdpa_get_device_num;
 	rte_vhost_driver_attach_vdpa_device;
 	rte_vhost_driver_detach_vdpa_device;
-	rte_vhost_driver_get_vdpa_device_id;
-	rte_vhost_get_vdpa_device_id;
+	rte_vhost_driver_get_vdpa_device;
+	rte_vhost_get_vdpa_device;
 	rte_vhost_driver_get_protocol_features;
 	rte_vhost_driver_get_queue_num;
 	rte_vhost_get_log_base;
diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index da575b608c..49267cebf9 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -55,12 +55,7 @@ struct vhost_user_socket {
 
 	uint64_t protocol_features;
 
-	/*
-	 * Device id to identify a specific backend device.
-	 * It's set to -1 for the default software implementation.
-	 * If valid, one socket can have 1 connection only.
-	 */
-	int vdpa_dev_id;
+	struct rte_vdpa_device *vdpa_dev;
 
 	struct vhost_device_ops const *notify_ops;
 };
@@ -230,7 +225,7 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
 
 	vhost_set_builtin_virtio_net(vid, vsocket->use_builtin_virtio_net);
 
-	vhost_attach_vdpa_device(vid, vsocket->vdpa_dev_id);
+	vhost_attach_vdpa_device(vid, vsocket->vdpa_dev);
 
 	if (vsocket->dequeue_zero_copy)
 		vhost_enable_dequeue_zero_copy(vid);
@@ -578,17 +573,18 @@ find_vhost_user_socket(const char *path)
 }
 
 int
-rte_vhost_driver_attach_vdpa_device(const char *path, int did)
+rte_vhost_driver_attach_vdpa_device(const char *path,
+		struct rte_vdpa_device *dev)
 {
 	struct vhost_user_socket *vsocket;
 
-	if (rte_vdpa_get_device(did) == NULL || path == NULL)
+	if (dev == NULL || path == NULL)
 		return -1;
 
 	pthread_mutex_lock(&vhost_user.mutex);
 	vsocket = find_vhost_user_socket(path);
 	if (vsocket)
-		vsocket->vdpa_dev_id = did;
+		vsocket->vdpa_dev = dev;
 	pthread_mutex_unlock(&vhost_user.mutex);
 
 	return vsocket ? 0 : -1;
@@ -602,25 +598,25 @@ rte_vhost_driver_detach_vdpa_device(const char *path)
 	pthread_mutex_lock(&vhost_user.mutex);
 	vsocket = find_vhost_user_socket(path);
 	if (vsocket)
-		vsocket->vdpa_dev_id = -1;
+		vsocket->vdpa_dev = NULL;
 	pthread_mutex_unlock(&vhost_user.mutex);
 
 	return vsocket ? 0 : -1;
 }
 
-int
-rte_vhost_driver_get_vdpa_device_id(const char *path)
+struct rte_vdpa_device *
+rte_vhost_driver_get_vdpa_device(const char *path)
 {
 	struct vhost_user_socket *vsocket;
-	int did = -1;
+	struct rte_vdpa_device *dev = NULL;
 
 	pthread_mutex_lock(&vhost_user.mutex);
 	vsocket = find_vhost_user_socket(path);
 	if (vsocket)
-		did = vsocket->vdpa_dev_id;
+		dev = vsocket->vdpa_dev;
 	pthread_mutex_unlock(&vhost_user.mutex);
 
-	return did;
+	return dev;
 }
 
 int
@@ -693,7 +689,6 @@ rte_vhost_driver_get_features(const char *path, uint64_t *features)
 	struct vhost_user_socket *vsocket;
 	uint64_t vdpa_features;
 	struct rte_vdpa_device *vdpa_dev;
-	int did = -1;
 	int ret = 0;
 
 	pthread_mutex_lock(&vhost_user.mutex);
@@ -705,8 +700,7 @@ rte_vhost_driver_get_features(const char *path, uint64_t *features)
 		goto unlock_exit;
 	}
 
-	did = vsocket->vdpa_dev_id;
-	vdpa_dev = rte_vdpa_get_device(did);
+	vdpa_dev = vsocket->vdpa_dev;
 	if (!vdpa_dev || !vdpa_dev->ops->get_features) {
 		*features = vsocket->features;
 		goto unlock_exit;
@@ -748,7 +742,6 @@ rte_vhost_driver_get_protocol_features(const char *path,
 	struct vhost_user_socket *vsocket;
 	uint64_t vdpa_protocol_features;
 	struct rte_vdpa_device *vdpa_dev;
-	int did = -1;
 	int ret = 0;
 
 	pthread_mutex_lock(&vhost_user.mutex);
@@ -760,8 +753,7 @@ rte_vhost_driver_get_protocol_features(const char *path,
 		goto unlock_exit;
 	}
 
-	did = vsocket->vdpa_dev_id;
-	vdpa_dev = rte_vdpa_get_device(did);
+	vdpa_dev = vsocket->vdpa_dev;
 	if (!vdpa_dev || !vdpa_dev->ops->get_protocol_features) {
 		*protocol_features = vsocket->protocol_features;
 		goto unlock_exit;
@@ -790,7 +782,6 @@ rte_vhost_driver_get_queue_num(const char *path, uint32_t *queue_num)
 	struct vhost_user_socket *vsocket;
 	uint32_t vdpa_queue_num;
 	struct rte_vdpa_device *vdpa_dev;
-	int did = -1;
 	int ret = 0;
 
 	pthread_mutex_lock(&vhost_user.mutex);
@@ -802,8 +793,7 @@ rte_vhost_driver_get_queue_num(const char *path, uint32_t *queue_num)
 		goto unlock_exit;
 	}
 
-	did = vsocket->vdpa_dev_id;
-	vdpa_dev = rte_vdpa_get_device(did);
+	vdpa_dev = vsocket->vdpa_dev;
 	if (!vdpa_dev || !vdpa_dev->ops->get_queue_num) {
 		*queue_num = VHOST_MAX_QUEUE_PAIRS;
 		goto unlock_exit;
@@ -878,7 +868,7 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
 			"error: failed to init connection mutex\n");
 		goto out_free;
 	}
-	vsocket->vdpa_dev_id = -1;
+	vsocket->vdpa_dev = NULL;
 	vsocket->dequeue_zero_copy = flags & RTE_VHOST_USER_DEQUEUE_ZERO_COPY;
 	vsocket->extbuf = flags & RTE_VHOST_USER_EXTBUF_SUPPORT;
 	vsocket->linearbuf = flags & RTE_VHOST_USER_LINEARBUF_SUPPORT;
diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 0266318440..0d822d6a3f 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -633,7 +633,6 @@ vhost_new_device(void)
 	dev->vid = i;
 	dev->flags = VIRTIO_DEV_BUILTIN_VIRTIO_NET;
 	dev->slave_req_fd = -1;
-	dev->vdpa_dev_id = -1;
 	dev->postcopy_ufd = -1;
 	rte_spinlock_init(&dev->slave_req_lock);
 
@@ -644,11 +643,9 @@ void
 vhost_destroy_device_notify(struct virtio_net *dev)
 {
 	struct rte_vdpa_device *vdpa_dev;
-	int did;
 
 	if (dev->flags & VIRTIO_DEV_RUNNING) {
-		did = dev->vdpa_dev_id;
-		vdpa_dev = rte_vdpa_get_device(did);
+		vdpa_dev = dev->vdpa_dev;
 		if (vdpa_dev && vdpa_dev->ops->dev_close)
 			vdpa_dev->ops->dev_close(dev->vid);
 		dev->flags &= ~VIRTIO_DEV_RUNNING;
@@ -677,17 +674,14 @@ vhost_destroy_device(int vid)
 }
 
 void
-vhost_attach_vdpa_device(int vid, int did)
+vhost_attach_vdpa_device(int vid, struct rte_vdpa_device *vdpa_dev)
 {
 	struct virtio_net *dev = get_device(vid);
 
 	if (dev == NULL)
 		return;
 
-	if (rte_vdpa_get_device(did) == NULL)
-		return;
-
-	dev->vdpa_dev_id = did;
+	dev->vdpa_dev = vdpa_dev;
 }
 
 void
@@ -1402,14 +1396,15 @@ rte_vhost_rx_queue_count(int vid, uint16_t qid)
 	return ret;
 }
 
-int rte_vhost_get_vdpa_device_id(int vid)
+struct rte_vdpa_device *
+rte_vhost_get_vdpa_device(int vid)
 {
 	struct virtio_net *dev = get_device(vid);
 
 	if (dev == NULL)
-		return -1;
+		return NULL;
 
-	return dev->vdpa_dev_id;
+	return dev->vdpa_dev;
 }
 
 int rte_vhost_get_log_base(int vid, uint64_t *log_base,
diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index df98d15de6..819857a65a 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -377,11 +377,7 @@ struct virtio_net {
 	int			postcopy_ufd;
 	int			postcopy_listening;
 
-	/*
-	 * Device id to identify a specific backend device.
-	 * It's set to -1 for the default software implementation.
-	 */
-	int			vdpa_dev_id;
+	struct rte_vdpa_device *vdpa_dev;
 
 	/* context data for the external message handlers */
 	void			*extern_data;
@@ -639,7 +635,7 @@ void free_vq(struct virtio_net *dev, struct vhost_virtqueue *vq);
 
 int alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx);
 
-void vhost_attach_vdpa_device(int vid, int did);
+void vhost_attach_vdpa_device(int vid, struct rte_vdpa_device *dev);
 
 void vhost_set_ifname(int, const char *if_name, unsigned int if_len);
 void vhost_enable_dequeue_zero_copy(int vid);
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 84bebad792..a43f0e9735 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -315,7 +315,6 @@ vhost_user_set_features(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	uint64_t features = msg->payload.u64;
 	uint64_t vhost_features = 0;
 	struct rte_vdpa_device *vdpa_dev;
-	int did = -1;
 
 	if (validate_msg_fds(msg, 0) != 0)
 		return RTE_VHOST_MSG_RESULT_ERR;
@@ -384,8 +383,7 @@ vhost_user_set_features(struct virtio_net **pdev, struct VhostUserMsg *msg,
 		}
 	}
 
-	did = dev->vdpa_dev_id;
-	vdpa_dev = rte_vdpa_get_device(did);
+	vdpa_dev = dev->vdpa_dev;
 	if (vdpa_dev && vdpa_dev->ops->set_features)
 		vdpa_dev->ops->set_features(dev->vid);
 
@@ -1971,7 +1969,6 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
 	int enable = (int)msg->payload.state.num;
 	int index = (int)msg->payload.state.index;
 	struct rte_vdpa_device *vdpa_dev;
-	int did = -1;
 
 	if (validate_msg_fds(msg, 0) != 0)
 		return RTE_VHOST_MSG_RESULT_ERR;
@@ -1980,8 +1977,7 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
 		"set queue enable: %d to qp idx: %d\n",
 		enable, index);
 
-	did = dev->vdpa_dev_id;
-	vdpa_dev = rte_vdpa_get_device(did);
+	vdpa_dev = dev->vdpa_dev;
 	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
 		vdpa_dev->ops->set_vring_state(dev->vid, index, enable);
 
@@ -2156,7 +2152,6 @@ vhost_user_send_rarp(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	struct virtio_net *dev = *pdev;
 	uint8_t *mac = (uint8_t *)&msg->payload.u64;
 	struct rte_vdpa_device *vdpa_dev;
-	int did = -1;
 
 	if (validate_msg_fds(msg, 0) != 0)
 		return RTE_VHOST_MSG_RESULT_ERR;
@@ -2174,8 +2169,7 @@ vhost_user_send_rarp(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	 * copied before the flag is set.
 	 */
 	__atomic_store_n(&dev->broadcast_rarp, 1, __ATOMIC_RELEASE);
-	did = dev->vdpa_dev_id;
-	vdpa_dev = rte_vdpa_get_device(did);
+	vdpa_dev = dev->vdpa_dev;
 	if (vdpa_dev && vdpa_dev->ops->migration_done)
 		vdpa_dev->ops->migration_done(dev->vid);
 
@@ -2622,7 +2616,6 @@ vhost_user_msg_handler(int vid, int fd)
 	struct virtio_net *dev;
 	struct VhostUserMsg msg;
 	struct rte_vdpa_device *vdpa_dev;
-	int did = -1;
 	int ret;
 	int unlock_required = 0;
 	bool handled;
@@ -2814,8 +2807,7 @@ vhost_user_msg_handler(int vid, int fd)
 		}
 	}
 
-	did = dev->vdpa_dev_id;
-	vdpa_dev = rte_vdpa_get_device(did);
+	vdpa_dev = dev->vdpa_dev;
 	if (vdpa_dev && virtio_is_ready(dev) &&
 			!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) &&
 			msg.request.master == VHOST_USER_SET_VRING_CALL) {
@@ -2964,7 +2956,7 @@ int rte_vhost_host_notifier_ctrl(int vid, bool enable)
 {
 	struct virtio_net *dev;
 	struct rte_vdpa_device *vdpa_dev;
-	int vfio_device_fd, did, ret = 0;
+	int vfio_device_fd, ret = 0;
 	uint64_t offset, size;
 	unsigned int i;
 
@@ -2972,9 +2964,9 @@ int rte_vhost_host_notifier_ctrl(int vid, bool enable)
 	if (!dev)
 		return -ENODEV;
 
-	did = dev->vdpa_dev_id;
-	if (did < 0)
-		return -EINVAL;
+	vdpa_dev = dev->vdpa_dev;
+	if (vdpa_dev == NULL)
+		return -ENODEV;
 
 	if (!(dev->features & (1ULL << VIRTIO_F_VERSION_1)) ||
 	    !(dev->features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) ||
@@ -2986,10 +2978,6 @@ int rte_vhost_host_notifier_ctrl(int vid, bool enable)
 			(1ULL << VHOST_USER_PROTOCOL_F_HOST_NOTIFIER)))
 		return -ENOTSUP;
 
-	vdpa_dev = rte_vdpa_get_device(did);
-	if (!vdpa_dev)
-		return -ENODEV;
-
 	RTE_FUNC_PTR_OR_ERR_RET(vdpa_dev->ops->get_vfio_device_fd, -ENOTSUP);
 	RTE_FUNC_PTR_OR_ERR_RET(vdpa_dev->ops->get_notify_area, -ENOTSUP);
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH 07/14] vhost: replace device ID in applications
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
                   ` (5 preceding siblings ...)
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 06/14] vhost: replace vDPA device ID in Vhost Maxime Coquelin
@ 2020-06-11 21:37 ` Maxime Coquelin
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 08/14] vhost: remove useless vDPA API Maxime Coquelin
                   ` (7 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin

This patch replaces the use of vDPA device ID with
vDPA device pointer. The goals is to remove the vDPA
device ID to avoid condusion with the Vhost ID.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 examples/vdpa/main.c                   | 79 +++++++++++++-------------
 lib/librte_vhost/rte_vdpa.h            | 21 ++++++-
 lib/librte_vhost/rte_vhost_version.map |  3 +-
 lib/librte_vhost/vdpa.c                | 19 +++++--
 4 files changed, 75 insertions(+), 47 deletions(-)

diff --git a/examples/vdpa/main.c b/examples/vdpa/main.c
index 8d707c9e2d..4417500d9b 100644
--- a/examples/vdpa/main.c
+++ b/examples/vdpa/main.c
@@ -26,7 +26,7 @@
 
 struct vdpa_port {
 	char ifname[MAX_PATH_LEN];
-	int did;
+	struct rte_vdpa_device *dev;
 	int vid;
 	uint64_t flags;
 };
@@ -101,16 +101,23 @@ static int
 new_device(int vid)
 {
 	char ifname[MAX_PATH_LEN];
+	struct rte_device *dev;
 	int i;
 
 	rte_vhost_get_ifname(vid, ifname, sizeof(ifname));
 	for (i = 0; i < MAX_VDPA_SAMPLE_PORTS; i++) {
-		if (strncmp(ifname, vports[i].ifname, MAX_PATH_LEN) == 0) {
-			printf("\nnew port %s, did: %d\n",
-					ifname, vports[i].did);
-			vports[i].vid = vid;
-			break;
+		if (strncmp(ifname, vports[i].ifname, MAX_PATH_LEN))
+			continue;
+
+		dev = rte_vdpa_get_rte_device(vports[i].dev);
+		if (!dev) {
+			RTE_LOG(ERR, VDPA,
+				"Failed to get generic device for port %d\n", i);
+			continue;
 		}
+		printf("\nnew port %s, device : %s\n", ifname, dev->name);
+		vports[i].vid = vid;
+		break;
 	}
 
 	if (i >= MAX_VDPA_SAMPLE_PORTS)
@@ -122,16 +129,24 @@ new_device(int vid)
 static void
 destroy_device(int vid)
 {
+	struct rte_device *dev;
 	char ifname[MAX_PATH_LEN];
 	int i;
 
 	rte_vhost_get_ifname(vid, ifname, sizeof(ifname));
 	for (i = 0; i < MAX_VDPA_SAMPLE_PORTS; i++) {
-		if (strcmp(ifname, vports[i].ifname) == 0) {
-			printf("\ndestroy port %s, did: %d\n",
-					ifname, vports[i].did);
-			break;
+		if (strncmp(ifname, vports[i].ifname, MAX_PATH_LEN))
+			continue;
+
+		dev = rte_vdpa_get_rte_device(vports[i].dev);
+		if (!dev) {
+			RTE_LOG(ERR, VDPA,
+				"Failed to get generic device for port %d\n", i);
+			continue;
 		}
+
+		printf("\ndestroy port %s, device: %s\n", ifname, dev->name);
+		break;
 	}
 }
 
@@ -145,8 +160,6 @@ start_vdpa(struct vdpa_port *vport)
 {
 	int ret;
 	char *socket_path = vport->ifname;
-	struct rte_vdpa_device *vdev;
-	int did = vport->did;
 
 	if (client_mode)
 		vport->flags |= RTE_VHOST_USER_CLIENT;
@@ -170,13 +183,7 @@ start_vdpa(struct vdpa_port *vport)
 			"register driver ops failed: %s\n",
 			socket_path);
 
-	vdev = rte_vdpa_get_device(did);
-	if (!vdev)
-		rte_exit(EXIT_FAILURE,
-			"vDPA device retrieval failed: %p\n",
-			vdev);
-
-	ret = rte_vhost_driver_attach_vdpa_device(socket_path, vdev);
+	ret = rte_vhost_driver_attach_vdpa_device(socket_path, vport->dev);
 	if (ret != 0)
 		rte_exit(EXIT_FAILURE,
 			"attach vdpa device failed: %s\n",
@@ -274,35 +281,31 @@ static void cmd_list_vdpa_devices_parsed(
 		struct cmdline *cl,
 		__rte_unused void *data)
 {
-	int did;
 	uint32_t queue_num;
 	uint64_t features;
 	struct rte_vdpa_device *vdev;
 	struct rte_device *dev;
 	struct rte_dev_iterator dev_iter;
 
-	cmdline_printf(cl, "device id\tdevice name\tqueue num\tsupported features\n");
+	cmdline_printf(cl, "device name\tqueue num\tsupported features\n");
 	RTE_DEV_FOREACH(dev, "class=vdpa", &dev_iter) {
-		did = rte_vdpa_find_device_id_by_name(dev->name);
-		if (did < 0)
-			continue;
-		vdev = rte_vdpa_get_device(did);
+		vdev = rte_vdpa_find_device_by_name(dev->name);
 		if (!vdev)
 			continue;
 		if (vdev->ops->get_queue_num(vdev, &queue_num) < 0) {
 			RTE_LOG(ERR, VDPA,
 				"failed to get vdpa queue number "
-				"for device id %d.\n", did);
+				"for device %s.\n", dev->name);
 			continue;
 		}
 		if (vdev->ops->get_features(vdev, &features) < 0) {
 			RTE_LOG(ERR, VDPA,
 				"failed to get vdpa features "
-				"for device id %d.\n", did);
+				"for device %s.\n", dev->name);
 			continue;
 		}
-		cmdline_printf(cl, "%d\t\t%s\t\t%" PRIu32 "\t\t0x%" PRIx64 "\n",
-			did, dev->name, queue_num, features);
+		cmdline_printf(cl, "%s\t\t%" PRIu32 "\t\t0x%" PRIx64 "\n",
+			dev->name, queue_num, features);
 	}
 }
 
@@ -330,18 +333,18 @@ static void cmd_create_vdpa_port_parsed(void *parsed_result,
 		struct cmdline *cl,
 		__rte_unused void *data)
 {
-	int did;
+	struct rte_vdpa_device *dev;
 	struct cmd_create_result *res = parsed_result;
 
 	rte_strscpy(vports[devcnt].ifname, res->socket_path, MAX_PATH_LEN);
-	did = rte_vdpa_find_device_id_by_name(res->bdf);
-	if (did < 0) {
+	dev = rte_vdpa_find_device_by_name(res->bdf);
+	if (dev == NULL) {
 		cmdline_printf(cl, "Unable to find vdpa device id for %s.\n",
 				res->bdf);
 		return;
 	}
 
-	vports[devcnt].did = did;
+	vports[devcnt].dev = dev;
 
 	if (start_vdpa(&vports[devcnt]) == 0)
 		devcnt++;
@@ -403,9 +406,9 @@ int
 main(int argc, char *argv[])
 {
 	char ch;
-	int did;
 	int ret;
 	struct cmdline *cl;
+	struct rte_vdpa_device *vdev;
 	struct rte_device *dev;
 	struct rte_dev_iterator dev_iter;
 
@@ -434,12 +437,12 @@ main(int argc, char *argv[])
 		cmdline_stdin_exit(cl);
 	} else {
 		RTE_DEV_FOREACH(dev, "class=vdpa", &dev_iter) {
-			did = rte_vdpa_find_device_id_by_name(dev->name);
-			if (did < 0) {
-				rte_panic("Failed to find device id for %s\n",
+			vdev = rte_vdpa_find_device_by_name(dev->name);
+			if (vdev == NULL) {
+				rte_panic("Failed to find vDPA dev for %s\n",
 						dev->name);
 			}
-			vports[devcnt].did = did;
+			vports[devcnt].dev = vdev;
 			snprintf(vports[devcnt].ifname, MAX_PATH_LEN, "%s%d",
 					iface, devcnt);
 
diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
index e7c785102d..6188bf6483 100644
--- a/lib/librte_vhost/rte_vdpa.h
+++ b/lib/librte_vhost/rte_vdpa.h
@@ -113,11 +113,26 @@ rte_vdpa_unregister_device(struct rte_vdpa_device *);
  * @param name
  *  the vdpa device name
  * @return
- *  device id on success, -1 on failure
+ *  vDPA device pointer on success, NULL on failure
  */
 __rte_experimental
-int
-rte_vdpa_find_device_id_by_name(const char *name);
+struct rte_vdpa_device *
+rte_vdpa_find_device_by_name(const char *name);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Get the generic device from the vdpa device
+ *
+ * @param vdpa_dev
+ *  the vdpa device pointer
+ * @return
+ *  generic device pointer on success, NULL on failure
+ */
+__rte_experimental
+struct rte_device *
+rte_vdpa_get_rte_device(struct rte_vdpa_device *vdpa_dev);
 
 /**
  * @warning
diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
index 4872bc4f35..43caab7553 100644
--- a/lib/librte_vhost/rte_vhost_version.map
+++ b/lib/librte_vhost/rte_vhost_version.map
@@ -66,5 +66,6 @@ EXPERIMENTAL {
 	rte_vhost_get_vhost_ring_inflight;
 	rte_vhost_get_vring_base_from_inflight;
 	rte_vhost_slave_config_change;
-	rte_vdpa_find_device_id_by_name;
+	rte_vdpa_find_device_by_name;
+	rte_vdpa_get_rte_device;
 };
diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
index 5ad96fe8a6..9ec5677a78 100644
--- a/lib/librte_vhost/vdpa.c
+++ b/lib/librte_vhost/vdpa.c
@@ -40,14 +40,14 @@ rte_vdpa_find_device_id(struct rte_vdpa_device *dev)
 	return -1;
 }
 
-int
-rte_vdpa_find_device_id_by_name(const char *name)
+struct rte_vdpa_device *
+rte_vdpa_find_device_by_name(const char *name)
 {
 	struct rte_vdpa_device *dev;
 	int i;
 
 	if (name == NULL)
-		return -1;
+		return NULL;
 
 	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
 		dev = &vdpa_devices[i];
@@ -55,10 +55,19 @@ rte_vdpa_find_device_id_by_name(const char *name)
 			continue;
 
 		if (strcmp(dev->device->name, name) == 0)
-			return i;
+			return dev;
 	}
 
-	return -1;
+	return NULL;
+}
+
+struct rte_device *
+rte_vdpa_get_rte_device(struct rte_vdpa_device *vdpa_dev)
+{
+	if (vdpa_dev == NULL)
+		return NULL;
+
+	return vdpa_dev->device;
 }
 
 struct rte_vdpa_device *
-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH 08/14] vhost: remove useless vDPA API
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
                   ` (6 preceding siblings ...)
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 07/14] vhost: replace device ID in applications Maxime Coquelin
@ 2020-06-11 21:37 ` Maxime Coquelin
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 09/14] vhost: use linked-list for vDPA devices Maxime Coquelin
                   ` (6 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin

vDPA is no more used outside of the vDPA internals,
so remove rte_vdpa_get_device() API that is now useless.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/rte_vdpa.h            | 30 -----------------
 lib/librte_vhost/rte_vhost_version.map |  1 -
 lib/librte_vhost/vdpa.c                | 46 ++++++--------------------
 3 files changed, 10 insertions(+), 67 deletions(-)

diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
index 6188bf6483..ca6cb0e882 100644
--- a/lib/librte_vhost/rte_vdpa.h
+++ b/lib/librte_vhost/rte_vdpa.h
@@ -134,36 +134,6 @@ __rte_experimental
 struct rte_device *
 rte_vdpa_get_rte_device(struct rte_vdpa_device *vdpa_dev);
 
-/**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice
- *
- * Find the device id of a vdpa device
- *
- * @param addr
- *  the vdpa device address
- * @return
- *  device id on success, -1 on failure
- */
-__rte_experimental
-int
-rte_vdpa_find_device_id(struct rte_vdpa_device *dev);
-
-/**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice
- *
- * Find a vdpa device based on device id
- *
- * @param did
- *  device id
- * @return
- *  rte_vdpa_device on success, NULL on failure
- */
-__rte_experimental
-struct rte_vdpa_device *
-rte_vdpa_get_device(int did);
-
 /**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
index 43caab7553..27d63ce6fe 100644
--- a/lib/librte_vhost/rte_vhost_version.map
+++ b/lib/librte_vhost/rte_vhost_version.map
@@ -36,7 +36,6 @@ EXPERIMENTAL {
 	rte_vdpa_register_device;
 	rte_vdpa_unregister_device;
 	rte_vdpa_find_device_id;
-	rte_vdpa_get_device;
 	rte_vdpa_get_device_num;
 	rte_vhost_driver_attach_vdpa_device;
 	rte_vhost_driver_detach_vdpa_device;
diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
index 9ec5677a78..ae4df4521d 100644
--- a/lib/librte_vhost/vdpa.c
+++ b/lib/librte_vhost/vdpa.c
@@ -19,27 +19,6 @@ static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE];
 static uint32_t vdpa_device_num;
 
 
-int
-rte_vdpa_find_device_id(struct rte_vdpa_device *dev)
-{
-	struct rte_vdpa_device *tmp_dev;
-	int i;
-
-	if (dev == NULL)
-		return -1;
-
-	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
-		tmp_dev = &vdpa_devices[i];
-		if (tmp_dev->ops == NULL)
-			continue;
-
-		if (tmp_dev == dev)
-			return i;
-	}
-
-	return -1;
-}
-
 struct rte_vdpa_device *
 rte_vdpa_find_device_by_name(const char *name)
 {
@@ -70,15 +49,6 @@ rte_vdpa_get_rte_device(struct rte_vdpa_device *vdpa_dev)
 	return vdpa_dev->device;
 }
 
-struct rte_vdpa_device *
-rte_vdpa_get_device(int did)
-{
-	if (did < 0 || did >= MAX_VHOST_DEVICE)
-		return NULL;
-
-	return &vdpa_devices[did];
-}
-
 struct rte_vdpa_device *
 rte_vdpa_register_device(struct rte_device *rte_dev,
 		struct rte_vdpa_dev_ops *ops)
@@ -117,15 +87,19 @@ rte_vdpa_register_device(struct rte_device *rte_dev,
 int
 rte_vdpa_unregister_device(struct rte_vdpa_device *vdev)
 {
-	int did = rte_vdpa_find_device_id(vdev);
+	int i;
 
-	if (did < 0 || vdpa_devices[did].ops == NULL)
-		return -1;
+	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
+		if (vdev != &vdpa_devices[i])
+			continue;
 
-	memset(&vdpa_devices[did], 0, sizeof(struct rte_vdpa_device));
-	vdpa_device_num--;
+		memset(vdev, 0, sizeof(struct rte_vdpa_device));
+		vdpa_device_num--;
 
-	return 0;
+		return 0;
+	}
+
+	return -1;
 }
 
 int
-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH 09/14] vhost: use linked-list for vDPA devices
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
                   ` (7 preceding siblings ...)
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 08/14] vhost: remove useless vDPA API Maxime Coquelin
@ 2020-06-11 21:37 ` Maxime Coquelin
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 10/14] vhost: introduce wrappers for some vDPA ops Maxime Coquelin
                   ` (5 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin

There is no more notion of device ID outside of vdpa.c.
We can now move from array to linked-list model for keeping
track of the vDPA devices.

There is no point in using array here, as all vDPA API are
used from the control path, so no performance concerns.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/rte_vdpa.h |   1 +
 lib/librte_vhost/vdpa.c     | 135 ++++++++++++++++++------------------
 2 files changed, 70 insertions(+), 66 deletions(-)

diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
index ca6cb0e882..011befdc71 100644
--- a/lib/librte_vhost/rte_vdpa.h
+++ b/lib/librte_vhost/rte_vdpa.h
@@ -65,6 +65,7 @@ struct rte_vdpa_dev_ops {
  * vdpa device structure includes device address and device operations.
  */
 struct rte_vdpa_device {
+	TAILQ_ENTRY(rte_vdpa_device) next;
 	/** Generic device information */
 	struct rte_device *device;
 	/** vdpa device operations */
diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
index ae4df4521d..8f4778e2fa 100644
--- a/lib/librte_vhost/vdpa.c
+++ b/lib/librte_vhost/vdpa.c
@@ -9,35 +9,54 @@
  */
 
 #include <stdbool.h>
+#include <sys/queue.h>
 
 #include <rte_class.h>
 #include <rte_malloc.h>
+#include <rte_spinlock.h>
+#include <rte_tailq.h>
+
 #include "rte_vdpa.h"
 #include "vhost.h"
 
-static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE];
+/** Double linked list of vDPA devices. */
+TAILQ_HEAD(vdpa_device_list, rte_vdpa_device);
+
+static struct vdpa_device_list vdpa_device_list =
+		TAILQ_HEAD_INITIALIZER(vdpa_device_list);
+static rte_spinlock_t vdpa_device_list_lock = RTE_SPINLOCK_INITIALIZER;
 static uint32_t vdpa_device_num;
 
 
-struct rte_vdpa_device *
-rte_vdpa_find_device_by_name(const char *name)
+/* Unsafe, needs to be called with vdpa_device_list_lock held */
+static struct rte_vdpa_device *
+__vdpa_find_device_by_name(const char *name)
 {
-	struct rte_vdpa_device *dev;
-	int i;
+	struct rte_vdpa_device *dev, *ret = NULL;
 
 	if (name == NULL)
 		return NULL;
 
-	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
-		dev = &vdpa_devices[i];
-		if (dev->ops == NULL)
-			continue;
-
-		if (strcmp(dev->device->name, name) == 0)
-			return dev;
+	TAILQ_FOREACH(dev, &vdpa_device_list, next) {
+		if (strcmp(dev->device->name, name) == 0) {
+			ret = dev;
+			break;
+		}
 	}
 
-	return NULL;
+	return ret;
+}
+
+struct rte_vdpa_device *
+rte_vdpa_find_device_by_name(const char *name)
+{
+	struct rte_vdpa_device *dev;
+
+	rte_spinlock_lock(&vdpa_device_list_lock);
+	dev = __vdpa_find_device_by_name(name);
+	rte_spinlock_unlock(&vdpa_device_list_lock);
+
+	return dev;
 }
 
 struct rte_device *
@@ -54,52 +73,52 @@ rte_vdpa_register_device(struct rte_device *rte_dev,
 		struct rte_vdpa_dev_ops *ops)
 {
 	struct rte_vdpa_device *dev;
-	int i;
 
-	if (vdpa_device_num >= MAX_VHOST_DEVICE || ops == NULL)
+	if (ops == NULL)
 		return NULL;
 
-	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
-		dev = &vdpa_devices[i];
-		if (dev->ops == NULL)
-			continue;
-
-		if (dev->device == rte_dev)
-			return NULL;
+	rte_spinlock_lock(&vdpa_device_list_lock);
+	/* Check the device hasn't been register already */
+	dev = __vdpa_find_device_by_name(rte_dev->name);
+	if (dev) {
+		dev = NULL;
+		goto out_unlock;
 	}
 
-	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
-		if (vdpa_devices[i].ops == NULL)
-			break;
-	}
-
-	if (i == MAX_VHOST_DEVICE)
-		return NULL;
+	dev = rte_zmalloc(NULL, sizeof(*dev), 0);
+	if (!dev)
+		goto out_unlock;
 
-	dev = &vdpa_devices[i];
 	dev->device = rte_dev;
 	dev->ops = ops;
+	TAILQ_INSERT_TAIL(&vdpa_device_list, dev, next);
 	vdpa_device_num++;
+out_unlock:
+	rte_spinlock_unlock(&vdpa_device_list_lock);
 
 	return dev;
 }
 
 int
-rte_vdpa_unregister_device(struct rte_vdpa_device *vdev)
+rte_vdpa_unregister_device(struct rte_vdpa_device *dev)
 {
-	int i;
+	struct rte_vdpa_device *cur_dev, *tmp_dev;
+	int ret = -1;
 
-	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
-		if (vdev != &vdpa_devices[i])
+	rte_spinlock_lock(&vdpa_device_list_lock);
+	TAILQ_FOREACH_SAFE(cur_dev, &vdpa_device_list, next, tmp_dev) {
+		if (dev != cur_dev)
 			continue;
 
-		memset(vdev, 0, sizeof(struct rte_vdpa_device));
+		TAILQ_REMOVE(&vdpa_device_list, dev, next);
+		rte_free(dev);
 		vdpa_device_num--;
-
-		return 0;
+		ret = 0;
+		break;
 	}
+	rte_spinlock_lock(&vdpa_device_list_lock);
 
-	return -1;
+	return ret;
 }
 
 int
@@ -210,14 +229,6 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m)
 	return -1;
 }
 
-static uint16_t
-vdpa_dev_to_id(const struct rte_vdpa_device *dev)
-{
-	if (dev == NULL)
-		return MAX_VHOST_DEVICE;
-	return dev - vdpa_devices;
-}
-
 static int
 vdpa_dev_match(struct rte_vdpa_device *dev,
 	      const struct rte_device *rte_dev)
@@ -237,30 +248,22 @@ vdpa_find_device(const struct rte_vdpa_device *start, rte_vdpa_cmp_t cmp,
 		struct rte_device *rte_dev)
 {
 	struct rte_vdpa_device *dev;
-	ptrdiff_t idx;
 
-	/* Avoid Undefined Behaviour */
-	if (start != NULL &&
-	    (start < &vdpa_devices[0] ||
-	     start >= &vdpa_devices[MAX_VHOST_DEVICE]))
-		return NULL;
-
-	if (start != NULL)
-		idx = vdpa_dev_to_id(start) + 1;
+	rte_spinlock_lock(&vdpa_device_list_lock);
+	if (start == NULL)
+		dev = TAILQ_FIRST(&vdpa_device_list);
 	else
-		idx = 0;
-	for (; idx < MAX_VHOST_DEVICE; idx++) {
-		dev = &vdpa_devices[idx];
-		/*
-		 * ToDo: Certainly better to introduce a state field,
-		 * but rely on ops being set for now.
-		 */
-		if (dev->ops == NULL)
-			continue;
+		dev = TAILQ_NEXT(start, next);
+
+	while (dev != NULL) {
 		if (cmp(dev, rte_dev) == 0)
-			return dev;
+			break;
+
+		dev = TAILQ_NEXT(dev, next);
 	}
-	return NULL;
+	rte_spinlock_unlock(&vdpa_device_list_lock);
+
+	return dev;
 }
 
 static void *
-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH 10/14] vhost: introduce wrappers for some vDPA ops
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
                   ` (8 preceding siblings ...)
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 09/14] vhost: use linked-list for vDPA devices Maxime Coquelin
@ 2020-06-11 21:37 ` Maxime Coquelin
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 11/14] examples/vdpa: use new wrappers instead of ops Maxime Coquelin
                   ` (4 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin

This patch is preliminary work to make the vDPA device
structure opaque to the user application. Some callbacks
of the vDPA devices are used to query capabilities before
attaching to a Vhost port. This patch introduces wrappers
for these ops.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/rte_vdpa.h            | 52 ++++++++++++++++++++++++++
 lib/librte_vhost/rte_vhost_version.map |  3 ++
 lib/librte_vhost/vdpa.c                | 28 ++++++++++++++
 3 files changed, 83 insertions(+)

diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
index 011befdc71..e830014d5a 100644
--- a/lib/librte_vhost/rte_vdpa.h
+++ b/lib/librte_vhost/rte_vdpa.h
@@ -185,4 +185,56 @@ rte_vhost_host_notifier_ctrl(int vid, bool enable);
 __rte_experimental
 int
 rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Get number of queue pairs supported by the vDPA device
+ *
+ * @param dev
+ *  vDP device pointer
+ * @param queue_num
+ *  pointer on where the number of queue is stored
+ * @return
+ *  0 on success, -1 on failure
+ */
+__rte_experimental
+int
+rte_vdpa_get_queue_num(struct rte_vdpa_device *dev, uint32_t *queue_num);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Get the Virtio features supported by the vDPA device
+ *
+ * @param dev
+ *  vDP device pointer
+ * @param features
+ *  pointer on where the supported features are stored
+ * @return
+ *  0 on success, -1 on failure
+ */
+__rte_experimental
+int
+rte_vdpa_get_features(struct rte_vdpa_device *dev, uint64_t *features);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Get the Vhost-user protocol features supported by the vDPA device
+ *
+ * @param dev
+ *  vDP device pointer
+ * @param features
+ *  pointer on where the supported protocol features are stored
+ * @return
+ *  0 on success, -1 on failure
+ */
+__rte_experimental
+int
+rte_vdpa_get_protocol_features(struct rte_vdpa_device *dev, uint64_t *features);
+
 #endif /* _RTE_VDPA_H_ */
diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
index 27d63ce6fe..6c74a3ec4f 100644
--- a/lib/librte_vhost/rte_vhost_version.map
+++ b/lib/librte_vhost/rte_vhost_version.map
@@ -67,4 +67,7 @@ EXPERIMENTAL {
 	rte_vhost_slave_config_change;
 	rte_vdpa_find_device_by_name;
 	rte_vdpa_get_rte_device;
+	rte_vdpa_get_queue_num;
+	rte_vdpa_get_features;
+	rte_vdpa_get_protocol_features;
 };
diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
index 8f4778e2fa..f00544b3ea 100644
--- a/lib/librte_vhost/vdpa.c
+++ b/lib/librte_vhost/vdpa.c
@@ -229,6 +229,34 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m)
 	return -1;
 }
 
+int
+rte_vdpa_get_queue_num(struct rte_vdpa_device *dev, uint32_t *queue_num)
+{
+	if (dev == NULL || dev->ops == NULL || dev->ops->get_queue_num == NULL)
+		return -1;
+
+	return dev->ops->get_queue_num(dev, queue_num);
+}
+
+int
+rte_vdpa_get_features(struct rte_vdpa_device *dev, uint64_t *features)
+{
+	if (dev == NULL || dev->ops == NULL || dev->ops->get_features == NULL)
+		return -1;
+
+	return dev->ops->get_features(dev, features);
+}
+
+int
+rte_vdpa_get_protocol_features(struct rte_vdpa_device *dev, uint64_t *features)
+{
+	if (dev == NULL || dev->ops == NULL ||
+			dev->ops->get_protocol_features == NULL)
+		return -1;
+
+	return dev->ops->get_protocol_features(dev, features);
+}
+
 static int
 vdpa_dev_match(struct rte_vdpa_device *dev,
 	      const struct rte_device *rte_dev)
-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH 11/14] examples/vdpa: use new wrappers instead of ops
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
                   ` (9 preceding siblings ...)
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 10/14] vhost: introduce wrappers for some vDPA ops Maxime Coquelin
@ 2020-06-11 21:37 ` Maxime Coquelin
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 12/14] examples/vdpa: remove useless device count Maxime Coquelin
                   ` (3 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin

Now that wrappers to query number of queues, Virtio
features and Vhost-user protocol features are available,
let's make the vDPA example to use them.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 examples/vdpa/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/examples/vdpa/main.c b/examples/vdpa/main.c
index 4417500d9b..5fac139c4e 100644
--- a/examples/vdpa/main.c
+++ b/examples/vdpa/main.c
@@ -292,13 +292,13 @@ static void cmd_list_vdpa_devices_parsed(
 		vdev = rte_vdpa_find_device_by_name(dev->name);
 		if (!vdev)
 			continue;
-		if (vdev->ops->get_queue_num(vdev, &queue_num) < 0) {
+		if (rte_vdpa_get_queue_num(vdev, &queue_num) < 0) {
 			RTE_LOG(ERR, VDPA,
 				"failed to get vdpa queue number "
 				"for device %s.\n", dev->name);
 			continue;
 		}
-		if (vdev->ops->get_features(vdev, &features) < 0) {
+		if (rte_vdpa_get_features(vdev, &features) < 0) {
 			RTE_LOG(ERR, VDPA,
 				"failed to get vdpa features "
 				"for device %s.\n", dev->name);
-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH 12/14] examples/vdpa: remove useless device count
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
                   ` (10 preceding siblings ...)
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 11/14] examples/vdpa: use new wrappers instead of ops Maxime Coquelin
@ 2020-06-11 21:37 ` Maxime Coquelin
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 13/14] vhost: remove vDPA device count API Maxime Coquelin
                   ` (2 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin

The VDPA example now uses the vDPA class iterator, so
knowing the number of available devices beforehand is
no longer needed.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 examples/vdpa/main.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/examples/vdpa/main.c b/examples/vdpa/main.c
index 5fac139c4e..c1cd778da6 100644
--- a/examples/vdpa/main.c
+++ b/examples/vdpa/main.c
@@ -34,7 +34,6 @@ struct vdpa_port {
 static struct vdpa_port vports[MAX_VDPA_SAMPLE_PORTS];
 
 static char iface[MAX_PATH_LEN];
-static int dev_total;
 static int devcnt;
 static int interactive;
 static int client_mode;
@@ -219,7 +218,7 @@ static void
 vdpa_sample_quit(void)
 {
 	int i;
-	for (i = 0; i < RTE_MIN(MAX_VDPA_SAMPLE_PORTS, dev_total); i++) {
+	for (i = 0; i < RTE_MIN(MAX_VDPA_SAMPLE_PORTS, devcnt); i++) {
 		if (vports[i].ifname[0] != '\0')
 			close_vdpa(&vports[i]);
 	}
@@ -418,10 +417,6 @@ main(int argc, char *argv[])
 	argc -= ret;
 	argv += ret;
 
-	dev_total = rte_vdpa_get_device_num();
-	if (dev_total <= 0)
-		rte_exit(EXIT_FAILURE, "No available vdpa device found\n");
-
 	signal(SIGINT, signal_handler);
 	signal(SIGTERM, signal_handler);
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH 13/14] vhost: remove vDPA device count API
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
                   ` (11 preceding siblings ...)
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 12/14] examples/vdpa: remove useless device count Maxime Coquelin
@ 2020-06-11 21:37 ` Maxime Coquelin
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 14/14] vhost: split vDPA header file Maxime Coquelin
  2020-06-12  9:38 ` [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Gaëtan Rivet
  14 siblings, 0 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin

This API is no more useful, this patch removes it.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/rte_vdpa.h            | 13 -------------
 lib/librte_vhost/rte_vhost_version.map |  1 -
 lib/librte_vhost/vdpa.c                |  9 ---------
 3 files changed, 23 deletions(-)

diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
index e830014d5a..25c218d0c4 100644
--- a/lib/librte_vhost/rte_vdpa.h
+++ b/lib/librte_vhost/rte_vdpa.h
@@ -135,19 +135,6 @@ __rte_experimental
 struct rte_device *
 rte_vdpa_get_rte_device(struct rte_vdpa_device *vdpa_dev);
 
-/**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice
- *
- * Get current available vdpa device number
- *
- * @return
- *  available vdpa device number
- */
-__rte_experimental
-int
-rte_vdpa_get_device_num(void);
-
 /**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
index 6c74a3ec4f..f4047207d0 100644
--- a/lib/librte_vhost/rte_vhost_version.map
+++ b/lib/librte_vhost/rte_vhost_version.map
@@ -36,7 +36,6 @@ EXPERIMENTAL {
 	rte_vdpa_register_device;
 	rte_vdpa_unregister_device;
 	rte_vdpa_find_device_id;
-	rte_vdpa_get_device_num;
 	rte_vhost_driver_attach_vdpa_device;
 	rte_vhost_driver_detach_vdpa_device;
 	rte_vhost_driver_get_vdpa_device;
diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
index f00544b3ea..3d2f6e2623 100644
--- a/lib/librte_vhost/vdpa.c
+++ b/lib/librte_vhost/vdpa.c
@@ -25,7 +25,6 @@ TAILQ_HEAD(vdpa_device_list, rte_vdpa_device);
 static struct vdpa_device_list vdpa_device_list =
 		TAILQ_HEAD_INITIALIZER(vdpa_device_list);
 static rte_spinlock_t vdpa_device_list_lock = RTE_SPINLOCK_INITIALIZER;
-static uint32_t vdpa_device_num;
 
 
 /* Unsafe, needs to be called with vdpa_device_list_lock held */
@@ -92,7 +91,6 @@ rte_vdpa_register_device(struct rte_device *rte_dev,
 	dev->device = rte_dev;
 	dev->ops = ops;
 	TAILQ_INSERT_TAIL(&vdpa_device_list, dev, next);
-	vdpa_device_num++;
 out_unlock:
 	rte_spinlock_unlock(&vdpa_device_list_lock);
 
@@ -112,7 +110,6 @@ rte_vdpa_unregister_device(struct rte_vdpa_device *dev)
 
 		TAILQ_REMOVE(&vdpa_device_list, dev, next);
 		rte_free(dev);
-		vdpa_device_num--;
 		ret = 0;
 		break;
 	}
@@ -121,12 +118,6 @@ rte_vdpa_unregister_device(struct rte_vdpa_device *dev)
 	return ret;
 }
 
-int
-rte_vdpa_get_device_num(void)
-{
-	return vdpa_device_num;
-}
-
 int
 rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m)
 {
-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [dpdk-dev] [PATCH 14/14] vhost: split vDPA header file
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
                   ` (12 preceding siblings ...)
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 13/14] vhost: remove vDPA device count API Maxime Coquelin
@ 2020-06-11 21:37 ` Maxime Coquelin
  2020-06-12  9:38 ` [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Gaëtan Rivet
  14 siblings, 0 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-11 21:37 UTC (permalink / raw)
  To: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena
  Cc: Maxime Coquelin

This patch split the vDPA header file in two, making
rte_vdpa_device structure opaque to the application.

Applications should only include rte_vdpa.h, while drivers
should include both rte_vdpa.h and rte_vdpa_dev.h.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/vdpa/ifc/ifcvf_vdpa.c   |   1 +
 drivers/vdpa/mlx5/mlx5_vdpa.h   |   1 +
 lib/librte_vhost/Makefile       |   3 +-
 lib/librte_vhost/meson.build    |   3 +-
 lib/librte_vhost/rte_vdpa.h     | 130 ------------------------------
 lib/librte_vhost/rte_vdpa_dev.h | 135 ++++++++++++++++++++++++++++++++
 lib/librte_vhost/vdpa.c         |   1 +
 lib/librte_vhost/vhost.h        |   1 +
 8 files changed, 143 insertions(+), 132 deletions(-)
 create mode 100644 lib/librte_vhost/rte_vdpa_dev.h

diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c b/drivers/vdpa/ifc/ifcvf_vdpa.c
index a8fdedba06..2f1773633c 100644
--- a/drivers/vdpa/ifc/ifcvf_vdpa.c
+++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
@@ -16,6 +16,7 @@
 #include <rte_bus_pci.h>
 #include <rte_vhost.h>
 #include <rte_vdpa.h>
+#include <rte_vdpa_dev.h>
 #include <rte_vfio.h>
 #include <rte_spinlock.h>
 #include <rte_log.h>
diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.h b/drivers/vdpa/mlx5/mlx5_vdpa.h
index ec8503f543..7d98aa4b3f 100644
--- a/drivers/vdpa/mlx5/mlx5_vdpa.h
+++ b/drivers/vdpa/mlx5/mlx5_vdpa.h
@@ -12,6 +12,7 @@
 #pragma GCC diagnostic ignored "-Wpedantic"
 #endif
 #include <rte_vdpa.h>
+#include <rte_vdpa_dev.h>
 #include <rte_vhost.h>
 #ifdef PEDANTIC
 #pragma GCC diagnostic error "-Wpedantic"
diff --git a/lib/librte_vhost/Makefile b/lib/librte_vhost/Makefile
index e592795f22..b7ff7dc4b3 100644
--- a/lib/librte_vhost/Makefile
+++ b/lib/librte_vhost/Makefile
@@ -41,7 +41,8 @@ SRCS-$(CONFIG_RTE_LIBRTE_VHOST) := fd_man.c iotlb.c socket.c vhost.c \
 					vhost_user.c virtio_net.c vdpa.c
 
 # install includes
-SYMLINK-$(CONFIG_RTE_LIBRTE_VHOST)-include += rte_vhost.h rte_vdpa.h
+SYMLINK-$(CONFIG_RTE_LIBRTE_VHOST)-include += rte_vhost.h rte_vdpa.h \
+						rte_vdpa_dev.h
 
 # only compile vhost crypto when cryptodev is enabled
 ifeq ($(CONFIG_RTE_LIBRTE_CRYPTODEV),y)
diff --git a/lib/librte_vhost/meson.build b/lib/librte_vhost/meson.build
index f80632848c..882a0eaf41 100644
--- a/lib/librte_vhost/meson.build
+++ b/lib/librte_vhost/meson.build
@@ -21,5 +21,6 @@ cflags += '-fno-strict-aliasing'
 sources = files('fd_man.c', 'iotlb.c', 'socket.c', 'vdpa.c',
 		'vhost.c', 'vhost_user.c',
 		'virtio_net.c', 'vhost_crypto.c')
-headers = files('rte_vhost.h', 'rte_vdpa.h', 'rte_vhost_crypto.h')
+headers = files('rte_vhost.h', 'rte_vdpa.h', 'rte_vdpa_dev.h',
+		'rte_vhost_crypto.h')
 deps += ['ethdev', 'cryptodev', 'hash', 'pci']
diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
index 25c218d0c4..5bb8462efa 100644
--- a/lib/librte_vhost/rte_vdpa.h
+++ b/lib/librte_vhost/rte_vdpa.h
@@ -11,100 +11,8 @@
  * Device specific vhost lib
  */
 
-#include <stdbool.h>
-
-#include <rte_pci.h>
-#include "rte_vhost.h"
-
-#define MAX_VDPA_NAME_LEN 128
-
 struct rte_vdpa_device;
 
-/**
- * vdpa device operations
- */
-struct rte_vdpa_dev_ops {
-	/** Get capabilities of this device */
-	int (*get_queue_num)(struct rte_vdpa_device *dev, uint32_t *queue_num);
-
-	/** Get supported features of this device */
-	int (*get_features)(struct rte_vdpa_device *dev, uint64_t *features);
-
-	/** Get supported protocol features of this device */
-	int (*get_protocol_features)(struct rte_vdpa_device *dev,
-			uint64_t *protocol_features);
-
-	/** Driver configure/close the device */
-	int (*dev_conf)(int vid);
-	int (*dev_close)(int vid);
-
-	/** Enable/disable this vring */
-	int (*set_vring_state)(int vid, int vring, int state);
-
-	/** Set features when changed */
-	int (*set_features)(int vid);
-
-	/** Destination operations when migration done */
-	int (*migration_done)(int vid);
-
-	/** Get the vfio group fd */
-	int (*get_vfio_group_fd)(int vid);
-
-	/** Get the vfio device fd */
-	int (*get_vfio_device_fd)(int vid);
-
-	/** Get the notify area info of the queue */
-	int (*get_notify_area)(int vid, int qid,
-			uint64_t *offset, uint64_t *size);
-
-	/** Reserved for future extension */
-	void *reserved[5];
-};
-
-/**
- * vdpa device structure includes device address and device operations.
- */
-struct rte_vdpa_device {
-	TAILQ_ENTRY(rte_vdpa_device) next;
-	/** Generic device information */
-	struct rte_device *device;
-	/** vdpa device operations */
-	struct rte_vdpa_dev_ops *ops;
-} __rte_cache_aligned;
-
-/**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice
- *
- * Register a vdpa device
- *
- * @param addr
- *  the vdpa device address
- * @param ops
- *  the vdpa device operations
- * @return
- *  vDPA device pointer on success, NULL on failure
- */
-__rte_experimental
-struct rte_vdpa_device *
-rte_vdpa_register_device(struct rte_device *rte_dev,
-		struct rte_vdpa_dev_ops *ops);
-
-/**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice
- *
- * Unregister a vdpa device
- *
- * @param did
- *  vDPA device pointer
- * @return
- *  device id on success, -1 on failure
- */
-__rte_experimental
-int
-rte_vdpa_unregister_device(struct rte_vdpa_device *);
-
 /**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
@@ -135,44 +43,6 @@ __rte_experimental
 struct rte_device *
 rte_vdpa_get_rte_device(struct rte_vdpa_device *vdpa_dev);
 
-/**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice
- *
- * Enable/Disable host notifier mapping for a vdpa port.
- *
- * @param vid
- *  vhost device id
- * @param enable
- *  true for host notifier map, false for host notifier unmap
- * @return
- *  0 on success, -1 on failure
- */
-__rte_experimental
-int
-rte_vhost_host_notifier_ctrl(int vid, bool enable);
-
-/**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice
- *
- * Synchronize the used ring from mediated ring to guest, log dirty
- * page for each writeable buffer, caller should handle the used
- * ring logging before device stop.
- *
- * @param vid
- *  vhost device id
- * @param qid
- *  vhost queue id
- * @param vring_m
- *  mediated virtio ring pointer
- * @return
- *  number of synced used entries on success, -1 on failure
- */
-__rte_experimental
-int
-rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m);
-
 /**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice
diff --git a/lib/librte_vhost/rte_vdpa_dev.h b/lib/librte_vhost/rte_vdpa_dev.h
new file mode 100644
index 0000000000..7ddfca24e4
--- /dev/null
+++ b/lib/librte_vhost/rte_vdpa_dev.h
@@ -0,0 +1,135 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2018 Intel Corporation
+ */
+
+#ifndef _RTE_VDPA_H_DEV_
+#define _RTE_VDPA_H_DEV_
+
+#include <stdbool.h>
+
+#include "rte_vhost.h"
+
+/**
+ * vdpa device operations
+ */
+struct rte_vdpa_dev_ops {
+	/** Get capabilities of this device */
+	int (*get_queue_num)(struct rte_vdpa_device *dev, uint32_t *queue_num);
+
+	/** Get supported features of this device */
+	int (*get_features)(struct rte_vdpa_device *dev, uint64_t *features);
+
+	/** Get supported protocol features of this device */
+	int (*get_protocol_features)(struct rte_vdpa_device *dev,
+			uint64_t *protocol_features);
+
+	/** Driver configure/close the device */
+	int (*dev_conf)(int vid);
+	int (*dev_close)(int vid);
+
+	/** Enable/disable this vring */
+	int (*set_vring_state)(int vid, int vring, int state);
+
+	/** Set features when changed */
+	int (*set_features)(int vid);
+
+	/** Destination operations when migration done */
+	int (*migration_done)(int vid);
+
+	/** Get the vfio group fd */
+	int (*get_vfio_group_fd)(int vid);
+
+	/** Get the vfio device fd */
+	int (*get_vfio_device_fd)(int vid);
+
+	/** Get the notify area info of the queue */
+	int (*get_notify_area)(int vid, int qid,
+			uint64_t *offset, uint64_t *size);
+
+	/** Reserved for future extension */
+	void *reserved[5];
+};
+
+/**
+ * vdpa device structure includes device address and device operations.
+ */
+struct rte_vdpa_device {
+	TAILQ_ENTRY(rte_vdpa_device) next;
+	/** Generic device information */
+	struct rte_device *device;
+	/** vdpa device operations */
+	struct rte_vdpa_dev_ops *ops;
+};
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Register a vdpa device
+ *
+ * @param rte_dev
+ *  the generic device pointer
+ * @param ops
+ *  the vdpa device operations
+ * @return
+ *  vDPA device pointer on success, NULL on failure
+ */
+__rte_experimental
+struct rte_vdpa_device *
+rte_vdpa_register_device(struct rte_device *rte_dev,
+		struct rte_vdpa_dev_ops *ops);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Unregister a vdpa device
+ *
+ * @param dev
+ *  vDPA device pointer
+ * @return
+ *  device id on success, -1 on failure
+ */
+__rte_experimental
+int
+rte_vdpa_unregister_device(struct rte_vdpa_device *dev);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Enable/Disable host notifier mapping for a vdpa port.
+ *
+ * @param vid
+ *  vhost device id
+ * @param enable
+ *  true for host notifier map, false for host notifier unmap
+ * @return
+ *  0 on success, -1 on failure
+ */
+__rte_experimental
+int
+rte_vhost_host_notifier_ctrl(int vid, bool enable);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Synchronize the used ring from mediated ring to guest, log dirty
+ * page for each writeable buffer, caller should handle the used
+ * ring logging before device stop.
+ *
+ * @param vid
+ *  vhost device id
+ * @param qid
+ *  vhost queue id
+ * @param vring_m
+ *  mediated virtio ring pointer
+ * @return
+ *  number of synced used entries on success, -1 on failure
+ */
+__rte_experimental
+int
+rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m);
+
+#endif /* _RTE_VDPA_DEV_H_ */
diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
index 3d2f6e2623..283294c25e 100644
--- a/lib/librte_vhost/vdpa.c
+++ b/lib/librte_vhost/vdpa.c
@@ -17,6 +17,7 @@
 #include <rte_tailq.h>
 
 #include "rte_vdpa.h"
+#include "rte_vdpa_dev.h"
 #include "vhost.h"
 
 /** Double linked list of vDPA devices. */
diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index 819857a65a..941a42637e 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -22,6 +22,7 @@
 
 #include "rte_vhost.h"
 #include "rte_vdpa.h"
+#include "rte_vdpa_dev.h"
 
 /* Used to indicate that the device is running on a data core */
 #define VIRTIO_DEV_RUNNING 1
-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH 00/14] vDPA API and framework rework
  2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
                   ` (13 preceding siblings ...)
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 14/14] vhost: split vDPA header file Maxime Coquelin
@ 2020-06-12  9:38 ` Gaëtan Rivet
  14 siblings, 0 replies; 25+ messages in thread
From: Gaëtan Rivet @ 2020-06-12  9:38 UTC (permalink / raw)
  To: Maxime Coquelin
  Cc: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena

Hello Maxime,

On 11/06/20 23:37 +0200, Maxime Coquelin wrote:
> This series aims to rework the vDPA framework and
> its API to better fit into the DPDK device model
> and also be more easily consumable by applications.
> 
> Main changes are creating a device class for vDPA,
> which enables applications to iterate vDPA devices
> in a generic way:
> 
> RTE_DEV_FOREACH(dev, "class=vdpa", it) {...}
> 

That's neat, I was wondering if the class layer would become useful
someday.

> Also the notion of vDPA device ID is replaced
> with both application & drivers using the
> rte_vdpa_device as reference. Doing that also
> made possible to store devices references into
> a linked list instead of a static array. Last
> patch makes the rte_vdpa_device structure
> content opaque to the applications, creating
> a clear barrier between application visible
> API and drivers visble ones.
> 
> The first two patches fixes issues in some
> busses iterators, causing segmentation faults
> when iterating only on a device class.
> 
> While reviewing, if you notice further possible
> improvements, please let me know. Target is to
> remove the experimental tag from vDPA APIs in
> next LTS release.
> 
> Thanks to David for giving me some ideas of
> improvements!
> 

I can't really comment on vhost parts and vdpa implems,
but overall this seems a good idea to me.

> Maxime Coquelin (14):
>   bus/dpaa: fix null pointer dereference
>   bus/fslmc: fix null pointer dereference
>   vhost: introduce vDPA devices class
>   vhost: make vDPA framework bus agnostic
>   vhost: replace device ID in vDPA ops
>   vhost: replace vDPA device ID in Vhost
>   vhost: replace device ID in applications
>   vhost: remove useless vDPA API
>   vhost: use linked-list for vDPA devices
>   vhost: introduce wrappers for some vDPA ops
>   examples/vdpa: use new wrappers instead of ops
>   examples/vdpa: remove useless device count
>   vhost: remove vDPA device count API
>   vhost: split vDPA header file
> 
>  drivers/bus/dpaa/dpaa_bus.c            |   5 +
>  drivers/bus/fslmc/fslmc_bus.c          |   5 +
>  drivers/vdpa/ifc/ifcvf_vdpa.c          |  84 ++++-----
>  drivers/vdpa/mlx5/mlx5_vdpa.c          |  85 +++++----
>  drivers/vdpa/mlx5/mlx5_vdpa.h          |   5 +-
>  examples/vdpa/main.c                   | 108 +++++------
>  lib/librte_vhost/Makefile              |   3 +-
>  lib/librte_vhost/meson.build           |   3 +-
>  lib/librte_vhost/rte_vdpa.h            | 172 ++++--------------
>  lib/librte_vhost/rte_vdpa_dev.h        | 135 ++++++++++++++
>  lib/librte_vhost/rte_vhost.h           |  20 ++-
>  lib/librte_vhost/rte_vhost_version.map |  11 +-
>  lib/librte_vhost/socket.c              |  48 ++---
>  lib/librte_vhost/vdpa.c                | 238 ++++++++++++++++---------
>  lib/librte_vhost/vhost.c               |  19 +-
>  lib/librte_vhost/vhost.h               |   9 +-
>  lib/librte_vhost/vhost_user.c          |  28 +--
>  17 files changed, 545 insertions(+), 433 deletions(-)
>  create mode 100644 lib/librte_vhost/rte_vdpa_dev.h
> 
> -- 
> 2.26.2
> 

-- 
Gaëtan

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH 01/14] bus/dpaa: fix null pointer dereference
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 01/14] bus/dpaa: fix null pointer dereference Maxime Coquelin
@ 2020-06-12  9:48   ` Gaëtan Rivet
  0 siblings, 0 replies; 25+ messages in thread
From: Gaëtan Rivet @ 2020-06-12  9:48 UTC (permalink / raw)
  To: Maxime Coquelin
  Cc: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena, stable

On 11/06/20 23:37 +0200, Maxime Coquelin wrote:
> This patches fixes a null pointer derefencing that happens
> when the device string passed to the iterator is NULL. This
> situation can happen when iterating on a class type.
> For example:
> 
> RTE_DEV_FOREACH(dev, "class=eth", &dev_iter) {
>     ...
> }
> 

Given this fix and the next, this seems like an oversight / bug from the
iterator actually.  Those two fixes are still correct but the root cause
should be addressed.

> Fixes: e79df833d3f6 ("bus/dpaa: support hotplug ops")
> Cc: stable@dpdk.org
> Cc: shreyansh.jain@nxp.com
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/bus/dpaa/dpaa_bus.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/bus/dpaa/dpaa_bus.c b/drivers/bus/dpaa/dpaa_bus.c
> index d53fe6083a..216f38acd4 100644
> --- a/drivers/bus/dpaa/dpaa_bus.c
> +++ b/drivers/bus/dpaa/dpaa_bus.c
> @@ -703,6 +703,11 @@ dpaa_bus_dev_iterate(const void *start, const char *str,
>  	struct rte_dpaa_device *dev;
>  	char *dup, *dev_name = NULL;
>  
> +	if (str == NULL) {
> +		DPAA_BUS_DEBUG("No device string\n");
> +		return NULL;
> +	}
> +
>  	/* Expectation is that device would be name=device_name */
>  	if (strncmp(str, "name=", 5) != 0) {
>  		DPAA_BUS_DEBUG("Invalid device string (%s)\n", str);
> -- 
> 2.26.2
> 

-- 
Gaëtan

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH 03/14] vhost: introduce vDPA devices class
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 03/14] vhost: introduce vDPA devices class Maxime Coquelin
@ 2020-06-12 12:37   ` Gaëtan Rivet
  2020-06-15  9:15     ` Maxime Coquelin
  0 siblings, 1 reply; 25+ messages in thread
From: Gaëtan Rivet @ 2020-06-12 12:37 UTC (permalink / raw)
  To: Maxime Coquelin
  Cc: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena

On 11/06/20 23:37 +0200, Maxime Coquelin wrote:
> This patch introduces vDPA device class. It will enable
> application to iterate over the vDPA devices.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vdpa.c | 116 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 98 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
> index b2b2a105f1..61ab9aadb4 100644
> --- a/lib/librte_vhost/vdpa.c
> +++ b/lib/librte_vhost/vdpa.c
> @@ -10,11 +10,12 @@
>  
>  #include <stdbool.h>
>  
> +#include <rte_class.h>
>  #include <rte_malloc.h>
>  #include "rte_vdpa.h"
>  #include "vhost.h"
>  
> -static struct rte_vdpa_device *vdpa_devices[MAX_VHOST_DEVICE];
> +static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE];
>  static uint32_t vdpa_device_num;
>  
>  static bool
> @@ -46,35 +47,28 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
>  		struct rte_vdpa_dev_ops *ops)
>  {
>  	struct rte_vdpa_device *dev;
> -	char device_name[MAX_VDPA_NAME_LEN];
>  	int i;
>  
>  	if (vdpa_device_num >= MAX_VHOST_DEVICE || addr == NULL || ops == NULL)
>  		return -1;
>  
>  	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
> -		dev = vdpa_devices[i];
> -		if (dev && is_same_vdpa_device(&dev->addr, addr))
> +		dev = &vdpa_devices[i];
> +		if (dev->ops && is_same_vdpa_device(&dev->addr, addr))
>  			return -1;
>  	}
>  
>  	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
> -		if (vdpa_devices[i] == NULL)
> +		if (vdpa_devices[i].ops == NULL)
>  			break;
>  	}
>  
>  	if (i == MAX_VHOST_DEVICE)
>  		return -1;
>  
> -	snprintf(device_name, sizeof(device_name), "vdpa-dev-%d", i);
> -	dev = rte_zmalloc(device_name, sizeof(struct rte_vdpa_device),
> -			RTE_CACHE_LINE_SIZE);
> -	if (!dev)
> -		return -1;
> -
> +	dev = &vdpa_devices[i];
>  	memcpy(&dev->addr, addr, sizeof(struct rte_vdpa_dev_addr));
>  	dev->ops = ops;
> -	vdpa_devices[i] = dev;
>  	vdpa_device_num++;
>  
>  	return i;
> @@ -83,11 +77,10 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
>  int
>  rte_vdpa_unregister_device(int did)
>  {
> -	if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did] == NULL)
> +	if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did].ops == NULL)
>  		return -1;
>  
> -	rte_free(vdpa_devices[did]);
> -	vdpa_devices[did] = NULL;
> +	memset(&vdpa_devices[did], 0, sizeof(struct rte_vdpa_device));
>  	vdpa_device_num--;
>  
>  	return did;
> @@ -103,8 +96,11 @@ rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr)
>  		return -1;
>  
>  	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
> -		dev = vdpa_devices[i];
> -		if (dev && is_same_vdpa_device(&dev->addr, addr))
> +		dev = &vdpa_devices[i];
> +		if (dev->ops == NULL)
> +			continue;
> +
> +		if (is_same_vdpa_device(&dev->addr, addr))
>  			return i;
>  	}
>  
> @@ -117,7 +113,7 @@ rte_vdpa_get_device(int did)
>  	if (did < 0 || did >= MAX_VHOST_DEVICE)
>  		return NULL;
>  
> -	return vdpa_devices[did];
> +	return &vdpa_devices[did];
>  }
>  
>  int
> @@ -227,3 +223,87 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m)
>  		free_ind_table(idesc);
>  	return -1;
>  }
> +
> +static uint16_t
> +vdpa_dev_to_id(const struct rte_vdpa_device *dev)
> +{
> +	if (dev == NULL)
> +		return MAX_VHOST_DEVICE;

I know it comes from ethdev, this helper was introduced by 214ed1acd12
and changed the comparison type from ptrdiff_t to uin16_t.

This is incorrect, ideally it should instead be a ptrdiff_t, then
if within acceptable range it should be casted to uin16_t to be used as
an index.

> +	return dev - vdpa_devices;
> +}
> +
> +static int
> +vdpa_dev_match(struct rte_vdpa_device *dev,
> +	      const struct rte_device *rte_dev)
> +{
> +	struct rte_vdpa_dev_addr addr;
> +
> +	/*  Only PCI bus supported for now */
> +	if (strcmp(rte_dev->bus->name, "pci") != 0)
> +		return -1;
> +
> +	addr.type = VDPA_ADDR_PCI;
> +
> +	if (rte_pci_addr_parse(rte_dev->name, &addr.pci_addr) != 0)
> +		return -1;
> +
> +	if (!is_same_vdpa_device(&dev->addr, &addr))
> +		return -1;
> +
> +	return 0;
> +}
> +
> +/* Generic rte_vdpa_dev comparison function. */
> +typedef int (*rte_vdpa_cmp_t)(struct rte_vdpa_device *,
> +		const struct rte_device *rte_dev);
> +
> +static struct rte_vdpa_device *
> +vdpa_find_device(const struct rte_vdpa_device *start, rte_vdpa_cmp_t cmp,
> +		struct rte_device *rte_dev)
> +{
> +	struct rte_vdpa_device *dev;
> +	ptrdiff_t idx;

The ptrdiff_t was used in ethdev due to the original

   start - &rte_eth_devices[0] + 1;

The diff was removed by commit 214ed1acd12 . 'idx' should be
the proper type to hold a device id, here probably uint16_t.

Ethdev class should be cleaned up as well.

> +
> +	/* Avoid Undefined Behaviour */
> +	if (start != NULL &&
> +	    (start < &vdpa_devices[0] ||
> +	     start >= &vdpa_devices[MAX_VHOST_DEVICE]))
> +		return NULL;

Same as before, this guard made sense mostly for the diff following.
It is coupled to the "random ptr to static array" comparison happening,
it would be better in vpda_dev_to_id() I think.

> +
> +	if (start != NULL)
> +		idx = vdpa_dev_to_id(start) + 1;
> +	else
> +		idx = 0;
> +	for (; idx < MAX_VHOST_DEVICE; idx++) {
> +		dev = &vdpa_devices[idx];
> +		/*
> +		 * ToDo: Certainly better to introduce a state field,
> +		 * but rely on ops being set for now.
> +		 */
> +		if (dev->ops == NULL)
> +			continue;
> +		if (cmp(dev, rte_dev) == 0)
> +			return dev;
> +	}
> +	return NULL;
> +}
> +
> +static void *
> +vdpa_dev_iterate(const void *start,
> +		const char *str,
> +		const struct rte_dev_iterator *it)
> +{
> +	struct rte_vdpa_device *vdpa_dev = NULL;
> +
> +	RTE_SET_USED(str);
> +
> +	vdpa_dev = vdpa_find_device(start, vdpa_dev_match, it->device);
> +
> +	return vdpa_dev;
> +}
> +
> +static struct rte_class rte_class_vdpa = {
> +	.dev_iterate = vdpa_dev_iterate,
> +};
> +
> +RTE_REGISTER_CLASS(vdpa, rte_class_vdpa);
> -- 
> 2.26.2
> 

-- 
Gaëtan

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH 04/14] vhost: make vDPA framework bus agnostic
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 04/14] vhost: make vDPA framework bus agnostic Maxime Coquelin
@ 2020-06-12 12:46   ` Gaëtan Rivet
  2020-06-15  9:17     ` Maxime Coquelin
  2020-06-19 11:14   ` Adrian Moreno
  1 sibling, 1 reply; 25+ messages in thread
From: Gaëtan Rivet @ 2020-06-12 12:46 UTC (permalink / raw)
  To: Maxime Coquelin
  Cc: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena

On 11/06/20 23:37 +0200, Maxime Coquelin wrote:
> This patch makes the vDPA framework to no more
> support only PCI devices, but any devices by relying
> on the generic device name as identifier.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/vdpa/ifc/ifcvf_vdpa.c          |  6 +-
>  drivers/vdpa/mlx5/mlx5_vdpa.c          |  8 +--
>  drivers/vdpa/mlx5/mlx5_vdpa.h          |  2 +-
>  examples/vdpa/main.c                   | 49 ++++++++--------
>  lib/librte_vhost/rte_vdpa.h            | 42 +++++++-------
>  lib/librte_vhost/rte_vhost_version.map |  1 +
>  lib/librte_vhost/vdpa.c                | 79 +++++++++++---------------
>  7 files changed, 85 insertions(+), 102 deletions(-)
> 

[...]

> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c b/drivers/vdpa/mlx5/mlx5_vdpa.c
> index 1113d6cef0..e8255c7d7e 100644
> --- a/drivers/vdpa/mlx5/mlx5_vdpa.c
> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
> @@ -501,14 +501,13 @@ mlx5_vdpa_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>  	priv->caps = attr.vdpa;
>  	priv->log_max_rqt_size = attr.log_max_rqt_size;
>  	priv->ctx = ctx;
> -	priv->dev_addr.pci_addr = pci_dev->addr;
> -	priv->dev_addr.type = VDPA_ADDR_PCI;
> +	priv->pci_dev = pci_dev;
>  	priv->var = mlx5_glue->dv_alloc_var(ctx, 0);
>  	if (!priv->var) {
>  		DRV_LOG(ERR, "Failed to allocate VAR %u.\n", errno);
>  		goto error;
>  	}
> -	priv->id = rte_vdpa_register_device(&priv->dev_addr, &mlx5_vdpa_ops);
> +	priv->id = rte_vdpa_register_device(&pci_dev->device, &mlx5_vdpa_ops);
>  	if (priv->id < 0) {
>  		DRV_LOG(ERR, "Failed to register vDPA device.");
>  		rte_errno = rte_errno ? rte_errno : EINVAL;
> @@ -550,8 +549,7 @@ mlx5_vdpa_pci_remove(struct rte_pci_device *pci_dev)
>  
>  	pthread_mutex_lock(&priv_list_lock);
>  	TAILQ_FOREACH(priv, &priv_list, next) {
> -		if (memcmp(&priv->dev_addr.pci_addr, &pci_dev->addr,
> -			   sizeof(pci_dev->addr)) == 0) {
> +		if (priv->pci_dev == pci_dev) {

Assignment is ok I think but comparison is not, rte_pci_addr_cmp()
should be used here instead. (memcmp was not correct either.)

The structure can be padded depending on arch.

Everything else seems ok to me.

-- 
Gaëtan

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH 03/14] vhost: introduce vDPA devices class
  2020-06-12 12:37   ` Gaëtan Rivet
@ 2020-06-15  9:15     ` Maxime Coquelin
  0 siblings, 0 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-15  9:15 UTC (permalink / raw)
  To: Gaëtan Rivet
  Cc: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena



On 6/12/20 2:37 PM, Gaëtan Rivet wrote:
> On 11/06/20 23:37 +0200, Maxime Coquelin wrote:
>> This patch introduces vDPA device class. It will enable
>> application to iterate over the vDPA devices.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  lib/librte_vhost/vdpa.c | 116 +++++++++++++++++++++++++++++++++-------
>>  1 file changed, 98 insertions(+), 18 deletions(-)
>>
>> diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
>> index b2b2a105f1..61ab9aadb4 100644
>> --- a/lib/librte_vhost/vdpa.c
>> +++ b/lib/librte_vhost/vdpa.c
>> @@ -10,11 +10,12 @@
>>  
>>  #include <stdbool.h>
>>  
>> +#include <rte_class.h>
>>  #include <rte_malloc.h>
>>  #include "rte_vdpa.h"
>>  #include "vhost.h"
>>  
>> -static struct rte_vdpa_device *vdpa_devices[MAX_VHOST_DEVICE];
>> +static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE];
>>  static uint32_t vdpa_device_num;
>>  
>>  static bool
>> @@ -46,35 +47,28 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
>>  		struct rte_vdpa_dev_ops *ops)
>>  {
>>  	struct rte_vdpa_device *dev;
>> -	char device_name[MAX_VDPA_NAME_LEN];
>>  	int i;
>>  
>>  	if (vdpa_device_num >= MAX_VHOST_DEVICE || addr == NULL || ops == NULL)
>>  		return -1;
>>  
>>  	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
>> -		dev = vdpa_devices[i];
>> -		if (dev && is_same_vdpa_device(&dev->addr, addr))
>> +		dev = &vdpa_devices[i];
>> +		if (dev->ops && is_same_vdpa_device(&dev->addr, addr))
>>  			return -1;
>>  	}
>>  
>>  	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
>> -		if (vdpa_devices[i] == NULL)
>> +		if (vdpa_devices[i].ops == NULL)
>>  			break;
>>  	}
>>  
>>  	if (i == MAX_VHOST_DEVICE)
>>  		return -1;
>>  
>> -	snprintf(device_name, sizeof(device_name), "vdpa-dev-%d", i);
>> -	dev = rte_zmalloc(device_name, sizeof(struct rte_vdpa_device),
>> -			RTE_CACHE_LINE_SIZE);
>> -	if (!dev)
>> -		return -1;
>> -
>> +	dev = &vdpa_devices[i];
>>  	memcpy(&dev->addr, addr, sizeof(struct rte_vdpa_dev_addr));
>>  	dev->ops = ops;
>> -	vdpa_devices[i] = dev;
>>  	vdpa_device_num++;
>>  
>>  	return i;
>> @@ -83,11 +77,10 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
>>  int
>>  rte_vdpa_unregister_device(int did)
>>  {
>> -	if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did] == NULL)
>> +	if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did].ops == NULL)
>>  		return -1;
>>  
>> -	rte_free(vdpa_devices[did]);
>> -	vdpa_devices[did] = NULL;
>> +	memset(&vdpa_devices[did], 0, sizeof(struct rte_vdpa_device));
>>  	vdpa_device_num--;
>>  
>>  	return did;
>> @@ -103,8 +96,11 @@ rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr)
>>  		return -1;
>>  
>>  	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
>> -		dev = vdpa_devices[i];
>> -		if (dev && is_same_vdpa_device(&dev->addr, addr))
>> +		dev = &vdpa_devices[i];
>> +		if (dev->ops == NULL)
>> +			continue;
>> +
>> +		if (is_same_vdpa_device(&dev->addr, addr))
>>  			return i;
>>  	}
>>  
>> @@ -117,7 +113,7 @@ rte_vdpa_get_device(int did)
>>  	if (did < 0 || did >= MAX_VHOST_DEVICE)
>>  		return NULL;
>>  
>> -	return vdpa_devices[did];
>> +	return &vdpa_devices[did];
>>  }
>>  
>>  int
>> @@ -227,3 +223,87 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m)
>>  		free_ind_table(idesc);
>>  	return -1;
>>  }
>> +
>> +static uint16_t
>> +vdpa_dev_to_id(const struct rte_vdpa_device *dev)
>> +{
>> +	if (dev == NULL)
>> +		return MAX_VHOST_DEVICE;
> 
> I know it comes from ethdev, this helper was introduced by 214ed1acd12
> and changed the comparison type from ptrdiff_t to uin16_t.
> 
> This is incorrect, ideally it should instead be a ptrdiff_t, then
> if within acceptable range it should be casted to uin16_t to be used as
> an index.
> 
>> +	return dev - vdpa_devices;
>> +}
>> +
>> +static int
>> +vdpa_dev_match(struct rte_vdpa_device *dev,
>> +	      const struct rte_device *rte_dev)
>> +{
>> +	struct rte_vdpa_dev_addr addr;
>> +
>> +	/*  Only PCI bus supported for now */
>> +	if (strcmp(rte_dev->bus->name, "pci") != 0)
>> +		return -1;
>> +
>> +	addr.type = VDPA_ADDR_PCI;
>> +
>> +	if (rte_pci_addr_parse(rte_dev->name, &addr.pci_addr) != 0)
>> +		return -1;
>> +
>> +	if (!is_same_vdpa_device(&dev->addr, &addr))
>> +		return -1;
>> +
>> +	return 0;
>> +}
>> +
>> +/* Generic rte_vdpa_dev comparison function. */
>> +typedef int (*rte_vdpa_cmp_t)(struct rte_vdpa_device *,
>> +		const struct rte_device *rte_dev);
>> +
>> +static struct rte_vdpa_device *
>> +vdpa_find_device(const struct rte_vdpa_device *start, rte_vdpa_cmp_t cmp,
>> +		struct rte_device *rte_dev)
>> +{
>> +	struct rte_vdpa_device *dev;
>> +	ptrdiff_t idx;
> 
> The ptrdiff_t was used in ethdev due to the original
> 
>    start - &rte_eth_devices[0] + 1;
> 
> The diff was removed by commit 214ed1acd12 . 'idx' should be
> the proper type to hold a device id, here probably uint16_t.

Ok, makes sense.

> Ethdev class should be cleaned up as well.
> 
>> +
>> +	/* Avoid Undefined Behaviour */
>> +	if (start != NULL &&
>> +	    (start < &vdpa_devices[0] ||
>> +	     start >= &vdpa_devices[MAX_VHOST_DEVICE]))
>> +		return NULL;
> 
> Same as before, this guard made sense mostly for the diff following.
> It is coupled to the "random ptr to static array" comparison happening,
> it would be better in vpda_dev_to_id() I think.

OK, I will clean that in next version, but but this part moves away
later in the series, when switching to linked-list.

Thanks,
Maxime

>> +
>> +	if (start != NULL)
>> +		idx = vdpa_dev_to_id(start) + 1;
>> +	else
>> +		idx = 0;
>> +	for (; idx < MAX_VHOST_DEVICE; idx++) {
>> +		dev = &vdpa_devices[idx];
>> +		/*
>> +		 * ToDo: Certainly better to introduce a state field,
>> +		 * but rely on ops being set for now.
>> +		 */
>> +		if (dev->ops == NULL)
>> +			continue;
>> +		if (cmp(dev, rte_dev) == 0)
>> +			return dev;
>> +	}
>> +	return NULL;
>> +}
>> +
>> +static void *
>> +vdpa_dev_iterate(const void *start,
>> +		const char *str,
>> +		const struct rte_dev_iterator *it)
>> +{
>> +	struct rte_vdpa_device *vdpa_dev = NULL;
>> +
>> +	RTE_SET_USED(str);
>> +
>> +	vdpa_dev = vdpa_find_device(start, vdpa_dev_match, it->device);
>> +
>> +	return vdpa_dev;
>> +}
>> +
>> +static struct rte_class rte_class_vdpa = {
>> +	.dev_iterate = vdpa_dev_iterate,
>> +};
>> +
>> +RTE_REGISTER_CLASS(vdpa, rte_class_vdpa);
>> -- 
>> 2.26.2
>>
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH 04/14] vhost: make vDPA framework bus agnostic
  2020-06-12 12:46   ` Gaëtan Rivet
@ 2020-06-15  9:17     ` Maxime Coquelin
  0 siblings, 0 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-15  9:17 UTC (permalink / raw)
  To: Gaëtan Rivet
  Cc: dev, matan, xiao.w.wang, zhihong.wang, xiaolong.ye, chenbo.xia,
	david.marchand, amorenoz, shreyansh.jain, viacheslavo,
	hemant.agrawal, sachin.saxena



On 6/12/20 2:46 PM, Gaëtan Rivet wrote:
> On 11/06/20 23:37 +0200, Maxime Coquelin wrote:
>> This patch makes the vDPA framework to no more
>> support only PCI devices, but any devices by relying
>> on the generic device name as identifier.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  drivers/vdpa/ifc/ifcvf_vdpa.c          |  6 +-
>>  drivers/vdpa/mlx5/mlx5_vdpa.c          |  8 +--
>>  drivers/vdpa/mlx5/mlx5_vdpa.h          |  2 +-
>>  examples/vdpa/main.c                   | 49 ++++++++--------
>>  lib/librte_vhost/rte_vdpa.h            | 42 +++++++-------
>>  lib/librte_vhost/rte_vhost_version.map |  1 +
>>  lib/librte_vhost/vdpa.c                | 79 +++++++++++---------------
>>  7 files changed, 85 insertions(+), 102 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c b/drivers/vdpa/mlx5/mlx5_vdpa.c
>> index 1113d6cef0..e8255c7d7e 100644
>> --- a/drivers/vdpa/mlx5/mlx5_vdpa.c
>> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
>> @@ -501,14 +501,13 @@ mlx5_vdpa_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>>  	priv->caps = attr.vdpa;
>>  	priv->log_max_rqt_size = attr.log_max_rqt_size;
>>  	priv->ctx = ctx;
>> -	priv->dev_addr.pci_addr = pci_dev->addr;
>> -	priv->dev_addr.type = VDPA_ADDR_PCI;
>> +	priv->pci_dev = pci_dev;
>>  	priv->var = mlx5_glue->dv_alloc_var(ctx, 0);
>>  	if (!priv->var) {
>>  		DRV_LOG(ERR, "Failed to allocate VAR %u.\n", errno);
>>  		goto error;
>>  	}
>> -	priv->id = rte_vdpa_register_device(&priv->dev_addr, &mlx5_vdpa_ops);
>> +	priv->id = rte_vdpa_register_device(&pci_dev->device, &mlx5_vdpa_ops);
>>  	if (priv->id < 0) {
>>  		DRV_LOG(ERR, "Failed to register vDPA device.");
>>  		rte_errno = rte_errno ? rte_errno : EINVAL;
>> @@ -550,8 +549,7 @@ mlx5_vdpa_pci_remove(struct rte_pci_device *pci_dev)
>>  
>>  	pthread_mutex_lock(&priv_list_lock);
>>  	TAILQ_FOREACH(priv, &priv_list, next) {
>> -		if (memcmp(&priv->dev_addr.pci_addr, &pci_dev->addr,
>> -			   sizeof(pci_dev->addr)) == 0) {
>> +		if (priv->pci_dev == pci_dev) {
> 
> Assignment is ok I think but comparison is not, rte_pci_addr_cmp()
> should be used here instead. (memcmp was not correct either.)
> 
> The structure can be padded depending on arch.
> 
> Everything else seems ok to me.
> 

Ok, I will change to use rte_pci_addr_cmp() in next revision.

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH 04/14] vhost: make vDPA framework bus agnostic
  2020-06-11 21:37 ` [dpdk-dev] [PATCH 04/14] vhost: make vDPA framework bus agnostic Maxime Coquelin
  2020-06-12 12:46   ` Gaëtan Rivet
@ 2020-06-19 11:14   ` Adrian Moreno
  2020-06-19 15:11     ` Maxime Coquelin
  1 sibling, 1 reply; 25+ messages in thread
From: Adrian Moreno @ 2020-06-19 11:14 UTC (permalink / raw)
  To: Maxime Coquelin, dev, matan, xiao.w.wang, zhihong.wang,
	xiaolong.ye, chenbo.xia, david.marchand, shreyansh.jain,
	viacheslavo, hemant.agrawal, sachin.saxena



On 6/11/20 11:37 PM, Maxime Coquelin wrote:
> This patch makes the vDPA framework to no more
> support only PCI devices, but any devices by relying
> on the generic device name as identifier.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/vdpa/ifc/ifcvf_vdpa.c          |  6 +-
>  drivers/vdpa/mlx5/mlx5_vdpa.c          |  8 +--
>  drivers/vdpa/mlx5/mlx5_vdpa.h          |  2 +-
>  examples/vdpa/main.c                   | 49 ++++++++--------
>  lib/librte_vhost/rte_vdpa.h            | 42 +++++++-------
>  lib/librte_vhost/rte_vhost_version.map |  1 +
>  lib/librte_vhost/vdpa.c                | 79 +++++++++++---------------
>  7 files changed, 85 insertions(+), 102 deletions(-)
> 
> diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c b/drivers/vdpa/ifc/ifcvf_vdpa.c
> index ec97178dcb..1fec1f1baf 100644
> --- a/drivers/vdpa/ifc/ifcvf_vdpa.c
> +++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
> @@ -47,7 +47,6 @@ static const char * const ifcvf_valid_arguments[] = {
>  static int ifcvf_vdpa_logtype;
>  
>  struct ifcvf_internal {
> -	struct rte_vdpa_dev_addr dev_addr;
>  	struct rte_pci_device *pdev;
>  	struct ifcvf_hw hw;
>  	int vfio_container_fd;
> @@ -1176,8 +1175,6 @@ ifcvf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>  		(1ULL << VHOST_USER_F_PROTOCOL_FEATURES) |
>  		(1ULL << VHOST_F_LOG_ALL);
>  
> -	internal->dev_addr.pci_addr = pci_dev->addr;
> -	internal->dev_addr.type = VDPA_ADDR_PCI;
>  	list->internal = internal;
>  
>  	if (rte_kvargs_count(kvlist, IFCVF_SW_FALLBACK_LM)) {
> @@ -1188,8 +1185,7 @@ ifcvf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>  	}
>  	internal->sw_lm = sw_fallback_lm;
>  
> -	internal->did = rte_vdpa_register_device(&internal->dev_addr,
> -				&ifcvf_ops);
> +	internal->did = rte_vdpa_register_device(&pci_dev->device, &ifcvf_ops);
>  	if (internal->did < 0) {
>  		DRV_LOG(ERR, "failed to register device %s", pci_dev->name);
>  		goto error;
> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c b/drivers/vdpa/mlx5/mlx5_vdpa.c
> index 1113d6cef0..e8255c7d7e 100644
> --- a/drivers/vdpa/mlx5/mlx5_vdpa.c
> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
> @@ -501,14 +501,13 @@ mlx5_vdpa_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>  	priv->caps = attr.vdpa;
>  	priv->log_max_rqt_size = attr.log_max_rqt_size;
>  	priv->ctx = ctx;
> -	priv->dev_addr.pci_addr = pci_dev->addr;
> -	priv->dev_addr.type = VDPA_ADDR_PCI;
> +	priv->pci_dev = pci_dev;
>  	priv->var = mlx5_glue->dv_alloc_var(ctx, 0);
>  	if (!priv->var) {
>  		DRV_LOG(ERR, "Failed to allocate VAR %u.\n", errno);
>  		goto error;
>  	}
> -	priv->id = rte_vdpa_register_device(&priv->dev_addr, &mlx5_vdpa_ops);
> +	priv->id = rte_vdpa_register_device(&pci_dev->device, &mlx5_vdpa_ops);
>  	if (priv->id < 0) {
>  		DRV_LOG(ERR, "Failed to register vDPA device.");
>  		rte_errno = rte_errno ? rte_errno : EINVAL;
> @@ -550,8 +549,7 @@ mlx5_vdpa_pci_remove(struct rte_pci_device *pci_dev)
>  
>  	pthread_mutex_lock(&priv_list_lock);
>  	TAILQ_FOREACH(priv, &priv_list, next) {
> -		if (memcmp(&priv->dev_addr.pci_addr, &pci_dev->addr,
> -			   sizeof(pci_dev->addr)) == 0) {
> +		if (priv->pci_dev == pci_dev) {
>  			found = 1;
>  			break;
>  		}
> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.h b/drivers/vdpa/mlx5/mlx5_vdpa.h
> index fcc216ac78..50ee3c5870 100644
> --- a/drivers/vdpa/mlx5/mlx5_vdpa.h
> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.h
> @@ -104,7 +104,7 @@ struct mlx5_vdpa_priv {
>  	int id; /* vDPA device id. */
>  	int vid; /* vhost device id. */
>  	struct ibv_context *ctx; /* Device context. */
> -	struct rte_vdpa_dev_addr dev_addr;
> +	struct rte_pci_device *pci_dev;
>  	struct mlx5_hca_vdpa_attr caps;
>  	uint32_t pdn; /* Protection Domain number. */
>  	struct ibv_pd *pd;
> diff --git a/examples/vdpa/main.c b/examples/vdpa/main.c
> index d9a9112b16..c12da69574 100644
> --- a/examples/vdpa/main.c
> +++ b/examples/vdpa/main.c
> @@ -271,10 +271,14 @@ static void cmd_list_vdpa_devices_parsed(
>  	uint32_t queue_num;
>  	uint64_t features;
>  	struct rte_vdpa_device *vdev;
> -	struct rte_pci_addr addr;
> +	struct rte_device *dev;
> +	struct rte_dev_iterator dev_iter;
>  
> -	cmdline_printf(cl, "device id\tdevice address\tqueue num\tsupported features\n");
> -	for (did = 0; did < dev_total; did++) {
> +	cmdline_printf(cl, "device id\tdevice name\tqueue num\tsupported features\n");
> +	RTE_DEV_FOREACH(dev, "class=vdpa", &dev_iter) {
> +		did = rte_vdpa_find_device_id_by_name(dev->name);
> +		if (did < 0)
> +			continue;
>  		vdev = rte_vdpa_get_device(did);
>  		if (!vdev)
>  			continue;
> @@ -290,11 +294,8 @@ static void cmd_list_vdpa_devices_parsed(
>  				"for device id %d.\n", did);
>  			continue;
>  		}
> -		addr = vdev->addr.pci_addr;
> -		cmdline_printf(cl,
> -			"%d\t\t" PCI_PRI_FMT "\t%" PRIu32 "\t\t0x%" PRIx64 "\n",
> -			did, addr.domain, addr.bus, addr.devid,
> -			addr.function, queue_num, features);
> +		cmdline_printf(cl, "%d\t\t%s\t\t%" PRIu32 "\t\t0x%" PRIx64 "\n",
> +			did, dev->name, queue_num, features);
>  	}
>  }
>  
> @@ -324,17 +325,12 @@ static void cmd_create_vdpa_port_parsed(void *parsed_result,
>  {
>  	int did;
>  	struct cmd_create_result *res = parsed_result;
> -	struct rte_vdpa_dev_addr addr;
>  
>  	rte_strscpy(vports[devcnt].ifname, res->socket_path, MAX_PATH_LEN);
> -	if (rte_pci_addr_parse(res->bdf, &addr.pci_addr) != 0) {
> -		cmdline_printf(cl, "Unable to parse the given bdf.\n");
> -		return;
> -	}
> -	addr.type = VDPA_ADDR_PCI;
> -	did = rte_vdpa_find_device_id(&addr);
> +	did = rte_vdpa_find_device_id_by_name(res->bdf);
>  	if (did < 0) {
> -		cmdline_printf(cl, "Unable to find vdpa device id.\n");
> +		cmdline_printf(cl, "Unable to find vdpa device id for %s.\n",
> +				res->bdf);
>  		return;
>  	}
>  
> @@ -400,9 +396,11 @@ int
>  main(int argc, char *argv[])
>  {
>  	char ch;
> -	int i;
> +	int did;
>  	int ret;
>  	struct cmdline *cl;
> +	struct rte_device *dev;
> +	struct rte_dev_iterator dev_iter;
>  
>  	ret = rte_eal_init(argc, argv);
>  	if (ret < 0)
> @@ -428,13 +426,18 @@ main(int argc, char *argv[])
>  		cmdline_interact(cl);
>  		cmdline_stdin_exit(cl);
>  	} else {
> -		for (i = 0; i < RTE_MIN(MAX_VDPA_SAMPLE_PORTS, dev_total);
> -				i++) {
> -			vports[i].did = i;
> -			snprintf(vports[i].ifname, MAX_PATH_LEN, "%s%d",
> -					iface, i);
> +		RTE_DEV_FOREACH(dev, "class=vdpa", &dev_iter) {
> +			did = rte_vdpa_find_device_id_by_name(dev->name);
> +			if (did < 0) {
> +				rte_panic("Failed to find device id for %s\n",
> +						dev->name);
> +			}
> +			vports[devcnt].did = did;
> +			snprintf(vports[devcnt].ifname, MAX_PATH_LEN, "%s%d",
> +					iface, devcnt);
>  
> -			start_vdpa(&vports[i]);
> +			start_vdpa(&vports[devcnt]);
> +			devcnt++;
>  		}
>  
>  		printf("enter \'q\' to quit\n");
> diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
> index 3c400ee79b..33037d39ea 100644
> --- a/lib/librte_vhost/rte_vdpa.h
> +++ b/lib/librte_vhost/rte_vdpa.h
> @@ -18,25 +18,6 @@
>  
>  #define MAX_VDPA_NAME_LEN 128
>  
> -enum vdpa_addr_type {
> -	VDPA_ADDR_PCI,
> -	VDPA_ADDR_MAX
> -};
> -
> -/**
> - * vdpa device address
> - */
> -struct rte_vdpa_dev_addr {
> -	/** vdpa address type */
> -	enum vdpa_addr_type type;
> -
> -	/** vdpa pci address */
> -	union {
> -		uint8_t __dummy[64];
> -		struct rte_pci_addr pci_addr;
> -	};
> -};
> -
>  /**
>   * vdpa device operations
>   */
> @@ -81,8 +62,8 @@ struct rte_vdpa_dev_ops {
>   * vdpa device structure includes device address and device operations.
>   */
>  struct rte_vdpa_device {
> -	/** vdpa device address */
> -	struct rte_vdpa_dev_addr addr;
> +	/** Generic device information */
> +	struct rte_device *device;
>  	/** vdpa device operations */
>  	struct rte_vdpa_dev_ops *ops;
>  } __rte_cache_aligned;
> @@ -102,7 +83,7 @@ struct rte_vdpa_device {
>   */
>  __rte_experimental
>  int
> -rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
> +rte_vdpa_register_device(struct rte_device *rte_dev,
>  		struct rte_vdpa_dev_ops *ops);
>  
>  /**
> @@ -120,6 +101,21 @@ __rte_experimental
>  int
>  rte_vdpa_unregister_device(int did);
>  
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Find the device id of a vdpa device from its name
> + *
> + * @param name
> + *  the vdpa device name
> + * @return
> + *  device id on success, -1 on failure
> + */
> +__rte_experimental
> +int
> +rte_vdpa_find_device_id_by_name(const char *name);
> +
>  /**
>   * @warning
>   * @b EXPERIMENTAL: this API may change without prior notice
> @@ -133,7 +129,7 @@ rte_vdpa_unregister_device(int did);
>   */
>  __rte_experimental
>  int
> -rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr);
> +rte_vdpa_find_device_id(struct rte_vdpa_device *dev);
>  
>  /**
>   * @warning
> diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
> index 051d08c120..1abfff8a0c 100644
> --- a/lib/librte_vhost/rte_vhost_version.map
> +++ b/lib/librte_vhost/rte_vhost_version.map
> @@ -66,4 +66,5 @@ EXPERIMENTAL {
>  	rte_vhost_get_vhost_ring_inflight;
>  	rte_vhost_get_vring_base_from_inflight;
>  	rte_vhost_slave_config_change;
> +	rte_vdpa_find_device_id_by_name;
>  };
> diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
> index 61ab9aadb4..5abc5a2a7c 100644
> --- a/lib/librte_vhost/vdpa.c
> +++ b/lib/librte_vhost/vdpa.c
> @@ -18,43 +18,22 @@
>  static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE];
>  static uint32_t vdpa_device_num;
>  
> -static bool
> -is_same_vdpa_device(struct rte_vdpa_dev_addr *a,
> -		struct rte_vdpa_dev_addr *b)
> -{
> -	bool ret = true;
> -
> -	if (a->type != b->type)
> -		return false;
> -
> -	switch (a->type) {
> -	case VDPA_ADDR_PCI:
> -		if (a->pci_addr.domain != b->pci_addr.domain ||
> -				a->pci_addr.bus != b->pci_addr.bus ||
> -				a->pci_addr.devid != b->pci_addr.devid ||
> -				a->pci_addr.function != b->pci_addr.function)
> -			ret = false;
> -		break;
> -	default:
> -		break;
> -	}
> -
> -	return ret;
> -}
> -
>  int
> -rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
> +rte_vdpa_register_device(struct rte_device *rte_dev,
>  		struct rte_vdpa_dev_ops *ops)
>  {
>  	struct rte_vdpa_device *dev;
>  	int i;
>  
> -	if (vdpa_device_num >= MAX_VHOST_DEVICE || addr == NULL || ops == NULL)
> +	if (vdpa_device_num >= MAX_VHOST_DEVICE || ops == NULL)
>  		return -1;
>  
>  	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
>  		dev = &vdpa_devices[i];
> -		if (dev->ops && is_same_vdpa_device(&dev->addr, addr))
> +		if (dev->ops == NULL)
> +			continue;
> +
> +		if (dev->device == rte_dev)
>  			return -1;
>  	}

If we change the order of the two "if" statemets and replace "continue" with
"break", we can remove the for loop that follows:

	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
		if (vdpa_devices[i].ops == NULL)
			break;
	}


>  
> @@ -67,7 +46,7 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
>  		return -1;
>  
>  	dev = &vdpa_devices[i];
> -	memcpy(&dev->addr, addr, sizeof(struct rte_vdpa_dev_addr));
> +	dev->device = rte_dev;
>  	dev->ops = ops;
>  	vdpa_device_num++;
>  
> @@ -87,12 +66,33 @@ rte_vdpa_unregister_device(int did)
>  }
>  
>  int
> -rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr)
> +rte_vdpa_find_device_id(struct rte_vdpa_device *dev)
> +{
> +	struct rte_vdpa_device *tmp_dev;
> +	int i;
> +
> +	if (dev == NULL)
> +		return -1;
> +
> +	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
> +		tmp_dev = &vdpa_devices[i];
> +		if (tmp_dev->ops == NULL)
> +			continue;
> +
> +		if (tmp_dev == dev)
> +			return i;
> +	}
> +
> +	return -1;
> +}
> +
> +int
> +rte_vdpa_find_device_id_by_name(const char *name)
>  {
>  	struct rte_vdpa_device *dev;
>  	int i;
>  
> -	if (addr == NULL)
> +	if (name == NULL)
>  		return -1;
>  
>  	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
> @@ -100,7 +100,7 @@ rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr)
>  		if (dev->ops == NULL)
>  			continue;
>  
> -		if (is_same_vdpa_device(&dev->addr, addr))
> +		if (strcmp(dev->device->name, name) == 0)
>  			return i;
>  	}
>  
> @@ -236,21 +236,10 @@ static int
>  vdpa_dev_match(struct rte_vdpa_device *dev,
>  	      const struct rte_device *rte_dev)
>  {
> -	struct rte_vdpa_dev_addr addr;
> +	if (dev->device == rte_dev)
> +		return 0;
>  
> -	/*  Only PCI bus supported for now */
> -	if (strcmp(rte_dev->bus->name, "pci") != 0)
> -		return -1;
> -
> -	addr.type = VDPA_ADDR_PCI;
> -
> -	if (rte_pci_addr_parse(rte_dev->name, &addr.pci_addr) != 0)
> -		return -1;
> -
> -	if (!is_same_vdpa_device(&dev->addr, &addr))
> -		return -1;
> -
> -	return 0;
> +	return -1;
>  }
>  
>  /* Generic rte_vdpa_dev comparison function. */
> 

-- 
Adrián Moreno


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH 04/14] vhost: make vDPA framework bus agnostic
  2020-06-19 11:14   ` Adrian Moreno
@ 2020-06-19 15:11     ` Maxime Coquelin
  2020-06-22  9:49       ` Adrian Moreno
  0 siblings, 1 reply; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-19 15:11 UTC (permalink / raw)
  To: Adrian Moreno, dev, matan, xiao.w.wang, zhihong.wang,
	xiaolong.ye, chenbo.xia, david.marchand, shreyansh.jain,
	viacheslavo, hemant.agrawal, sachin.saxena



On 6/19/20 1:14 PM, Adrian Moreno wrote:
> 
> 
> On 6/11/20 11:37 PM, Maxime Coquelin wrote:
>> This patch makes the vDPA framework to no more
>> support only PCI devices, but any devices by relying
>> on the generic device name as identifier.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  drivers/vdpa/ifc/ifcvf_vdpa.c          |  6 +-
>>  drivers/vdpa/mlx5/mlx5_vdpa.c          |  8 +--
>>  drivers/vdpa/mlx5/mlx5_vdpa.h          |  2 +-
>>  examples/vdpa/main.c                   | 49 ++++++++--------
>>  lib/librte_vhost/rte_vdpa.h            | 42 +++++++-------
>>  lib/librte_vhost/rte_vhost_version.map |  1 +
>>  lib/librte_vhost/vdpa.c                | 79 +++++++++++---------------
>>  7 files changed, 85 insertions(+), 102 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c b/drivers/vdpa/ifc/ifcvf_vdpa.c
>> index ec97178dcb..1fec1f1baf 100644
>> --- a/drivers/vdpa/ifc/ifcvf_vdpa.c
>> +++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
>> @@ -47,7 +47,6 @@ static const char * const ifcvf_valid_arguments[] = {
>>  static int ifcvf_vdpa_logtype;
>>  
>>  struct ifcvf_internal {
>> -	struct rte_vdpa_dev_addr dev_addr;
>>  	struct rte_pci_device *pdev;
>>  	struct ifcvf_hw hw;
>>  	int vfio_container_fd;
>> @@ -1176,8 +1175,6 @@ ifcvf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>>  		(1ULL << VHOST_USER_F_PROTOCOL_FEATURES) |
>>  		(1ULL << VHOST_F_LOG_ALL);
>>  
>> -	internal->dev_addr.pci_addr = pci_dev->addr;
>> -	internal->dev_addr.type = VDPA_ADDR_PCI;
>>  	list->internal = internal;
>>  
>>  	if (rte_kvargs_count(kvlist, IFCVF_SW_FALLBACK_LM)) {
>> @@ -1188,8 +1185,7 @@ ifcvf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>>  	}
>>  	internal->sw_lm = sw_fallback_lm;
>>  
>> -	internal->did = rte_vdpa_register_device(&internal->dev_addr,
>> -				&ifcvf_ops);
>> +	internal->did = rte_vdpa_register_device(&pci_dev->device, &ifcvf_ops);
>>  	if (internal->did < 0) {
>>  		DRV_LOG(ERR, "failed to register device %s", pci_dev->name);
>>  		goto error;
>> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c b/drivers/vdpa/mlx5/mlx5_vdpa.c
>> index 1113d6cef0..e8255c7d7e 100644
>> --- a/drivers/vdpa/mlx5/mlx5_vdpa.c
>> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
>> @@ -501,14 +501,13 @@ mlx5_vdpa_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>>  	priv->caps = attr.vdpa;
>>  	priv->log_max_rqt_size = attr.log_max_rqt_size;
>>  	priv->ctx = ctx;
>> -	priv->dev_addr.pci_addr = pci_dev->addr;
>> -	priv->dev_addr.type = VDPA_ADDR_PCI;
>> +	priv->pci_dev = pci_dev;
>>  	priv->var = mlx5_glue->dv_alloc_var(ctx, 0);
>>  	if (!priv->var) {
>>  		DRV_LOG(ERR, "Failed to allocate VAR %u.\n", errno);
>>  		goto error;
>>  	}
>> -	priv->id = rte_vdpa_register_device(&priv->dev_addr, &mlx5_vdpa_ops);
>> +	priv->id = rte_vdpa_register_device(&pci_dev->device, &mlx5_vdpa_ops);
>>  	if (priv->id < 0) {
>>  		DRV_LOG(ERR, "Failed to register vDPA device.");
>>  		rte_errno = rte_errno ? rte_errno : EINVAL;
>> @@ -550,8 +549,7 @@ mlx5_vdpa_pci_remove(struct rte_pci_device *pci_dev)
>>  
>>  	pthread_mutex_lock(&priv_list_lock);
>>  	TAILQ_FOREACH(priv, &priv_list, next) {
>> -		if (memcmp(&priv->dev_addr.pci_addr, &pci_dev->addr,
>> -			   sizeof(pci_dev->addr)) == 0) {
>> +		if (priv->pci_dev == pci_dev) {
>>  			found = 1;
>>  			break;
>>  		}
>> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.h b/drivers/vdpa/mlx5/mlx5_vdpa.h
>> index fcc216ac78..50ee3c5870 100644
>> --- a/drivers/vdpa/mlx5/mlx5_vdpa.h
>> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.h
>> @@ -104,7 +104,7 @@ struct mlx5_vdpa_priv {
>>  	int id; /* vDPA device id. */
>>  	int vid; /* vhost device id. */
>>  	struct ibv_context *ctx; /* Device context. */
>> -	struct rte_vdpa_dev_addr dev_addr;
>> +	struct rte_pci_device *pci_dev;
>>  	struct mlx5_hca_vdpa_attr caps;
>>  	uint32_t pdn; /* Protection Domain number. */
>>  	struct ibv_pd *pd;
>> diff --git a/examples/vdpa/main.c b/examples/vdpa/main.c
>> index d9a9112b16..c12da69574 100644
>> --- a/examples/vdpa/main.c
>> +++ b/examples/vdpa/main.c
>> @@ -271,10 +271,14 @@ static void cmd_list_vdpa_devices_parsed(
>>  	uint32_t queue_num;
>>  	uint64_t features;
>>  	struct rte_vdpa_device *vdev;
>> -	struct rte_pci_addr addr;
>> +	struct rte_device *dev;
>> +	struct rte_dev_iterator dev_iter;
>>  
>> -	cmdline_printf(cl, "device id\tdevice address\tqueue num\tsupported features\n");
>> -	for (did = 0; did < dev_total; did++) {
>> +	cmdline_printf(cl, "device id\tdevice name\tqueue num\tsupported features\n");
>> +	RTE_DEV_FOREACH(dev, "class=vdpa", &dev_iter) {
>> +		did = rte_vdpa_find_device_id_by_name(dev->name);
>> +		if (did < 0)
>> +			continue;
>>  		vdev = rte_vdpa_get_device(did);
>>  		if (!vdev)
>>  			continue;
>> @@ -290,11 +294,8 @@ static void cmd_list_vdpa_devices_parsed(
>>  				"for device id %d.\n", did);
>>  			continue;
>>  		}
>> -		addr = vdev->addr.pci_addr;
>> -		cmdline_printf(cl,
>> -			"%d\t\t" PCI_PRI_FMT "\t%" PRIu32 "\t\t0x%" PRIx64 "\n",
>> -			did, addr.domain, addr.bus, addr.devid,
>> -			addr.function, queue_num, features);
>> +		cmdline_printf(cl, "%d\t\t%s\t\t%" PRIu32 "\t\t0x%" PRIx64 "\n",
>> +			did, dev->name, queue_num, features);
>>  	}
>>  }
>>  
>> @@ -324,17 +325,12 @@ static void cmd_create_vdpa_port_parsed(void *parsed_result,
>>  {
>>  	int did;
>>  	struct cmd_create_result *res = parsed_result;
>> -	struct rte_vdpa_dev_addr addr;
>>  
>>  	rte_strscpy(vports[devcnt].ifname, res->socket_path, MAX_PATH_LEN);
>> -	if (rte_pci_addr_parse(res->bdf, &addr.pci_addr) != 0) {
>> -		cmdline_printf(cl, "Unable to parse the given bdf.\n");
>> -		return;
>> -	}
>> -	addr.type = VDPA_ADDR_PCI;
>> -	did = rte_vdpa_find_device_id(&addr);
>> +	did = rte_vdpa_find_device_id_by_name(res->bdf);
>>  	if (did < 0) {
>> -		cmdline_printf(cl, "Unable to find vdpa device id.\n");
>> +		cmdline_printf(cl, "Unable to find vdpa device id for %s.\n",
>> +				res->bdf);
>>  		return;
>>  	}
>>  
>> @@ -400,9 +396,11 @@ int
>>  main(int argc, char *argv[])
>>  {
>>  	char ch;
>> -	int i;
>> +	int did;
>>  	int ret;
>>  	struct cmdline *cl;
>> +	struct rte_device *dev;
>> +	struct rte_dev_iterator dev_iter;
>>  
>>  	ret = rte_eal_init(argc, argv);
>>  	if (ret < 0)
>> @@ -428,13 +426,18 @@ main(int argc, char *argv[])
>>  		cmdline_interact(cl);
>>  		cmdline_stdin_exit(cl);
>>  	} else {
>> -		for (i = 0; i < RTE_MIN(MAX_VDPA_SAMPLE_PORTS, dev_total);
>> -				i++) {
>> -			vports[i].did = i;
>> -			snprintf(vports[i].ifname, MAX_PATH_LEN, "%s%d",
>> -					iface, i);
>> +		RTE_DEV_FOREACH(dev, "class=vdpa", &dev_iter) {
>> +			did = rte_vdpa_find_device_id_by_name(dev->name);
>> +			if (did < 0) {
>> +				rte_panic("Failed to find device id for %s\n",
>> +						dev->name);
>> +			}
>> +			vports[devcnt].did = did;
>> +			snprintf(vports[devcnt].ifname, MAX_PATH_LEN, "%s%d",
>> +					iface, devcnt);
>>  
>> -			start_vdpa(&vports[i]);
>> +			start_vdpa(&vports[devcnt]);
>> +			devcnt++;
>>  		}
>>  
>>  		printf("enter \'q\' to quit\n");
>> diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
>> index 3c400ee79b..33037d39ea 100644
>> --- a/lib/librte_vhost/rte_vdpa.h
>> +++ b/lib/librte_vhost/rte_vdpa.h
>> @@ -18,25 +18,6 @@
>>  
>>  #define MAX_VDPA_NAME_LEN 128
>>  
>> -enum vdpa_addr_type {
>> -	VDPA_ADDR_PCI,
>> -	VDPA_ADDR_MAX
>> -};
>> -
>> -/**
>> - * vdpa device address
>> - */
>> -struct rte_vdpa_dev_addr {
>> -	/** vdpa address type */
>> -	enum vdpa_addr_type type;
>> -
>> -	/** vdpa pci address */
>> -	union {
>> -		uint8_t __dummy[64];
>> -		struct rte_pci_addr pci_addr;
>> -	};
>> -};
>> -
>>  /**
>>   * vdpa device operations
>>   */
>> @@ -81,8 +62,8 @@ struct rte_vdpa_dev_ops {
>>   * vdpa device structure includes device address and device operations.
>>   */
>>  struct rte_vdpa_device {
>> -	/** vdpa device address */
>> -	struct rte_vdpa_dev_addr addr;
>> +	/** Generic device information */
>> +	struct rte_device *device;
>>  	/** vdpa device operations */
>>  	struct rte_vdpa_dev_ops *ops;
>>  } __rte_cache_aligned;
>> @@ -102,7 +83,7 @@ struct rte_vdpa_device {
>>   */
>>  __rte_experimental
>>  int
>> -rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
>> +rte_vdpa_register_device(struct rte_device *rte_dev,
>>  		struct rte_vdpa_dev_ops *ops);
>>  
>>  /**
>> @@ -120,6 +101,21 @@ __rte_experimental
>>  int
>>  rte_vdpa_unregister_device(int did);
>>  
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change without prior notice
>> + *
>> + * Find the device id of a vdpa device from its name
>> + *
>> + * @param name
>> + *  the vdpa device name
>> + * @return
>> + *  device id on success, -1 on failure
>> + */
>> +__rte_experimental
>> +int
>> +rte_vdpa_find_device_id_by_name(const char *name);
>> +
>>  /**
>>   * @warning
>>   * @b EXPERIMENTAL: this API may change without prior notice
>> @@ -133,7 +129,7 @@ rte_vdpa_unregister_device(int did);
>>   */
>>  __rte_experimental
>>  int
>> -rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr);
>> +rte_vdpa_find_device_id(struct rte_vdpa_device *dev);
>>  
>>  /**
>>   * @warning
>> diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
>> index 051d08c120..1abfff8a0c 100644
>> --- a/lib/librte_vhost/rte_vhost_version.map
>> +++ b/lib/librte_vhost/rte_vhost_version.map
>> @@ -66,4 +66,5 @@ EXPERIMENTAL {
>>  	rte_vhost_get_vhost_ring_inflight;
>>  	rte_vhost_get_vring_base_from_inflight;
>>  	rte_vhost_slave_config_change;
>> +	rte_vdpa_find_device_id_by_name;
>>  };
>> diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
>> index 61ab9aadb4..5abc5a2a7c 100644
>> --- a/lib/librte_vhost/vdpa.c
>> +++ b/lib/librte_vhost/vdpa.c
>> @@ -18,43 +18,22 @@
>>  static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE];
>>  static uint32_t vdpa_device_num;
>>  
>> -static bool
>> -is_same_vdpa_device(struct rte_vdpa_dev_addr *a,
>> -		struct rte_vdpa_dev_addr *b)
>> -{
>> -	bool ret = true;
>> -
>> -	if (a->type != b->type)
>> -		return false;
>> -
>> -	switch (a->type) {
>> -	case VDPA_ADDR_PCI:
>> -		if (a->pci_addr.domain != b->pci_addr.domain ||
>> -				a->pci_addr.bus != b->pci_addr.bus ||
>> -				a->pci_addr.devid != b->pci_addr.devid ||
>> -				a->pci_addr.function != b->pci_addr.function)
>> -			ret = false;
>> -		break;
>> -	default:
>> -		break;
>> -	}
>> -
>> -	return ret;
>> -}
>> -
>>  int
>> -rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
>> +rte_vdpa_register_device(struct rte_device *rte_dev,
>>  		struct rte_vdpa_dev_ops *ops)
>>  {
>>  	struct rte_vdpa_device *dev;
>>  	int i;
>>  
>> -	if (vdpa_device_num >= MAX_VHOST_DEVICE || addr == NULL || ops == NULL)
>> +	if (vdpa_device_num >= MAX_VHOST_DEVICE || ops == NULL)
>>  		return -1;
>>  
>>  	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
>>  		dev = &vdpa_devices[i];
>> -		if (dev->ops && is_same_vdpa_device(&dev->addr, addr))
>> +		if (dev->ops == NULL)
>> +			continue;
>> +
>> +		if (dev->device == rte_dev)
>>  			return -1;
>>  	}
> 
> If we change the order of the two "if" statemets and replace "continue" with
> "break", we can remove the for loop that follows:
> 
> 	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
> 		if (vdpa_devices[i].ops == NULL)
> 			break;
> 	}

Do you mean like this?

	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
		if (dev->device == rte_dev)
			return -1;

		if (vdpa_devices[i].ops == NULL)
			break;
	}

If so the behaviour will be different, because you can have holes in the
array if a device is unregistered.

With above change it would stop looking if device is already registered
at the first hole, so could end into double registration of the same
device.

> 
>>  
>> @@ -67,7 +46,7 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
>>  		return -1;
>>  
>>  	dev = &vdpa_devices[i];
>> -	memcpy(&dev->addr, addr, sizeof(struct rte_vdpa_dev_addr));
>> +	dev->device = rte_dev;
>>  	dev->ops = ops;
>>  	vdpa_device_num++;
>>  
>> @@ -87,12 +66,33 @@ rte_vdpa_unregister_device(int did)
>>  }
>>  
>>  int
>> -rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr)
>> +rte_vdpa_find_device_id(struct rte_vdpa_device *dev)
>> +{
>> +	struct rte_vdpa_device *tmp_dev;
>> +	int i;
>> +
>> +	if (dev == NULL)
>> +		return -1;
>> +
>> +	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
>> +		tmp_dev = &vdpa_devices[i];
>> +		if (tmp_dev->ops == NULL)
>> +			continue;
>> +
>> +		if (tmp_dev == dev)
>> +			return i;
>> +	}
>> +
>> +	return -1;
>> +}
>> +
>> +int
>> +rte_vdpa_find_device_id_by_name(const char *name)
>>  {
>>  	struct rte_vdpa_device *dev;
>>  	int i;
>>  
>> -	if (addr == NULL)
>> +	if (name == NULL)
>>  		return -1;
>>  
>>  	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
>> @@ -100,7 +100,7 @@ rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr)
>>  		if (dev->ops == NULL)
>>  			continue;
>>  
>> -		if (is_same_vdpa_device(&dev->addr, addr))
>> +		if (strcmp(dev->device->name, name) == 0)
>>  			return i;
>>  	}
>>  
>> @@ -236,21 +236,10 @@ static int
>>  vdpa_dev_match(struct rte_vdpa_device *dev,
>>  	      const struct rte_device *rte_dev)
>>  {
>> -	struct rte_vdpa_dev_addr addr;
>> +	if (dev->device == rte_dev)
>> +		return 0;
>>  
>> -	/*  Only PCI bus supported for now */
>> -	if (strcmp(rte_dev->bus->name, "pci") != 0)
>> -		return -1;
>> -
>> -	addr.type = VDPA_ADDR_PCI;
>> -
>> -	if (rte_pci_addr_parse(rte_dev->name, &addr.pci_addr) != 0)
>> -		return -1;
>> -
>> -	if (!is_same_vdpa_device(&dev->addr, &addr))
>> -		return -1;
>> -
>> -	return 0;
>> +	return -1;
>>  }
>>  
>>  /* Generic rte_vdpa_dev comparison function. */
>>
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH 04/14] vhost: make vDPA framework bus agnostic
  2020-06-19 15:11     ` Maxime Coquelin
@ 2020-06-22  9:49       ` Adrian Moreno
  2020-06-22 12:34         ` Maxime Coquelin
  0 siblings, 1 reply; 25+ messages in thread
From: Adrian Moreno @ 2020-06-22  9:49 UTC (permalink / raw)
  To: Maxime Coquelin, dev, matan, xiao.w.wang, zhihong.wang,
	xiaolong.ye, chenbo.xia, david.marchand, shreyansh.jain,
	viacheslavo, hemant.agrawal, sachin.saxena



On 6/19/20 5:11 PM, Maxime Coquelin wrote:
> 
> 
> On 6/19/20 1:14 PM, Adrian Moreno wrote:
>>
>>
>> On 6/11/20 11:37 PM, Maxime Coquelin wrote:
>>> This patch makes the vDPA framework to no more
>>> support only PCI devices, but any devices by relying
>>> on the generic device name as identifier.
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>>  drivers/vdpa/ifc/ifcvf_vdpa.c          |  6 +-
>>>  drivers/vdpa/mlx5/mlx5_vdpa.c          |  8 +--
>>>  drivers/vdpa/mlx5/mlx5_vdpa.h          |  2 +-
>>>  examples/vdpa/main.c                   | 49 ++++++++--------
>>>  lib/librte_vhost/rte_vdpa.h            | 42 +++++++-------
>>>  lib/librte_vhost/rte_vhost_version.map |  1 +
>>>  lib/librte_vhost/vdpa.c                | 79 +++++++++++---------------
>>>  7 files changed, 85 insertions(+), 102 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c b/drivers/vdpa/ifc/ifcvf_vdpa.c
>>> index ec97178dcb..1fec1f1baf 100644
>>> --- a/drivers/vdpa/ifc/ifcvf_vdpa.c
>>> +++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
>>> @@ -47,7 +47,6 @@ static const char * const ifcvf_valid_arguments[] = {
>>>  static int ifcvf_vdpa_logtype;
>>>  
>>>  struct ifcvf_internal {
>>> -	struct rte_vdpa_dev_addr dev_addr;
>>>  	struct rte_pci_device *pdev;
>>>  	struct ifcvf_hw hw;
>>>  	int vfio_container_fd;
>>> @@ -1176,8 +1175,6 @@ ifcvf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>>>  		(1ULL << VHOST_USER_F_PROTOCOL_FEATURES) |
>>>  		(1ULL << VHOST_F_LOG_ALL);
>>>  
>>> -	internal->dev_addr.pci_addr = pci_dev->addr;
>>> -	internal->dev_addr.type = VDPA_ADDR_PCI;
>>>  	list->internal = internal;
>>>  
>>>  	if (rte_kvargs_count(kvlist, IFCVF_SW_FALLBACK_LM)) {
>>> @@ -1188,8 +1185,7 @@ ifcvf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>>>  	}
>>>  	internal->sw_lm = sw_fallback_lm;
>>>  
>>> -	internal->did = rte_vdpa_register_device(&internal->dev_addr,
>>> -				&ifcvf_ops);
>>> +	internal->did = rte_vdpa_register_device(&pci_dev->device, &ifcvf_ops);
>>>  	if (internal->did < 0) {
>>>  		DRV_LOG(ERR, "failed to register device %s", pci_dev->name);
>>>  		goto error;
>>> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c b/drivers/vdpa/mlx5/mlx5_vdpa.c
>>> index 1113d6cef0..e8255c7d7e 100644
>>> --- a/drivers/vdpa/mlx5/mlx5_vdpa.c
>>> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
>>> @@ -501,14 +501,13 @@ mlx5_vdpa_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>>>  	priv->caps = attr.vdpa;
>>>  	priv->log_max_rqt_size = attr.log_max_rqt_size;
>>>  	priv->ctx = ctx;
>>> -	priv->dev_addr.pci_addr = pci_dev->addr;
>>> -	priv->dev_addr.type = VDPA_ADDR_PCI;
>>> +	priv->pci_dev = pci_dev;
>>>  	priv->var = mlx5_glue->dv_alloc_var(ctx, 0);
>>>  	if (!priv->var) {
>>>  		DRV_LOG(ERR, "Failed to allocate VAR %u.\n", errno);
>>>  		goto error;
>>>  	}
>>> -	priv->id = rte_vdpa_register_device(&priv->dev_addr, &mlx5_vdpa_ops);
>>> +	priv->id = rte_vdpa_register_device(&pci_dev->device, &mlx5_vdpa_ops);
>>>  	if (priv->id < 0) {
>>>  		DRV_LOG(ERR, "Failed to register vDPA device.");
>>>  		rte_errno = rte_errno ? rte_errno : EINVAL;
>>> @@ -550,8 +549,7 @@ mlx5_vdpa_pci_remove(struct rte_pci_device *pci_dev)
>>>  
>>>  	pthread_mutex_lock(&priv_list_lock);
>>>  	TAILQ_FOREACH(priv, &priv_list, next) {
>>> -		if (memcmp(&priv->dev_addr.pci_addr, &pci_dev->addr,
>>> -			   sizeof(pci_dev->addr)) == 0) {
>>> +		if (priv->pci_dev == pci_dev) {
>>>  			found = 1;
>>>  			break;
>>>  		}
>>> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.h b/drivers/vdpa/mlx5/mlx5_vdpa.h
>>> index fcc216ac78..50ee3c5870 100644
>>> --- a/drivers/vdpa/mlx5/mlx5_vdpa.h
>>> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.h
>>> @@ -104,7 +104,7 @@ struct mlx5_vdpa_priv {
>>>  	int id; /* vDPA device id. */
>>>  	int vid; /* vhost device id. */
>>>  	struct ibv_context *ctx; /* Device context. */
>>> -	struct rte_vdpa_dev_addr dev_addr;
>>> +	struct rte_pci_device *pci_dev;
>>>  	struct mlx5_hca_vdpa_attr caps;
>>>  	uint32_t pdn; /* Protection Domain number. */
>>>  	struct ibv_pd *pd;
>>> diff --git a/examples/vdpa/main.c b/examples/vdpa/main.c
>>> index d9a9112b16..c12da69574 100644
>>> --- a/examples/vdpa/main.c
>>> +++ b/examples/vdpa/main.c
>>> @@ -271,10 +271,14 @@ static void cmd_list_vdpa_devices_parsed(
>>>  	uint32_t queue_num;
>>>  	uint64_t features;
>>>  	struct rte_vdpa_device *vdev;
>>> -	struct rte_pci_addr addr;
>>> +	struct rte_device *dev;
>>> +	struct rte_dev_iterator dev_iter;
>>>  
>>> -	cmdline_printf(cl, "device id\tdevice address\tqueue num\tsupported features\n");
>>> -	for (did = 0; did < dev_total; did++) {
>>> +	cmdline_printf(cl, "device id\tdevice name\tqueue num\tsupported features\n");
>>> +	RTE_DEV_FOREACH(dev, "class=vdpa", &dev_iter) {
>>> +		did = rte_vdpa_find_device_id_by_name(dev->name);
>>> +		if (did < 0)
>>> +			continue;
>>>  		vdev = rte_vdpa_get_device(did);
>>>  		if (!vdev)
>>>  			continue;
>>> @@ -290,11 +294,8 @@ static void cmd_list_vdpa_devices_parsed(
>>>  				"for device id %d.\n", did);
>>>  			continue;
>>>  		}
>>> -		addr = vdev->addr.pci_addr;
>>> -		cmdline_printf(cl,
>>> -			"%d\t\t" PCI_PRI_FMT "\t%" PRIu32 "\t\t0x%" PRIx64 "\n",
>>> -			did, addr.domain, addr.bus, addr.devid,
>>> -			addr.function, queue_num, features);
>>> +		cmdline_printf(cl, "%d\t\t%s\t\t%" PRIu32 "\t\t0x%" PRIx64 "\n",
>>> +			did, dev->name, queue_num, features);
>>>  	}
>>>  }
>>>  
>>> @@ -324,17 +325,12 @@ static void cmd_create_vdpa_port_parsed(void *parsed_result,
>>>  {
>>>  	int did;
>>>  	struct cmd_create_result *res = parsed_result;
>>> -	struct rte_vdpa_dev_addr addr;
>>>  
>>>  	rte_strscpy(vports[devcnt].ifname, res->socket_path, MAX_PATH_LEN);
>>> -	if (rte_pci_addr_parse(res->bdf, &addr.pci_addr) != 0) {
>>> -		cmdline_printf(cl, "Unable to parse the given bdf.\n");
>>> -		return;
>>> -	}
>>> -	addr.type = VDPA_ADDR_PCI;
>>> -	did = rte_vdpa_find_device_id(&addr);
>>> +	did = rte_vdpa_find_device_id_by_name(res->bdf);
>>>  	if (did < 0) {
>>> -		cmdline_printf(cl, "Unable to find vdpa device id.\n");
>>> +		cmdline_printf(cl, "Unable to find vdpa device id for %s.\n",
>>> +				res->bdf);
>>>  		return;
>>>  	}
>>>  
>>> @@ -400,9 +396,11 @@ int
>>>  main(int argc, char *argv[])
>>>  {
>>>  	char ch;
>>> -	int i;
>>> +	int did;
>>>  	int ret;
>>>  	struct cmdline *cl;
>>> +	struct rte_device *dev;
>>> +	struct rte_dev_iterator dev_iter;
>>>  
>>>  	ret = rte_eal_init(argc, argv);
>>>  	if (ret < 0)
>>> @@ -428,13 +426,18 @@ main(int argc, char *argv[])
>>>  		cmdline_interact(cl);
>>>  		cmdline_stdin_exit(cl);
>>>  	} else {
>>> -		for (i = 0; i < RTE_MIN(MAX_VDPA_SAMPLE_PORTS, dev_total);
>>> -				i++) {
>>> -			vports[i].did = i;
>>> -			snprintf(vports[i].ifname, MAX_PATH_LEN, "%s%d",
>>> -					iface, i);
>>> +		RTE_DEV_FOREACH(dev, "class=vdpa", &dev_iter) {
>>> +			did = rte_vdpa_find_device_id_by_name(dev->name);
>>> +			if (did < 0) {
>>> +				rte_panic("Failed to find device id for %s\n",
>>> +						dev->name);
>>> +			}
>>> +			vports[devcnt].did = did;
>>> +			snprintf(vports[devcnt].ifname, MAX_PATH_LEN, "%s%d",
>>> +					iface, devcnt);
>>>  
>>> -			start_vdpa(&vports[i]);
>>> +			start_vdpa(&vports[devcnt]);
>>> +			devcnt++;
>>>  		}
>>>  
>>>  		printf("enter \'q\' to quit\n");
>>> diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
>>> index 3c400ee79b..33037d39ea 100644
>>> --- a/lib/librte_vhost/rte_vdpa.h
>>> +++ b/lib/librte_vhost/rte_vdpa.h
>>> @@ -18,25 +18,6 @@
>>>  
>>>  #define MAX_VDPA_NAME_LEN 128
>>>  
>>> -enum vdpa_addr_type {
>>> -	VDPA_ADDR_PCI,
>>> -	VDPA_ADDR_MAX
>>> -};
>>> -
>>> -/**
>>> - * vdpa device address
>>> - */
>>> -struct rte_vdpa_dev_addr {
>>> -	/** vdpa address type */
>>> -	enum vdpa_addr_type type;
>>> -
>>> -	/** vdpa pci address */
>>> -	union {
>>> -		uint8_t __dummy[64];
>>> -		struct rte_pci_addr pci_addr;
>>> -	};
>>> -};
>>> -
>>>  /**
>>>   * vdpa device operations
>>>   */
>>> @@ -81,8 +62,8 @@ struct rte_vdpa_dev_ops {
>>>   * vdpa device structure includes device address and device operations.
>>>   */
>>>  struct rte_vdpa_device {
>>> -	/** vdpa device address */
>>> -	struct rte_vdpa_dev_addr addr;
>>> +	/** Generic device information */
>>> +	struct rte_device *device;
>>>  	/** vdpa device operations */
>>>  	struct rte_vdpa_dev_ops *ops;
>>>  } __rte_cache_aligned;
>>> @@ -102,7 +83,7 @@ struct rte_vdpa_device {
>>>   */
>>>  __rte_experimental
>>>  int
>>> -rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
>>> +rte_vdpa_register_device(struct rte_device *rte_dev,
>>>  		struct rte_vdpa_dev_ops *ops);
>>>  
>>>  /**
>>> @@ -120,6 +101,21 @@ __rte_experimental
>>>  int
>>>  rte_vdpa_unregister_device(int did);
>>>  
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>> + *
>>> + * Find the device id of a vdpa device from its name
>>> + *
>>> + * @param name
>>> + *  the vdpa device name
>>> + * @return
>>> + *  device id on success, -1 on failure
>>> + */
>>> +__rte_experimental
>>> +int
>>> +rte_vdpa_find_device_id_by_name(const char *name);
>>> +
>>>  /**
>>>   * @warning
>>>   * @b EXPERIMENTAL: this API may change without prior notice
>>> @@ -133,7 +129,7 @@ rte_vdpa_unregister_device(int did);
>>>   */
>>>  __rte_experimental
>>>  int
>>> -rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr);
>>> +rte_vdpa_find_device_id(struct rte_vdpa_device *dev);
>>>  
>>>  /**
>>>   * @warning
>>> diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
>>> index 051d08c120..1abfff8a0c 100644
>>> --- a/lib/librte_vhost/rte_vhost_version.map
>>> +++ b/lib/librte_vhost/rte_vhost_version.map
>>> @@ -66,4 +66,5 @@ EXPERIMENTAL {
>>>  	rte_vhost_get_vhost_ring_inflight;
>>>  	rte_vhost_get_vring_base_from_inflight;
>>>  	rte_vhost_slave_config_change;
>>> +	rte_vdpa_find_device_id_by_name;
>>>  };
>>> diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
>>> index 61ab9aadb4..5abc5a2a7c 100644
>>> --- a/lib/librte_vhost/vdpa.c
>>> +++ b/lib/librte_vhost/vdpa.c
>>> @@ -18,43 +18,22 @@
>>>  static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE];
>>>  static uint32_t vdpa_device_num;
>>>  
>>> -static bool
>>> -is_same_vdpa_device(struct rte_vdpa_dev_addr *a,
>>> -		struct rte_vdpa_dev_addr *b)
>>> -{
>>> -	bool ret = true;
>>> -
>>> -	if (a->type != b->type)
>>> -		return false;
>>> -
>>> -	switch (a->type) {
>>> -	case VDPA_ADDR_PCI:
>>> -		if (a->pci_addr.domain != b->pci_addr.domain ||
>>> -				a->pci_addr.bus != b->pci_addr.bus ||
>>> -				a->pci_addr.devid != b->pci_addr.devid ||
>>> -				a->pci_addr.function != b->pci_addr.function)
>>> -			ret = false;
>>> -		break;
>>> -	default:
>>> -		break;
>>> -	}
>>> -
>>> -	return ret;
>>> -}
>>> -
>>>  int
>>> -rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
>>> +rte_vdpa_register_device(struct rte_device *rte_dev,
>>>  		struct rte_vdpa_dev_ops *ops)
>>>  {
>>>  	struct rte_vdpa_device *dev;
>>>  	int i;
>>>  
>>> -	if (vdpa_device_num >= MAX_VHOST_DEVICE || addr == NULL || ops == NULL)
>>> +	if (vdpa_device_num >= MAX_VHOST_DEVICE || ops == NULL)
>>>  		return -1;
>>>  
>>>  	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
>>>  		dev = &vdpa_devices[i];
>>> -		if (dev->ops && is_same_vdpa_device(&dev->addr, addr))
>>> +		if (dev->ops == NULL)
>>> +			continue;
>>> +
>>> +		if (dev->device == rte_dev)
>>>  			return -1;
>>>  	}
>>
>> If we change the order of the two "if" statemets and replace "continue" with
>> "break", we can remove the for loop that follows:
>>
>> 	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
>> 		if (vdpa_devices[i].ops == NULL)
>> 			break;
>> 	}
> 
> Do you mean like this?
> 
> 	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
> 		if (dev->device == rte_dev)
> 			return -1;
> 
> 		if (vdpa_devices[i].ops == NULL)
> 			break;
> 	}
> 
> If so the behaviour will be different, because you can have holes in the
> array if a device is unregistered.
> 
Right, did not account for the unregistered holes. Still the double iteration
could be avoided by storing a pointer/index in the stack but maybe not needed in
this patch.

> With above change it would stop looking if device is already registered
> at the first hole, so could end into double registration of the same
> device.
> 
>>
>>>  
>>> @@ -67,7 +46,7 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
>>>  		return -1;
>>>  
>>>  	dev = &vdpa_devices[i];
>>> -	memcpy(&dev->addr, addr, sizeof(struct rte_vdpa_dev_addr));
>>> +	dev->device = rte_dev;
>>>  	dev->ops = ops;
>>>  	vdpa_device_num++;
>>>  
>>> @@ -87,12 +66,33 @@ rte_vdpa_unregister_device(int did)
>>>  }
>>>  
>>>  int
>>> -rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr)
>>> +rte_vdpa_find_device_id(struct rte_vdpa_device *dev)
>>> +{
>>> +	struct rte_vdpa_device *tmp_dev;
>>> +	int i;
>>> +
>>> +	if (dev == NULL)
>>> +		return -1;
>>> +
>>> +	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
>>> +		tmp_dev = &vdpa_devices[i];
>>> +		if (tmp_dev->ops == NULL)
>>> +			continue;
>>> +
>>> +		if (tmp_dev == dev)
>>> +			return i;
>>> +	}
>>> +
>>> +	return -1;
>>> +}
>>> +
>>> +int
>>> +rte_vdpa_find_device_id_by_name(const char *name)
>>>  {
>>>  	struct rte_vdpa_device *dev;
>>>  	int i;
>>>  
>>> -	if (addr == NULL)
>>> +	if (name == NULL)
>>>  		return -1;
>>>  
>>>  	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
>>> @@ -100,7 +100,7 @@ rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr)
>>>  		if (dev->ops == NULL)
>>>  			continue;
>>>  
>>> -		if (is_same_vdpa_device(&dev->addr, addr))
>>> +		if (strcmp(dev->device->name, name) == 0)
>>>  			return i;
>>>  	}
>>>  
>>> @@ -236,21 +236,10 @@ static int
>>>  vdpa_dev_match(struct rte_vdpa_device *dev,
>>>  	      const struct rte_device *rte_dev)
>>>  {
>>> -	struct rte_vdpa_dev_addr addr;
>>> +	if (dev->device == rte_dev)
>>> +		return 0;
>>>  
>>> -	/*  Only PCI bus supported for now */
>>> -	if (strcmp(rte_dev->bus->name, "pci") != 0)
>>> -		return -1;
>>> -
>>> -	addr.type = VDPA_ADDR_PCI;
>>> -
>>> -	if (rte_pci_addr_parse(rte_dev->name, &addr.pci_addr) != 0)
>>> -		return -1;
>>> -
>>> -	if (!is_same_vdpa_device(&dev->addr, &addr))
>>> -		return -1;
>>> -
>>> -	return 0;
>>> +	return -1;
>>>  }
>>>  
>>>  /* Generic rte_vdpa_dev comparison function. */
>>>
>>
> 

-- 
Adrián Moreno


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [dpdk-dev] [PATCH 04/14] vhost: make vDPA framework bus agnostic
  2020-06-22  9:49       ` Adrian Moreno
@ 2020-06-22 12:34         ` Maxime Coquelin
  0 siblings, 0 replies; 25+ messages in thread
From: Maxime Coquelin @ 2020-06-22 12:34 UTC (permalink / raw)
  To: Adrian Moreno, dev, matan, xiao.w.wang, zhihong.wang,
	xiaolong.ye, chenbo.xia, david.marchand, shreyansh.jain,
	viacheslavo, hemant.agrawal, sachin.saxena



On 6/22/20 11:49 AM, Adrian Moreno wrote:
> 
> 
> On 6/19/20 5:11 PM, Maxime Coquelin wrote:
>>
>>
>> On 6/19/20 1:14 PM, Adrian Moreno wrote:
>>>
>>>
>>> On 6/11/20 11:37 PM, Maxime Coquelin wrote:
>>>> This patch makes the vDPA framework to no more
>>>> support only PCI devices, but any devices by relying
>>>> on the generic device name as identifier.
>>>>
>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> ---
>>>>  drivers/vdpa/ifc/ifcvf_vdpa.c          |  6 +-
>>>>  drivers/vdpa/mlx5/mlx5_vdpa.c          |  8 +--
>>>>  drivers/vdpa/mlx5/mlx5_vdpa.h          |  2 +-
>>>>  examples/vdpa/main.c                   | 49 ++++++++--------
>>>>  lib/librte_vhost/rte_vdpa.h            | 42 +++++++-------
>>>>  lib/librte_vhost/rte_vhost_version.map |  1 +
>>>>  lib/librte_vhost/vdpa.c                | 79 +++++++++++---------------
>>>>  7 files changed, 85 insertions(+), 102 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c b/drivers/vdpa/ifc/ifcvf_vdpa.c
>>>> index ec97178dcb..1fec1f1baf 100644
>>>> --- a/drivers/vdpa/ifc/ifcvf_vdpa.c
>>>> +++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
>>>> @@ -47,7 +47,6 @@ static const char * const ifcvf_valid_arguments[] = {
>>>>  static int ifcvf_vdpa_logtype;
>>>>  
>>>>  struct ifcvf_internal {
>>>> -	struct rte_vdpa_dev_addr dev_addr;
>>>>  	struct rte_pci_device *pdev;
>>>>  	struct ifcvf_hw hw;
>>>>  	int vfio_container_fd;
>>>> @@ -1176,8 +1175,6 @@ ifcvf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>>>>  		(1ULL << VHOST_USER_F_PROTOCOL_FEATURES) |
>>>>  		(1ULL << VHOST_F_LOG_ALL);
>>>>  
>>>> -	internal->dev_addr.pci_addr = pci_dev->addr;
>>>> -	internal->dev_addr.type = VDPA_ADDR_PCI;
>>>>  	list->internal = internal;
>>>>  
>>>>  	if (rte_kvargs_count(kvlist, IFCVF_SW_FALLBACK_LM)) {
>>>> @@ -1188,8 +1185,7 @@ ifcvf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>>>>  	}
>>>>  	internal->sw_lm = sw_fallback_lm;
>>>>  
>>>> -	internal->did = rte_vdpa_register_device(&internal->dev_addr,
>>>> -				&ifcvf_ops);
>>>> +	internal->did = rte_vdpa_register_device(&pci_dev->device, &ifcvf_ops);
>>>>  	if (internal->did < 0) {
>>>>  		DRV_LOG(ERR, "failed to register device %s", pci_dev->name);
>>>>  		goto error;
>>>> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c b/drivers/vdpa/mlx5/mlx5_vdpa.c
>>>> index 1113d6cef0..e8255c7d7e 100644
>>>> --- a/drivers/vdpa/mlx5/mlx5_vdpa.c
>>>> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
>>>> @@ -501,14 +501,13 @@ mlx5_vdpa_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>>>>  	priv->caps = attr.vdpa;
>>>>  	priv->log_max_rqt_size = attr.log_max_rqt_size;
>>>>  	priv->ctx = ctx;
>>>> -	priv->dev_addr.pci_addr = pci_dev->addr;
>>>> -	priv->dev_addr.type = VDPA_ADDR_PCI;
>>>> +	priv->pci_dev = pci_dev;
>>>>  	priv->var = mlx5_glue->dv_alloc_var(ctx, 0);
>>>>  	if (!priv->var) {
>>>>  		DRV_LOG(ERR, "Failed to allocate VAR %u.\n", errno);
>>>>  		goto error;
>>>>  	}
>>>> -	priv->id = rte_vdpa_register_device(&priv->dev_addr, &mlx5_vdpa_ops);
>>>> +	priv->id = rte_vdpa_register_device(&pci_dev->device, &mlx5_vdpa_ops);
>>>>  	if (priv->id < 0) {
>>>>  		DRV_LOG(ERR, "Failed to register vDPA device.");
>>>>  		rte_errno = rte_errno ? rte_errno : EINVAL;
>>>> @@ -550,8 +549,7 @@ mlx5_vdpa_pci_remove(struct rte_pci_device *pci_dev)
>>>>  
>>>>  	pthread_mutex_lock(&priv_list_lock);
>>>>  	TAILQ_FOREACH(priv, &priv_list, next) {
>>>> -		if (memcmp(&priv->dev_addr.pci_addr, &pci_dev->addr,
>>>> -			   sizeof(pci_dev->addr)) == 0) {
>>>> +		if (priv->pci_dev == pci_dev) {
>>>>  			found = 1;
>>>>  			break;
>>>>  		}
>>>> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.h b/drivers/vdpa/mlx5/mlx5_vdpa.h
>>>> index fcc216ac78..50ee3c5870 100644
>>>> --- a/drivers/vdpa/mlx5/mlx5_vdpa.h
>>>> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.h
>>>> @@ -104,7 +104,7 @@ struct mlx5_vdpa_priv {
>>>>  	int id; /* vDPA device id. */
>>>>  	int vid; /* vhost device id. */
>>>>  	struct ibv_context *ctx; /* Device context. */
>>>> -	struct rte_vdpa_dev_addr dev_addr;
>>>> +	struct rte_pci_device *pci_dev;
>>>>  	struct mlx5_hca_vdpa_attr caps;
>>>>  	uint32_t pdn; /* Protection Domain number. */
>>>>  	struct ibv_pd *pd;
>>>> diff --git a/examples/vdpa/main.c b/examples/vdpa/main.c
>>>> index d9a9112b16..c12da69574 100644
>>>> --- a/examples/vdpa/main.c
>>>> +++ b/examples/vdpa/main.c
>>>> @@ -271,10 +271,14 @@ static void cmd_list_vdpa_devices_parsed(
>>>>  	uint32_t queue_num;
>>>>  	uint64_t features;
>>>>  	struct rte_vdpa_device *vdev;
>>>> -	struct rte_pci_addr addr;
>>>> +	struct rte_device *dev;
>>>> +	struct rte_dev_iterator dev_iter;
>>>>  
>>>> -	cmdline_printf(cl, "device id\tdevice address\tqueue num\tsupported features\n");
>>>> -	for (did = 0; did < dev_total; did++) {
>>>> +	cmdline_printf(cl, "device id\tdevice name\tqueue num\tsupported features\n");
>>>> +	RTE_DEV_FOREACH(dev, "class=vdpa", &dev_iter) {
>>>> +		did = rte_vdpa_find_device_id_by_name(dev->name);
>>>> +		if (did < 0)
>>>> +			continue;
>>>>  		vdev = rte_vdpa_get_device(did);
>>>>  		if (!vdev)
>>>>  			continue;
>>>> @@ -290,11 +294,8 @@ static void cmd_list_vdpa_devices_parsed(
>>>>  				"for device id %d.\n", did);
>>>>  			continue;
>>>>  		}
>>>> -		addr = vdev->addr.pci_addr;
>>>> -		cmdline_printf(cl,
>>>> -			"%d\t\t" PCI_PRI_FMT "\t%" PRIu32 "\t\t0x%" PRIx64 "\n",
>>>> -			did, addr.domain, addr.bus, addr.devid,
>>>> -			addr.function, queue_num, features);
>>>> +		cmdline_printf(cl, "%d\t\t%s\t\t%" PRIu32 "\t\t0x%" PRIx64 "\n",
>>>> +			did, dev->name, queue_num, features);
>>>>  	}
>>>>  }
>>>>  
>>>> @@ -324,17 +325,12 @@ static void cmd_create_vdpa_port_parsed(void *parsed_result,
>>>>  {
>>>>  	int did;
>>>>  	struct cmd_create_result *res = parsed_result;
>>>> -	struct rte_vdpa_dev_addr addr;
>>>>  
>>>>  	rte_strscpy(vports[devcnt].ifname, res->socket_path, MAX_PATH_LEN);
>>>> -	if (rte_pci_addr_parse(res->bdf, &addr.pci_addr) != 0) {
>>>> -		cmdline_printf(cl, "Unable to parse the given bdf.\n");
>>>> -		return;
>>>> -	}
>>>> -	addr.type = VDPA_ADDR_PCI;
>>>> -	did = rte_vdpa_find_device_id(&addr);
>>>> +	did = rte_vdpa_find_device_id_by_name(res->bdf);
>>>>  	if (did < 0) {
>>>> -		cmdline_printf(cl, "Unable to find vdpa device id.\n");
>>>> +		cmdline_printf(cl, "Unable to find vdpa device id for %s.\n",
>>>> +				res->bdf);
>>>>  		return;
>>>>  	}
>>>>  
>>>> @@ -400,9 +396,11 @@ int
>>>>  main(int argc, char *argv[])
>>>>  {
>>>>  	char ch;
>>>> -	int i;
>>>> +	int did;
>>>>  	int ret;
>>>>  	struct cmdline *cl;
>>>> +	struct rte_device *dev;
>>>> +	struct rte_dev_iterator dev_iter;
>>>>  
>>>>  	ret = rte_eal_init(argc, argv);
>>>>  	if (ret < 0)
>>>> @@ -428,13 +426,18 @@ main(int argc, char *argv[])
>>>>  		cmdline_interact(cl);
>>>>  		cmdline_stdin_exit(cl);
>>>>  	} else {
>>>> -		for (i = 0; i < RTE_MIN(MAX_VDPA_SAMPLE_PORTS, dev_total);
>>>> -				i++) {
>>>> -			vports[i].did = i;
>>>> -			snprintf(vports[i].ifname, MAX_PATH_LEN, "%s%d",
>>>> -					iface, i);
>>>> +		RTE_DEV_FOREACH(dev, "class=vdpa", &dev_iter) {
>>>> +			did = rte_vdpa_find_device_id_by_name(dev->name);
>>>> +			if (did < 0) {
>>>> +				rte_panic("Failed to find device id for %s\n",
>>>> +						dev->name);
>>>> +			}
>>>> +			vports[devcnt].did = did;
>>>> +			snprintf(vports[devcnt].ifname, MAX_PATH_LEN, "%s%d",
>>>> +					iface, devcnt);
>>>>  
>>>> -			start_vdpa(&vports[i]);
>>>> +			start_vdpa(&vports[devcnt]);
>>>> +			devcnt++;
>>>>  		}
>>>>  
>>>>  		printf("enter \'q\' to quit\n");
>>>> diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
>>>> index 3c400ee79b..33037d39ea 100644
>>>> --- a/lib/librte_vhost/rte_vdpa.h
>>>> +++ b/lib/librte_vhost/rte_vdpa.h
>>>> @@ -18,25 +18,6 @@
>>>>  
>>>>  #define MAX_VDPA_NAME_LEN 128
>>>>  
>>>> -enum vdpa_addr_type {
>>>> -	VDPA_ADDR_PCI,
>>>> -	VDPA_ADDR_MAX
>>>> -};
>>>> -
>>>> -/**
>>>> - * vdpa device address
>>>> - */
>>>> -struct rte_vdpa_dev_addr {
>>>> -	/** vdpa address type */
>>>> -	enum vdpa_addr_type type;
>>>> -
>>>> -	/** vdpa pci address */
>>>> -	union {
>>>> -		uint8_t __dummy[64];
>>>> -		struct rte_pci_addr pci_addr;
>>>> -	};
>>>> -};
>>>> -
>>>>  /**
>>>>   * vdpa device operations
>>>>   */
>>>> @@ -81,8 +62,8 @@ struct rte_vdpa_dev_ops {
>>>>   * vdpa device structure includes device address and device operations.
>>>>   */
>>>>  struct rte_vdpa_device {
>>>> -	/** vdpa device address */
>>>> -	struct rte_vdpa_dev_addr addr;
>>>> +	/** Generic device information */
>>>> +	struct rte_device *device;
>>>>  	/** vdpa device operations */
>>>>  	struct rte_vdpa_dev_ops *ops;
>>>>  } __rte_cache_aligned;
>>>> @@ -102,7 +83,7 @@ struct rte_vdpa_device {
>>>>   */
>>>>  __rte_experimental
>>>>  int
>>>> -rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
>>>> +rte_vdpa_register_device(struct rte_device *rte_dev,
>>>>  		struct rte_vdpa_dev_ops *ops);
>>>>  
>>>>  /**
>>>> @@ -120,6 +101,21 @@ __rte_experimental
>>>>  int
>>>>  rte_vdpa_unregister_device(int did);
>>>>  
>>>> +/**
>>>> + * @warning
>>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>>> + *
>>>> + * Find the device id of a vdpa device from its name
>>>> + *
>>>> + * @param name
>>>> + *  the vdpa device name
>>>> + * @return
>>>> + *  device id on success, -1 on failure
>>>> + */
>>>> +__rte_experimental
>>>> +int
>>>> +rte_vdpa_find_device_id_by_name(const char *name);
>>>> +
>>>>  /**
>>>>   * @warning
>>>>   * @b EXPERIMENTAL: this API may change without prior notice
>>>> @@ -133,7 +129,7 @@ rte_vdpa_unregister_device(int did);
>>>>   */
>>>>  __rte_experimental
>>>>  int
>>>> -rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr);
>>>> +rte_vdpa_find_device_id(struct rte_vdpa_device *dev);
>>>>  
>>>>  /**
>>>>   * @warning
>>>> diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
>>>> index 051d08c120..1abfff8a0c 100644
>>>> --- a/lib/librte_vhost/rte_vhost_version.map
>>>> +++ b/lib/librte_vhost/rte_vhost_version.map
>>>> @@ -66,4 +66,5 @@ EXPERIMENTAL {
>>>>  	rte_vhost_get_vhost_ring_inflight;
>>>>  	rte_vhost_get_vring_base_from_inflight;
>>>>  	rte_vhost_slave_config_change;
>>>> +	rte_vdpa_find_device_id_by_name;
>>>>  };
>>>> diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
>>>> index 61ab9aadb4..5abc5a2a7c 100644
>>>> --- a/lib/librte_vhost/vdpa.c
>>>> +++ b/lib/librte_vhost/vdpa.c
>>>> @@ -18,43 +18,22 @@
>>>>  static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE];
>>>>  static uint32_t vdpa_device_num;
>>>>  
>>>> -static bool
>>>> -is_same_vdpa_device(struct rte_vdpa_dev_addr *a,
>>>> -		struct rte_vdpa_dev_addr *b)
>>>> -{
>>>> -	bool ret = true;
>>>> -
>>>> -	if (a->type != b->type)
>>>> -		return false;
>>>> -
>>>> -	switch (a->type) {
>>>> -	case VDPA_ADDR_PCI:
>>>> -		if (a->pci_addr.domain != b->pci_addr.domain ||
>>>> -				a->pci_addr.bus != b->pci_addr.bus ||
>>>> -				a->pci_addr.devid != b->pci_addr.devid ||
>>>> -				a->pci_addr.function != b->pci_addr.function)
>>>> -			ret = false;
>>>> -		break;
>>>> -	default:
>>>> -		break;
>>>> -	}
>>>> -
>>>> -	return ret;
>>>> -}
>>>> -
>>>>  int
>>>> -rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
>>>> +rte_vdpa_register_device(struct rte_device *rte_dev,
>>>>  		struct rte_vdpa_dev_ops *ops)
>>>>  {
>>>>  	struct rte_vdpa_device *dev;
>>>>  	int i;
>>>>  
>>>> -	if (vdpa_device_num >= MAX_VHOST_DEVICE || addr == NULL || ops == NULL)
>>>> +	if (vdpa_device_num >= MAX_VHOST_DEVICE || ops == NULL)
>>>>  		return -1;
>>>>  
>>>>  	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
>>>>  		dev = &vdpa_devices[i];
>>>> -		if (dev->ops && is_same_vdpa_device(&dev->addr, addr))
>>>> +		if (dev->ops == NULL)
>>>> +			continue;
>>>> +
>>>> +		if (dev->device == rte_dev)
>>>>  			return -1;
>>>>  	}
>>>
>>> If we change the order of the two "if" statemets and replace "continue" with
>>> "break", we can remove the for loop that follows:
>>>
>>> 	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
>>> 		if (vdpa_devices[i].ops == NULL)
>>> 			break;
>>> 	}
>>
>> Do you mean like this?
>>
>> 	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
>> 		if (dev->device == rte_dev)
>> 			return -1;
>>
>> 		if (vdpa_devices[i].ops == NULL)
>> 			break;
>> 	}
>>
>> If so the behaviour will be different, because you can have holes in the
>> array if a device is unregistered.
>>
> Right, did not account for the unregistered holes. Still the double iteration
> could be avoided by storing a pointer/index in the stack but maybe not needed in
> this patch.

Yes, certainly. Now these iterations are just an intermediate step of
the series. The end result gets rid of these loops, so maybe not worth
the effort of optimizing it.

>> With above change it would stop looking if device is already registered
>> at the first hole, so could end into double registration of the same
>> device.
>>
>>>
>>>>  
>>>> @@ -67,7 +46,7 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr,
>>>>  		return -1;
>>>>  
>>>>  	dev = &vdpa_devices[i];
>>>> -	memcpy(&dev->addr, addr, sizeof(struct rte_vdpa_dev_addr));
>>>> +	dev->device = rte_dev;
>>>>  	dev->ops = ops;
>>>>  	vdpa_device_num++;
>>>>  
>>>> @@ -87,12 +66,33 @@ rte_vdpa_unregister_device(int did)
>>>>  }
>>>>  
>>>>  int
>>>> -rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr)
>>>> +rte_vdpa_find_device_id(struct rte_vdpa_device *dev)
>>>> +{
>>>> +	struct rte_vdpa_device *tmp_dev;
>>>> +	int i;
>>>> +
>>>> +	if (dev == NULL)
>>>> +		return -1;
>>>> +
>>>> +	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
>>>> +		tmp_dev = &vdpa_devices[i];
>>>> +		if (tmp_dev->ops == NULL)
>>>> +			continue;
>>>> +
>>>> +		if (tmp_dev == dev)
>>>> +			return i;
>>>> +	}
>>>> +
>>>> +	return -1;
>>>> +}
>>>> +
>>>> +int
>>>> +rte_vdpa_find_device_id_by_name(const char *name)
>>>>  {
>>>>  	struct rte_vdpa_device *dev;
>>>>  	int i;
>>>>  
>>>> -	if (addr == NULL)
>>>> +	if (name == NULL)
>>>>  		return -1;
>>>>  
>>>>  	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
>>>> @@ -100,7 +100,7 @@ rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr)
>>>>  		if (dev->ops == NULL)
>>>>  			continue;
>>>>  
>>>> -		if (is_same_vdpa_device(&dev->addr, addr))
>>>> +		if (strcmp(dev->device->name, name) == 0)
>>>>  			return i;
>>>>  	}
>>>>  
>>>> @@ -236,21 +236,10 @@ static int
>>>>  vdpa_dev_match(struct rte_vdpa_device *dev,
>>>>  	      const struct rte_device *rte_dev)
>>>>  {
>>>> -	struct rte_vdpa_dev_addr addr;
>>>> +	if (dev->device == rte_dev)
>>>> +		return 0;
>>>>  
>>>> -	/*  Only PCI bus supported for now */
>>>> -	if (strcmp(rte_dev->bus->name, "pci") != 0)
>>>> -		return -1;
>>>> -
>>>> -	addr.type = VDPA_ADDR_PCI;
>>>> -
>>>> -	if (rte_pci_addr_parse(rte_dev->name, &addr.pci_addr) != 0)
>>>> -		return -1;
>>>> -
>>>> -	if (!is_same_vdpa_device(&dev->addr, &addr))
>>>> -		return -1;
>>>> -
>>>> -	return 0;
>>>> +	return -1;
>>>>  }
>>>>  
>>>>  /* Generic rte_vdpa_dev comparison function. */
>>>>
>>>
>>
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2020-06-22 12:35 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-11 21:37 [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Maxime Coquelin
2020-06-11 21:37 ` [dpdk-dev] [PATCH 01/14] bus/dpaa: fix null pointer dereference Maxime Coquelin
2020-06-12  9:48   ` Gaëtan Rivet
2020-06-11 21:37 ` [dpdk-dev] [PATCH 02/14] bus/fslmc: " Maxime Coquelin
2020-06-11 21:37 ` [dpdk-dev] [PATCH 03/14] vhost: introduce vDPA devices class Maxime Coquelin
2020-06-12 12:37   ` Gaëtan Rivet
2020-06-15  9:15     ` Maxime Coquelin
2020-06-11 21:37 ` [dpdk-dev] [PATCH 04/14] vhost: make vDPA framework bus agnostic Maxime Coquelin
2020-06-12 12:46   ` Gaëtan Rivet
2020-06-15  9:17     ` Maxime Coquelin
2020-06-19 11:14   ` Adrian Moreno
2020-06-19 15:11     ` Maxime Coquelin
2020-06-22  9:49       ` Adrian Moreno
2020-06-22 12:34         ` Maxime Coquelin
2020-06-11 21:37 ` [dpdk-dev] [PATCH 05/14] vhost: replace device ID in vDPA ops Maxime Coquelin
2020-06-11 21:37 ` [dpdk-dev] [PATCH 06/14] vhost: replace vDPA device ID in Vhost Maxime Coquelin
2020-06-11 21:37 ` [dpdk-dev] [PATCH 07/14] vhost: replace device ID in applications Maxime Coquelin
2020-06-11 21:37 ` [dpdk-dev] [PATCH 08/14] vhost: remove useless vDPA API Maxime Coquelin
2020-06-11 21:37 ` [dpdk-dev] [PATCH 09/14] vhost: use linked-list for vDPA devices Maxime Coquelin
2020-06-11 21:37 ` [dpdk-dev] [PATCH 10/14] vhost: introduce wrappers for some vDPA ops Maxime Coquelin
2020-06-11 21:37 ` [dpdk-dev] [PATCH 11/14] examples/vdpa: use new wrappers instead of ops Maxime Coquelin
2020-06-11 21:37 ` [dpdk-dev] [PATCH 12/14] examples/vdpa: remove useless device count Maxime Coquelin
2020-06-11 21:37 ` [dpdk-dev] [PATCH 13/14] vhost: remove vDPA device count API Maxime Coquelin
2020-06-11 21:37 ` [dpdk-dev] [PATCH 14/14] vhost: split vDPA header file Maxime Coquelin
2020-06-12  9:38 ` [dpdk-dev] [PATCH 00/14] vDPA API and framework rework Gaëtan Rivet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).