The current code picked the highest version advertised by the host. WS2012 R2
has implemented a protocol version for KVP that is not compatible with prior
protocol versions of KVP. Fix the bug in the current code by explicitly specifying
the protocol version that the guest can support.
Signed-off-by: K. Y. Srinivasan <[email protected]>
Reviewed-by: Haiyang Zhang <[email protected]>
---
drivers/hv/channel_mgmt.c | 75 +++++++++++++++++++++++++++++++-------------
drivers/hv/hv_kvp.c | 24 ++++++++++++++-
drivers/hv/hv_snapshot.c | 18 +++-------
drivers/hv/hv_util.c | 21 +++++++++++--
include/linux/hyperv.h | 10 +++++-
5 files changed, 109 insertions(+), 39 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 0df7590..12ec8c8 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -48,30 +48,39 @@ struct vmbus_channel_message_table_entry {
* @negop is of type &struct icmsg_negotiate.
* Set up and fill in default negotiate response message.
*
- * The max_fw_version specifies the maximum framework version that
- * we can support and max _srv_version specifies the maximum service
- * version we can support. A special value MAX_SRV_VER can be
- * specified to indicate that we can handle the maximum version
- * exposed by the host.
+ * The fw_version specifies the framework version that
+ * we can support and srv_version specifies the service
+ * version we can support.
*
* Mainly used by Hyper-V drivers.
*/
-void vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
+bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
struct icmsg_negotiate *negop, u8 *buf,
- int max_fw_version, int max_srv_version)
+ int fw_version, int srv_version)
{
- int icframe_vercnt;
- int icmsg_vercnt;
+ int icframe_major, icframe_minor;
+ int icmsg_major, icmsg_minor;
+ int fw_major, fw_minor;
+ int srv_major, srv_minor;
int i;
+ bool found_match = false;
icmsghdrp->icmsgsize = 0x10;
+ fw_major = (fw_version >> 16);
+ fw_minor = (fw_version & 0xFFFF);
+
+ srv_major = (srv_version >> 16);
+ srv_minor = (srv_version & 0xFFFF);
negop = (struct icmsg_negotiate *)&buf[
sizeof(struct vmbuspipe_hdr) +
sizeof(struct icmsg_hdr)];
- icframe_vercnt = negop->icframe_vercnt;
- icmsg_vercnt = negop->icmsg_vercnt;
+ icframe_major = negop->icframe_vercnt;
+ icframe_minor = 0;
+
+ icmsg_major = negop->icmsg_vercnt;
+ icmsg_minor = 0;
/*
* Select the framework version number we will
@@ -79,26 +88,48 @@ void vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
*/
for (i = 0; i < negop->icframe_vercnt; i++) {
- if (negop->icversion_data[i].major <= max_fw_version)
- icframe_vercnt = negop->icversion_data[i].major;
+ if ((negop->icversion_data[i].major == fw_major) &&
+ (negop->icversion_data[i].minor == fw_minor)) {
+ icframe_major = negop->icversion_data[i].major;
+ icframe_minor = negop->icversion_data[i].minor;
+ found_match = true;
+ }
}
+ if (!found_match)
+ goto fw_error;
+
+ found_match = false;
+
for (i = negop->icframe_vercnt;
(i < negop->icframe_vercnt + negop->icmsg_vercnt); i++) {
- if (negop->icversion_data[i].major <= max_srv_version)
- icmsg_vercnt = negop->icversion_data[i].major;
+ if ((negop->icversion_data[i].major == srv_major) &&
+ (negop->icversion_data[i].minor == srv_minor)) {
+ icmsg_major = negop->icversion_data[i].major;
+ icmsg_minor = negop->icversion_data[i].minor;
+ found_match = true;
+ }
}
/*
- * Respond with the maximum framework and service
+ * Respond with the framework and service
* version numbers we can support.
*/
- negop->icframe_vercnt = 1;
- negop->icmsg_vercnt = 1;
- negop->icversion_data[0].major = icframe_vercnt;
- negop->icversion_data[0].minor = 0;
- negop->icversion_data[1].major = icmsg_vercnt;
- negop->icversion_data[1].minor = 0;
+
+fw_error:
+ if (!found_match) {
+ negop->icframe_vercnt = 0;
+ negop->icmsg_vercnt = 0;
+ } else {
+ negop->icframe_vercnt = 1;
+ negop->icmsg_vercnt = 1;
+ }
+
+ negop->icversion_data[0].major = icframe_major;
+ negop->icversion_data[0].minor = icframe_minor;
+ negop->icversion_data[1].major = icmsg_major;
+ negop->icversion_data[1].minor = icmsg_minor;
+ return found_match;
}
EXPORT_SYMBOL_GPL(vmbus_prep_negotiate_resp);
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index ed50e9e..5312720 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -29,6 +29,16 @@
#include <linux/hyperv.h>
+/*
+ * Pre win8 version numbers used in ws2008 and ws 2008 r2 (win7)
+ */
+#define WIN7_SRV_MAJOR 3
+#define WIN7_SRV_MINOR 0
+#define WIN7_SRV_MAJOR_MINOR (WIN7_SRV_MAJOR << 16 | WIN7_SRV_MINOR)
+
+#define WIN8_SRV_MAJOR 4
+#define WIN8_SRV_MINOR 0
+#define WIN8_SRV_MAJOR_MINOR (WIN8_SRV_MAJOR << 16 | WIN8_SRV_MINOR)
/*
* Global state maintained for transaction that is being processed.
@@ -593,8 +603,19 @@ void hv_kvp_onchannelcallback(void *context)
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
+ /*
+ * We start with win8 version and if the host cannot
+ * support that we use the previous version.
+ */
+ if (vmbus_prep_negotiate_resp(icmsghdrp, negop,
+ recv_buffer, UTIL_FW_MAJOR_MINOR,
+ WIN8_SRV_MAJOR_MINOR))
+ goto done;
+
vmbus_prep_negotiate_resp(icmsghdrp, negop,
- recv_buffer, MAX_SRV_VER, MAX_SRV_VER);
+ recv_buffer, UTIL_FW_MAJOR_MINOR,
+ WIN7_SRV_MAJOR_MINOR);
+
} else {
kvp_msg = (struct hv_kvp_msg *)&recv_buffer[
sizeof(struct vmbuspipe_hdr) +
@@ -626,6 +647,7 @@ void hv_kvp_onchannelcallback(void *context)
return;
}
+done:
icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
| ICMSGHDRFLAG_RESPONSE;
diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index 8ad5653..e4572f3 100644
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -24,6 +24,10 @@
#include <linux/workqueue.h>
#include <linux/hyperv.h>
+#define VSS_MAJOR 5
+#define VSS_MINOR 0
+#define VSS_MAJOR_MINOR (VSS_MAJOR << 16 | VSS_MINOR)
+
/*
@@ -186,18 +190,8 @@ void hv_vss_onchannelcallback(void *context)
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
vmbus_prep_negotiate_resp(icmsghdrp, negop,
- recv_buffer, MAX_SRV_VER, MAX_SRV_VER);
- /*
- * We currently negotiate the highest number the
- * host has presented. If this version is not
- * atleast 5.0, reject.
- */
- negop = (struct icmsg_negotiate *)&recv_buffer[
- sizeof(struct vmbuspipe_hdr) +
- sizeof(struct icmsg_hdr)];
-
- if (negop->icversion_data[1].major < 5)
- negop->icframe_vercnt = 0;
+ recv_buffer, UTIL_FW_MAJOR_MINOR,
+ VSS_MAJOR_MINOR);
} else {
vss_msg = (struct hv_vss_msg *)&recv_buffer[
sizeof(struct vmbuspipe_hdr) +
diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index 2f561c5..c16164d 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -28,6 +28,18 @@
#include <linux/reboot.h>
#include <linux/hyperv.h>
+#define SHUTDOWN_MAJOR 3
+#define SHUTDOWN_MINOR 0
+#define SHUTDOWN_MAJOR_MINOR (SHUTDOWN_MAJOR << 16 | SHUTDOWN_MINOR)
+
+#define TIMESYNCH_MAJOR 3
+#define TIMESYNCH_MINOR 0
+#define TIMESYNCH_MAJOR_MINOR (TIMESYNCH_MAJOR << 16 | TIMESYNCH_MINOR)
+
+#define HEARTBEAT_MAJOR 3
+#define HEARTBEAT_MINOR 0
+#define HEARTBEAT_MAJOR_MINOR (HEARTBEAT_MAJOR << 16 | HEARTBEAT_MINOR)
+
static void shutdown_onchannelcallback(void *context);
static struct hv_util_service util_shutdown = {
.util_cb = shutdown_onchannelcallback,
@@ -87,7 +99,8 @@ static void shutdown_onchannelcallback(void *context)
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
vmbus_prep_negotiate_resp(icmsghdrp, negop,
- shut_txf_buf, MAX_SRV_VER, MAX_SRV_VER);
+ shut_txf_buf, UTIL_FW_MAJOR_MINOR,
+ SHUTDOWN_MAJOR_MINOR);
} else {
shutdown_msg =
(struct shutdown_msg_data *)&shut_txf_buf[
@@ -213,7 +226,8 @@ static void timesync_onchannelcallback(void *context)
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
vmbus_prep_negotiate_resp(icmsghdrp, NULL, time_txf_buf,
- MAX_SRV_VER, MAX_SRV_VER);
+ UTIL_FW_MAJOR_MINOR,
+ TIMESYNCH_MAJOR_MINOR);
} else {
timedatap = (struct ictimesync_data *)&time_txf_buf[
sizeof(struct vmbuspipe_hdr) +
@@ -253,7 +267,8 @@ static void heartbeat_onchannelcallback(void *context)
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
vmbus_prep_negotiate_resp(icmsghdrp, NULL,
- hbeat_txf_buf, MAX_SRV_VER, MAX_SRV_VER);
+ hbeat_txf_buf, UTIL_FW_MAJOR_MINOR,
+ HEARTBEAT_MAJOR_MINOR);
} else {
heartbeat_msg =
(struct heartbeat_msg_data *)&hbeat_txf_buf[
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index fae8bac..4994907 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -27,6 +27,14 @@
#include <linux/types.h>
+/*
+ * Framework version for util services.
+ */
+
+#define UTIL_FW_MAJOR 3
+#define UTIL_FW_MINOR 0
+#define UTIL_FW_MAJOR_MINOR (UTIL_FW_MAJOR << 16 | UTIL_FW_MINOR)
+
/*
* Implementation of host controlled snapshot of the guest.
@@ -1494,7 +1502,7 @@ struct hyperv_service_callback {
};
#define MAX_SRV_VER 0x7ffffff
-extern void vmbus_prep_negotiate_resp(struct icmsg_hdr *,
+extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *,
struct icmsg_negotiate *, u8 *, int,
int);
--
1.7.4.1
> -----Original Message-----
> From: K. Y. Srinivasan [mailto:[email protected]]
> Sent: Tuesday, July 02, 2013 1:32 PM
> To: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Cc: KY Srinivasan
> Subject: [PATCH 1/1] Drivers: hv: util: Fix a bug in version negotiation code for util
> services
>
> The current code picked the highest version advertised by the host. WS2012 R2
> has implemented a protocol version for KVP that is not compatible with prior
> protocol versions of KVP. Fix the bug in the current code by explicitly specifying
> the protocol version that the guest can support.
Greg,
Do you want me to resubmit this patch.
Regards,
K. Y
>
> Signed-off-by: K. Y. Srinivasan <[email protected]>
> Reviewed-by: Haiyang Zhang <[email protected]>
> ---
> drivers/hv/channel_mgmt.c | 75 +++++++++++++++++++++++++++++++-------
> ------
> drivers/hv/hv_kvp.c | 24 ++++++++++++++-
> drivers/hv/hv_snapshot.c | 18 +++-------
> drivers/hv/hv_util.c | 21 +++++++++++--
> include/linux/hyperv.h | 10 +++++-
> 5 files changed, 109 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 0df7590..12ec8c8 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -48,30 +48,39 @@ struct vmbus_channel_message_table_entry {
> * @negop is of type &struct icmsg_negotiate.
> * Set up and fill in default negotiate response message.
> *
> - * The max_fw_version specifies the maximum framework version that
> - * we can support and max _srv_version specifies the maximum service
> - * version we can support. A special value MAX_SRV_VER can be
> - * specified to indicate that we can handle the maximum version
> - * exposed by the host.
> + * The fw_version specifies the framework version that
> + * we can support and srv_version specifies the service
> + * version we can support.
> *
> * Mainly used by Hyper-V drivers.
> */
> -void vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
> +bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
> struct icmsg_negotiate *negop, u8 *buf,
> - int max_fw_version, int max_srv_version)
> + int fw_version, int srv_version)
> {
> - int icframe_vercnt;
> - int icmsg_vercnt;
> + int icframe_major, icframe_minor;
> + int icmsg_major, icmsg_minor;
> + int fw_major, fw_minor;
> + int srv_major, srv_minor;
> int i;
> + bool found_match = false;
>
> icmsghdrp->icmsgsize = 0x10;
> + fw_major = (fw_version >> 16);
> + fw_minor = (fw_version & 0xFFFF);
> +
> + srv_major = (srv_version >> 16);
> + srv_minor = (srv_version & 0xFFFF);
>
> negop = (struct icmsg_negotiate *)&buf[
> sizeof(struct vmbuspipe_hdr) +
> sizeof(struct icmsg_hdr)];
>
> - icframe_vercnt = negop->icframe_vercnt;
> - icmsg_vercnt = negop->icmsg_vercnt;
> + icframe_major = negop->icframe_vercnt;
> + icframe_minor = 0;
> +
> + icmsg_major = negop->icmsg_vercnt;
> + icmsg_minor = 0;
>
> /*
> * Select the framework version number we will
> @@ -79,26 +88,48 @@ void vmbus_prep_negotiate_resp(struct icmsg_hdr
> *icmsghdrp,
> */
>
> for (i = 0; i < negop->icframe_vercnt; i++) {
> - if (negop->icversion_data[i].major <= max_fw_version)
> - icframe_vercnt = negop->icversion_data[i].major;
> + if ((negop->icversion_data[i].major == fw_major) &&
> + (negop->icversion_data[i].minor == fw_minor)) {
> + icframe_major = negop->icversion_data[i].major;
> + icframe_minor = negop->icversion_data[i].minor;
> + found_match = true;
> + }
> }
>
> + if (!found_match)
> + goto fw_error;
> +
> + found_match = false;
> +
> for (i = negop->icframe_vercnt;
> (i < negop->icframe_vercnt + negop->icmsg_vercnt); i++) {
> - if (negop->icversion_data[i].major <= max_srv_version)
> - icmsg_vercnt = negop->icversion_data[i].major;
> + if ((negop->icversion_data[i].major == srv_major) &&
> + (negop->icversion_data[i].minor == srv_minor)) {
> + icmsg_major = negop->icversion_data[i].major;
> + icmsg_minor = negop->icversion_data[i].minor;
> + found_match = true;
> + }
> }
>
> /*
> - * Respond with the maximum framework and service
> + * Respond with the framework and service
> * version numbers we can support.
> */
> - negop->icframe_vercnt = 1;
> - negop->icmsg_vercnt = 1;
> - negop->icversion_data[0].major = icframe_vercnt;
> - negop->icversion_data[0].minor = 0;
> - negop->icversion_data[1].major = icmsg_vercnt;
> - negop->icversion_data[1].minor = 0;
> +
> +fw_error:
> + if (!found_match) {
> + negop->icframe_vercnt = 0;
> + negop->icmsg_vercnt = 0;
> + } else {
> + negop->icframe_vercnt = 1;
> + negop->icmsg_vercnt = 1;
> + }
> +
> + negop->icversion_data[0].major = icframe_major;
> + negop->icversion_data[0].minor = icframe_minor;
> + negop->icversion_data[1].major = icmsg_major;
> + negop->icversion_data[1].minor = icmsg_minor;
> + return found_match;
> }
>
> EXPORT_SYMBOL_GPL(vmbus_prep_negotiate_resp);
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index ed50e9e..5312720 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -29,6 +29,16 @@
> #include <linux/hyperv.h>
>
>
> +/*
> + * Pre win8 version numbers used in ws2008 and ws 2008 r2 (win7)
> + */
> +#define WIN7_SRV_MAJOR 3
> +#define WIN7_SRV_MINOR 0
> +#define WIN7_SRV_MAJOR_MINOR (WIN7_SRV_MAJOR << 16 |
> WIN7_SRV_MINOR)
> +
> +#define WIN8_SRV_MAJOR 4
> +#define WIN8_SRV_MINOR 0
> +#define WIN8_SRV_MAJOR_MINOR (WIN8_SRV_MAJOR << 16 |
> WIN8_SRV_MINOR)
>
> /*
> * Global state maintained for transaction that is being processed.
> @@ -593,8 +603,19 @@ void hv_kvp_onchannelcallback(void *context)
> sizeof(struct vmbuspipe_hdr)];
>
> if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> + /*
> + * We start with win8 version and if the host cannot
> + * support that we use the previous version.
> + */
> + if (vmbus_prep_negotiate_resp(icmsghdrp, negop,
> + recv_buffer, UTIL_FW_MAJOR_MINOR,
> + WIN8_SRV_MAJOR_MINOR))
> + goto done;
> +
> vmbus_prep_negotiate_resp(icmsghdrp, negop,
> - recv_buffer, MAX_SRV_VER, MAX_SRV_VER);
> + recv_buffer, UTIL_FW_MAJOR_MINOR,
> + WIN7_SRV_MAJOR_MINOR);
> +
> } else {
> kvp_msg = (struct hv_kvp_msg *)&recv_buffer[
> sizeof(struct vmbuspipe_hdr) +
> @@ -626,6 +647,7 @@ void hv_kvp_onchannelcallback(void *context)
> return;
>
> }
> +done:
>
> icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
> | ICMSGHDRFLAG_RESPONSE;
> diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
> index 8ad5653..e4572f3 100644
> --- a/drivers/hv/hv_snapshot.c
> +++ b/drivers/hv/hv_snapshot.c
> @@ -24,6 +24,10 @@
> #include <linux/workqueue.h>
> #include <linux/hyperv.h>
>
> +#define VSS_MAJOR 5
> +#define VSS_MINOR 0
> +#define VSS_MAJOR_MINOR (VSS_MAJOR << 16 | VSS_MINOR)
> +
>
>
> /*
> @@ -186,18 +190,8 @@ void hv_vss_onchannelcallback(void *context)
>
> if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> vmbus_prep_negotiate_resp(icmsghdrp, negop,
> - recv_buffer, MAX_SRV_VER, MAX_SRV_VER);
> - /*
> - * We currently negotiate the highest number the
> - * host has presented. If this version is not
> - * atleast 5.0, reject.
> - */
> - negop = (struct icmsg_negotiate *)&recv_buffer[
> - sizeof(struct vmbuspipe_hdr) +
> - sizeof(struct icmsg_hdr)];
> -
> - if (negop->icversion_data[1].major < 5)
> - negop->icframe_vercnt = 0;
> + recv_buffer, UTIL_FW_MAJOR_MINOR,
> + VSS_MAJOR_MINOR);
> } else {
> vss_msg = (struct hv_vss_msg *)&recv_buffer[
> sizeof(struct vmbuspipe_hdr) +
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index 2f561c5..c16164d 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -28,6 +28,18 @@
> #include <linux/reboot.h>
> #include <linux/hyperv.h>
>
> +#define SHUTDOWN_MAJOR 3
> +#define SHUTDOWN_MINOR 0
> +#define SHUTDOWN_MAJOR_MINOR (SHUTDOWN_MAJOR << 16 |
> SHUTDOWN_MINOR)
> +
> +#define TIMESYNCH_MAJOR 3
> +#define TIMESYNCH_MINOR 0
> +#define TIMESYNCH_MAJOR_MINOR (TIMESYNCH_MAJOR << 16 |
> TIMESYNCH_MINOR)
> +
> +#define HEARTBEAT_MAJOR 3
> +#define HEARTBEAT_MINOR 0
> +#define HEARTBEAT_MAJOR_MINOR (HEARTBEAT_MAJOR << 16 |
> HEARTBEAT_MINOR)
> +
> static void shutdown_onchannelcallback(void *context);
> static struct hv_util_service util_shutdown = {
> .util_cb = shutdown_onchannelcallback,
> @@ -87,7 +99,8 @@ static void shutdown_onchannelcallback(void *context)
>
> if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> vmbus_prep_negotiate_resp(icmsghdrp, negop,
> - shut_txf_buf, MAX_SRV_VER,
> MAX_SRV_VER);
> + shut_txf_buf, UTIL_FW_MAJOR_MINOR,
> + SHUTDOWN_MAJOR_MINOR);
> } else {
> shutdown_msg =
> (struct shutdown_msg_data *)&shut_txf_buf[
> @@ -213,7 +226,8 @@ static void timesync_onchannelcallback(void *context)
>
> if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> vmbus_prep_negotiate_resp(icmsghdrp, NULL,
> time_txf_buf,
> - MAX_SRV_VER, MAX_SRV_VER);
> + UTIL_FW_MAJOR_MINOR,
> + TIMESYNCH_MAJOR_MINOR);
> } else {
> timedatap = (struct ictimesync_data *)&time_txf_buf[
> sizeof(struct vmbuspipe_hdr) +
> @@ -253,7 +267,8 @@ static void heartbeat_onchannelcallback(void *context)
>
> if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> vmbus_prep_negotiate_resp(icmsghdrp, NULL,
> - hbeat_txf_buf, MAX_SRV_VER, MAX_SRV_VER);
> + hbeat_txf_buf, UTIL_FW_MAJOR_MINOR,
> + HEARTBEAT_MAJOR_MINOR);
> } else {
> heartbeat_msg =
> (struct heartbeat_msg_data *)&hbeat_txf_buf[
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index fae8bac..4994907 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -27,6 +27,14 @@
>
> #include <linux/types.h>
>
> +/*
> + * Framework version for util services.
> + */
> +
> +#define UTIL_FW_MAJOR 3
> +#define UTIL_FW_MINOR 0
> +#define UTIL_FW_MAJOR_MINOR (UTIL_FW_MAJOR << 16 |
> UTIL_FW_MINOR)
> +
>
> /*
> * Implementation of host controlled snapshot of the guest.
> @@ -1494,7 +1502,7 @@ struct hyperv_service_callback {
> };
>
> #define MAX_SRV_VER 0x7ffffff
> -extern void vmbus_prep_negotiate_resp(struct icmsg_hdr *,
> +extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *,
> struct icmsg_negotiate *, u8 *, int,
> int);
>
> --
> 1.7.4.1
>
>
On Wed, Jul 17, 2013 at 12:35:42PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: K. Y. Srinivasan [mailto:[email protected]]
> > Sent: Tuesday, July 02, 2013 1:32 PM
> > To: [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]
> > Cc: KY Srinivasan
> > Subject: [PATCH 1/1] Drivers: hv: util: Fix a bug in version negotiation code for util
> > services
> >
> > The current code picked the highest version advertised by the host. WS2012 R2
> > has implemented a protocol version for KVP that is not compatible with prior
> > protocol versions of KVP. Fix the bug in the current code by explicitly specifying
> > the protocol version that the guest can support.
>
> Greg,
>
> Do you want me to resubmit this patch.
No, it's not lost, I'm just working on 3.11 patches at the moment, I'll
push this one to 3.12 later.
thanks,
greg k-h
> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> Sent: Wednesday, July 17, 2013 1:13 PM
> To: KY Srinivasan
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 1/1] Drivers: hv: util: Fix a bug in version negotiation code for
> util services
>
> On Wed, Jul 17, 2013 at 12:35:42PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: K. Y. Srinivasan [mailto:[email protected]]
> > > Sent: Tuesday, July 02, 2013 1:32 PM
> > > To: [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]
> > > Cc: KY Srinivasan
> > > Subject: [PATCH 1/1] Drivers: hv: util: Fix a bug in version negotiation code for
> util
> > > services
> > >
> > > The current code picked the highest version advertised by the host. WS2012
> R2
> > > has implemented a protocol version for KVP that is not compatible with prior
> > > protocol versions of KVP. Fix the bug in the current code by explicitly
> specifying
> > > the protocol version that the guest can support.
> >
> > Greg,
> >
> > Do you want me to resubmit this patch.
>
> No, it's not lost, I'm just working on 3.11 patches at the moment, I'll
> push this one to 3.12 later.
Thanks Greg. Since you checked in other patches that I had sent after I had sent this one, I was thinking perhaps this got lost.
Regards,
K. Y
On Tue, Jul 02, K. Y. Srinivasan wrote:
> The current code picked the highest version advertised by the host. WS2012 R2
> has implemented a protocol version for KVP that is not compatible with prior
> protocol versions of KVP. Fix the bug in the current code by explicitly specifying
> the protocol version that the guest can support.
> -void vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
> +bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
> struct icmsg_negotiate *negop, u8 *buf,
> - int max_fw_version, int max_srv_version)
> + int fw_version, int srv_version)
> +fw_error:
> + if (!found_match) {
> + negop->icframe_vercnt = 0;
> + negop->icmsg_vercnt = 0;
> + } else {
> + negop->icframe_vercnt = 1;
> + negop->icmsg_vercnt = 1;
> + }
> +
> + negop->icversion_data[0].major = icframe_major;
> + negop->icversion_data[0].minor = icframe_minor;
> + negop->icversion_data[1].major = icmsg_major;
> + negop->icversion_data[1].minor = icmsg_minor;
> + return found_match;
> }
The new vmbus_prep_negotiate_resp function modifies the *negop buffer if
no match is found. If called twice from hv_kvp_onchannelcallback, the
second call uses the modified buffer from the the first call. As a
result the matching will fail for both calls. If I leave the buffer
alone, kvp works again on ws2008.
I suggest to let callers deal with error handling. Also as a cleanup,
vmbus_prep_negotiate_resp does not make use of the negop passed to it.
So that arg can be removed.
Olaf
On Wed, Sep 04, Olaf Hering wrote:
> I suggest to let callers deal with error handling. Also as a cleanup,
> vmbus_prep_negotiate_resp does not make use of the negop passed to it.
> So that arg can be removed.
Simplify vmbus_prep_negotiate_resp. If we take the optimistic approach
that negotiation will not fail for any of the callers of
vmbus_prep_negotiate_resp this patch on top of yours should fix 2008 and
2012r2.
Olaf
---
drivers/hv/channel_mgmt.c | 22 +++++++++-------------
drivers/hv/hv_kvp.c | 8 ++++----
drivers/hv/hv_snapshot.c | 3 +--
drivers/hv/hv_util.c | 7 +++----
include/linux/hyperv.h | 4 +---
5 files changed, 18 insertions(+), 26 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 12ec8c8..dcaad3e 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -41,7 +41,6 @@ struct vmbus_channel_message_table_entry {
/**
* vmbus_prep_negotiate_resp() - Create default response for Hyper-V Negotiate message
* @icmsghdrp: Pointer to msg header structure
- * @icmsg_negotiate: Pointer to negotiate message structure
* @buf: Raw buffer channel data
*
* @icmsghdrp is of type &struct icmsg_hdr.
@@ -54,10 +53,10 @@ struct vmbus_channel_message_table_entry {
*
* Mainly used by Hyper-V drivers.
*/
-bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
- struct icmsg_negotiate *negop, u8 *buf,
+bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp, u8 *buf,
int fw_version, int srv_version)
{
+ struct icmsg_negotiate *negop;
int icframe_major, icframe_minor;
int icmsg_major, icmsg_minor;
int fw_major, fw_minor;
@@ -65,7 +64,6 @@ bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
int i;
bool found_match = false;
- icmsghdrp->icmsgsize = 0x10;
fw_major = (fw_version >> 16);
fw_minor = (fw_version & 0xFFFF);
@@ -116,19 +114,17 @@ bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
* version numbers we can support.
*/
-fw_error:
- if (!found_match) {
- negop->icframe_vercnt = 0;
- negop->icmsg_vercnt = 0;
- } else {
+ if (found_match) {
+ icmsghdrp->icmsgsize = 0x10;
negop->icframe_vercnt = 1;
negop->icmsg_vercnt = 1;
+ negop->icversion_data[0].major = icframe_major;
+ negop->icversion_data[0].minor = icframe_minor;
+ negop->icversion_data[1].major = icmsg_major;
+ negop->icversion_data[1].minor = icmsg_minor;
}
- negop->icversion_data[0].major = icframe_major;
- negop->icversion_data[0].minor = icframe_minor;
- negop->icversion_data[1].major = icmsg_major;
- negop->icversion_data[1].minor = icmsg_minor;
+fw_error:
return found_match;
}
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 5312720..31e338a 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -584,7 +584,6 @@ void hv_kvp_onchannelcallback(void *context)
struct hv_kvp_msg *kvp_msg;
struct icmsg_hdr *icmsghdrp;
- struct icmsg_negotiate *negop = NULL;
if (kvp_transaction.active) {
/*
@@ -607,14 +606,15 @@ void hv_kvp_onchannelcallback(void *context)
* We start with win8 version and if the host cannot
* support that we use the previous version.
*/
- if (vmbus_prep_negotiate_resp(icmsghdrp, negop,
+ if (vmbus_prep_negotiate_resp(icmsghdrp,
recv_buffer, UTIL_FW_MAJOR_MINOR,
WIN8_SRV_MAJOR_MINOR))
goto done;
- vmbus_prep_negotiate_resp(icmsghdrp, negop,
+ if (vmbus_prep_negotiate_resp(icmsghdrp,
recv_buffer, UTIL_FW_MAJOR_MINOR,
- WIN7_SRV_MAJOR_MINOR);
+ WIN7_SRV_MAJOR_MINOR))
+ goto done;
} else {
kvp_msg = (struct hv_kvp_msg *)&recv_buffer[
diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index e4572f3..51e8203 100644
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -170,7 +170,6 @@ void hv_vss_onchannelcallback(void *context)
struct icmsg_hdr *icmsghdrp;
- struct icmsg_negotiate *negop = NULL;
if (vss_transaction.active) {
/*
@@ -189,7 +188,7 @@ void hv_vss_onchannelcallback(void *context)
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
- vmbus_prep_negotiate_resp(icmsghdrp, negop,
+ vmbus_prep_negotiate_resp(icmsghdrp,
recv_buffer, UTIL_FW_MAJOR_MINOR,
VSS_MAJOR_MINOR);
} else {
diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index c16164d..01d7b17 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -88,7 +88,6 @@ static void shutdown_onchannelcallback(void *context)
struct shutdown_msg_data *shutdown_msg;
struct icmsg_hdr *icmsghdrp;
- struct icmsg_negotiate *negop = NULL;
vmbus_recvpacket(channel, shut_txf_buf,
PAGE_SIZE, &recvlen, &requestid);
@@ -98,7 +97,7 @@ static void shutdown_onchannelcallback(void *context)
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
- vmbus_prep_negotiate_resp(icmsghdrp, negop,
+ vmbus_prep_negotiate_resp(icmsghdrp,
shut_txf_buf, UTIL_FW_MAJOR_MINOR,
SHUTDOWN_MAJOR_MINOR);
} else {
@@ -225,7 +224,7 @@ static void timesync_onchannelcallback(void *context)
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
- vmbus_prep_negotiate_resp(icmsghdrp, NULL, time_txf_buf,
+ vmbus_prep_negotiate_resp(icmsghdrp, time_txf_buf,
UTIL_FW_MAJOR_MINOR,
TIMESYNCH_MAJOR_MINOR);
} else {
@@ -266,7 +265,7 @@ static void heartbeat_onchannelcallback(void *context)
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
- vmbus_prep_negotiate_resp(icmsghdrp, NULL,
+ vmbus_prep_negotiate_resp(icmsghdrp,
hbeat_txf_buf, UTIL_FW_MAJOR_MINOR,
HEARTBEAT_MAJOR_MINOR);
} else {
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 4994907..084a858 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1502,9 +1502,7 @@ struct hyperv_service_callback {
};
#define MAX_SRV_VER 0x7ffffff
-extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *,
- struct icmsg_negotiate *, u8 *, int,
- int);
+extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *, u8 *, int, int);
int hv_kvp_init(struct hv_util_service *);
void hv_kvp_deinit(void);
> -----Original Message-----
> From: Olaf Hering [mailto:[email protected]]
> Sent: Wednesday, September 04, 2013 8:07 AM
> To: KY Srinivasan
> Cc: [email protected]; [email protected]
> Subject: Re: [PATCH 1/1] Drivers: hv: util: Fix a bug in version negotiation code for
> util services
>
> On Wed, Sep 04, Olaf Hering wrote:
>
> > I suggest to let callers deal with error handling. Also as a cleanup,
> > vmbus_prep_negotiate_resp does not make use of the negop passed to it.
> > So that arg can be removed.
>
> Simplify vmbus_prep_negotiate_resp. If we take the optimistic approach
> that negotiation will not fail for any of the callers of
> vmbus_prep_negotiate_resp this patch on top of yours should fix 2008 and
> 2012r2.
Thanks Olaf. I would prefer that we explicitly fail the negotiations than assume that it will succeed.
Essentially, I want the same behavior as what we had before but only for the final version being
negotiated. If it is ok with you, I can spin up the patch and send it out.
Regards,
K. Y
>
> Olaf
>
> ---
> drivers/hv/channel_mgmt.c | 22 +++++++++-------------
> drivers/hv/hv_kvp.c | 8 ++++----
> drivers/hv/hv_snapshot.c | 3 +--
> drivers/hv/hv_util.c | 7 +++----
> include/linux/hyperv.h | 4 +---
> 5 files changed, 18 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 12ec8c8..dcaad3e 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -41,7 +41,6 @@ struct vmbus_channel_message_table_entry {
> /**
> * vmbus_prep_negotiate_resp() - Create default response for Hyper-V
> Negotiate message
> * @icmsghdrp: Pointer to msg header structure
> - * @icmsg_negotiate: Pointer to negotiate message structure
> * @buf: Raw buffer channel data
> *
> * @icmsghdrp is of type &struct icmsg_hdr.
> @@ -54,10 +53,10 @@ struct vmbus_channel_message_table_entry {
> *
> * Mainly used by Hyper-V drivers.
> */
> -bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
> - struct icmsg_negotiate *negop, u8 *buf,
> +bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp, u8 *buf,
> int fw_version, int srv_version)
> {
> + struct icmsg_negotiate *negop;
> int icframe_major, icframe_minor;
> int icmsg_major, icmsg_minor;
> int fw_major, fw_minor;
> @@ -65,7 +64,6 @@ bool vmbus_prep_negotiate_resp(struct icmsg_hdr
> *icmsghdrp,
> int i;
> bool found_match = false;
>
> - icmsghdrp->icmsgsize = 0x10;
> fw_major = (fw_version >> 16);
> fw_minor = (fw_version & 0xFFFF);
>
> @@ -116,19 +114,17 @@ bool vmbus_prep_negotiate_resp(struct icmsg_hdr
> *icmsghdrp,
> * version numbers we can support.
> */
>
> -fw_error:
> - if (!found_match) {
> - negop->icframe_vercnt = 0;
> - negop->icmsg_vercnt = 0;
> - } else {
> + if (found_match) {
> + icmsghdrp->icmsgsize = 0x10;
> negop->icframe_vercnt = 1;
> negop->icmsg_vercnt = 1;
> + negop->icversion_data[0].major = icframe_major;
> + negop->icversion_data[0].minor = icframe_minor;
> + negop->icversion_data[1].major = icmsg_major;
> + negop->icversion_data[1].minor = icmsg_minor;
> }
>
> - negop->icversion_data[0].major = icframe_major;
> - negop->icversion_data[0].minor = icframe_minor;
> - negop->icversion_data[1].major = icmsg_major;
> - negop->icversion_data[1].minor = icmsg_minor;
> +fw_error:
> return found_match;
> }
>
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index 5312720..31e338a 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -584,7 +584,6 @@ void hv_kvp_onchannelcallback(void *context)
> struct hv_kvp_msg *kvp_msg;
>
> struct icmsg_hdr *icmsghdrp;
> - struct icmsg_negotiate *negop = NULL;
>
> if (kvp_transaction.active) {
> /*
> @@ -607,14 +606,15 @@ void hv_kvp_onchannelcallback(void *context)
> * We start with win8 version and if the host cannot
> * support that we use the previous version.
> */
> - if (vmbus_prep_negotiate_resp(icmsghdrp, negop,
> + if (vmbus_prep_negotiate_resp(icmsghdrp,
> recv_buffer, UTIL_FW_MAJOR_MINOR,
> WIN8_SRV_MAJOR_MINOR))
> goto done;
>
> - vmbus_prep_negotiate_resp(icmsghdrp, negop,
> + if (vmbus_prep_negotiate_resp(icmsghdrp,
> recv_buffer, UTIL_FW_MAJOR_MINOR,
> - WIN7_SRV_MAJOR_MINOR);
> + WIN7_SRV_MAJOR_MINOR))
> + goto done;
>
> } else {
> kvp_msg = (struct hv_kvp_msg *)&recv_buffer[
> diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
> index e4572f3..51e8203 100644
> --- a/drivers/hv/hv_snapshot.c
> +++ b/drivers/hv/hv_snapshot.c
> @@ -170,7 +170,6 @@ void hv_vss_onchannelcallback(void *context)
>
>
> struct icmsg_hdr *icmsghdrp;
> - struct icmsg_negotiate *negop = NULL;
>
> if (vss_transaction.active) {
> /*
> @@ -189,7 +188,7 @@ void hv_vss_onchannelcallback(void *context)
> sizeof(struct vmbuspipe_hdr)];
>
> if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> - vmbus_prep_negotiate_resp(icmsghdrp, negop,
> + vmbus_prep_negotiate_resp(icmsghdrp,
> recv_buffer, UTIL_FW_MAJOR_MINOR,
> VSS_MAJOR_MINOR);
> } else {
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index c16164d..01d7b17 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -88,7 +88,6 @@ static void shutdown_onchannelcallback(void *context)
> struct shutdown_msg_data *shutdown_msg;
>
> struct icmsg_hdr *icmsghdrp;
> - struct icmsg_negotiate *negop = NULL;
>
> vmbus_recvpacket(channel, shut_txf_buf,
> PAGE_SIZE, &recvlen, &requestid);
> @@ -98,7 +97,7 @@ static void shutdown_onchannelcallback(void *context)
> sizeof(struct vmbuspipe_hdr)];
>
> if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> - vmbus_prep_negotiate_resp(icmsghdrp, negop,
> + vmbus_prep_negotiate_resp(icmsghdrp,
> shut_txf_buf, UTIL_FW_MAJOR_MINOR,
> SHUTDOWN_MAJOR_MINOR);
> } else {
> @@ -225,7 +224,7 @@ static void timesync_onchannelcallback(void *context)
> sizeof(struct vmbuspipe_hdr)];
>
> if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> - vmbus_prep_negotiate_resp(icmsghdrp, NULL,
> time_txf_buf,
> + vmbus_prep_negotiate_resp(icmsghdrp, time_txf_buf,
> UTIL_FW_MAJOR_MINOR,
> TIMESYNCH_MAJOR_MINOR);
> } else {
> @@ -266,7 +265,7 @@ static void heartbeat_onchannelcallback(void *context)
> sizeof(struct vmbuspipe_hdr)];
>
> if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> - vmbus_prep_negotiate_resp(icmsghdrp, NULL,
> + vmbus_prep_negotiate_resp(icmsghdrp,
> hbeat_txf_buf, UTIL_FW_MAJOR_MINOR,
> HEARTBEAT_MAJOR_MINOR);
> } else {
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 4994907..084a858 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1502,9 +1502,7 @@ struct hyperv_service_callback {
> };
>
> #define MAX_SRV_VER 0x7ffffff
> -extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *,
> - struct icmsg_negotiate *, u8 *, int,
> - int);
> +extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *, u8 *, int, int);
>
> int hv_kvp_init(struct hv_util_service *);
> void hv_kvp_deinit(void);
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, Sep 04, KY Srinivasan wrote:
> Thanks Olaf. I would prefer that we explicitly fail the negotiations than assume that it will succeed.
> Essentially, I want the same behavior as what we had before but only for the final version being
> negotiated. If it is ok with you, I can spin up the patch and send it out.
Sure, whatever works.
Olaf