Patchwork [v4,for,2.0] virtio-net: add vlan receive state to RxFilterInfo

login
register
mail settings
Submitter Amos Kong
Date March 26, 2014, 12:19 a.m.
Message ID <1395793183-19894-1-git-send-email-akong@redhat.com>
Download mbox | patch
Permalink /patch/333753/
State New
Headers show

Comments

Amos Kong - March 26, 2014, 12:19 a.m.
Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't
filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't negotiated.

This patch added a new field to @RxFilterInfo to indicate vlan receive
state ('normal', 'none', 'all'). If VIRTIO_NET_F_CTRL_VLAN isn't
negotiated, vlan receive state will be 'all', then all VLAN-tagged packets
will be received by guest.

This patch also fixed a boundary issue in visiting vlan table.

[1] http://lists.nongnu.org/archive/html/qemu-devel/2014-02/msg02604.html

Signed-off-by: Amos Kong <akong@redhat.com>
---
V2: don't make vlan-table optional, add a flag to indicate
    if vlan table is used by management
V3: change the new filed to RxState (mst)
V4: fix boundary of vlan mapping (eric)
---
 hw/net/virtio-net.c | 42 +++++++++++++++++++++++++++++-------------
 qapi-schema.json    |  3 +++
 qmp-commands.hx     |  2 ++
 3 files changed, 34 insertions(+), 13 deletions(-)
Eric Blake - March 26, 2014, 1:51 a.m.
On 03/25/2014 06:19 PM, Amos Kong wrote:
> Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't
> filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't negotiated.
> 
> This patch added a new field to @RxFilterInfo to indicate vlan receive
> state ('normal', 'none', 'all'). If VIRTIO_NET_F_CTRL_VLAN isn't
> negotiated, vlan receive state will be 'all', then all VLAN-tagged packets
> will be received by guest.
> 
> This patch also fixed a boundary issue in visiting vlan table.

The use of "also" means this might have been better as two patches (bug
fix separate from qapi addition).  But as we are getting close to 2.0,
I'm okay if you add:

> 
> [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-02/msg02604.html
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>
Michael S. Tsirkin - March 26, 2014, 6:46 a.m.
On Wed, Mar 26, 2014 at 08:19:43AM +0800, Amos Kong wrote:
> Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't
> filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't negotiated.

Yes but that fix is unfortunately wrong as it tests guest_features
on reset.
How about preparing a correct one?


> This patch added a new field to @RxFilterInfo to indicate vlan receive
> state ('normal', 'none', 'all'). If VIRTIO_NET_F_CTRL_VLAN isn't
> negotiated, vlan receive state will be 'all', then all VLAN-tagged packets
> will be received by guest.
> 
> This patch also fixed a boundary issue in visiting vlan table.
> 
> [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-02/msg02604.html
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
> V2: don't make vlan-table optional, add a flag to indicate
>     if vlan table is used by management
> V3: change the new filed to RxState (mst)
> V4: fix boundary of vlan mapping (eric)
> ---
>  hw/net/virtio-net.c | 42 +++++++++++++++++++++++++++++-------------
>  qapi-schema.json    |  3 +++
>  qmp-commands.hx     |  2 ++
>  3 files changed, 34 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index fd23c46..43b4eda 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -222,13 +222,33 @@ static char *mac_strdup_printf(const uint8_t *mac)
>                              mac[1], mac[2], mac[3], mac[4], mac[5]);
>  }
>  
> +static intList *get_vlan_table(VirtIONet *n)
> +{
> +    intList *list, *entry;
> +    int i, j;
> +
> +    list = NULL;
> +    for (i = 0; i < MAX_VLAN >> 5; i++) {
> +        for (j = 0; n->vlans[i] && j <= 0x1f; j++) {
> +            if (n->vlans[i] & (1U << j)) {
> +                entry = g_malloc0(sizeof(*entry));
> +                entry->value = (i << 5) + j;
> +                entry->next = list;
> +                list = entry;
> +            }
> +        }
> +    }
> +
> +    return list;
> +}
> +
>  static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
>  {
>      VirtIONet *n = qemu_get_nic_opaque(nc);
> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>      RxFilterInfo *info;
>      strList *str_list, *entry;
> -    intList *int_list, *int_entry;
> -    int i, j;
> +    int i;
>  
>      info = g_malloc0(sizeof(*info));
>      info->name = g_strdup(nc->name);
> @@ -273,19 +293,15 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
>          str_list = entry;
>      }
>      info->multicast_table = str_list;
> +    info->vlan_table = get_vlan_table(n);
>  
> -    int_list = NULL;
> -    for (i = 0; i < MAX_VLAN >> 5; i++) {
> -        for (j = 0; n->vlans[i] && j < 0x1f; j++) {
> -            if (n->vlans[i] & (1U << j)) {
> -                int_entry = g_malloc0(sizeof(*int_entry));
> -                int_entry->value = (i << 5) + j;
> -                int_entry->next = int_list;
> -                int_list = int_entry;
> -            }
> -        }
> +    if (!((1 << VIRTIO_NET_F_CTRL_VLAN) & vdev->guest_features)) {
> +        info->vlan = RX_STATE_ALL;
> +    } else if (!info->vlan_table) {
> +        info->vlan = RX_STATE_NONE;
> +    } else {
> +        info->vlan = RX_STATE_NORMAL;
>      }

Generally I'm not sure why do we have NONE - for mac
and unicast too - we could send an empty list instead.

I'm fine with keeping it as is for now though.

> -    info->vlan_table = int_list;
>  
>      /* enable event notification after query */
>      nc->rxfilter_notify_enabled = 1;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index b68cd44..391356f 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4184,6 +4184,8 @@
>  #
>  # @unicast: unicast receive state
>  #
> +# @vlan: vlan receive state (Since 2.0)
> +#
>  # @broadcast-allowed: whether to receive broadcast
>  #
>  # @multicast-overflow: multicast table is overflowed or not
> @@ -4207,6 +4209,7 @@
>      'promiscuous':        'bool',
>      'multicast':          'RxState',
>      'unicast':            'RxState',
> +    'vlan':               'RxState',
>      'broadcast-allowed':  'bool',
>      'multicast-overflow': 'bool',
>      'unicast-overflow':   'bool',
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index a22621f..ed3ab92 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3407,6 +3407,7 @@ Each array entry contains the following:
>  - "promiscuous": promiscuous mode is enabled (json-bool)
>  - "multicast": multicast receive state (one of 'normal', 'none', 'all')
>  - "unicast": unicast receive state  (one of 'normal', 'none', 'all')
> +- "vlan": vlan receive state (one of 'normal', 'none', 'all') (Since 2.0)
>  - "broadcast-allowed": allow to receive broadcast (json-bool)
>  - "multicast-overflow": multicast table is overflowed (json-bool)
>  - "unicast-overflow": unicast table is overflowed (json-bool)
> @@ -3424,6 +3425,7 @@ Example:
>              "name": "vnet0",
>              "main-mac": "52:54:00:12:34:56",
>              "unicast": "normal",
> +            "vlan": "normal",
>              "vlan-table": [
>                  4,
>                  0
> -- 
> 1.8.5.3
Amos Kong - March 26, 2014, 10:36 a.m.
On Wed, Mar 26, 2014 at 08:46:35AM +0200, Michael S. Tsirkin wrote:
> On Wed, Mar 26, 2014 at 08:19:43AM +0800, Amos Kong wrote:
> > Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't
> > filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't negotiated.
> 
> Yes but that fix is unfortunately wrong as it tests guest_features
> on reset.
> How about preparing a correct one?
 
I just sent a v2:
[PATCH v2 for 2.0] virtio-net: Do not filter VLANs without F_CTRL_VLAN
 
> > This patch added a new field to @RxFilterInfo to indicate vlan receive
> > state ('normal', 'none', 'all'). If VIRTIO_NET_F_CTRL_VLAN isn't
> > negotiated, vlan receive state will be 'all', then all VLAN-tagged packets
> > will be received by guest.
> > 
> > This patch also fixed a boundary issue in visiting vlan table.
> > 
> > [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-02/msg02604.html
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> > V2: don't make vlan-table optional, add a flag to indicate
> >     if vlan table is used by management
> > V3: change the new filed to RxState (mst)
> > V4: fix boundary of vlan mapping (eric)

...

> > +    if (!((1 << VIRTIO_NET_F_CTRL_VLAN) & vdev->guest_features)) {
> > +        info->vlan = RX_STATE_ALL;
> > +    } else if (!info->vlan_table) {
> > +        info->vlan = RX_STATE_NONE;
> > +    } else {
> > +        info->vlan = RX_STATE_NORMAL;
> >      }
> 
> Generally I'm not sure why do we have NONE - for mac
> and unicast too - we could send an empty list instead.
> 
> I'm fine with keeping it as is for now though.

It's only be used at the beginning of init stage, guest driver hasn't
change vlan table. state will be normal if default '0' is added to
vlan table.

> > -    info->vlan_table = int_list;
> >  
> >      /* enable event notification after query */
> >      nc->rxfilter_notify_enabled = 1;
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index b68cd44..391356f 100644

Thanks, Amos

Patch

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index fd23c46..43b4eda 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -222,13 +222,33 @@  static char *mac_strdup_printf(const uint8_t *mac)
                             mac[1], mac[2], mac[3], mac[4], mac[5]);
 }
 
+static intList *get_vlan_table(VirtIONet *n)
+{
+    intList *list, *entry;
+    int i, j;
+
+    list = NULL;
+    for (i = 0; i < MAX_VLAN >> 5; i++) {
+        for (j = 0; n->vlans[i] && j <= 0x1f; j++) {
+            if (n->vlans[i] & (1U << j)) {
+                entry = g_malloc0(sizeof(*entry));
+                entry->value = (i << 5) + j;
+                entry->next = list;
+                list = entry;
+            }
+        }
+    }
+
+    return list;
+}
+
 static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
 {
     VirtIONet *n = qemu_get_nic_opaque(nc);
+    VirtIODevice *vdev = VIRTIO_DEVICE(n);
     RxFilterInfo *info;
     strList *str_list, *entry;
-    intList *int_list, *int_entry;
-    int i, j;
+    int i;
 
     info = g_malloc0(sizeof(*info));
     info->name = g_strdup(nc->name);
@@ -273,19 +293,15 @@  static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
         str_list = entry;
     }
     info->multicast_table = str_list;
+    info->vlan_table = get_vlan_table(n);
 
-    int_list = NULL;
-    for (i = 0; i < MAX_VLAN >> 5; i++) {
-        for (j = 0; n->vlans[i] && j < 0x1f; j++) {
-            if (n->vlans[i] & (1U << j)) {
-                int_entry = g_malloc0(sizeof(*int_entry));
-                int_entry->value = (i << 5) + j;
-                int_entry->next = int_list;
-                int_list = int_entry;
-            }
-        }
+    if (!((1 << VIRTIO_NET_F_CTRL_VLAN) & vdev->guest_features)) {
+        info->vlan = RX_STATE_ALL;
+    } else if (!info->vlan_table) {
+        info->vlan = RX_STATE_NONE;
+    } else {
+        info->vlan = RX_STATE_NORMAL;
     }
-    info->vlan_table = int_list;
 
     /* enable event notification after query */
     nc->rxfilter_notify_enabled = 1;
diff --git a/qapi-schema.json b/qapi-schema.json
index b68cd44..391356f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4184,6 +4184,8 @@ 
 #
 # @unicast: unicast receive state
 #
+# @vlan: vlan receive state (Since 2.0)
+#
 # @broadcast-allowed: whether to receive broadcast
 #
 # @multicast-overflow: multicast table is overflowed or not
@@ -4207,6 +4209,7 @@ 
     'promiscuous':        'bool',
     'multicast':          'RxState',
     'unicast':            'RxState',
+    'vlan':               'RxState',
     'broadcast-allowed':  'bool',
     'multicast-overflow': 'bool',
     'unicast-overflow':   'bool',
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a22621f..ed3ab92 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3407,6 +3407,7 @@  Each array entry contains the following:
 - "promiscuous": promiscuous mode is enabled (json-bool)
 - "multicast": multicast receive state (one of 'normal', 'none', 'all')
 - "unicast": unicast receive state  (one of 'normal', 'none', 'all')
+- "vlan": vlan receive state (one of 'normal', 'none', 'all') (Since 2.0)
 - "broadcast-allowed": allow to receive broadcast (json-bool)
 - "multicast-overflow": multicast table is overflowed (json-bool)
 - "unicast-overflow": unicast table is overflowed (json-bool)
@@ -3424,6 +3425,7 @@  Example:
             "name": "vnet0",
             "main-mac": "52:54:00:12:34:56",
             "unicast": "normal",
+            "vlan": "normal",
             "vlan-table": [
                 4,
                 0