> -----Original Message-----
> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Saturday, May 30, 2020 1:47 AM
> To: wangyunjian <wangyunj...@huawei.com>
> Cc: dev@dpdk.org; sthem...@microsoft.com; Lilijun (Jerry)
> <jerry.lili...@huawei.com>; xudingke <xudin...@huawei.com>;
> sta...@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 <wangyunj...@huawei.com> wrote:
>
> > From: Yunjian Wang <wangyunj...@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: sta...@dpdk.org
> >
> > Signed-off-by: Yunjian Wang <wangyunj...@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 <step...@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 <step...@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
