Skip to contentSkip to conversation

Miscellaneous fixes/enhancements

PATCH In review [email protected] 4 participants 12 messages Aug 10, 2018 – Aug 14, 2018

Subsystems (3)
Stable branch Hyper-V/Azure core and drivers Userspace I/O (UIO)
lore
Share & tools
lore

[email protected]

author
#0 lore raw

cover Miscellaneous fixes/enhancements

From: "K. Y. Srinivasan" <[email protected]>

Miscellaneous fixes/enhancements.

K. Y. Srinivasan (1):
Tools: hv: Fix a bug in the key delete code

Michael Kelley (1):
Drivers: hv: vmbus: Fix synic per-cpu context initialization

Stephen Hemminger (3):
vmbus: add driver_override support
uio_hv_generic: increase size of receive and send buffers
uio_hv_generic: drop #ifdef DEBUG

 Documentation/ABI/testing/sysfs-bus-vmbus |  21 ++++
 drivers/hv/hv.c                           |  15 ++-
 drivers/hv/vmbus_drv.c                    | 115 ++++++++++++++++++----
 drivers/uio/uio_hv_generic.c              |   7 +-
 include/linux/hyperv.h                    |   1 +
 tools/hv/hv_kvp_daemon.c                  |   2 +-
 6 files changed, 134 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-vmbus
Signature · 1 line
2.18.0
author
#1 lore raw

PATCH 1/5 Tools: hv: Fix a bug in the key delete code

From: "K. Y. Srinivasan" <[email protected]>

Fix a bug in the key delete code - the num_records range from 0 to num_records-1.

Signed-off-by K. Y. Srinivasan <[email protected]>
Reported-by David Binderman <[email protected]>
Cc <[email protected]>
---
 tools/hv/hv_kvp_daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/hv/hv_kvp_daemon.c +1 -1
index dbf6e8bd98ba..bbb2a8ef367c 100644
@@ -286,7 +286,7 @@ static int kvp_key_delete(int pool, const __u8 *key, int key_size)
 		 * Found a match; just move the remaining
 		 * entries up.
 		 */
-		if (i == num_records) {
+		if (i == (num_records - 1)) {
 			kvp_file_info[pool].num_records--;
 			kvp_update_file(pool);
 			return 0;
Signature · 1 line
2.18.0
author
#2 lore raw

PATCH 2/5 vmbus: add driver_override support

From: Stephen Hemminger <[email protected]>

Add support for overriding the default driver for a VMBus device in the same way that it can be done for PCI devices. This patch adds the /sys/bus/vmbus/devices/.../driver_override file and the logic for matching.

This is used by driverctl tool to do driver override.
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.com%2Fdriverctl%2Fdriverctl&amp;data=02%7C01%7Ckys%40microsoft.com%7C42e803feb2c544ef6ea908d5fd538878%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636693457619960040&amp;sdata=kEyYHRIjNZCk%2B37moCSqbrZL426YccNQrsWpENcrZdw%3D&amp;reserved=0

Signed-off-by Stephen Hemminger <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 Documentation/ABI/testing/sysfs-bus-vmbus |  21 ++++
 drivers/hv/vmbus_drv.c                    | 115 ++++++++++++++++++----
 include/linux/hyperv.h                    |   1 +
 3 files changed, 118 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-vmbus
Documentation/ABI/testing/sysfs-bus-vmbus new file +21 -0
new file mode 100644
index 000000000000..91e6c065973c
@@ -0,0 +1,21 @@
+What:		/sys/bus/vmbus/devices/.../driver_override
+Date:		August 2019
+Contact:	Stephen Hemminger <[email protected]>
+Description:
+		This file allows the driver for a device to be specified which
+		will override standard static and dynamic ID matching.  When
+		specified, only a driver with a name matching the value written
+		to driver_override will have an opportunity to bind to the
+		device.  The override is specified by writing a string to the
+		driver_override file (echo uio_hv_generic > driver_override) and
+		may be cleared with an empty string (echo > driver_override).
+		This returns the device to standard matching rules binding.
+		Writing to driver_override does not automatically unbind the
+		device from its current driver or make any attempt to
+		automatically load the specified driver.  If no driver with a
+		matching name is currently loaded in the kernel, the device
+		will not bind to any driver.  This also allows devices to
+		opt-out of driver binding using a driver_override name such as
+		"none".  Only a single driver may be specified in the override,
+		there is no support for parsing delimiters.
+
drivers/hv/vmbus_drv.c +96 -19

164 lines not on this page · raw message

include/linux/hyperv.h +1 -0
index efda23cf32c7..2c3798bcb01c 100644
@@ -1125,6 +1125,7 @@ struct hv_device {
 	u16 device_id;
 
 	struct device device;
+	char *driver_override; /* Driver name to force a match */
 
 	struct vmbus_channel *channel;
 	struct kset	     *channels_kset;
Signature · 1 line
2.18.0
#3 lore raw

RE: [PATCH 2/5] vmbus: add driver_override support

From: [email protected] <[email protected]> Sent: Friday, August 10, 2018 4:06 PM

Quoted 21 lines

From: Stephen Hemminger <[email protected]>

Add support for overriding the default driver for a VMBus device in the same way that it can be done for PCI devices. This patch adds the /sys/bus/vmbus/devices/.../driver_override file and the logic for matching.

This is used by driverctl tool to do driver override.
https://gitlab.com/driverctl/driverctl

Signed-off-by Stephen Hemminger <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>

---

drivers/hv/vmbus_drv.c +0 -0
index b1b548a21f91..e6d8fdac6d8b 100644
@@ -498,6 +498,54 @@ static ssize_t device_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(device);

drivers/hv/vmbus_drv.c static ssize_t device_show(struct device *dev, · line 510

Quoted 10 lines
drivers/hv/vmbus_drv.c +10 -0
+static ssize_t driver_override_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct hv_device *hv_dev = device_to_hv_device(dev);
+	char *driver_override, *old, *cp;
+
+	/* We need to keep extra room for a newline */
+	if (count >= (PAGE_SIZE - 1))
+		return -EINVAL;

Does 'count' actually have a relationship to PAGE_SIZE, or is PAGE_SIZE just used as an arbitrary size limit? I'm wondering what happens on ARM64 with a 64K page size, for example. If it's just arbitrary, coding such a constant would be better.

Quoted 4 lines
drivers/hv/vmbus_drv.c +4 -0
+/*
+ * Return a matching hv_vmbus_device_id pointer.
+ * If there is no match, return NULL.
+ */

drivers/hv/vmbus_drv.c static const struct hv_vmbus_device_id *hv_vmbus_get_id(struct hv_driver *drv, · line 663

Quoted 10 lines
drivers/hv/vmbus_drv.c +8 -1
+static const struct hv_vmbus_device_id *hv_vmbus_get_id(struct hv_driver *drv,
+							struct hv_device *dev)
+{
+	const uuid_le *guid = &dev->dev_type;
+	const struct hv_vmbus_device_id *id;
 
-	return NULL;
+	/* When driver_override is set, only bind to the matching driver */
+	if (dev->driver_override && strcmp(dev->driver_override, drv->name))
+		return NULL;

This function needs to be covered by the device lock, so that dev->driver_override can't be set to NULL and the memory freed during the above 'if' statement. When called from vmbus_probe(), the device lock is held, so it's good. But when called from vmbus_match(), the device lock may not be held: consider the path __driver_attach() -> driver_match_device() -> vmbus_match().

Michael

maintainer
#4 lore raw

Re: [PATCH 2/5] vmbus: add driver_override support

Quoted 23 lines · Michael Kelley (EOSG)

From: [email protected] <[email protected]> Sent: Friday, August 10, 2018 4:06 PM

Quoted 21 lines

From: Stephen Hemminger <[email protected]>

Add support for overriding the default driver for a VMBus device in the same way that it can be done for PCI devices. This patch adds the /sys/bus/vmbus/devices/.../driver_override file and the logic for matching.

This is used by driverctl tool to do driver override.
https://gitlab.com/driverctl/driverctl

Signed-off-by Stephen Hemminger <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>

---

drivers/hv/vmbus_drv.c +0 -0
index b1b548a21f91..e6d8fdac6d8b 100644
@@ -498,6 +498,54 @@ static ssize_t device_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(device);
Quoted 16 lines

drivers/hv/vmbus_drv.c static ssize_t device_show(struct device *dev, · line 510

Quoted 10 lines
drivers/hv/vmbus_drv.c +10 -0
+static ssize_t driver_override_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct hv_device *hv_dev = device_to_hv_device(dev);
+	char *driver_override, *old, *cp;
+
+	/* We need to keep extra room for a newline */
+	if (count >= (PAGE_SIZE - 1))
+		return -EINVAL;

Does 'count' actually have a relationship to PAGE_SIZE, or is PAGE_SIZE just used as an arbitrary size limit? I'm wondering what happens on ARM64 with a 64K page size, for example. If it's just arbitrary, coding such a constant would be better.

sysfs buffers are PAGE_SIZE big.

#5 lore raw

Re: [PATCH 2/5] vmbus: add driver_override support

In reply to Michael Kelley (EOSG)
Quoted 23 lines · On Mon, 13 Aug 2018 19:30:50 +0000 "Michael Kelley (EOSG)"…

From: [email protected] <[email protected]> Sent: Friday, August 10, 2018 4:06 PM

Quoted 21 lines

From: Stephen Hemminger <[email protected]>

Add support for overriding the default driver for a VMBus device in the same way that it can be done for PCI devices. This patch adds the /sys/bus/vmbus/devices/.../driver_override file and the logic for matching.

This is used by driverctl tool to do driver override.
https://gitlab.com/driverctl/driverctl

Signed-off-by Stephen Hemminger <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>

---

drivers/hv/vmbus_drv.c +0 -0
index b1b548a21f91..e6d8fdac6d8b 100644
@@ -498,6 +498,54 @@ static ssize_t device_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(device);
Quoted 16 lines

drivers/hv/vmbus_drv.c static ssize_t device_show(struct device *dev, · line 510

Quoted 10 lines
drivers/hv/vmbus_drv.c +10 -0
+static ssize_t driver_override_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct hv_device *hv_dev = device_to_hv_device(dev);
+	char *driver_override, *old, *cp;
+
+	/* We need to keep extra room for a newline */
+	if (count >= (PAGE_SIZE - 1))
+		return -EINVAL;

Does 'count' actually have a relationship to PAGE_SIZE, or is PAGE_SIZE just used as an arbitrary size limit? I'm wondering what happens on ARM64 with a 64K page size, for example. If it's just arbitrary, coding such a constant would be better.

This comes from original code how sysfs works. Sysfs uses PAGE_SIZE for string buffers on store. This code snippet was cloned from PCI version of same thing.

Quoted 10 lines
Quoted 10 lines
drivers/hv/vmbus_drv.c +9 -0
+/*
+ * Return a matching hv_vmbus_device_id pointer.
+ * If there is no match, return NULL.
+ */
+static const struct hv_vmbus_device_id *hv_vmbus_get_id(struct hv_driver *drv,
+							struct hv_device *dev)
+{
+	const uuid_le *guid = &dev->dev_type;
+	const struct hv_vmbus_device_id *id;
Quoted 11 lines

drivers/hv/vmbus_drv.c static const struct hv_vmbus_device_id *hv_vmbus_get_id(struct hv_driver *drv, · line 663

Quoted 4 lines
drivers/hv/vmbus_drv.c +3 -1
-	return NULL;
+	/* When driver_override is set, only bind to the matching driver */
+	if (dev->driver_override && strcmp(dev->driver_override, drv->name))
+		return NULL;

This function needs to be covered by the device lock, so that dev->driver_override can't be set to NULL and the memory freed during the above 'if' statement. When called from vmbus_probe(), the device lock is held, so it's good. But when called from vmbus_match(), the device lock may not be held: consider the path __driver_attach() -> driver_match_device() -> vmbus_match().

The patch clones existing locking model from PCI.
So either both are broken, or somehow vmbus is behaving differently. I will investigate.

#6 lore raw

Re: [PATCH 2/5] vmbus: add driver_override support

In reply to Michael Kelley (EOSG)
Quoted 10 lines · On Mon, 13 Aug 2018 19:30:50 +0000 "Michael Kelley (EOSG)"…
Quoted 10 lines
drivers/hv/vmbus_drv.c +9 -0
+/*
+ * Return a matching hv_vmbus_device_id pointer.
+ * If there is no match, return NULL.
+ */
+static const struct hv_vmbus_device_id *hv_vmbus_get_id(struct hv_driver *drv,
+							struct hv_device *dev)
+{
+	const uuid_le *guid = &dev->dev_type;
+	const struct hv_vmbus_device_id *id;
Quoted 11 lines

drivers/hv/vmbus_drv.c static const struct hv_vmbus_device_id *hv_vmbus_get_id(struct hv_driver *drv, · line 663

Quoted 4 lines
drivers/hv/vmbus_drv.c +3 -1
-	return NULL;
+	/* When driver_override is set, only bind to the matching driver */
+	if (dev->driver_override && strcmp(dev->driver_override, drv->name))
+		return NULL;

This function needs to be covered by the device lock, so that dev->driver_override can't be set to NULL and the memory freed during the above 'if' statement. When called from vmbus_probe(), the device lock is held, so it's good. But when called from vmbus_match(), the device lock may not be held: consider the path __driver_attach() -> driver_match_device() -> vmbus_match().

The function hv_vmbus_get_id is called from that path. i.e. __device_attach -> driver-match_device -> vmbus_match. and __device_attach always does:

	device_lock(dev);

The code in driver _override_store uses the same device_lock when storing the new value.

This is same locking as is done in pci-sysfs.c

#7 lore raw

RE: [PATCH 2/5] vmbus: add driver_override support

From: Stephen Hemminger <[email protected]> Sent: Tuesday, August 14, 2018 9:35 AM

Quoted 25 lines
Quoted 10 lines · On Mon, 13 Aug 2018 19:30:50 +0000 "Michael Kelley (EOSG)"…
Quoted 10 lines
drivers/hv/vmbus_drv.c +9 -0
+/*
+ * Return a matching hv_vmbus_device_id pointer.
+ * If there is no match, return NULL.
+ */
+static const struct hv_vmbus_device_id *hv_vmbus_get_id(struct hv_driver *drv,
+							struct hv_device *dev)
+{
+	const uuid_le *guid = &dev->dev_type;
+	const struct hv_vmbus_device_id *id;
Quoted 11 lines

drivers/hv/vmbus_drv.c static const struct hv_vmbus_device_id *hv_vmbus_get_id(struct hv_driver *drv, · line 663

Quoted 4 lines
drivers/hv/vmbus_drv.c +3 -1
-	return NULL;
+	/* When driver_override is set, only bind to the matching driver */
+	if (dev->driver_override && strcmp(dev->driver_override, drv->name))
+		return NULL;

This function needs to be covered by the device lock, so that dev->driver_override can't be set to NULL and the memory freed during the above 'if' statement. When called from vmbus_probe(), the device lock is held, so it's good. But when called from vmbus_match(), the device lock may not be held: consider the path __driver_attach() -> driver_match_device() -> vmbus_match().

Quoted 4 lines

The function hv_vmbus_get_id is called from that path. i.e. __device_attach -> driver-match_device -> vmbus_match. and __device_attach always does:

	device_lock(dev);

Agreed. The __device_attach() path holds the device lock and all is good. But the __driver_attach() path does not hold the device lock when the match function is called, leaving the code open to a potential race. Same problem could happen in the pci subsystem, so the issue is more generic and probably should be evaluated and dealt with separately.

Michael

Quoted 5 lines

The code in driver _override_store uses the same device_lock when storing the new value.

This is same locking as is done in pci-sysfs.c

author
#8 lore raw

PATCH 3/5 uio_hv_generic: increase size of receive and send buffers

In reply to [email protected]

From: Stephen Hemminger <[email protected]>

When using DPDK there is significant performance boost by using the largest possible send and receive buffer area.

Unfortunately, with UIO model there is not a good way to configure this at run time. But it is okay to have a bigger buffer available even if application only decides to use a smaller piece of it.

Signed-off-by Stephen Hemminger <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/uio/uio_hv_generic.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/uio/uio_hv_generic.c +3 -3
index c690d100adcd..35678d08d9d8 100644
@@ -35,13 +35,13 @@
 
 #include "../hv/hyperv_vmbus.h"
 
-#define DRIVER_VERSION	"0.02.0"
+#define DRIVER_VERSION	"0.02.1"
 #define DRIVER_AUTHOR	"Stephen Hemminger <sthemmin at microsoft.com>"
 #define DRIVER_DESC	"Generic UIO driver for VMBus devices"
 
 #define HV_RING_SIZE	 512	/* pages */
-#define SEND_BUFFER_SIZE (15 * 1024 * 1024)
-#define RECV_BUFFER_SIZE (15 * 1024 * 1024)
+#define SEND_BUFFER_SIZE (16 * 1024 * 1024)
+#define RECV_BUFFER_SIZE (31 * 1024 * 1024)
 
 /*
  * List of resources to be mapped to user space
Signature · 1 line
2.18.0
author
#9 lore raw

PATCH 4/5 uio_hv_generic: drop #ifdef DEBUG

In reply to [email protected]

From: Stephen Hemminger <[email protected]>

DEBUG is leftover from the development phase, remove it.

Signed-off-by Stephen Hemminger <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/uio/uio_hv_generic.c | 1 -
 1 file changed, 1 deletion(-)
drivers/uio/uio_hv_generic.c +0 -1
index 35678d08d9d8..ab0c0e7e8a44 100644
@@ -19,7 +19,6 @@
  * # echo -n "ed963694-e847-4b2a-85af-bc9cfc11d6f3" \
  *    > /sys/bus/vmbus/drivers/uio_hv_generic/bind
  */
-#define DEBUG 1
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/device.h>
Signature · 1 line
2.18.0
author
#10 lore raw

PATCH 5/5 Drivers: hv: vmbus: Fix synic per-cpu context initialization

In reply to [email protected]

From: Michael Kelley <[email protected]>

If hv_synic_alloc() errors out, the state of the per-cpu context for some CPUs is unknown since the zero'ing is done as each CPU is iterated over. In such case, hv_synic_cleanup() may try to free memory based on uninitialized values. Fix this by zero'ing the per-cpu context for all CPUs before doing any memory allocations that might fail.

Signed-off-by Michael Kelley <[email protected]>
Reported-by Dan Carpenter <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/hv.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
drivers/hv/hv.c +12 -3
index 748a1c4172a6..332d7c34be5c 100644
@@ -189,6 +189,17 @@ static void hv_init_clockevent_device(struct clock_event_device *dev, int cpu)
 int hv_synic_alloc(void)
 {
 	int cpu;
+	struct hv_per_cpu_context *hv_cpu;
+
+	/*
+	 * First, zero all per-cpu memory areas so hv_synic_free() can
+	 * detect what memory has been allocated and cleanup properly
+	 * after any failures.
+	 */
+	for_each_present_cpu(cpu) {
+		hv_cpu = per_cpu_ptr(hv_context.cpu_context, cpu);
+		memset(hv_cpu, 0, sizeof(*hv_cpu));
+	}
 
 	hv_context.hv_numa_map = kcalloc(nr_node_ids, sizeof(struct cpumask),
 					 GFP_KERNEL);

@@ -198,10 +209,8 @@ int hv_synic_alloc(void)
 	}
 
 	for_each_present_cpu(cpu) {
-		struct hv_per_cpu_context *hv_cpu
-			= per_cpu_ptr(hv_context.cpu_context, cpu);
+		hv_cpu = per_cpu_ptr(hv_context.cpu_context, cpu);
 
-		memset(hv_cpu, 0, sizeof(*hv_cpu));
 		tasklet_init(&hv_cpu->msg_dpc,
 			     vmbus_on_msg_dpc, (unsigned long) hv_cpu);
Signature · 1 line
2.18.0
Reviewed-by Michael Kelley on v1 1/5
#11 lore raw

RE: [PATCH 1/5] Tools: hv: Fix a bug in the key delete code

In reply to [email protected]

From: [email protected] <[email protected]> Sent: Friday, August 10, 2018 4:06 PM

Quoted 8 lines

Fix a bug in the key delete code - the num_records range from 0 to num_records-1.

Signed-off-by K. Y. Srinivasan <[email protected]>
Reported-by David Binderman <[email protected]>
Cc <[email protected]>

---

Reviewed-by Michael Kelley <[email protected]>