Re: [PATCH]: An implementation of HyperV KVP functionality
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. Srinivasanksriniva...@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.
+ *
+ */
+
+
+#includelinux/net.h
+#includelinux/nls.h
+#includelinux/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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH]: An implementation of HyperV KVP functionality
On 11/11/2010 at 3:49 PM, in message 2010124904.24010...@nehalam,
Stephen Hemminger shemmin...@vyatta.com wrote:
On Thu, 11 Nov 2010 13:03:10 -0700
Ky Srinivasan ksriniva...@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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH]: An implementation of HyperV KVP functionality
On 11/11/2010 at 3:49 PM, in message 2010124904.24010...@nehalam,
Stephen Hemminger shemmin...@vyatta.com wrote:
On Thu, 11 Nov 2010 13:03:10 -0700
Ky Srinivasan ksriniva...@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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH]: An implementation of HyperV KVP functionality
On 11/11/2010 at 3:49 PM, in message 2010124904.24010...@nehalam,
Stephen Hemminger shemmin...@vyatta.com wrote:
On Thu, 11 Nov 2010 13:03:10 -0700
Ky Srinivasan ksriniva...@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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH]: An implementation of HyperV KVP functionality
On 11/11/2010 at 4:15 PM, in message 2010211548.ga31...@kroah.com,
Greg
KH g...@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 ksriniva...@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_SIZE512
+#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.0
-0500
+++ linux.trees.git/drivers/staging/hv/Makefile 2010-11-11
11:24:54.0
-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
Re: [PATCH]: An implementation of HyperV KVP functionality
On 11/11/2010 at 4:19 PM, in message 2010211904.gb31...@kroah.com,
Greg
KH g...@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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH]: An implementation of HyperV KVP functionality
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.0
-0500
+++ linux.trees.git/include/linux/connector.h 2010-11-11
13:14:52.0
-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_IDX0x9 /* MSFT KVP
functionality */
Did you reserve this number with anyone? Who?
I sent an email toEvgeniy Polyakov (john...@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 z...@ioremap.net instead.
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH]: An implementation of HyperV KVP functionality
On Fri, Nov 12, 2010 at 11:29:58AM -0700, Ky Srinivasan wrote:
On 11/11/2010 at 4:19 PM, in message 2010211904.gb31...@kroah.com,
Greg
KH g...@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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH]: An implementation of HyperV KVP functionality
On 11/12/2010 at 1:47 PM, in message 20101112184753.ga20...@kroah.com,
Greg
KH g...@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.h2010-11-09
17:22:15.0
-0500
+++ linux.trees.git/include/linux/connector.h 2010-11-11
13:14:52.0
-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 (john...@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 z...@ioremap.net instead.
Thanks I will do just that.
K. Y
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH]: An implementation of HyperV KVP functionality
On Fri, Nov 12, 2010 at 01:59:42PM -0700, Ky Srinivasan wrote:
On 11/12/2010 at 1:47 PM, in message 20101112184753.ga20...@kroah.com,
Greg
KH g...@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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
[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
From: K. Y. Srinivasan ksriniva...@novell.com
Subject: An implementation of key/value pair feature (KVP) for Linux
on HyperV.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
Index: linux.trees.git/drivers/staging/hv/kvp.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux.trees.git/drivers/staging/hv/kvp.c2010-11-11 13:45:17.0
-0500
@@ -0,0 +1,404 @@
+/*
+ * An implementation of key value pair (KVP) functionality for Linux.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan ksriniva...@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_SIZE512
+#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_KEY10
+#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
Re: [PATCH]: An implementation of HyperV KVP functionality
On Thu, 11 Nov 2010 13:03:10 -0700
Ky Srinivasan ksriniva...@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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH]: An implementation of HyperV KVP functionality
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH]: An implementation of HyperV KVP functionality
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 ksriniva...@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_SIZE512
+#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.0 -0500
+++ linux.trees.git/drivers/staging/hv/Makefile 2010-11-11
11:24:54.0 -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.0 +
+++ linux.trees.git/drivers/staging/hv/kvp.h 2010-11-10 14:03:47.0
-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,
+
