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

[Greg KH <greg@kroah.com>]

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

Why does the kernel care about the string at all?

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

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

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

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

thanks,

greg k-h