Re: [PATCH 1/1] Drivers: hv: vmbus: Add additional distro specific Kconfig options

Re: [PATCH 1/1] Drivers: hv: vmbus: Add additional distro specific Kconfig options

[Greg KH]

On Wed, Apr 10, 2013 at 09:08:17AM -0700, K. Y. Srinivasan wrote:
> The guest ID captures information about the guest and has room for
> distributions to add distro specific information. Add Kconfig options
> to support distro specific information to be managed easily.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
>  drivers/hv/Kconfig        |   14 ++++++++++++++
>  drivers/hv/hv.c           |    4 +++-
>  drivers/hv/hyperv_vmbus.h |    2 +-
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> index 0403b51..d2ca9c7 100644
> --- a/drivers/hv/Kconfig
> +++ b/drivers/hv/Kconfig
> @@ -19,4 +19,18 @@ config HYPERV_BALLOON
>  	help
>  	  Select this option to enable Hyper-V Balloon driver.
>  
> +config HYPERV_GUEST_D1
> +	hex "Distro specific information"
> +	range 0x00 0xff
> +	default 0
> +	help
> +	  This specifies the Distro vendor

And just what is a distro supposed to set the value here to?  For
example, what would Arch Linux pick?  Fedora?  RHEL?  SLES?  openSUSE?
You do know that there are more than 0xff different Linux distro
"vendors"?  Do they all just get to pick a number that they like?

> +config HYPERV_GUEST_D2
> +	hex "Additional Distro specific information"
> +	range 0x0000 0xffff
> +	default 0
> +	help
> +	  Additional Distro specific kernel version information

Again, what is a distro supposed to do here?  What's wrong with the
kernel version information that the kernel already has?  Can't you just
use that string?

greg k-h

[PATCH 1/1] Drivers: hv: vmbus: Add additional distro specific Kconfig options

[K. Y. Srinivasan]

The guest ID captures information about the guest and has room for
distributions to add distro specific information. Add Kconfig options
to support distro specific information to be managed easily.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/Kconfig        |   14 ++++++++++++++
 drivers/hv/hv.c           |    4 +++-
 drivers/hv/hyperv_vmbus.h |    2 +-
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 0403b51..d2ca9c7 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -19,4 +19,18 @@ config HYPERV_BALLOON
 	help
 	  Select this option to enable Hyper-V Balloon driver.
 
+config HYPERV_GUEST_D1
+	hex "Distro specific information"
+	range 0x00 0xff
+	default 0
+	help
+	  This specifies the Distro vendor
+
+config HYPERV_GUEST_D2
+	hex "Additional Distro specific information"
+	range 0x0000 0xffff
+	default 0
+	help
+	  Additional Distro specific kernel version information
+
 endmenu
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index ae49237..7bd2e88 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -148,7 +148,9 @@ int hv_init(void)
 	/*
 	 * Write our OS ID.
 	 */
-	hv_context.guestid = generate_guest_id(0, LINUX_VERSION_CODE, 0);
+	hv_context.guestid = generate_guest_id(HYPERV_GUEST_D1,
+						LINUX_VERSION_CODE,
+						HYPERV_GUEST_D2);
 	wrmsrl(HV_X64_MSR_GUEST_OS_ID, hv_context.guestid);
 
 	/* See if the hypercall page is already set */
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 12f2f9e..8c9ebd0 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -410,7 +410,7 @@ enum {
  *
  * Bit(s)
  * 63 - Indicates if the OS is Open Source or not; 1 is Open Source
- * 62:56 - Os Type; Linux is 0x100
+ * 62:56 - Os Type; Linux is 0x1
  * 55:48 - Distro specific identification
  * 47:16 - Linux kernel version number
  * 15:0  - Distro specific identification
-- 
1.7.4.1

RE: [PATCH 1/1] Drivers: hv: vmbus: Add additional distro specific Kconfig options

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Wednesday, April 10, 2013 11:50 AM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH 1/1] Drivers: hv: vmbus: Add additional distro specific Kconfig
> options
> 
> On Wed, Apr 10, 2013 at 09:08:17AM -0700, K. Y. Srinivasan wrote:
> > The guest ID captures information about the guest and has room for
> > distributions to add distro specific information. Add Kconfig options
> > to support distro specific information to be managed easily.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > ---
> >  drivers/hv/Kconfig        |   14 ++++++++++++++
> >  drivers/hv/hv.c           |    4 +++-
> >  drivers/hv/hyperv_vmbus.h |    2 +-
> >  3 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> > index 0403b51..d2ca9c7 100644
> > --- a/drivers/hv/Kconfig
> > +++ b/drivers/hv/Kconfig
> > @@ -19,4 +19,18 @@ config HYPERV_BALLOON
> >  	help
> >  	  Select this option to enable Hyper-V Balloon driver.
> >
> > +config HYPERV_GUEST_D1
> > +	hex "Distro specific information"
> > +	range 0x00 0xff
> > +	default 0
> > +	help
> > +	  This specifies the Distro vendor
> 
> And just what is a distro supposed to set the value here to?  For
> example, what would Arch Linux pick?  Fedora?  RHEL?  SLES?  openSUSE?
> You do know that there are more than 0xff different Linux distro
> "vendors"?  Do they all just get to pick a number that they like?

MSFT is planning to manage this space; we will assign distributions unique numbers
that they can use here.

> 
> > +config HYPERV_GUEST_D2
> > +	hex "Additional Distro specific information"
> > +	range 0x0000 0xffff
> > +	default 0
> > +	help
> > +	  Additional Distro specific kernel version information
> 
> Again, what is a distro supposed to do here?  What's wrong with the
> kernel version information that the kernel already has?  Can't you just
> use that string?


We already have the kernel version information. Distros can choose to use this
as they please. We are recommending that they use this field to track some distro specific
build number.

Regards,

K. Y

Re: [PATCH 1/1] Drivers: hv: vmbus: Add additional distro specific Kconfig options

[Greg KH]

On Wed, Apr 10, 2013 at 04:12:24PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Wednesday, April 10, 2013 11:50 AM
> > To: KY Srinivasan
> > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> > apw@canonical.com; jasowang@redhat.com
> > Subject: Re: [PATCH 1/1] Drivers: hv: vmbus: Add additional distro specific Kconfig
> > options
> > 
> > On Wed, Apr 10, 2013 at 09:08:17AM -0700, K. Y. Srinivasan wrote:
> > > The guest ID captures information about the guest and has room for
> > > distributions to add distro specific information. Add Kconfig options
> > > to support distro specific information to be managed easily.
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > ---
> > >  drivers/hv/Kconfig        |   14 ++++++++++++++
> > >  drivers/hv/hv.c           |    4 +++-
> > >  drivers/hv/hyperv_vmbus.h |    2 +-
> > >  3 files changed, 18 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> > > index 0403b51..d2ca9c7 100644
> > > --- a/drivers/hv/Kconfig
> > > +++ b/drivers/hv/Kconfig
> > > @@ -19,4 +19,18 @@ config HYPERV_BALLOON
> > >  	help
> > >  	  Select this option to enable Hyper-V Balloon driver.
> > >
> > > +config HYPERV_GUEST_D1
> > > +	hex "Distro specific information"
> > > +	range 0x00 0xff
> > > +	default 0
> > > +	help
> > > +	  This specifies the Distro vendor
> > 
> > And just what is a distro supposed to set the value here to?  For
> > example, what would Arch Linux pick?  Fedora?  RHEL?  SLES?  openSUSE?
> > You do know that there are more than 0xff different Linux distro
> > "vendors"?  Do they all just get to pick a number that they like?
> 
> MSFT is planning to manage this space; we will assign distributions
> unique numbers that they can use here.

And how will that happen?  A single line in a Kconfig file doesn't cut
it, sorry.

What's wrong with lanana.org managing it instead?  That's who manages
all of the rest of the kernel-limited resources.

And what happens when you have more than 256?  We already know that's
not going to be enough numbers, why start out already with an obsolete
range?

> > > +config HYPERV_GUEST_D2
> > > +	hex "Additional Distro specific information"
> > > +	range 0x0000 0xffff
> > > +	default 0
> > > +	help
> > > +	  Additional Distro specific kernel version information
> > 
> > Again, what is a distro supposed to do here?  What's wrong with the
> > kernel version information that the kernel already has?  Can't you just
> > use that string?
> 
> We already have the kernel version information. Distros can choose to
> use this as they please. We are recommending that they use this field
> to track some distro specific build number.

That's in the kernel version already, why put it here again?

Again, you need _way_ more than just a single Kconfig line for new
configuration options where you expect distros to set a value to a
unique thing,

And lastly, you do know about /etc/os-release, the cross-distro,
standardized way to specify distro version and build information,
right?  Why aren't you just using that instead of creating your own
"unique" way of defining the exact same thing?

Don't create a new standard just when we all finally picked one to
follow, it makes it look like you don't think what the community decided
on is valid, which isn't a wise decision.

greg k-h

RE: [PATCH 1/1] Drivers: hv: vmbus: Add additional distro specific Kconfig options

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Wednesday, April 10, 2013 12:34 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH 1/1] Drivers: hv: vmbus: Add additional distro specific Kconfig
> options
> 
> On Wed, Apr 10, 2013 at 04:12:24PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > Sent: Wednesday, April 10, 2013 11:50 AM
> > > To: KY Srinivasan
> > > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> olaf@aepfle.de;
> > > apw@canonical.com; jasowang@redhat.com
> > > Subject: Re: [PATCH 1/1] Drivers: hv: vmbus: Add additional distro specific
> Kconfig
> > > options
> > >
> > > On Wed, Apr 10, 2013 at 09:08:17AM -0700, K. Y. Srinivasan wrote:
> > > > The guest ID captures information about the guest and has room for
> > > > distributions to add distro specific information. Add Kconfig options
> > > > to support distro specific information to be managed easily.
> > > >
> > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > ---
> > > >  drivers/hv/Kconfig        |   14 ++++++++++++++
> > > >  drivers/hv/hv.c           |    4 +++-
> > > >  drivers/hv/hyperv_vmbus.h |    2 +-
> > > >  3 files changed, 18 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> > > > index 0403b51..d2ca9c7 100644
> > > > --- a/drivers/hv/Kconfig
> > > > +++ b/drivers/hv/Kconfig
> > > > @@ -19,4 +19,18 @@ config HYPERV_BALLOON
> > > >  	help
> > > >  	  Select this option to enable Hyper-V Balloon driver.
> > > >
> > > > +config HYPERV_GUEST_D1
> > > > +	hex "Distro specific information"
> > > > +	range 0x00 0xff
> > > > +	default 0
> > > > +	help
> > > > +	  This specifies the Distro vendor
> > >
> > > And just what is a distro supposed to set the value here to?  For
> > > example, what would Arch Linux pick?  Fedora?  RHEL?  SLES?  openSUSE?
> > > You do know that there are more than 0xff different Linux distro
> > > "vendors"?  Do they all just get to pick a number that they like?
> >
> > MSFT is planning to manage this space; we will assign distributions
> > unique numbers that they can use here.
> 
> And how will that happen?  A single line in a Kconfig file doesn't cut
> it, sorry.
> 
> What's wrong with lanana.org managing it instead?  That's who manages
> all of the rest of the kernel-limited resources.
> 
> And what happens when you have more than 256?  We already know that's
> not going to be enough numbers, why start out already with an obsolete
> range?

Greg,
You are reading more into this than there is. As it stands now (without this patch), the code
that constructs the guest OS ID has place holders for adding in some distro specific tags into
this ID. Look at the function generate_guest_id() in  hyperv_vmbus.h. Currently these values are
hard-coded to 0. All I am doing here is to present a more maintainable way of customizing these.
The default values for these are currently 0 and even with this patch, the default values will continue
to be 0. However, if a distro chooses to modify these defaults, with this patch they can do so in a more
maintainable way - they could set this config parameter on a per-tree basis.

This information will potentially be used to automate the support infrastructure. We feel that with 24 bits
between the two distro specific tags, we have enough bits to differentiate the distros that may run on 
our platform.
 
> 
> > > > +config HYPERV_GUEST_D2
> > > > +	hex "Additional Distro specific information"
> > > > +	range 0x0000 0xffff
> > > > +	default 0
> > > > +	help
> > > > +	  Additional Distro specific kernel version information
> > >
> > > Again, what is a distro supposed to do here?  What's wrong with the
> > > kernel version information that the kernel already has?  Can't you just
> > > use that string?
> >
> > We already have the kernel version information. Distros can choose to
> > use this as they please. We are recommending that they use this field
> > to track some distro specific build number.
> 
> That's in the kernel version already, why put it here again?
> 
> Again, you need _way_ more than just a single Kconfig line for new
> configuration options where you expect distros to set a value to a
> unique thing,
> 
> And lastly, you do know about /etc/os-release, the cross-distro,
> standardized way to specify distro version and build information,
> right?  Why aren't you just using that instead of creating your own
> "unique" way of defining the exact same thing?
> 
> Don't create a new standard just when we all finally picked one to
> follow, it makes it look like you don't think what the community decided
> on is valid, which isn't a wise decision.

As I said, we are not proposing any new standard here. KVP currently retrieves info
from /etc/os-release. The tooling on the Windows host uses the guest OS ID MSR to identify guest
operating systems running on the host . With what we currently have, we can detect that it is Linux based
on the information we use to construct the OS ID. This information also includes the kernel version number
as well. This patch merely makes it easy to add distro specific tags. It is entirely up to distros to decide if they want to
add these tags. The default values are 0 for both these fields and they continue to be 0.

Regards,

K. Y

Re: [PATCH 1/1] Drivers: hv: vmbus: Add additional distro specific Kconfig options

[Greg KH]

On Wed, Apr 10, 2013 at 05:37:41PM +0000, KY Srinivasan wrote:
> You are reading more into this than there is. As it stands now
> (without this patch), the code that constructs the guest OS ID has
> place holders for adding in some distro specific tags into this ID.

I understand that, the fact that you now want this to be a configurable
value at build time, yet provide exactly NO information to anyone as to
what to set the values to, what the values mean, or anything else about
these values this is why I am rejecting this patch.

If you are already properly handling os-release, then there is no need
for these values to mean anything for a Linux guest, as it would be
totally redundant, and probably wrong (hint, the same kernel binary can
be running on different os userspace layers.)

sorry,

greg k-h