| Message ID | 1395793183-19894-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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
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
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
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(-)