Mailing List Archive

[PATCH 1/5] uio: Add hv_vmbus_client driver
Generic VMBus driver that can be dynamically bound, to any simple
low speed Hyper-V VMBus device. This driver supports single
channel and uses hypercall instead of monitor bits to interrupt
host, which is suitable for low speed devices. Additionally, the
driver also provide the flexibility of custom ring buffer sizes and
avoid memory allocation for gpadl to offer memory optimization.

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
drivers/uio/Kconfig | 12 ++
drivers/uio/Makefile | 1 +
drivers/uio/uio_hv_vmbus_client.c | 232 ++++++++++++++++++++++++++++++
3 files changed, 245 insertions(+)
create mode 100644 drivers/uio/uio_hv_vmbus_client.c

diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 2e16c5338e5b..dcc727e6fd3f 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -166,6 +166,18 @@ config UIO_HV_GENERIC

If you compile this as a module, it will be called uio_hv_generic.

+config UIO_HV_SLOW_DEVICES
+ tristate "Generic driver for low speed VMBus devices"
+ depends on HYPERV
+ help
+ Generic driver that you can bind, dynamically, to any simple
+ Hyper-V VMBus device with single channel and these devices
+ uses hypercall instead of monitor bits to interrupt host. This
+ driver also provide the flexibility of custom ring buffer sizes.
+ It is useful for slower VMbus devices.
+
+ If you compile this as a module, it will be called uio_hv_vmbus_client.
+
config UIO_DFL
tristate "Generic driver for DFL (Device Feature List) bus"
depends on FPGA_DFL
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index f2f416a14228..44be0f96da34 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -11,4 +11,5 @@ obj-$(CONFIG_UIO_PRUSS) += uio_pruss.o
obj-$(CONFIG_UIO_MF624) += uio_mf624.o
obj-$(CONFIG_UIO_FSL_ELBC_GPCM) += uio_fsl_elbc_gpcm.o
obj-$(CONFIG_UIO_HV_GENERIC) += uio_hv_generic.o
+obj-$(CONFIG_UIO_HV_SLOW_DEVICES) += uio_hv_vmbus_client.o
obj-$(CONFIG_UIO_DFL) += uio_dfl.o
diff --git a/drivers/uio/uio_hv_vmbus_client.c b/drivers/uio/uio_hv_vmbus_client.c
new file mode 100644
index 000000000000..a5a47caeffe1
--- /dev/null
+++ b/drivers/uio/uio_hv_vmbus_client.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * uio_hv_vmbus_client - UIO driver for low speed VMBus devices
+ *
+ * Copyright (c) 2023, Microsoft Corporation.
+ *
+ * Authors:
+ * Saurabh Sengar <ssengar@microsoft.com>
+ *
+ * Since the driver does not declare any device ids, you must allocate
+ * id and bind the device to the driver yourself. For example:
+ * driverctl -b vmbus set-override <dev uuid> uio_hv_vmbus_client
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/uio_driver.h>
+#include <linux/hyperv.h>
+#include <linux/vmalloc.h>
+#include <linux/sysfs.h>
+
+#define DRIVER_VERSION "0.0.1"
+#define DRIVER_AUTHOR "Saurabh Sengar <ssengar@microsoft.com>"
+#define DRIVER_DESC "Generic UIO driver for low speed VMBus devices"
+
+#define DEFAULT_HV_RING_SIZE VMBUS_RING_SIZE(3 * HV_HYP_PAGE_SIZE)
+static int ring_size = DEFAULT_HV_RING_SIZE;
+
+struct uio_hv_vmbus_dev {
+ struct uio_info info;
+ struct hv_device *device;
+ atomic_t refcnt;
+};
+
+/* Sysfs API to allow mmap of the ring buffers
+ * The ring buffer is allocated as contiguous memory by vmbus_open
+ */
+static int uio_hv_vmbus_mmap(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *attr, struct vm_area_struct *vma)
+{
+ struct vmbus_channel *channel = container_of(kobj, struct vmbus_channel, kobj);
+ void *ring_buffer = page_address(channel->ringbuffer_page);
+
+ return vm_iomap_memory(vma, virt_to_phys(ring_buffer),
+ channel->ringbuffer_pagecount << PAGE_SHIFT);
+}
+
+static const struct bin_attribute ring_buffer_bin_attr = {
+ .attr = {
+ .name = "ringbuffer",
+ .mode = 0600,
+ },
+ .mmap = uio_hv_vmbus_mmap,
+};
+
+/*
+ * This is the irqcontrol callback to be registered to uio_info.
+ * It can be used to disable/enable interrupt from user space processes.
+ *
+ * @param info
+ * pointer to uio_info.
+ * @param irq_state
+ * state value. 1 to enable interrupt, 0 to disable interrupt.
+ */
+static int uio_hv_vmbus_irqcontrol(struct uio_info *info, s32 irq_state)
+{
+ struct uio_hv_vmbus_dev *pdata = info->priv;
+ struct hv_device *hv_dev = pdata->device;
+
+ /* Issue a full memory barrier before triggering the notification */
+ virt_mb();
+
+ vmbus_setevent(hv_dev->channel);
+ return 0;
+}
+
+/*
+ * Callback from vmbus_event when something is in inbound ring.
+ */
+static void uio_hv_vmbus_channel_cb(void *context)
+{
+ struct uio_hv_vmbus_dev *pdata = context;
+
+ /* Issue a full memory barrier before sending the event to userspace */
+ virt_mb();
+
+ uio_event_notify(&pdata->info);
+}
+
+static int uio_hv_vmbus_open(struct uio_info *info, struct inode *inode)
+{
+ struct uio_hv_vmbus_dev *pdata = container_of(info, struct uio_hv_vmbus_dev, info);
+ struct hv_device *hv_dev = pdata->device;
+ struct vmbus_channel *channel = hv_dev->channel;
+ int ret = 0;
+
+ if (atomic_inc_return(&pdata->refcnt) != 1)
+ return 0;
+
+ ret = vmbus_open(channel, ring_size, ring_size, NULL, 0,
+ uio_hv_vmbus_channel_cb, pdata);
+ if (ret) {
+ dev_err(&hv_dev->device, "error %d when opening the channel\n", ret);
+ goto done;
+ }
+ channel->inbound.ring_buffer->interrupt_mask = 0;
+ set_channel_read_mode(channel, HV_CALL_ISR);
+
+ ret = sysfs_create_bin_file(&channel->kobj, &ring_buffer_bin_attr);
+ if (ret)
+ dev_notice(&hv_dev->device, "sysfs create ring bin file failed; %d\n", ret);
+
+ return 0;
+
+done:
+ atomic_dec(&pdata->refcnt);
+ return ret;
+}
+
+/* VMbus primary channel is closed on last close */
+static int uio_hv_vmbus_release(struct uio_info *info, struct inode *inode)
+{
+ struct uio_hv_vmbus_dev *pdata = container_of(info, struct uio_hv_vmbus_dev, info);
+ struct hv_device *hv_dev = pdata->device;
+ struct vmbus_channel *channel = hv_dev->channel;
+
+ sysfs_remove_bin_file(&channel->kobj, &ring_buffer_bin_attr);
+
+ if (atomic_dec_and_test(&pdata->refcnt))
+ vmbus_close(channel);
+
+ return 0;
+}
+
+static ssize_t ring_size_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ return scnprintf(buf, PAGE_SIZE, "%d\n", ring_size);
+}
+
+static ssize_t ring_size_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ unsigned int val;
+
+ if (kstrtouint(buf, 0, &val) < 0)
+ return -EINVAL;
+
+ if (val < HV_HYP_PAGE_SIZE)
+ return -EINVAL;
+
+ ring_size = val;
+
+ return count;
+}
+static DEVICE_ATTR_RW(ring_size);
+
+static int uio_hv_vmbus_probe(struct hv_device *dev, const struct hv_vmbus_device_id *dev_id)
+{
+ struct uio_hv_vmbus_dev *pdata;
+ int ret = 0;
+ char *name = NULL;
+
+ pdata = devm_kzalloc(&dev->device, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ name = kasprintf(GFP_KERNEL, "%pUl", &dev->dev_instance);
+
+ /* Fill general uio info */
+ pdata->info.name = name; /* /sys/class/uio/uioX/name */
+ pdata->info.version = DRIVER_VERSION;
+ pdata->info.irqcontrol = uio_hv_vmbus_irqcontrol;
+ pdata->info.open = uio_hv_vmbus_open;
+ pdata->info.release = uio_hv_vmbus_release;
+ pdata->info.irq = UIO_IRQ_CUSTOM;
+ pdata->info.priv = pdata;
+ pdata->device = dev;
+
+ ret = uio_register_device(&dev->device, &pdata->info);
+ if (ret) {
+ dev_err(&dev->device, "uio_hv_vmbus register failed\n");
+ goto fail;
+ }
+
+ ret = sysfs_create_file(&dev->device.kobj, &dev_attr_ring_size.attr);
+ if (ret)
+ dev_notice(&dev->device, "sysfs create ring size file failed; %d\n", ret);
+
+ hv_set_drvdata(dev, pdata);
+ return 0;
+
+fail:
+ kfree(pdata);
+ return ret;
+}
+
+static void
+uio_hv_vmbus_remove(struct hv_device *dev)
+{
+ struct uio_hv_vmbus_dev *pdata = hv_get_drvdata(dev);
+
+ sysfs_remove_file(&dev->device.kobj, &dev_attr_ring_size.attr);
+ if (pdata)
+ uio_unregister_device(&pdata->info);
+}
+
+static struct hv_driver uio_hv_vmbus_drv = {
+ .name = "uio_hv_vmbus_client",
+ .id_table = NULL, /* only dynamic id's */
+ .probe = uio_hv_vmbus_probe,
+ .remove = uio_hv_vmbus_remove,
+};
+
+static int __init uio_hv_vmbus_init(void)
+{
+ return vmbus_driver_register(&uio_hv_vmbus_drv);
+}
+
+static void __exit uio_hv_vmbus_exit(void)
+{
+ vmbus_driver_unregister(&uio_hv_vmbus_drv);
+}
+
+module_init(uio_hv_vmbus_init);
+module_exit(uio_hv_vmbus_exit);
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
--
2.34.1
Re: [PATCH 1/5] uio: Add hv_vmbus_client driver [ In reply to ]
Hi Saurabh,

kernel test robot noticed the following build warnings:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.4-rc4 next-20230602]
[.If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Saurabh-Sengar/uio-Add-hv_vmbus_client-driver/20230602-160029
base: char-misc/char-misc-testing
patch link: https://lore.kernel.org/r/1685692629-31351-2-git-send-email-ssengar%40linux.microsoft.com
patch subject: [PATCH 1/5] uio: Add hv_vmbus_client driver
config: i386-randconfig-c001-20230604 (https://download.01.org/0day-ci/archive/20230604/202306040910.13oLufQd-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306040910.13oLufQd-lkp@intel.com/

cocci warnings: (new ones prefixed by >>)
>> drivers/uio/uio_hv_vmbus_client.c:195:1-6: WARNING: invalid free of devm_ allocated data

vim +195 drivers/uio/uio_hv_vmbus_client.c

158
159 static int uio_hv_vmbus_probe(struct hv_device *dev, const struct hv_vmbus_device_id *dev_id)
160 {
161 struct uio_hv_vmbus_dev *pdata;
162 int ret = 0;
163 char *name = NULL;
164
165 pdata = devm_kzalloc(&dev->device, sizeof(*pdata), GFP_KERNEL);
166 if (!pdata)
167 return -ENOMEM;
168
169 name = kasprintf(GFP_KERNEL, "%pUl", &dev->dev_instance);
170
171 /* Fill general uio info */
172 pdata->info.name = name; /* /sys/class/uio/uioX/name */
173 pdata->info.version = DRIVER_VERSION;
174 pdata->info.irqcontrol = uio_hv_vmbus_irqcontrol;
175 pdata->info.open = uio_hv_vmbus_open;
176 pdata->info.release = uio_hv_vmbus_release;
177 pdata->info.irq = UIO_IRQ_CUSTOM;
178 pdata->info.priv = pdata;
179 pdata->device = dev;
180
181 ret = uio_register_device(&dev->device, &pdata->info);
182 if (ret) {
183 dev_err(&dev->device, "uio_hv_vmbus register failed\n");
184 goto fail;
185 }
186
187 ret = sysfs_create_file(&dev->device.kobj, &dev_attr_ring_size.attr);
188 if (ret)
189 dev_notice(&dev->device, "sysfs create ring size file failed; %d\n", ret);
190
191 hv_set_drvdata(dev, pdata);
192 return 0;
193
194 fail:
> 195 kfree(pdata);
196 return ret;
197 }
198

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH 1/5] uio: Add hv_vmbus_client driver [ In reply to ]
On Fri, Jun 02, 2023 at 12:57:05AM -0700, Saurabh Sengar wrote:
> + * Since the driver does not declare any device ids, you must allocate
> + * id and bind the device to the driver yourself. For example:
> + * driverctl -b vmbus set-override <dev uuid> uio_hv_vmbus_client

Shouldn't this be documented somewhere?

> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/uio_driver.h>
> +#include <linux/hyperv.h>
> +#include <linux/vmalloc.h>
> +#include <linux/sysfs.h>
> +
> +#define DRIVER_VERSION "0.0.1"

Why this number? Why not "1"?

The whole "driver version" thing doesn't really make sense here, we
should just drop it from the uio later, right?

> +#define DRIVER_AUTHOR "Saurabh Sengar <ssengar@microsoft.com>"
> +#define DRIVER_DESC "Generic UIO driver for low speed VMBus devices"
> +
> +#define DEFAULT_HV_RING_SIZE VMBUS_RING_SIZE(3 * HV_HYP_PAGE_SIZE)
> +static int ring_size = DEFAULT_HV_RING_SIZE;
> +
> +struct uio_hv_vmbus_dev {
> + struct uio_info info;
> + struct hv_device *device;
> + atomic_t refcnt;

Why is this refcnt needed?

Have you seen the other threads about how attempting to block multiple
opens in UIO drivers really does not work at all? Please drop all of
that logic, it is not correct.


> +static ssize_t ring_size_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + return scnprintf(buf, PAGE_SIZE, "%d\n", ring_size);

Did you use checkpatch?

It should have said to use sysfs_emit(), right?

> + ret = sysfs_create_file(&dev->device.kobj, &dev_attr_ring_size.attr);

If you ever have to use a sysfs_* call in a driver, that's a huge hint
something is wrong. Please fix this to not race with userspace and
loose.

> + if (ret)
> + dev_notice(&dev->device, "sysfs create ring size file failed; %d\n", ret);

Why not just use dev_err() for this and other errors? Why "notice"?

> +MODULE_VERSION(DRIVER_VERSION);

This means nothing, please drop.

thanks,

greg k-h
RE: [EXTERNAL] Re: [PATCH 1/5] uio: Add hv_vmbus_client driver [ In reply to ]
> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Sunday, June 4, 2023 12:40 PM
> To: Saurabh Sengar <ssengar@linux.microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <decui@microsoft.com>; Michael Kelley (LINUX) <mikelley@microsoft.com>;
> linux-kernel@vger.kernel.org; linux-hyperv@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH 1/5] uio: Add hv_vmbus_client driver
>
> On Fri, Jun 02, 2023 at 12:57:05AM -0700, Saurabh Sengar wrote:
> > + * Since the driver does not declare any device ids, you must
> > + allocate
> > + * id and bind the device to the driver yourself. For example:
> > + * driverctl -b vmbus set-override <dev uuid> uio_hv_vmbus_client
>
> Shouldn't this be documented somewhere?

I will update it in Documentation/driver-api/uio-howto.rst.

>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/uio_driver.h>
> > +#include <linux/hyperv.h>
> > +#include <linux/vmalloc.h>
> > +#include <linux/sysfs.h>
> > +
> > +#define DRIVER_VERSION "0.0.1"
>
> Why this number? Why not "1"?
>
> The whole "driver version" thing doesn't really make sense here, we should
> just drop it from the uio later, right?

will remove in V2.

>
> > +#define DRIVER_AUTHOR "Saurabh Sengar <ssengar@microsoft.com>"
> > +#define DRIVER_DESC "Generic UIO driver for low speed VMBus
> devices"
> > +
> > +#define DEFAULT_HV_RING_SIZE VMBUS_RING_SIZE(3 *
> HV_HYP_PAGE_SIZE)
> > +static int ring_size = DEFAULT_HV_RING_SIZE;
> > +
> > +struct uio_hv_vmbus_dev {
> > + struct uio_info info;
> > + struct hv_device *device;
> > + atomic_t refcnt;
>
> Why is this refcnt needed?
>
> Have you seen the other threads about how attempting to block multiple
> opens in UIO drivers really does not work at all? Please drop all of that logic,
> it is not correct.
>

Will remove.

>
> > +static ssize_t ring_size_show(struct device *dev, struct device_attribute
> *attr,
> > + char *buf)
> > +{
> > + return scnprintf(buf, PAGE_SIZE, "%d\n", ring_size);
>
> Did you use checkpatch?
>
> It should have said to use sysfs_emit(), right?

Yes, I am using "--strict" switch for checkpatch.pl, still didn't get this warning.
However, I will use sysfs_emit() in V2.

>
> > + ret = sysfs_create_file(&dev->device.kobj,
> > +&dev_attr_ring_size.attr);
>
> If you ever have to use a sysfs_* call in a driver, that's a huge hint something
> is wrong. Please fix this to not race with userspace and loose.

Thanks for pointing this out, will look into this.

>
> > + if (ret)
> > + dev_notice(&dev->device, "sysfs create ring size file failed;
> > +%d\n", ret);
>
> Why not just use dev_err() for this and other errors? Why "notice"?

Will fix.

>
> > +MODULE_VERSION(DRIVER_VERSION);
>
> This means nothing, please drop.

OK.

Thanks for review.
- Saurabh

>
> thanks,
>
> greg k-h