[PATCH 00/10] Drivers: hv: Miscellaneous fixes.

[PATCH 00/10] Drivers: hv: Miscellaneous fixes.

[K. Y. Srinivasan]

Miscellaneous fixes.

Denis V. Lunev (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           |   40 ++++++++++++++------------------------
 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(+), 92 deletions(-)

-- 
1.7.4.1

[PATCH 01/10] Drivers: hv: util: Increase the timeout for util services

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

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 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

[PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

[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>
---
 drivers/hv/hv_fcopy.c     |   37 +++++++++++++------------------------
 drivers/hv/hv_kvp.c       |   28 ++++++++++------------------
 drivers/hv/hv_snapshot.c  |   29 +++++++++++------------------
 drivers/hv/hyperv_vmbus.h |    6 +-----
 4 files changed, 35 insertions(+), 65 deletions(-)

diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
index bbdec50..4eab465 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);
@@ -295,9 +288,6 @@ static int fcopy_on_msg(void *msg, int len)
 	if (fcopy_transaction.state == HVUTIL_DEVICE_INIT)
 		return fcopy_handle_handshake(*val);
 
-	if (fcopy_transaction.state != HVUTIL_USERSPACE_REQ)
-		return -EINVAL;
-
 	/*
 	 * Complete the transaction by forwarding the result
 	 * to the host. But first, cancel the timeout.
@@ -305,9 +295,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

Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

[Vitaly Kuznetsov]

"K. Y. Srinivasan" <kys@microsoft.com> writes:

> 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>
> ---
>  drivers/hv/hv_fcopy.c     |   37 +++++++++++++------------------------
>  drivers/hv/hv_kvp.c       |   28 ++++++++++------------------
>  drivers/hv/hv_snapshot.c  |   29 +++++++++++------------------
>  drivers/hv/hyperv_vmbus.h |    6 +-----
>  4 files changed, 35 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
> index bbdec50..4eab465 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);
> @@ -295,9 +288,6 @@ static int fcopy_on_msg(void *msg, int len)
>  	if (fcopy_transaction.state == HVUTIL_DEVICE_INIT)
>  		return fcopy_handle_handshake(*val);
>
> -	if (fcopy_transaction.state != HVUTIL_USERSPACE_REQ)
> -		return -EINVAL;
> -

This particular change seems unrelated and I'm unsure it's safe to
remove this check. It is meant to protect against daemon screwing the
protocol and writing to the device when it wasn't requested for an
action. It is correct to propagate -EINVAL in this case. Or am I missing
something and the check is redundant now?

Thanks,

[...]

-- 
  Vitaly

Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

[Olaf Hering]

On Thu, Oct 08, Vitaly Kuznetsov wrote:

> > @@ -295,9 +288,6 @@ static int fcopy_on_msg(void *msg, int len)
> >  	if (fcopy_transaction.state == HVUTIL_DEVICE_INIT)
> >  		return fcopy_handle_handshake(*val);
> >
> > -	if (fcopy_transaction.state != HVUTIL_USERSPACE_REQ)
> > -		return -EINVAL;
> > -
> 
> This particular change seems unrelated and I'm unsure it's safe to
> remove this check. It is meant to protect against daemon screwing the
> protocol and writing to the device when it wasn't requested for an
> action. It is correct to propagate -EINVAL in this case. Or am I missing
> something and the check is redundant now?

What can happen if there is an odd write request? If there is a timeout
scheduled some return value will be sent to the host. Then the state is
set to RESET and eventually vmbus_recvpacket will receive something.
That something will be processed and passed to the daemon.

If there was no timeout scheduled the write will just return.

Olaf

Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

[Vitaly Kuznetsov]

Olaf Hering <olaf@aepfle.de> writes:

> On Thu, Oct 08, Vitaly Kuznetsov wrote:
>
>> > @@ -295,9 +288,6 @@ static int fcopy_on_msg(void *msg, int len)
>> >  	if (fcopy_transaction.state == HVUTIL_DEVICE_INIT)
>> >  		return fcopy_handle_handshake(*val);
>> >
>> > -	if (fcopy_transaction.state != HVUTIL_USERSPACE_REQ)
>> > -		return -EINVAL;
>> > -
>> 
>> This particular change seems unrelated and I'm unsure it's safe to
>> remove this check. It is meant to protect against daemon screwing the
>> protocol and writing to the device when it wasn't requested for an
>> action. It is correct to propagate -EINVAL in this case. Or am I missing
>> something and the check is redundant now?
>
> What can happen if there is an odd write request?

I think we don't want to propagate misbehaving daemon's data to the
host -- let's cut it here. E.g. imagine there is no communication going
on and daemon starts writing something to the device. In case we remove
the check we'll be doing fcopy_respond_to_host() for each daemon's write
flooding the host.

> If there is a timeout
> scheduled some return value will be sent to the host. Then the state is
> set to RESET and eventually vmbus_recvpacket will receive something.
> That something will be processed and passed to the daemon.
>
> If there was no timeout scheduled the write will just return.

yes, but after doing fcopy_respond_to_host(). I'd suggest we leave the
check in place, better safe than sorry.

-- 
  Vitaly

RE: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

[KY Srinivasan]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Thursday, October 8, 2015 6:53 AM
> To: Olaf Hering <olaf@aepfle.de>
> Cc: KY Srinivasan <kys@microsoft.com>; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; apw@canonical.com;
> jasowang@redhat.com
> Subject: Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in
> interrupt context
> 
> Olaf Hering <olaf@aepfle.de> writes:
> 
> > On Thu, Oct 08, Vitaly Kuznetsov wrote:
> >
> >> > @@ -295,9 +288,6 @@ static int fcopy_on_msg(void *msg, int len)
> >> >  	if (fcopy_transaction.state == HVUTIL_DEVICE_INIT)
> >> >  		return fcopy_handle_handshake(*val);
> >> >
> >> > -	if (fcopy_transaction.state != HVUTIL_USERSPACE_REQ)
> >> > -		return -EINVAL;
> >> > -
> >>
> >> This particular change seems unrelated and I'm unsure it's safe to
> >> remove this check. It is meant to protect against daemon screwing the
> >> protocol and writing to the device when it wasn't requested for an
> >> action. It is correct to propagate -EINVAL in this case. Or am I missing
> >> something and the check is redundant now?
> >
> > What can happen if there is an odd write request?
> 
> I think we don't want to propagate misbehaving daemon's data to the
> host -- let's cut it here. E.g. imagine there is no communication going
> on and daemon starts writing something to the device. In case we remove
> the check we'll be doing fcopy_respond_to_host() for each daemon's write
> flooding the host.
> 
> > If there is a timeout
> > scheduled some return value will be sent to the host. Then the state is
> > set to RESET and eventually vmbus_recvpacket will receive something.
> > That something will be processed and passed to the daemon.
> >
> > If there was no timeout scheduled the write will just return.
> 
> yes, but after doing fcopy_respond_to_host(). I'd suggest we leave the
> check in place, better safe than sorry.

Agreed; Olaf, if it is ok with you, I can fix it up and send.

Regards,

K. Y
> 
> --
>   Vitaly

Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

[Olaf Hering]

On Thu, Oct 08, KY Srinivasan wrote:

> > yes, but after doing fcopy_respond_to_host(). I'd suggest we leave the
> > check in place, better safe than sorry.
> 
> Agreed; Olaf, if it is ok with you, I can fix it up and send.

I will retest with this part reverted. I think without two code paths
entering hv_fcopy_callback it should be ok to leave this check in.

Olaf

Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

[Vitaly Kuznetsov]

Olaf Hering <olaf@aepfle.de> writes:

> On Thu, Oct 08, KY Srinivasan wrote:
>
>> > yes, but after doing fcopy_respond_to_host(). I'd suggest we leave the
>> > check in place, better safe than sorry.
>> 
>> Agreed; Olaf, if it is ok with you, I can fix it up and send.
>
> I will retest with this part reverted. I think without two code paths
> entering hv_fcopy_callback it should be ok to leave this check in.

I think hv_fcopy_callback() is not involved here: we call fcopy_on_msg()
every time userspace daemon writes to the device and it is not anyhow
synchronized with host-guest communication. 

-- 
  Vitaly

Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

[Olaf Hering]

On Fri, Oct 09, Vitaly Kuznetsov wrote:

> Olaf Hering <olaf@aepfle.de> writes:
> 
> > On Thu, Oct 08, KY Srinivasan wrote:
> >
> >> > yes, but after doing fcopy_respond_to_host(). I'd suggest we leave the
> >> > check in place, better safe than sorry.
> >> 
> >> Agreed; Olaf, if it is ok with you, I can fix it up and send.
> >
> > I will retest with this part reverted. I think without two code paths
> > entering hv_fcopy_callback it should be ok to leave this check in.
> 
> I think hv_fcopy_callback() is not involved here: we call fcopy_on_msg()
> every time userspace daemon writes to the device and it is not anyhow
> synchronized with host-guest communication. 

An earlier variant of this patch used locks around the vmbus_recvpacket
and the result was used to decide which thread of execution notifies the
daemon. I think if the interrupt ran earlier than the daemon did the
write then the state expected in fcopy_on_msg would obviously be wrong.
As a result the daemon will just terminate with EFAULT. With the check
removed it would proceed, and either not chancel the timeout or
vmbus_recvpacket reads nothing.

But now that it is single threaded the state in fcopy_on_msg should be
as expected. As said, will retest. Either later today or on Monday.

Olaf

RE: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2427 bytes --]



> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Friday, October 9, 2015 4:29 AM
> To: Vitaly Kuznetsov <vkuznets@redhat.com>
> Cc: KY Srinivasan <kys@microsoft.com>; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; apw@canonical.com;
> jasowang@redhat.com
> Subject: Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in
> interrupt context
> 
> On Fri, Oct 09, Vitaly Kuznetsov wrote:
> 
> > Olaf Hering <olaf@aepfle.de> writes:
> >
> > > On Thu, Oct 08, KY Srinivasan wrote:
> > >
> > >> > yes, but after doing fcopy_respond_to_host(). I'd suggest we leave
> the
> > >> > check in place, better safe than sorry.
> > >>
> > >> Agreed; Olaf, if it is ok with you, I can fix it up and send.
> > >
> > > I will retest with this part reverted. I think without two code paths
> > > entering hv_fcopy_callback it should be ok to leave this check in.
> >
> > I think hv_fcopy_callback() is not involved here: we call fcopy_on_msg()
> > every time userspace daemon writes to the device and it is not anyhow
> > synchronized with host-guest communication.
> 
> An earlier variant of this patch used locks around the vmbus_recvpacket
> and the result was used to decide which thread of execution notifies the
> daemon. I think if the interrupt ran earlier than the daemon did the
> write then the state expected in fcopy_on_msg would obviously be wrong.
> As a result the daemon will just terminate with EFAULT. With the check
> removed it would proceed, and either not chancel the timeout or
> vmbus_recvpacket reads nothing.
> 
> But now that it is single threaded the state in fcopy_on_msg should be
> as expected. As said, will retest. Either later today or on Monday.

The only case I can see is if the daemon does not respond in a timely fashion.
In this case, we would timeout and terminate the transaction prematurely.
In this case when the daemon ultimately responds, we would view that response as
an error and return EINVAL and I think in this case it is fine to terminate
the daemon; especially now that we are going to increase the timeout value
to 30 seconds.

Let me know the results of your testing. I will repost the patches after I hear from
you.

Regards,

K. Y

 
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

[Olaf Hering]

On Fri, Oct 09, Olaf Hering wrote:

> On Thu, Oct 08, KY Srinivasan wrote:
> 
> > > yes, but after doing fcopy_respond_to_host(). I'd suggest we leave the
> > > check in place, better safe than sorry.
> > 
> > Agreed; Olaf, if it is ok with you, I can fix it up and send.
> 
> I will retest with this part reverted. I think without two code paths
> entering hv_fcopy_callback it should be ok to leave this check in.

Today I restored the "fcopy_transaction.state != HVUTIL_USERSPACE_REQ"
check and its working fine.

Olaf

RE: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1211 bytes --]



> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Tuesday, October 13, 2015 2:47 AM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>; gregkh@linuxfoundation.org;
> linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in
> interrupt context
> 
> On Fri, Oct 09, Olaf Hering wrote:
> 
> > On Thu, Oct 08, KY Srinivasan wrote:
> >
> > > > yes, but after doing fcopy_respond_to_host(). I'd suggest we leave the
> > > > check in place, better safe than sorry.
> > >
> > > Agreed; Olaf, if it is ok with you, I can fix it up and send.
> >
> > I will retest with this part reverted. I think without two code paths
> > entering hv_fcopy_callback it should be ok to leave this check in.
> 
> Today I restored the "fcopy_transaction.state != HVUTIL_USERSPACE_REQ"
> check and its working fine.

Thanks Olaf. I will repost the patches with the update.

K. Y
> 
> Olaf
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH 03/10] tools: hv: report ENOSPC errors in hv_fcopy_daemon

[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

[PATCH 04/10] tools: hv: remove repeated HV_FCOPY string

[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

[PATCH 05/10] Drivers: hv: util: catch allocation errors

[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

[PATCH 06/10] Drivers: hv: utils: use memdup_user in hvt_op_write

[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

[PATCH 07/10] drivers/hv: cleanup synic msrs if vmbus connect failed

[K. Y. Srinivasan]

From: Denis V. Lunev <den@openvz.org>

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

Re: [PATCH 07/10] drivers/hv: cleanup synic msrs if vmbus connect failed

[Denis V. Lunev]

On 10/08/2015 05:01 AM, K. Y. Srinivasan wrote:
> From: Denis V. Lunev <den@openvz.org>

K.Y.,

there is one subtle thing in this submission. You have changed "From:"
field in comparison with the original letter. I have submitted
the patch with "From: Andrey Smetanin <asmetanin@virtuozzo.com>"
In this case Author: in the resulted git mainstream commit will
be Andrey. With your submission the resulted Author will be I.

This was already happened once with

commit cc2dd4027a43bb36c846f195a764edabc0828602
Author: Denis V. Lunev <den@openvz.org>
Date:   Sat Aug 1 16:08:20 2015 -0700

     mshyperv: fix recognition of Hyper-V guest crash MSR's

The situation looks a bit unfair.

Can we do something with that now/next time?

Den

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

RE: [PATCH 07/10] drivers/hv: cleanup synic msrs if vmbus connect failed

[KY Srinivasan]

> -----Original Message-----
> From: Denis V. Lunev [mailto:den@openvz.org]
> Sent: Thursday, October 8, 2015 10:20 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
> Cc: Andrey Smetanin <asmetanin@virtuozzo.com>; Haiyang Zhang
> <haiyangz@microsoft.com>
> Subject: Re: [PATCH 07/10] drivers/hv: cleanup synic msrs if vmbus connect
> failed
> 
> On 10/08/2015 05:01 AM, K. Y. Srinivasan wrote:
> > From: Denis V. Lunev <den@openvz.org>
> 
> K.Y.,
> 
> there is one subtle thing in this submission. You have changed "From:"
> field in comparison with the original letter. I have submitted
> the patch with "From: Andrey Smetanin <asmetanin@virtuozzo.com>"
> In this case Author: in the resulted git mainstream commit will
> be Andrey. With your submission the resulted Author will be I.
> 
> This was already happened once with
> 
> commit cc2dd4027a43bb36c846f195a764edabc0828602
> Author: Denis V. Lunev <den@openvz.org>
> Date:   Sat Aug 1 16:08:20 2015 -0700
> 
>      mshyperv: fix recognition of Hyper-V guest crash MSR's
> 
> The situation looks a bit unfair.
> 
> Can we do something with that now/next time?

I am going to be resubmitting this series. I will fix it up.

Regards,

K. Y
> 
> Den
> 
> > 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>
> > ---
> >   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();

Re: [PATCH 07/10] drivers/hv: cleanup synic msrs if vmbus connect failed

[Denis V. Lunev]

On 10/08/2015 08:28 PM, KY Srinivasan wrote:
>
>> -----Original Message-----
>> From: Denis V. Lunev [mailto:den@openvz.org]
>> Sent: Thursday, October 8, 2015 10:20 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
>> Cc: Andrey Smetanin <asmetanin@virtuozzo.com>; Haiyang Zhang
>> <haiyangz@microsoft.com>
>> Subject: Re: [PATCH 07/10] drivers/hv: cleanup synic msrs if vmbus connect
>> failed
>>
>> On 10/08/2015 05:01 AM, K. Y. Srinivasan wrote:
>>> From: Denis V. Lunev <den@openvz.org>
>> K.Y.,
>>
>> there is one subtle thing in this submission. You have changed "From:"
>> field in comparison with the original letter. I have submitted
>> the patch with "From: Andrey Smetanin <asmetanin@virtuozzo.com>"
>> In this case Author: in the resulted git mainstream commit will
>> be Andrey. With your submission the resulted Author will be I.
>>
>> This was already happened once with
>>
>> commit cc2dd4027a43bb36c846f195a764edabc0828602
>> Author: Denis V. Lunev <den@openvz.org>
>> Date:   Sat Aug 1 16:08:20 2015 -0700
>>
>>       mshyperv: fix recognition of Hyper-V guest crash MSR's
>>
>> The situation looks a bit unfair.
>>
>> Can we do something with that now/next time?
> I am going to be resubmitting this series. I will fix it up.
thank you very much :)

Den

[PATCH 08/10] drivers:hv: Export a function that maps Linux CPU num onto Hyper-V proc num

[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

[PATCH 09/10] drivers:hv: Export the API to invoke a hypercall on Hyper-V

[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

[PATCH 10/10] drivers:hv: Define the channel type for Hyper-V PCI Express pass-through

[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..1ae615b 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -358,6 +358,7 @@ enum {
 	SCSI,
 	NIC,
 	ND_NIC,
+	HV_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

Re: [PATCH 10/10] drivers:hv: Define the channel type for Hyper-V PCI Express pass-through

[Vitaly Kuznetsov]

"K. Y. Srinivasan" <kys@microsoft.com> writes:

> 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..1ae615b 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -358,6 +358,7 @@ enum {
>  	SCSI,
>  	NIC,
>  	ND_NIC,
> +	HV_PCIE,
>  	MAX_PERF_CHN,
>  };

It seems all other members of the enum don't have HV_ prefix... should
we add it there or remove it from HV_PCIE?

>
> @@ -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, },
>  };

And here everything is prefixed with HV_ ... so I'd say we add HV_ to
all members of the previously mentioned enum.

[...]

-- 
  Vitaly

Re: [PATCH 01/10] Drivers: hv: util: Increase the timeout for util services

[Olaf Hering]

On Wed, Oct 07, K. Y. Srinivasan wrote:

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

Is this an arbitrary number, or does the host allow such a large delay
for the response?

Olaf

RE: [PATCH 01/10] Drivers: hv: util: Increase the timeout for util services

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1133 bytes --]



> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Thursday, October 8, 2015 6:24 AM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; apw@canonical.com; vkuznets@redhat.com;
> jasowang@redhat.com
> Subject: Re: [PATCH 01/10] Drivers: hv: util: Increase the timeout for util
> services
> 
> On Wed, Oct 07, K. Y. Srinivasan wrote:
> 
> > 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.
> 
> Is this an arbitrary number, or does the host allow such a large delay
> for the response?

Checked with the host guys and it looks like host sets up a 60 
second timeout for each transaction and if it times out; it may retry.

Anything below 60 seconds should work for us.

I will add this to the commit log.

K. Y 
> 
> Olaf
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥