Linux Virtualization - Nov 2010 - [PATCH]: An implementation of HyperV KVP functionality

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

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: kvp.patch
Url:
http://lists.linux-foundation.org/pipermail/virtualization/attachments/20101111/85bac067/attachment-0001.txt
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kvp_daemon.c
Type: application/octet-stream
Size: 10284 bytes
Desc: not available
Url :
http://lists.linux-foundation.org/pipermail/virtualization/attachments/20101111/85bac067/attachment-0001.obj

On Thu, 11 Nov 2010 13:03:10 -0700
"Ky Srinivasan" <ksrinivasan at 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

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

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

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