Re: [dpdk-dev] [PATCH] bus/vmbus: fix wrong allocation for device.name

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 28 May 2020 20:03:07 +0800
wangyunjian <wangyunjian@huawei.com> wrote:

> From: Yunjian Wang <wangyunjian@huawei.com>
> 
> We do not need and should not allocate memory for device.name.
> The device.name should be set point to the devargs->name.
> 
> Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>

The correct fix is more complex. The code needs to act more like PCI.
The name should be in the vmbus_device structure (like PCI).
And the devargs logic needs to be more involved and also handle
whitelist/blacklisting.

Here is a very rough idea what that would look like:

From edf90357a99c7970546ef00941a9593bca46d08e Mon Sep 17 00:00:00 2001
From: Stephen Hemminger <stephen@networkplumber.org>
Date: Fri, 29 May 2020 10:45:26 -0700
Subject: [PATCH] vmbus: support whitelist/blacklist

This makes VMBus work like PCI and support blacklist and
whitelist command line arguments.

It also moves the vmbus device name into the internal
structure.

This is compile tested only.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/bus/vmbus/linux/vmbus_bus.c | 30 ++++++++--------
 drivers/bus/vmbus/private.h         |  5 ++-
 drivers/bus/vmbus/rte_bus_vmbus.h   |  2 +-
 drivers/bus/vmbus/vmbus_common.c    | 53 ++++++++++++++++++++++++++---
 4 files changed, 67 insertions(+), 23 deletions(-)

diff --git a/drivers/bus/vmbus/linux/vmbus_bus.c b/drivers/bus/vmbus/linux/vmbus_bus.c
index 3c924eee1412..9162b30bb46c 100644
--- a/drivers/bus/vmbus/linux/vmbus_bus.c
+++ b/drivers/bus/vmbus/linux/vmbus_bus.c
@@ -14,7 +14,6 @@
 #include <rte_uuid.h>
 #include <rte_tailq.h>
 #include <rte_log.h>
-#include <rte_devargs.h>
 #include <rte_memory.h>
 #include <rte_malloc.h>
 #include <rte_bus_vmbus.h>
@@ -230,7 +229,7 @@ rte_vmbus_unmap_device(struct rte_vmbus_device *dev)
 
 /* Scan one vmbus sysfs entry, and fill the devices list from it. */
 static int
-vmbus_scan_one(const char *name)
+vmbus_scan_one(rte_uuid_t id, const char *name)
 {
 	struct rte_vmbus_device *dev, *dev2;
 	char filename[PATH_MAX];
@@ -242,14 +241,10 @@ vmbus_scan_one(const char *name)
 		return -1;
 
 	dev->device.bus = &rte_vmbus_bus.bus;
-	dev->device.name = strdup(name);
-	if (!dev->device.name)
-		goto error;
+	rte_uuid_copy(dev->device_id, id);
 
 	/* sysfs base directory
 	 *   /sys/bus/vmbus/devices/7a08391f-f5a0-4ac0-9802-d13fd964f8df
-	 * or on older kernel
-	 *   /sys/bus/vmbus/devices/vmbus_1
 	 */
 	snprintf(dirname, sizeof(dirname), "%s/%s",
 		 SYSFS_VMBUS_DEVICES, name);
@@ -265,11 +260,6 @@ vmbus_scan_one(const char *name)
 		return 0;
 	}
 
-	/* get device id */
-	snprintf(filename, sizeof(filename), "%s/device_id", dirname);
-	if (parse_sysfs_uuid(filename, dev->device_id) < 0)
-		goto error;
-
 	/* get relid */
 	snprintf(filename, sizeof(filename), "%s/id", dirname);
 	if (eal_parse_sysfs_value(filename, &tmp) < 0)
@@ -295,9 +285,6 @@ vmbus_scan_one(const char *name)
 		dev->device.numa_node = SOCKET_ID_ANY;
 	}
 
-	dev->device.devargs = vmbus_devargs_lookup(dev);
-
-	/* device is valid, add in list (sorted) */
 	VMBUS_LOG(DEBUG, "Adding vmbus device %s", name);
 
 	TAILQ_FOREACH(dev2, &rte_vmbus_bus.device_list, next) {
@@ -346,10 +333,21 @@ rte_vmbus_scan(void)
 	}
 
 	while ((e = readdir(dir)) != NULL) {
+		rte_uuid_t id;
+
 		if (e->d_name[0] == '.')
 			continue;
 
-		if (vmbus_scan_one(e->d_name) < 0)
+		if (rte_uuid_parse(e->d_name, id) != 0) {
+			VMBUS_LOG(DEBUG, "ignore '%s' non-uuid in vmbus/devices",
+				  e->d_name);
+			continue;
+		}
+
+		if (vmbus_ignore_device(id))
+			continue;
+
+		if (vmbus_scan_one(id, e->d_name) < 0)
 			goto error;
 	}
 	closedir(dir);
diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h
index f19b14e4a657..dd6a17c7238c 100644
--- a/drivers/bus/vmbus/private.h
+++ b/drivers/bus/vmbus/private.h
@@ -71,12 +71,15 @@ struct vmbus_channel {
 #define VMBUS_MAX_CHANNELS	64
 
 struct rte_devargs *
-vmbus_devargs_lookup(struct rte_vmbus_device *dev);
+vmbus_devargs_lookup(rte_uuid_t device_id);
+
+void vmbus_name_set(struct rte_vmbus_device *dev);
 
 int vmbus_chan_create(const struct rte_vmbus_device *device,
 		      uint16_t relid, uint16_t subid, uint8_t monitor_id,
 		      struct vmbus_channel **new_chan);
 
+bool vmbus_ignore_device(rte_uuid_t device_id);
 void vmbus_add_device(struct rte_vmbus_device *vmbus_dev);
 void vmbus_insert_device(struct rte_vmbus_device *exist_vmbus_dev,
 			 struct rte_vmbus_device *new_vmbus_dev);
diff --git a/drivers/bus/vmbus/rte_bus_vmbus.h b/drivers/bus/vmbus/rte_bus_vmbus.h
index 4cf73ce81513..62d067f19179 100644
--- a/drivers/bus/vmbus/rte_bus_vmbus.h
+++ b/drivers/bus/vmbus/rte_bus_vmbus.h
@@ -73,7 +73,7 @@ struct rte_vmbus_device {
 	uint32_t *int_page;		       /**< VMBUS interrupt page */
 	struct vmbus_channel *primary;	       /**< VMBUS primary channel */
 	struct vmbus_mon_page *monitor_page;   /**< VMBUS monitor page */
-
+	char name[RTE_UUID_STRLEN];	       /**< VMBUS uuid (ASCII) */
 	struct rte_intr_handle intr_handle;    /**< Interrupt handle */
 	struct rte_mem_resource resource[VMBUS_MAX_RESOURCE];
 };
diff --git a/drivers/bus/vmbus/vmbus_common.c b/drivers/bus/vmbus/vmbus_common.c
index 3adef01c95de..ee2180f1d10c 100644
--- a/drivers/bus/vmbus/vmbus_common.c
+++ b/drivers/bus/vmbus/vmbus_common.c
@@ -61,6 +61,21 @@ vmbus_unmap_resource(void *requested_addr, size_t size)
 			  requested_addr);
 }
 
+void
+vmbus_name_set(struct rte_vmbus_device *dev)
+{
+	struct rte_devargs *devargs;
+
+	devargs = vmbus_devargs_lookup(dev->device_id);
+	rte_uuid_unparse(dev->device_id, dev->name, sizeof(dev->name));
+
+	/* if the device is not blacklisted, no rte_devargs exists for it. */
+	if (devargs)
+		dev->device.name = dev->device.devargs->name;
+	else
+		dev->device.name = dev->name;
+}
+
 /**
  * Match the VMBUS driver and device using UUID table
  *
@@ -85,6 +100,7 @@ vmbus_match(const struct rte_vmbus_driver *dr,
 
 	return false;
 }
+
 /*
  * If device ID match, call the devinit() function of the driver.
  */
@@ -99,10 +115,18 @@ vmbus_probe_one_driver(struct rte_vmbus_driver *dr,
 		return 1;	 /* not supported */
 
 	rte_uuid_unparse(dev->device_id, guid, sizeof(guid));
-	VMBUS_LOG(INFO, "VMBUS device %s on NUMA socket %i",
+	VMBUS_LOG(DEBUG, "VMBUS device %s on NUMA socket %i",
 		  guid, dev->device.numa_node);
 
-	/* TODO add blacklisted */
+	if (dev->device.devargs != NULL &&
+	    dev->device.devargs->policy == RTE_DEV_BLACKLISTED) {
+		VMBUS_LOG(INFO, "  Device is blacklisted, not initializing\n");
+		return 1;
+	}
+
+	/* Older kernel versions do not report NUMA node for vmbus */
+	if (dev->device.numa_node < 0)
+		dev->device.numa_node = 0;
 
 	/* map resources for device */
 	ret = rte_vmbus_map_device(dev);
@@ -210,15 +234,16 @@ vmbus_parse(const char *name, void *addr)
  *	-w 'vmbus:635a7ae3-091e-4410-ad59-667c4f8c04c3,latency=20'
  */
 struct rte_devargs *
-vmbus_devargs_lookup(struct rte_vmbus_device *dev)
+vmbus_devargs_lookup(rte_uuid_t device_id)
 {
 	struct rte_devargs *devargs;
-	rte_uuid_t addr;
 
 	RTE_EAL_DEVARGS_FOREACH("vmbus", devargs) {
+		rte_uuid_t addr;
+
 		vmbus_parse(devargs->name, &addr);
 
-		if (rte_uuid_compare(dev->device_id, addr) == 0)
+		if (rte_uuid_compare(device_id, addr) == 0)
 			return devargs;
 	}
 	return NULL;
@@ -285,6 +310,24 @@ vmbus_find_device(const struct rte_device *start, rte_dev_cmp_t cmp,
 	return NULL;
 }
 
+bool
+vmbus_ignore_device(rte_uuid_t device_id)
+{
+	struct rte_devargs *devargs = vmbus_devargs_lookup(device_id);
+
+	switch (rte_vmbus_bus.bus.conf.scan_mode) {
+	case RTE_BUS_SCAN_WHITELIST:
+		if (devargs && devargs->policy == RTE_DEV_WHITELISTED)
+			return false;
+		break;
+	case RTE_BUS_SCAN_UNDEFINED:
+	case RTE_BUS_SCAN_BLACKLIST:
+		if (devargs == NULL || devargs->policy != RTE_DEV_BLACKLISTED)
+			return false;
+		break;
+	}
+	return true;
+}
 
 struct rte_vmbus_bus rte_vmbus_bus = {
 	.bus = {
-- 
2.26.2