[dpdk-dev] [PATCH 0/4] couple hotplug fix

[dpdk-dev] [PATCH 0/4] couple hotplug fix

[Qi Zhang]

The patchset fix couple issues that related with hotplug add and hotplug
remove.

Qi Zhang (4):
  eal: fix hotplug add and hotplug remove
  bus/pci: fix PCI address compare
  bus/pci: enable vfio unmap resource for secondary
  vfio: remove uneccessary IPC for group fd clear

 drivers/bus/pci/linux/pci_vfio.c               | 118 +++++++++++++++++++------
 lib/librte_eal/common/eal_common_dev.c         |  26 +++---
 lib/librte_eal/linuxapp/eal/eal_vfio.c         |  45 ++--------
 lib/librte_eal/linuxapp/eal/eal_vfio.h         |   1 -
 lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c |   8 --
 5 files changed, 110 insertions(+), 88 deletions(-)

-- 
2.13.6

[dpdk-dev] [PATCH 1/4] eal: fix hotplug add and hotplug remove

[Qi Zhang]

If hotplug add an already plugged PCI device, it will
cause rte_pci_device->device.name be corrupted due to unexpected
rte_devargs_remove. Also if try to hotplug remove an already
unplugged device, it will cause segment fault due to unexpected
bus->unplug on a rte_device whose driver is NULL.
The patch fix these issues.

Fixes: 7e8b26650146 ("eal: fix hotplug add / remove")
Cc: stable@dpdk.org

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 lib/librte_eal/common/eal_common_dev.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 61cb3b162..0fa8c815d 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -42,18 +42,6 @@ static struct dev_event_cb_list dev_event_cbs;
 /* spinlock for device callbacks */
 static rte_spinlock_t dev_event_lock = RTE_SPINLOCK_INITIALIZER;
 
-static int cmp_detached_dev_name(const struct rte_device *dev,
-	const void *_name)
-{
-	const char *name = _name;
-
-	/* skip attached devices */
-	if (dev->driver != NULL)
-		return 1;
-
-	return strcmp(dev->name, name);
-}
-
 static int cmp_dev_name(const struct rte_device *dev, const void *_name)
 {
 	const char *name = _name;
@@ -151,14 +139,19 @@ int __rte_experimental rte_eal_hotplug_add(const char *busname, const char *devn
 	if (ret)
 		goto err_devarg;
 
-	dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
+	dev = bus->find_device(NULL, cmp_dev_name, devname);
 	if (dev == NULL) {
-		RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
+		RTE_LOG(ERR, EAL, "Cannot find device (%s)\n",
 			devname);
 		ret = -ENODEV;
 		goto err_devarg;
 	}
 
+	if (dev->driver != NULL) {
+		RTE_LOG(ERR, EAL, "Device is already plugged\n");
+		return -EEXIST;
+	}
+
 	ret = bus->plug(dev);
 	if (ret) {
 		RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n",
@@ -200,6 +193,11 @@ rte_eal_hotplug_remove(const char *busname, const char *devname)
 		return -EINVAL;
 	}
 
+	if (dev->driver == NULL) {
+		RTE_LOG(ERR, EAL, "Device is already unplugged\n");
+		return -ENOENT;
+	}
+
 	ret = bus->unplug(dev);
 	if (ret)
 		RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n",
-- 
2.13.6

Re: [dpdk-dev] [PATCH 1/4] eal: fix hotplug add and hotplug remove

[Gaëtan Rivet]

Hi Qi,

On Thu, Jul 12, 2018 at 10:01:41PM +0800, Qi Zhang wrote:
> If hotplug add an already plugged PCI device, it will
> cause rte_pci_device->device.name be corrupted due to unexpected
> rte_devargs_remove. Also if try to hotplug remove an already
> unplugged device, it will cause segment fault due to unexpected
> bus->unplug on a rte_device whose driver is NULL.
> The patch fix these issues.
> 
> Fixes: 7e8b26650146 ("eal: fix hotplug add / remove")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>

I think we should consolidate this API at some point, maybe list the
possible error values as a part of it and remove the experimental tag.

In any case, the fix seems correct, thanks,

Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

-- 
Gaëtan Rivet
6WIND

[dpdk-dev] [PATCH 2/4] bus/pci: fix PCI address compare

[Qi Zhang]

When use memcmp to compare two PCI address, sizeof(struct rte_pci_addr)
is 4 bytes aligned, and it is 8. While only 7 byte of struct rte_pci_addr
is valid. So compare the 8th byte will cause the unexpected result, which
happens when repeatedly attach/detach a device.

Fixes: 94c0776b1bad ("vfio: support hotplug")
Cc: stable@dpdk.org

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 drivers/bus/pci/linux/pci_vfio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index aeeaa9ed8..933b95540 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -642,7 +642,7 @@ pci_vfio_unmap_resource(struct rte_pci_device *dev)
 	vfio_res_list = RTE_TAILQ_CAST(rte_vfio_tailq.head, mapped_pci_res_list);
 	/* Get vfio_res */
 	TAILQ_FOREACH(vfio_res, vfio_res_list, next) {
-		if (memcmp(&vfio_res->pci_addr, &dev->addr, sizeof(dev->addr)))
+		if (rte_pci_addr_cmp(&vfio_res->pci_addr, &dev->addr))
 			continue;
 		break;
 	}
-- 
2.13.6

Re: [dpdk-dev] [PATCH 2/4] bus/pci: fix PCI address compare

[Gaëtan Rivet]

On Thu, Jul 12, 2018 at 10:01:42PM +0800, Qi Zhang wrote:
> When use memcmp to compare two PCI address, sizeof(struct rte_pci_addr)
> is 4 bytes aligned, and it is 8. While only 7 byte of struct rte_pci_addr
> is valid. So compare the 8th byte will cause the unexpected result, which
> happens when repeatedly attach/detach a device.
> 
> Fixes: 94c0776b1bad ("vfio: support hotplug")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  drivers/bus/pci/linux/pci_vfio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index aeeaa9ed8..933b95540 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -642,7 +642,7 @@ pci_vfio_unmap_resource(struct rte_pci_device *dev)
>  	vfio_res_list = RTE_TAILQ_CAST(rte_vfio_tailq.head, mapped_pci_res_list);
>  	/* Get vfio_res */
>  	TAILQ_FOREACH(vfio_res, vfio_res_list, next) {
> -		if (memcmp(&vfio_res->pci_addr, &dev->addr, sizeof(dev->addr)))
> +		if (rte_pci_addr_cmp(&vfio_res->pci_addr, &dev->addr))
>  			continue;
>  		break;
>  	}
> -- 
> 2.13.6
> 

-- 
Gaëtan Rivet
6WIND

[dpdk-dev] [PATCH 3/4] bus/pci: enable vfio unmap resource for secondary

[Qi Zhang]

Subroutine to unmap VFIO resource is shared by secondary and
primary, and it does not work on the secondary process. Since
for secondary process, it is not necessary to close interrupt
handler, set pci bus mastering and remove vfio_res from
vfio_res_list. So, the patch adds a dedicate function to handle
the situation when a device is unmapped on a secondary process.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 drivers/bus/pci/linux/pci_vfio.c | 118 +++++++++++++++++++++++++++++----------
 1 file changed, 90 insertions(+), 28 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 933b95540..686386d6a 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -584,6 +584,9 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
 		dev->mem_resource[i].addr = maps[i].addr;
 	}
 
+	/* we need save vfio_dev_fd, so it can be used during release */
+	dev->intr_handle.vfio_dev_fd = vfio_dev_fd;
+
 	return 0;
 err_vfio_dev_fd:
 	close(vfio_dev_fd);
@@ -603,22 +606,58 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
 		return pci_vfio_map_resource_secondary(dev);
 }
 
-int
-pci_vfio_unmap_resource(struct rte_pci_device *dev)
+static struct mapped_pci_resource *
+find_and_unmap_vfio_resource(struct mapped_pci_res_list *vfio_res_list,
+			struct rte_pci_device *dev,
+			const char *pci_addr)
+{
+	struct mapped_pci_resource *vfio_res = NULL;
+	struct pci_map *maps;
+	int i;
+
+	/* Get vfio_res */
+	TAILQ_FOREACH(vfio_res, vfio_res_list, next) {
+		if (rte_pci_addr_cmp(&vfio_res->pci_addr, &dev->addr))
+			continue;
+		break;
+	}
+
+	if  (vfio_res == NULL)
+		return vfio_res;
+
+	RTE_LOG(INFO, EAL, "Releasing pci mapped resource for %s\n",
+		pci_addr);
+
+	maps = vfio_res->maps;
+	for (i = 0; i < (int) vfio_res->nb_maps; i++) {
+
+		/*
+		 * We do not need to be aware of MSI-X table BAR mappings as
+		 * when mapping. Just using current maps array is enough
+		 */
+		if (maps[i].addr) {
+			RTE_LOG(INFO, EAL, "Calling pci_unmap_resource for %s at %p\n",
+				pci_addr, maps[i].addr);
+			pci_unmap_resource(maps[i].addr, maps[i].size);
+		}
+	}
+
+	return vfio_res;
+}
+
+static int
+pci_vfio_unmap_resource_primary(struct rte_pci_device *dev)
 {
 	char pci_addr[PATH_MAX] = {0};
 	struct rte_pci_addr *loc = &dev->addr;
-	int i, ret;
 	struct mapped_pci_resource *vfio_res = NULL;
 	struct mapped_pci_res_list *vfio_res_list;
-
-	struct pci_map *maps;
+	int ret;
 
 	/* store PCI address string */
 	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
 			loc->domain, loc->bus, loc->devid, loc->function);
 
-
 	if (close(dev->intr_handle.fd) < 0) {
 		RTE_LOG(INFO, EAL, "Error when closing eventfd file descriptor for %s\n",
 			pci_addr);
@@ -639,13 +678,10 @@ pci_vfio_unmap_resource(struct rte_pci_device *dev)
 		return ret;
 	}
 
-	vfio_res_list = RTE_TAILQ_CAST(rte_vfio_tailq.head, mapped_pci_res_list);
-	/* Get vfio_res */
-	TAILQ_FOREACH(vfio_res, vfio_res_list, next) {
-		if (rte_pci_addr_cmp(&vfio_res->pci_addr, &dev->addr))
-			continue;
-		break;
-	}
+	vfio_res_list =
+		RTE_TAILQ_CAST(rte_vfio_tailq.head, mapped_pci_res_list);
+	vfio_res = find_and_unmap_vfio_resource(vfio_res_list, dev, pci_addr);
+
 	/* if we haven't found our tailq entry, something's wrong */
 	if (vfio_res == NULL) {
 		RTE_LOG(ERR, EAL, "  %s cannot find TAILQ entry for PCI device!\n",
@@ -653,30 +689,56 @@ pci_vfio_unmap_resource(struct rte_pci_device *dev)
 		return -1;
 	}
 
-	/* unmap BARs */
-	maps = vfio_res->maps;
+	TAILQ_REMOVE(vfio_res_list, vfio_res, next);
 
-	RTE_LOG(INFO, EAL, "Releasing pci mapped resource for %s\n",
-		pci_addr);
-	for (i = 0; i < (int) vfio_res->nb_maps; i++) {
+	return 0;
+}
 
-		/*
-		 * We do not need to be aware of MSI-X table BAR mappings as
-		 * when mapping. Just using current maps array is enough
-		 */
-		if (maps[i].addr) {
-			RTE_LOG(INFO, EAL, "Calling pci_unmap_resource for %s at %p\n",
-				pci_addr, maps[i].addr);
-			pci_unmap_resource(maps[i].addr, maps[i].size);
-		}
+static int
+pci_vfio_unmap_resource_secondary(struct rte_pci_device *dev)
+{
+	char pci_addr[PATH_MAX] = {0};
+	struct rte_pci_addr *loc = &dev->addr;
+	struct mapped_pci_resource *vfio_res = NULL;
+	struct mapped_pci_res_list *vfio_res_list;
+	int ret;
+
+	/* store PCI address string */
+	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
+			loc->domain, loc->bus, loc->devid, loc->function);
+
+	ret = rte_vfio_release_device(rte_pci_get_sysfs_path(), pci_addr,
+				  dev->intr_handle.vfio_dev_fd);
+	if (ret < 0) {
+		RTE_LOG(ERR, EAL,
+			"%s(): cannot release device\n", __func__);
+		return ret;
 	}
 
-	TAILQ_REMOVE(vfio_res_list, vfio_res, next);
+	vfio_res_list =
+		RTE_TAILQ_CAST(rte_vfio_tailq.head, mapped_pci_res_list);
+	vfio_res = find_and_unmap_vfio_resource(vfio_res_list, dev, pci_addr);
+
+	/* if we haven't found our tailq entry, something's wrong */
+	if (vfio_res == NULL) {
+		RTE_LOG(ERR, EAL, "  %s cannot find TAILQ entry for PCI device!\n",
+				pci_addr);
+		return -1;
+	}
 
 	return 0;
 }
 
 int
+pci_vfio_unmap_resource(struct rte_pci_device *dev)
+{
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+		return pci_vfio_unmap_resource_primary(dev);
+	else
+		return pci_vfio_unmap_resource_secondary(dev);
+}
+
+int
 pci_vfio_ioport_map(struct rte_pci_device *dev, int bar,
 		    struct rte_pci_ioport *p)
 {
-- 
2.13.6

[dpdk-dev] [PATCH 4/4] vfio: remove uneccessary IPC for group fd clear

[Qi Zhang]

Clear vfio_group_fd is not necessary to involve any IPC.
Also, current IPC implementation for SOCKET_CLR_GROUP is not
correct. rte_vfio_clear_group on secondary will always fail,
that prevent device be detached correctly on a secondary process.
The patch simply removes all IPC related stuff in
rte_vfio_clear_group.

Fixes: 83a73c5fef66 ("vfio: use generic multi-process channel")
Cc: stable@dpdk.org

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_vfio.c         | 45 +++++---------------------
 lib/librte_eal/linuxapp/eal/eal_vfio.h         |  1 -
 lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c |  8 -----
 3 files changed, 8 insertions(+), 46 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c b/lib/librte_eal/linuxapp/eal/eal_vfio.c
index a2bbdfbf4..c0eccddc3 100644
--- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
@@ -575,10 +575,6 @@ int
 rte_vfio_clear_group(int vfio_group_fd)
 {
 	int i;
-	struct rte_mp_msg mp_req, *mp_rep;
-	struct rte_mp_reply mp_reply;
-	struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
-	struct vfio_mp_param *p = (struct vfio_mp_param *)mp_req.param;
 	struct vfio_config *vfio_cfg;
 
 	vfio_cfg = get_vfio_cfg_by_group_fd(vfio_group_fd);
@@ -587,40 +583,15 @@ rte_vfio_clear_group(int vfio_group_fd)
 		return -1;
 	}
 
-	if (internal_config.process_type == RTE_PROC_PRIMARY) {
-
-		i = get_vfio_group_idx(vfio_group_fd);
-		if (i < 0)
-			return -1;
-		vfio_cfg->vfio_groups[i].group_num = -1;
-		vfio_cfg->vfio_groups[i].fd = -1;
-		vfio_cfg->vfio_groups[i].devices = 0;
-		vfio_cfg->vfio_active_groups--;
-		return 0;
-	}
-
-	p->req = SOCKET_CLR_GROUP;
-	p->group_num = vfio_group_fd;
-	strcpy(mp_req.name, EAL_VFIO_MP);
-	mp_req.len_param = sizeof(*p);
-	mp_req.num_fds = 0;
-
-	if (rte_mp_request_sync(&mp_req, &mp_reply, &ts) == 0 &&
-	    mp_reply.nb_received == 1) {
-		mp_rep = &mp_reply.msgs[0];
-		p = (struct vfio_mp_param *)mp_rep->param;
-		if (p->result == SOCKET_OK) {
-			free(mp_reply.msgs);
-			return 0;
-		} else if (p->result == SOCKET_NO_FD)
-			RTE_LOG(ERR, EAL, "  BAD VFIO group fd!\n");
-		else
-			RTE_LOG(ERR, EAL, "  no such VFIO group fd!\n");
-
-		free(mp_reply.msgs);
-	}
+	i = get_vfio_group_idx(vfio_group_fd);
+	if (i < 0)
+		return -1;
+	vfio_cfg->vfio_groups[i].group_num = -1;
+	vfio_cfg->vfio_groups[i].fd = -1;
+	vfio_cfg->vfio_groups[i].devices = 0;
+	vfio_cfg->vfio_active_groups--;
 
-	return -1;
+	return 0;
 }
 
 int
diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.h b/lib/librte_eal/linuxapp/eal/eal_vfio.h
index e65b10374..68d4750a5 100644
--- a/lib/librte_eal/linuxapp/eal/eal_vfio.h
+++ b/lib/librte_eal/linuxapp/eal/eal_vfio.h
@@ -129,7 +129,6 @@ int vfio_mp_sync_setup(void);
 
 #define SOCKET_REQ_CONTAINER 0x100
 #define SOCKET_REQ_GROUP 0x200
-#define SOCKET_CLR_GROUP 0x300
 #define SOCKET_OK 0x0
 #define SOCKET_NO_FD 0x1
 #define SOCKET_ERR 0xFF
diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
index 9c202bb08..680a24aae 100644
--- a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
+++ b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
@@ -55,14 +55,6 @@ vfio_mp_primary(const struct rte_mp_msg *msg, const void *peer)
 			reply.fds[0] = fd;
 		}
 		break;
-	case SOCKET_CLR_GROUP:
-		r->req = SOCKET_CLR_GROUP;
-		r->group_num = m->group_num;
-		if (rte_vfio_clear_group(m->group_num) < 0)
-			r->result = SOCKET_NO_FD;
-		else
-			r->result = SOCKET_OK;
-		break;
 	case SOCKET_REQ_CONTAINER:
 		r->req = SOCKET_REQ_CONTAINER;
 		fd = rte_vfio_get_container_fd();
-- 
2.13.6

Re: [dpdk-dev] [PATCH 0/4] couple hotplug fix

[Thomas Monjalon]

12/07/2018 16:01, Qi Zhang:
> The patchset fix couple issues that related with hotplug add and hotplug
> remove.
> 
> Qi Zhang (4):
>   eal: fix hotplug add and hotplug remove
>   bus/pci: fix PCI address compare
>   bus/pci: enable vfio unmap resource for secondary
>   vfio: remove uneccessary IPC for group fd clear

Applied, thanks