Drivers: hv: vmbus: Wait for offers and log missing offers

Channel offers are requested during VMBus initialization and resume from hibernation. Add support to wait for all 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 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 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 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*10000
* Changed commit msg as per suggestions
* Added a comment for warning case of wait_for_completion timeout

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

drivers/hv/channel_mgmt.c +17 -15

index 3c6011a48dab..a2e9ebe5bf72 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;
 	}

@@ -1296,13 +1284,22 @@ EXPORT_SYMBOL_GPL(vmbus_hvsock_device_unregister);
 
 /*
  * vmbus_onoffers_delivered -
- * This is invoked when all offers have been delivered.
+ * CHANNELMSG_ALLOFFERS_DELIVERED message arrives after all the essential
+ * boot-time offers are delivered. Other channels can be hot added
+ * or removed later, even immediately after the all-offers-delivered
+ * message. A boot-time offer will be any of the virtual hardware the
+ * VM is configured with at boot.
  *
- * Nothing to do here.
+ * Virtual devices like Mellanox NIC may not be included in the list of
+ * these initial boot offers because it is an optional accelerator to
+ * the synthetic VMBus NIC. It is 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);
 }
 
 /*

@@ -1578,7 +1575,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 offers to arrive.
  */
 int vmbus_request_offers(void)
 {

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

@@ -1607,6 +1609,29 @@ int vmbus_request_offers(void)
 		goto cleanup;
 	}

drivers/hv/channel_mgmt.c +14 -0

+
+	/*
+	 * Flush handling of offer messages (which may initiate work on
+	 * other work queues).
+	 */
+	flush_workqueue(vmbus_connection.work_queue);
+
+	/*
+	 * Flush workqueues for processing the incoming offers. Subchannel
+	 * offers and processing can happen later, so there is no need to
+	 * flush those workqueues 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..80cc65dac740 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 channels. Note that
+	 * some channels may still be being process on a work queue.
 	 */
-	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 -16

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);
 

Quoted 123 lines · Naman Jain

Channel offers are requested during VMBus initialization and resume from hibernation. Add support to wait for all 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 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 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 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*10000
* Changed commit msg as per suggestions
* Added a comment for warning case of wait_for_completion timeout

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

drivers/hv/channel_mgmt.c +17 -15

index 3c6011a48dab..a2e9ebe5bf72 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;
  	}

@@ -1296,13 +1284,22 @@ EXPORT_SYMBOL_GPL(vmbus_hvsock_device_unregister);
 
  /*
   * vmbus_onoffers_delivered -
- * This is invoked when all offers have been delivered.
+ * CHANNELMSG_ALLOFFERS_DELIVERED message arrives after all the essential
+ * boot-time offers are delivered. Other channels can be hot added
+ * or removed later, even immediately after the all-offers-delivered
+ * message. A boot-time offer will be any of the virtual hardware the
+ * VM is configured with at boot.
   *
- * Nothing to do here.
+ * Virtual devices like Mellanox NIC may not be included in the list of
+ * these initial boot offers because it is an optional accelerator to
+ * the synthetic VMBus NIC. It is 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);
  }
 
  /*

@@ -1578,7 +1575,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 offers to arrive.
   */
  int vmbus_request_offers(void)
  {

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

@@ -1607,6 +1609,29 @@ int vmbus_request_offers(void)
  		goto cleanup;
  	}

drivers/hv/channel_mgmt.c int vmbus_request_offers(void) · line 1620

Quoted 10 lines

drivers/hv/channel_mgmt.c +9 -0

 
+	/*
+	 * Wait for the host to send all 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, msecs_to_jiffies(10000))) {
+		pr_warn("timed out waiting for all offers to be delivered...\n");
+	}

secs_to_jiffies() has been merged [1] so please update this to a call to secs_to_jiffies(10). A Cocinelle script will probably take care of it sooner or later in any case.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=b35108a51cf7bab58d7eace1267d7965978bcdb8

Channel offers are requested during VMBus initialization and resume from hibernation. Add support to wait for all 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 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 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 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*10000
* Changed commit msg as per suggestions
* Added a comment for warning case of wait_for_completion timeout

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

drivers/hv/channel_mgmt.c +0 -12

index 3c6011a48dab..a2e9ebe5bf72 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 +15 -1

  */
 static void vmbus_onoffers_delivered(
 			struct vmbus_channel_message_header *hdr)
 {
+	complete(&vmbus_connection.all_offers_delivered_event);
 }
 
 /*

@@ -1578,7 +1575,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 offers to arrive.
  */
 int vmbus_request_offers(void)
 {

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

@@ -1607,6 +1609,29 @@ int vmbus_request_offers(void)
 		goto cleanup;
 	}
 
+	/*
+	 * Wait for the host to send all 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, msecs_to_jiffies(10000))) {
+		pr_warn("timed out waiting for all offers to be delivered...\n");

drivers/hv/channel_mgmt.c +3 -0

+	 */
+	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 +0 -0

index d2856023d53c..80cc65dac740 100644

drivers/hv/hyperv_vmbus.h +2 -6

+	 * some channels may still be being process on a work queue.
 	 */
-	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: Naman Jain <[email protected]> Sent: Tuesday, October 29, 2024 1:02 AM

Quoted 3 lines

Channel offers are requested during VMBus initialization and resume from hibernation. Add support to wait for all channel offers to be

Given the definition of ALLOFFERS_DELIVERED, maybe change to:

" wait for all boot-time channel offers"

Quoted 6 lines

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.

Quoted 4 lines

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 offers.

"finished processing boot-time offers."

Quoted 5 lines

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 offers are going to be received.

"no further boot-time offers"

Quoted 8 lines

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]>

Quoted 10 lines

---
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

You've used slightly different phrasing here ("essential boot offer list"). Potential confusion can be avoided if the terminology is super consistent. I'm good with "boot-time offers" (or something else if you prefer). I'm not sure what "essential" means here, unless it refers to offers for VFs from SR-IOV NICs (like Mellanox) being excluded. But it should be fine to use something short like "boot-time offers" and then note the VF exception in the code comments as you've done.

Quoted 48 lines

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 | 55 ++++++++++++++++++++++++++++-----------
  drivers/hv/connection.c   |  4 +--
  drivers/hv/hyperv_vmbus.h | 14 +++-------
  drivers/hv/vmbus_drv.c    | 16 ------------
  4 files changed, 45 insertions(+), 44 deletions(-)

drivers/hv/channel_mgmt.c +0 -12

index 3c6011a48dab..a2e9ebe5bf72 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;
  	}

@@ -1296,13 +1284,22 @@
 EXPORT_SYMBOL_GPL(vmbus_hvsock_device_unregister);
 

drivers/hv/channel_mgmt.c EXPORT_SYMBOL_GPL(vmbus_hvsock_device_unregister); · line 1287

Quoted 4 lines

drivers/hv/channel_mgmt.c +1 -1

  /*
   * vmbus_onoffers_delivered -
- * This is invoked when all offers have been delivered.
+ * CHANNELMSG_ALLOFFERS_DELIVERED message arrives after all the essential

Again, I would drop "essential" here, as its meaning in this context isn't defined anywhere.

drivers/hv/channel_mgmt.c EXPORT_SYMBOL_GPL(vmbus_hvsock_device_unregister); · line 1291

Quoted 4 lines

drivers/hv/channel_mgmt.c +4 -0

+ * boot-time offers are delivered. Other channels can be hot added
+ * or removed later, even immediately after the all-offers-delivered
+ * message. A boot-time offer will be any of the virtual hardware the
+ * VM is configured with at boot.

This is good to have a definition of "boot-time offer". But I think some additional specification is appropriate. Let me suggest the following:

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.

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).

[Please confirm that my suggested wording is correct!]

Quoted 46 lines

drivers/hv/channel_mgmt.c +19 -1

   */
  static void vmbus_onoffers_delivered(
  			struct vmbus_channel_message_header *hdr)
  {
+	complete(&vmbus_connection.all_offers_delivered_event);
  }
 
  /*

@@ -1578,7 +1575,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 offers to arrive.
   */
  int vmbus_request_offers(void)
  {

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

@@ -1607,6 +1609,29 @@ int vmbus_request_offers(void)
  		goto cleanup;
  	}
 
+	/*
+	 * Wait for the host to send all 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, msecs_to_jiffies(10000))) {
+		pr_warn("timed out waiting for all offers to be delivered...\n");
+	}
+
+	/*
+	 * Flush handling of offer messages (which may initiate work on

drivers/hv/channel_mgmt.c int vmbus_request_offers(void) · line 1629

Quoted 6 lines

drivers/hv/channel_mgmt.c +6 -0

+	 * other work queues).
+	 */
+	flush_workqueue(vmbus_connection.work_queue);
+
+	/*
+	 * Flush workqueues for processing the incoming offers. Subchannel

s/workqueues/workqueue/

drivers/hv/channel_mgmt.c int vmbus_request_offers(void) · line 1631

Quoted 3 lines

drivers/hv/channel_mgmt.c +2 -0

+	 * offers and processing can happen later, so there is no need to
+	 * flush those workqueues here.

s/those workqueues/that workqueue/

Quoted 29 lines

drivers/hv/channel_mgmt.c +3 -0

+	 */
+	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 +0 -0

index d2856023d53c..80cc65dac740 100644
@@ -287,18 +287,10 @@ struct vmbus_connection {
  	struct completion ready_for_suspend_event;
 

drivers/hv/hyperv_vmbus.h struct vmbus_connection { · line 290

Quoted 7 lines

drivers/hv/hyperv_vmbus.h +1 -5

  	/*
-	 * 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 channels. Note that

"all boot-time channels"

Quoted 63 lines

drivers/hv/hyperv_vmbus.h +2 -6

+	 * some channels may still be being process on a work queue.
  	 */
-	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 -16

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);
 

Quoted 2 lines

--
2.34.1

When resuming from hibernation, log any channels that were present before hibernation but now are gone.
In general, the essential virtual devices configured for a VM, remain same, before and after the hibernation and its not very common that some offers are missing.

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 as 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 v1:
https://lore.kernel.org/all/[email protected]/
* 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 +7 -0

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. */

drivers/hv/vmbus_drv.c +0 -1

 	/* Reset the event for the next suspend. */
 	reinit_completion(&vmbus_connection.ready_for_suspend_event);
 
--

2.34.1

From: Naman Jain <[email protected]> Sent: Tuesday, October 29, 2024 1:02 AM

Quoted 6 lines

When resuming from hibernation, log any channels that were present before hibernation but now are gone.
In general, the essential virtual devices configured for a VM, remain same, before and after the hibernation and its not very common that some offers are missing.

The wording here is a bit jumbled. And let's use consistent terminology. I'd suggest:

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.

Quoted 42 lines

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 as 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 v1:
https://lore.kernel.org/all/[email protected]/
* 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 +7 -0

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. */

drivers/hv/vmbus_drv.c static int vmbus_bus_resume(struct device *dev) · line 2513

Quoted 10 lines

drivers/hv/vmbus_drv.c +10 -0

+		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);
+

Dexuan and John have explained how in Azure VMs, there should not be any VFs assigned to the VM at the time of hibernation. So the above check for missing offers does not trigger an error message due to VF offers coming after the all-offers-received message.

But what about the case of a VM running on a local Hyper-V? I'm not completely clear, but in that case I don't think any VFs are removed before the hibernation, especially for VM-initiated hibernation. It's

a reasonable scenario to later resume that same VM, with the same VF assigned to the VM. Because of the way current code counts the offers, vmbus_bus_resume() waits for the VF to be offered again, and all the channels get correct post-resume relids. But the changes in this patch set break that scenario. Since vmbus_bus_resume() now proceeds before the VF offer arrives, hv_pci_resume() calling vmbus_open() could use the pre-hibernation relid for the VF and break things. Certainly the "not present after resume" error message would be spurious.

Maybe the focus here is Azure, and it's tolerable for the local Hyper-V case with a VF to not work pending later fixes. But I thought I'd call out the potential issue (assuming my thinking is correct).

Michael

Quoted 6 lines

drivers/hv/vmbus_drv.c +0 -1

  	/* Reset the event for the next suspend. */
  	reinit_completion(&vmbus_connection.ready_for_suspend_event);
 
--

2.34.1

Quoted 8 lines · Michael Kelley

From: Naman Jain <[email protected]> Sent: Tuesday, October 29, 2024 1:02 AM

Quoted 6 lines

When resuming from hibernation, log any channels that were present before hibernation but now are gone.
In general, the essential virtual devices configured for a VM, remain same, before and after the hibernation and its not very common that some offers are missing.

Quoted 7 lines

The wording here is a bit jumbled. And let's use consistent terminology. I'd suggest:

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.

Sure, I'll make the required changes.

Quoted 59 lines

Quoted 43 lines

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 as 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 v1:
https://lore.kernel.org/all/20241018115811.5530-1- [email protected]/
* 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 +7 -0

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. */

drivers/hv/vmbus_drv.c static int vmbus_bus_resume(struct device *dev) · line 2514

Quoted 10 lines

drivers/hv/vmbus_drv.c +10 -0

+        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);
+

Dexuan and John have explained how in Azure VMs, there should not be any VFs assigned to the VM at the time of hibernation. So the above check for missing offers does not trigger an error message due to VF offers coming after the all-offers-received message.

Quoted 3 lines

But what about the case of a VM running on a local Hyper-V? I'm not completely clear, but in that case I don't think any VFs are removed before the hibernation, especially for VM-initiated hibernation. It's

I am not sure about this behavior. I have requested Dexuan offline for a comment.

Quoted 10 lines

a reasonable scenario to later resume that same VM, with the same VF assigned to the VM. Because of the way current code counts the offers, vmbus_bus_resume() waits for the VF to be offered again, and all the channels get correct post-resume relids. But the changes in this patch set break that scenario. Since vmbus_bus_resume() now proceeds before the VF offer arrives, hv_pci_resume() calling vmbus_open() could use the pre-hibernation relid for the VF and break things. Certainly the "not present after resume" error message would be spurious.

Quoted 5 lines

Maybe the focus here is Azure, and it's tolerable for the local Hyper-V case with a VF to not work pending later fixes. But I thought I'd call out the potential issue (assuming my thinking is correct).

Michael

IIUC, below scenarios can happen based on your comment-

Case 1:
VF channel offer is received in time before hv_pci_resume() and there are no problems.

Case 2:
Resume proceeds just after getting ALLOFFERS_DELIVERED msg and a warning is printed that this VF channel is not present after resume. Then two scenarios can happen:
  Case 2.1:
  VF channel offer is received before hv_pci_resume() and things work fine. Warning printed is spurious.
  Case 2.2:
  VM channel offer is not received before hv_pci_resume() and relid is not yet restored by onoffer. This is a problem. Warning is printed in this case for missing offer.

I think it all depends on whether or not VFs are removed in local HyperV VMs. I'll try to get this information. Thanks for pointing this out.

Regards,
Naman

Quoted 7 lines

Quoted 7 lines

drivers/hv/vmbus_drv.c +0 -0

       /* Reset the event for the next suspend. */
       reinit_completion(&vmbus_connection.ready_for_suspend_event);

Signature · 2 lines

-- 
2.34.1

Quoted 4 lines · Michael Kelley

Quoted 2 lines

From: Naman Jain <[email protected]> Sent: Tuesday, October 29, 2024 1:02 AM

Quoted 1 line

Quoted 67 lines

Quoted 26 lines

Quoted 10 lines

drivers/hv/vmbus_drv.c +6 -0

@@ -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. */

drivers/hv/vmbus_drv.c static int vmbus_bus_resume(struct device *dev) · line 2513

Quoted 10 lines

drivers/hv/vmbus_drv.c +10 -0

+        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);
+

Dexuan and John have explained how in Azure VMs, there should not be any VFs assigned to the VM at the time of hibernation. So the above check for missing offers does not trigger an error message due to VF offers coming after the all-offers-received message.

Quoted 3 lines

But what about the case of a VM running on a local Hyper-V? I'm not completely clear, but in that case I don't think any VFs are removed before the hibernation, especially for VM-initiated hibernation. It's

I am not sure about this behavior. I have requested Dexuan offline for a comment.

Quoted 10 lines

a reasonable scenario to later resume that same VM, with the same VF assigned to the VM. Because of the way current code counts the offers, vmbus_bus_resume() waits for the VF to be offered again, and all the channels get correct post-resume relids. But the changes in this patch set break that scenario. Since vmbus_bus_resume() now proceeds before the VF offer arrives, hv_pci_resume() calling vmbus_open() could use the pre-hibernation relid for the VF and break things. Certainly the "not present after resume" error message would be spurious.

Quoted 5 lines

Maybe the focus here is Azure, and it's tolerable for the local Hyper-V case with a VF to not work pending later fixes. But I thought I'd call out the potential issue (assuming my thinking is correct).

Michael

IIUC, below scenarios can happen based on your comment-

Case 1:
VF channel offer is received in time before hv_pci_resume() and there are no problems.

Case 2:
Resume proceeds just after getting ALLOFFERS_DELIVERED msg and a warning is printed that this VF channel is not present after resume. Then two scenarios can happen:
  Case 2.1:
  VF channel offer is received before hv_pci_resume() and things work fine. Warning printed is spurious.
  Case 2.2:
  VM channel offer is not received before hv_pci_resume() and relid is not yet restored by onoffer. This is a problem. Warning is printed in this case for missing offer.

Quoted 7 lines

I think it all depends on whether or not VFs are removed in local HyperV VMs. I'll try to get this information. Thanks for pointing this out.

Regards,
Naman

Hi Michael,
I discussed with Dexuan and we tried these scenarios. Here are the observations:

For the two ways of host initiated hibernation:

#1: Invoke-Hibernate $vm -Device (Uses the guest shutdown component) OR
#2: Invoke-Hibernate $vm -ComputerSystem (Uses the RequestStateChange ability)

#1 does not remove the VF before sending the hibernate message to the VM via hv_utils, but #2 does.

With both #1 and #2, during resume, the host offers the vPCI vmbus device before the vmbus_onoffers_delivered() is called. Whether or not VFs are removed doesn't matter here, because during resume the first fresh kernel always requests the VF device, meaning it has become a boot-time device when the 'old' kernel is resuming back. So the issue we are discussing will not happen in practice and the patch won't break things and won't print spurious warnings. If its OK, please let me know, I'll then proceed with v3.

From: Naman Jain <[email protected]> Sent: Sunday, November 10, 2024 9:44 PM

Quoted 6 lines · Naman Jain

Quoted 4 lines · Michael Kelley

Quoted 2 lines

From: Naman Jain <[email protected]> Sent: Tuesday, October 29, 2024 1:02 AM

Quoted 1 line

[snip]

Quoted 79 lines

Quoted 67 lines

Quoted 26 lines

Quoted 10 lines

drivers/hv/vmbus_drv.c +6 -0

@@ -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. */

drivers/hv/vmbus_drv.c static int vmbus_bus_resume(struct device *dev) · line 2513

Quoted 10 lines

drivers/hv/vmbus_drv.c +10 -0

+        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);
+

Dexuan and John have explained how in Azure VMs, there should not be any VFs assigned to the VM at the time of hibernation. So the above check for missing offers does not trigger an error message due to VF offers coming after the all-offers-received message.

Quoted 3 lines

But what about the case of a VM running on a local Hyper-V? I'm not completely clear, but in that case I don't think any VFs are removed before the hibernation, especially for VM-initiated hibernation. It's

I am not sure about this behavior. I have requested Dexuan offline for a comment.

Quoted 10 lines

a reasonable scenario to later resume that same VM, with the same VF assigned to the VM. Because of the way current code counts the offers, vmbus_bus_resume() waits for the VF to be offered again, and all the channels get correct post-resume relids. But the changes in this patch set break that scenario. Since vmbus_bus_resume() now proceeds before the VF offer arrives, hv_pci_resume() calling vmbus_open() could use the pre-hibernation relid for the VF and break things. Certainly the "not present after resume" error message would be spurious.

Quoted 5 lines

Maybe the focus here is Azure, and it's tolerable for the local Hyper-V case with a VF to not work pending later fixes. But I thought I'd call out the potential issue (assuming my thinking is correct).

Michael

IIUC, below scenarios can happen based on your comment-

Case 1:
VF channel offer is received in time before hv_pci_resume() and there are no problems.

Case 2:
Resume proceeds just after getting ALLOFFERS_DELIVERED msg and a warning is printed that this VF channel is not present after resume. Then two scenarios can happen:
  Case 2.1:
  VF channel offer is received before hv_pci_resume() and things work fine. Warning printed is spurious.
  Case 2.2:
  VM channel offer is not received before hv_pci_resume() and relid is not yet restored by onoffer. This is a problem. Warning is printed in this case for missing offer.

Quoted 7 lines

I think it all depends on whether or not VFs are removed in local HyperV VMs. I'll try to get this information. Thanks for pointing this out.

Regards,
Naman

Hi Michael,
I discussed with Dexuan and we tried these scenarios. Here are the observations:

Quoted 6 lines

For the two ways of host initiated hibernation:

#1: Invoke-Hibernate $vm -Device (Uses the guest shutdown component) OR
#2: Invoke-Hibernate $vm -ComputerSystem (Uses the RequestStateChange ability)

Question: What Powershell module provides "Invoke-Hibernate"? It's not present on my Windows 11 system that is running Hyper-V, and I can't find any documentation about it on the web. Or maybe Invoke-Hibernate is a Powershell *script*?

Quoted 5 lines

#1 does not remove the VF before sending the hibernate message to the VM via hv_utils, but #2 does.

Quoted 9 lines

With both #1 and #2, during resume, the host offers the vPCI vmbus device before the vmbus_onoffers_delivered() is called. Whether or not VFs are removed doesn't matter here, because during resume the first fresh kernel always requests the VF device, meaning it has become a boot-time device when the 'old' kernel is resuming back. So the issue we are discussing will not happen in practice and the patch won't break things and won't print spurious warnings. If its OK, please let me know, I'll then proceed with v3.

Ah, this is interesting. I'm assuming these are the details:

1) VM boots with the intent of resuming from hibernation (though Hyper-V doesn't know about that intent)

2)  Original fresh kernel is loaded and begins initialization
3)  VMBus offers come in for boot-time devices, which excludes SR-IOV VFs.
4)  ALLOFFERS_DELIVERED message comes in
5)  The storvsc driver initializes for the virtual disks on the VM
6)  Kernel initialization code finds and reads the swap space to see if a

hibernation image is present. If so, it reads in the hibernation image.
7) The suspend sequence is initiated (just like during hibernation) to shutdown the VMBus devices and terminate the VMBus connection.
8) Control is transferred to the previously read-in hibernation image
9) The hibernation image runs the resume sequence, which initiates a new VMBus connection and requests offers
10) VMBus offers come in for whatever VMBus devices were present when Step 7 initiated the suspend sequence. If a VF device was present at that time, an offer for that VF device will come in and will match up with the VF that was present in the VM at the time of hibernation.
11) ALLOFFERS_DELIVERED message comes in again for the
newly initiated VMBus connection.

The netvsc driver gets initialized *after* step 4, but we don't know exactly *when* relative to the storvsc driver. The netvsc driver must tell Hyper-V that it can handle an SR-IOV VF, and the VF offer is sent sometime after that. While this netvsc/VF sequence is happening, the storvsc driver is reading the hibernation image from swap (Step 6).

I think the sequence you describe works when reading the hibernation image from swap takes 10's of seconds, or even several minutes in an Azure VM with a remote disk. That gives plenty of time for the VF to get initialized and be fully present when Step 7 starts. But there's no *guarantee* that the VF is initialized by then. It's also not clear to me what action by the guest causes Hyper-V to treat the VF as "added to the VM" so that in Step 10 the VF offer is sent before ALLOFFERS_DELIVERED.

The sequence you describe also happens in an Azure VM, even if the VF is removed before hibernation. When the VF offer arrives during Step 10, it doesn't match with any VFs that were in the VM at the time of hibernation. It's treated as a new device, just like it would be if the offer arrived after ALLOFFERS_DELIVERED.

But it seems like there's still the risk of having a fast swap disk and a small hibernation image that can be read in a shorter amount of time than it takes to initialize the VF to the point that Hyper-V treats it as added to the VM. Without knowing what that point is, it's hard to assess the likelihood of that happening. Or maybe there's an interlock I'm not aware of that ensures Step 7 can't proceed while the netvsc/VF sequence is in progress.

So maybe it's best to proceed with this patch, and deal with the risk later when/if it becomes reality. I'm OK if you want to do that. This has been an interesting discussion that I'll try to capture in some high-level documentation about how Linux guests on Hyper-V do hibernation!

Michael

Quoted 96 lines

From: Naman Jain <[email protected]> Sent: Sunday, November 10, 2024 9:44 PM

Quoted 6 lines · Naman Jain

Quoted 4 lines · Michael Kelley

Quoted 2 lines

From: Naman Jain <[email protected]> Sent: Tuesday, October 29, 2024 1:02 AM

Quoted 1 line

[snip]

Quoted 85 lines

Quoted 67 lines

Quoted 26 lines

Quoted 10 lines

drivers/hv/vmbus_drv.c +6 -0

@@ -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. */

drivers/hv/vmbus_drv.c static int vmbus_bus_resume(struct device *dev) · line 2513

Quoted 10 lines

drivers/hv/vmbus_drv.c +10 -0

+        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);
+

Dexuan and John have explained how in Azure VMs, there should not be any VFs assigned to the VM at the time of hibernation. So the above check for missing offers does not trigger an error message due to VF offers coming after the all-offers-received message.

Quoted 3 lines

But what about the case of a VM running on a local Hyper-V? I'm not completely clear, but in that case I don't think any VFs are removed before the hibernation, especially for VM-initiated hibernation. It's

I am not sure about this behavior. I have requested Dexuan offline for a comment.

Quoted 10 lines

a reasonable scenario to later resume that same VM, with the same VF assigned to the VM. Because of the way current code counts the offers, vmbus_bus_resume() waits for the VF to be offered again, and all the channels get correct post-resume relids. But the changes in this patch set break that scenario. Since vmbus_bus_resume() now proceeds before the VF offer arrives, hv_pci_resume() calling vmbus_open() could use the pre-hibernation relid for the VF and break things. Certainly the "not present after resume" error message would be spurious.

Quoted 5 lines

Maybe the focus here is Azure, and it's tolerable for the local Hyper-V case with a VF to not work pending later fixes. But I thought I'd call out the potential issue (assuming my thinking is correct).

Michael

IIUC, below scenarios can happen based on your comment-

Case 1:
VF channel offer is received in time before hv_pci_resume() and there are no problems.

Case 2:
Resume proceeds just after getting ALLOFFERS_DELIVERED msg and a warning is printed that this VF channel is not present after resume. Then two scenarios can happen:
  Case 2.1:
  VF channel offer is received before hv_pci_resume() and things work fine. Warning printed is spurious.
  Case 2.2:
  VM channel offer is not received before hv_pci_resume() and relid is not yet restored by onoffer. This is a problem. Warning is printed in this case for missing offer.

Quoted 7 lines

I think it all depends on whether or not VFs are removed in local HyperV VMs. I'll try to get this information. Thanks for pointing this out.

Regards,
Naman

Hi Michael,
I discussed with Dexuan and we tried these scenarios. Here are the observations:

For the two ways of host initiated hibernation:

#1: Invoke-Hibernate $vm -Device (Uses the guest shutdown component) OR
#2: Invoke-Hibernate $vm -ComputerSystem (Uses the RequestStateChange ability)

Quoted 4 lines

Question: What Powershell module provides "Invoke-Hibernate"? It's not present on my Windows 11 system that is running Hyper-V, and I can't find any documentation about it on the web. Or maybe Invoke-Hibernate is a Powershell *script*?

Powertest is one of the internal packages, which has some commands to trigger host-initiated hibernation. I should probably have mentioned that in my original comment.

Quoted 24 lines

Quoted 5 lines

#1 does not remove the VF before sending the hibernate message to the VM via hv_utils, but #2 does.

Quoted 9 lines

With both #1 and #2, during resume, the host offers the vPCI vmbus device before the vmbus_onoffers_delivered() is called. Whether or not VFs are removed doesn't matter here, because during resume the first fresh kernel always requests the VF device, meaning it has become a boot-time device when the 'old' kernel is resuming back. So the issue we are discussing will not happen in practice and the patch won't break things and won't print spurious warnings. If its OK, please let me know, I'll then proceed with v3.

Ah, this is interesting. I'm assuming these are the details:

1) VM boots with the intent of resuming from hibernation (though Hyper-V doesn't know about that intent)

2)  Original fresh kernel is loaded and begins initialization
3)  VMBus offers come in for boot-time devices, which excludes SR-IOV VFs.
4)  ALLOFFERS_DELIVERED message comes in
5)  The storvsc driver initializes for the virtual disks on the VM
6)  Kernel initialization code finds and reads the swap space to see if a

Quoted 13 lines

hibernation image is present. If so, it reads in the hibernation image.
7) The suspend sequence is initiated (just like during hibernation) to shutdown the VMBus devices and terminate the VMBus connection.
8) Control is transferred to the previously read-in hibernation image
9) The hibernation image runs the resume sequence, which initiates a new VMBus connection and requests offers
10) VMBus offers come in for whatever VMBus devices were present when Step 7 initiated the suspend sequence. If a VF device was present at that time, an offer for that VF device will come in and will match up with the VF that was present in the VM at the time of hibernation.
11) ALLOFFERS_DELIVERED message comes in again for the
newly initiated VMBus connection.

3), 4) works differently IMO. There is no request_for_offers, or ALLOFFERS_DELIVERED for fresh kernel. Otherwise on adding the prints in kernel, we should have seen these function calls *twice* in one hibernation-resume cycle. But that is not the case.

When the older/original kernel boots up, and requests offers, it gets those VF offers again as part of boot time offers, and then ALLOFFERS_DELIVERED msg comes. I'm still trying to figure out how fresh kernel requests for VF offers or if it gets those offers automatically from the host. I will update my findings so that it can be put up in documentation which you mentioned.

Quoted 7 lines

The netvsc driver gets initialized *after* step 4, but we don't know exactly *when* relative to the storvsc driver. The netvsc driver must tell Hyper-V that it can handle an SR-IOV VF, and the VF offer is sent sometime after that. While this netvsc/VF sequence is happening, the storvsc driver is reading the hibernation image from swap (Step 6).

Maybe this is how fresh kernel gets the offers for VF devices.

Quoted 23 lines

I think the sequence you describe works when reading the hibernation image from swap takes 10's of seconds, or even several minutes in an Azure VM with a remote disk. That gives plenty of time for the VF to get initialized and be fully present when Step 7 starts. But there's no *guarantee* that the VF is initialized by then. It's also not clear to me what action by the guest causes Hyper-V to treat the VF as "added to the VM" so that in Step 10 the VF offer is sent before ALLOFFERS_DELIVERED.

The sequence you describe also happens in an Azure VM, even if the VF is removed before hibernation. When the VF offer arrives during Step 10, it doesn't match with any VFs that were in the VM at the time of hibernation. It's treated as a new device, just like it would be if the offer arrived after ALLOFFERS_DELIVERED.

But it seems like there's still the risk of having a fast swap disk and a small hibernation image that can be read in a shorter amount of time than it takes to initialize the VF to the point that Hyper-V treats it as added to the VM. Without knowing what that point is, it's hard to assess the likelihood of that happening. Or maybe there's an interlock I'm not aware of that ensures Step 7 can't proceed while the netvsc/VF sequence is in progress.

Quoted 7 lines

So maybe it's best to proceed with this patch, and deal with the risk later when/if it becomes reality. I'm OK if you want to do that. This has been an interesting discussion that I'll try to capture in some high-level documentation about how Linux guests on Hyper-V do hibernation!

Michael

I have sent v3 with the changes we discussed.

Regards,
Naman

From: Naman Jain <[email protected]> Sent: Wednesday, November 13, 2024 12:47 AM

Quoted 11 lines · Michael Kelley

Quoted 9 lines

From: Naman Jain <[email protected]> Sent: Sunday, November 10, 2024 9:44 PM

Quoted 6 lines · Naman Jain

Quoted 4 lines · Michael Kelley

Quoted 2 lines

From: Naman Jain <[email protected]> Sent: Tuesday, October 29, 2024 1:02 AM

Quoted 1 line

[snip]

Quoted 22 lines

Quoted 21 lines

1) VM boots with the intent of resuming from hibernation (though Hyper-V doesn't know about that intent)

2)  Original fresh kernel is loaded and begins initialization
3)  VMBus offers come in for boot-time devices, which excludes SR-IOV VFs.
4)  ALLOFFERS_DELIVERED message comes in
5)  The storvsc driver initializes for the virtual disks on the VM
6)  Kernel initialization code finds and reads the swap space to see if a

hibernation image is present. If so, it reads in the hibernation image.
7) The suspend sequence is initiated (just like during hibernation) to shutdown the VMBus devices and terminate the VMBus connection.
8) Control is transferred to the previously read-in hibernation image
9) The hibernation image runs the resume sequence, which initiates a new VMBus connection and requests offers
10) VMBus offers come in for whatever VMBus devices were present when Step 7 initiated the suspend sequence. If a VF device was present at that time, an offer for that VF device will come in and will match up with the VF that was present in the VM at the time of hibernation.
11) ALLOFFERS_DELIVERED message comes in again for the
newly initiated VMBus connection.

Quoted 5 lines

3), 4) works differently IMO. There is no request_for_offers, or ALLOFFERS_DELIVERED for fresh kernel. Otherwise on adding the prints in kernel, we should have seen these function calls *twice* in one hibernation-resume cycle. But that is not the case.

Quoted 6 lines

When the older/original kernel boots up, and requests offers, it gets those VF offers again as part of boot time offers, and then ALLOFFERS_DELIVERED msg comes. I'm still trying to figure out how fresh kernel requests for VF offers or if it gets those offers automatically from the host. I will update my findings so that it can be put up in documentation which you mentioned.

Hmmm. I'm not sure what might be happening. I'll be interested in what you find. I do indeed want to call out the details in my documentation. And I'll also try to repro myself.

Michael

Quoted 47 lines

Quoted 7 lines

The netvsc driver gets initialized *after* step 4, but we don't know exactly *when* relative to the storvsc driver. The netvsc driver must tell Hyper-V that it can handle an SR-IOV VF, and the VF offer is sent sometime after that. While this netvsc/VF sequence is happening, the storvsc driver is reading the hibernation image from swap (Step 6).

Maybe this is how fresh kernel gets the offers for VF devices.

Quoted 23 lines

I think the sequence you describe works when reading the hibernation image from swap takes 10's of seconds, or even several minutes in an Azure VM with a remote disk. That gives plenty of time for the VF to get initialized and be fully present when Step 7 starts. But there's no *guarantee* that the VF is initialized by then. It's also not clear to me what action by the guest causes Hyper-V to treat the VF as "added to the VM" so that in Step 10 the VF offer is sent before ALLOFFERS_DELIVERED.

The sequence you describe also happens in an Azure VM, even if the VF is removed before hibernation. When the VF offer arrives during Step 10, it doesn't match with any VFs that were in the VM at the time of hibernation. It's treated as a new device, just like it would be if the offer arrived after ALLOFFERS_DELIVERED.

But it seems like there's still the risk of having a fast swap disk and a small hibernation image that can be read in a shorter amount of time than it takes to initialize the VF to the point that Hyper-V treats it as added to the VM. Without knowing what that point is, it's hard to assess the likelihood of that happening. Or maybe there's an interlock I'm not aware of that ensures Step 7 can't proceed while the netvsc/VF sequence is in progress.

Quoted 7 lines

So maybe it's best to proceed with this patch, and deal with the risk later when/if it becomes reality. I'm OK if you want to do that. This has been an interesting discussion that I'll try to capture in some high-level documentation about how Linux guests on Hyper-V do hibernation!

Michael

I have sent v3 with the changes we discussed.

Regards,
Naman