env_dpdk/pci: remove thread safety from PCI APIs

For VMD driver we'll need to introduce some way of
iterating over all spdk pci device objects and we would
like to achieve that with simple spdk_pci_get_first_dev()/get_next_dev()
APIs. To make it thread safe though, we would have to
expose some public pci mutex to be locked around the
iteration and we don't want to do that, so we'll make
PCI APIs usable from only a single thread - this will
prevent any pci devices from being removed inbetween
subsequent get_first/get_next calls.

We currently have the following players accessing pci
device state:
 1) public APIs, obviously (on any thread right now)
 2) VFIO hotremove callback (dpdk interrupt thread)
 3) rte_eal_alarm for detaching rte_pci_devices (dpdk
    interrupt thread)
 4) DPDK hotplug IPC (dpdk interrupt thread)

There is g_pci_mutex providing the thread safety, but
even today it doesn't protect #3 and #4, making the
entire pci layer prone to data corruption.

To make #3 and #4 safe, we would have to lock inside
device init/fini callbacks (spdk_pci_device_init/fini),
but those are called directly inside the public device
attach/detach functions which already lock.

So now, with the decision to drop thread safety from
public pci APIs, we narrow down the locks inside public
functions and introduce locks inside those lower-level
init/fini callbacks.

Change-Id: I5dcbc9cdcbab65ee76cd3c42890f596069ec9a8a
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/458930
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>

lib/env_dpdk/pci.c

@@ -142,6 +142,7 @@ spdk_pci_device_rte_hotremove(const char *device_name,
 			      void *cb_arg)
 {
 	struct spdk_pci_device *dev;
+	bool can_detach = false;
 
 	if (event != RTE_DEV_EVENT_REMOVE) {
 		return;
@@ -151,23 +152,20 @@ spdk_pci_device_rte_hotremove(const char *device_name,
 	TAILQ_FOREACH(dev, &g_pci_devices, internal.tailq) {
 		struct rte_pci_device *rte_dev = dev->dev_handle;
 
-		if (strcmp(rte_dev->name, device_name) == 0) {
+		if (strcmp(rte_dev->name, device_name) == 0 &&
-			if (!dev->internal.pending_removal &&
+		    !dev->internal.pending_removal) {
-			    !dev->internal.attached) {
+			can_detach = !dev->internal.attached;
-				/* if device is not attached, we
+			/* prevent any further attaches */
-				 * can remove it right away.
+			dev->internal.pending_removal = true;
-				 */
-				spdk_detach_rte(dev);
-			} else {
-				/* otherwise we let the upper layers
-				 * detach it first.
-				 */
-				dev->internal.pending_removal = true;
-			}
 			break;
 		}
 	}
 	pthread_mutex_unlock(&g_pci_mutex);
+
+	if (dev != NULL && can_detach) {
+		/* if device is not attached, we can remove it right away. */
+		spdk_detach_rte(dev);
+	}
 }
 #endif
 
@@ -276,8 +274,10 @@ spdk_pci_device_init(struct rte_pci_driver *_drv,
 		dev->internal.attached = true;
 	}
 
+	pthread_mutex_lock(&g_pci_mutex);
 	TAILQ_INSERT_TAIL(&g_pci_devices, dev, internal.tailq);
 	spdk_vtophys_pci_device_added(dev->dev_handle);
+	pthread_mutex_unlock(&g_pci_mutex);
 	return 0;
 }
 
@@ -286,6 +286,7 @@ spdk_pci_device_fini(struct rte_pci_device *_dev)
 {
 	struct spdk_pci_device *dev;
 
+	pthread_mutex_lock(&g_pci_mutex);
 	TAILQ_FOREACH(dev, &g_pci_devices, internal.tailq) {
 		if (dev->dev_handle == _dev) {
 			break;
@@ -294,11 +295,13 @@ spdk_pci_device_fini(struct rte_pci_device *_dev)
 
 	if (dev == NULL || dev->internal.attached) {
 		/* The device might be still referenced somewhere in SPDK. */
+		pthread_mutex_unlock(&g_pci_mutex);
 		return -1;
 	}
 
 	spdk_vtophys_pci_device_removed(dev->dev_handle);
 	TAILQ_REMOVE(&g_pci_devices, dev, internal.tailq);
+	pthread_mutex_unlock(&g_pci_mutex);
 	free(dev);
 	return 0;
 
@@ -324,7 +327,6 @@ spdk_pci_device_attach(struct spdk_pci_driver *driver,
 	spdk_pci_addr_fmt(bdf, sizeof(bdf), pci_address);
 
 	pthread_mutex_lock(&g_pci_mutex);
-
 	TAILQ_FOREACH(dev, &g_pci_devices, internal.tailq) {
 		if (spdk_pci_addr_compare(&dev->addr, pci_address) == 0) {
 			break;
@@ -344,6 +346,7 @@ spdk_pci_device_attach(struct spdk_pci_driver *driver,
 		pthread_mutex_unlock(&g_pci_mutex);
 		return rc;
 	}
+	pthread_mutex_unlock(&g_pci_mutex);
 
 	if (!driver->is_registered) {
 		driver->is_registered = true;
@@ -372,8 +375,6 @@ spdk_pci_device_attach(struct spdk_pci_driver *driver,
 
 	driver->cb_arg = NULL;
 	driver->cb_fn = NULL;
-	pthread_mutex_unlock(&g_pci_mutex);
-
 	return rc == 0 ? 0 : -1;
 }
 
@@ -390,7 +391,6 @@ spdk_pci_enumerate(struct spdk_pci_driver *driver,
 	int rc;
 
 	pthread_mutex_lock(&g_pci_mutex);
-
 	TAILQ_FOREACH(dev, &g_pci_devices, internal.tailq) {
 		if (dev->internal.attached ||
 		    dev->internal.driver != driver ||
@@ -406,6 +406,7 @@ spdk_pci_enumerate(struct spdk_pci_driver *driver,
 			return -1;
 		}
 	}
+	pthread_mutex_unlock(&g_pci_mutex);
 
 	if (!driver->is_registered) {
 		driver->is_registered = true;
@@ -418,14 +419,11 @@ spdk_pci_enumerate(struct spdk_pci_driver *driver,
 	if (rte_bus_scan() != 0 || rte_bus_probe() != 0) {
 		driver->cb_arg = NULL;
 		driver->cb_fn = NULL;
-		pthread_mutex_unlock(&g_pci_mutex);
 		return -1;
 	}
 
 	driver->cb_arg = NULL;
 	driver->cb_fn = NULL;
-	pthread_mutex_unlock(&g_pci_mutex);
-
 	return 0;
 }