| Message ID | alpine.DEB.2.02.1402122242370.13560@eru.sfritsch.de |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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
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. >
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. > >
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
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 >
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.
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,
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(-)