MFC r292861:

hyperv: vmbus: run non-blocking message handlers in vmbus_msg_swintr()

We'll remove the per-channel control_work_queue because it can't properly
do serialization of message handling, e.g., when there are 2 NIC devices,
vmbus_channel_on_offer() -> hv_queue_work_item() has a race condition:
for an SMP VM, vmbus_channel_process_offer() can run concurrently on
different CPUs and if the second NIC's
vmbus_channel_process_offer() -> hv_vmbus_child_device_register() runs
first, the second NIC's name will be hn0 and the first NIC's name will
be hn1!

We can fix the race condition by removing the per-channel control_work_queue
and run all the message handlers in the global
hv_vmbus_g_connection.work_queue -- we'll do this in the next patch.

With the coming next patch, we have to run the non-blocking handlers
directly in the kernel thread vmbus_msg_swintr(), because the special
handling of sub-channel: when a sub-channel (e.g., of the storvsc driver)
is received and being handled in vmbus_channel_on_offer() running on the
global hv_vmbus_g_connection.work_queue, vmbus_channel_process_offer()
invokes channel->sc_creation_callback, i.e., storvsc_handle_sc_creation,
and the callback will invoke hv_vmbus_channel_open() -> hv_vmbus_post_message
and expect a further reply from the host, but the handling of the further
messag can't be done because the current message's handling hasn't finished
yet; as result, hv_vmbus_channel_open() -> sema_timedwait() will time out
and th device can't work.

Also renamed the handler type from hv_pfn_channel_msg_handler to
vmbus_msg_handler: the 'pfn' and 'channel' in the old name make no sense.

Submitted by: Dexuan Cui <decui microsoft com>
Reviewed by:  royger
Differential Revision:        https://reviews.freebsd.org/D4596

MFC r292859:

hyperv: vmbus: remove the per-channel control_work_queue

Now vmbus_channel_on_offer() -> vmbus_channel_process_offer() can
safely run on the global hv_vmbus_g_connection.work_queue now.

We remove the per-channel control_work_queue to achieve the proper
serialization of the message handling.

I removed the bogus TODO in vmbus_channel_on_offer(): a vmbus offer
can only come from the parent partition, i.e., the host.

PR:           kern/205156
Submitted by: Dexuan Cui <decui microsoft com>
Reviewed by:  Howard Su <howard0su gmail com>, delphij
Differential Revision:        https://reviews.freebsd.org/D4597

4 files changed, 67 insertions, 59 deletions

diff --git a/sys/dev/hyperv/include/hyperv.h b/sys/dev/hyperv/include/hyperv.h
index 6727503..b5600ba 100644
--- a/sys/dev/hyperv/include/hyperv.h
+++ b/sys/dev/hyperv/include/hyperv.h
@@ -759,7 +759,6 @@ typedef struct hv_vmbus_channel {
 	hv_vmbus_ring_buffer_info	inbound;
 
 	struct mtx			inbound_lock;
-	hv_vmbus_handle			control_work_queue;
 
 	hv_vmbus_pfn_channel_callback	on_channel_callback;
 	void*				channel_callback_context;
diff --git a/sys/dev/hyperv/vmbus/hv_channel_mgmt.c b/sys/dev/hyperv/vmbus/hv_channel_mgmt.c
index d17d696..c7f3538 100644
--- a/sys/dev/hyperv/vmbus/hv_channel_mgmt.c
+++ b/sys/dev/hyperv/vmbus/hv_channel_mgmt.c
@@ -34,13 +34,6 @@ __FBSDID("$FreeBSD$");
 
 #include "hv_vmbus_priv.h"
 
-typedef void (*hv_pfn_channel_msg_handler)(hv_vmbus_channel_msg_header* msg);
-
-typedef struct hv_vmbus_channel_msg_table_entry {
-	hv_vmbus_channel_msg_type    messageType;
-	hv_pfn_channel_msg_handler   messageHandler;
-} hv_vmbus_channel_msg_table_entry;
-
 /*
  * Internal functions
  */
@@ -52,36 +45,46 @@ static void vmbus_channel_on_gpadl_created(hv_vmbus_channel_msg_header* hdr);
 static void vmbus_channel_on_gpadl_torndown(hv_vmbus_channel_msg_header* hdr);
 static void vmbus_channel_on_offers_delivered(hv_vmbus_channel_msg_header* hdr);
 static void vmbus_channel_on_version_response(hv_vmbus_channel_msg_header* hdr);
-static void vmbus_channel_process_offer(void *context);
 
 /**
  * Channel message dispatch table
  */
 hv_vmbus_channel_msg_table_entry
     g_channel_message_table[HV_CHANNEL_MESSAGE_COUNT] = {
-	{ HV_CHANNEL_MESSAGE_INVALID, NULL },
-	{ HV_CHANNEL_MESSAGE_OFFER_CHANNEL, vmbus_channel_on_offer },
+	{ HV_CHANNEL_MESSAGE_INVALID,
+		0, NULL },
+	{ HV_CHANNEL_MESSAGE_OFFER_CHANNEL,
+		0, vmbus_channel_on_offer },
 	{ HV_CHANNEL_MESSAGE_RESCIND_CHANNEL_OFFER,
-		vmbus_channel_on_offer_rescind },
-	{ HV_CHANNEL_MESSAGE_REQUEST_OFFERS, NULL },
+		0, vmbus_channel_on_offer_rescind },
+	{ HV_CHANNEL_MESSAGE_REQUEST_OFFERS,
+		0, NULL },
 	{ HV_CHANNEL_MESSAGE_ALL_OFFERS_DELIVERED,
-		vmbus_channel_on_offers_delivered },
-	{ HV_CHANNEL_MESSAGE_OPEN_CHANNEL, NULL },
+		1, vmbus_channel_on_offers_delivered },
+	{ HV_CHANNEL_MESSAGE_OPEN_CHANNEL,
+		0, NULL },
 	{ HV_CHANNEL_MESSAGE_OPEN_CHANNEL_RESULT,
-		vmbus_channel_on_open_result },
-	{ HV_CHANNEL_MESSAGE_CLOSE_CHANNEL, NULL },
-	{ HV_CHANNEL_MESSAGEL_GPADL_HEADER, NULL },
-	{ HV_CHANNEL_MESSAGE_GPADL_BODY, NULL },
+		1, vmbus_channel_on_open_result },
+	{ HV_CHANNEL_MESSAGE_CLOSE_CHANNEL,
+		0, NULL },
+	{ HV_CHANNEL_MESSAGEL_GPADL_HEADER,
+		0, NULL },
+	{ HV_CHANNEL_MESSAGE_GPADL_BODY,
+		0, NULL },
 	{ HV_CHANNEL_MESSAGE_GPADL_CREATED,
-		vmbus_channel_on_gpadl_created },
-	{ HV_CHANNEL_MESSAGE_GPADL_TEARDOWN, NULL },
+		1, vmbus_channel_on_gpadl_created },
+	{ HV_CHANNEL_MESSAGE_GPADL_TEARDOWN,
+		0, NULL },
 	{ HV_CHANNEL_MESSAGE_GPADL_TORNDOWN,
-		vmbus_channel_on_gpadl_torndown },
-	{ HV_CHANNEL_MESSAGE_REL_ID_RELEASED, NULL },
-	{ HV_CHANNEL_MESSAGE_INITIATED_CONTACT, NULL },
+		1, vmbus_channel_on_gpadl_torndown },
+	{ HV_CHANNEL_MESSAGE_REL_ID_RELEASED,
+		0, NULL },
+	{ HV_CHANNEL_MESSAGE_INITIATED_CONTACT,
+		0, NULL },
 	{ HV_CHANNEL_MESSAGE_VERSION_RESPONSE,
-		vmbus_channel_on_version_response },
-	{ HV_CHANNEL_MESSAGE_UNLOAD, NULL }
+		1, vmbus_channel_on_version_response },
+	{ HV_CHANNEL_MESSAGE_UNLOAD,
+		0, NULL }
 };
 
 
@@ -209,15 +212,6 @@ hv_queue_work_item(
 	return (taskqueue_enqueue(wq->queue, &w->work));
 }
 
-/**
- * @brief Rescind the offer by initiating a device removal
- */
-static void
-vmbus_channel_process_rescind_offer(void *context)
-{
-	hv_vmbus_channel* channel = (hv_vmbus_channel*) context;
-	hv_vmbus_child_device_unregister(channel->device);
-}
 
 /**
  * @brief Allocate and initialize a vmbus channel object
@@ -240,14 +234,6 @@ hv_vmbus_allocate_channel(void)
 
 	TAILQ_INIT(&channel->sc_list_anchor);
 
-	channel->control_work_queue = hv_work_queue_create("control");
-
-	if (channel->control_work_queue == NULL) {
-	    mtx_destroy(&channel->inbound_lock);
-	    free(channel, M_DEVBUF);
-	    return (NULL);
-	}
-
 	return (channel);
 }
 
@@ -258,7 +244,6 @@ static inline void
 ReleaseVmbusChannel(void *context)
 {
 	hv_vmbus_channel* channel = (hv_vmbus_channel*) context;
-	hv_work_queue_close(channel->control_work_queue);
 	free(channel, M_DEVBUF);
 }
 
@@ -284,14 +269,12 @@ hv_vmbus_free_vmbus_channel(hv_vmbus_channel* channel)
  * associated with this offer
  */
 static void
-vmbus_channel_process_offer(void *context)
+vmbus_channel_process_offer(hv_vmbus_channel *new_channel)
 {
-	hv_vmbus_channel*	new_channel;
 	boolean_t		f_new;
 	hv_vmbus_channel*	channel;
 	int			ret;
 
-	new_channel = (hv_vmbus_channel*) context;
 	f_new = TRUE;
 	channel = NULL;
 
@@ -524,11 +507,7 @@ vmbus_channel_on_offer(hv_vmbus_channel_msg_header* hdr)
 	new_channel->monitor_group = (uint8_t) offer->monitor_id / 32;
 	new_channel->monitor_bit = (uint8_t) offer->monitor_id % 32;
 
-	/* TODO: Make sure the offer comes from our parent partition */
-	hv_queue_work_item(
-	    new_channel->control_work_queue,
-	    vmbus_channel_process_offer,
-	    new_channel);
+	vmbus_channel_process_offer(new_channel);
 }
 
 /**
@@ -549,8 +528,7 @@ vmbus_channel_on_offer_rescind(hv_vmbus_channel_msg_header* hdr)
 	if (channel == NULL) 
 	    return;
 
-	hv_queue_work_item(channel->control_work_queue,
-	    vmbus_channel_process_rescind_offer, channel);
+	hv_vmbus_child_device_unregister(channel->device);
 }
 
 /**
diff --git a/sys/dev/hyperv/vmbus/hv_vmbus_drv_freebsd.c b/sys/dev/hyperv/vmbus/hv_vmbus_drv_freebsd.c
index 91813bb..f7eae26 100644
--- a/sys/dev/hyperv/vmbus/hv_vmbus_drv_freebsd.c
+++ b/sys/dev/hyperv/vmbus/hv_vmbus_drv_freebsd.c
@@ -76,8 +76,12 @@ vmbus_msg_swintr(void *arg)
 {
 	int 			cpu;
 	void*			page_addr;
+	hv_vmbus_channel_msg_header	 *hdr;
+	hv_vmbus_channel_msg_table_entry *entry;
+	hv_vmbus_channel_msg_type msg_type;
 	hv_vmbus_message*	msg;
 	hv_vmbus_message*	copied;
+	static bool warned	= false;
 
 	cpu = (int)(long)arg;
 	KASSERT(cpu <= mp_maxid, ("VMBUS: vmbus_msg_swintr: "
@@ -87,9 +91,24 @@ vmbus_msg_swintr(void *arg)
 	msg = (hv_vmbus_message*) page_addr + HV_VMBUS_MESSAGE_SINT;
 
 	for (;;) {
-		if (msg->header.message_type == HV_MESSAGE_TYPE_NONE) {
+		if (msg->header.message_type == HV_MESSAGE_TYPE_NONE)
 			break; /* no message */
-		} else {
+
+		hdr = (hv_vmbus_channel_msg_header *)msg->u.payload;
+		msg_type = hdr->message_type;
+
+		if (msg_type >= HV_CHANNEL_MESSAGE_COUNT && !warned) {
+			warned = true;
+			printf("VMBUS: unknown message type = %d\n", msg_type);
+			goto handled;
+		}
+
+		entry = &g_channel_message_table[msg_type];
+
+		if (entry->handler_no_sleep)
+			entry->messageHandler(hdr);
+		else {
+
 			copied = malloc(sizeof(hv_vmbus_message),
 					M_DEVBUF, M_NOWAIT);
 			KASSERT(copied != NULL,
@@ -97,11 +116,13 @@ vmbus_msg_swintr(void *arg)
 					" hv_vmbus_message!"));
 			if (copied == NULL)
 				continue;
+
 			memcpy(copied, msg, sizeof(hv_vmbus_message));
 			hv_queue_work_item(hv_vmbus_g_connection.work_queue,
-			hv_vmbus_on_channel_message, copied);
-	    }
-
+					   hv_vmbus_on_channel_message,
+					   copied);
+		}
+handled:
 	    msg->header.message_type = HV_MESSAGE_TYPE_NONE;
 
 	    /*
diff --git a/sys/dev/hyperv/vmbus/hv_vmbus_priv.h b/sys/dev/hyperv/vmbus/hv_vmbus_priv.h
index faa6dec..0503d06 100644
--- a/sys/dev/hyperv/vmbus/hv_vmbus_priv.h
+++ b/sys/dev/hyperv/vmbus/hv_vmbus_priv.h
@@ -586,6 +586,16 @@ typedef enum {
 extern hv_vmbus_context		hv_vmbus_g_context;
 extern hv_vmbus_connection	hv_vmbus_g_connection;
 
+typedef void (*vmbus_msg_handler)(hv_vmbus_channel_msg_header *msg);
+
+typedef struct hv_vmbus_channel_msg_table_entry {
+	hv_vmbus_channel_msg_type    messageType;
+
+	bool   handler_no_sleep; /* true: the handler doesn't sleep */
+	vmbus_msg_handler   messageHandler;
+} hv_vmbus_channel_msg_table_entry;
+
+extern hv_vmbus_channel_msg_table_entry	g_channel_message_table[];
 
 /*
  * Private, VM Bus functions