Skip to contentSkip to conversation

Drivers: hv: vmbus: Miscellaneous fixes and cleanup

PATCH In review K. Y. Srinivasan 3 participants 15 messages Nov 12, 2015

Subsystems (2)
Kbuild Hyper-V/Azure core and drivers
lore
Share & tools
lore

K. Y. Srinivasan

author
#0 lore raw

v2 cover Drivers: hv: vmbus: Miscellaneous fixes and cleanup

Miscellaneous fixes and cleanup.
Changes from V1:
Fixed the typo in patch number 3.
Included the patch tools/hv: Use include/uapi with __EXPORTED_HEADERS__

Dexuan Cui (5):
Drivers: hv: vmbus: serialize process_chn_event() and
vmbus_close_internal()
Drivers: hv: vmbus: do sanity check of channel state in
vmbus_close_internal()
Drivers: hv: vmbus: fix rescind-offer handling for device without a
driver
Drivers: hv: vmbus: release relid on error in vmbus_process_offer()
Drivers: hv: vmbus: channge vmbus_connection.channel_lock to mutex

Jake Oshins (1):
drivers:hv: Allow for MMIO claims that span ACPI _CRS records

K. Y. Srinivasan (4):
Drivers: hv: vmbus: Use uuid_le type consistently
Drivers: hv: vmbus: Use uuid_le_cmp() for comparing GUIDs
Drivers: hv: vmbus: Get rid of the unused macro
Drivers: hv: vmbus: Get rid of the unused irq variable

Kamal Mostafa (1):
tools/hv: Use include/uapi with __EXPORTED_HEADERS__

Olaf Hering (1):
Drivers: hv: vss: run only on supported host versions

 drivers/hv/channel.c            |   39 ++++++++++++---
 drivers/hv/channel_mgmt.c       |   42 ++++++++++-------
 drivers/hv/connection.c         |    7 +--
 drivers/hv/hv_snapshot.c        |    5 ++
 drivers/hv/hyperv_vmbus.h       |    2 +-
 drivers/hv/vmbus_drv.c          |   61 +++++++++++------------
 include/linux/hyperv.h          |  101 ++++++++++++---------------------------
 include/linux/mod_devicetable.h |    2 +-
 scripts/mod/file2alias.c        |    2 +-
 tools/hv/Makefile               |    2 +
 10 files changed, 128 insertions(+), 135 deletions(-)
Signature · 1 line
1.7.4.1
author
#1 lore raw

PATCH v2 1/12 Drivers: hv: vss: run only on supported host versions

From: Olaf Hering <[email protected]>

The Backup integration service on WS2012 has appearently trouble to negotiate with a guest which does not support the provided util version. Currently the VSS driver supports only version 5/0. A WS2012 offers only version 1/x and 3/x, and vmbus_prep_negotiate_resp correctly returns an empty icframe_vercnt/icmsg_vercnt. But the host ignores that and continues to send ICMSGTYPE_NEGOTIATE messages. The result are weird errors during boot and general misbehaviour.

Check the Windows version to work around the host bug, skip hv_vss_init on WS2012 and older.

Signed-off-by Olaf Hering <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/hv_snapshot.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/hv/hv_snapshot.c +5 -0
index a548ae4..81882d4 100644
@@ -331,6 +331,11 @@ static void vss_on_reset(void)
 int
 hv_vss_init(struct hv_util_service *srv)
 {
+	if (vmbus_proto_version < VERSION_WIN8_1) {
+		pr_warn("Integration service 'Backup (volume snapshot)'"
+			" not supported on this host version.\n");
+		return -ENOTSUPP;
+	}
 	recv_buffer = srv->recv_buffer;
 
 	/*
Signature · 1 line
1.7.4.1
author
#2 lore raw

PATCH v2 2/12 Drivers: hv: vmbus: Use uuid_le type consistently

Consistently use uuid_le type in the Hyper-V driver code.

Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/channel_mgmt.c       |    2 +-
 drivers/hv/vmbus_drv.c          |   10 ++--
 include/linux/hyperv.h          |   92 +++++++++++++-------------------------
 include/linux/mod_devicetable.h |    2 +-
 scripts/mod/file2alias.c        |    2 +-
 5 files changed, 40 insertions(+), 68 deletions(-)
drivers/hv/channel_mgmt.c +1 -1
index a77646b..38470aa 100644
@@ -408,7 +408,7 @@ static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_gui
 	struct cpumask *alloced_mask;
 
 	for (i = IDE; i < MAX_PERF_CHN; i++) {
-		if (!memcmp(type_guid->b, hp_devs[i].guid,
+		if (!memcmp(type_guid->b, &hp_devs[i].guid,
 				 sizeof(uuid_le))) {
 			perf_chn = true;
 			break;
drivers/hv/vmbus_drv.c +5 -5
index c01b689..7078b5f 100644
@@ -531,7 +531,7 @@ static int vmbus_uevent(struct device *device, struct kobj_uevent_env *env)
 
 static const uuid_le null_guid;
 
-static inline bool is_null_guid(const __u8 *guid)
+static inline bool is_null_guid(const uuid_le *guid)
 {
 	if (memcmp(guid, &null_guid, sizeof(uuid_le)))
 		return false;

@@ -544,9 +544,9 @@ static inline bool is_null_guid(const __u8 *guid)
  */
 static const struct hv_vmbus_device_id *hv_vmbus_get_id(
 					const struct hv_vmbus_device_id *id,
-					const __u8 *guid)
+					const uuid_le *guid)
 {
-	for (; !is_null_guid(id->guid); id++)
+	for (; !is_null_guid(&id->guid); id++)
 		if (!memcmp(&id->guid, guid, sizeof(uuid_le)))
 			return id;
 

@@ -563,7 +563,7 @@ static int vmbus_match(struct device *device, struct device_driver *driver)
 	struct hv_driver *drv = drv_to_hv_drv(driver);
 	struct hv_device *hv_dev = device_to_hv_device(device);
 
-	if (hv_vmbus_get_id(drv->id_table, hv_dev->dev_type.b))
+	if (hv_vmbus_get_id(drv->id_table, &hv_dev->dev_type))
 		return 1;
 
 	return 0;

@@ -580,7 +580,7 @@ static int vmbus_probe(struct device *child_device)
 	struct hv_device *dev = device_to_hv_device(child_device);
 	const struct hv_vmbus_device_id *dev_id;
 
-	dev_id = hv_vmbus_get_id(drv->id_table, dev->dev_type.b);
+	dev_id = hv_vmbus_get_id(drv->id_table, &dev->dev_type);
 	if (drv->probe) {
 		ret = drv->probe(dev, dev_id);
 		if (ret != 0)
include/linux/hyperv.h +32 -60

186 lines not on this page · raw message

include/linux/mod_devicetable.h +1 -1
index 6975cbf..10f83a4 100644
@@ -396,7 +396,7 @@ struct virtio_device_id {
  * For Hyper-V devices we use the device guid as the id.
  */
 struct hv_vmbus_device_id {
-	__u8 guid[16];
+	uuid_le guid;
 	kernel_ulong_t driver_data;	/* Data private to the driver */
 };
scripts/mod/file2alias.c +1 -1
index 9bc2cfe..4f62291 100644
@@ -917,7 +917,7 @@ static int do_vmbus_entry(const char *filename, void *symval,
 	char guid_name[(sizeof(*guid) + 1) * 2];
 
 	for (i = 0; i < (sizeof(*guid) * 2); i += 2)
-		sprintf(&guid_name[i], "%02x", TO_NATIVE((*guid)[i/2]));
+		sprintf(&guid_name[i], "%02x", TO_NATIVE((guid->b)[i/2]));
 
 	strcpy(alias, "vmbus:");
 	strcat(alias, guid_name);
Signature · 1 line
1.7.4.1
author
#3 lore raw

PATCH v2 3/12 Drivers: hv: vmbus: Use uuid_le_cmp() for comparing GUIDs

In reply to K. Y. Srinivasan

Use uuid_le_cmp() for comparing GUIDs.

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

---
V2: Fixed the typo in both the commit log and Subject line.

 drivers/hv/channel_mgmt.c |    3 +--
 drivers/hv/vmbus_drv.c    |    4 ++--
 2 files changed, 3 insertions(+), 4 deletions(-)
drivers/hv/channel_mgmt.c +1 -2
index 38470aa..dc4fb0b 100644
@@ -408,8 +408,7 @@ static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_gui
 	struct cpumask *alloced_mask;
 
 	for (i = IDE; i < MAX_PERF_CHN; i++) {
-		if (!memcmp(type_guid->b, &hp_devs[i].guid,
-				 sizeof(uuid_le))) {
+		if (!uuid_le_cmp(*type_guid, hp_devs[i].guid)) {
 			perf_chn = true;
 			break;
 		}
drivers/hv/vmbus_drv.c +2 -2
index 7078b5f..9e0e25c 100644
@@ -533,7 +533,7 @@ static const uuid_le null_guid;
 
 static inline bool is_null_guid(const uuid_le *guid)
 {
-	if (memcmp(guid, &null_guid, sizeof(uuid_le)))
+	if (uuid_le_cmp(*guid, null_guid))
 		return false;
 	return true;
 }

@@ -547,7 +547,7 @@ static const struct hv_vmbus_device_id *hv_vmbus_get_id(
 					const uuid_le *guid)
 {
 	for (; !is_null_guid(&id->guid); id++)
-		if (!memcmp(&id->guid, guid, sizeof(uuid_le)))
+		if (!uuid_le_cmp(id->guid, *guid))
 			return id;
 
 	return NULL;
Signature · 1 line
1.7.4.1
author
#4 lore raw

PATCH v2 4/12 Drivers: hv: vmbus: Get rid of the unused macro

In reply to K. Y. Srinivasan

The macro VMBUS_DEVICE() is unused; get rid of it.

Signed-off-by K. Y. Srinivasan <[email protected]>
---
 include/linux/hyperv.h |   13 -------------
 1 files changed, 0 insertions(+), 13 deletions(-)
include/linux/hyperv.h +0 -13
index ee243a7..437c9c8 100644
@@ -985,19 +985,6 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,
 int vmbus_cpu_number_to_vp_number(int cpu_number);
 u64 hv_do_hypercall(u64 control, void *input, void *output);
 
-/**
- * VMBUS_DEVICE - macro used to describe a specific hyperv vmbus device
- *
- * This macro is used to create a struct hv_vmbus_device_id that matches a
- * specific device.
- */
-#define VMBUS_DEVICE(g0, g1, g2, g3, g4, g5, g6, g7,	\
-		     g8, g9, ga, gb, gc, gd, ge, gf)	\
-	.guid = { g0, g1, g2, g3, g4, g5, g6, g7,	\
-		  g8, g9, ga, gb, gc, gd, ge, gf },
-
-
-
 /*
  * GUID definitions of various offer types - services offered to the guest.
  */
Signature · 1 line
1.7.4.1
author
#5 lore raw

PATCH v2 5/12 Drivers: hv: vmbus: Get rid of the unused irq variable

In reply to K. Y. Srinivasan

The irq we extract from ACPI is not used - we deliver hypervisor interrupts on a special vector. Make the necessary adjustments.

Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/vmbus_drv.c |   16 +++-------------
 1 files changed, 3 insertions(+), 13 deletions(-)
drivers/hv/vmbus_drv.c +3 -13
index 9e0e25c..ab888a1 100644
@@ -47,7 +47,6 @@ static struct acpi_device  *hv_acpi_dev;
 
 static struct tasklet_struct msg_dpc;
 static struct completion probe_event;
-static int irq;
 
 
 static void hyperv_report_panic(struct pt_regs *regs)

@@ -835,10 +834,9 @@ static void vmbus_isr(void)
  * Here, we
  *	- initialize the vmbus driver context
  *	- invoke the vmbus hv main init routine
- *	- get the irq resource
  *	- retrieve the channel offers
  */
-static int vmbus_bus_init(int irq)
+static int vmbus_bus_init(void)
 {
 	int ret;
 

@@ -1033,9 +1031,6 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
 	struct resource **prev_res = NULL;
 
 	switch (res->type) {
-	case ACPI_RESOURCE_TYPE_IRQ:
-		irq = res->data.irq.interrupts[0];
-		return AE_OK;
 
 	/*
 	 * "Address" descriptors are for bus windows. Ignore

@@ -1294,7 +1289,7 @@ static int __init hv_acpi_init(void)
 	init_completion(&probe_event);
 
 	/*
-	 * Get irq resources first.
+	 * Get ACPI resources first.
 	 */
 	ret = acpi_bus_register_driver(&vmbus_acpi_driver);
 

@@ -1307,12 +1302,7 @@ static int __init hv_acpi_init(void)
 		goto cleanup;
 	}
 
-	if (irq <= 0) {
-		ret = -ENODEV;
-		goto cleanup;
-	}
-
-	ret = vmbus_bus_init(irq);
+	ret = vmbus_bus_init();
 	if (ret)
 		goto cleanup;
Signature · 1 line
1.7.4.1
author
#6 lore raw

PATCH v2 6/12 Drivers: hv: vmbus: serialize process_chn_event() and vmbus_close_internal()

In reply to K. Y. Srinivasan

From: Dexuan Cui <[email protected]>

process_chn_event(), running in the tasklet, can race with vmbus_close_internal() in the case of SMP guest, e.g., when the former is accessing channel->inbound.ring_buffer, the latter could be freeing the ring_buffer pages.

To resolve the race, we can serialize them by disabling the tasklet when the latter is running here.

Signed-off-by Dexuan Cui <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/channel.c |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)
drivers/hv/channel.c +19 -2
index c4dcab0..f7f3d5c 100644
@@ -28,6 +28,7 @@
 #include <linux/module.h>
 #include <linux/hyperv.h>
 #include <linux/uio.h>
+#include <linux/interrupt.h>
 
 #include "hyperv_vmbus.h"
 

@@ -496,8 +497,21 @@ static void reset_channel_cb(void *arg)
 static int vmbus_close_internal(struct vmbus_channel *channel)
 {
 	struct vmbus_channel_close_channel *msg;
+	struct tasklet_struct *tasklet;
 	int ret;
 
+	/*
+	 * process_chn_event(), running in the tasklet, can race
+	 * with vmbus_close_internal() in the case of SMP guest, e.g., when
+	 * the former is accessing channel->inbound.ring_buffer, the latter
+	 * could be freeing the ring_buffer pages.
+	 *
+	 * To resolve the race, we can serialize them by disabling the
+	 * tasklet when the latter is running here.
+	 */
+	tasklet = hv_context.event_dpc[channel->target_cpu];
+	tasklet_disable(tasklet);
+
 	channel->state = CHANNEL_OPEN_STATE;
 	channel->sc_creation_callback = NULL;
 	/* Stop callback and cancel the timer asap */

@@ -525,7 +539,7 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
 		 * If we failed to post the close msg,
 		 * it is perhaps better to leak memory.
 		 */
-		return ret;
+		goto out;
 	}
 
 	/* Tear down the gpadl for the channel's ring buffer */

@@ -538,7 +552,7 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
 			 * If we failed to teardown gpadl,
 			 * it is perhaps better to leak memory.
 			 */
-			return ret;
+			goto out;
 		}
 	}
 

@@ -555,6 +569,9 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
 	if (channel->rescind)
 		hv_process_channel_removal(channel,
 					   channel->offermsg.child_relid);
+out:
+	tasklet_enable(tasklet);
+
 	return ret;
 }
Signature · 1 line
1.7.4.1
author
#7 lore raw

PATCH v2 7/12 Drivers: hv: vmbus: do sanity check of channel state in vmbus_close_internal()

In reply to K. Y. Srinivasan

From: Dexuan Cui <[email protected]>

This fixes an incorrect assumption of channel state in the function.

Signed-off-by Dexuan Cui <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/channel.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
drivers/hv/channel.c +12 -0
index f7f3d5c..00e1be7 100644
@@ -512,6 +512,18 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
 	tasklet = hv_context.event_dpc[channel->target_cpu];
 	tasklet_disable(tasklet);
 
+	/*
+	 * In case a device driver's probe() fails (e.g.,
+	 * util_probe() -> vmbus_open() returns -ENOMEM) and the device is
+	 * rescinded later (e.g., we dynamically disble an Integrated Service
+	 * in Hyper-V Manager), the driver's remove() invokes vmbus_close():
+	 * here we should skip most of the below cleanup work.
+	 */
+	if (channel->state != CHANNEL_OPENED_STATE) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	channel->state = CHANNEL_OPEN_STATE;
 	channel->sc_creation_callback = NULL;
 	/* Stop callback and cancel the timer asap */
Signature · 1 line
1.7.4.1
author
#8 lore raw

PATCH v2 8/12 Drivers: hv: vmbus: fix rescind-offer handling for device without a driver

In reply to K. Y. Srinivasan

From: Dexuan Cui <[email protected]>

In the path vmbus_onoffer_rescind() -> vmbus_device_unregister() -> device_unregister() -> ... -> __device_release_driver(), we can see for a device without a driver loaded: dev->driver is NULL, so dev->bus->remove(dev), namely vmbus_remove(), isn't invoked.

As a result, vmbus_remove() -> hv_process_channel_removal() isn't invoked and some cleanups(like sending a CHANNELMSG_RELID_RELEASED message to the host) aren't done.

We can demo the issue this way:
1. rmmod hv_utils;
2. disable the Heartbeat Integration Service in Hyper-V Manager and lsvmbus shows the device disappears.
3. re-enable the Heartbeat in Hyper-V Manager and modprobe hv_utils, but lsvmbus shows the device can't appear again.
This is because, the host thinks the VM hasn't released the relid, so can't re-offer the device to the VM.

We can fix the issue by moving hv_process_channel_removal() from vmbus_close_internal() to vmbus_device_release(), since the latter is always invoked on device_unregister(), whether or not the dev has a driver loaded.

Signed-off-by Dexuan Cui <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/channel.c      |    6 ------
 drivers/hv/channel_mgmt.c |    6 +++---
 drivers/hv/vmbus_drv.c    |   15 +++------------
 3 files changed, 6 insertions(+), 21 deletions(-)
drivers/hv/channel.c +0 -6
index 00e1be7..77d2579 100644
@@ -575,12 +575,6 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
 	free_pages((unsigned long)channel->ringbuffer_pages,
 		get_order(channel->ringbuffer_pagecount * PAGE_SIZE));
 
-	/*
-	 * If the channel has been rescinded; process device removal.
-	 */
-	if (channel->rescind)
-		hv_process_channel_removal(channel,
-					   channel->offermsg.child_relid);
 out:
 	tasklet_enable(tasklet);
drivers/hv/channel_mgmt.c +3 -3
index dc4fb0b..7903acc 100644
@@ -191,6 +191,8 @@ void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid)
 	if (channel == NULL)
 		return;
 
+	BUG_ON(!channel->rescind);
+
 	if (channel->target_cpu != get_cpu()) {
 		put_cpu();
 		smp_call_function_single(channel->target_cpu,

@@ -230,9 +232,7 @@ void vmbus_free_channels(void)
 
 	list_for_each_entry_safe(channel, tmp, &vmbus_connection.chn_list,
 		listentry) {
-		/* if we don't set rescind to true, vmbus_close_internal()
-		 * won't invoke hv_process_channel_removal().
-		 */
+		/* hv_process_channel_removal() needs this */
 		channel->rescind = true;
 
 		vmbus_device_unregister(channel->device_obj);
drivers/hv/vmbus_drv.c +3 -12
index ab888a1..f123bca 100644
@@ -601,23 +601,11 @@ static int vmbus_remove(struct device *child_device)
 {
 	struct hv_driver *drv;
 	struct hv_device *dev = device_to_hv_device(child_device);
-	u32 relid = dev->channel->offermsg.child_relid;
 
 	if (child_device->driver) {
 		drv = drv_to_hv_drv(child_device->driver);
 		if (drv->remove)
 			drv->remove(dev);
-		else {
-			hv_process_channel_removal(dev->channel, relid);
-			pr_err("remove not set for driver %s\n",
-				dev_name(child_device));
-		}
-	} else {
-		/*
-		 * We don't have a driver for this device; deal with the
-		 * rescind message by removing the channel.
-		 */
-		hv_process_channel_removal(dev->channel, relid);
 	}
 
 	return 0;

@@ -652,7 +640,10 @@ static void vmbus_shutdown(struct device *child_device)
 static void vmbus_device_release(struct device *device)
 {
 	struct hv_device *hv_dev = device_to_hv_device(device);
+	struct vmbus_channel *channel = hv_dev->channel;
 
+	hv_process_channel_removal(channel,
+				   channel->offermsg.child_relid);
 	kfree(hv_dev);
 
 }
Signature · 1 line
1.7.4.1
author
#9 lore raw

PATCH v2 9/12 Drivers: hv: vmbus: release relid on error in vmbus_process_offer()

In reply to K. Y. Srinivasan

From: Dexuan Cui <[email protected]>

We want to simplify vmbus_onoffer_rescind() by not invoking hv_process_channel_removal(NULL, ...).

Signed-off-by Dexuan Cui <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/channel_mgmt.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)
drivers/hv/channel_mgmt.c +15 -6
index 7903acc..9c9da3a 100644
@@ -177,19 +177,22 @@ static void percpu_channel_deq(void *arg)
 }
 
 
-void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid)
+static void vmbus_release_relid(u32 relid)
 {
 	struct vmbus_channel_relid_released msg;
-	unsigned long flags;
-	struct vmbus_channel *primary_channel;
 
 	memset(&msg, 0, sizeof(struct vmbus_channel_relid_released));
 	msg.child_relid = relid;
 	msg.header.msgtype = CHANNELMSG_RELID_RELEASED;
 	vmbus_post_msg(&msg, sizeof(struct vmbus_channel_relid_released));
+}
 
-	if (channel == NULL)
-		return;
+void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid)
+{
+	unsigned long flags;
+	struct vmbus_channel *primary_channel;
+
+	vmbus_release_relid(relid);
 
 	BUG_ON(!channel->rescind);
 

@@ -336,6 +339,8 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
 	return;
 
 err_deq_chan:
+	vmbus_release_relid(newchannel->offermsg.child_relid);
+
 	spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
 	list_del(&newchannel->listentry);
 	spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);

@@ -587,7 +592,11 @@ static void vmbus_onoffer_rescind(struct vmbus_channel_message_header *hdr)
 	channel = relid2channel(rescind->child_relid);
 
 	if (channel == NULL) {
-		hv_process_channel_removal(NULL, rescind->child_relid);
+		/*
+		 * This is very impossible, because in
+		 * vmbus_process_offer(), we have already invoked
+		 * vmbus_release_relid() on error.
+		 */
 		return;
 	}
Signature · 1 line
1.7.4.1
author
#10 lore raw

PATCH v2 10/12 Drivers: hv: vmbus: channge vmbus_connection.channel_lock to mutex

In reply to K. Y. Srinivasan

From: Dexuan Cui <[email protected]>

spinlock is unnecessary here.
mutex is enough.

Signed-off-by Dexuan Cui <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/channel_mgmt.c |   12 ++++++------
 drivers/hv/connection.c   |    7 +++----
 drivers/hv/hyperv_vmbus.h |    2 +-
 3 files changed, 10 insertions(+), 11 deletions(-)
drivers/hv/channel_mgmt.c +6 -6
index 9c9da3a..d013171 100644
@@ -206,9 +206,9 @@ void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid)
 	}
 
 	if (channel->primary_channel == NULL) {
-		spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
+		mutex_lock(&vmbus_connection.channel_mutex);
 		list_del(&channel->listentry);
-		spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
+		mutex_unlock(&vmbus_connection.channel_mutex);
 
 		primary_channel = channel;
 	} else {

@@ -253,7 +253,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
 	unsigned long flags;
 
 	/* Make sure this is a new offer */
-	spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
+	mutex_lock(&vmbus_connection.channel_mutex);
 
 	list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
 		if (!uuid_le_cmp(channel->offermsg.offer.if_type,

@@ -269,7 +269,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
 		list_add_tail(&newchannel->listentry,
 			      &vmbus_connection.chn_list);
 
-	spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
+	mutex_unlock(&vmbus_connection.channel_mutex);
 
 	if (!fnew) {
 		/*

@@ -341,9 +341,9 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
 err_deq_chan:
 	vmbus_release_relid(newchannel->offermsg.child_relid);
 
-	spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
+	mutex_lock(&vmbus_connection.channel_mutex);
 	list_del(&newchannel->listentry);
-	spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
+	mutex_unlock(&vmbus_connection.channel_mutex);
 
 	if (newchannel->target_cpu != get_cpu()) {
 		put_cpu();
drivers/hv/connection.c +3 -4
index 4fc2e88..521f48e 100644
@@ -146,7 +146,7 @@ int vmbus_connect(void)
 	spin_lock_init(&vmbus_connection.channelmsg_lock);
 
 	INIT_LIST_HEAD(&vmbus_connection.chn_list);
-	spin_lock_init(&vmbus_connection.channel_lock);
+	mutex_init(&vmbus_connection.channel_mutex);
 
 	/*
 	 * Setup the vmbus event connection for channel interrupt

@@ -282,11 +282,10 @@ struct vmbus_channel *relid2channel(u32 relid)
 {
 	struct vmbus_channel *channel;
 	struct vmbus_channel *found_channel  = NULL;
-	unsigned long flags;
 	struct list_head *cur, *tmp;
 	struct vmbus_channel *cur_sc;
 
-	spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
+	mutex_lock(&vmbus_connection.channel_mutex);
 	list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
 		if (channel->offermsg.child_relid == relid) {
 			found_channel = channel;

@@ -305,7 +304,7 @@ struct vmbus_channel *relid2channel(u32 relid)
 			}
 		}
 	}
-	spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
+	mutex_unlock(&vmbus_connection.channel_mutex);
 
 	return found_channel;
 }
drivers/hv/hyperv_vmbus.h +1 -1
index 64950d8..0af6dce 100644
@@ -688,7 +688,7 @@ struct vmbus_connection {
 
 	/* List of channels */
 	struct list_head chn_list;
-	spinlock_t channel_lock;
+	struct mutex channel_mutex;
 
 	struct workqueue_struct *work_queue;
 };
Signature · 1 line
1.7.4.1
#11 lore raw

Re: [PATCH V2 10/12] Drivers: hv: vmbus: channge vmbus_connection.channel_lock to mutex

Quoted 4 lines · K. Y. Srinivasan

From: Dexuan Cui <[email protected]>

spinlock is unnecessary here.
mutex is enough.

Hm, mutex is usually required when we need to sleep and a spinlock is enough otherwise :-)

Or are you trying to say we don't need to disable interrupts here? In that can maybe we can just switch to spin_lock()/spin_unlock()?

Quoted 103 lines
Signed-off-by Dexuan Cui <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/channel_mgmt.c |   12 ++++++------
 drivers/hv/connection.c   |    7 +++----
 drivers/hv/hyperv_vmbus.h |    2 +-
 3 files changed, 10 insertions(+), 11 deletions(-)
drivers/hv/channel_mgmt.c +6 -6
index 9c9da3a..d013171 100644
@@ -206,9 +206,9 @@ void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid)
 	}
 
 	if (channel->primary_channel == NULL) {
-		spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
+		mutex_lock(&vmbus_connection.channel_mutex);
 		list_del(&channel->listentry);
-		spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
+		mutex_unlock(&vmbus_connection.channel_mutex);
 
 		primary_channel = channel;
 	} else {

@@ -253,7 +253,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
 	unsigned long flags;
 
 	/* Make sure this is a new offer */
-	spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
+	mutex_lock(&vmbus_connection.channel_mutex);
 
 	list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
 		if (!uuid_le_cmp(channel->offermsg.offer.if_type,

@@ -269,7 +269,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
 		list_add_tail(&newchannel->listentry,
 			      &vmbus_connection.chn_list);
 
-	spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
+	mutex_unlock(&vmbus_connection.channel_mutex);
 
 	if (!fnew) {
 		/*

@@ -341,9 +341,9 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
 err_deq_chan:
 	vmbus_release_relid(newchannel->offermsg.child_relid);
 
-	spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
+	mutex_lock(&vmbus_connection.channel_mutex);
 	list_del(&newchannel->listentry);
-	spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
+	mutex_unlock(&vmbus_connection.channel_mutex);
 
 	if (newchannel->target_cpu != get_cpu()) {
 		put_cpu();
drivers/hv/connection.c +3 -4
index 4fc2e88..521f48e 100644
@@ -146,7 +146,7 @@ int vmbus_connect(void)
 	spin_lock_init(&vmbus_connection.channelmsg_lock);
 
 	INIT_LIST_HEAD(&vmbus_connection.chn_list);
-	spin_lock_init(&vmbus_connection.channel_lock);
+	mutex_init(&vmbus_connection.channel_mutex);
 
 	/*
 	 * Setup the vmbus event connection for channel interrupt

@@ -282,11 +282,10 @@ struct vmbus_channel *relid2channel(u32 relid)
 {
 	struct vmbus_channel *channel;
 	struct vmbus_channel *found_channel  = NULL;
-	unsigned long flags;
 	struct list_head *cur, *tmp;
 	struct vmbus_channel *cur_sc;
 
-	spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
+	mutex_lock(&vmbus_connection.channel_mutex);
 	list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
 		if (channel->offermsg.child_relid == relid) {
 			found_channel = channel;

@@ -305,7 +304,7 @@ struct vmbus_channel *relid2channel(u32 relid)
 			}
 		}
 	}
-	spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
+	mutex_unlock(&vmbus_connection.channel_mutex);
 
 	return found_channel;
 }
drivers/hv/hyperv_vmbus.h +1 -1
index 64950d8..0af6dce 100644
@@ -688,7 +688,7 @@ struct vmbus_connection {
 
 	/* List of channels */
 	struct list_head chn_list;
-	spinlock_t channel_lock;
+	struct mutex channel_mutex;
 
 	struct workqueue_struct *work_queue;
 };
Signature · 1 line
  Vitaly
maintainer
#12 lore raw

RE: [PATCH V2 10/12] Drivers: hv: vmbus: channge vmbus_connection.channel_lock to mutex

Quoted 11 lines

From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Thursday, November 12, 2015 18:41
To: Dexuan Cui <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; KY Srinivasan <[email protected]>
Subject: Re: [PATCH V2 10/12] Drivers: hv: vmbus: channge vmbus_connection.channel_lock to mutex

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

Quoted 7 lines
Quoted 4 lines

From: Dexuan Cui <[email protected]>

spinlock is unnecessary here.
mutex is enough.

Hm, mutex is usually required when we need to sleep and a spinlock is enough otherwise :-)

Sorry, I should have written a better changelog. :-)

Quoted 1 line

Or are you trying to say we don't need to disable interrupts here? In

Yes.
Here we try to protect vmbus_connection.chn_list and the related channel pointer (see relid2channel()) from being updated in parallel.

The parallel paths, e.g., vmbus_process_offer() and vmbus_onoffer_rescind(), are in process context and no irq context is involved, so we don't need disable interrupts at all.

Quoted 1 line

that can maybe we can just switch to spin_lock()/spin_unlock()?

Switching to mutex actually makes preparation for a later patch (which will be posted to LKML once this pachset is accepted):

Drivers: hv: vmbus: add an API vmbus_hvsock_device_unregister()
https://github.com/dcui/linux/commit/185afe8394a9bdae2be11ee1ea2a38d05e373025
(on branch decui/vmsock_1020)

For a vmsock socket connection, the host and the guest can be closing the connection at the same time.

When the host tries to close the connection, a rescind offer is received in the VM.

When the guest tries to close the connection, a new vmbus API vmbus_hvsock_device_unregister(channel) is invoked, so vmbus_hvsock_device_unregister() -> vmbus_device_unregister() is invoked and this can be running in parallel with vmbus_onoffer_rescind() -> vmbus_device_unregister().

The issue of "vmbus_onoffer_rescind () -> relid2channel()" is:
it returns a channel pointer without the spinlock held, so actually there is no real protection for the channel pointer.

So IMO we need to serialize vmbus_onoffer_rescind() and vmbus_hvsock_device_unregister().
Here I use mutex (rather than spinlock) because the critical section can sleep, due to vmbus_device_unregister().

Thanks,
-- Dexuan

author
#13 lore raw

PATCH v2 11/12 drivers:hv: Allow for MMIO claims that span ACPI _CRS records

In reply to K. Y. Srinivasan

From: Jake Oshins <[email protected]>

This patch makes 16GB GPUs work in Hyper-V VMs, since, for compatibility reasons, the Hyper-V BIOS lists MMIO ranges in 2GB chunks in its root bus's _CRS object.

Signed-off-by Jake Oshins <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/vmbus_drv.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)
drivers/hv/vmbus_drv.c +16 -0
index f123bca..328e4c3 100644
@@ -1063,12 +1063,28 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
 	new_res->start = start;
 	new_res->end = end;
 
+	/*
+	 * Stick ranges from higher in address space at the front of the list.
+	 * If two ranges are adjacent, merge them.
+	 */
 	do {
 		if (!*old_res) {
 			*old_res = new_res;
 			break;
 		}
 
+		if (((*old_res)->end + 1) == new_res->start) {
+			(*old_res)->end = new_res->end;
+			kfree(new_res);
+			break;
+		}
+
+		if ((*old_res)->start == new_res->end + 1) {
+			(*old_res)->start = new_res->start;
+			kfree(new_res);
+			break;
+		}
+
 		if ((*old_res)->end < new_res->start) {
 			new_res->sibling = *old_res;
 			if (prev_res)
Signature · 1 line
1.7.4.1
author
#14 lore raw

PATCH v2 12/12 tools/hv: Use include/uapi with __EXPORTED_HEADERS__

In reply to K. Y. Srinivasan

From: Kamal Mostafa <[email protected]>

Use the local uapi headers to keep in sync with "recently" added #define's (e.g. VSS_OP_REGISTER1).

Fixes 3eb2094c59e89db2bedd401e23c7a870081c9edb
Signed-off-by Kamal Mostafa <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 tools/hv/Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
tools/hv/Makefile +2 -0
index a8ab795..a8c4644 100644
@@ -5,6 +5,8 @@ PTHREAD_LIBS = -lpthread
 WARNINGS = -Wall -Wextra
 CFLAGS = $(WARNINGS) -g $(PTHREAD_LIBS) $(shell getconf LFS_CFLAGS)
 
+CFLAGS += -D__EXPORTED_HEADERS__ -I../../include/uapi -I../../include
+
 all: hv_kvp_daemon hv_vss_daemon hv_fcopy_daemon
 %: %.c
 	$(CC) $(CFLAGS) -o $@ $^
Signature · 1 line
1.7.4.1