Drivers: hv: vmbus: Wait for boot-time offers and log missing offers

Channel offers are requested during VMBus initialization and resume from hibernation. Add support to wait for all boot-time channel offers to be delivered and processed before returning from vmbus_request_offers.

This is in analogy to a PCI bus not returning from probe until it has scanned all devices on the bus.

Without this, user mode can race with VMBus initialization and miss channel offers. User mode has no way to work around this other than sleeping for a while, since there is no way to know when VMBus has finished processing boot-time offers.

With this added functionality, remove earlier logic which keeps track of count of offered channels post resume from hibernation. Once all offers delivered message is received, no further boot-time offers are going to be received. Consequently, logic to prevent suspend from happening after previous resume had missing offers, is also removed.

Co-developed-by John Starks <[email protected]>

Signed-off-by John Starks <[email protected]>

Signed-off-by Naman Jain <[email protected]>

Reviewed-by Easwar Hariharan <[email protected]>

---
Changes since v2:
* Incorporated Easwar's suggestion to use secs_to_jiffies() as his changes are now merged.
* Addressed Michael's comments:
* Used boot-time offers/channels/devices to maintain consistency
* Rephrased CHANNELMSG_ALLOFFERS_DELIVERED handler function comments for better explanation. Thanks for sharing the write-up.
* Changed commit msg and other things as per suggestions
* Addressed Dexuan's comments, which came up in offline discussion:
* Changed timeout for waiting for all offers delivered msg to 60s instead of 10s. Reason being, the host can experience some servicing events or diagnostics events, which may take a long time and hence may fail to offer all the devices within 10s.
* Minor additions in commit subject.

Changes since v1:
* Added Easwar's Reviewed-By tag
* Addressed Michael's comments:
* Added explanation of all offers delivered message in comments
* Removed infinite wait for offers logic, and changed it wait once.
* Removed sub channel workqueue flush logic
* Added comments on why MLX device offer is not expected as part of this essential boot offer list. I refrained from adding too many details on it as it felt like it is beyond the scope of this patch series and may not be relevant to this. However, please let me know if something needs to be added.
* Addressed Saurabh's comments:
* Changed timeout value to 10000 ms instead of 10*10000
* Changed commit msg as per suggestions
* Added a comment for warning case of wait_for_completion timeout

---
 drivers/hv/channel_mgmt.c | 61 +++++++++++++++++++++++++++++----------
 drivers/hv/connection.c   |  4 +--
 drivers/hv/hyperv_vmbus.h | 14 ++-------
 drivers/hv/vmbus_drv.c    | 16 ----------
 4 files changed, 51 insertions(+), 44 deletions(-)

drivers/hv/channel_mgmt.c +0 -12

index 3c6011a48dab..b1a095671e32 100644
@@ -944,16 +944,6 @@ void vmbus_initiate_unload(bool crash)
 		vmbus_wait_for_unload();
 }
 
-static void check_ready_for_resume_event(void)
-{
-	/*
-	 * If all the old primary channels have been fixed up, then it's safe
-	 * to resume.
-	 */
-	if (atomic_dec_and_test(&vmbus_connection.nr_chan_fixup_on_resume))
-		complete(&vmbus_connection.ready_for_resume_event);
-}
-
 static void vmbus_setup_channel_state(struct vmbus_channel *channel,
 				      struct vmbus_channel_offer_channel *offer)
 {

@@ -1109,8 +1099,6 @@ static void vmbus_onoffer(struct
 vmbus_channel_message_header *hdr)
 
 		/* Add the channel back to the array of channels. */
 		vmbus_channel_map_relid(oldchannel);
-		check_ready_for_resume_event();
-
 		mutex_unlock(&vmbus_connection.channel_mutex);
 		return;

drivers/hv/channel_mgmt.c +17 -2

 	}

@@ -1296,13 +1284,28 @@
 EXPORT_SYMBOL_GPL(vmbus_hvsock_device_unregister);
 
 /*
  * vmbus_onoffers_delivered -
- * This is invoked when all offers have been delivered.
+ * The CHANNELMSG_ALLOFFERS_DELIVERED message arrives after all
+ * boot-time offers are delivered. A boot-time offer is for the primary
+ * channel for any virtual hardware configured in the VM at the time it boots.
+ * Boot-time offers include offers for physical devices assigned to the VM
+ * via Hyper-V's Discrete Device Assignment (DDA) functionality that are
+ * handled as virtual PCI devices in Linux (e.g., NVMe devices and GPUs).
+ * Boot-time offers do not include offers for VMBus sub-channels. Because
+ * devices can be hot-added to the VM after it is booted, additional channel
+ * offers that aren't boot-time offers can be received at any time after the
+ * all-offers-delivered message.
  *
- * Nothing to do here.
+ * SR-IOV NIC Virtual Functions (VFs) assigned to a VM are not considered
+ * to be assigned to the VM at boot-time, and offers for VFs may occur after
+ * the all-offers-delivered message. VFs are optional accelerators to the
+ * synthetic VMBus NIC and are effectively hot-added only after the VMBus
+ * NIC channel is opened (once it knows the guest can support it, via the
+ * sriov bit in the netvsc protocol).
  */
 static void vmbus_onoffers_delivered(
 			struct vmbus_channel_message_header *hdr)
 {
+	complete(&vmbus_connection.all_offers_delivered_event);
 }
 

/*

drivers/hv/channel_mgmt.c +2 -1

@@ -1578,7 +1581,8 @@ void vmbus_onmessage(struct
 vmbus_channel_message_header *hdr)
 }
 
 /*
- * vmbus_request_offers - Send a request to get all our pending offers.
+ * vmbus_request_offers - Send a request to get all our pending offers
+ * and wait for all boot-time offers to arrive.
  */
 int vmbus_request_offers(void)

{

drivers/hv/channel_mgmt.c +27 -0

@@ -1596,6 +1600,10 @@ int vmbus_request_offers(void)
 
 	msg->msgtype = CHANNELMSG_REQUESTOFFERS;
 
+	/*
+	 * This REQUESTOFFERS message will result in the host sending an all
+	 * offers delivered message after all the boot-time offers are sent.
+	 */
 	ret = vmbus_post_msg(msg, sizeof(struct vmbus_channel_message_header),
 			     true);
 

@@ -1607,6 +1615,29 @@ int vmbus_request_offers(void)
 		goto cleanup;
 	}
 
+	/*
+	 * Wait for the host to send all boot-time offers.
+	 * Keeping it as a best-effort mechanism, where a warning is
+	 * printed if a timeout occurs, and execution is resumed.
+	 */
+	if (!wait_for_completion_timeout(
+		&vmbus_connection.all_offers_delivered_event, secs_to_jiffies(60))) {
+		pr_warn("timed out waiting for all boot-time offers to be delivered.\n");
+	}
+
+	/*
+	 * Flush handling of offer messages (which may initiate work on
+	 * other work queues).
+	 */
+	flush_workqueue(vmbus_connection.work_queue);
+
+	/*
+	 * Flush workqueue for processing the incoming offers. Subchannel
+	 * offers and their processing can happen later, so there is no need to
+	 * flush that workqueue here.
+	 */
+	flush_workqueue(vmbus_connection.handle_primary_chan_wq);
+
 cleanup:
 	kfree(msginfo);
 

drivers/hv/connection.c +2 -2

index f001ae880e1d..8351360bba16 100644
@@ -34,8 +34,8 @@ struct vmbus_connection vmbus_connection = {
 
 	.ready_for_suspend_event = COMPLETION_INITIALIZER(
 				  vmbus_connection.ready_for_suspend_event),
-	.ready_for_resume_event	= COMPLETION_INITIALIZER(
-				  vmbus_connection.ready_for_resume_event),
+	.all_offers_delivered_event = COMPLETION_INITIALIZER(
+				  vmbus_connection.all_offers_delivered_event),
 };
 EXPORT_SYMBOL_GPL(vmbus_connection);
 

drivers/hv/hyperv_vmbus.h +3 -11

index d2856023d53c..66160995519a 100644
@@ -287,18 +287,10 @@ struct vmbus_connection {
 	struct completion ready_for_suspend_event;
 
 	/*
-	 * The number of primary channels that should be "fixed up"
-	 * upon resume: these channels are re-offered upon resume, and some
-	 * fields of the channel offers (i.e. child_relid and connection_id)
-	 * can change, so the old offermsg must be fixed up, before the resume
-	 * callbacks of the VSC drivers start to further touch the channels.
+	 * Completed once the host has offered all boot-time channels.
+	 * Note that some channels may still be under process on a workqueue.
 	 */
-	atomic_t nr_chan_fixup_on_resume;
-	/*
-	 * vmbus_bus_resume() waits for "nr_chan_fixup_on_resume" to
-	 * drop to zero.
-	 */
-	struct completion ready_for_resume_event;
+	struct completion all_offers_delivered_event;
 };
 
 

drivers/hv/vmbus_drv.c +0 -17

index 9b15f7daf505..bd3fc41dc06b 100644
@@ -2427,11 +2427,6 @@ static int vmbus_bus_suspend(struct device *dev)
 	if (atomic_read(&vmbus_connection.nr_chan_close_on_suspend) > 0)
 		wait_for_completion(&vmbus_connection.ready_for_suspend_event);
 
-	if (atomic_read(&vmbus_connection.nr_chan_fixup_on_resume) != 0) {
-		pr_err("Can not suspend due to a previous failed resuming\n");
-		return -EBUSY;
-	}
-
 	mutex_lock(&vmbus_connection.channel_mutex);
 
 	list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {

@@ -2456,17 +2451,12 @@ static int vmbus_bus_suspend(struct device *dev)
 			pr_err("Sub-channel not deleted!\n");
 			WARN_ON_ONCE(1);
 		}
-
-		atomic_inc(&vmbus_connection.nr_chan_fixup_on_resume);
 	}
 
 	mutex_unlock(&vmbus_connection.channel_mutex);
 
 	vmbus_initiate_unload(false);
 
-	/* Reset the event for the next resume. */
-	reinit_completion(&vmbus_connection.ready_for_resume_event);
-
 	return 0;
 }
 

@@ -2502,14 +2492,8 @@ static int vmbus_bus_resume(struct device *dev)
 	if (ret != 0)
 		return ret;
 
-	WARN_ON(atomic_read(&vmbus_connection.nr_chan_fixup_on_resume) ==
 0);
-
 	vmbus_request_offers();
 
-	if (wait_for_completion_timeout(
-		&vmbus_connection.ready_for_resume_event, 10 * HZ) == 0)
-		pr_err("Some vmbus device is missing after suspending?\n");
-
 	/* Reset the event for the next suspend. */
 	reinit_completion(&vmbus_connection.ready_for_suspend_event);
 
--

2.34.1

From: John Starks <[email protected]>

When resuming from hibernation, log any channels that were present before hibernation but now are gone.
In general, the boot-time devices configured for a resuming VM should be the same as the devices in the VM at the time of hibernation. It's uncommon for the configuration to have been changed such that offers are missing. Changing the configuration violates the rules for hibernation anyway.
The cleanup of missing channels is not straight-forward and dependent on individual device driver functionality and implementation, so it can be added in future with separate changes.

Signed-off-by John Starks <[email protected]>

Co-developed-by Naman Jain <[email protected]>

Signed-off-by Naman Jain <[email protected]>

Reviewed-by Easwar Hariharan <[email protected]>

---
Changes since v2:
* Addressed Michael's comments:
* Changed commit msg as per suggestions
* Addressed Dexuan's comments, which came up in offline discussion:
* Minor additions in commit subject.

Changes since v1:
* Added Easwar's Reviewed-By tag
* Addressed Saurabh's comments:
* Added a note for missing channel cleanup in comments and commit msg

---
 drivers/hv/vmbus_drv.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/hv/vmbus_drv.c +17 -1

index bd3fc41dc06b..08214f28694a 100644
@@ -2462,6 +2462,7 @@ static int vmbus_bus_suspend(struct device *dev)
 
 static int vmbus_bus_resume(struct device *dev)
 {
+	struct vmbus_channel *channel;
 	struct vmbus_channel_msginfo *msginfo;
 	size_t msgsize;
 	int ret;

@@ -2494,6 +2495,22 @@ static int vmbus_bus_resume(struct device *dev)
 
 	vmbus_request_offers();
 
+	mutex_lock(&vmbus_connection.channel_mutex);
+	list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
+		if (channel->offermsg.child_relid != INVALID_RELID)
+			continue;
+
+		/* hvsock channels are not expected to be present. */
+		if (is_hvsock_channel(channel))
+			continue;
+
+		pr_err("channel %pUl/%pUl not present after resume.\n",
+			&channel->offermsg.offer.if_type,
+			&channel->offermsg.offer.if_instance);
+		/* ToDo: Cleanup these channels here */
+	}
+	mutex_unlock(&vmbus_connection.channel_mutex);
+
 	/* Reset the event for the next suspend. */
 	reinit_completion(&vmbus_connection.ready_for_suspend_event);
 
--

2.34.1

After VM requests for channel offers during boot or resume from hibernation, host offers the devices that the VM is configured with and then sends a separate message indicating that all the boot-time channel offers are delivered. Wait for this message to make this boot-time offers request and receipt process synchronous.

Without this, user mode can race with VMBus initialization and miss channel offers. User mode has no way to work around this other than sleeping for a while, since there is no way to know when VMBus has finished processing boot-time offers.

This is in analogy to a PCI bus not returning from probe until it has scanned all devices on the bus.

As part of this implementation, some code cleanup is also done for the logic which becomes redundant due to this change.

Second patch prints the channels which are not offered when resume happens from hibernation to supply more information to the end user.

Changes since v2:
https://lore.kernel.org/all/[email protected]/
* Incorporated Easwar's suggestion to use secs_to_jiffies() as his changes are now merged.
* Addressed Michael's comments:
* Used boot-time offers/channels/devices to maintain consistency
* Rephrased CHANNELMSG_ALLOFFERS_DELIVERED handler function comments for better explanation. Thanks for sharing the write-up.
* Changed commit msg and other things as per suggestions
* Addressed Dexuan's comments, which came up in offline discussion:
* Changed timeout for waiting for all offers delivered msg to 60s instead of 10s. Reason being, the host can experience some servicing events or diagnostics events, which may take a long time and hence may fail to offer all the devices within 10s.
* Minor additions in commit subject of both patches
* Rebased on latest linux-next master tip

Changes since v1:
https://lore.kernel.org/all/[email protected]/
* Added Easwar's Reviewed-By tag
* Addressed Michael's comments:
* Added explanation of all offers delivered message in comments
* Removed infinite wait for offers logic, and changed it wait once.
* Removed sub channel workqueue flush logic
* Added comments on why MLX device offer is not expected as part of this essential boot offer list. I refrained from adding too many details on it as it felt like it is beyond the scope of this patch series and may not be relevant to this. However, please let me know if something needs to be added.
* Addressed Saurabh's comments:
* Changed timeout value to 10000 ms instead of 10*1000
* Changed commit msg as per suggestions
* Added a comment for warning case of wait_for_completion timeout
* Added a note for missing channel cleanup in comments and commit msg

John Starks (1):
Drivers: hv: vmbus: Log on missing offers if any

Naman Jain (1):
Drivers: hv: vmbus: Wait for boot-time offers during boot and resume

 drivers/hv/channel_mgmt.c | 61 +++++++++++++++++++++++++++++----------
 drivers/hv/connection.c   |  4 +--
 drivers/hv/hyperv_vmbus.h | 14 ++-------
 drivers/hv/vmbus_drv.c    | 31 ++++++++++----------
 4 files changed, 67 insertions(+), 43 deletions(-)

base-commit: 28955f4fa2823e39f1ecfb3a37a364563527afbc

After VM requests for channel offers during boot or resume from hibernation, host offers the devices that the VM is configured with and then sends a separate message indicating that all the boot-time channel offers are delivered. Wait for this message to make this boot-time offers request and receipt process synchronous.

Without this, user mode can race with VMBus initialization and miss channel offers. User mode has no way to work around this other than sleeping for a while, since there is no way to know when VMBus has finished processing boot-time offers.

This is in analogy to a PCI bus not returning from probe until it has scanned all devices on the bus.

As part of this implementation, some code cleanup is also done for the logic which becomes redundant due to this change.

Second patch prints the channels which are not offered when resume happens from hibernation to supply more information to the end user.

Changes since v2:
https://lore.kernel.org/all/[email protected]/
* Incorporated Easwar's suggestion to use secs_to_jiffies() as his changes are now merged.
* Addressed Michael's comments:
* Used boot-time offers/channels/devices to maintain consistency
* Rephrased CHANNELMSG_ALLOFFERS_DELIVERED handler function comments for better explanation. Thanks for sharing the write-up.
* Changed commit msg and other things as per suggestions
* Addressed Dexuan's comments, which came up in offline discussion:
* Changed timeout for waiting for all offers delivered msg to 60s instead of 10s. Reason being, the host can experience some servicing events or diagnostics events, which may take a long time and hence may fail to offer all the devices within 10s.
* Minor additions in commit subject of both patches
* Rebased on latest linux-next master tip

Changes since v1:
https://lore.kernel.org/all/[email protected]/
* Added Easwar's Reviewed-By tag
* Addressed Michael's comments:
* Added explanation of all offers delivered message in comments
* Removed infinite wait for offers logic, and changed it wait once.
* Removed sub channel workqueue flush logic
* Added comments on why MLX device offer is not expected as part of this essential boot offer list. I refrained from adding too many details on it as it felt like it is beyond the scope of this patch series and may not be relevant to this. However, please let me know if something needs to be added.
* Addressed Saurabh's comments:
* Changed timeout value to 10000 ms instead of 10*1000
* Changed commit msg as per suggestions
* Added a comment for warning case of wait_for_completion timeout
* Added a note for missing channel cleanup in comments and commit msg

John Starks (1):
Drivers: hv: vmbus: Log on missing offers if any

Naman Jain (1):
Drivers: hv: vmbus: Wait for boot-time offers during boot and resume

 drivers/hv/channel_mgmt.c | 61 +++++++++++++++++++++++++++++----------
 drivers/hv/connection.c   |  4 +--
 drivers/hv/hyperv_vmbus.h | 14 ++-------
 drivers/hv/vmbus_drv.c    | 31 ++++++++++----------
 4 files changed, 67 insertions(+), 43 deletions(-)

base-commit: 28955f4fa2823e39f1ecfb3a37a364563527afbc

Signature · 1 line

2.34.1