[PATCH]: An implementation of HyperV KVP functionality

[PATCH]: An implementation of HyperV KVP functionality

[Ky Srinivasan]

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

I am enclosing a patch that implements the KVP (Key Value Pair) functionality for Linux guests on HyperV. This functionality allows Microsoft Management stack to query information from the guest. This functionality is implemented in two parts: (a) A kernel component that communicates with the host and (b) A user level daemon that implements data gathering. The attached patch (kvp.patch) implements the kernel component. I am also attaching the code for the user-level daemon (kvp_daemon.c)  for reference.

Regards,

K. Y


[-- Attachment #2: kvp.patch --]
[-- Type: text/plain, Size: 18054 bytes --]

From: K. Y. Srinivasan <ksrinivasan@novell.com>

Subject: An implementation of key/value pair feature (KVP) for Linux 
on HyperV.

Signed-off-by: K. Y. Srinivasan <ksrinivasan@novell.com> 

Index: linux.trees.git/drivers/staging/hv/kvp.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux.trees.git/drivers/staging/hv/kvp.c	2010-11-11 13:45:17.000000000 -0500
@@ -0,0 +1,404 @@
+/*
+ * An implementation of key value pair (KVP) functionality for Linux.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan <ksrinivasan@novell.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT.  See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+
+#include <linux/net.h>
+#include <linux/nls.h>
+#include <linux/connector.h>
+
+#include "logging.h"
+#include "osd.h"
+#include "vmbus.h"
+#include "vmbus_packet_format.h"
+#include "vmbus_channel_interface.h"
+#include "version_info.h"
+#include "channel.h"
+#include "vmbus_private.h"
+#include "vmbus_api.h"
+#include "utils.h"
+#include "kvp.h"
+
+
+/*
+ *
+ * The following definitions are shared with the user-mode component; do not
+ * change any of this without making the corresponding changes in
+ * the KVP user-mode component.
+ */
+
+#define CN_KVP_VAL             0x1 /* This supports queries from the kernel */
+#define CN_KVP_USER_VAL       0x2 /* This supports queries from the user */
+
+
+/*
+ * KVP protocol: The user mode component first registers with the
+ * the kernel component. Subsequently, the kernel component requests, data
+ * for the specified keys. In response to this message the user mode component
+ * fills in the value corresponding to the specified key. We overload the
+ * sequence field in the cn_msg header to define our KVP message types.
+ *
+ * XXXKYS: Have a shared header file between the user and kernel (TODO)
+ */
+
+enum kvp_op {
+	KVP_REGISTER = 0, /* Register the user mode component */
+	KVP_KERNEL_GET,/*Kernel is requesting the value for the specified key*/
+	KVP_KERNEL_SET, /*Kernel is providing the value for the specified key*/
+	KVP_USER_GET, /*User is requesting the value for the specified key*/
+	KVP_USER_SET /*User is providing the value for the specified key*/
+};
+
+
+
+#define KVP_KEY_SIZE    512
+#define KVP_VALUE_SIZE  2048
+
+
+typedef struct kvp_msg {
+	__u32 kvp_key; /* Key */
+	__u8  kvp_value[0]; /* Corresponding value */
+} kvp_msg_t;
+
+/*
+ * End of shared definitions.
+ */
+
+/*
+ * Registry value types.
+ */
+
+#define REG_SZ 1
+
+/*
+ * Array of keys we support in Linux.
+ *
+ */
+#define KVP_MAX_KEY	10
+#define KVP_LIC_VERSION 1
+
+
+static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
+				"IntegrationServicesVersion",
+				"NetworkAddressIPv4",
+				"NetworkAddressIPv6",
+				"OSBuildNumber",
+				"OSName",
+				"OSMajorVersion",
+				"OSMinorVersion",
+				"OSVersion",
+				"ProcessorArchitecture",
+				};
+
+/*
+ * Global state maintained for transaction that is being processed.
+ * Note that only one transaction can be active at any point in time.
+ *
+ * This state is set when we receive a request from the host; we
+ * cleanup this state when the transaction is completed - when we respond
+ * to the host with the key value.
+ */
+
+static u8 *recv_buffer; /* the receive buffer that we allocated */
+static int recv_len; /* number of bytes received. */
+static struct vmbus_channel *recv_channel; /*chn on which we got the request*/
+static u64 recv_req_id; /* request ID. */
+static int kvp_current_index;
+
+static int
+kvp_send_key(int index);
+
+static
+void kvp_respond_to_host(int key, char *value);
+
+static struct cb_id kvp_id = { CN_KVP_IDX, CN_KVP_VAL };
+static char kvp_name[] = "kvp_kernel_module";
+
+static bool kvp_transaction_active;
+
+static struct timer_list kvp_timer;
+
+static void kvp_timer_func(unsigned long __data)
+{
+	u32	key = *((u32 *)__data);
+	/*
+	 * If the timer fires, the user-mode component has not responded;
+	 * process the pending transaction.
+	 */
+	kvp_respond_to_host(key, "Guest timed out");
+}
+
+/*
+ * Callback when data is received from user mode.
+ */
+
+static void
+kvp_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
+{
+	struct kvp_msg *message;
+
+	message = (struct kvp_msg *)msg->data;
+	if (msg->seq == KVP_REGISTER)
+		printk(KERN_WARNING "KVP: user-mode registering done.\n");
+
+	if (msg->seq == KVP_USER_SET) {
+		/*
+		 * Complete the transaction by forwarding the key value
+		 * to the host. But first, cancel the timeout.
+		 */
+		if (del_timer_sync(&kvp_timer))
+			kvp_respond_to_host(message->kvp_key,
+						message->kvp_value);
+	}
+}
+
+static int
+kvp_send_key(int index)
+{
+	struct cn_msg *msg;
+
+	msg = kzalloc(sizeof(*msg) + sizeof(kvp_msg_t) , GFP_ATOMIC);
+
+	if (msg) {
+		msg->id.idx =  CN_KVP_IDX;
+		msg->id.val = CN_KVP_VAL;
+		msg->seq = KVP_KERNEL_GET;
+		((kvp_msg_t *)msg->data)->kvp_key = index;
+		msg->len = sizeof(kvp_msg_t);
+		cn_netlink_send(msg, 0, GFP_ATOMIC);
+		kfree(msg);
+		return 0;
+	}
+	return 1;
+}
+
+/*
+ * Send a response back to the host.
+ * key specifies the key for which the value is being returned.
+ */
+
+static
+void kvp_respond_to_host(int key, char *value)
+{
+	ic_kvp_msg_t *kvp_msg;
+	ic_kvp_msg_enumerate_t *kvp_data;
+	char	*key_name;
+	struct icmsg_hdr *icmsghdrp;
+	int	keylen, valuelen;
+	u8	*buf;
+	u32	buf_len;
+	struct vmbus_channel *channel;
+	u64	req_id;
+
+	/*
+	 * If a transaction is not active; log and return.
+	 */
+
+	if (!kvp_transaction_active) {
+		/*
+		 * This is a spurious call!
+		 */
+		printk(KERN_WARNING "KVP: Transaction not active\n");
+		return;
+	}
+	/*
+	 * Copy the global state for completing the transaction. Note that
+	 * only one transaction can be active at a time.
+	 */
+
+	buf = recv_buffer;
+	buf_len = recv_len;
+	channel = recv_channel;
+	req_id = recv_req_id;
+
+	kvp_transaction_active = false;
+
+	icmsghdrp = (struct icmsg_hdr *)&buf[sizeof(struct vmbuspipe_hdr)];
+	kvp_msg = (ic_kvp_msg_t *)&buf[sizeof(struct vmbuspipe_hdr) +
+					sizeof(struct icmsg_hdr)];
+	kvp_data = &kvp_msg->kvp_data;
+	key_name = kvp_keys[key];
+
+	/*
+	 * The windows host expects the key/value pair to be encoded
+	 * in utf16.
+	 */
+	keylen = utf8s_to_utf16s(key_name, strlen(key_name),
+				(wchar_t *)kvp_data->data.key);
+	kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
+	valuelen = utf8s_to_utf16s(value, strlen(value),
+				(wchar_t *)kvp_data->data.value);
+	kvp_data->data.value_size = 2*(valuelen + 1); /* utf16 encoding */
+
+	kvp_data->data.value_type = REG_SZ; /* all our values are strings */
+	icmsghdrp->status = HV_S_OK;
+
+	icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION | ICMSGHDRFLAG_RESPONSE;
+
+	vmbus_sendpacket(channel, buf, buf_len, req_id,
+				VmbusPacketTypeDataInBand, 0);
+
+	/*
+	 * Free up the buffer that was allocatted when we received a message
+	 * from the host.
+	 */
+	kfree(buf);
+}
+
+/*
+ * This callback is invoked when we get a KVP message from the host.
+ * The host ensures that only one KVP transaction can be active at a time.
+ * KVP implementation in Linux needs to forward the key to a user-mde
+ * component to retrive the corresponding value. Consequently, we cannot
+ * respond to the host in the conext of this callback. Since the host
+ * guarantees that at most only one transaction can be active at a time,
+ * we stash away the transaction state in a set of global variables.
+ */
+
+void kvp_onchannelcallback(void *context)
+{
+	struct vmbus_channel *channel = context;
+	u8 *buf;
+	u32 buflen, recvlen;
+	u64 requestid;
+
+	ic_kvp_msg_t *kvp_msg;
+	ic_kvp_msg_enumerate_t *kvp_data;
+
+	struct icmsg_hdr *icmsghdrp;
+	struct icmsg_negotiate *negop = NULL;
+
+
+	buflen = PAGE_SIZE;
+	buf = kmalloc(buflen, GFP_ATOMIC);
+
+	vmbus_recvpacket(channel, buf, buflen, &recvlen, &requestid);
+
+	if (recvlen > 0) {
+		DPRINT_DBG(VMBUS, "KVP packet: len=%d, requestid=%lld",
+			   recvlen, requestid);
+
+		icmsghdrp = (struct icmsg_hdr *)&buf[
+			sizeof(struct vmbuspipe_hdr)];
+
+		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
+			prep_negotiate_resp(icmsghdrp, negop, buf);
+		} else {
+			kvp_msg = (ic_kvp_msg_t *)&buf[
+				sizeof(struct vmbuspipe_hdr) +
+				sizeof(struct icmsg_hdr)];
+
+			kvp_data = &kvp_msg->kvp_data;
+
+			/*
+			 * We only support the "get" operation on
+			 * "ICKvpExchangePoolAuto" pool.
+			 */
+
+			if ((kvp_msg->kvp_hdr.pool != ICKvpExchangePoolAuto) ||
+				(kvp_msg->kvp_hdr.operation !=
+				ICKvpExchangeOperationEnumerate) ||
+				kvp_transaction_active) {
+				if (kvp_transaction_active)
+					printk(KERN_WARNING
+						"KVP: Invalid call\n");
+				icmsghdrp->status = HV_E_FAIL;
+				goto callback_done;
+			}
+
+			/*
+			 * Stash away this global state for completing the
+			 * transaction; note transactions are serialized.
+			 */
+			recv_buffer = buf;
+			recv_len = recvlen;
+			recv_channel = channel;
+			recv_req_id = requestid;
+
+			switch (kvp_data->index) {
+			case (KVP_MAX_KEY):
+				/*
+				 * We don't support this key
+				 * and any key beyond this.
+				 */
+				icmsghdrp->status = HV_E_FAIL;
+				goto callback_done;
+
+			case (KVP_LIC_VERSION):
+				kvp_transaction_active = true;
+				kvp_respond_to_host(kvp_data->index,
+						HV_DRV_VERSION);
+				return;
+			default:
+				/*
+				 * Get the information from the
+				 * user-mode component.
+				 * component. This transaction will be
+				 * completed when we get the value from
+				 * the user-mode component.
+				 * Set a timeout to deal with
+				 * user-mode not responding.
+				 */
+				kvp_current_index = kvp_data->index;
+				mod_timer(&kvp_timer, jiffies+75);
+				kvp_transaction_active = true;
+				kvp_send_key(kvp_data->index);
+				return;
+			}
+
+		}
+
+callback_done:
+
+		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
+			| ICMSGHDRFLAG_RESPONSE;
+
+		vmbus_sendpacket(channel, buf,
+				       recvlen, requestid,
+				       VmbusPacketTypeDataInBand, 0);
+	}
+
+	kfree(buf);
+
+
+}
+
+int
+kvp_init(void)
+{
+	int err;
+
+	err = cn_add_callback(&kvp_id, kvp_name, kvp_cn_callback);
+	if (err)
+		return err;
+
+	setup_timer(&kvp_timer, kvp_timer_func,
+		(unsigned long)&kvp_current_index);
+	return 0;
+}
+
+void kvp_deinit(void)
+{
+	cn_del_callback(&kvp_id);
+	del_timer_sync(&kvp_timer);
+}
+
Index: linux.trees.git/drivers/staging/hv/Makefile
===================================================================
--- linux.trees.git.orig/drivers/staging/hv/Makefile	2010-11-10 14:01:55.000000000 -0500
+++ linux.trees.git/drivers/staging/hv/Makefile	2010-11-11 11:24:54.000000000 -0500
@@ -2,7 +2,7 @@ obj-$(CONFIG_HYPERV)		+= hv_vmbus.o hv_t
 obj-$(CONFIG_HYPERV_STORAGE)	+= hv_storvsc.o
 obj-$(CONFIG_HYPERV_BLOCK)	+= hv_blkvsc.o
 obj-$(CONFIG_HYPERV_NET)	+= hv_netvsc.o
-obj-$(CONFIG_HYPERV_UTILS)	+= hv_utils.o
+obj-$(CONFIG_HYPERV_UTILS)	+= hv_util.o
 
 hv_vmbus-y := vmbus_drv.o osd.o \
 		 vmbus.o hv.o connection.o channel.o \
@@ -10,3 +10,4 @@ hv_vmbus-y := vmbus_drv.o osd.o \
 hv_storvsc-y := storvsc_drv.o storvsc.o
 hv_blkvsc-y := blkvsc_drv.o blkvsc.o
 hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o
+hv_util-y :=  hv_utils.o kvp.o
Index: linux.trees.git/drivers/staging/hv/kvp.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux.trees.git/drivers/staging/hv/kvp.h	2010-11-10 14:03:47.000000000 -0500
@@ -0,0 +1,101 @@
+/*
+ * An implementation of key value pair (KVP) functionality for Linux.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan <ksrinivasan@novell.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT.  See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+#ifndef	_KVP_H
+#define	_KVP_H_
+
+/*
+ * Maximum value size - used for both key names and value data, and includes
+ * any applicable NULL terminators.
+ *
+ * Note:  This limit is somewhat arbitrary, but falls easily within what is
+ * supported for all native guests (back to Win 2000) and what is reasonable
+ * for the IC KVP exchange functionality.  Note that Windows Me/98/95 are
+ * limited to 255 character key names.
+ *
+ * MSDN recommends not storing data values larger than 2048 bytes in the
+ * registry.
+ *
+ * Note:  This value is used in defining the KVP exchange message - this value
+ * cannot be modified without affecting the message size and compatability.
+ */
+
+/*
+ * bytes, including any null terminators
+ */
+#define IC_KVP_EXCHANGE_MAX_VALUE_SIZE          (2048)
+
+
+/*
+ * Maximum key size - the registry limit for the length of an entry name
+ * is 256 characters, including the null terminator
+ */
+
+#define IC_KVP_EXCHANGE_MAX_KEY_SIZE            (512)
+
+
+typedef enum {
+	ICKvpExchangeOperationGet = 0,
+	ICKvpExchangeOperationSet,
+	ICKvpExchangeOperationDelete,
+	ICKvpExchangeOperationEnumerate,
+	ICKvpExchangeOperationCount /* Number of operations, must be last. */
+} IC_KVP_EXCHANGE_OPERATION;
+
+typedef enum {
+	ICKvpExchangePoolExternal = 0,
+	ICKvpExchangePoolGuest,
+	ICKvpExchangePoolAuto,
+	ICKvpExchangePoolAutoExternal,
+	ICKvpExchangePoolInternal,
+	ICKvpExchangePoolCount /* Number of pools, must be last. */
+} IC_KVP_EXCHANGE_POOL;
+
+typedef struct ic_kvp_hdr {
+	u8 operation;
+	u8 pool;
+} ic_kvp_hdr_t;
+
+typedef struct ic_kvp_exchg_msg_value {
+	u32 value_type;
+	u32 key_size;
+	u32 value_size;
+	u8 key[IC_KVP_EXCHANGE_MAX_KEY_SIZE];
+	u8 value[IC_KVP_EXCHANGE_MAX_VALUE_SIZE];
+} ic_kvp_exchg_msg_value_t;
+
+typedef struct ic_kvp__msg_enumerate {
+	u32 index;
+	ic_kvp_exchg_msg_value_t data;
+} ic_kvp_msg_enumerate_t;
+
+typedef struct ic_kvp_msg {
+	ic_kvp_hdr_t	kvp_hdr;
+	ic_kvp_msg_enumerate_t	kvp_data;
+} ic_kvp_msg_t;
+
+int kvp_init(void);
+void kvp_deinit(void);
+void kvp_onchannelcallback(void *);
+
+#endif	/* _KVP_H */
+
Index: linux.trees.git/drivers/staging/hv/utils.h
===================================================================
--- linux.trees.git.orig/drivers/staging/hv/utils.h	2010-11-10 10:30:23.000000000 -0500
+++ linux.trees.git/drivers/staging/hv/utils.h	2010-11-10 14:03:47.000000000 -0500
@@ -102,6 +102,7 @@ struct ictimesync_data{
 #define HV_SHUTDOWN_MSG		0
 #define HV_TIMESYNC_MSG		1
 #define HV_HEARTBEAT_MSG	2
+#define HV_KVP_MSG		3
 
 struct hyperv_service_callback {
 	u8 msg_type;
Index: linux.trees.git/drivers/staging/hv/hv_utils.c
===================================================================
--- linux.trees.git.orig/drivers/staging/hv/hv_utils.c	2010-11-10 14:02:40.000000000 -0500
+++ linux.trees.git/drivers/staging/hv/hv_utils.c	2010-11-11 13:42:38.000000000 -0500
@@ -37,6 +37,7 @@
 #include "vmbus_private.h"
 #include "vmbus_api.h"
 #include "utils.h"
+#include "kvp.h"
 
 
 static void shutdown_onchannelcallback(void *context)
@@ -268,6 +269,9 @@ static int __init init_hyperv_utils(void
 {
 	printk(KERN_INFO "Registering HyperV Utility Driver\n");
 
+	if (kvp_init())
+		return -ENODEV;
+
 	if (!dmi_check_system(hv_utils_dmi_table))
 		return -ENODEV;
 
@@ -283,6 +287,10 @@ static int __init init_hyperv_utils(void
 		&heartbeat_onchannelcallback;
 	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &heartbeat_onchannelcallback;
 
+	hv_cb_utils[HV_KVP_MSG].channel->OnChannelCallback =
+		&kvp_onchannelcallback;
+
+
 	return 0;
 }
 
@@ -301,6 +309,12 @@ static void exit_hyperv_utils(void)
 	hv_cb_utils[HV_HEARTBEAT_MSG].channel->OnChannelCallback =
 		&chn_cb_negotiate;
 	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &chn_cb_negotiate;
+
+	hv_cb_utils[HV_KVP_MSG].channel->OnChannelCallback =
+		&chn_cb_negotiate;
+
+	kvp_deinit();
+
 }
 
 module_init(init_hyperv_utils);
Index: linux.trees.git/include/linux/connector.h
===================================================================
--- linux.trees.git.orig/include/linux/connector.h	2010-11-09 17:22:15.000000000 -0500
+++ linux.trees.git/include/linux/connector.h	2010-11-11 13:14:52.000000000 -0500
@@ -42,8 +42,9 @@
 #define CN_VAL_DM_USERSPACE_LOG		0x1
 #define CN_IDX_DRBD			0x8
 #define CN_VAL_DRBD			0x1
+#define CN_KVP_IDX			0x9     /* MSFT KVP functionality */
 
-#define CN_NETLINK_USERS		8
+#define CN_NETLINK_USERS		10
 
 /*
  * Maximum connector's message size.

[-- Attachment #3: kvp_daemon.c --]
[-- Type: application/octet-stream, Size: 10284 bytes --]

/*
 * An implementation of key value pair (KVP) functionality for Linux.
 *
 *
 * Copyright (C) 2010, Novell, Inc.
 * Author : K. Y. Srinivasan <ksrinivasan@novell.com>
 *
 * This program is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 as published
 * by the Free Software Foundation.
 *
 * This program is distributed in the hope that it will be useful, but
 * WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
 * NON INFRINGEMENT.  See the GNU General Public License for more
 * details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
 *
 */


#include <sys/types.h>
#include <sys/socket.h>
#include <sys/poll.h>
#include <sys/utsname.h>
#include <asm/types.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <arpa/inet.h>
#include <linux/connector.h>
#include <linux/netlink.h>
#include <sys/socket.h>
#include <ifaddrs.h>
#include <netdb.h>
#include <syslog.h>

/*
 * KYS: TODO. Need to register these in the kernel.
 *
 * The following definitions are shared with the in-kernel component; do not
 * change any of this without making the corresponding changes in 
 * the KVP kernel component.
 */
#define CN_KVP_IDX		0x9     /* MSFT KVP functionality */
#define CN_KVP_VAL		0x1 /* This supports queries from the kernel */
#define CN_KVP_USER_VAL		0x2 /* This supports queries from the user  */

/*
 * KVP protocol: The user mode component first registers with the 
 * the kernel component. Subsequently, the kernel component requests, data
 * for the specified keys. In response to this message the user mode component
 * fills in the value corresponding to the specified key. We overload the 
 * sequence field in the cn_msg header to define our KVP message types.
 *
 * We use this infrastructure for also supporting queries from user mode
 * application for state that may be maintained in the KVP kernel component.
 *
 * XXXKYS: Have a shared header file between the user and kernel (TODO)
 */

enum kvp_op {
	KVP_REGISTER = 0, /* Register the user mode component*/
	KVP_KERNEL_GET, /*Kernel is requesting the value for the specified key*/
	KVP_KERNEL_SET, /*Kernel is providing the value for the specified key*/
	KVP_USER_GET, /*User is requesting the value for the specified key*/
	KVP_USER_SET /*User is providing the value for the specified key*/
};

#define KVP_KEY_SIZE	512
#define KVP_VALUE_SIZE	2048
	
typedef struct kvp_msg {
	__u32	kvp_key;
	__u8	kvp_value[KVP_VALUE_SIZE];
} kvp_msg_t;

typedef enum {
	FullyQualifiedDomainName = 0,
	IntegrationServicesVersion, /*This key is serviced in the kernel*/
	NetworkAddressIPv4,
	NetworkAddressIPv6,
	OSBuildNumber,
	OSName,
	OSMajorVersion,
	OSMinorVersion,
	OSVersion,
	ProcessorArchitecture
} kvp_key_name_t;
 
/*
 * End of shared definitions.
 */

static char kvp_send_buffer[4096];
static char kvp_recv_buffer[4096];
static struct sockaddr_nl addr;

static char os_name[100];
static char os_major[50];
static char os_minor[50];
static char processor_arch[50];
static char os_build[100];

void kvp_get_os_info(void)
{
	FILE	*file;
	char	*eol;
	struct utsname buf;

	uname(&buf);
	strcpy(os_build, buf.release);
	strcpy(processor_arch, buf.machine);

	if ((file = fopen("/etc/SuSE-release", "r")) != NULL)
		goto kvp_osinfo_found;
	if ((file  = fopen("/etc/SuSE-release", "r")) != NULL)
		goto kvp_osinfo_found;
	/*
	 * Add code for other supported platforms.
	 */

	/*
	 * We don't have information about the os.
	 */
	strcpy(os_name, "Linux");
	strcpy(os_major, "0");
	strcpy(os_minor, "0");
	return;

kvp_osinfo_found:
	fgets(os_name, 99, file);
	eol = index(os_name, '\n');
	*eol = '\0';
	fgets(os_major, 49, file);
	eol = index(os_major, '\n');
	*eol = '\0';
	fgets(os_minor, 49, file);
	eol = index(os_minor, '\n');
	*eol = '\0';
	fclose(file);
	return;
}
	
static int
kvp_get_ip_address(int family, char *buffer, int length)
{
	struct ifaddrs *ifap;
	struct ifaddrs *curp;
	int ipv4_len = strlen("255.255.255.255") + 1;
	int ipv6_len = strlen("ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff")+1; 
	int offset = 0;
	const char *str;
	char tmp[50];
	int error = 0;

	/*
	 * On entry into this function, the buffer is capable of holding the
	 * maximum key value (2048 bytes).
	 */
	
	if (getifaddrs(&ifap)) {
		strcpy(buffer, "getifaddrs failed\n");
		return 1;
	}

	curp = ifap;
	while (curp != NULL) {
		if ((curp->ifa_addr != NULL) &&  
		   (curp->ifa_addr->sa_family == family)) {
			if (family == AF_INET) {
				struct sockaddr_in *addr = 
				(struct sockaddr_in *) curp->ifa_addr;

				if (!(str = inet_ntop(family, &addr->sin_addr, 
					tmp, 50))) {
					strcpy(buffer, "inet_ntop failed\n");
					error = 1;
					goto getaddr_done;
				}
				if (offset == 0) 
					strcpy(buffer, tmp);
				else 
					strcat(buffer, tmp);
				strcat(buffer, ";");
				
				offset += strlen(str) +1;
				if ((length - offset) < (ipv4_len + 1)) {
					goto getaddr_done;
				}
			} else {
			/*
		 	 * We only support AF_INET and AF_INET6
			 * and the list of addresses is seperated by a ";".
			 */
				struct sockaddr_in6 *addr = 
				(struct sockaddr_in6 *) curp->ifa_addr;

				if (!(str = inet_ntop(family,
					&addr->sin6_addr.s6_addr,
					tmp, 50))) {
					strcpy(buffer, "inet_ntop failed\n");
					error = 1;
					goto getaddr_done;
				}
				if (offset == 0) 
					strcpy(buffer, tmp);
				else 
					strcat(buffer, tmp);
				strcat(buffer, ";");
				offset += strlen(str) +1;
				if ((length - offset) < (ipv6_len +1)) {
					goto getaddr_done;
				}
			}
				
		}
		curp = curp->ifa_next;
	}

getaddr_done:
	freeifaddrs(ifap);
	return error;
}


static int
kvp_get_domain_name(char *buffer, int length)
{
	struct addrinfo	hints, *info ;
	gethostname(buffer, length);
	int error =0;

	memset(&hints, 0, sizeof(hints));
	hints.ai_family = AF_INET; /*Get only ipv4 addrinfo. */
	hints.ai_socktype = SOCK_STREAM;
	hints.ai_flags = AI_CANONNAME;
	
	if ((error = getaddrinfo(buffer, "http", &hints, &info)) !=0) {
		strcpy(buffer, "getaddrinfo failed\n");
		error = 1;
		goto get_domain_done;
	}
	strcpy(buffer, info->ai_canonname);
get_domain_done:
	freeaddrinfo(info);
	return (error);
}

static int 
netlink_send(int fd, struct cn_msg *msg)
{
	struct nlmsghdr *nlh;
	unsigned int size;
	struct msghdr message;
	char buffer[64];
	struct iovec iov[2];

	size = NLMSG_SPACE(sizeof(struct cn_msg) + msg->len);

	nlh = (struct nlmsghdr *)buffer;
	nlh->nlmsg_seq = 0;
	nlh->nlmsg_pid = getpid();
	nlh->nlmsg_type = NLMSG_DONE;
	nlh->nlmsg_len = NLMSG_LENGTH(size - sizeof(*nlh));
	nlh->nlmsg_flags = 0;
	
	iov[0].iov_base = nlh;
	iov[0].iov_len = sizeof(*nlh);

	iov[1].iov_base = msg;
	iov[1].iov_len = size;

	memset(&message, 0, sizeof(message));
	message.msg_name = &addr;
	message.msg_namelen = sizeof(addr); 
	message.msg_iov = iov;
	message.msg_iovlen = 2;

	return (sendmsg(fd, &message, 0));
}

main(void)
{
	int fd, len, sock_opt;
	int error;
	struct cn_msg *message;
	struct pollfd pfd;
	struct nlmsghdr *incoming_msg;
	struct cn_msg	*incoming_cn_msg;
	char	*key_value;
	kvp_key_name_t key_name;

	daemon(1, 0);
	openlog("KVP", 0, LOG_USER);
	syslog(LOG_INFO, "KVP starting; pid is:%d", getpid());
	/*
	 * Retrieve OS release information.
	 */
	kvp_get_os_info();

	fd = socket(AF_NETLINK, SOCK_DGRAM, NETLINK_CONNECTOR);
	if (fd < 0) {
		syslog(LOG_ERR, "netlink socket creation failed; error:%d",fd);
		exit -1;
	}
	addr.nl_family = AF_NETLINK;
	addr.nl_pad = 0;
	addr.nl_pid = 0;
	addr.nl_groups = CN_KVP_IDX;


	if ((error = bind(fd, (struct sockaddr *)&addr, sizeof(addr))) < 0) {
		syslog(LOG_ERR, "bind failed; error:%d",error);
		close(fd);
		exit -1;
	}
	sock_opt = addr.nl_groups;
	setsockopt(fd, 270, 1, &sock_opt, sizeof(sock_opt));
	/*
	 * Register ourselves with the kernel.
	 */
	message = (struct cn_msg *)kvp_send_buffer;
	message->id.idx = CN_KVP_IDX;
	message->id.val = CN_KVP_VAL;
	message->seq = KVP_REGISTER;
	message->ack = 0;
	message->len = 0;

	len = netlink_send(fd, message);
	if (len < 0) {
		syslog(LOG_ERR, "netlink_send failed; error:%d", len);
		close(fd);
		exit -1;
	}
	
	pfd.fd = fd;
	
	while (1) {
		pfd.events = POLLIN;
		pfd.revents = 0;
		poll(&pfd, 1, -1);

		len = recv(fd, kvp_recv_buffer, sizeof(kvp_recv_buffer), 0);

		if (len < 0) {
			syslog(LOG_ERR, "recv failed; error:%d", len);
			close(fd);
			return -1;
		}
	
		incoming_msg = (struct nlmsghdr *)kvp_recv_buffer;
		incoming_cn_msg = (struct cn_msg *)NLMSG_DATA(incoming_msg);

		if (incoming_cn_msg->seq != KVP_KERNEL_GET) {
			continue;
		}

		key_name = 
		((struct kvp_msg *)incoming_cn_msg->data)->kvp_key;
		key_value =
		((struct kvp_msg *)incoming_cn_msg->data)->kvp_value;

		switch (key_name) {
			case FullyQualifiedDomainName:
				kvp_get_domain_name(key_value, KVP_VALUE_SIZE);
				break;
			case NetworkAddressIPv4:
				kvp_get_ip_address(AF_INET, key_value, 
						KVP_VALUE_SIZE);
				break;
			case NetworkAddressIPv6:
				kvp_get_ip_address(AF_INET6, key_value, 
						KVP_VALUE_SIZE);
				break;
			case OSBuildNumber:
				strcpy(key_value, os_build);
				break;
			case OSName:
				strcpy(key_value, os_name);
				break;
			case OSMajorVersion:
				strcpy(key_value, os_major);
				break;
			case OSMinorVersion:
				strcpy(key_value, os_minor);
				break;
			case OSVersion:
				strcpy(key_value, os_build);
				break;
			case ProcessorArchitecture:
				strcpy(key_value, processor_arch);
				break;
			default:	
				strcpy(key_value, "Unknown Key");
				break;
		}
		/*
		 * Send the value back to the kernel. The response is 
		 * already in the receive buffer. Update the cn_msg header to 
		 * reflect the key value that has been added to the message
		 */

		incoming_cn_msg->id.idx = CN_KVP_IDX;
		incoming_cn_msg->id.val = CN_KVP_VAL;
		incoming_cn_msg->seq = KVP_USER_SET; 
		incoming_cn_msg->ack = 0;
		incoming_cn_msg->len = KVP_KEY_SIZE + strlen(key_value);

		len = netlink_send(fd, incoming_cn_msg);
		if (len < 0) {
			syslog(LOG_ERR, "net_link send failed; error:%d", len);
			exit -1;
		}
	}

}

[-- Attachment #4: Type: text/plain, Size: 159 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

Re: [PATCH]: An implementation of HyperV KVP functionality

[Stephen Hemminger]

On Thu, 11 Nov 2010 13:03:10 -0700
"Ky Srinivasan" <ksrinivasan@novell.com> wrote:

> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
> +				"IntegrationServicesVersion",
> +				"NetworkAddressIPv4",
> +				"NetworkAddressIPv6",
> +				"OSBuildNumber",
> +				"OSName",
> +				"OSMajorVersion",
> +				"OSMinorVersion",
> +				"OSVersion",
> +				"ProcessorArchitecture",
> +				};

Minor nit:
static const char *kvp_keys[KVP_MAX_KEY] = {
	"FullQualifiedDomainName",
...

+/*
+ * Global state maintained for transaction that is being processed.
+ * Note that only one transaction can be active at any point in time.
+ *
+ * This state is set when we receive a request from the host; we
+ * cleanup this state when the transaction is completed - when we respond
+ * to the host with the key value.
+ */
+
+static u8 *recv_buffer; /* the receive buffer that we allocated */
+static int recv_len; /* number of bytes received. */
+static struct vmbus_channel *recv_channel; /*chn on which we got the request*/
+static u64 recv_req_id; /* request ID. */
+static int kvp_current_index;
+

I would put all the state variables for the transaction in one
structure, 

+static void kvp_timer_func(unsigned long __data)
+{
+	u32	key = *((u32 *)__data);
+	/*
+	 * If the timer fires, the user-mode component has not responded;
+	 * process the pending transaction.
+	 */
+	kvp_respond_to_host(key, "Guest timed out");
+}

delayed_work is sometimes better for things like this, since it
runs in user context and can sleep.

+			case (KVP_MAX_KEY):
+				/*
+				 * We don't support this key
+				 * and any key beyond this.
+				 */
+				icmsghdrp->status = HV_E_FAIL;
+				goto callback_done;
+

case labels do not need parens

Re: [PATCH]: An implementation of HyperV KVP functionality

[Greg KH]

On Thu, Nov 11, 2010 at 01:03:10PM -0700, Ky Srinivasan wrote:
> +/*
> + * An implementation of key value pair (KVP) functionality for Linux.
> + *
> + *
> + * Copyright (C) 2010, Novell, Inc.
> + * Author : K. Y. Srinivasan <ksrinivasan@novell.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.

This is all that is needed.

> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + * NON INFRINGEMENT.  See the GNU General Public License for more
> + * details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.

You can delete this chunk.

> +/*
> + * KVP protocol: The user mode component first registers with the
> + * the kernel component. Subsequently, the kernel component requests, data
> + * for the specified keys. In response to this message the user mode component
> + * fills in the value corresponding to the specified key. We overload the
> + * sequence field in the cn_msg header to define our KVP message types.
> + *
> + * XXXKYS: Have a shared header file between the user and kernel (TODO)
> + */
> +
> +enum kvp_op {
> +	KVP_REGISTER = 0, /* Register the user mode component */
> +	KVP_KERNEL_GET,/*Kernel is requesting the value for the specified key*/
> +	KVP_KERNEL_SET, /*Kernel is providing the value for the specified key*/
> +	KVP_USER_GET, /*User is requesting the value for the specified key*/
> +	KVP_USER_SET /*User is providing the value for the specified key*/
> +};

As these values are shared between the kernel and userspace, you should
specifically define them.

Also, your spaces with the /* stuff is incorrect.

And, "KVP"?  That's very generic, how about, "HYPERV_KVP_..." instead?
That fits the global namespace much better.

s/kvp_op/hyperv_kvp_op/ as well.

> +#define KVP_KEY_SIZE    512
> +#define KVP_VALUE_SIZE  2048
> +
> +
> +typedef struct kvp_msg {
> +	__u32 kvp_key; /* Key */
> +	__u8  kvp_value[0]; /* Corresponding value */
> +} kvp_msg_t;

I thought that kvp_value was really KVP_VALUE_SIZE?

And no typedefs, you did run your code through checkpatch.pl, right?
Why ignore the stuff it spits back at you?


> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
> +				"IntegrationServicesVersion",
> +				"NetworkAddressIPv4",
> +				"NetworkAddressIPv6",
> +				"OSBuildNumber",
> +				"OSName",
> +				"OSMajorVersion",
> +				"OSMinorVersion",
> +				"OSVersion",
> +				"ProcessorArchitecture",
> +				};

Why list these at all, as more might come in the future, and the kernel
really doesn't care about them, right?

> +int
> +kvp_init(void)

All of your global symbols should have "hyperv_" on the front of them.

> --- linux.trees.git.orig/drivers/staging/hv/Makefile	2010-11-10 14:01:55.000000000 -0500
> +++ linux.trees.git/drivers/staging/hv/Makefile	2010-11-11 11:24:54.000000000 -0500
> @@ -2,7 +2,7 @@ obj-$(CONFIG_HYPERV)		+= hv_vmbus.o hv_t
>  obj-$(CONFIG_HYPERV_STORAGE)	+= hv_storvsc.o
>  obj-$(CONFIG_HYPERV_BLOCK)	+= hv_blkvsc.o
>  obj-$(CONFIG_HYPERV_NET)	+= hv_netvsc.o
> -obj-$(CONFIG_HYPERV_UTILS)	+= hv_utils.o
> +obj-$(CONFIG_HYPERV_UTILS)	+= hv_util.o

Ick, you just renamed the kernel module.  Did you really mean to do
this?  What tools are now going to break because you did this?  (I'm
thinking installers here...)

>  
>  hv_vmbus-y := vmbus_drv.o osd.o \
>  		 vmbus.o hv.o connection.o channel.o \
> @@ -10,3 +10,4 @@ hv_vmbus-y := vmbus_drv.o osd.o \
>  hv_storvsc-y := storvsc_drv.o storvsc.o
>  hv_blkvsc-y := blkvsc_drv.o blkvsc.o
>  hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o
> +hv_util-y :=  hv_utils.o kvp.o
> Index: linux.trees.git/drivers/staging/hv/kvp.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux.trees.git/drivers/staging/hv/kvp.h	2010-11-10 14:03:47.000000000 -0500

hyperv_kvp.h as this is going to be a global header file, right?

> +typedef enum {
> +	ICKvpExchangeOperationGet = 0,
> +	ICKvpExchangeOperationSet,
> +	ICKvpExchangeOperationDelete,
> +	ICKvpExchangeOperationEnumerate,
> +	ICKvpExchangeOperationCount /* Number of operations, must be last. */
> +} IC_KVP_EXCHANGE_OPERATION;
> +
> +typedef enum {
> +	ICKvpExchangePoolExternal = 0,
> +	ICKvpExchangePoolGuest,
> +	ICKvpExchangePoolAuto,
> +	ICKvpExchangePoolAutoExternal,
> +	ICKvpExchangePoolInternal,
> +	ICKvpExchangePoolCount /* Number of pools, must be last. */
> +} IC_KVP_EXCHANGE_POOL;
> +
> +typedef struct ic_kvp_hdr {
> +	u8 operation;
> +	u8 pool;
> +} ic_kvp_hdr_t;
> +
> +typedef struct ic_kvp_exchg_msg_value {
> +	u32 value_type;
> +	u32 key_size;
> +	u32 value_size;
> +	u8 key[IC_KVP_EXCHANGE_MAX_KEY_SIZE];
> +	u8 value[IC_KVP_EXCHANGE_MAX_VALUE_SIZE];
> +} ic_kvp_exchg_msg_value_t;
> +
> +typedef struct ic_kvp__msg_enumerate {
> +	u32 index;
> +	ic_kvp_exchg_msg_value_t data;
> +} ic_kvp_msg_enumerate_t;
> +
> +typedef struct ic_kvp_msg {
> +	ic_kvp_hdr_t	kvp_hdr;
> +	ic_kvp_msg_enumerate_t	kvp_data;
> +} ic_kvp_msg_t;

Again, no typedefs, and fix up the names of these structures to be
understandable :)

> --- linux.trees.git.orig/include/linux/connector.h	2010-11-09 17:22:15.000000000 -0500
> +++ linux.trees.git/include/linux/connector.h	2010-11-11 13:14:52.000000000 -0500
> @@ -42,8 +42,9 @@
>  #define CN_VAL_DM_USERSPACE_LOG		0x1
>  #define CN_IDX_DRBD			0x8
>  #define CN_VAL_DRBD			0x1
> +#define CN_KVP_IDX			0x9     /* MSFT KVP functionality */

Did you reserve this number with anyone?  Who?

> -#define CN_NETLINK_USERS		8
> +#define CN_NETLINK_USERS		10

Are you sure you incremented this properly?

thanks,

greg k-h

Re: [PATCH]: An implementation of HyperV KVP functionality

[Greg KH]

On Thu, Nov 11, 2010 at 01:03:10PM -0700, Ky Srinivasan wrote:
> +/*
> + * Array of keys we support in Linux.

Not really, you can support "any" number of keys as the kernel shouldn't
care, or did I get it wrong?

> + *
> + */
> +#define KVP_MAX_KEY	10
> +#define KVP_LIC_VERSION 1

Um, this is a nice magic number, care to explain it a bit more?

> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
> +				"IntegrationServicesVersion",

Looks like it matches up with this, right?  You might want to make that
a bit more "tied" together.

> +			case (KVP_LIC_VERSION):
> +				kvp_transaction_active = true;
> +				kvp_respond_to_host(kvp_data->index,
> +						HV_DRV_VERSION);

Why are you doing this in the kernel?  Why not do it from userspace like
all other messages?

thanks,

greg k-h

Re: [PATCH]: An implementation of HyperV KVP functionality

[Ky Srinivasan]

>>> On 11/11/2010 at  3:49 PM, in message <20101111124904.24010ee5@nehalam>,
Stephen Hemminger <shemminger@vyatta.com> wrote: 
> On Thu, 11 Nov 2010 13:03:10 -0700
> "Ky Srinivasan" <ksrinivasan@novell.com> wrote:
> 
>> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
>> +				"IntegrationServicesVersion",
>> +				"NetworkAddressIPv4",
>> +				"NetworkAddressIPv6",
>> +				"OSBuildNumber",
>> +				"OSName",
>> +				"OSMajorVersion",
>> +				"OSMinorVersion",
>> +				"OSVersion",
>> +				"ProcessorArchitecture",
>> +				};
> 
> Minor nit:
> static const char *kvp_keys[KVP_MAX_KEY] = {
> 	"FullQualifiedDomainName",
Will do.
> ...
> 
> +/*
> + * Global state maintained for transaction that is being processed.
> + * Note that only one transaction can be active at any point in time.
> + *
> + * This state is set when we receive a request from the host; we
> + * cleanup this state when the transaction is completed - when we respond
> + * to the host with the key value.
> + */
> +
> +static u8 *recv_buffer; /* the receive buffer that we allocated */
> +static int recv_len; /* number of bytes received. */
> +static struct vmbus_channel *recv_channel; /*chn on which we got the 
> request*/
> +static u64 recv_req_id; /* request ID. */
> +static int kvp_current_index;
> +
> 
> I would put all the state variables for the transaction in one
> structure,

Will do. 
> 
> +static void kvp_timer_func(unsigned long __data)
> +{
> +	u32	key = *((u32 *)__data);
> +	/*
> +	 * If the timer fires, the user-mode component has not responded;
> +	 * process the pending transaction.
> +	 */
> +	kvp_respond_to_host(key, "Guest timed out");
> +}
> 
> delayed_work is sometimes better for things like this, since it
> runs in user context and can sleep.
Although I don't need to block (sleep) in this code path, I agree with you having a  full context is preferable. I will make the appropriate changes.
> 
> +			case (KVP_MAX_KEY):
> +				/*
> +				 * We don't support this key
> +				 * and any key beyond this.
> +				 */
> +				icmsghdrp->status = HV_E_FAIL;
> +				goto callback_done;
> +
> 
> case labels do not need parens

Thank you; my next version of this patch will incorporate your feedback.

Regards,

K. Y

Re: [PATCH]: An implementation of HyperV KVP functionality

[Ky Srinivasan]

>>> On 11/11/2010 at  3:49 PM, in message <20101111124904.24010ee5@nehalam>,
Stephen Hemminger <shemminger@vyatta.com> wrote: 
> On Thu, 11 Nov 2010 13:03:10 -0700
> "Ky Srinivasan" <ksrinivasan@novell.com> wrote:
> 
>> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
>> +				"IntegrationServicesVersion",
>> +				"NetworkAddressIPv4",
>> +				"NetworkAddressIPv6",
>> +				"OSBuildNumber",
>> +				"OSName",
>> +				"OSMajorVersion",
>> +				"OSMinorVersion",
>> +				"OSVersion",
>> +				"ProcessorArchitecture",
>> +				};
> 
> Minor nit:
> static const char *kvp_keys[KVP_MAX_KEY] = {
> 	"FullQualifiedDomainName",
Will do.
> ...
> 
> +/*
> + * Global state maintained for transaction that is being processed.
> + * Note that only one transaction can be active at any point in time.
> + *
> + * This state is set when we receive a request from the host; we
> + * cleanup this state when the transaction is completed - when we respond
> + * to the host with the key value.
> + */
> +
> +static u8 *recv_buffer; /* the receive buffer that we allocated */
> +static int recv_len; /* number of bytes received. */
> +static struct vmbus_channel *recv_channel; /*chn on which we got the 
> request*/
> +static u64 recv_req_id; /* request ID. */
> +static int kvp_current_index;
> +
> 
> I would put all the state variables for the transaction in one
> structure,

Will do. 
> 
> +static void kvp_timer_func(unsigned long __data)
> +{
> +	u32	key = *((u32 *)__data);
> +	/*
> +	 * If the timer fires, the user-mode component has not responded;
> +	 * process the pending transaction.
> +	 */
> +	kvp_respond_to_host(key, "Guest timed out");
> +}
> 
> delayed_work is sometimes better for things like this, since it
> runs in user context and can sleep.
Although I don't need to block (sleep) in this code path, I agree with you having a  full context is preferable. I will make the appropriate changes.
> 
> +			case (KVP_MAX_KEY):
> +				/*
> +				 * We don't support this key
> +				 * and any key beyond this.
> +				 */
> +				icmsghdrp->status = HV_E_FAIL;
> +				goto callback_done;
> +
> 
> case labels do not need parens

Thank you; my next version of this patch will incorporate your feedback.

Regards,

K. Y

Re: [PATCH]: An implementation of HyperV KVP functionality

[Ky Srinivasan]

>>> On 11/11/2010 at  3:49 PM, in message <20101111124904.24010ee5@nehalam>,
Stephen Hemminger <shemminger@vyatta.com> wrote: 
> On Thu, 11 Nov 2010 13:03:10 -0700
> "Ky Srinivasan" <ksrinivasan@novell.com> wrote:
> 
>> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
>> +				"IntegrationServicesVersion",
>> +				"NetworkAddressIPv4",
>> +				"NetworkAddressIPv6",
>> +				"OSBuildNumber",
>> +				"OSName",
>> +				"OSMajorVersion",
>> +				"OSMinorVersion",
>> +				"OSVersion",
>> +				"ProcessorArchitecture",
>> +				};
> 
> Minor nit:
> static const char *kvp_keys[KVP_MAX_KEY] = {
> 	"FullQualifiedDomainName",
Will do.
> ...
> 
> +/*
> + * Global state maintained for transaction that is being processed.
> + * Note that only one transaction can be active at any point in time.
> + *
> + * This state is set when we receive a request from the host; we
> + * cleanup this state when the transaction is completed - when we respond
> + * to the host with the key value.
> + */
> +
> +static u8 *recv_buffer; /* the receive buffer that we allocated */
> +static int recv_len; /* number of bytes received. */
> +static struct vmbus_channel *recv_channel; /*chn on which we got the 
> request*/
> +static u64 recv_req_id; /* request ID. */
> +static int kvp_current_index;
> +
> 
> I would put all the state variables for the transaction in one
> structure,

Will do. 
> 
> +static void kvp_timer_func(unsigned long __data)
> +{
> +	u32	key = *((u32 *)__data);
> +	/*
> +	 * If the timer fires, the user-mode component has not responded;
> +	 * process the pending transaction.
> +	 */
> +	kvp_respond_to_host(key, "Guest timed out");
> +}
> 
> delayed_work is sometimes better for things like this, since it
> runs in user context and can sleep.
Although I don't need to block (sleep) in this code path, I agree with you having a  full context is preferable. I will make the appropriate changes.
> 
> +			case (KVP_MAX_KEY):
> +				/*
> +				 * We don't support this key
> +				 * and any key beyond this.
> +				 */
> +				icmsghdrp->status = HV_E_FAIL;
> +				goto callback_done;
> +
> 
> case labels do not need parens

Thank you; my next version of this patch will incorporate your feedback.

Regards,

K. Y

Re: [PATCH]: An implementation of HyperV KVP functionality

[Ky Srinivasan]

>>> On 11/11/2010 at  4:15 PM, in message <20101111211548.GA31373@kroah.com>, Greg
KH <greg@kroah.com> wrote: 
> On Thu, Nov 11, 2010 at 01:03:10PM -0700, Ky Srinivasan wrote:
>> +/*
>> + * An implementation of key value pair (KVP) functionality for Linux.
>> + *
>> + *
>> + * Copyright (C) 2010, Novell, Inc.
>> + * Author : K. Y. Srinivasan <ksrinivasan@novell.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published
>> + * by the Free Software Foundation.
> 
> This is all that is needed.
> 
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
>> + * NON INFRINGEMENT.  See the GNU General Public License for more
>> + * details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> 
> You can delete this chunk.

Will do.
> 
>> +/*
>> + * KVP protocol: The user mode component first registers with the
>> + * the kernel component. Subsequently, the kernel component requests, data
>> + * for the specified keys. In response to this message the user mode 
> component
>> + * fills in the value corresponding to the specified key. We overload the
>> + * sequence field in the cn_msg header to define our KVP message types.
>> + *
>> + * XXXKYS: Have a shared header file between the user and kernel (TODO)
>> + */
>> +
>> +enum kvp_op {
>> +	KVP_REGISTER = 0, /* Register the user mode component */
>> +	KVP_KERNEL_GET,/*Kernel is requesting the value for the specified key*/
>> +	KVP_KERNEL_SET, /*Kernel is providing the value for the specified key*/
>> +	KVP_USER_GET, /*User is requesting the value for the specified key*/
>> +	KVP_USER_SET /*User is providing the value for the specified key*/
>> +};
> 
> As these values are shared between the kernel and userspace, you should
> specifically define them.
> 
> Also, your spaces with the /* stuff is incorrect.
> 
> And, "KVP"?  That's very generic, how about, "HYPERV_KVP_..." instead?
> That fits the global namespace much better.

I will change the names; deal with the comments etc.
> 
> s/kvp_op/hyperv_kvp_op/ as well.
> 
>> +#define KVP_KEY_SIZE    512
>> +#define KVP_VALUE_SIZE  2048
>> +
>> +
>> +typedef struct kvp_msg {
>> +	__u32 kvp_key; /* Key */
>> +	__u8  kvp_value[0]; /* Corresponding value */
>> +} kvp_msg_t;
> 
> I thought that kvp_value was really KVP_VALUE_SIZE?

kvp_value is typed information and KVP_VALUE_SIZE specifies the maximum size of the supported value. For instance if kvp_value is a string (which is the case for all the values currently supported), KVP_VALUE_SIZE specifies the maximum size of the string that will be supported.

> 
> And no typedefs, you did run your code through checkpatch.pl, right?
> Why ignore the stuff it spits back at you?
I will fix this.
> 
> 
>> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
>> +				"IntegrationServicesVersion",
>> +				"NetworkAddressIPv4",
>> +				"NetworkAddressIPv6",
>> +				"OSBuildNumber",
>> +				"OSName",
>> +				"OSMajorVersion",
>> +				"OSMinorVersion",
>> +				"OSVersion",
>> +				"ProcessorArchitecture",
>> +				};
> 
> Why list these at all, as more might come in the future, and the kernel
> really doesn't care about them, right?
The core HyperV KVP protocol is such that the host iterates through an index (starting with 0); the guest is free to associate a key with the index and return the associated value. The host side iteration is stopped when the guest returns a failure on an index. MSFT has specified the keys and their ordinal value in their KVP specification. The array you see above is part of that specification.
> 
>> +int
>> +kvp_init(void)
> 
> All of your global symbols should have "hyperv_" on the front of them.

Will do.
> 
>> --- linux.trees.git.orig/drivers/staging/hv/Makefile	2010-11-10 14:01:55.000000000 
> -0500
>> +++ linux.trees.git/drivers/staging/hv/Makefile	2010-11-11 11:24:54.000000000 
> -0500
>> @@ -2,7 +2,7 @@ obj-$(CONFIG_HYPERV)		+= hv_vmbus.o hv_t
>>  obj-$(CONFIG_HYPERV_STORAGE)	+= hv_storvsc.o
>>  obj-$(CONFIG_HYPERV_BLOCK)	+= hv_blkvsc.o
>>  obj-$(CONFIG_HYPERV_NET)	+= hv_netvsc.o
>> -obj-$(CONFIG_HYPERV_UTILS)	+= hv_utils.o
>> +obj-$(CONFIG_HYPERV_UTILS)	+= hv_util.o
> 
> Ick, you just renamed the kernel module.  Did you really mean to do
> this?  What tools are now going to break because you did this?  (I'm
> thinking installers here...)

Oops! I will fix that.
> 
>>  
>>  hv_vmbus-y := vmbus_drv.o osd.o \
>>  		 vmbus.o hv.o connection.o channel.o \
>> @@ -10,3 +10,4 @@ hv_vmbus-y := vmbus_drv.o osd.o \
>>  hv_storvsc-y := storvsc_drv.o storvsc.o
>>  hv_blkvsc-y := blkvsc_drv.o blkvsc.o
>>  hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o
>> +hv_util-y :=  hv_utils.o kvp.o
>> Index: linux.trees.git/drivers/staging/hv/kvp.h
>> ===================================================================
>> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
>> +++ linux.trees.git/drivers/staging/hv/kvp.h	2010-11-10 14:03:47.000000000 -0500
> 
> hyperv_kvp.h as this is going to be a global header file, right?
> 
>> +typedef enum {
>> +	ICKvpExchangeOperationGet = 0,
>> +	ICKvpExchangeOperationSet,
>> +	ICKvpExchangeOperationDelete,
>> +	ICKvpExchangeOperationEnumerate,
>> +	ICKvpExchangeOperationCount /* Number of operations, must be last. */
>> +} IC_KVP_EXCHANGE_OPERATION;
>> +
>> +typedef enum {
>> +	ICKvpExchangePoolExternal = 0,
>> +	ICKvpExchangePoolGuest,
>> +	ICKvpExchangePoolAuto,
>> +	ICKvpExchangePoolAutoExternal,
>> +	ICKvpExchangePoolInternal,
>> +	ICKvpExchangePoolCount /* Number of pools, must be last. */
>> +} IC_KVP_EXCHANGE_POOL;
>> +
>> +typedef struct ic_kvp_hdr {
>> +	u8 operation;
>> +	u8 pool;
>> +} ic_kvp_hdr_t;
>> +
>> +typedef struct ic_kvp_exchg_msg_value {
>> +	u32 value_type;
>> +	u32 key_size;
>> +	u32 value_size;
>> +	u8 key[IC_KVP_EXCHANGE_MAX_KEY_SIZE];
>> +	u8 value[IC_KVP_EXCHANGE_MAX_VALUE_SIZE];
>> +} ic_kvp_exchg_msg_value_t;
>> +
>> +typedef struct ic_kvp__msg_enumerate {
>> +	u32 index;
>> +	ic_kvp_exchg_msg_value_t data;
>> +} ic_kvp_msg_enumerate_t;
>> +
>> +typedef struct ic_kvp_msg {
>> +	ic_kvp_hdr_t	kvp_hdr;
>> +	ic_kvp_msg_enumerate_t	kvp_data;
>> +} ic_kvp_msg_t;
> 
> Again, no typedefs, and fix up the names of these structures to be
> understandable :)

Will do. With regards to making them understandable, I will try!
> 
>> --- linux.trees.git.orig/include/linux/connector.h	2010-11-09 17:22:15.000000000 
> -0500
>> +++ linux.trees.git/include/linux/connector.h	2010-11-11 13:14:52.000000000 
> -0500
>> @@ -42,8 +42,9 @@
>>  #define CN_VAL_DM_USERSPACE_LOG		0x1
>>  #define CN_IDX_DRBD			0x8
>>  #define CN_VAL_DRBD			0x1
>> +#define CN_KVP_IDX			0x9     /* MSFT KVP functionality */
> 
> Did you reserve this number with anyone?  Who?

I sent an email to	Evgeniy Polyakov  (Johnpol@2ka.mipt). The mail bounced back. I was hoping you would help me register this index. Would it make sense to have a separate patch to deal with registering this index?
 
> 
>> -#define CN_NETLINK_USERS		8
>> +#define CN_NETLINK_USERS		10
> 
> Are you sure you incremented this properly?

Oops! I will fix this.
> 
> thanks,
> 
> greg k-h

Re: [PATCH]: An implementation of HyperV KVP functionality

[Ky Srinivasan]

>>> On 11/11/2010 at  4:19 PM, in message <20101111211904.GB31373@kroah.com>, Greg
KH <greg@kroah.com> wrote: 
> On Thu, Nov 11, 2010 at 01:03:10PM -0700, Ky Srinivasan wrote:
>> +/*
>> + * Array of keys we support in Linux.
> 
> Not really, you can support "any" number of keys as the kernel shouldn't
> care, or did I get it wrong?
We currently support only the keys that have been specified in the KVP specification. I have a more detailed response on the core KVP protocol in response to your other email on this topic.
> 
>> + *
>> + */
>> +#define KVP_MAX_KEY	10
>> +#define KVP_LIC_VERSION 1
> 
> Um, this is a nice magic number, care to explain it a bit more?
As I noted in an earlier email, the KVP specification currently requires that we support 10 keys and it also specifies the ordering of these keys.  The information for the key "IntegrationServicesVersion", is only available in the kernel (one of the other LIC drivers defines  this information). 

>> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
>> +				"IntegrationServicesVersion",
> 
> Looks like it matches up with this, right?  You might want to make that
> a bit more "tied" together.
> 
Yes; I will fix this.
>> +			case (KVP_LIC_VERSION):
>> +				kvp_transaction_active = true;
>> +				kvp_respond_to_host(kvp_data->index,
>> +						HV_DRV_VERSION);
> 
> Why are you doing this in the kernel?  Why not do it from userspace like
> all other messages?
This information is only available in the kernel (defined by another LIC driver).

Regards,

K. Y
> 
> thanks,
> 
> greg k-h

Re: [PATCH]: An implementation of HyperV KVP functionality

[Greg KH]

On Fri, Nov 12, 2010 at 11:06:18AM -0700, Ky Srinivasan wrote:
> >> +typedef struct kvp_msg {
> >> +	__u32 kvp_key; /* Key */
> >> +	__u8  kvp_value[0]; /* Corresponding value */
> >> +} kvp_msg_t;
> > 
> > I thought that kvp_value was really KVP_VALUE_SIZE?
> 
> kvp_value is typed information and KVP_VALUE_SIZE specifies the
> maximum size of the supported value. For instance if kvp_value is a
> string (which is the case for all the values currently supported),
> KVP_VALUE_SIZE specifies the maximum size of the string that will be
> supported.

So it's a variable length structure?  How do you konw how long the
structure is, does that depend on the key?

> > And no typedefs, you did run your code through checkpatch.pl, right?
> > Why ignore the stuff it spits back at you?
> I will fix this.
> > 
> > 
> >> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
> >> +				"IntegrationServicesVersion",
> >> +				"NetworkAddressIPv4",
> >> +				"NetworkAddressIPv6",
> >> +				"OSBuildNumber",
> >> +				"OSName",
> >> +				"OSMajorVersion",
> >> +				"OSMinorVersion",
> >> +				"OSVersion",
> >> +				"ProcessorArchitecture",
> >> +				};
> > 
> > Why list these at all, as more might come in the future, and the kernel
> > really doesn't care about them, right?
> The core HyperV KVP protocol is such that the host iterates through an
> index (starting with 0); the guest is free to associate a key with the
> index and return the associated value. The host side iteration is
> stopped when the guest returns a failure on an index. MSFT has
> specified the keys and their ordinal value in their KVP specification.
> The array you see above is part of that specification.

So you match on the string name of the key?  I'm confused, as I didn't
think I saw you matching on the string name, only the key value.

Also, as the kernel isn't handling the key type (with one exception),
why even list these in the kernel at all?  I'm all for documenting
stuff, but don't use code memory for documentation :)

> >> --- linux.trees.git.orig/include/linux/connector.h	2010-11-09 17:22:15.000000000 
> > -0500
> >> +++ linux.trees.git/include/linux/connector.h	2010-11-11 13:14:52.000000000 
> > -0500
> >> @@ -42,8 +42,9 @@
> >>  #define CN_VAL_DM_USERSPACE_LOG		0x1
> >>  #define CN_IDX_DRBD			0x8
> >>  #define CN_VAL_DRBD			0x1
> >> +#define CN_KVP_IDX			0x9     /* MSFT KVP functionality */
> > 
> > Did you reserve this number with anyone?  Who?
> 
> I sent an email to	Evgeniy Polyakov  (Johnpol@2ka.mipt). The mail
> bounced back. I was hoping you would help me register this index.
> Would it make sense to have a separate patch to deal with registering
> this index?

Yes, as I can not modify non-staging code without an ack from that
maintainer.  And I'm pretty sure that Evgeniy's address has just
changed, use "Evgeniy Polyakov <zbr@ioremap.net>" instead.

thanks,

greg k-h

Re: [PATCH]: An implementation of HyperV KVP functionality

[Greg KH]

On Fri, Nov 12, 2010 at 11:29:58AM -0700, Ky Srinivasan wrote:
> 
> 
> >>> On 11/11/2010 at  4:19 PM, in message <20101111211904.GB31373@kroah.com>, Greg
> KH <greg@kroah.com> wrote: 
> > On Thu, Nov 11, 2010 at 01:03:10PM -0700, Ky Srinivasan wrote:
> >> +/*
> >> + * Array of keys we support in Linux.
> > 
> > Not really, you can support "any" number of keys as the kernel shouldn't
> > care, or did I get it wrong?
> We currently support only the keys that have been specified in the KVP specification. I have a more detailed response on the core KVP protocol in response to your other email on this topic.
> > 
> >> + *
> >> + */
> >> +#define KVP_MAX_KEY	10
> >> +#define KVP_LIC_VERSION 1
> > 
> > Um, this is a nice magic number, care to explain it a bit more?
> As I noted in an earlier email, the KVP specification currently requires that we support 10 keys and it also specifies the ordering of these keys.  The information for the key "IntegrationServicesVersion", is only available in the kernel (one of the other LIC drivers defines  this information). 
> 
> >> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
> >> +				"IntegrationServicesVersion",
> > 
> > Looks like it matches up with this, right?  You might want to make that
> > a bit more "tied" together.
> > 
> Yes; I will fix this.
> >> +			case (KVP_LIC_VERSION):
> >> +				kvp_transaction_active = true;
> >> +				kvp_respond_to_host(kvp_data->index,
> >> +						HV_DRV_VERSION);
> > 
> > Why are you doing this in the kernel?  Why not do it from userspace like
> > all other messages?
> This information is only available in the kernel (defined by another LIC driver).

I thought it was exported through a modinfo parameter?  If not, you
could set the MODULE_VERSION field be the as this which then is exported
to userspace so you would not have to do any thing like this in the
kernel.

thanks,

greg k-h

Re: [PATCH]: An implementation of HyperV KVP functionality

[Ky Srinivasan]

>>> On 11/12/2010 at  1:47 PM, in message <20101112184753.GA20893@kroah.com>, Greg
KH <greg@kroah.com> wrote: 
> On Fri, Nov 12, 2010 at 11:06:18AM -0700, Ky Srinivasan wrote:
>> >> +typedef struct kvp_msg {
>> >> +	__u32 kvp_key; /* Key */
>> >> +	__u8  kvp_value[0]; /* Corresponding value */
>> >> +} kvp_msg_t;
>> > 
>> > I thought that kvp_value was really KVP_VALUE_SIZE?
>> 
>> kvp_value is typed information and KVP_VALUE_SIZE specifies the
>> maximum size of the supported value. For instance if kvp_value is a
>> string (which is the case for all the values currently supported),
>> KVP_VALUE_SIZE specifies the maximum size of the string that will be
>> supported.
> 
> So it's a variable length structure?  How do you konw how long the
> structure is, does that depend on the key?

kvp_value is a null terminated string. In the current implementation; the kernel component sends an index (key) to the user mode and receives the corresponding value - a string.
> 
>> > And no typedefs, you did run your code through checkpatch.pl, right?
>> > Why ignore the stuff it spits back at you?
>> I will fix this.
>> > 
>> > 
>> >> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
>> >> +				"IntegrationServicesVersion",
>> >> +				"NetworkAddressIPv4",
>> >> +				"NetworkAddressIPv6",
>> >> +				"OSBuildNumber",
>> >> +				"OSName",
>> >> +				"OSMajorVersion",
>> >> +				"OSMinorVersion",
>> >> +				"OSVersion",
>> >> +				"ProcessorArchitecture",
>> >> +				};
>> > 
>> > Why list these at all, as more might come in the future, and the kernel
>> > really doesn't care about them, right?
>> The core HyperV KVP protocol is such that the host iterates through an
>> index (starting with 0); the guest is free to associate a key with the
>> index and return the associated value. The host side iteration is
>> stopped when the guest returns a failure on an index. MSFT has
>> specified the keys and their ordinal value in their KVP specification.
>> The array you see above is part of that specification.
> 
> So you match on the string name of the key?  I'm confused, as I didn't
> think I saw you matching on the string name, only the key value.
> 
> Also, as the kernel isn't handling the key type (with one exception),
> why even list these in the kernel at all?  I'm all for documenting
> stuff, but don't use code memory for documentation :)

When we respond back to the host, we need to specify  the key for which the value is being returned. Note that the host only iterates over an integer key space while the guest is expected to return both the key name (guest is free to associate the key name with the integer index)  and the corresponding value. I use, the array of strings kvp_keys[] to implement the mapping between the integer index and the key name. Look at the function kvp_respond_to_host() where we lookup the kvp_keys[] array prior to responding back to the host.

In the next iteration of this patch, I am thinking of moving index to key mapping code into the user-level daemon, so the kernel side will not have to be modified as the key-space expands (in the future).

> 
>> >> --- linux.trees.git.orig/include/linux/connector.h	2010-11-09 17:22:15.000000000 
>> > -0500
>> >> +++ linux.trees.git/include/linux/connector.h	2010-11-11 13:14:52.000000000 
>> > -0500
>> >> @@ -42,8 +42,9 @@
>> >>  #define CN_VAL_DM_USERSPACE_LOG		0x1
>> >>  #define CN_IDX_DRBD			0x8
>> >>  #define CN_VAL_DRBD			0x1
>> >> +#define CN_KVP_IDX			0x9     /* MSFT KVP functionality */
>> > 
>> > Did you reserve this number with anyone?  Who?
>> 
>> I sent an email to	Evgeniy Polyakov  (Johnpol@2ka.mipt). The mail
>> bounced back. I was hoping you would help me register this index.
>> Would it make sense to have a separate patch to deal with registering
>> this index?
> 
> Yes, as I can not modify non-staging code without an ack from that
> maintainer.  And I'm pretty sure that Evgeniy's address has just
> changed, use "Evgeniy Polyakov <zbr@ioremap.net>" instead.
Thanks I will do just that.

K. Y
> 
> thanks,
> 
> greg k-h

Re: [PATCH]: An implementation of HyperV KVP functionality

[Greg KH]

On Fri, Nov 12, 2010 at 01:59:42PM -0700, Ky Srinivasan wrote:
> 
> 
> >>> On 11/12/2010 at  1:47 PM, in message <20101112184753.GA20893@kroah.com>, Greg
> KH <greg@kroah.com> wrote: 
> > On Fri, Nov 12, 2010 at 11:06:18AM -0700, Ky Srinivasan wrote:
> >> >> +typedef struct kvp_msg {
> >> >> +	__u32 kvp_key; /* Key */
> >> >> +	__u8  kvp_value[0]; /* Corresponding value */
> >> >> +} kvp_msg_t;
> >> > 
> >> > I thought that kvp_value was really KVP_VALUE_SIZE?
> >> 
> >> kvp_value is typed information and KVP_VALUE_SIZE specifies the
> >> maximum size of the supported value. For instance if kvp_value is a
> >> string (which is the case for all the values currently supported),
> >> KVP_VALUE_SIZE specifies the maximum size of the string that will be
> >> supported.
> > 
> > So it's a variable length structure?  How do you konw how long the
> > structure is, does that depend on the key?
> 
> kvp_value is a null terminated string. In the current implementation;
> the kernel component sends an index (key) to the user mode and
> receives the corresponding value - a string.

Why does the kernel care about the string at all?

> >> > And no typedefs, you did run your code through checkpatch.pl, right?
> >> > Why ignore the stuff it spits back at you?
> >> I will fix this.
> >> > 
> >> > 
> >> >> +static char *kvp_keys[KVP_MAX_KEY] = {"FullyQualifiedDomainName",
> >> >> +				"IntegrationServicesVersion",
> >> >> +				"NetworkAddressIPv4",
> >> >> +				"NetworkAddressIPv6",
> >> >> +				"OSBuildNumber",
> >> >> +				"OSName",
> >> >> +				"OSMajorVersion",
> >> >> +				"OSMinorVersion",
> >> >> +				"OSVersion",
> >> >> +				"ProcessorArchitecture",
> >> >> +				};
> >> > 
> >> > Why list these at all, as more might come in the future, and the kernel
> >> > really doesn't care about them, right?
> >> The core HyperV KVP protocol is such that the host iterates through an
> >> index (starting with 0); the guest is free to associate a key with the
> >> index and return the associated value. The host side iteration is
> >> stopped when the guest returns a failure on an index. MSFT has
> >> specified the keys and their ordinal value in their KVP specification.
> >> The array you see above is part of that specification.
> > 
> > So you match on the string name of the key?  I'm confused, as I didn't
> > think I saw you matching on the string name, only the key value.
> > 
> > Also, as the kernel isn't handling the key type (with one exception),
> > why even list these in the kernel at all?  I'm all for documenting
> > stuff, but don't use code memory for documentation :)
> 
> When we respond back to the host, we need to specify  the key for
> which the value is being returned. Note that the host only iterates
> over an integer key space while the guest is expected to return both
> the key name (guest is free to associate the key name with the integer
> index)  and the corresponding value.

Ah, to verify that the guest really did know what the key value that was
being used was for?  Odd way of verification, but I guess it works.

> I use, the array of strings kvp_keys[] to implement the mapping
> between the integer index and the key name. Look at the function
> kvp_respond_to_host() where we lookup the kvp_keys[] array prior to
> responding back to the host.
> 
> In the next iteration of this patch, I am thinking of moving index to
> key mapping code into the user-level daemon, so the kernel side will
> not have to be modified as the key-space expands (in the future).

Yes, I would recommend that so the kernel would not have to be modified
for every time the hyperv kernel is also changed :)

thanks,

greg k-h

Re: [PATCH]: An implementation of HyperV KVP functionality

[Dor Laor]

On 11/11/2010 10:03 PM, Ky Srinivasan wrote:

> + * An implementation of key value pair (KVP) functionality for Linux.
> + *
> + *
> + * Copyright (C) 2010, Novell, Inc.
> + * Author : K. Y. Srinivasan<ksrinivasan@novell.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + * NON INFRINGEMENT.  See the GNU General Public License for more
> + * details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + */
> +
> +
> +#include<linux/net.h>
> +#include<linux/nls.h>
> +#include<linux/connector.h>
> +
> +#include "logging.h"
> +#include "osd.h"
> +#include "vmbus.h"
> +#include "vmbus_packet_format.h"
> +#include "vmbus_channel_interface.h"
> +#include "version_info.h"
> +#include "channel.h"
> +#include "vmbus_private.h"
> +#include "vmbus_api.h"
> +#include "utils.h"
> +#include "kvp.h"
> +
> +
> +/*
> + *
> + * The following definitions are shared with the user-mode component; do not
> + * change any of this without making the corresponding changes in
> + * the KVP user-mode component.
> + */
> +
> +#define CN_KVP_VAL             0x1 /* This supports queries from the kernel */
> +#define CN_KVP_USER_VAL       0x2 /* This supports queries from the user */
> +
> +
> +/*
> + * KVP protocol: The user mode component first registers with the

Is there a spec that describes all of the possible messages?
For Linux virtio we use it in parallel to the code to make sure all 
potential guest OS will follow the same standard.

What about live migration semantics like migrate while transaction is 
on? and having the guest aware of the migration so the host data will be 
updated?

..


> main(void)
> {
> 	int fd, len, sock_opt;
> 	int error;
> 	struct cn_msg *message;
> 	struct pollfd pfd;
> 	struct nlmsghdr *incoming_msg;
> 	struct cn_msg	*incoming_cn_msg;
> 	char	*key_value;
> 	kvp_key_name_t key_name;
>
> 	daemon(1, 0);
> 	openlog("KVP", 0, LOG_USER);
> 	syslog(LOG_INFO, "KVP starting; pid is:%d", getpid());
> 	/*
> 	 * Retrieve OS release information.
> 	 */
> 	kvp_get_os_info();
>
> 	fd = socket(AF_NETLINK, SOCK_DGRAM, NETLINK_CONNECTOR);

This is nice. One of the first tried for virtio-serial was to use 
netlink. We even wanted unix_domain socket for AF_VIRT that will reach 
the host but davem nacked it.