All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 00/10] Drivers: hv: Miscellaneous fixes.
@ 2015-10-30  1:12 K. Y. Srinivasan
  2015-10-30  1:12 ` [PATCH V2 01/10] Drivers: hv: util: Increase the timeout for util services K. Y. Srinivasan
  0 siblings, 1 reply; 15+ messages in thread
From: K. Y. Srinivasan @ 2015-10-30  1:12 UTC (permalink / raw)
  To: gregkh, linux-kernel, devel, olaf, apw, vkuznets, jasowang
  Cc: K. Y. Srinivasan

Miscellaneous fixes. In this version I have addressed review comments
from Vitaly Kuznetsov <vkuznets@redhat.com>. Also fixed up the
authorship of the patch: drivers/hv: cleanup synic msrs if vmbus connect failed

Andrey Smetanin (1):
  drivers/hv: cleanup synic msrs if vmbus connect failed

Jake Oshins (3):
  drivers:hv: Export a function that maps Linux CPU num onto Hyper-V
    proc num
  drivers:hv: Export the API to invoke a hypercall on Hyper-V
  drivers:hv: Define the channel type for Hyper-V PCI Express
    pass-through

K. Y. Srinivasan (1):
  Drivers: hv: util: Increase the timeout for util services

Olaf Hering (5):
  Drivers: hv: utils: run polling callback always in interrupt context
  tools: hv: report ENOSPC errors in hv_fcopy_daemon
  tools: hv: remove repeated HV_FCOPY string
  Drivers: hv: util: catch allocation errors
  Drivers: hv: utils: use memdup_user in hvt_op_write

 drivers/hv/channel_mgmt.c       |    3 +++
 drivers/hv/hv.c                 |   20 ++++++++++----------
 drivers/hv/hv_fcopy.c           |   37 +++++++++++++++----------------------
 drivers/hv/hv_kvp.c             |   31 ++++++++++++-------------------
 drivers/hv/hv_snapshot.c        |   29 +++++++++++------------------
 drivers/hv/hv_utils_transport.c |   18 ++++++++++--------
 drivers/hv/hyperv_vmbus.h       |   13 +++++++------
 drivers/hv/vmbus_drv.c          |   21 ++++++++++++++++++++-
 include/linux/hyperv.h          |   14 ++++++++++++++
 include/uapi/linux/hyperv.h     |    1 +
 tools/hv/hv_fcopy_daemon.c      |   24 +++++++++++++++++++-----
 11 files changed, 122 insertions(+), 89 deletions(-)

-- 
1.7.4.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH V2 01/10] Drivers: hv: util: Increase the timeout for util services
  2015-10-30  1:12 [PATCH V2 00/10] Drivers: hv: Miscellaneous fixes K. Y. Srinivasan
@ 2015-10-30  1:12 ` K. Y. Srinivasan
  2015-10-30  1:12   ` [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in interrupt context K. Y. Srinivasan
                     ` (8 more replies)
  0 siblings, 9 replies; 15+ messages in thread
From: K. Y. Srinivasan @ 2015-10-30  1:12 UTC (permalink / raw)
  To: gregkh, linux-kernel, devel, olaf, apw, vkuznets, jasowang
  Cc: K. Y. Srinivasan

Util services such as KVP and FCOPY need assistance from daemon's running
in user space. Increase the timeout so we don't prematurely terminate
the transaction in the kernel. Host sets up a 60 second timeout for
all util driver transactions. The host will retry the transaction if it
times out. Set the guest timeout at 30 seconds.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
	V2: Added additional details to commit log - Olaf	

 drivers/hv/hv_fcopy.c     |    3 ++-
 drivers/hv/hv_kvp.c       |    3 ++-
 drivers/hv/hyperv_vmbus.h |    5 +++++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
index db4b887..bbdec50 100644
--- a/drivers/hv/hv_fcopy.c
+++ b/drivers/hv/hv_fcopy.c
@@ -275,7 +275,8 @@ void hv_fcopy_onchannelcallback(void *context)
 		 * Send the information to the user-level daemon.
 		 */
 		schedule_work(&fcopy_send_work);
-		schedule_delayed_work(&fcopy_timeout_work, 5*HZ);
+		schedule_delayed_work(&fcopy_timeout_work,
+				      HV_UTIL_TIMEOUT * HZ);
 		return;
 	}
 	icmsghdr->icflags = ICMSGHDRFLAG_TRANSACTION | ICMSGHDRFLAG_RESPONSE;
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 74c38a9..e6aa33a 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -668,7 +668,8 @@ void hv_kvp_onchannelcallback(void *context)
 			 * user-mode not responding.
 			 */
 			schedule_work(&kvp_sendkey_work);
-			schedule_delayed_work(&kvp_timeout_work, 5*HZ);
+			schedule_delayed_work(&kvp_timeout_work,
+					      HV_UTIL_TIMEOUT * HZ);
 
 			return;
 
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 3d70e36..f26599b 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -31,6 +31,11 @@
 #include <linux/hyperv.h>
 
 /*
+ * Timeout for services such as KVP and fcopy.
+ */
+#define HV_UTIL_TIMEOUT 30
+
+/*
  * The below CPUID leaves are present if VersionAndFeatures.HypervisorPresent
  * is set by CPUID(HVCPUID_VERSION_FEATURES).
  */
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in interrupt context
  2015-10-30  1:12 ` [PATCH V2 01/10] Drivers: hv: util: Increase the timeout for util services K. Y. Srinivasan
@ 2015-10-30  1:12   ` K. Y. Srinivasan
  2015-12-10 10:18     ` Dexuan Cui
  2015-10-30  1:12   ` [PATCH V2 03/10] tools: hv: report ENOSPC errors in hv_fcopy_daemon K. Y. Srinivasan
                     ` (7 subsequent siblings)
  8 siblings, 1 reply; 15+ messages in thread
From: K. Y. Srinivasan @ 2015-10-30  1:12 UTC (permalink / raw)
  To: gregkh, linux-kernel, devel, olaf, apw, vkuznets, jasowang
  Cc: K. Y. Srinivasan

From: Olaf Hering <olaf@aepfle.de>

All channel interrupts are bound to specific VCPUs in the guest
at the point channel is created. While currently, we invoke the
polling function on the correct CPU (the CPU to which the channel
is bound to) in some cases we may run the polling function in
a non-interrupt context. This  potentially can cause an issue as the
polling function can be interrupted by the channel callback function.
Fix the issue by running the polling function on the appropriate CPU
at interrupt level. Additional details of the issue being addressed by
this patch are given below:

Currently hv_fcopy_onchannelcallback is called from interrupts and also
via the ->write function of hv_utils. Since the used global variables to
maintain state are not thread safe the state can get out of sync.
This affects the variable state as well as the channel inbound buffer.

As suggested by KY adjust hv_poll_channel to always run the given
callback on the cpu which the channel is bound to. This avoids the need
for locking because all the util services are single threaded and only
one transaction is active at any given point in time.

Additionally, remove the context variable, they will always be the same as
recv_channel.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
	V2: Added the check to catch unsolicited daemon writes - Vitaly

 drivers/hv/hv_fcopy.c     |   34 +++++++++++++---------------------
 drivers/hv/hv_kvp.c       |   28 ++++++++++------------------
 drivers/hv/hv_snapshot.c  |   29 +++++++++++------------------
 drivers/hv/hyperv_vmbus.h |    6 +-----
 4 files changed, 35 insertions(+), 62 deletions(-)

diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
index bbdec50..06a7166 100644
--- a/drivers/hv/hv_fcopy.c
+++ b/drivers/hv/hv_fcopy.c
@@ -51,7 +51,6 @@ static struct {
 	struct hv_fcopy_hdr  *fcopy_msg; /* current message */
 	struct vmbus_channel *recv_channel; /* chn we got the request */
 	u64 recv_req_id; /* request ID. */
-	void *fcopy_context; /* for the channel callback */
 } fcopy_transaction;
 
 static void fcopy_respond_to_host(int error);
@@ -67,6 +66,13 @@ static struct hvutil_transport *hvt;
  */
 static int dm_reg_value;
 
+static void fcopy_poll_wrapper(void *channel)
+{
+	/* Transaction is finished, reset the state here to avoid races. */
+	fcopy_transaction.state = HVUTIL_READY;
+	hv_fcopy_onchannelcallback(channel);
+}
+
 static void fcopy_timeout_func(struct work_struct *dummy)
 {
 	/*
@@ -74,13 +80,7 @@ static void fcopy_timeout_func(struct work_struct *dummy)
 	 * process the pending transaction.
 	 */
 	fcopy_respond_to_host(HV_E_FAIL);
-
-	/* Transaction is finished, reset the state. */
-	if (fcopy_transaction.state > HVUTIL_READY)
-		fcopy_transaction.state = HVUTIL_READY;
-
-	hv_poll_channel(fcopy_transaction.fcopy_context,
-			hv_fcopy_onchannelcallback);
+	hv_poll_channel(fcopy_transaction.recv_channel, fcopy_poll_wrapper);
 }
 
 static int fcopy_handle_handshake(u32 version)
@@ -108,9 +108,9 @@ static int fcopy_handle_handshake(u32 version)
 		return -EINVAL;
 	}
 	pr_debug("FCP: userspace daemon ver. %d registered\n", version);
+	/* Forward state for hv_fcopy_onchannelcallback */
 	fcopy_transaction.state = HVUTIL_READY;
-	hv_poll_channel(fcopy_transaction.fcopy_context,
-			hv_fcopy_onchannelcallback);
+	hv_poll_channel(fcopy_transaction.recv_channel, fcopy_poll_wrapper);
 	return 0;
 }
 
@@ -227,15 +227,8 @@ void hv_fcopy_onchannelcallback(void *context)
 	int util_fw_version;
 	int fcopy_srv_version;
 
-	if (fcopy_transaction.state > HVUTIL_READY) {
-		/*
-		 * We will defer processing this callback once
-		 * the current transaction is complete.
-		 */
-		fcopy_transaction.fcopy_context = context;
+	if (fcopy_transaction.state > HVUTIL_READY)
 		return;
-	}
-	fcopy_transaction.fcopy_context = NULL;
 
 	vmbus_recvpacket(channel, recv_buffer, PAGE_SIZE * 2, &recvlen,
 			 &requestid);
@@ -305,9 +298,8 @@ static int fcopy_on_msg(void *msg, int len)
 	if (cancel_delayed_work_sync(&fcopy_timeout_work)) {
 		fcopy_transaction.state = HVUTIL_USERSPACE_RECV;
 		fcopy_respond_to_host(*val);
-		fcopy_transaction.state = HVUTIL_READY;
-		hv_poll_channel(fcopy_transaction.fcopy_context,
-				hv_fcopy_onchannelcallback);
+		hv_poll_channel(fcopy_transaction.recv_channel,
+				fcopy_poll_wrapper);
 	}
 
 	return 0;
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index e6aa33a..2a3420c 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -66,7 +66,6 @@ static struct {
 	struct hv_kvp_msg  *kvp_msg; /* current message */
 	struct vmbus_channel *recv_channel; /* chn we got the request */
 	u64 recv_req_id; /* request ID. */
-	void *kvp_context; /* for the channel callback */
 } kvp_transaction;
 
 /*
@@ -94,6 +93,13 @@ static struct hvutil_transport *hvt;
  */
 #define HV_DRV_VERSION           "3.1"
 
+static void kvp_poll_wrapper(void *channel)
+{
+	/* Transaction is finished, reset the state here to avoid races. */
+	kvp_transaction.state = HVUTIL_READY;
+	hv_kvp_onchannelcallback(channel);
+}
+
 static void
 kvp_register(int reg_value)
 {
@@ -121,12 +127,7 @@ static void kvp_timeout_func(struct work_struct *dummy)
 	 */
 	kvp_respond_to_host(NULL, HV_E_FAIL);
 
-	/* Transaction is finished, reset the state. */
-	if (kvp_transaction.state > HVUTIL_READY)
-		kvp_transaction.state = HVUTIL_READY;
-
-	hv_poll_channel(kvp_transaction.kvp_context,
-			hv_kvp_onchannelcallback);
+	hv_poll_channel(kvp_transaction.recv_channel, kvp_poll_wrapper);
 }
 
 static int kvp_handle_handshake(struct hv_kvp_msg *msg)
@@ -218,9 +219,7 @@ static int kvp_on_msg(void *msg, int len)
 	 */
 	if (cancel_delayed_work_sync(&kvp_timeout_work)) {
 		kvp_respond_to_host(message, error);
-		kvp_transaction.state = HVUTIL_READY;
-		hv_poll_channel(kvp_transaction.kvp_context,
-				hv_kvp_onchannelcallback);
+		hv_poll_channel(kvp_transaction.recv_channel, kvp_poll_wrapper);
 	}
 
 	return 0;
@@ -596,15 +595,8 @@ void hv_kvp_onchannelcallback(void *context)
 	int util_fw_version;
 	int kvp_srv_version;
 
-	if (kvp_transaction.state > HVUTIL_READY) {
-		/*
-		 * We will defer processing this callback once
-		 * the current transaction is complete.
-		 */
-		kvp_transaction.kvp_context = context;
+	if (kvp_transaction.state > HVUTIL_READY)
 		return;
-	}
-	kvp_transaction.kvp_context = NULL;
 
 	vmbus_recvpacket(channel, recv_buffer, PAGE_SIZE * 4, &recvlen,
 			 &requestid);
diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index 815405f..a548ae4 100644
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -53,7 +53,6 @@ static struct {
 	struct vmbus_channel *recv_channel; /* chn we got the request */
 	u64 recv_req_id; /* request ID. */
 	struct hv_vss_msg  *msg; /* current message */
-	void *vss_context; /* for the channel callback */
 } vss_transaction;
 
 
@@ -74,6 +73,13 @@ static void vss_timeout_func(struct work_struct *dummy);
 static DECLARE_DELAYED_WORK(vss_timeout_work, vss_timeout_func);
 static DECLARE_WORK(vss_send_op_work, vss_send_op);
 
+static void vss_poll_wrapper(void *channel)
+{
+	/* Transaction is finished, reset the state here to avoid races. */
+	vss_transaction.state = HVUTIL_READY;
+	hv_vss_onchannelcallback(channel);
+}
+
 /*
  * Callback when data is received from user mode.
  */
@@ -86,12 +92,7 @@ static void vss_timeout_func(struct work_struct *dummy)
 	pr_warn("VSS: timeout waiting for daemon to reply\n");
 	vss_respond_to_host(HV_E_FAIL);
 
-	/* Transaction is finished, reset the state. */
-	if (vss_transaction.state > HVUTIL_READY)
-		vss_transaction.state = HVUTIL_READY;
-
-	hv_poll_channel(vss_transaction.vss_context,
-			hv_vss_onchannelcallback);
+	hv_poll_channel(vss_transaction.recv_channel, vss_poll_wrapper);
 }
 
 static int vss_handle_handshake(struct hv_vss_msg *vss_msg)
@@ -138,9 +139,8 @@ static int vss_on_msg(void *msg, int len)
 		if (cancel_delayed_work_sync(&vss_timeout_work)) {
 			vss_respond_to_host(vss_msg->error);
 			/* Transaction is finished, reset the state. */
-			vss_transaction.state = HVUTIL_READY;
-			hv_poll_channel(vss_transaction.vss_context,
-					hv_vss_onchannelcallback);
+			hv_poll_channel(vss_transaction.recv_channel,
+					vss_poll_wrapper);
 		}
 	} else {
 		/* This is a spurious call! */
@@ -238,15 +238,8 @@ void hv_vss_onchannelcallback(void *context)
 	struct icmsg_hdr *icmsghdrp;
 	struct icmsg_negotiate *negop = NULL;
 
-	if (vss_transaction.state > HVUTIL_READY) {
-		/*
-		 * We will defer processing this callback once
-		 * the current transaction is complete.
-		 */
-		vss_transaction.vss_context = context;
+	if (vss_transaction.state > HVUTIL_READY)
 		return;
-	}
-	vss_transaction.vss_context = NULL;
 
 	vmbus_recvpacket(channel, recv_buffer, PAGE_SIZE * 2, &recvlen,
 			 &requestid);
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index f26599b..40c0c855 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -769,11 +769,7 @@ static inline void hv_poll_channel(struct vmbus_channel *channel,
 	if (!channel)
 		return;
 
-	if (channel->target_cpu != smp_processor_id())
-		smp_call_function_single(channel->target_cpu,
-					 cb, channel, true);
-	else
-		cb(channel);
+	smp_call_function_single(channel->target_cpu, cb, channel, true);
 }
 
 enum hvutil_device_state {
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH V2 03/10] tools: hv: report ENOSPC errors in hv_fcopy_daemon
  2015-10-30  1:12 ` [PATCH V2 01/10] Drivers: hv: util: Increase the timeout for util services K. Y. Srinivasan
  2015-10-30  1:12   ` [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in interrupt context K. Y. Srinivasan
@ 2015-10-30  1:12   ` K. Y. Srinivasan
  2015-10-30  1:12   ` [PATCH V2 04/10] tools: hv: remove repeated HV_FCOPY string K. Y. Srinivasan
                     ` (6 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: K. Y. Srinivasan @ 2015-10-30  1:12 UTC (permalink / raw)
  To: gregkh, linux-kernel, devel, olaf, apw, vkuznets, jasowang
  Cc: K. Y. Srinivasan

From: Olaf Hering <olaf@aepfle.de>

Currently some "Unspecified error 0x80004005" is reported on the Windows
side if something fails. Handle the ENOSPC case and return
ERROR_DISK_FULL, which allows at least Copy-VMFile to report a meaning
full error.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 include/uapi/linux/hyperv.h |    1 +
 tools/hv/hv_fcopy_daemon.c  |   20 +++++++++++++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/hyperv.h b/include/uapi/linux/hyperv.h
index e4c0a35..e347b24 100644
--- a/include/uapi/linux/hyperv.h
+++ b/include/uapi/linux/hyperv.h
@@ -313,6 +313,7 @@ enum hv_kvp_exchg_pool {
 #define HV_INVALIDARG			0x80070057
 #define HV_GUID_NOTFOUND		0x80041002
 #define HV_ERROR_ALREADY_EXISTS		0x80070050
+#define HV_ERROR_DISK_FULL		0x80070070
 
 #define ADDR_FAMILY_NONE	0x00
 #define ADDR_FAMILY_IPV4	0x01
diff --git a/tools/hv/hv_fcopy_daemon.c b/tools/hv/hv_fcopy_daemon.c
index 5480e4e..f1d7426 100644
--- a/tools/hv/hv_fcopy_daemon.c
+++ b/tools/hv/hv_fcopy_daemon.c
@@ -37,12 +37,14 @@
 
 static int target_fd;
 static char target_fname[W_MAX_PATH];
+static unsigned long long filesize;
 
 static int hv_start_fcopy(struct hv_start_fcopy *smsg)
 {
 	int error = HV_E_FAIL;
 	char *q, *p;
 
+	filesize = 0;
 	p = (char *)smsg->path_name;
 	snprintf(target_fname, sizeof(target_fname), "%s/%s",
 		 (char *)smsg->path_name, (char *)smsg->file_name);
@@ -98,14 +100,26 @@ done:
 static int hv_copy_data(struct hv_do_fcopy *cpmsg)
 {
 	ssize_t bytes_written;
+	int ret = 0;
 
 	bytes_written = pwrite(target_fd, cpmsg->data, cpmsg->size,
 				cpmsg->offset);
 
-	if (bytes_written != cpmsg->size)
-		return HV_E_FAIL;
+	filesize += cpmsg->size;
+	if (bytes_written != cpmsg->size) {
+		switch (errno) {
+		case ENOSPC:
+			ret = HV_ERROR_DISK_FULL;
+			break;
+		default:
+			ret = HV_E_FAIL;
+			break;
+		}
+		syslog(LOG_ERR, "pwrite failed to write %llu bytes: %ld (%s)",
+		       filesize, (long)bytes_written, strerror(errno));
+	}
 
-	return 0;
+	return ret;
 }
 
 static int hv_copy_finished(void)
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH V2 04/10] tools: hv: remove repeated HV_FCOPY string
  2015-10-30  1:12 ` [PATCH V2 01/10] Drivers: hv: util: Increase the timeout for util services K. Y. Srinivasan
  2015-10-30  1:12   ` [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in interrupt context K. Y. Srinivasan
  2015-10-30  1:12   ` [PATCH V2 03/10] tools: hv: report ENOSPC errors in hv_fcopy_daemon K. Y. Srinivasan
@ 2015-10-30  1:12   ` K. Y. Srinivasan
  2015-10-30  1:12   ` [PATCH V2 05/10] Drivers: hv: util: catch allocation errors K. Y. Srinivasan
                     ` (5 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: K. Y. Srinivasan @ 2015-10-30  1:12 UTC (permalink / raw)
  To: gregkh, linux-kernel, devel, olaf, apw, vkuznets, jasowang
  Cc: K. Y. Srinivasan

From: Olaf Hering <olaf@aepfle.de>

HV_FCOPY is already used as identifier in syslog.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 tools/hv/hv_fcopy_daemon.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/hv/hv_fcopy_daemon.c b/tools/hv/hv_fcopy_daemon.c
index f1d7426..fdc9ca4 100644
--- a/tools/hv/hv_fcopy_daemon.c
+++ b/tools/hv/hv_fcopy_daemon.c
@@ -179,7 +179,7 @@ int main(int argc, char *argv[])
 	}
 
 	openlog("HV_FCOPY", 0, LOG_USER);
-	syslog(LOG_INFO, "HV_FCOPY starting; pid is:%d", getpid());
+	syslog(LOG_INFO, "starting; pid is:%d", getpid());
 
 	fcopy_fd = open("/dev/vmbus/hv_fcopy", O_RDWR);
 
@@ -215,7 +215,7 @@ int main(int argc, char *argv[])
 			}
 			kernel_modver = *(__u32 *)buffer;
 			in_handshake = 0;
-			syslog(LOG_INFO, "HV_FCOPY: kernel module version: %d",
+			syslog(LOG_INFO, "kernel module version: %d",
 			       kernel_modver);
 			continue;
 		}
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH V2 05/10] Drivers: hv: util: catch allocation errors
  2015-10-30  1:12 ` [PATCH V2 01/10] Drivers: hv: util: Increase the timeout for util services K. Y. Srinivasan
                     ` (2 preceding siblings ...)
  2015-10-30  1:12   ` [PATCH V2 04/10] tools: hv: remove repeated HV_FCOPY string K. Y. Srinivasan
@ 2015-10-30  1:12   ` K. Y. Srinivasan
  2015-10-30  1:12   ` [PATCH V2 06/10] Drivers: hv: utils: use memdup_user in hvt_op_write K. Y. Srinivasan
                     ` (4 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: K. Y. Srinivasan @ 2015-10-30  1:12 UTC (permalink / raw)
  To: gregkh, linux-kernel, devel, olaf, apw, vkuznets, jasowang
  Cc: K. Y. Srinivasan

From: Olaf Hering <olaf@aepfle.de>

Catch allocation errors in hvutil_transport_send.

Fixes: 14b50f80c32d ('Drivers: hv: util: introduce hv_utils_transport abstraction')

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/hv_utils_transport.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c
index 6a9d80a..1505ee6 100644
--- a/drivers/hv/hv_utils_transport.c
+++ b/drivers/hv/hv_utils_transport.c
@@ -204,9 +204,12 @@ int hvutil_transport_send(struct hvutil_transport *hvt, void *msg, int len)
 		goto out_unlock;
 	}
 	hvt->outmsg = kzalloc(len, GFP_KERNEL);
-	memcpy(hvt->outmsg, msg, len);
-	hvt->outmsg_len = len;
-	wake_up_interruptible(&hvt->outmsg_q);
+	if (hvt->outmsg) {
+		memcpy(hvt->outmsg, msg, len);
+		hvt->outmsg_len = len;
+		wake_up_interruptible(&hvt->outmsg_q);
+	} else
+		ret = -ENOMEM;
 out_unlock:
 	mutex_unlock(&hvt->outmsg_lock);
 	return ret;
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH V2 06/10] Drivers: hv: utils: use memdup_user in hvt_op_write
  2015-10-30  1:12 ` [PATCH V2 01/10] Drivers: hv: util: Increase the timeout for util services K. Y. Srinivasan
                     ` (3 preceding siblings ...)
  2015-10-30  1:12   ` [PATCH V2 05/10] Drivers: hv: util: catch allocation errors K. Y. Srinivasan
@ 2015-10-30  1:12   ` K. Y. Srinivasan
  2015-10-30  1:12   ` [PATCH V2 07/10] drivers/hv: cleanup synic msrs if vmbus connect failed K. Y. Srinivasan
                     ` (3 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: K. Y. Srinivasan @ 2015-10-30  1:12 UTC (permalink / raw)
  To: gregkh, linux-kernel, devel, olaf, apw, vkuznets, jasowang
  Cc: K. Y. Srinivasan

From: Olaf Hering <olaf@aepfle.de>

Use memdup_user to handle OOM.

Fixes: 14b50f80c32d ('Drivers: hv: util: introduce hv_utils_transport abstraction')

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/hv_utils_transport.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c
index 1505ee6..24b2766 100644
--- a/drivers/hv/hv_utils_transport.c
+++ b/drivers/hv/hv_utils_transport.c
@@ -80,11 +80,10 @@ static ssize_t hvt_op_write(struct file *file, const char __user *buf,
 
 	hvt = container_of(file->f_op, struct hvutil_transport, fops);
 
-	inmsg = kzalloc(count, GFP_KERNEL);
-	if (copy_from_user(inmsg, buf, count)) {
-		kfree(inmsg);
-		return -EFAULT;
-	}
+	inmsg = memdup_user(buf, count);
+	if (IS_ERR(inmsg))
+		return PTR_ERR(inmsg);
+
 	if (hvt->on_msg(inmsg, count))
 		return -EFAULT;
 	kfree(inmsg);
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH V2 07/10] drivers/hv: cleanup synic msrs if vmbus connect failed
  2015-10-30  1:12 ` [PATCH V2 01/10] Drivers: hv: util: Increase the timeout for util services K. Y. Srinivasan
                     ` (4 preceding siblings ...)
  2015-10-30  1:12   ` [PATCH V2 06/10] Drivers: hv: utils: use memdup_user in hvt_op_write K. Y. Srinivasan
@ 2015-10-30  1:12   ` K. Y. Srinivasan
  2015-10-30  1:12   ` [PATCH V2 08/10] drivers:hv: Export a function that maps Linux CPU num onto Hyper-V proc num K. Y. Srinivasan
                     ` (2 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: K. Y. Srinivasan @ 2015-10-30  1:12 UTC (permalink / raw)
  To: gregkh, linux-kernel, devel, olaf, apw, vkuznets, jasowang
  Cc: Andrey Smetanin, Denis V. Lunev, K. Y. Srinivasan, Haiyang Zhang

From: Andrey Smetanin <asmetanin@virtuozzo.com>

Before vmbus_connect() synic is setup per vcpu - this means
hypervisor receives writes at synic msr's and probably allocate
hypervisor resources per synic setup.

If vmbus_connect() failed for some reason it's neccessary to cleanup
synic setup by call hv_synic_cleanup() at each vcpu to get a chance
to free allocated resources by hypervisor per synic.

This patch does appropriate cleanup in case of vmbus_connect() failure.

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
CC: "K. Y. Srinivasan" <kys@microsoft.com>
CC: Haiyang Zhang <haiyangz@microsoft.com>
CC: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
	V2: Fixed up the name of the author (Denis)

 drivers/hv/vmbus_drv.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index f19b6f7..3297731 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -867,7 +867,7 @@ static int vmbus_bus_init(int irq)
 	on_each_cpu(hv_synic_init, NULL, 1);
 	ret = vmbus_connect();
 	if (ret)
-		goto err_alloc;
+		goto err_connect;
 
 	if (vmbus_proto_version > VERSION_WIN7)
 		cpu_hotplug_disable();
@@ -885,6 +885,8 @@ static int vmbus_bus_init(int irq)
 
 	return 0;
 
+err_connect:
+	on_each_cpu(hv_synic_cleanup, NULL, 1);
 err_alloc:
 	hv_synic_free();
 	hv_remove_vmbus_irq();
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH V2 08/10] drivers:hv: Export a function that maps Linux CPU num onto Hyper-V proc num
  2015-10-30  1:12 ` [PATCH V2 01/10] Drivers: hv: util: Increase the timeout for util services K. Y. Srinivasan
                     ` (5 preceding siblings ...)
  2015-10-30  1:12   ` [PATCH V2 07/10] drivers/hv: cleanup synic msrs if vmbus connect failed K. Y. Srinivasan
@ 2015-10-30  1:12   ` K. Y. Srinivasan
  2015-10-30  1:12   ` [PATCH V2 09/10] drivers:hv: Export the API to invoke a hypercall on Hyper-V K. Y. Srinivasan
  2015-10-30  1:12   ` [PATCH V2 10/10] drivers:hv: Define the channel type for Hyper-V PCI Express pass-through K. Y. Srinivasan
  8 siblings, 0 replies; 15+ messages in thread
From: K. Y. Srinivasan @ 2015-10-30  1:12 UTC (permalink / raw)
  To: gregkh, linux-kernel, devel, olaf, apw, vkuznets, jasowang
  Cc: Jake Oshins, K. Y. Srinivasan

From: Jake Oshins <jakeo@microsoft.com>

This patch exposes the mapping between Linux CPU number and Hyper-V virtual
processor number.  This is necessary because the hypervisor needs to know which
virtual processors to target when making a mapping in the Interrupt Redirection
Table in the I/O MMU.

Signed-off-by: Jake Oshins <jakeo@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/vmbus_drv.c |   17 +++++++++++++++++
 include/linux/hyperv.h |    2 ++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 3297731..c01b689 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1193,6 +1193,23 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,
 }
 EXPORT_SYMBOL_GPL(vmbus_allocate_mmio);
 
+/**
+ * vmbus_cpu_number_to_vp_number() - Map CPU to VP.
+ * @cpu_number: CPU number in Linux terms
+ *
+ * This function returns the mapping between the Linux processor
+ * number and the hypervisor's virtual processor number, useful
+ * in making hypercalls and such that talk about specific
+ * processors.
+ *
+ * Return: Virtual processor number in Hyper-V terms
+ */
+int vmbus_cpu_number_to_vp_number(int cpu_number)
+{
+	return hv_context.vp_index[cpu_number];
+}
+EXPORT_SYMBOL_GPL(vmbus_cpu_number_to_vp_number);
+
 static int vmbus_acpi_add(struct acpi_device *device)
 {
 	acpi_status result;
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 54733d5..02393b6 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -982,6 +982,8 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,
 			resource_size_t size, resource_size_t align,
 			bool fb_overlap_ok);
 
+int vmbus_cpu_number_to_vp_number(int cpu_number);
+
 /**
  * VMBUS_DEVICE - macro used to describe a specific hyperv vmbus device
  *
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH V2 09/10] drivers:hv: Export the API to invoke a hypercall on Hyper-V
  2015-10-30  1:12 ` [PATCH V2 01/10] Drivers: hv: util: Increase the timeout for util services K. Y. Srinivasan
                     ` (6 preceding siblings ...)
  2015-10-30  1:12   ` [PATCH V2 08/10] drivers:hv: Export a function that maps Linux CPU num onto Hyper-V proc num K. Y. Srinivasan
@ 2015-10-30  1:12   ` K. Y. Srinivasan
  2015-10-30  1:12   ` [PATCH V2 10/10] drivers:hv: Define the channel type for Hyper-V PCI Express pass-through K. Y. Srinivasan
  8 siblings, 0 replies; 15+ messages in thread
From: K. Y. Srinivasan @ 2015-10-30  1:12 UTC (permalink / raw)
  To: gregkh, linux-kernel, devel, olaf, apw, vkuznets, jasowang
  Cc: Jake Oshins, K. Y. Srinivasan

From: Jake Oshins <jakeo@microsoft.com>

This patch exposes the function that hv_vmbus.ko uses to make hypercalls.  This
is necessary for retargeting an interrupt when it is given a new affinity.

Since we are exporting this API, rename the API as it will be visible outside
the hv.c file.

Signed-off-by: Jake Oshins <jakeo@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/hv.c           |   20 ++++++++++----------
 drivers/hv/hyperv_vmbus.h |    2 +-
 include/linux/hyperv.h    |    1 +
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 6341be8..7a06933 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -89,9 +89,9 @@ static int query_hypervisor_info(void)
 }
 
 /*
- * do_hypercall- Invoke the specified hypercall
+ * hv_do_hypercall- Invoke the specified hypercall
  */
-static u64 do_hypercall(u64 control, void *input, void *output)
+u64 hv_do_hypercall(u64 control, void *input, void *output)
 {
 	u64 input_address = (input) ? virt_to_phys(input) : 0;
 	u64 output_address = (output) ? virt_to_phys(output) : 0;
@@ -132,6 +132,7 @@ static u64 do_hypercall(u64 control, void *input, void *output)
 	return hv_status_lo | ((u64)hv_status_hi << 32);
 #endif /* !x86_64 */
 }
+EXPORT_SYMBOL_GPL(hv_do_hypercall);
 
 #ifdef CONFIG_X86_64
 static cycle_t read_hv_clock_tsc(struct clocksource *arg)
@@ -315,7 +316,7 @@ int hv_post_message(union hv_connection_id connection_id,
 {
 
 	struct hv_input_post_message *aligned_msg;
-	u16 status;
+	u64 status;
 
 	if (payload_size > HV_MESSAGE_PAYLOAD_BYTE_COUNT)
 		return -EMSGSIZE;
@@ -329,11 +330,10 @@ int hv_post_message(union hv_connection_id connection_id,
 	aligned_msg->payload_size = payload_size;
 	memcpy((void *)aligned_msg->payload, payload, payload_size);
 
-	status = do_hypercall(HVCALL_POST_MESSAGE, aligned_msg, NULL)
-		& 0xFFFF;
+	status = hv_do_hypercall(HVCALL_POST_MESSAGE, aligned_msg, NULL);
 
 	put_cpu();
-	return status;
+	return status & 0xFFFF;
 }
 
 
@@ -343,13 +343,13 @@ int hv_post_message(union hv_connection_id connection_id,
  *
  * This involves a hypercall.
  */
-u16 hv_signal_event(void *con_id)
+int hv_signal_event(void *con_id)
 {
-	u16 status;
+	u64 status;
 
-	status = (do_hypercall(HVCALL_SIGNAL_EVENT, con_id, NULL) & 0xFFFF);
+	status = hv_do_hypercall(HVCALL_SIGNAL_EVENT, con_id, NULL);
 
-	return status;
+	return status & 0xFFFF;
 }
 
 static int hv_ce_set_next_event(unsigned long delta,
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 40c0c855..64950d8 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -592,7 +592,7 @@ extern int hv_post_message(union hv_connection_id connection_id,
 			 enum hv_message_type message_type,
 			 void *payload, size_t payload_size);
 
-extern u16 hv_signal_event(void *con_id);
+extern int hv_signal_event(void *con_id);
 
 extern int hv_synic_alloc(void);
 
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 02393b6..ea0a0e3 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -983,6 +983,7 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,
 			bool fb_overlap_ok);
 
 int vmbus_cpu_number_to_vp_number(int cpu_number);
+u64 hv_do_hypercall(u64 control, void *input, void *output);
 
 /**
  * VMBUS_DEVICE - macro used to describe a specific hyperv vmbus device
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH V2 10/10] drivers:hv: Define the channel type for Hyper-V PCI Express pass-through
  2015-10-30  1:12 ` [PATCH V2 01/10] Drivers: hv: util: Increase the timeout for util services K. Y. Srinivasan
                     ` (7 preceding siblings ...)
  2015-10-30  1:12   ` [PATCH V2 09/10] drivers:hv: Export the API to invoke a hypercall on Hyper-V K. Y. Srinivasan
@ 2015-10-30  1:12   ` K. Y. Srinivasan
  8 siblings, 0 replies; 15+ messages in thread
From: K. Y. Srinivasan @ 2015-10-30  1:12 UTC (permalink / raw)
  To: gregkh, linux-kernel, devel, olaf, apw, vkuznets, jasowang
  Cc: Jake Oshins, K. Y. Srinivasan

From: Jake Oshins <jakeo@microsoft.com>

This defines the channel type for PCI front-ends in Hyper-V VMs.

Signed-off-by: Jake Oshins <jakeo@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/channel_mgmt.c |    3 +++
 include/linux/hyperv.h    |   11 +++++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 652afd1..a77646b 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -358,6 +358,7 @@ enum {
 	SCSI,
 	NIC,
 	ND_NIC,
+	PCIE,
 	MAX_PERF_CHN,
 };
 
@@ -375,6 +376,8 @@ static const struct hv_vmbus_device_id hp_devs[] = {
 	{ HV_NIC_GUID, },
 	/* NetworkDirect Guest RDMA */
 	{ HV_ND_GUID, },
+	/* PCI Express Pass Through */
+	{ HV_PCIE_GUID, },
 };
 
 
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index ea0a0e3..5587899 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1140,6 +1140,17 @@ u64 hv_do_hypercall(u64 control, void *input, void *output);
 		}
 
 /*
+ * PCI Express Pass Through
+ * {44C4F61D-4444-4400-9D52-802E27EDE19F}
+ */
+
+#define HV_PCIE_GUID \
+	.guid = { \
+			0x1D, 0xF6, 0xC4, 0x44, 0x44, 0x44, 0x00, 0x44, \
+			0x9D, 0x52, 0x80, 0x2E, 0x27, 0xED, 0xE1, 0x9F \
+		}
+
+/*
  * Common header for Hyper-V ICs
  */
 
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* RE: [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in interrupt context
  2015-10-30  1:12   ` [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in interrupt context K. Y. Srinivasan
@ 2015-12-10 10:18     ` Dexuan Cui
  2015-12-10 23:23       ` KY Srinivasan
  0 siblings, 1 reply; 15+ messages in thread
From: Dexuan Cui @ 2015-12-10 10:18 UTC (permalink / raw)
  To: KY Srinivasan, gregkh, linux-kernel, devel, olaf, apw, vkuznets,
	jasowang

> -----Original Message-----
> From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On Behalf
> Of K. Y. Srinivasan
> Sent: Friday, October 30, 2015 9:13
> To: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> vkuznets@redhat.com; jasowang@redhat.com
> Subject: [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in
> interrupt context
> 
> From: Olaf Hering <olaf@aepfle.de>
> 
> All channel interrupts are bound to specific VCPUs in the guest
> at the point channel is created. While currently, we invoke the
> polling function on the correct CPU (the CPU to which the channel
> is bound to) in some cases we may run the polling function in
> a non-interrupt context. This  potentially can cause an issue as the
> polling function can be interrupted by the channel callback function.
> Fix the issue by running the polling function on the appropriate CPU
> at interrupt level. Additional details of the issue being addressed by
> this patch are given below:
> 
> Currently hv_fcopy_onchannelcallback is called from interrupts and also
> via the ->write function of hv_utils. Since the used global variables to
> maintain state are not thread safe the state can get out of sync.
> This affects the variable state as well as the channel inbound buffer.
> 
> As suggested by KY adjust hv_poll_channel to always run the given
> callback on the cpu which the channel is bound to. This avoids the need
> for locking because all the util services are single threaded and only
> one transaction is active at any given point in time.
> 
> Additionally, remove the context variable, they will always be the same as
> recv_channel.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
> 	V2: Added the check to catch unsolicited daemon writes - Vitaly
> 
>  drivers/hv/hv_fcopy.c     |   34 +++++++++++++---------------------
>  drivers/hv/hv_kvp.c       |   28 ++++++++++------------------
>  drivers/hv/hv_snapshot.c  |   29 +++++++++++------------------
>  drivers/hv/hyperv_vmbus.h |    6 +-----
>  4 files changed, 35 insertions(+), 62 deletions(-)

(Sorry for not joining the discussion when the patch was firstly made)

It looks the patch has not been Greg's tree yet.

I have 2 questions about the patch:

1. hv_poll_channel() is invoked in fcopy_handle_handshake(), but not in
vss_handle_handshake() and kvp_handle_handshake().
Why -- I guess we missed the vss/kvp cases somehow?

2.  With the patch, hv_fcopy_onchannelcallback() can be invoked in the
tasklet (i.e., vmbus_on_event(). NB: local irq is enabled), and in the
hard irq handler(the IPI handler, e.g., 
fcopy_poll_wrapper() -> fcopy_poll_wrapper()).

Can the former be interrupted by the latter?
e.g., when the callback is running in the tasklet on vCPU0,
fcopy_timeout_func() or fcopy_on_msg() could send the IPI to
vCPU0 from another vCPU.

Thanks,
-- Dexuan

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in interrupt context
  2015-12-10 10:18     ` Dexuan Cui
@ 2015-12-10 23:23       ` KY Srinivasan
  2015-12-11  7:33         ` Dexuan Cui
  0 siblings, 1 reply; 15+ messages in thread
From: KY Srinivasan @ 2015-12-10 23:23 UTC (permalink / raw)
  To: Dexuan Cui, gregkh, linux-kernel, devel, olaf, apw, vkuznets, jasowang



> -----Original Message-----
> From: Dexuan Cui
> Sent: Thursday, December 10, 2015 2:19 AM
> To: KY Srinivasan <kys@microsoft.com>; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com
> Subject: RE: [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in
> interrupt context
> 
> > -----Original Message-----
> > From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On
> Behalf
> > Of K. Y. Srinivasan
> > Sent: Friday, October 30, 2015 9:13
> > To: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> > vkuznets@redhat.com; jasowang@redhat.com
> > Subject: [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in
> > interrupt context
> >
> > From: Olaf Hering <olaf@aepfle.de>
> >
> > All channel interrupts are bound to specific VCPUs in the guest
> > at the point channel is created. While currently, we invoke the
> > polling function on the correct CPU (the CPU to which the channel
> > is bound to) in some cases we may run the polling function in
> > a non-interrupt context. This  potentially can cause an issue as the
> > polling function can be interrupted by the channel callback function.
> > Fix the issue by running the polling function on the appropriate CPU
> > at interrupt level. Additional details of the issue being addressed by
> > this patch are given below:
> >
> > Currently hv_fcopy_onchannelcallback is called from interrupts and also
> > via the ->write function of hv_utils. Since the used global variables to
> > maintain state are not thread safe the state can get out of sync.
> > This affects the variable state as well as the channel inbound buffer.
> >
> > As suggested by KY adjust hv_poll_channel to always run the given
> > callback on the cpu which the channel is bound to. This avoids the need
> > for locking because all the util services are single threaded and only
> > one transaction is active at any given point in time.
> >
> > Additionally, remove the context variable, they will always be the same as
> > recv_channel.
> >
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > ---
> > 	V2: Added the check to catch unsolicited daemon writes - Vitaly
> >
> >  drivers/hv/hv_fcopy.c     |   34 +++++++++++++---------------------
> >  drivers/hv/hv_kvp.c       |   28 ++++++++++------------------
> >  drivers/hv/hv_snapshot.c  |   29 +++++++++++------------------
> >  drivers/hv/hyperv_vmbus.h |    6 +-----
> >  4 files changed, 35 insertions(+), 62 deletions(-)
> 
> (Sorry for not joining the discussion when the patch was firstly made)
> 
> It looks the patch has not been Greg's tree yet.
> 
> I have 2 questions about the patch:
> 
> 1. hv_poll_channel() is invoked in fcopy_handle_handshake(), but not in
> vss_handle_handshake() and kvp_handle_handshake().
> Why -- I guess we missed the vss/kvp cases somehow?
I will fix this.

> 
> 2.  With the patch, hv_fcopy_onchannelcallback() can be invoked in the
> tasklet (i.e., vmbus_on_event(). NB: local irq is enabled), and in the
> hard irq handler(the IPI handler, e.g.,
> fcopy_poll_wrapper() -> fcopy_poll_wrapper()).
> 
> Can the former be interrupted by the latter?
> e.g., when the callback is running in the tasklet on vCPU0,
> fcopy_timeout_func() or fcopy_on_msg() could send the IPI to
> vCPU0 from another vCPU.

Keep in mind that when the poll function is run, the state will not be
HVUTIL_READY. The state will be set to HVUTIL_READY in the IPI
handler. So, it is ok if the tasklet is interrupted by the IPI handler.

Regards,

K. Y


^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in interrupt context
  2015-12-10 23:23       ` KY Srinivasan
@ 2015-12-11  7:33         ` Dexuan Cui
  2015-12-12  0:49           ` KY Srinivasan
  0 siblings, 1 reply; 15+ messages in thread
From: Dexuan Cui @ 2015-12-11  7:33 UTC (permalink / raw)
  To: KY Srinivasan, gregkh, linux-kernel, devel, olaf, apw, vkuznets,
	jasowang

[-- Attachment #1: Type: text/plain, Size: 1553 bytes --]

> -----Original Message-----
> From: KY Srinivasan
> Sent: Friday, December 11, 2015 7:23
> > It looks the patch has not been Greg's tree yet.
> >
> > I have 2 questions about the patch:
> >
> > 1. hv_poll_channel() is invoked in fcopy_handle_handshake(), but not in
> > vss_handle_handshake() and kvp_handle_handshake().
> > Why -- I guess we missed the vss/kvp cases somehow?
> I will fix this.

Thanks, KY!
BTW, I fixed another small issue by https://lkml.org/lkml/2015/12/10/50
(The mail is attached for your convenience)

> > 2.  With the patch, hv_fcopy_onchannelcallback() can be invoked in the
> > tasklet (i.e., vmbus_on_event(). NB: local irq is enabled), and in the
> > hard irq handler(the IPI handler, e.g.,
> > fcopy_poll_wrapper() -> fcopy_poll_wrapper()).
> >
> > Can the former be interrupted by the latter?
> > e.g., when the callback is running in the tasklet on vCPU0,
> > fcopy_timeout_func() or fcopy_on_msg() could send the IPI to
> > vCPU0 from another vCPU.
> 
> Keep in mind that when the poll function is run, the state will not be
> HVUTIL_READY. The state will be set to HVUTIL_READY in the IPI
> handler. So, it is ok if the tasklet is interrupted by the IPI handler.
> 
> K. Y

Got it.

BTW, in fcopy_handle_handshake(), IMO the line
fcopy_transaction.state = HVUTIL_READY;
just before 
hv_poll_channel(fcopy_transaction.recv_channel, fcopy_poll_wrapper);
should be removed? Because in fcopy_poll_wrapper() we always have
the same line. 

Ditto for kvp/vss.

Thanks,
-- Dexuan

[-- Attachment #2: Type: message/rfc822, Size: 7138 bytes --]

From: Dexuan Cui <decui@microsoft.com>
To: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "driverdev-devel@linuxdriverproject.org" <driverdev-devel@linuxdriverproject.org>, "olaf@aepfle.de" <olaf@aepfle.de>, "apw@canonical.com" <apw@canonical.com>, "jasowang@redhat.com" <jasowang@redhat.com>, KY Srinivasan <kys@microsoft.com>, "vkuznets@redhat.com" <vkuznets@redhat.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Subject: [PATCH] tools: hv: vss: fix the write()'s argument: error -> vss_msg
Date: Thu, 10 Dec 2015 09:00:57 +0000
Message-ID: <1449738057-4596-1-git-send-email-decui@microsoft.com>

I found this by chance. I don't have a specific bug caused by this.

Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Signed-off-by: Dexuan Cui <decui@microsoft.com>
Cc: stable@vger.kernel.org
---
 tools/hv/hv_vss_daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/hv/hv_vss_daemon.c b/tools/hv/hv_vss_daemon.c
index 96234b6..5d51d6f 100644
--- a/tools/hv/hv_vss_daemon.c
+++ b/tools/hv/hv_vss_daemon.c
@@ -254,7 +254,7 @@ int main(int argc, char *argv[])
                        syslog(LOG_ERR, "Illegal op:%d\n", op);
                }
                vss_msg->error = error;
-               len = write(vss_fd, &error, sizeof(struct hv_vss_msg));
+               len = write(vss_fd, vss_msg, sizeof(struct hv_vss_msg));
                if (len != sizeof(struct hv_vss_msg)) {
                        syslog(LOG_ERR, "write failed; error: %d %s", errno,
                               strerror(errno));
--
1.9.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fdriverdev.linuxdriverproject.org%2fmailman%2flistinfo%2fdriverdev-devel&data=01%7c01%7cdecui%40064d.mgd.microsoft.com%7c6038ce2d24784746fa0408d30134f8d9%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=6Hs1UNlA11wBtnU5XvHhpCgkUlIhWVMKxPHVj9UAZq8%3d

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* RE: [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in interrupt context
  2015-12-11  7:33         ` Dexuan Cui
@ 2015-12-12  0:49           ` KY Srinivasan
  0 siblings, 0 replies; 15+ messages in thread
From: KY Srinivasan @ 2015-12-12  0:49 UTC (permalink / raw)
  To: Dexuan Cui, gregkh, linux-kernel, devel, olaf, apw, vkuznets, jasowang



> -----Original Message-----
> From: Dexuan Cui
> Sent: Thursday, December 10, 2015 11:33 PM
> To: KY Srinivasan <kys@microsoft.com>; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com
> Subject: RE: [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in
> interrupt context
> 
> > -----Original Message-----
> > From: KY Srinivasan
> > Sent: Friday, December 11, 2015 7:23
> > > It looks the patch has not been Greg's tree yet.
> > >
> > > I have 2 questions about the patch:
> > >
> > > 1. hv_poll_channel() is invoked in fcopy_handle_handshake(), but not in
> > > vss_handle_handshake() and kvp_handle_handshake().
> > > Why -- I guess we missed the vss/kvp cases somehow?
> > I will fix this.
> 
> Thanks, KY!
> BTW, I fixed another small issue by https://lkml.org/lkml/2015/12/10/50
> (The mail is attached for your convenience)

I will pick this up.

> 
> > > 2.  With the patch, hv_fcopy_onchannelcallback() can be invoked in the
> > > tasklet (i.e., vmbus_on_event(). NB: local irq is enabled), and in the
> > > hard irq handler(the IPI handler, e.g.,
> > > fcopy_poll_wrapper() -> fcopy_poll_wrapper()).
> > >
> > > Can the former be interrupted by the latter?
> > > e.g., when the callback is running in the tasklet on vCPU0,
> > > fcopy_timeout_func() or fcopy_on_msg() could send the IPI to
> > > vCPU0 from another vCPU.
> >
> > Keep in mind that when the poll function is run, the state will not be
> > HVUTIL_READY. The state will be set to HVUTIL_READY in the IPI
> > handler. So, it is ok if the tasklet is interrupted by the IPI handler.
> >
> > K. Y
> 
> Got it.
> 
> BTW, in fcopy_handle_handshake(), IMO the line
> fcopy_transaction.state = HVUTIL_READY;
> just before
> hv_poll_channel(fcopy_transaction.recv_channel, fcopy_poll_wrapper);
> should be removed? Because in fcopy_poll_wrapper() we always have
> the same line.
> 
> Ditto for kvp/vss.
> 
> Thanks,

I will fix this and resend.

Thanks,

K. Y
> -- Dexuan


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2015-12-12  0:50 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-30  1:12 [PATCH V2 00/10] Drivers: hv: Miscellaneous fixes K. Y. Srinivasan
2015-10-30  1:12 ` [PATCH V2 01/10] Drivers: hv: util: Increase the timeout for util services K. Y. Srinivasan
2015-10-30  1:12   ` [PATCH V2 02/10] Drivers: hv: utils: run polling callback always in interrupt context K. Y. Srinivasan
2015-12-10 10:18     ` Dexuan Cui
2015-12-10 23:23       ` KY Srinivasan
2015-12-11  7:33         ` Dexuan Cui
2015-12-12  0:49           ` KY Srinivasan
2015-10-30  1:12   ` [PATCH V2 03/10] tools: hv: report ENOSPC errors in hv_fcopy_daemon K. Y. Srinivasan
2015-10-30  1:12   ` [PATCH V2 04/10] tools: hv: remove repeated HV_FCOPY string K. Y. Srinivasan
2015-10-30  1:12   ` [PATCH V2 05/10] Drivers: hv: util: catch allocation errors K. Y. Srinivasan
2015-10-30  1:12   ` [PATCH V2 06/10] Drivers: hv: utils: use memdup_user in hvt_op_write K. Y. Srinivasan
2015-10-30  1:12   ` [PATCH V2 07/10] drivers/hv: cleanup synic msrs if vmbus connect failed K. Y. Srinivasan
2015-10-30  1:12   ` [PATCH V2 08/10] drivers:hv: Export a function that maps Linux CPU num onto Hyper-V proc num K. Y. Srinivasan
2015-10-30  1:12   ` [PATCH V2 09/10] drivers:hv: Export the API to invoke a hypercall on Hyper-V K. Y. Srinivasan
2015-10-30  1:12   ` [PATCH V2 10/10] drivers:hv: Define the channel type for Hyper-V PCI Express pass-through K. Y. Srinivasan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.