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

[wangyunjian <wangyunjian@huawei.com>]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, May 30, 2020 1:47 AM
> To: wangyunjian <wangyunjian@huawei.com>
> Cc: dev@dpdk.org; sthemmin@microsoft.com; Lilijun (Jerry)
> <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com>;
> stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] bus/vmbus: fix wrong allocation for
> device.name
> 
> 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:
> 

Looks good , this patch is a more complete fix.

Thanks,
Yunjian

> 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