[Qemu-devel] virtio-net VLAN filtering bug

[Qemu-devel] virtio-net VLAN filtering bug

[Stefan Fritsch]

Hi,

if the feature VIRTIO_NET_F_CTRL_VLAN is not negotiated, virtio-net should 
not filter VLANs. That means it should send all packets to the guest 
instead of dropping all packets that have any VLAN id. The following patch 
fixes the issue.

Cheers,
Stefan


--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -312,13 +312,16 @@ static void virtio_net_reset(VirtIODevice *vdev)
     n->mac_table.first_multi = 0;
     n->mac_table.multi_overflow = 0;
     n->mac_table.uni_overflow = 0;
     memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
     memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
     qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
-    memset(n->vlans, 0, MAX_VLAN >> 3);
+    if (vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN))
+        memset(n->vlans, 0, MAX_VLAN >> 3);
+    else
+        memset(n->vlans, 0xff, MAX_VLAN >> 3);
 }
 
 static void peer_test_vnet_hdr(VirtIONet *n)
 {
     NetClientState *nc = qemu_get_queue(n->nic);
     if (!nc->peer) {
@@ -512,12 +515,17 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
         }
         if (!tap_get_vhost_net(nc->peer)) {
             continue;
         }
         vhost_net_ack_features(tap_get_vhost_net(nc->peer), features);
     }
+
+    if (vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN))
+        memset(n->vlans, 0, MAX_VLAN >> 3);
+    else
+        memset(n->vlans, 0xff, MAX_VLAN >> 3);
 }
 
 static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
                                      struct iovec *iov, unsigned int iov_cnt)
 {
     uint8_t on;

[Qemu-devel] [PATCH] virtio-net: Do not filter VLANs without F_CTRL_VLAN

[Stefan Fritsch]

If VIRTIO_NET_F_CTRL_VLAN is not negotiated, do not filter out all
VLAN-tagged packets but send them to the guest.

Signed-off-by: Stefan Fritsch <sf@sfritsch.de>
---

This time CCing the maintainers.

This fixes VLANs with OpenBSD guests (and probably NetBSD, too, because
the OpenBSD driver started as a port from NetBSD).


 hw/net/virtio-net.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3626608..0ae9a91 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -315,7 +315,11 @@ static void virtio_net_reset(VirtIODevice *vdev)
     memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
     memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
     qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
-    memset(n->vlans, 0, MAX_VLAN >> 3);
+    if (vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) {
+        memset(n->vlans, 0, MAX_VLAN >> 3);
+    } else {
+        memset(n->vlans, 0xff, MAX_VLAN >> 3);
+    }
 }
 
 static void peer_test_vnet_hdr(VirtIONet *n)
@@ -515,6 +519,12 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
         }
         vhost_net_ack_features(tap_get_vhost_net(nc->peer), features);
     }
+
+    if (vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) {
+        memset(n->vlans, 0, MAX_VLAN >> 3);
+    } else {
+        memset(n->vlans, 0xff, MAX_VLAN >> 3);
+    }
 }
 
 static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] virtio-net: Do not filter VLANs without F_CTRL_VLAN

[Michael S. Tsirkin]

On Wed, Feb 12, 2014 at 10:46:28PM +0100, Stefan Fritsch wrote:
> If VIRTIO_NET_F_CTRL_VLAN is not negotiated, do not filter out all
> VLAN-tagged packets but send them to the guest.
> 
> Signed-off-by: Stefan Fritsch <sf@sfritsch.de>

Thanks for the patch.
I think there are still some issues after this
patch: we need to notify management when
this bit state changes.
And I think libvirt still does not look at the filter info
so it's probably not too late, and cleaner to simply tell it:
"all-vlans".
that is, add
    '*vlan':          'RxState',
to the schema.

(is it true that it needs to be * because old qemu does not produce it?
 maybe not ...)

Taking all this into account - this calls for checking
this bit in receive_filter like we do for e.g.
unicast addresses.

Amos, you wrote
commit b1be42803b31a913bab65bab563a8760ad2e7f7f
Author: Amos Kong <akong@redhat.com>
Date:   Fri Jun 14 15:45:52 2013 +0800

    net: add support of mac-programming over macvtap in QEMU side
which conflicts here - could you take a look please?

Also Cc schema maintainers.

> ---
> 
> This time CCing the maintainers.
> 
> This fixes VLANs with OpenBSD guests (and probably NetBSD, too, because
> the OpenBSD driver started as a port from NetBSD).
> 
> 
>  hw/net/virtio-net.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 3626608..0ae9a91 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -315,7 +315,11 @@ static void virtio_net_reset(VirtIODevice *vdev)
>      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
>      memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
>      qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> -    memset(n->vlans, 0, MAX_VLAN >> 3);
> +    if (vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) {
> +        memset(n->vlans, 0, MAX_VLAN >> 3);
> +    } else {
> +        memset(n->vlans, 0xff, MAX_VLAN >> 3);
> +    }
>  }
>  
>  static void peer_test_vnet_hdr(VirtIONet *n)

This chunk doesn't make sense to me.
features are never set at reset, are they?

> @@ -515,6 +519,12 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
>          }
>          vhost_net_ack_features(tap_get_vhost_net(nc->peer), features);
>      }
> +
> +    if (vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) {
> +        memset(n->vlans, 0, MAX_VLAN >> 3);
> +    } else {
> +        memset(n->vlans, 0xff, MAX_VLAN >> 3);
> +    }
>  }
>  
>  static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> -- 
> 1.7.10.4

Re: [Qemu-devel] [PATCH] virtio-net: Do not filter VLANs without F_CTRL_VLAN

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1832 bytes --]

On 02/16/2014 03:33 AM, Michael S. Tsirkin wrote:
> On Wed, Feb 12, 2014 at 10:46:28PM +0100, Stefan Fritsch wrote:
>> If VIRTIO_NET_F_CTRL_VLAN is not negotiated, do not filter out all
>> VLAN-tagged packets but send them to the guest.
>>
>> Signed-off-by: Stefan Fritsch <sf@sfritsch.de>
> 
> Thanks for the patch.
> I think there are still some issues after this
> patch: we need to notify management when
> this bit state changes.
> And I think libvirt still does not look at the filter info
> so it's probably not too late, and cleaner to simply tell it:
> "all-vlans".
> that is, add
>     '*vlan':          'RxState',
> to the schema.
> 
> (is it true that it needs to be * because old qemu does not produce it?
>  maybe not ...)

The following conversions are back-compat safe:

changing an input parameter from mandatory to optional
adding an optional input parameter
changing an output parameter from optional to mandatory
adding an output parameter (optional or always-present)

Regarding whether a parameter must be marked as optional: on input,
optional means the user can omit it.  On output, marking it as optional
means the current version of qemu will sometimes generate it, and
sometimes avoid it.  If qemu always generates it in the current version,
even if it was not generated in an previous version, then an output
parameter does not have to be marked optional (that is, behavior of
previous qemu versions should not impact the choice of whether the
current qemu marks an output as optional - only whether the current qemu
sometimes omits the parameter).  However, once an output parameter is
marked mandatory, you must never remove it in future qemu versions.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] virtio-net: Do not filter VLANs without F_CTRL_VLAN

[Stefan Fritsch]

On Sun, 16 Feb 2014, Michael S. Tsirkin wrote:

> On Wed, Feb 12, 2014 at 10:46:28PM +0100, Stefan Fritsch wrote:
> > If VIRTIO_NET_F_CTRL_VLAN is not negotiated, do not filter out all
> > VLAN-tagged packets but send them to the guest.
> > 
> > Signed-off-by: Stefan Fritsch <sf@sfritsch.de>
> 
> Thanks for the patch.
> I think there are still some issues after this
> patch: we need to notify management when
> this bit state changes.
> And I think libvirt still does not look at the filter info
> so it's probably not too late, and cleaner to simply tell it:
> "all-vlans".
> that is, add
>     '*vlan':          'RxState',
> to the schema.

I am not familiar with these interfaces and can't comment on that.

> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 3626608..0ae9a91 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -315,7 +315,11 @@ static void virtio_net_reset(VirtIODevice *vdev)
> >      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
> >      memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
> >      qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> > -    memset(n->vlans, 0, MAX_VLAN >> 3);
> > +    if (vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) {
> > +        memset(n->vlans, 0, MAX_VLAN >> 3);
> > +    } else {
> > +        memset(n->vlans, 0xff, MAX_VLAN >> 3);
> > +    }
> >  }
> >  
> >  static void peer_test_vnet_hdr(VirtIONet *n)
> 
> This chunk doesn't make sense to me.
> features are never set at reset, are they?

You are right. This chunk is not necessary. I have checked that the second 
chunk alone fixes the issue.

> > @@ -515,6 +519,12 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
> >          }
> >          vhost_net_ack_features(tap_get_vhost_net(nc->peer), features);
> >      }
> > +
> > +    if (vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) {
> > +        memset(n->vlans, 0, MAX_VLAN >> 3);
> > +    } else {
> > +        memset(n->vlans, 0xff, MAX_VLAN >> 3);
> > +    }
> >  }
> >  
> >  static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> > -- 
> > 1.7.10.4
> 

Cheers,
Stefan

Re: [Qemu-devel] [PATCH] virtio-net: Do not filter VLANs without F_CTRL_VLAN

[Amos Kong]

On Wed, Feb 12, 2014 at 10:46:28PM +0100, Stefan Fritsch wrote:
> If VIRTIO_NET_F_CTRL_VLAN is not negotiated, do not filter out all
> VLAN-tagged packets but send them to the guest.

Can we just update receive_filter() to filter out VLAN-tagged packets
only when VIRTIO_NET_F_CTRL_VLAN is negotiated?

@@ -913,7 +940,8 @@ static int receive_filter(VirtIONet
*n, const uint8_t *buf, int size)
 
     if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
         int vid = be16_to_cpup((uint16_t *)(ptr + 14)) & 0xfff;
-        if (!(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
+        if ((vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) &&
+            !(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
             return 0;
     }
 
> Signed-off-by: Stefan Fritsch <sf@sfritsch.de>
> ---
> 
> This time CCing the maintainers.
> 
> This fixes VLANs with OpenBSD guests (and probably NetBSD, too, because
> the OpenBSD driver started as a port from NetBSD).
> 
> 
>  hw/net/virtio-net.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 3626608..0ae9a91 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -315,7 +315,11 @@ static void virtio_net_reset(VirtIODevice *vdev)
>      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
>      memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
>      qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> -    memset(n->vlans, 0, MAX_VLAN >> 3);
> +    if (vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) {
> +        memset(n->vlans, 0, MAX_VLAN >> 3);
> +    } else {
> +        memset(n->vlans, 0xff, MAX_VLAN >> 3);
> +    }
>  }
>  
>  static void peer_test_vnet_hdr(VirtIONet *n)
> @@ -515,6 +519,12 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
>          }
>          vhost_net_ack_features(tap_get_vhost_net(nc->peer), features);
>      }
> +
> +    if (vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) {
> +        memset(n->vlans, 0, MAX_VLAN >> 3);
> +    } else {
> +        memset(n->vlans, 0xff, MAX_VLAN >> 3);
> +    }
>  }
>  
>  static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> -- 
> 1.7.10.4

-- 
			Amos.

Re: [Qemu-devel] [PATCH] virtio-net: Do not filter VLANs without F_CTRL_VLAN

[Stefan Fritsch]

On Fri, 21 Feb 2014, Amos Kong wrote:

> On Wed, Feb 12, 2014 at 10:46:28PM +0100, Stefan Fritsch wrote:
> > If VIRTIO_NET_F_CTRL_VLAN is not negotiated, do not filter out all
> > VLAN-tagged packets but send them to the guest.
> 
> Can we just update receive_filter() to filter out VLAN-tagged packets
> only when VIRTIO_NET_F_CTRL_VLAN is negotiated?

We could. But this adds a (very small) per-packet overhead while my patch 
only adds overhead during reset. Therefore I didn't take that approach. 
But if changing receive_filter() makes management much easier, that could 
be acceptable.

> 
> @@ -913,7 +940,8 @@ static int receive_filter(VirtIONet
> *n, const uint8_t *buf, int size)
>  
>      if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
>          int vid = be16_to_cpup((uint16_t *)(ptr + 14)) & 0xfff;
> -        if (!(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
> +        if ((vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) &&
> +            !(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
>              return 0;
>      }
>  
> > Signed-off-by: Stefan Fritsch <sf@sfritsch.de>
> > ---
> > 
> > This time CCing the maintainers.
> > 
> > This fixes VLANs with OpenBSD guests (and probably NetBSD, too, because
> > the OpenBSD driver started as a port from NetBSD).
> > 
> > 
> >  hw/net/virtio-net.c |   12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 3626608..0ae9a91 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -315,7 +315,11 @@ static void virtio_net_reset(VirtIODevice *vdev)
> >      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
> >      memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
> >      qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> > -    memset(n->vlans, 0, MAX_VLAN >> 3);
> > +    if (vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) {
> > +        memset(n->vlans, 0, MAX_VLAN >> 3);
> > +    } else {
> > +        memset(n->vlans, 0xff, MAX_VLAN >> 3);
> > +    }
> >  }
> >  
> >  static void peer_test_vnet_hdr(VirtIONet *n)
> > @@ -515,6 +519,12 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
> >          }
> >          vhost_net_ack_features(tap_get_vhost_net(nc->peer), features);
> >      }
> > +
> > +    if (vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) {
> > +        memset(n->vlans, 0, MAX_VLAN >> 3);
> > +    } else {
> > +        memset(n->vlans, 0xff, MAX_VLAN >> 3);
> > +    }
> >  }
> >  
> >  static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> > -- 
> > 1.7.10.4
> 
> -- 
> 			Amos.
> 

Re: [Qemu-devel] [PATCH] virtio-net: Do not filter VLANs without F_CTRL_VLAN

[Amos Kong]

On Sun, Feb 23, 2014 at 09:27:33AM +0100, Stefan Fritsch wrote:
> On Fri, 21 Feb 2014, Amos Kong wrote:
> 
> > On Wed, Feb 12, 2014 at 10:46:28PM +0100, Stefan Fritsch wrote:
> > > If VIRTIO_NET_F_CTRL_VLAN is not negotiated, do not filter out all
> > > VLAN-tagged packets but send them to the guest.
> > 
> > Can we just update receive_filter() to filter out VLAN-tagged packets
> > only when VIRTIO_NET_F_CTRL_VLAN is negotiated?

If we change receive_filter(), we also need a flag to indicate
management this feature isn't negotiated, management will do some
additional operation to host device to get same effect.
 
> We could. But this adds a (very small) per-packet overhead while my patch 
> only adds overhead during reset. Therefore I didn't take that approach. 
> But if changing receive_filter() makes management much easier, that could 
> be acceptable.

Actually your solution is better, QEMU will return a long list
[0,1,2,...4095] to management, host device will filter all the vlan
packets and send to QEMU.

So the problem raised by mst doesn't exist.

Thanks, Amos

> > @@ -913,7 +940,8 @@ static int receive_filter(VirtIONet
> > *n, const uint8_t *buf, int size)
> >  
> >      if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
> >          int vid = be16_to_cpup((uint16_t *)(ptr + 14)) & 0xfff;
> > -        if (!(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
> > +        if ((vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) &&
> > +            !(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
> >              return 0;
> >      }
> >  
> > > Signed-off-by: Stefan Fritsch <sf@sfritsch.de>
> > > ---
> > > 
> > > This time CCing the maintainers.
> > > 
> > > This fixes VLANs with OpenBSD guests (and probably NetBSD, too, because
> > > the OpenBSD driver started as a port from NetBSD).
> > > 
> > > 
> > >  hw/net/virtio-net.c |   12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index 3626608..0ae9a91 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -315,7 +315,11 @@ static void virtio_net_reset(VirtIODevice *vdev)
> > >      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
> > >      memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
> > >      qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> > > -    memset(n->vlans, 0, MAX_VLAN >> 3);
> > > +    if (vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) {
> > > +        memset(n->vlans, 0, MAX_VLAN >> 3);
> > > +    } else {
> > > +        memset(n->vlans, 0xff, MAX_VLAN >> 3);
> > > +    }
> > >  }
> > >  
> > >  static void peer_test_vnet_hdr(VirtIONet *n)
> > > @@ -515,6 +519,12 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
> > >          }
> > >          vhost_net_ack_features(tap_get_vhost_net(nc->peer), features);
> > >      }
> > > +
> > > +    if (vdev->guest_features & (1 << VIRTIO_NET_F_CTRL_VLAN)) {
> > > +        memset(n->vlans, 0, MAX_VLAN >> 3);
> > > +    } else {
> > > +        memset(n->vlans, 0xff, MAX_VLAN >> 3);
> > > +    }
> > >  }
> > >  
> > >  static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> > > -- 
> > > 1.7.10.4
> > 
> > -- 
> > 			Amos.
> > 

-- 
			Amos.

Re: [Qemu-devel] [PATCH] virtio-net: Do not filter VLANs without F_CTRL_VLAN

[Stefan Fritsch]

Hi,

Am Dienstag, 25. Februar 2014, 11:06:33 schrieb Amos Kong:
> > > On Wed, Feb 12, 2014 at 10:46:28PM +0100, Stefan Fritsch wrote:
> > > > If VIRTIO_NET_F_CTRL_VLAN is not negotiated, do not filter out
> > > > all
> > > > VLAN-tagged packets but send them to the guest.


AFAICS, no fix has been committed, yet. Is there anything I need to do 
to get this fixed?


> > > Can we just update receive_filter() to filter out VLAN-tagged
> > > packets only when VIRTIO_NET_F_CTRL_VLAN is negotiated?
> 
> If we change receive_filter(), we also need a flag to indicate
> management this feature isn't negotiated, management will do some
> additional operation to host device to get same effect.
>  
> 
> > We could. But this adds a (very small) per-packet overhead while
> > my patch  only adds overhead during reset. Therefore I didn't
> > take that approach. But if changing receive_filter() makes
> > management much easier, that could be acceptable.
> 
> Actually your solution is better, QEMU will return a long list
> [0,1,2,...4095] to management, host device will filter all the vlan
> packets and send to QEMU.
> 
> So the problem raised by mst doesn't exist.

Cheers,
Stefan

Re: [Qemu-devel] [PATCH] virtio-net: Do not filter VLANs without F_CTRL_VLAN

[Amos Kong]

On Wed, Mar 19, 2014 at 11:38:57PM +0100, Stefan Fritsch wrote:
> Hi,
> 
> Am Dienstag, 25. Februar 2014, 11:06:33 schrieb Amos Kong:
> > > > On Wed, Feb 12, 2014 at 10:46:28PM +0100, Stefan Fritsch wrote:
> > > > > If VIRTIO_NET_F_CTRL_VLAN is not negotiated, do not filter out
> > > > > all
> > > > > VLAN-tagged packets but send them to the guest.
> 
> 
> AFAICS, no fix has been committed, yet. Is there anything I need to do 
> to get this fixed?
 
Michael asked in IRC to continually add a RxState for vlan in
RxFilter notification through QMP events.

(sorry I didn't talk too much becaused I'm in meeting)

I mentioned in [1] [2], we don't need to add this new state
if we apply patch Stefan's patch[3]

Michael said Stefan's fix is wrong, but the problem exists.
The problem is the bug fixed by [3], not the problem that was
mentioned in [4] 


Michael, what's wrong with Stefan's patch?


Thanks, Amos

[1] http://lists.gnu.org/archive/html/qemu-devel/2014-02/msg03835.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg04434.html
[3] virtio-net: Do not filter VLANs without F_CTRL_VLAN
[4] https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg02604.html


> > > > Can we just update receive_filter() to filter out VLAN-tagged
> > > > packets only when VIRTIO_NET_F_CTRL_VLAN is negotiated?
> > 
> > If we change receive_filter(), we also need a flag to indicate
> > management this feature isn't negotiated, management will do some
> > additional operation to host device to get same effect.
> >  
> > 
> > > We could. But this adds a (very small) per-packet overhead while
> > > my patch  only adds overhead during reset. Therefore I didn't
> > > take that approach. But if changing receive_filter() makes
> > > management much easier, that could be acceptable.
> > 
> > Actually your solution is better, QEMU will return a long list
> > [0,1,2,...4095] to management, host device will filter all the vlan
> > packets and send to QEMU.
> > 
> > So the problem raised by mst doesn't exist.
> 
> Cheers,
> Stefan
> 

-- 
			Amos.

Re: [Qemu-devel] [PATCH] virtio-net: Do not filter VLANs without F_CTRL_VLAN

[Michael S. Tsirkin]

On Tue, Mar 25, 2014 at 05:39:12PM +0800, Amos Kong wrote:
> On Wed, Mar 19, 2014 at 11:38:57PM +0100, Stefan Fritsch wrote:
> > Hi,
> > 
> > Am Dienstag, 25. Februar 2014, 11:06:33 schrieb Amos Kong:
> > > > > On Wed, Feb 12, 2014 at 10:46:28PM +0100, Stefan Fritsch wrote:
> > > > > > If VIRTIO_NET_F_CTRL_VLAN is not negotiated, do not filter out
> > > > > > all
> > > > > > VLAN-tagged packets but send them to the guest.
> > 
> > 
> > AFAICS, no fix has been committed, yet. Is there anything I need to do 
> > to get this fixed?
>  
> Michael asked in IRC to continually add a RxState for vlan in
> RxFilter notification through QMP events.
> 
> (sorry I didn't talk too much becaused I'm in meeting)
> 
> I mentioned in [1] [2], we don't need to add this new state
> if we apply patch Stefan's patch[3]
> 
> Michael said Stefan's fix is wrong, but the problem exists.
> The problem is the bug fixed by [3], not the problem that was
> mentioned in [4] 
> 
> 
> Michael, what's wrong with Stefan's patch?
> 
> 
> Thanks, Amos
> 
> [1] http://lists.gnu.org/archive/html/qemu-devel/2014-02/msg03835.html
> [2] https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg04434.html
> [3] virtio-net: Do not filter VLANs without F_CTRL_VLAN
> [4] https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg02604.html
> 

Mainly what's wrong is the effect this has on the output of
query-rx-filter. the patch is incomplete: management should be able to
see that vlan filtering is disabled.

Besides, looking at guest features at reset time is also
not a good idea, it works by luck.
A better place would be virtio_net_set_features.

> > > > > Can we just update receive_filter() to filter out VLAN-tagged
> > > > > packets only when VIRTIO_NET_F_CTRL_VLAN is negotiated?
> > > 
> > > If we change receive_filter(), we also need a flag to indicate
> > > management this feature isn't negotiated, management will do some
> > > additional operation to host device to get same effect.
> > >  
> > > 
> > > > We could. But this adds a (very small) per-packet overhead while
> > > > my patch  only adds overhead during reset. Therefore I didn't
> > > > take that approach. But if changing receive_filter() makes
> > > > management much easier, that could be acceptable.
> > > 
> > > Actually your solution is better, QEMU will return a long list
> > > [0,1,2,...4095] to management, host device will filter all the vlan
> > > packets and send to QEMU.
> > > 
> > > So the problem raised by mst doesn't exist.
> > 
> > Cheers,
> > Stefan
> > 
> 
> -- 
> 			Amos.