RE: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address injection
> -Original Message-
> From: devel-boun...@linuxdriverproject.org [mailto:devel-
> boun...@linuxdriverproject.org] On Behalf Of KY Srinivasan
> Sent: Monday, August 13, 2012 10:57 PM
> To: Greg KH
> Cc: o...@aepfle.de; linux-kernel@vger.kernel.org;
> virtualizat...@lists.osdl.org;
> a...@canonical.com; de...@linuxdriverproject.org; b...@decadent.org.uk
> Subject: RE: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address
> injection
>
>
>
> > -Original Message-
> > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > Sent: Monday, August 13, 2012 9:38 PM
> > To: KY Srinivasan
> > Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org;
> > virtualizat...@lists.osdl.org; o...@aepfle.de; a...@canonical.com;
> > b...@decadent.org.uk
> > Subject: Re: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP
> > address
> > injection
> >
> > On Mon, Aug 13, 2012 at 10:06:51AM -0700, K. Y. Srinivasan wrote:
> > > Add the necessary definitions for supporting the IP injection
> > > functionality.
> > >
> > > Signed-off-by: K. Y. Srinivasan
> > > Reviewed-by: Haiyang Zhang
> > > Reviewed-by: Olaf Hering
> > > Reviewed-by: Ben Hutchings
> > > ---
> > > drivers/hv/hv_util.c |4 +-
> > > include/linux/hyperv.h | 76
> > -
> > > tools/hv/hv_kvp_daemon.c |2 +-
> > > 3 files changed, 77 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> > > index d3ac6a4..a0667de 100644
> > > --- a/drivers/hv/hv_util.c
> > > +++ b/drivers/hv/hv_util.c
> > > @@ -263,7 +263,7 @@ static int util_probe(struct hv_device *dev,
> > > (struct hv_util_service *)dev_id->driver_data;
> > > int ret;
> > >
> > > - srv->recv_buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > > + srv->recv_buffer = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
> > > if (!srv->recv_buffer)
> > > return -ENOMEM;
> > > if (srv->util_init) {
> > > @@ -274,7 +274,7 @@ static int util_probe(struct hv_device *dev,
> > > }
> > > }
> > >
> > > - ret = vmbus_open(dev->channel, 2 * PAGE_SIZE, 2 * PAGE_SIZE, NULL,
> > 0,
> > > + ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL,
> > 0,
> > > srv->util_cb, dev->channel);
> > > if (ret)
> > > goto error;
> > > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > > index 68ed7f7..11afc4e 100644
> > > --- a/include/linux/hyperv.h
> > > +++ b/include/linux/hyperv.h
> > > @@ -122,12 +122,53 @@
> > > #define REG_U32 4
> > > #define REG_U64 8
> > >
> > > +/*
> > > + * As we look at expanding the KVP functionality to include
> > > + * IP injection functionality, we need to maintain binary
> > > + * compatibility with older daemons.
> > > + *
> > > + * The KVP opcodes are defined by the host and it was unfortunate
> > > + * that I chose to treat the registration operation as part of the
> > > + * KVP operations defined by the host.
> > > + * Here is the level of compatibility
> > > + * (between the user level daemon and the kernel KVP driver) that we
> > > + * will implement:
> > > + *
> > > + * An older daemon will always be supported on a newer driver.
> > > + * A given user level daemon will require a minimal version of the
> > > + * kernel driver.
> > > + * If we cannot handle the version differences, we will fail gracefully
> > > + * (this can happen when we have a user level daemon that is more
> > > + * advanced than the KVP driver.
> > > + *
> > > + * We will use values used in this handshake for determining if we have
> > > + * workable user level daemon and the kernel driver. We begin by taking
> > > the
> > > + * registration opcode out of the KVP opcode namespace. We will however,
> > > + * maintain compatibility with the existing user-level daemon code.
> > > + */
> > > +
> > > +/*
> > > + * Daemon code not supporting IP injection (legacy daemon).
> > > + */
> > > +
> > > +#define KVP_OP_REGISTER 4
> >
> > Huh?
> >
> > > +/*
> > > + * Daemon code supporting IP injection.
> > > + * The KVP opcode field is used to communicate the
> >
RE: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address injection
-Original Message-
From: devel-boun...@linuxdriverproject.org [mailto:devel-
boun...@linuxdriverproject.org] On Behalf Of KY Srinivasan
Sent: Monday, August 13, 2012 10:57 PM
To: Greg KH
Cc: o...@aepfle.de; linux-kernel@vger.kernel.org;
virtualizat...@lists.osdl.org;
a...@canonical.com; de...@linuxdriverproject.org; b...@decadent.org.uk
Subject: RE: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address
injection
-Original Message-
From: Greg KH [mailto:gre...@linuxfoundation.org]
Sent: Monday, August 13, 2012 9:38 PM
To: KY Srinivasan
Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org;
virtualizat...@lists.osdl.org; o...@aepfle.de; a...@canonical.com;
b...@decadent.org.uk
Subject: Re: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP
address
injection
On Mon, Aug 13, 2012 at 10:06:51AM -0700, K. Y. Srinivasan wrote:
Add the necessary definitions for supporting the IP injection
functionality.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Reviewed-by: Haiyang Zhang haiya...@microsoft.com
Reviewed-by: Olaf Hering o...@aepfle.de
Reviewed-by: Ben Hutchings b...@decadent.org.uk
---
drivers/hv/hv_util.c |4 +-
include/linux/hyperv.h | 76
-
tools/hv/hv_kvp_daemon.c |2 +-
3 files changed, 77 insertions(+), 5 deletions(-)
diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index d3ac6a4..a0667de 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -263,7 +263,7 @@ static int util_probe(struct hv_device *dev,
(struct hv_util_service *)dev_id-driver_data;
int ret;
- srv-recv_buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
+ srv-recv_buffer = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
if (!srv-recv_buffer)
return -ENOMEM;
if (srv-util_init) {
@@ -274,7 +274,7 @@ static int util_probe(struct hv_device *dev,
}
}
- ret = vmbus_open(dev-channel, 2 * PAGE_SIZE, 2 * PAGE_SIZE, NULL,
0,
+ ret = vmbus_open(dev-channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL,
0,
srv-util_cb, dev-channel);
if (ret)
goto error;
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 68ed7f7..11afc4e 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -122,12 +122,53 @@
#define REG_U32 4
#define REG_U64 8
+/*
+ * As we look at expanding the KVP functionality to include
+ * IP injection functionality, we need to maintain binary
+ * compatibility with older daemons.
+ *
+ * The KVP opcodes are defined by the host and it was unfortunate
+ * that I chose to treat the registration operation as part of the
+ * KVP operations defined by the host.
+ * Here is the level of compatibility
+ * (between the user level daemon and the kernel KVP driver) that we
+ * will implement:
+ *
+ * An older daemon will always be supported on a newer driver.
+ * A given user level daemon will require a minimal version of the
+ * kernel driver.
+ * If we cannot handle the version differences, we will fail gracefully
+ * (this can happen when we have a user level daemon that is more
+ * advanced than the KVP driver.
+ *
+ * We will use values used in this handshake for determining if we have
+ * workable user level daemon and the kernel driver. We begin by taking
the
+ * registration opcode out of the KVP opcode namespace. We will however,
+ * maintain compatibility with the existing user-level daemon code.
+ */
+
+/*
+ * Daemon code not supporting IP injection (legacy daemon).
+ */
+
+#define KVP_OP_REGISTER 4
Huh?
+/*
+ * Daemon code supporting IP injection.
+ * The KVP opcode field is used to communicate the
+ * registration information; so define a namespace that
+ * will be distinct from the host defined KVP opcode.
+ */
+
+#define KVP_OP_REGISTER1 100
+
enum hv_kvp_exchg_op {
KVP_OP_GET = 0,
KVP_OP_SET,
KVP_OP_DELETE,
KVP_OP_ENUMERATE,
- KVP_OP_REGISTER,
+ KVP_OP_GET_IP_INFO,
+ KVP_OP_SET_IP_INFO,
So you overloaded the command and somehow think that is ok? How is that
supposed to work? Why not just always keep it there, but fail if it is
called as you know you have a mismatch?
Otherwise, again, you just broke older tools on a newer kernel.
Or am I missing something here?
Greg,
The registration operation occurs when the daemon first starts up. I should
have
established
a distinct namespace for the daemon versions that would not overlap with the
host
defined KVP operations initially. Unfortunately when I first implemented KVP,
I
did not know
about the new KVP verbs and so selected a value that ended up colliding with
the
new KVP
operations. To maintain compatibility with older daemons, I have
RE: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address injection
> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Monday, August 13, 2012 9:38 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org;
> virtualizat...@lists.osdl.org; o...@aepfle.de; a...@canonical.com;
> b...@decadent.org.uk
> Subject: Re: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address
> injection
>
> On Mon, Aug 13, 2012 at 10:06:51AM -0700, K. Y. Srinivasan wrote:
> > Add the necessary definitions for supporting the IP injection functionality.
> >
> > Signed-off-by: K. Y. Srinivasan
> > Reviewed-by: Haiyang Zhang
> > Reviewed-by: Olaf Hering
> > Reviewed-by: Ben Hutchings
> > ---
> > drivers/hv/hv_util.c |4 +-
> > include/linux/hyperv.h | 76
> -
> > tools/hv/hv_kvp_daemon.c |2 +-
> > 3 files changed, 77 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> > index d3ac6a4..a0667de 100644
> > --- a/drivers/hv/hv_util.c
> > +++ b/drivers/hv/hv_util.c
> > @@ -263,7 +263,7 @@ static int util_probe(struct hv_device *dev,
> > (struct hv_util_service *)dev_id->driver_data;
> > int ret;
> >
> > - srv->recv_buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > + srv->recv_buffer = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
> > if (!srv->recv_buffer)
> > return -ENOMEM;
> > if (srv->util_init) {
> > @@ -274,7 +274,7 @@ static int util_probe(struct hv_device *dev,
> > }
> > }
> >
> > - ret = vmbus_open(dev->channel, 2 * PAGE_SIZE, 2 * PAGE_SIZE, NULL,
> 0,
> > + ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL,
> 0,
> > srv->util_cb, dev->channel);
> > if (ret)
> > goto error;
> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > index 68ed7f7..11afc4e 100644
> > --- a/include/linux/hyperv.h
> > +++ b/include/linux/hyperv.h
> > @@ -122,12 +122,53 @@
> > #define REG_U32 4
> > #define REG_U64 8
> >
> > +/*
> > + * As we look at expanding the KVP functionality to include
> > + * IP injection functionality, we need to maintain binary
> > + * compatibility with older daemons.
> > + *
> > + * The KVP opcodes are defined by the host and it was unfortunate
> > + * that I chose to treat the registration operation as part of the
> > + * KVP operations defined by the host.
> > + * Here is the level of compatibility
> > + * (between the user level daemon and the kernel KVP driver) that we
> > + * will implement:
> > + *
> > + * An older daemon will always be supported on a newer driver.
> > + * A given user level daemon will require a minimal version of the
> > + * kernel driver.
> > + * If we cannot handle the version differences, we will fail gracefully
> > + * (this can happen when we have a user level daemon that is more
> > + * advanced than the KVP driver.
> > + *
> > + * We will use values used in this handshake for determining if we have
> > + * workable user level daemon and the kernel driver. We begin by taking the
> > + * registration opcode out of the KVP opcode namespace. We will however,
> > + * maintain compatibility with the existing user-level daemon code.
> > + */
> > +
> > +/*
> > + * Daemon code not supporting IP injection (legacy daemon).
> > + */
> > +
> > +#define KVP_OP_REGISTER4
>
> Huh?
>
> > +/*
> > + * Daemon code supporting IP injection.
> > + * The KVP opcode field is used to communicate the
> > + * registration information; so define a namespace that
> > + * will be distinct from the host defined KVP opcode.
> > + */
> > +
> > +#define KVP_OP_REGISTER1 100
> > +
> > enum hv_kvp_exchg_op {
> > KVP_OP_GET = 0,
> > KVP_OP_SET,
> > KVP_OP_DELETE,
> > KVP_OP_ENUMERATE,
> > - KVP_OP_REGISTER,
> > + KVP_OP_GET_IP_INFO,
> > + KVP_OP_SET_IP_INFO,
>
> So you overloaded the command and somehow think that is ok? How is that
> supposed to work? Why not just always keep it there, but fail if it is
> called as you know you have a mismatch?
>
> Otherwise, again, you just broke older tools on a newer kernel.
>
> Or am I missing something here?
Greg,
The registration operation occurs when the daemon first starts up. I should
have established
a distinct namespace for the daemon versions that would not overlap with the
host
defined KVP
Re: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address injection
On Mon, 2012-08-13 at 18:38 -0700, Greg KH wrote:
> On Mon, Aug 13, 2012 at 10:06:51AM -0700, K. Y. Srinivasan wrote:
> > Add the necessary definitions for supporting the IP injection functionality.
[...]
> > --- a/include/linux/hyperv.h
> > +++ b/include/linux/hyperv.h
> > @@ -122,12 +122,53 @@
> > #define REG_U32 4
> > #define REG_U64 8
> >
> > +/*
> > + * As we look at expanding the KVP functionality to include
> > + * IP injection functionality, we need to maintain binary
> > + * compatibility with older daemons.
> > + *
> > + * The KVP opcodes are defined by the host and it was unfortunate
> > + * that I chose to treat the registration operation as part of the
> > + * KVP operations defined by the host.
> > + * Here is the level of compatibility
> > + * (between the user level daemon and the kernel KVP driver) that we
> > + * will implement:
> > + *
> > + * An older daemon will always be supported on a newer driver.
> > + * A given user level daemon will require a minimal version of the
> > + * kernel driver.
> > + * If we cannot handle the version differences, we will fail gracefully
> > + * (this can happen when we have a user level daemon that is more
> > + * advanced than the KVP driver.
> > + *
> > + * We will use values used in this handshake for determining if we have
> > + * workable user level daemon and the kernel driver. We begin by taking the
> > + * registration opcode out of the KVP opcode namespace. We will however,
> > + * maintain compatibility with the existing user-level daemon code.
> > + */
> > +
> > +/*
> > + * Daemon code not supporting IP injection (legacy daemon).
> > + */
> > +
> > +#define KVP_OP_REGISTER4
>
> Huh?
>
> > +/*
> > + * Daemon code supporting IP injection.
> > + * The KVP opcode field is used to communicate the
> > + * registration information; so define a namespace that
> > + * will be distinct from the host defined KVP opcode.
> > + */
> > +
> > +#define KVP_OP_REGISTER1 100
> > +
> > enum hv_kvp_exchg_op {
> > KVP_OP_GET = 0,
> > KVP_OP_SET,
> > KVP_OP_DELETE,
> > KVP_OP_ENUMERATE,
> > - KVP_OP_REGISTER,
> > + KVP_OP_GET_IP_INFO,
> > + KVP_OP_SET_IP_INFO,
>
> So you overloaded the command and somehow think that is ok? How is that
> supposed to work? Why not just always keep it there, but fail if it is
> called as you know you have a mismatch?
>
> Otherwise, again, you just broke older tools on a newer kernel.
>
> Or am I missing something here?
You are. The above enumeration is for the hypervisor-to-guest protocol,
whereas KVP_OP_REGISTER and KVP_OP_REGISTER1 are only used between
daemon and driver. The registration operation code should have been
defined as a sufficiently high value to avoid collision.
However, since the daemon will always send one of the registration
operations at start of day (and then never again), it seems that the
driver can avoid confusing registration with a reply to
KVP_OP_GET_IP_INFO. Instead, the two registration operation codes
distinguish the capabilities of the daemon and actually aid backward
compatibility.
Ben.
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
signature.asc
Description: This is a digitally signed message part
Re: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address injection
On Mon, Aug 13, 2012 at 10:06:51AM -0700, K. Y. Srinivasan wrote:
> Add the necessary definitions for supporting the IP injection functionality.
>
> Signed-off-by: K. Y. Srinivasan
> Reviewed-by: Haiyang Zhang
> Reviewed-by: Olaf Hering
> Reviewed-by: Ben Hutchings
> ---
> drivers/hv/hv_util.c |4 +-
> include/linux/hyperv.h | 76 -
> tools/hv/hv_kvp_daemon.c |2 +-
> 3 files changed, 77 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index d3ac6a4..a0667de 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -263,7 +263,7 @@ static int util_probe(struct hv_device *dev,
> (struct hv_util_service *)dev_id->driver_data;
> int ret;
>
> - srv->recv_buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
> + srv->recv_buffer = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
> if (!srv->recv_buffer)
> return -ENOMEM;
> if (srv->util_init) {
> @@ -274,7 +274,7 @@ static int util_probe(struct hv_device *dev,
> }
> }
>
> - ret = vmbus_open(dev->channel, 2 * PAGE_SIZE, 2 * PAGE_SIZE, NULL, 0,
> + ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL, 0,
> srv->util_cb, dev->channel);
> if (ret)
> goto error;
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 68ed7f7..11afc4e 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -122,12 +122,53 @@
> #define REG_U32 4
> #define REG_U64 8
>
> +/*
> + * As we look at expanding the KVP functionality to include
> + * IP injection functionality, we need to maintain binary
> + * compatibility with older daemons.
> + *
> + * The KVP opcodes are defined by the host and it was unfortunate
> + * that I chose to treat the registration operation as part of the
> + * KVP operations defined by the host.
> + * Here is the level of compatibility
> + * (between the user level daemon and the kernel KVP driver) that we
> + * will implement:
> + *
> + * An older daemon will always be supported on a newer driver.
> + * A given user level daemon will require a minimal version of the
> + * kernel driver.
> + * If we cannot handle the version differences, we will fail gracefully
> + * (this can happen when we have a user level daemon that is more
> + * advanced than the KVP driver.
> + *
> + * We will use values used in this handshake for determining if we have
> + * workable user level daemon and the kernel driver. We begin by taking the
> + * registration opcode out of the KVP opcode namespace. We will however,
> + * maintain compatibility with the existing user-level daemon code.
> + */
> +
> +/*
> + * Daemon code not supporting IP injection (legacy daemon).
> + */
> +
> +#define KVP_OP_REGISTER 4
Huh?
> +/*
> + * Daemon code supporting IP injection.
> + * The KVP opcode field is used to communicate the
> + * registration information; so define a namespace that
> + * will be distinct from the host defined KVP opcode.
> + */
> +
> +#define KVP_OP_REGISTER1 100
> +
> enum hv_kvp_exchg_op {
> KVP_OP_GET = 0,
> KVP_OP_SET,
> KVP_OP_DELETE,
> KVP_OP_ENUMERATE,
> - KVP_OP_REGISTER,
> + KVP_OP_GET_IP_INFO,
> + KVP_OP_SET_IP_INFO,
So you overloaded the command and somehow think that is ok? How is that
supposed to work? Why not just always keep it there, but fail if it is
called as you know you have a mismatch?
Otherwise, again, you just broke older tools on a newer kernel.
Or am I missing something here?
confused,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address injection
On Mon, Aug 13, 2012 at 10:06:51AM -0700, K. Y. Srinivasan wrote:
Add the necessary definitions for supporting the IP injection functionality.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Reviewed-by: Haiyang Zhang haiya...@microsoft.com
Reviewed-by: Olaf Hering o...@aepfle.de
Reviewed-by: Ben Hutchings b...@decadent.org.uk
---
drivers/hv/hv_util.c |4 +-
include/linux/hyperv.h | 76 -
tools/hv/hv_kvp_daemon.c |2 +-
3 files changed, 77 insertions(+), 5 deletions(-)
diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index d3ac6a4..a0667de 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -263,7 +263,7 @@ static int util_probe(struct hv_device *dev,
(struct hv_util_service *)dev_id-driver_data;
int ret;
- srv-recv_buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
+ srv-recv_buffer = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
if (!srv-recv_buffer)
return -ENOMEM;
if (srv-util_init) {
@@ -274,7 +274,7 @@ static int util_probe(struct hv_device *dev,
}
}
- ret = vmbus_open(dev-channel, 2 * PAGE_SIZE, 2 * PAGE_SIZE, NULL, 0,
+ ret = vmbus_open(dev-channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL, 0,
srv-util_cb, dev-channel);
if (ret)
goto error;
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 68ed7f7..11afc4e 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -122,12 +122,53 @@
#define REG_U32 4
#define REG_U64 8
+/*
+ * As we look at expanding the KVP functionality to include
+ * IP injection functionality, we need to maintain binary
+ * compatibility with older daemons.
+ *
+ * The KVP opcodes are defined by the host and it was unfortunate
+ * that I chose to treat the registration operation as part of the
+ * KVP operations defined by the host.
+ * Here is the level of compatibility
+ * (between the user level daemon and the kernel KVP driver) that we
+ * will implement:
+ *
+ * An older daemon will always be supported on a newer driver.
+ * A given user level daemon will require a minimal version of the
+ * kernel driver.
+ * If we cannot handle the version differences, we will fail gracefully
+ * (this can happen when we have a user level daemon that is more
+ * advanced than the KVP driver.
+ *
+ * We will use values used in this handshake for determining if we have
+ * workable user level daemon and the kernel driver. We begin by taking the
+ * registration opcode out of the KVP opcode namespace. We will however,
+ * maintain compatibility with the existing user-level daemon code.
+ */
+
+/*
+ * Daemon code not supporting IP injection (legacy daemon).
+ */
+
+#define KVP_OP_REGISTER 4
Huh?
+/*
+ * Daemon code supporting IP injection.
+ * The KVP opcode field is used to communicate the
+ * registration information; so define a namespace that
+ * will be distinct from the host defined KVP opcode.
+ */
+
+#define KVP_OP_REGISTER1 100
+
enum hv_kvp_exchg_op {
KVP_OP_GET = 0,
KVP_OP_SET,
KVP_OP_DELETE,
KVP_OP_ENUMERATE,
- KVP_OP_REGISTER,
+ KVP_OP_GET_IP_INFO,
+ KVP_OP_SET_IP_INFO,
So you overloaded the command and somehow think that is ok? How is that
supposed to work? Why not just always keep it there, but fail if it is
called as you know you have a mismatch?
Otherwise, again, you just broke older tools on a newer kernel.
Or am I missing something here?
confused,
greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address injection
On Mon, 2012-08-13 at 18:38 -0700, Greg KH wrote:
On Mon, Aug 13, 2012 at 10:06:51AM -0700, K. Y. Srinivasan wrote:
Add the necessary definitions for supporting the IP injection functionality.
[...]
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -122,12 +122,53 @@
#define REG_U32 4
#define REG_U64 8
+/*
+ * As we look at expanding the KVP functionality to include
+ * IP injection functionality, we need to maintain binary
+ * compatibility with older daemons.
+ *
+ * The KVP opcodes are defined by the host and it was unfortunate
+ * that I chose to treat the registration operation as part of the
+ * KVP operations defined by the host.
+ * Here is the level of compatibility
+ * (between the user level daemon and the kernel KVP driver) that we
+ * will implement:
+ *
+ * An older daemon will always be supported on a newer driver.
+ * A given user level daemon will require a minimal version of the
+ * kernel driver.
+ * If we cannot handle the version differences, we will fail gracefully
+ * (this can happen when we have a user level daemon that is more
+ * advanced than the KVP driver.
+ *
+ * We will use values used in this handshake for determining if we have
+ * workable user level daemon and the kernel driver. We begin by taking the
+ * registration opcode out of the KVP opcode namespace. We will however,
+ * maintain compatibility with the existing user-level daemon code.
+ */
+
+/*
+ * Daemon code not supporting IP injection (legacy daemon).
+ */
+
+#define KVP_OP_REGISTER4
Huh?
+/*
+ * Daemon code supporting IP injection.
+ * The KVP opcode field is used to communicate the
+ * registration information; so define a namespace that
+ * will be distinct from the host defined KVP opcode.
+ */
+
+#define KVP_OP_REGISTER1 100
+
enum hv_kvp_exchg_op {
KVP_OP_GET = 0,
KVP_OP_SET,
KVP_OP_DELETE,
KVP_OP_ENUMERATE,
- KVP_OP_REGISTER,
+ KVP_OP_GET_IP_INFO,
+ KVP_OP_SET_IP_INFO,
So you overloaded the command and somehow think that is ok? How is that
supposed to work? Why not just always keep it there, but fail if it is
called as you know you have a mismatch?
Otherwise, again, you just broke older tools on a newer kernel.
Or am I missing something here?
You are. The above enumeration is for the hypervisor-to-guest protocol,
whereas KVP_OP_REGISTER and KVP_OP_REGISTER1 are only used between
daemon and driver. The registration operation code should have been
defined as a sufficiently high value to avoid collision.
However, since the daemon will always send one of the registration
operations at start of day (and then never again), it seems that the
driver can avoid confusing registration with a reply to
KVP_OP_GET_IP_INFO. Instead, the two registration operation codes
distinguish the capabilities of the daemon and actually aid backward
compatibility.
Ben.
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
signature.asc
Description: This is a digitally signed message part
RE: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address injection
-Original Message-
From: Greg KH [mailto:gre...@linuxfoundation.org]
Sent: Monday, August 13, 2012 9:38 PM
To: KY Srinivasan
Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org;
virtualizat...@lists.osdl.org; o...@aepfle.de; a...@canonical.com;
b...@decadent.org.uk
Subject: Re: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address
injection
On Mon, Aug 13, 2012 at 10:06:51AM -0700, K. Y. Srinivasan wrote:
Add the necessary definitions for supporting the IP injection functionality.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Reviewed-by: Haiyang Zhang haiya...@microsoft.com
Reviewed-by: Olaf Hering o...@aepfle.de
Reviewed-by: Ben Hutchings b...@decadent.org.uk
---
drivers/hv/hv_util.c |4 +-
include/linux/hyperv.h | 76
-
tools/hv/hv_kvp_daemon.c |2 +-
3 files changed, 77 insertions(+), 5 deletions(-)
diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index d3ac6a4..a0667de 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -263,7 +263,7 @@ static int util_probe(struct hv_device *dev,
(struct hv_util_service *)dev_id-driver_data;
int ret;
- srv-recv_buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
+ srv-recv_buffer = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
if (!srv-recv_buffer)
return -ENOMEM;
if (srv-util_init) {
@@ -274,7 +274,7 @@ static int util_probe(struct hv_device *dev,
}
}
- ret = vmbus_open(dev-channel, 2 * PAGE_SIZE, 2 * PAGE_SIZE, NULL,
0,
+ ret = vmbus_open(dev-channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL,
0,
srv-util_cb, dev-channel);
if (ret)
goto error;
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 68ed7f7..11afc4e 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -122,12 +122,53 @@
#define REG_U32 4
#define REG_U64 8
+/*
+ * As we look at expanding the KVP functionality to include
+ * IP injection functionality, we need to maintain binary
+ * compatibility with older daemons.
+ *
+ * The KVP opcodes are defined by the host and it was unfortunate
+ * that I chose to treat the registration operation as part of the
+ * KVP operations defined by the host.
+ * Here is the level of compatibility
+ * (between the user level daemon and the kernel KVP driver) that we
+ * will implement:
+ *
+ * An older daemon will always be supported on a newer driver.
+ * A given user level daemon will require a minimal version of the
+ * kernel driver.
+ * If we cannot handle the version differences, we will fail gracefully
+ * (this can happen when we have a user level daemon that is more
+ * advanced than the KVP driver.
+ *
+ * We will use values used in this handshake for determining if we have
+ * workable user level daemon and the kernel driver. We begin by taking the
+ * registration opcode out of the KVP opcode namespace. We will however,
+ * maintain compatibility with the existing user-level daemon code.
+ */
+
+/*
+ * Daemon code not supporting IP injection (legacy daemon).
+ */
+
+#define KVP_OP_REGISTER4
Huh?
+/*
+ * Daemon code supporting IP injection.
+ * The KVP opcode field is used to communicate the
+ * registration information; so define a namespace that
+ * will be distinct from the host defined KVP opcode.
+ */
+
+#define KVP_OP_REGISTER1 100
+
enum hv_kvp_exchg_op {
KVP_OP_GET = 0,
KVP_OP_SET,
KVP_OP_DELETE,
KVP_OP_ENUMERATE,
- KVP_OP_REGISTER,
+ KVP_OP_GET_IP_INFO,
+ KVP_OP_SET_IP_INFO,
So you overloaded the command and somehow think that is ok? How is that
supposed to work? Why not just always keep it there, but fail if it is
called as you know you have a mismatch?
Otherwise, again, you just broke older tools on a newer kernel.
Or am I missing something here?
Greg,
The registration operation occurs when the daemon first starts up. I should
have established
a distinct namespace for the daemon versions that would not overlap with the
host
defined KVP operations initially. Unfortunately when I first implemented KVP, I
did not know
about the new KVP verbs and so selected a value that ended up colliding with
the new KVP
operations. To maintain compatibility with older daemons, I have to support
this old registration
value, which is what you are seeing here. Since the initial driver/daemon
handshake phase does
not overlap with the normal functioning of the KVP stack, we can use the old
daemon
registration value to distinguish that the daemon does not support IP
injection. The current
implementation does support a compatible environment for older daemons.
Regards,
K. Y
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More
