[Qemu-devel] [PATCH] virtio-net: Remove RX_EXTRA feature

[Qemu-devel] [PATCH] virtio-net: Remove RX_EXTRA feature

[Alex Williamson]

No need to use a feature bit to identify the RX modes.  The guest
will get an error back if the backend doesn't support these controls.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 hw/virtio-net.c |    3 +--
 hw/virtio-net.h |    8 ++++----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index d584287..fd5753a 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -122,8 +122,7 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev)
                         (1 << VIRTIO_NET_F_STATUS) |
                         (1 << VIRTIO_NET_F_CTRL_VQ) |
                         (1 << VIRTIO_NET_F_CTRL_RX) |
-                        (1 << VIRTIO_NET_F_CTRL_VLAN) |
-                        (1 << VIRTIO_NET_F_CTRL_RX_EXTRA);
+                        (1 << VIRTIO_NET_F_CTRL_VLAN);
 
     return features;
 }
diff --git a/hw/virtio-net.h b/hw/virtio-net.h
index 2085181..9bc4210 100644
--- a/hw/virtio-net.h
+++ b/hw/virtio-net.h
@@ -43,7 +43,6 @@
 #define VIRTIO_NET_F_CTRL_VQ    17      /* Control channel available */
 #define VIRTIO_NET_F_CTRL_RX    18      /* Control channel RX mode support */
 #define VIRTIO_NET_F_CTRL_VLAN  19      /* Control channel VLAN filtering */
-#define VIRTIO_NET_F_CTRL_RX_EXTRA 20   /* Extra RX mode control support */
 
 #define VIRTIO_NET_S_LINK_UP    1       /* Link is up */
 
@@ -106,9 +105,10 @@ typedef uint8_t virtio_net_ctrl_ack;
 /*
  * Control the RX mode, ie. promisucous, allmulti, etc...
  * All commands require an "out" sg entry containing a 1 byte
- * state value, zero = disable, non-zero = enable.  Commands
- * 0 and 1 are supported with the VIRTIO_NET_F_CTRL_RX feature.
- * Commands 2-5 are added with VIRTIO_NET_F_CTRL_RX_EXTRA.
+ * state value, zero = disable, non-zero = enable.  This group
+ * of commands are available with the VIRTIO_NET_F_CTRL_RX
+ * feature.  Individual commands may return VIRTIO_NET_ERR if
+ * unsupported by the host.
  */
 #define VIRTIO_NET_CTRL_RX_MODE    0
  #define VIRTIO_NET_CTRL_RX_MODE_PROMISC      0

Re: [Qemu-devel] [PATCH] virtio-net: Remove RX_EXTRA feature

[Michael S. Tsirkin]

On Fri, Jun 12, 2009 at 01:17:04PM -0600, Alex Williamson wrote:
> No need to use a feature bit to identify the RX modes.  The guest
> will get an error back if the backend doesn't support these controls.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>

What's the motivation here - are we running out of feature bits already?
Using feature bits for everything makes the interface more consistent
and IMO easier to use.


> ---
> 
>  hw/virtio-net.c |    3 +--
>  hw/virtio-net.h |    8 ++++----
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index d584287..fd5753a 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -122,8 +122,7 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev)
>                          (1 << VIRTIO_NET_F_STATUS) |
>                          (1 << VIRTIO_NET_F_CTRL_VQ) |
>                          (1 << VIRTIO_NET_F_CTRL_RX) |
> -                        (1 << VIRTIO_NET_F_CTRL_VLAN) |
> -                        (1 << VIRTIO_NET_F_CTRL_RX_EXTRA);
> +                        (1 << VIRTIO_NET_F_CTRL_VLAN);
>  
>      return features;
>  }
> diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> index 2085181..9bc4210 100644
> --- a/hw/virtio-net.h
> +++ b/hw/virtio-net.h
> @@ -43,7 +43,6 @@
>  #define VIRTIO_NET_F_CTRL_VQ    17      /* Control channel available */
>  #define VIRTIO_NET_F_CTRL_RX    18      /* Control channel RX mode support */
>  #define VIRTIO_NET_F_CTRL_VLAN  19      /* Control channel VLAN filtering */
> -#define VIRTIO_NET_F_CTRL_RX_EXTRA 20   /* Extra RX mode control support */
>  
>  #define VIRTIO_NET_S_LINK_UP    1       /* Link is up */
>  
> @@ -106,9 +105,10 @@ typedef uint8_t virtio_net_ctrl_ack;
>  /*
>   * Control the RX mode, ie. promisucous, allmulti, etc...
>   * All commands require an "out" sg entry containing a 1 byte
> - * state value, zero = disable, non-zero = enable.  Commands
> - * 0 and 1 are supported with the VIRTIO_NET_F_CTRL_RX feature.
> - * Commands 2-5 are added with VIRTIO_NET_F_CTRL_RX_EXTRA.
> + * state value, zero = disable, non-zero = enable.  This group
> + * of commands are available with the VIRTIO_NET_F_CTRL_RX
> + * feature.  Individual commands may return VIRTIO_NET_ERR if
> + * unsupported by the host.
>   */
>  #define VIRTIO_NET_CTRL_RX_MODE    0
>   #define VIRTIO_NET_CTRL_RX_MODE_PROMISC      0
> 
> 

Re: [Qemu-devel] [PATCH] virtio-net: Remove RX_EXTRA feature

[Alex Williamson]

On Sun, Jun 14, 2009 at 3:39 AM, Michael S. Tsirkin<mst@redhat.com> wrote:
> On Fri, Jun 12, 2009 at 01:17:04PM -0600, Alex Williamson wrote:
>> No need to use a feature bit to identify the RX modes.  The guest
>> will get an error back if the backend doesn't support these controls.
>>
>> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
>
> What's the motivation here - are we running out of feature bits already?
> Using feature bits for everything makes the interface more consistent
> and IMO easier to use.

See the thread starting here:

http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00519.html

Both Rusty and Mark voted against the feature bit.

Alex

Re: [Qemu-devel] [PATCH] virtio-net: Remove RX_EXTRA feature

[Rusty Russell]

On Sun, 14 Jun 2009 11:01:37 pm Alex Williamson wrote:
> On Sun, Jun 14, 2009 at 3:39 AM, Michael S. Tsirkin<mst@redhat.com> wrote:
> > On Fri, Jun 12, 2009 at 01:17:04PM -0600, Alex Williamson wrote:
> >> No need to use a feature bit to identify the RX modes.  The guest
> >> will get an error back if the backend doesn't support these controls.
> >>
> >> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> >
> > What's the motivation here - are we running out of feature bits already?
> > Using feature bits for everything makes the interface more consistent
> > and IMO easier to use.
>
> See the thread starting here:
>
> http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00519.html
>
> Both Rusty and Mark voted against the feature bit.

Not quite.  I was unclear.  I support the use of a feature bit.  My question 
is more meta.

Features do need justification.  (Not much, but some).  It's hard to see the 
use case where this feature offers significant advantages over promisc.  Without 
that, I'd wait until someone reports a need.

Thanks,
Rusty.

Re: [Qemu-devel] [PATCH] virtio-net: Remove RX_EXTRA feature

[Alex Williamson]

On Mon, 2009-06-15 at 14:48 +0930, Rusty Russell wrote:
> On Sun, 14 Jun 2009 11:01:37 pm Alex Williamson wrote:
> > On Sun, Jun 14, 2009 at 3:39 AM, Michael S. Tsirkin<mst@redhat.com> wrote:
> > > On Fri, Jun 12, 2009 at 01:17:04PM -0600, Alex Williamson wrote:
> > >> No need to use a feature bit to identify the RX modes.  The guest
> > >> will get an error back if the backend doesn't support these controls.
> > >>
> > >> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> > >
> > > What's the motivation here - are we running out of feature bits already?
> > > Using feature bits for everything makes the interface more consistent
> > > and IMO easier to use.
> >
> > See the thread starting here:
> >
> > http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00519.html
> >
> > Both Rusty and Mark voted against the feature bit.
> 
> Not quite.  I was unclear.  I support the use of a feature bit.  My question 
> is more meta.

Oh, ok.  Let's put this patch on hold then.

> Features do need justification.  (Not much, but some).  It's hard to see the 
> use case where this feature offers significant advantages over promisc.  Without 
> that, I'd wait until someone reports a need.

Thanks for clarifying, a need has been reported.  DMX is a proprietary
OS that intends to run on KVM and make use of virtio-net.  They would
like to be able to toggle unicast and multicast independently as well as
mask broadcast traffic.  The original series re-arranged some of the
filter code in the virtio-net backend that allows these filter options
to drop out nicely.  Thanks,

Alex

Re: [Qemu-devel] [PATCH] virtio-net: Remove RX_EXTRA feature

[Rusty Russell]

On Tue, 16 Jun 2009 12:15:44 am Alex Williamson wrote:
> On Mon, 2009-06-15 at 14:48 +0930, Rusty Russell wrote:
> >  It's hard to see
> > the use case where this feature offers significant advantages over
> > promisc.  Without that, I'd wait until someone reports a need.
>
> Thanks for clarifying, a need has been reported.  DMX is a proprietary
> OS that intends to run on KVM and make use of virtio-net.  They would
> like to be able to toggle unicast and multicast independently as well as
> mask broadcast traffic.  The original series re-arranged some of the
> filter code in the virtio-net backend that allows these filter options
> to drop out nicely.

That's sufficient for me.

Could you re-send feature-based patches if you're happy?
Rusty.

Re: [Qemu-devel] [PATCH] virtio-net: Remove RX_EXTRA feature

[Alex Williamson]

On Tue, 2009-06-16 at 12:49 +0930, Rusty Russell wrote:
> On Tue, 16 Jun 2009 12:15:44 am Alex Williamson wrote:
> > On Mon, 2009-06-15 at 14:48 +0930, Rusty Russell wrote:
> > >  It's hard to see
> > > the use case where this feature offers significant advantages over
> > > promisc.  Without that, I'd wait until someone reports a need.
> >
> > Thanks for clarifying, a need has been reported.  DMX is a proprietary
> > OS that intends to run on KVM and make use of virtio-net.  They would
> > like to be able to toggle unicast and multicast independently as well as
> > mask broadcast traffic.  The original series re-arranged some of the
> > filter code in the virtio-net backend that allows these filter options
> > to drop out nicely.
> 
> That's sufficient for me.

Great, this feature bit removal patch is official dead then.

> Could you re-send feature-based patches if you're happy?

The feature based version already went into the tree via Mark's patch
queue, but I'll bounce you copies in case you have any comments that we
can fixup in tree before we sync the Linux virtio-net header.  Thanks,

Alex