[PATCH 0000/0004] drivers: hv

[PATCH 0000/0004] drivers: hv

[K. Y. Srinivasan]

This patch-set further enhances the KVP functionality for Linux
guests:

	1. Supports most of the Win8 KVP protocol.
	2. Supports operations on all the pools.

Regards,

K. Y

[PATCH 1/4] Drivers: hv: Add new message types to enhance KVP

[K. Y. Srinivasan]

Add additional KVP (Key Value Pair) protocol  messages to
enhance KVP functionality for Linux guests on Hyper-V. As part of this,
patch define an explicit version negoitiation message.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/hv/hv_kvp.c      |    5 +++--
 include/linux/hyperv.h   |   30 +++++++++++++++++++++++++++---
 tools/hv/hv_kvp_daemon.c |    2 +-
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 0ef4c1f..779109b 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -78,7 +78,7 @@ kvp_register(void)
 
 	if (msg) {
 		kvp_msg = (struct hv_kvp_msg *)msg->data;
-		version = kvp_msg->body.kvp_version;
+		version = kvp_msg->body.kvp_register.version;
 		msg->id.idx =  CN_KVP_IDX;
 		msg->id.val = CN_KVP_VAL;
 
@@ -122,7 +122,8 @@ kvp_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 		 * to the host. But first, cancel the timeout.
 		 */
 		if (cancel_delayed_work_sync(&kvp_work))
-			kvp_respond_to_host(data->data.key, data->data.value,
+			kvp_respond_to_host(data->data.key,
+					 data->data.value,
 					!strlen(data->data.key));
 	}
 }
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index e57a6c6..a2d8c54 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -149,7 +149,11 @@ struct hv_kvp_exchg_msg_value {
 	__u32 key_size;
 	__u32 value_size;
 	__u8 key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
-	__u8 value[HV_KVP_EXCHANGE_MAX_VALUE_SIZE];
+	union {
+		__u8 value[HV_KVP_EXCHANGE_MAX_VALUE_SIZE];
+		__u32 value_u32;
+		__u64 value_u64;
+	};
 } __attribute__((packed));
 
 struct hv_kvp_msg_enumerate {
@@ -157,11 +161,31 @@ struct hv_kvp_msg_enumerate {
 	struct hv_kvp_exchg_msg_value data;
 } __attribute__((packed));
 
+struct hv_kvp_msg_get {
+	struct hv_kvp_exchg_msg_value data;
+};
+
+struct hv_kvp_msg_set {
+	struct hv_kvp_exchg_msg_value data;
+};
+
+struct hv_kvp_msg_delete {
+	__u32 key_size;
+	__u8 key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
+};
+
+struct hv_kvp_register {
+	__u8 version[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
+};
+
 struct hv_kvp_msg {
 	struct hv_kvp_hdr	kvp_hdr;
 	union {
-		struct hv_kvp_msg_enumerate     kvp_enum_data;
-		char    kvp_version[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
+		struct hv_kvp_msg_get		kvp_get;
+		struct hv_kvp_msg_set		kvp_set;
+		struct hv_kvp_msg_delete	kvp_delete;
+		struct hv_kvp_msg_enumerate	kvp_enum_data;
+		struct hv_kvp_register		kvp_register;
 	} body;
 } __attribute__((packed));
 
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 4ebf703..00d3f7c 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -378,7 +378,7 @@ int main(void)
 			 * Driver is registering with us; stash away the version
 			 * information.
 			 */
-			p = (char *)hv_msg->body.kvp_version;
+			p = (char *)hv_msg->body.kvp_register.version;
 			lic_version = malloc(strlen(p) + 1);
 			if (lic_version) {
 				strcpy(lic_version, p);
-- 
1.7.4.1

[PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver

[K. Y. Srinivasan]

Now support the newly defined KVP message types. It turns out that the host
pushes a set of stand key value pairs as soon as the guest opens the KVP channel.
Since we cannot handle these tuples until the user level daemon loads up, defer
reading the KVP channel until the user level daemon is launched.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/hv/hv_kvp.c      |  184 ++++++++++++++++++++++++++++++++++++----------
 include/linux/hyperv.h   |    2 +
 tools/hv/hv_kvp_daemon.c |    7 ++
 3 files changed, 153 insertions(+), 40 deletions(-)

diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 779109b..3b2eeaa 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -42,9 +42,10 @@
 static struct {
 	bool active; /* transaction status - active or not */
 	int recv_len; /* number of bytes received. */
-	int index; /* current index */
+	struct hv_kvp_msg  *kvp_msg; /* current message */
 	struct vmbus_channel *recv_channel; /* chn we got the request */
 	u64 recv_req_id; /* request ID. */
+	void *kvp_context; /* for the channel callback */
 } kvp_transaction;
 
 static void kvp_send_key(struct work_struct *dummy);
@@ -110,12 +111,15 @@ kvp_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 	struct hv_kvp_msg_enumerate *data;
 
 	message = (struct hv_kvp_msg *)msg->data;
-	if (message->kvp_hdr.operation == KVP_OP_REGISTER) {
+	switch (message->kvp_hdr.operation) {
+	case KVP_OP_REGISTER:
 		pr_info("KVP: user-mode registering done.\n");
 		kvp_register();
-	}
+		kvp_transaction.active = false;
+		hv_kvp_onchannelcallback(kvp_transaction.kvp_context);
+		break;
 
-	if (message->kvp_hdr.operation == KVP_OP_ENUMERATE) {
+	default:
 		data = &message->body.kvp_enum_data;
 		/*
 		 * Complete the transaction by forwarding the key value
@@ -133,7 +137,11 @@ kvp_send_key(struct work_struct *dummy)
 {
 	struct cn_msg *msg;
 	struct hv_kvp_msg *message;
-	int index = kvp_transaction.index;
+	struct hv_kvp_msg *in_msg;
+	__u8 operation = kvp_transaction.kvp_msg->kvp_hdr.operation;
+	__u8 pool = kvp_transaction.kvp_msg->kvp_hdr.pool;
+	__u32 val32;
+	__u64 val64;
 
 	msg = kzalloc(sizeof(*msg) + sizeof(struct hv_kvp_msg) , GFP_ATOMIC);
 
@@ -142,8 +150,85 @@ kvp_send_key(struct work_struct *dummy)
 		msg->id.val = CN_KVP_VAL;
 
 		message = (struct hv_kvp_msg *)msg->data;
-		message->kvp_hdr.operation = KVP_OP_ENUMERATE;
-		message->body.kvp_enum_data.index = index;
+		message->kvp_hdr.operation = operation;
+		message->kvp_hdr.pool = pool;
+		in_msg = kvp_transaction.kvp_msg;
+
+		/*
+		 * The key/value strings sent from the host are encoded in
+		 * in utf16; convert it to utf8 strings.
+		 */
+
+		switch (message->kvp_hdr.operation) {
+		case KVP_OP_SET:
+			switch (in_msg->body.kvp_set.data.value_type) {
+			case REG_SZ:
+				/*
+				 * The value is a string - utf16 encoding.
+				 */
+				message->body.kvp_set.data.value_size =
+				utf16s_to_utf8s(
+				(wchar_t *)
+				in_msg->body.kvp_set.data.value,
+				in_msg->body.kvp_set.data.value_size,
+				UTF16_LITTLE_ENDIAN,
+				message->body.kvp_set.data.value,
+				HV_KVP_EXCHANGE_MAX_VALUE_SIZE) + 1;
+				break;
+
+			case REG_U32:
+				/*
+				 * The value is a 32 bit scalar.
+				 * We save this as a utf8 string.
+				 */
+				val32 =
+				in_msg->body.kvp_set.data.value_u32;
+				message->body.kvp_set.data.value_size =
+				sprintf(message->body.kvp_set.data.value,
+					"%d", val32) + 1;
+				break;
+
+			case REG_U64:
+				/*
+				 * The value is a 64 bit scalar.
+				 * We save this as a utf8 string.
+				 */
+				val64 =
+				in_msg->body.kvp_set.data.value_u64;
+				message->body.kvp_set.data.value_size =
+				sprintf(message->body.kvp_set.data.value,
+					"%llu", val64) + 1;
+				break;
+
+			}
+		case KVP_OP_GET:
+			message->body.kvp_set.data.key_size =
+				utf16s_to_utf8s(
+				(wchar_t *)in_msg->body.kvp_set.data.key,
+				in_msg->body.kvp_set.data.key_size,
+				UTF16_LITTLE_ENDIAN,
+				message->body.kvp_set.data.key,
+				HV_KVP_EXCHANGE_MAX_KEY_SIZE) + 1;
+
+			break;
+
+		case KVP_OP_DELETE:
+			message->body.kvp_delete.key_size =
+				utf16s_to_utf8s(
+				(wchar_t *)in_msg->body.kvp_delete.key,
+				in_msg->body.kvp_delete.key_size,
+				UTF16_LITTLE_ENDIAN,
+				message->body.kvp_delete.key,
+				HV_KVP_EXCHANGE_MAX_KEY_SIZE) + 1;
+
+			break;
+
+		case KVP_OP_ENUMERATE:
+			message->body.kvp_enum_data.index =
+				in_msg->body.kvp_enum_data.index;
+			break;
+		}
+
 		msg->len = sizeof(struct hv_kvp_msg);
 		cn_netlink_send(msg, 0, GFP_ATOMIC);
 		kfree(msg);
@@ -159,7 +244,7 @@ static void
 kvp_respond_to_host(char *key, char *value, int error)
 {
 	struct hv_kvp_msg  *kvp_msg;
-	struct hv_kvp_msg_enumerate  *kvp_data;
+	struct hv_kvp_exchg_msg_value  *kvp_data;
 	char	*key_name;
 	struct icmsg_hdr *icmsghdrp;
 	int	keylen, valuelen;
@@ -189,6 +274,9 @@ kvp_respond_to_host(char *key, char *value, int error)
 
 	kvp_transaction.active = false;
 
+	icmsghdrp = (struct icmsg_hdr *)
+			&recv_buffer[sizeof(struct vmbuspipe_hdr)];
+
 	if (channel->onchannel_callback == NULL)
 		/*
 		 * We have raced with util driver being unloaded;
@@ -196,41 +284,57 @@ kvp_respond_to_host(char *key, char *value, int error)
 		 */
 		return;
 
-	icmsghdrp = (struct icmsg_hdr *)
-			&recv_buffer[sizeof(struct vmbuspipe_hdr)];
-	kvp_msg = (struct hv_kvp_msg *)
-			&recv_buffer[sizeof(struct vmbuspipe_hdr) +
-			sizeof(struct icmsg_hdr)];
-	kvp_data = &kvp_msg->body.kvp_enum_data;
-	key_name = key;
 
 	/*
 	 * If the error parameter is set, terminate the host's enumeration.
 	 */
 	if (error) {
 		/*
-		 * We don't support this index or the we have timedout;
+		 * Something failed or the we have timedout;
 		 * terminate the host-side iteration by returning an error.
 		 */
 		icmsghdrp->status = HV_E_FAIL;
 		goto response_done;
 	}
 
+	icmsghdrp->status = HV_S_OK;
+
+	kvp_msg = (struct hv_kvp_msg *)
+			&recv_buffer[sizeof(struct vmbuspipe_hdr) +
+			sizeof(struct icmsg_hdr)];
+
+	switch (kvp_transaction.kvp_msg->kvp_hdr.operation) {
+	case KVP_OP_GET:
+		kvp_data = &kvp_msg->body.kvp_get.data;
+		goto copy_value;
+
+	case KVP_OP_SET:
+	case KVP_OP_DELETE:
+		goto response_done;
+
+	default:
+		break;
+	}
+
+	kvp_data = &kvp_msg->body.kvp_enum_data.data;
+	key_name = key;
+
 	/*
 	 * The windows host expects the key/value pair to be encoded
 	 * in utf16.
 	 */
 	keylen = utf8s_to_utf16s(key_name, strlen(key_name), UTF16_HOST_ENDIAN,
-				(wchar_t *) kvp_data->data.key,
+				(wchar_t *) kvp_data->key,
 				HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
-	kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
+	kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
+
+copy_value:
 	valuelen = utf8s_to_utf16s(value, strlen(value), UTF16_HOST_ENDIAN,
-				(wchar_t *) kvp_data->data.value,
+				(wchar_t *) kvp_data->value,
 				HV_KVP_EXCHANGE_MAX_VALUE_SIZE / 2);
-	kvp_data->data.value_size = 2*(valuelen + 1); /* utf16 encoding */
+	kvp_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;
+	kvp_data->value_type = REG_SZ; /* all our values are strings */
 
 response_done:
 	icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION | ICMSGHDRFLAG_RESPONSE;
@@ -257,11 +361,18 @@ void hv_kvp_onchannelcallback(void *context)
 	u64 requestid;
 
 	struct hv_kvp_msg *kvp_msg;
-	struct hv_kvp_msg_enumerate *kvp_data;
 
 	struct icmsg_hdr *icmsghdrp;
 	struct icmsg_negotiate *negop = NULL;
 
+	if (kvp_transaction.active) {
+		/*
+		 * We will defer processing this callback once
+		 * the current transaction is complete.
+		 */
+		kvp_transaction.kvp_context = context;
+		return;
+	}
 
 	vmbus_recvpacket(channel, recv_buffer, PAGE_SIZE, &recvlen, &requestid);
 
@@ -276,29 +387,16 @@ void hv_kvp_onchannelcallback(void *context)
 				sizeof(struct vmbuspipe_hdr) +
 				sizeof(struct icmsg_hdr)];
 
-			kvp_data = &kvp_msg->body.kvp_enum_data;
-
-			/*
-			 * We only support the "get" operation on
-			 * "KVP_POOL_AUTO" pool.
-			 */
-
-			if ((kvp_msg->kvp_hdr.pool != KVP_POOL_AUTO) ||
-				(kvp_msg->kvp_hdr.operation !=
-				KVP_OP_ENUMERATE)) {
-				icmsghdrp->status = HV_E_FAIL;
-				goto callback_done;
-			}
-
 			/*
 			 * Stash away this global state for completing the
 			 * transaction; note transactions are serialized.
 			 */
+
 			kvp_transaction.recv_len = recvlen;
 			kvp_transaction.recv_channel = channel;
 			kvp_transaction.recv_req_id = requestid;
 			kvp_transaction.active = true;
-			kvp_transaction.index = kvp_data->index;
+			kvp_transaction.kvp_msg = kvp_msg;
 
 			/*
 			 * Get the information from the
@@ -316,8 +414,6 @@ void hv_kvp_onchannelcallback(void *context)
 
 		}
 
-callback_done:
-
 		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
 			| ICMSGHDRFLAG_RESPONSE;
 
@@ -338,6 +434,14 @@ hv_kvp_init(struct hv_util_service *srv)
 		return err;
 	recv_buffer = srv->recv_buffer;
 
+	/*
+	 * When this driver loads, the user level daemon that
+	 * processes the host requests may not yet be running.
+	 * Defer processing channel callbacks until the daemon
+	 * has registered.
+	 */
+	kvp_transaction.active = true;
+
 	return 0;
 }
 
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index a2d8c54..e88a979 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -119,6 +119,8 @@
  */
 
 #define REG_SZ 1
+#define REG_U32 4
+#define REG_U64 8
 
 enum hv_kvp_exchg_op {
 	KVP_OP_GET = 0,
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 00d3f7c..a98878c 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -389,10 +389,16 @@ int main(void)
 			}
 			continue;
 
+		case KVP_OP_SET:
+		case KVP_OP_GET:
+		case KVP_OP_DELETE:
 		default:
 			break;
 		}
 
+		if (hv_msg->kvp_hdr.operation != KVP_OP_ENUMERATE)
+			goto kvp_done;
+
 		hv_msg = (struct hv_kvp_msg *)incoming_cn_msg->data;
 		key_name = (char *)hv_msg->body.kvp_enum_data.data.key;
 		key_value = (char *)hv_msg->body.kvp_enum_data.data.value;
@@ -454,6 +460,7 @@ int main(void)
 		 * already in the receive buffer. Update the cn_msg header to
 		 * reflect the key value that has been added to the message
 		 */
+kvp_done:
 
 		incoming_cn_msg->id.idx = CN_KVP_IDX;
 		incoming_cn_msg->id.val = CN_KVP_VAL;
-- 
1.7.4.1

Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver

[Dan Carpenter]

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

On Sat, Mar 10, 2012 at 03:32:09PM -0800, K. Y. Srinivasan wrote:
> +		switch (message->kvp_hdr.operation) {
> +		case KVP_OP_SET:
> +			switch (in_msg->body.kvp_set.data.value_type) {
> +			case REG_SZ:
> +				/*
> +				 * The value is a string - utf16 encoding.
> +				 */
> +				message->body.kvp_set.data.value_size =
> +				utf16s_to_utf8s(
> +				(wchar_t *)
> +				in_msg->body.kvp_set.data.value,
> +				in_msg->body.kvp_set.data.value_size,
> +				UTF16_LITTLE_ENDIAN,
> +				message->body.kvp_set.data.value,
> +				HV_KVP_EXCHANGE_MAX_VALUE_SIZE) + 1;
> +				break;
> +

This block of unreadable text is so nasty.

You could return directly if the msg = kmalloc() fails and pull
everything in one indent level.  It's normally more readable to
handle errors as soon as possible anyway.

Probably that's not enough to make a difference and we'd need to
introduce a new function.

Btw I don't know if utf16s_to_utf8s() counts the NUL char or not.
It feels like maybe we could end up with ->value_size equal to
HV_KVP_EXCHANGE_MAX_VALUE_SIZE + 1.

regards,
dan carpenter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver

[Alan Stern]

On Sun, 11 Mar 2012, Dan Carpenter wrote:

> Btw I don't know if utf16s_to_utf8s() counts the NUL char or not.
> It feels like maybe we could end up with ->value_size equal to
> HV_KVP_EXCHANGE_MAX_VALUE_SIZE + 1.

It does not count NUL characters.  If it encounters a NUL character in
the input, it stops right away without copying that character to the
output.  If it reaches the end of the input, it does not add a
terminating NUL character to the output.

Alan Stern

RE: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Sunday, March 11, 2012 6:43 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> Alan Stern
> Subject: Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP
> messages in the driver
> 
> On Sat, Mar 10, 2012 at 03:32:09PM -0800, K. Y. Srinivasan wrote:
> > +		switch (message->kvp_hdr.operation) {
> > +		case KVP_OP_SET:
> > +			switch (in_msg->body.kvp_set.data.value_type) {
> > +			case REG_SZ:
> > +				/*
> > +				 * The value is a string - utf16 encoding.
> > +				 */
> > +				message->body.kvp_set.data.value_size =
> > +				utf16s_to_utf8s(
> > +				(wchar_t *)
> > +				in_msg->body.kvp_set.data.value,
> > +				in_msg->body.kvp_set.data.value_size,
> > +				UTF16_LITTLE_ENDIAN,
> > +				message->body.kvp_set.data.value,
> > +				HV_KVP_EXCHANGE_MAX_VALUE_SIZE) + 1;
> > +				break;
> > +
> 
> This block of unreadable text is so nasty.
> 
> You could return directly if the msg = kmalloc() fails and pull
> everything in one indent level.  It's normally more readable to
> handle errors as soon as possible anyway.

True.

> 
> Probably that's not enough to make a difference and we'd need to
> introduce a new function.
> 
> Btw I don't know if utf16s_to_utf8s() counts the NUL char or not.
> It feels like maybe we could end up with ->value_size equal to
> HV_KVP_EXCHANGE_MAX_VALUE_SIZE + 1.

The MAX value is set to accommodate the maximum string that will ever
be handled including the string terminator. The function utf16s_to_utf8s()
returns the converted string length but the returned length does not 
include the string terminator (like strlen), hence the "+1".

Dan, I will see if there are other comments on these patches and will
accommodate your suggestion then. If there are no other comments,
would you mind if I addressed your comments here in a separate patch.  


Regards,

K. Y

> 
> regards,
> dan carpenter

Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver

[Dan Carpenter]

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

On Sun, Mar 11, 2012 at 04:56:06PM +0000, KY Srinivasan wrote:
> > Probably that's not enough to make a difference and we'd need to
> > introduce a new function.
> > 
> > Btw I don't know if utf16s_to_utf8s() counts the NUL char or not.
> > It feels like maybe we could end up with ->value_size equal to
> > HV_KVP_EXCHANGE_MAX_VALUE_SIZE + 1.
> 
> The MAX value is set to accommodate the maximum string that will ever
> be handled including the string terminator. The function utf16s_to_utf8s()
> returns the converted string length but the returned length does not 
> include the string terminator (like strlen), hence the "+1".
>

sprintf() and friends copy the NUL terminator but utf16s_to_utf8s()
doesn't so the code isn't right and it does seem like maybe we could
end up with a ->value_size equal to HV_KVP_EXCHANGE_MAX_VALUE_SIZE +
1.

regards,
dan carpenter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Sunday, March 11, 2012 2:49 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> Alan Stern
> Subject: Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP
> messages in the driver
> 
> On Sun, Mar 11, 2012 at 04:56:06PM +0000, KY Srinivasan wrote:
> > > Probably that's not enough to make a difference and we'd need to
> > > introduce a new function.
> > >
> > > Btw I don't know if utf16s_to_utf8s() counts the NUL char or not.
> > > It feels like maybe we could end up with ->value_size equal to
> > > HV_KVP_EXCHANGE_MAX_VALUE_SIZE + 1.
> >
> > The MAX value is set to accommodate the maximum string that will ever
> > be handled including the string terminator. The function utf16s_to_utf8s()
> > returns the converted string length but the returned length does not
> > include the string terminator (like strlen), hence the "+1".
> >
> 
> sprintf() and friends copy the NUL terminator but utf16s_to_utf8s()
> doesn't so the code isn't right and it does seem like maybe we could
> end up with a ->value_size equal to HV_KVP_EXCHANGE_MAX_VALUE_SIZE +
> 1.

You are right in that utf16s_to_utf8s() does not copy the string terminator. This
is not an issue in this case since the buffer for the utf8 string is zeroed out to begin
with (this memory was allocated using kzalloc()). The return value of the utf16s_to_utf8s()
is the length of the utf8s string as what would be returned by strlen. I add one to take into account
the string terminator character for further processing. As I said before the MAX value takes into
account the terminating character for all the strings handled.

Regards,

K. Y

Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver

[Dan Carpenter]

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

On Sun, Mar 11, 2012 at 08:53:57PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Sunday, March 11, 2012 2:49 PM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> > Alan Stern
> > Subject: Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP
> > messages in the driver
> > 
> > On Sun, Mar 11, 2012 at 04:56:06PM +0000, KY Srinivasan wrote:
> > > > Probably that's not enough to make a difference and we'd need to
> > > > introduce a new function.
> > > >
> > > > Btw I don't know if utf16s_to_utf8s() counts the NUL char or not.
> > > > It feels like maybe we could end up with ->value_size equal to
> > > > HV_KVP_EXCHANGE_MAX_VALUE_SIZE + 1.
> > >
> > > The MAX value is set to accommodate the maximum string that will ever
> > > be handled including the string terminator. The function utf16s_to_utf8s()
> > > returns the converted string length but the returned length does not
> > > include the string terminator (like strlen), hence the "+1".
> > >
> > 
> > sprintf() and friends copy the NUL terminator but utf16s_to_utf8s()
> > doesn't so the code isn't right and it does seem like maybe we could
> > end up with a ->value_size equal to HV_KVP_EXCHANGE_MAX_VALUE_SIZE +
> > 1.
> 
> You are right in that utf16s_to_utf8s() does not copy the string
> terminator. This is not an issue in this case since the buffer for
> the utf8 string is zeroed out to begin with (this memory was
> allocated using kzalloc()). The return value of the
> utf16s_to_utf8s() is the length of the utf8s string as what would
> be returned by strlen.


There is no strlen() involved...  It returns the number of bytes
copied to the output string.  It doesn't copy a NUL.  We pass
HV_KVP_EXCHANGE_MAX_VALUE_SIZE bytes as the limit.  So it fills up
the buffer with non-null characters and we have an off-by-one.

> I add one to take into account the string
> terminator character for further processing. As I said before the
> MAX value takes into account the terminating character for all the
> strings handled.

So you're saying that since we control the input string, we'll never
hit the max?  Still, why not pass HV_KVP_EXCHANGE_MAX_VALUE_SIZE - 1
to leave room for the NUL just for correctness?  We'd still add one
to the return value but we wouldn't go over the size of the buffer.

Again, I don't really know how utf16s_to_utf8s() works so I might
have misunderstood.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, March 12, 2012 1:22 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; ohering@suse.com; linux-
> kernel@vger.kernel.org; virtualization@lists.osdl.org; Alan Stern;
> devel@linuxdriverproject.org
> Subject: Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP
> messages in the driver
> 
> On Sun, Mar 11, 2012 at 08:53:57PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > Sent: Sunday, March 11, 2012 2:49 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org;
> ohering@suse.com;
> > > Alan Stern
> > > Subject: Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP
> > > messages in the driver
> > >
> > > On Sun, Mar 11, 2012 at 04:56:06PM +0000, KY Srinivasan wrote:
> > > > > Probably that's not enough to make a difference and we'd need to
> > > > > introduce a new function.
> > > > >
> > > > > Btw I don't know if utf16s_to_utf8s() counts the NUL char or not.
> > > > > It feels like maybe we could end up with ->value_size equal to
> > > > > HV_KVP_EXCHANGE_MAX_VALUE_SIZE + 1.
> > > >
> > > > The MAX value is set to accommodate the maximum string that will ever
> > > > be handled including the string terminator. The function utf16s_to_utf8s()
> > > > returns the converted string length but the returned length does not
> > > > include the string terminator (like strlen), hence the "+1".
> > > >
> > >
> > > sprintf() and friends copy the NUL terminator but utf16s_to_utf8s()
> > > doesn't so the code isn't right and it does seem like maybe we could
> > > end up with a ->value_size equal to HV_KVP_EXCHANGE_MAX_VALUE_SIZE
> +
> > > 1.
> >
> > You are right in that utf16s_to_utf8s() does not copy the string
> > terminator. This is not an issue in this case since the buffer for
> > the utf8 string is zeroed out to begin with (this memory was
> > allocated using kzalloc()). The return value of the
> > utf16s_to_utf8s() is the length of the utf8s string as what would
> > be returned by strlen.
> 
> 
> There is no strlen() involved...  It returns the number of bytes
> copied to the output string.  It doesn't copy a NUL.  We pass
> HV_KVP_EXCHANGE_MAX_VALUE_SIZE bytes as the limit.  So it fills up
> the buffer with non-null characters and we have an off-by-one.
> 
Dan,
I am sorry for not being as precise as I should be:
utf16s_to_utf8s() takes two length parameters - the length of the utf16 string
that is to be converted and the second the length of the utf8 output string.
The windows host manipulates all string in utf16 encoding and the string we get
from the host is guaranteed to be less than or equal to MAX value that we have
including the terminating character. In my code, I simply pass the length of the 
utf16 string as received from the host.

The parameter that I am currently passing MAX length value is the "maxout" 
parameter of the utf16s_utf8s() function. This by definition is the size of the
output buffer and in this case it happens to be MAX characters big.

> > I add one to take into account the string
> > terminator character for further processing. As I said before the
> > MAX value takes into account the terminating character for all the
> > strings handled.
> 
> So you're saying that since we control the input string, we'll never
> hit the max?  Still, why not pass HV_KVP_EXCHANGE_MAX_VALUE_SIZE - 1
> to leave room for the NUL just for correctness?  We'd still add one
> to the return value but we wouldn't go over the size of the buffer.

As I described earlier, with the  host side guarantee that the string we get from
the host is always guaranteed to be less than or equal to MAX length
(including the terminating character), there is no question of going over the
size of the output buffer which is sized based on the host specification.

Regards,

K. Y

Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver

[Dan Carpenter]

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

On Mon, Mar 12, 2012 at 12:36:53PM +0000, KY Srinivasan wrote:
> Dan,
> I am sorry for not being as precise as I should be:
> utf16s_to_utf8s() takes two length parameters - the length of the utf16 string
> that is to be converted and the second the length of the utf8 output string.
> The windows host manipulates all string in utf16 encoding and the string we get
> from the host is guaranteed to be less than or equal to MAX value that we have
> including the terminating character. In my code, I simply pass the length of the 
> utf16 string as received from the host.
> 
> The parameter that I am currently passing MAX length value is the "maxout" 
> parameter of the utf16s_utf8s() function. This by definition is the size of the
> output buffer and in this case it happens to be MAX characters big.
> 

I also think I'm not being as clear as I should...  I understand
that you trust the input; I'm say that for correctness sake you
should specify a output size which leaves room for the NUL char.

I can't say I know this code very well so I could be wrong, but it's
what we do inside usb_string() for example.  Can someone who knows
the code check if we should do something like this:

diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 3b2eeaa..3a97f52 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -173,7 +173,7 @@ kvp_send_key(struct work_struct *dummy)
 				in_msg->body.kvp_set.data.value_size,
 				UTF16_LITTLE_ENDIAN,
 				message->body.kvp_set.data.value,
-				HV_KVP_EXCHANGE_MAX_VALUE_SIZE) + 1;
+				HV_KVP_EXCHANGE_MAX_VALUE_SIZE - 1) + 1;
 				break;
 
 			case REG_U32:
@@ -208,7 +208,7 @@ kvp_send_key(struct work_struct *dummy)
 				in_msg->body.kvp_set.data.key_size,
 				UTF16_LITTLE_ENDIAN,
 				message->body.kvp_set.data.key,
-				HV_KVP_EXCHANGE_MAX_KEY_SIZE) + 1;
+				HV_KVP_EXCHANGE_MAX_KEY_SIZE - 1) + 1;
 
 			break;
 
@@ -219,7 +219,7 @@ kvp_send_key(struct work_struct *dummy)
 				in_msg->body.kvp_delete.key_size,
 				UTF16_LITTLE_ENDIAN,
 				message->body.kvp_delete.key,
-				HV_KVP_EXCHANGE_MAX_KEY_SIZE) + 1;
+				HV_KVP_EXCHANGE_MAX_KEY_SIZE - 1) + 1;
 
 			break;
 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, March 12, 2012 9:04 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; ohering@suse.com; linux-
> kernel@vger.kernel.org; virtualization@lists.osdl.org; Alan Stern;
> devel@linuxdriverproject.org
> Subject: Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP
> messages in the driver
> 
> On Mon, Mar 12, 2012 at 12:36:53PM +0000, KY Srinivasan wrote:
> > Dan,
> > I am sorry for not being as precise as I should be:
> > utf16s_to_utf8s() takes two length parameters - the length of the utf16 string
> > that is to be converted and the second the length of the utf8 output string.
> > The windows host manipulates all string in utf16 encoding and the string we get
> > from the host is guaranteed to be less than or equal to MAX value that we have
> > including the terminating character. In my code, I simply pass the length of the
> > utf16 string as received from the host.
> >
> > The parameter that I am currently passing MAX length value is the "maxout"
> > parameter of the utf16s_utf8s() function. This by definition is the size of the
> > output buffer and in this case it happens to be MAX characters big.
> >
> 
> I also think I'm not being as clear as I should...  I understand
> that you trust the input; I'm say that for correctness sake you
> should specify a output size which leaves room for the NUL char.
> 
> I can't say I know this code very well so I could be wrong, but it's
> what we do inside usb_string() for example.  Can someone who knows
> the code check if we should do something like this:
> 

Dan, 

Sorry I could not get back to you earlier. You are right, in that I am trusting
the host! I am of the firm belief that in a virtualized environment, if you don't trust
the host, there is not a whole lot you can do in a guest! I have had this arguments 
with other on this mailing list in the past. Having said that, I think we have spent more
time debating this than we should; your proposal is reasonable and I will go ahead and
re-spin those patches based on your comments. I should be posting them shortly.

Regards,

K. Y

Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver

[Dan Carpenter]

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

On Thu, Mar 15, 2012 at 11:36:16PM +0000, KY Srinivasan wrote:
> 
> Dan, 
> 
> Sorry I could not get back to you earlier. You are right, in that I am trusting
> the host! I am of the firm belief that in a virtualized environment, if you don't trust
> the host, there is not a whole lot you can do in a guest! I have had this arguments 
> with other on this mailing list in the past. Having said that, I think we have spent more
> time debating this than we should; your proposal is reasonable and I will go ahead and
> re-spin those patches based on your comments. I should be posting them shortly.
> 
> Regards,

It's not about trusting the host or not trusting the host.  It's
about "if you're going to specify a limitter, it can't be off by one
even if you don't expect to hit the limit".

But thanks for redoing these.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, March 16, 2012 1:38 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; ohering@suse.com; linux-
> kernel@vger.kernel.org; virtualization@lists.osdl.org; Alan Stern;
> devel@linuxdriverproject.org
> Subject: Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP
> messages in the driver
> 
> On Thu, Mar 15, 2012 at 11:36:16PM +0000, KY Srinivasan wrote:
> >
> > Dan,
> >
> > Sorry I could not get back to you earlier. You are right, in that I am trusting
> > the host! I am of the firm belief that in a virtualized environment, if you don't
> trust
> > the host, there is not a whole lot you can do in a guest! I have had this
> arguments
> > with other on this mailing list in the past. Having said that, I think we have spent
> more
> > time debating this than we should; your proposal is reasonable and I will go
> ahead and
> > re-spin those patches based on your comments. I should be posting them
> shortly.
> >
> > Regards,
> 
> It's not about trusting the host or not trusting the host.  It's
> about "if you're going to specify a limitter, it can't be off by one
> even if you don't expect to hit the limit".
> 
> But thanks for redoing these.

Thank you, for taking the time to review.

Regards,

K. Y

[PATCH 3/4] Tools: hv: Fully support the new KVP verbs in the user level daemon

[K. Y. Srinivasan]

Now fully support the new KVP messages in the user level daemon. Hyper-V defines
multiple persistent pools to which the host can write/read/modify KVP tuples.
In this patch we implement a file for each specified pool, where the KVP tuples
will stored in the guest.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 tools/hv/hv_kvp_daemon.c |  281 +++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 280 insertions(+), 1 deletions(-)

diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index a98878c..2fb9c3d 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -39,7 +39,8 @@
 #include <ifaddrs.h>
 #include <netdb.h>
 #include <syslog.h>
-
+#include <sys/stat.h>
+#include <fcntl.h>
 
 /*
  * KVP protocol: The user mode component first registers with the
@@ -79,6 +80,250 @@ static char *os_build;
 static char *lic_version;
 static struct utsname uts_buf;
 
+
+#define MAX_FILE_NAME 100
+#define ENTRIES_PER_BLOCK 50
+
+struct kvp_record {
+	__u8 key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
+	__u8 value[HV_KVP_EXCHANGE_MAX_VALUE_SIZE];
+};
+
+struct kvp_file_state {
+	int fd;
+	int num_blocks;
+	struct kvp_record *records;
+	int num_records;
+	__u8 fname[MAX_FILE_NAME];
+};
+
+static struct kvp_file_state kvp_file_info[KVP_POOL_COUNT];
+
+static void kvp_acquire_lock(int pool)
+{
+	struct flock fl = {F_WRLCK, SEEK_SET, 0, 0, 0};
+	fl.l_pid = getpid();
+
+	if (fcntl(kvp_file_info[pool].fd, F_SETLKW, &fl) == -1) {
+		syslog(LOG_ERR, "Failed to acquire the lock pool: %d", pool);
+		exit(-1);
+	}
+}
+
+static void kvp_release_lock(int pool)
+{
+	struct flock fl = {F_UNLCK, SEEK_SET, 0, 0, 0};
+	fl.l_pid = getpid();
+
+	if (fcntl(kvp_file_info[pool].fd, F_SETLK, &fl) == -1) {
+		perror("fcntl");
+		syslog(LOG_ERR, "Failed to release the lock pool: %d", pool);
+		exit(-1);
+	}
+}
+
+static void kvp_update_file(int pool)
+{
+	FILE *filep;
+	size_t bytes_written;
+
+	/*
+	 * We are going to write our in-memory registry out to
+	 * disk; acquire the lock first.
+	 */
+	kvp_acquire_lock(pool);
+
+	filep = fopen(kvp_file_info[pool].fname, "w");
+	if (!filep) {
+		kvp_release_lock(pool);
+		syslog(LOG_ERR, "Failed to open file, pool: %d", pool);
+		exit(-1);
+	}
+
+	bytes_written = fwrite(kvp_file_info[pool].records,
+				sizeof(struct kvp_record),
+				kvp_file_info[pool].num_records, filep);
+
+	fflush(filep);
+	kvp_release_lock(pool);
+}
+
+static int kvp_file_init(void)
+{
+	int ret, fd;
+	FILE *filep;
+	size_t records_read;
+	__u8 *fname;
+	struct kvp_record *record;
+	struct kvp_record *readp;
+	int num_blocks;
+	int i;
+	int alloc_unit = sizeof(struct kvp_record) * ENTRIES_PER_BLOCK;
+
+	if (access("/var/opt/hyperv", F_OK)) {
+		if (mkdir("/var/opt/hyperv", S_IRUSR | S_IWUSR | S_IROTH)) {
+			syslog(LOG_ERR, " Failed to create /var/opt/hyperv");
+			exit(-1);
+		}
+	}
+
+	for (i = 0; i < KVP_POOL_COUNT; i++) {
+		fname = kvp_file_info[i].fname;
+		records_read = 0;
+		num_blocks = 1;
+		sprintf(fname, "/var/opt/hyperv/.kvp_pool_%d", i);
+		fd = open(fname, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR | S_IROTH);
+
+		if (fd == -1)
+			return 1;
+
+
+		filep = fopen(fname, "r");
+		if (!filep)
+			return 1;
+
+		record = malloc(alloc_unit * num_blocks);
+		if (record == NULL) {
+			fclose(filep);
+			return 1;
+		}
+		while (!feof(filep)) {
+			readp = &record[records_read];
+			records_read += fread(readp, sizeof(struct kvp_record),
+					ENTRIES_PER_BLOCK,
+					filep);
+
+			if (!feof(filep)) {
+				/*
+				 * We have more data to read.
+				 */
+				num_blocks++;
+				record = realloc(record, alloc_unit *
+						num_blocks);
+				if (record == NULL) {
+					fclose(filep);
+					return 1;
+				}
+				continue;
+			}
+			break;
+		}
+		kvp_file_info[i].fd = fd;
+		kvp_file_info[i].num_blocks = num_blocks;
+		kvp_file_info[i].records = record;
+		kvp_file_info[i].num_records = records_read;
+		fclose(filep);
+
+	}
+
+	return 0;
+}
+
+static int kvp_key_delete(int pool, __u8 *key, int key_size)
+{
+	int i;
+	int j, k;
+	int num_records = kvp_file_info[pool].num_records;
+	struct kvp_record *record = kvp_file_info[pool].records;
+
+	for (i = 0; i < num_records; i++) {
+		if (memcmp(key, record[i].key, key_size))
+			continue;
+		/*
+		 * Found a match; just move the remaining
+		 * entries up.
+		 */
+		if (i == num_records) {
+			kvp_file_info[pool].num_records--;
+			kvp_update_file(pool);
+			return 0;
+		}
+
+		j = i;
+		k = j + 1;
+		for (; k < num_records; k++) {
+			strcpy(record[j].key, record[k].key);
+			strcpy(record[j].value, record[k].value);
+			j++;
+		}
+
+		kvp_file_info[pool].num_records--;
+		kvp_update_file(pool);
+		return 0;
+	}
+	return 1;
+}
+
+static int kvp_key_add_or_modify(int pool, __u8 *key, int key_size, __u8 *value,
+			int value_size)
+{
+	int i;
+	int j, k;
+	int num_records = kvp_file_info[pool].num_records;
+	struct kvp_record *record = kvp_file_info[pool].records;
+	int num_blocks = kvp_file_info[pool].num_blocks;
+
+	if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
+		(value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
+		return 1;
+
+	for (i = 0; i < num_records; i++) {
+		if (memcmp(key, record[i].key, key_size))
+			continue;
+		/*
+		 * Found a match; just update the value -
+		 * this is the modify case.
+		 */
+		memcpy(record[i].value, value, value_size);
+		kvp_update_file(pool);
+		return 0;
+	}
+
+	/*
+	 * Need to add a new entry;
+	 */
+	if (num_records == (ENTRIES_PER_BLOCK * num_blocks)) {
+		/* Need to allocate a larger array for reg entries. */
+		record = realloc(record, sizeof(struct kvp_record) *
+			 ENTRIES_PER_BLOCK * (num_blocks + 1));
+
+		if (record == NULL)
+			return 1;
+		kvp_file_info[pool].num_blocks++;
+
+	}
+	memcpy(record[i].value, value, value_size);
+	memcpy(record[i].key, key, key_size);
+	kvp_file_info[pool].records = record;
+	kvp_file_info[pool].num_records++;
+	kvp_update_file(pool);
+	return 0;
+}
+
+static int kvp_get_value(int pool, __u8 *key, int key_size, __u8 *value,
+			int value_size)
+{
+	int i;
+	int num_records = kvp_file_info[pool].num_records;
+	struct kvp_record *record = kvp_file_info[pool].records;
+
+	if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
+		(value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
+		return 1;
+
+	for (i = 0; i < num_records; i++) {
+		if (memcmp(key, record[i].key, key_size))
+			continue;
+		/*
+		 * Found a match; just copy the value out.
+		 */
+		memcpy(value, record[i].value, value_size);
+		return 0;
+	}
+
+	return 1;
+}
+
 void kvp_get_os_info(void)
 {
 	FILE	*file;
@@ -315,6 +560,11 @@ int main(void)
 	 */
 	kvp_get_os_info();
 
+	if (kvp_file_init()) {
+		syslog(LOG_ERR, "Failed to initialize the pools");
+		exit(-1);
+	}
+
 	fd = socket(AF_NETLINK, SOCK_DGRAM, NETLINK_CONNECTOR);
 	if (fd < 0) {
 		syslog(LOG_ERR, "netlink socket creation failed; error:%d", fd);
@@ -389,9 +639,38 @@ int main(void)
 			}
 			continue;
 
+		/*
+		 * The current protocol with the kernel component uses a
+		 * NULL key name to pass an error condition.
+		 * For the SET, GET and DELETE operations,
+		 * use the existing protocol to pass back error.
+		 */
+
 		case KVP_OP_SET:
+			if (kvp_key_add_or_modify(hv_msg->kvp_hdr.pool,
+					hv_msg->body.kvp_set.data.key,
+					hv_msg->body.kvp_set.data.key_size,
+					hv_msg->body.kvp_set.data.value,
+					hv_msg->body.kvp_set.data.value_size))
+				strcpy(hv_msg->body.kvp_set.data.key, "");
+			break;
+
 		case KVP_OP_GET:
+			if (kvp_get_value(hv_msg->kvp_hdr.pool,
+					hv_msg->body.kvp_set.data.key,
+					hv_msg->body.kvp_set.data.key_size,
+					hv_msg->body.kvp_set.data.value,
+					hv_msg->body.kvp_set.data.value_size))
+				strcpy(hv_msg->body.kvp_set.data.key, "");
+			break;
+
 		case KVP_OP_DELETE:
+			if (kvp_key_delete(hv_msg->kvp_hdr.pool,
+					hv_msg->body.kvp_delete.key,
+					hv_msg->body.kvp_delete.key_size))
+				strcpy(hv_msg->body.kvp_delete.key, "");
+			break;
+
 		default:
 			break;
 		}
-- 
1.7.4.1

[PATCH 4/4] Tools: hv: Support enumeration from all the pools

[K. Y. Srinivasan]

We have supported enumeration only from the AUTO pool. Now support
enumeration from all the available pools.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/hv/hv_kvp.c      |    7 ++-
 include/linux/hyperv.h   |    1 +
 tools/hv/hv_kvp_daemon.c |  124 +++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 122 insertions(+), 10 deletions(-)

diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 3b2eeaa..1a70b10 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -286,14 +286,15 @@ kvp_respond_to_host(char *key, char *value, int error)
 
 
 	/*
-	 * If the error parameter is set, terminate the host's enumeration.
+	 * If the error parameter is set, terminate the host's enumeration
+	 * on this pool.
 	 */
 	if (error) {
 		/*
 		 * Something failed or the we have timedout;
-		 * terminate the host-side iteration by returning an error.
+		 * terminate the current  host-side iteration.
 		 */
-		icmsghdrp->status = HV_E_FAIL;
+		icmsghdrp->status = HV_S_CONT;
 		goto response_done;
 	}
 
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index e88a979..5852545 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -952,6 +952,7 @@ void vmbus_driver_unregister(struct hv_driver *hv_driver);
 
 #define HV_S_OK				0x00000000
 #define HV_E_FAIL			0x80004005
+#define HV_S_CONT			0x80070103
 #define HV_ERROR_NOT_SUPPORTED		0x80070032
 #define HV_ERROR_MACHINE_LOCKED		0x800704F7
 
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 2fb9c3d..146fd61 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -148,6 +148,51 @@ static void kvp_update_file(int pool)
 	kvp_release_lock(pool);
 }
 
+static void kvp_update_mem_state(int pool)
+{
+	FILE *filep;
+	size_t records_read = 0;
+	struct kvp_record *record = kvp_file_info[pool].records;
+	struct kvp_record *readp;
+	int num_blocks = kvp_file_info[pool].num_blocks;
+	int alloc_unit = sizeof(struct kvp_record) * ENTRIES_PER_BLOCK;
+
+	kvp_acquire_lock(pool);
+
+	filep = fopen(kvp_file_info[pool].fname, "r");
+	if (!filep) {
+		kvp_release_lock(pool);
+		syslog(LOG_ERR, "Failed to open file, pool: %d", pool);
+		exit(-1);
+	}
+	while (!feof(filep)) {
+		readp = &record[records_read];
+		records_read += fread(readp, sizeof(struct kvp_record),
+					ENTRIES_PER_BLOCK * num_blocks,
+					filep);
+
+		if (!feof(filep)) {
+			/*
+			 * We have more data to read.
+			 */
+			num_blocks++;
+			record = realloc(record, alloc_unit * num_blocks);
+
+			if (record == NULL) {
+				syslog(LOG_ERR, "malloc failed");
+				exit(-1);
+			}
+			continue;
+		}
+		break;
+	}
+
+	kvp_file_info[pool].num_blocks = num_blocks;
+	kvp_file_info[pool].records = record;
+	kvp_file_info[pool].num_records = records_read;
+
+	kvp_release_lock(pool);
+}
 static int kvp_file_init(void)
 {
 	int ret, fd;
@@ -223,8 +268,16 @@ static int kvp_key_delete(int pool, __u8 *key, int key_size)
 {
 	int i;
 	int j, k;
-	int num_records = kvp_file_info[pool].num_records;
-	struct kvp_record *record = kvp_file_info[pool].records;
+	int num_records;
+	struct kvp_record *record;
+
+	/*
+	 * First update the in-memory state.
+	 */
+	kvp_update_mem_state(pool);
+
+	num_records = kvp_file_info[pool].num_records;
+	record = kvp_file_info[pool].records;
 
 	for (i = 0; i < num_records; i++) {
 		if (memcmp(key, record[i].key, key_size))
@@ -259,14 +312,23 @@ static int kvp_key_add_or_modify(int pool, __u8 *key, int key_size, __u8 *value,
 {
 	int i;
 	int j, k;
-	int num_records = kvp_file_info[pool].num_records;
-	struct kvp_record *record = kvp_file_info[pool].records;
-	int num_blocks = kvp_file_info[pool].num_blocks;
+	int num_records;
+	struct kvp_record *record;
+	int num_blocks;
 
 	if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
 		(value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
 		return 1;
 
+	/*
+	 * First update the in-memory state.
+	 */
+	kvp_update_mem_state(pool);
+
+	num_records = kvp_file_info[pool].num_records;
+	record = kvp_file_info[pool].records;
+	num_blocks = kvp_file_info[pool].num_blocks;
+
 	for (i = 0; i < num_records; i++) {
 		if (memcmp(key, record[i].key, key_size))
 			continue;
@@ -304,13 +366,21 @@ static int kvp_get_value(int pool, __u8 *key, int key_size, __u8 *value,
 			int value_size)
 {
 	int i;
-	int num_records = kvp_file_info[pool].num_records;
-	struct kvp_record *record = kvp_file_info[pool].records;
+	int num_records;
+	struct kvp_record *record;
 
 	if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
 		(value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE))
 		return 1;
 
+	/*
+	 * First update the in-memory state.
+	 */
+	kvp_update_mem_state(pool);
+
+	num_records = kvp_file_info[pool].num_records;
+	record = kvp_file_info[pool].records;
+
 	for (i = 0; i < num_records; i++) {
 		if (memcmp(key, record[i].key, key_size))
 			continue;
@@ -324,6 +394,31 @@ static int kvp_get_value(int pool, __u8 *key, int key_size, __u8 *value,
 	return 1;
 }
 
+static void kvp_pool_enumerate(int pool, int index, __u8 *key, int key_size,
+				__u8 *value, int value_size)
+{
+	struct kvp_record *record;
+
+	/*
+	 * First update our in-memory database.
+	 */
+	kvp_update_mem_state(pool);
+	record = kvp_file_info[pool].records;
+
+	if (index >= kvp_file_info[pool].num_records) {
+		/*
+		 * This is an invalid index; terminate enumeration;
+		 * - a NULL value will do the trick.
+		 */
+		strcpy(value, "");
+		return;
+	}
+
+	memcpy(key, record[index].key, key_size);
+	memcpy(value, record[index].value, value_size);
+}
+
+
 void kvp_get_os_info(void)
 {
 	FILE	*file;
@@ -678,6 +773,21 @@ int main(void)
 		if (hv_msg->kvp_hdr.operation != KVP_OP_ENUMERATE)
 			goto kvp_done;
 
+		/*
+		 * If the pool is KVP_POOL_AUTO, dynamically generate
+		 * both the key and the value; if not read from the
+		 * appropriate pool.
+		 */
+		if (hv_msg->kvp_hdr.pool != KVP_POOL_AUTO) {
+			kvp_pool_enumerate(hv_msg->kvp_hdr.pool,
+					hv_msg->body.kvp_enum_data.index,
+					hv_msg->body.kvp_enum_data.data.key,
+					HV_KVP_EXCHANGE_MAX_KEY_SIZE,
+					hv_msg->body.kvp_enum_data.data.value,
+					HV_KVP_EXCHANGE_MAX_VALUE_SIZE);
+			goto kvp_done;
+		}
+
 		hv_msg = (struct hv_kvp_msg *)incoming_cn_msg->data;
 		key_name = (char *)hv_msg->body.kvp_enum_data.data.key;
 		key_value = (char *)hv_msg->body.kvp_enum_data.data.value;
-- 
1.7.4.1

Re: [PATCH 0000/0004] drivers: hv

[Greg KH]

On Sat, Mar 10, 2012 at 03:31:40PM -0800, K. Y. Srinivasan wrote:
> 
> This patch-set further enhances the KVP functionality for Linux
> guests:
> 
> 	1. Supports most of the Win8 KVP protocol.
> 	2. Supports operations on all the pools.

I've only applied the first patch, due to the issues with the second.
Feel free to resend the rest when you have them worked out.

greg k-h

RE: [PATCH 0000/0004] drivers: hv

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, March 13, 2012 5:51 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; ohering@suse.com
> Subject: Re: [PATCH 0000/0004] drivers: hv
> 
> On Sat, Mar 10, 2012 at 03:31:40PM -0800, K. Y. Srinivasan wrote:
> >
> > This patch-set further enhances the KVP functionality for Linux
> > guests:
> >
> > 	1. Supports most of the Win8 KVP protocol.
> > 	2. Supports operations on all the pools.
> 
> I've only applied the first patch, due to the issues with the second.
> Feel free to resend the rest when you have them worked out.

Thanks Greg. I don't think there are any substantive issues with the remaining 
patches. I will fix them up and re-send them soon.

K. Y