From: wangyunjian <wangyunjian@huawei.com>
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"sthemmin@microsoft.com" <sthemmin@microsoft.com>,
"Lilijun (Jerry)" <jerry.lilijun@huawei.com>,
xudingke <xudingke@huawei.com>,
"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] bus/vmbus: fix wrong allocation for device.name
Date: Mon, 1 Jun 2020 02:02:02 +0000 [thread overview]
Message-ID: <34EFBCA9F01B0748BEB6B629CE643AE60D02D5FD@dggemm513-mbx.china.huawei.com> (raw)
In-Reply-To: <20200529104726.0a76d7bf@hermes.lan>
> -----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
prev parent reply other threads:[~2020-06-01 2:02 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-28 12:03 wangyunjian
2020-05-28 16:12 ` Stephen Hemminger
2020-05-29 7:24 ` wangyunjian
2020-05-29 17:47 ` Stephen Hemminger
2020-06-01 2:02 ` wangyunjian [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=34EFBCA9F01B0748BEB6B629CE643AE60D02D5FD@dggemm513-mbx.china.huawei.com \
--to=wangyunjian@huawei.com \
--cc=dev@dpdk.org \
--cc=jerry.lilijun@huawei.com \
--cc=stable@dpdk.org \
--cc=stephen@networkplumber.org \
--cc=sthemmin@microsoft.com \
--cc=xudingke@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).