[PATCH]: An implementation of HyperV KVP functionality

Ky Srinivasan ksrinivasan at novell.com
Fri Nov 12 18:06:18 UTC 2010



>>> On 11/11/2010 at  4:15 PM, in message <20101111211548.GA31373 at kroah.com>, Greg
KH <greg at 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 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.

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




More information about the devel mailing list