> -----Original Message-----
> From: K. Y. Srinivasan [mailto:k...@microsoft.com]
> Sent: Tuesday, July 02, 2013 1:32 PM
> To: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
> jasow...@redhat.com
> 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 <k...@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiya...@microsoft.com>
> ---
> 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
>
>
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
