* Re: [RFC] virtio_net: multicast address list never sent to host
2013-12-05 23:20 [RFC] virtio_net: multicast address list never sent to host Stephen Hemminger
@ 2013-12-06 14:38 ` Vlad Yasevich
2013-12-06 22:51 ` Stephen Hemminger
2013-12-07 0:42 ` Rusty Russell
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Vlad Yasevich @ 2013-12-06 14:38 UTC (permalink / raw)
To: Stephen Hemminger, Alex Williamson, Rusty Russell,
Michael S. Tsirkin
Cc: netdev
On 12/05/2013 06:20 PM, Stephen Hemminger wrote:
>
> The virtio_net driver never sends the second address list to
> the host. This is because send command takes a pointer to scatter list
> to send but only inserts that one entry into the scatter list that
> is sent to the host.
>
> This bug has been there since:
> commit f565a7c259d71cc186753653d978c646d2354b36
> Author: Alex Williamson <alex.williamson@hp.com>
> Date: Wed Feb 4 09:02:45 2009 +0000
>
> virtio_net: Add a MAC filter table
>
>
> I also dropped the in argument since it is never used.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>
>
> --- a/drivers/net/virtio_net.c 2013-12-05 15:11:34.000000000 -0800
> +++ b/drivers/net/virtio_net.c 2013-12-05 15:17:16.211568831 -0800
> @@ -877,16 +877,16 @@ static netdev_tx_t start_xmit(struct sk_
> * never fail unless improperly formated.
> */
> static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> - struct scatterlist *out,
> - struct scatterlist *in)
> + struct scatterlist *out, unsigned out_cnt)
> {
> struct scatterlist *sgs[4], hdr, stat;
> struct virtio_net_ctrl_hdr ctrl;
> virtio_net_ctrl_ack status = ~0;
> - unsigned out_num = 0, in_num = 0, tmp;
> + unsigned out_num = 0, tmp;
>
> /* Caller should know better */
> BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
> + BUG_ON(out_cnt > 2);
>
Not sure if we need this. Right now we only pass 2, but if some
future command needs more, this is yet another place that needs
to change.
> ctrl.class = class;
> ctrl.cmd = cmd;
> @@ -894,17 +894,14 @@ static bool virtnet_send_command(struct
> sg_init_one(&hdr, &ctrl, sizeof(ctrl));
> sgs[out_num++] = &hdr;
>
> - if (out)
> - sgs[out_num++] = out;
> - if (in)
> - sgs[out_num + in_num++] = in;
> + while (out_cnt-- > 0)
> + sgs[out_num++] = out++;
I think we can skip sg_count and just use
while ((sg = sg_next(out)) != NULL)
sgs[out_num++] = sg;
And we can probably do this loop for both 'in' and 'out' pointers.
-vlad
>
> /* Add return status. */
> sg_init_one(&stat, &status, sizeof(status));
> - sgs[out_num + in_num++] = &stat;
> + sgs[out_num] = &stat;
>
> - BUG_ON(out_num + in_num > ARRAY_SIZE(sgs));
> - BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi, GFP_ATOMIC)
> + BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC)
> < 0);
>
> if (unlikely(!virtqueue_kick(vi->cvq)))
> @@ -936,7 +933,7 @@ static int virtnet_set_mac_address(struc
> sg_init_one(&sg, addr->sa_data, dev->addr_len);
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> VIRTIO_NET_CTRL_MAC_ADDR_SET,
> - &sg, NULL)) {
> + &sg, 1)) {
> dev_warn(&vdev->dev,
> "Failed to set mac address by vq command.\n");
> return -EINVAL;
> @@ -1009,7 +1006,7 @@ static void virtnet_ack_link_announce(st
> {
> rtnl_lock();
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
> - VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL, NULL))
> + VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL, 0))
> dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
> rtnl_unlock();
> }
> @@ -1027,7 +1024,7 @@ static int virtnet_set_queues(struct vir
> sg_init_one(&sg, &s, sizeof(s));
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
> - VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, NULL)) {
> + VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, 1)) {
> dev_warn(&dev->dev, "Fail to set num of queue pairs to %d\n",
> queue_pairs);
> return -EINVAL;
> @@ -1078,7 +1075,7 @@ static void virtnet_set_rx_mode(struct n
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> VIRTIO_NET_CTRL_RX_PROMISC,
> - sg, NULL))
> + sg, 1))
> dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
> promisc ? "en" : "dis");
>
> @@ -1086,7 +1083,7 @@ static void virtnet_set_rx_mode(struct n
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> VIRTIO_NET_CTRL_RX_ALLMULTI,
> - sg, NULL))
> + sg, 1))
> dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
> allmulti ? "en" : "dis");
>
> @@ -1123,7 +1120,7 @@ static void virtnet_set_rx_mode(struct n
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> VIRTIO_NET_CTRL_MAC_TABLE_SET,
> - sg, NULL))
> + sg, 2))
> dev_warn(&dev->dev, "Failed to set MAC filter table.\n");
>
> kfree(buf);
> @@ -1138,7 +1135,7 @@ static int virtnet_vlan_rx_add_vid(struc
> sg_init_one(&sg, &vid, sizeof(vid));
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> - VIRTIO_NET_CTRL_VLAN_ADD, &sg, NULL))
> + VIRTIO_NET_CTRL_VLAN_ADD, &sg, 1))
> dev_warn(&dev->dev, "Failed to add VLAN ID %d.\n", vid);
> return 0;
> }
> @@ -1152,7 +1149,7 @@ static int virtnet_vlan_rx_kill_vid(stru
> sg_init_one(&sg, &vid, sizeof(vid));
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> - VIRTIO_NET_CTRL_VLAN_DEL, &sg, NULL))
> + VIRTIO_NET_CTRL_VLAN_DEL, &sg, 1))
> dev_warn(&dev->dev, "Failed to kill VLAN ID %d.\n", vid);
> return 0;
> }
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC] virtio_net: multicast address list never sent to host
2013-12-06 14:38 ` Vlad Yasevich
@ 2013-12-06 22:51 ` Stephen Hemminger
2013-12-07 1:37 ` Vlad Yasevich
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Hemminger @ 2013-12-06 22:51 UTC (permalink / raw)
To: vyasevic; +Cc: Alex Williamson, Rusty Russell, Michael S. Tsirkin, netdev
On Fri, 06 Dec 2013 09:38:35 -0500
Vlad Yasevich <vyasevic@redhat.com> wrote:
> > - sgs[out_num++] = out;
> > - if (in)
> > - sgs[out_num + in_num++] = in;
> > + while (out_cnt-- > 0)
> > + sgs[out_num++] = out++;
>
> I think we can skip sg_count and just use
> while ((sg = sg_next(out)) != NULL)
> sgs[out_num++] = sg;
>
> And we can probably do this loop for both 'in' and 'out' pointers.
>
> -vlad
That won't work because callers pass a list with a single element,
as in:
> static int virtnet_set_mac_address(struct net_device *dev, void *p)
> {
> struct virtnet_info *vi = netdev_priv(dev);
> struct virtio_device *vdev = vi->vdev;
> int ret;
> struct sockaddr *addr = p;
> struct scatterlist sg;
>
> ret = eth_prepare_mac_addr_change(dev, p);
> if (ret)
> return ret;
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
> sg_init_one(&sg, addr->sa_data, dev->addr_len);
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> VIRTIO_NET_CTRL_MAC_ADDR_SET,
> &sg, NULL)) {
> dev_warn(&vdev->dev,
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC] virtio_net: multicast address list never sent to host
2013-12-06 22:51 ` Stephen Hemminger
@ 2013-12-07 1:37 ` Vlad Yasevich
0 siblings, 0 replies; 9+ messages in thread
From: Vlad Yasevich @ 2013-12-07 1:37 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Alex Williamson, Rusty Russell, Michael S. Tsirkin, netdev
[fixed alex's address to stop bounces]
On 12/06/2013 05:51 PM, Stephen Hemminger wrote:
> On Fri, 06 Dec 2013 09:38:35 -0500
> Vlad Yasevich <vyasevic@redhat.com> wrote:
>
>>> - sgs[out_num++] = out;
>>> - if (in)
>>> - sgs[out_num + in_num++] = in;
>>> + while (out_cnt-- > 0)
>>> + sgs[out_num++] = out++;
>>
>> I think we can skip sg_count and just use
>> while ((sg = sg_next(out)) != NULL)
>> sgs[out_num++] = sg;
>>
>> And we can probably do this loop for both 'in' and 'out' pointers.
>>
>> -vlad
>
>
> That won't work because callers pass a list with a single element,
> as in:
Ok, then how about:
for (sg = out; sg; sg = sg_next(sg))
sgs[out_num++] = sg;
for (sg = in; sg; sg = sg_next(sg))
sgs[in_num++] = sg;
since even a list of 1 element can be iterated using sg_next().
This makes it generic and will not require this function to change
if we ever had more then 2 elements to set, or even more then 1 element
to receive.
-vlad
>
>> static int virtnet_set_mac_address(struct net_device *dev, void *p)
>> {
>> struct virtnet_info *vi = netdev_priv(dev);
>> struct virtio_device *vdev = vi->vdev;
>> int ret;
>> struct sockaddr *addr = p;
>> struct scatterlist sg;
>>
>> ret = eth_prepare_mac_addr_change(dev, p);
>> if (ret)
>> return ret;
>>
>> if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
>> sg_init_one(&sg, addr->sa_data, dev->addr_len);
>> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
>> VIRTIO_NET_CTRL_MAC_ADDR_SET,
>> &sg, NULL)) {
>> dev_warn(&vdev->dev,
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] virtio_net: multicast address list never sent to host
2013-12-05 23:20 [RFC] virtio_net: multicast address list never sent to host Stephen Hemminger
2013-12-06 14:38 ` Vlad Yasevich
@ 2013-12-07 0:42 ` Rusty Russell
2013-12-08 12:35 ` Michael S. Tsirkin
2013-12-10 8:22 ` Amos Kong
3 siblings, 0 replies; 9+ messages in thread
From: Rusty Russell @ 2013-12-07 0:42 UTC (permalink / raw)
To: Stephen Hemminger, Alex Williamson, Michael S. Tsirkin; +Cc: netdev
Stephen Hemminger <stephen@networkplumber.org> writes:
> The virtio_net driver never sends the second address list to
> the host. This is because send command takes a pointer to scatter list
> to send but only inserts that one entry into the scatter list that
> is sent to the host.
>
> This bug has been there since:
> commit f565a7c259d71cc186753653d978c646d2354b36
> Author: Alex Williamson <alex.williamson@hp.com>
> Date: Wed Feb 4 09:02:45 2009 +0000
>
> virtio_net: Add a MAC filter table
>
>
> I also dropped the in argument since it is never used.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Ouch. Took me a while to untangle the two changes (arg change and the
VIRTIO_NET_CTRL_MAC_TABLE_SET change which is the actual fix), but it
looks good.
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Thanks!
Rusty.
> --- a/drivers/net/virtio_net.c 2013-12-05 15:11:34.000000000 -0800
> +++ b/drivers/net/virtio_net.c 2013-12-05 15:17:16.211568831 -0800
> @@ -877,16 +877,16 @@ static netdev_tx_t start_xmit(struct sk_
> * never fail unless improperly formated.
> */
> static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> - struct scatterlist *out,
> - struct scatterlist *in)
> + struct scatterlist *out, unsigned out_cnt)
> {
> struct scatterlist *sgs[4], hdr, stat;
> struct virtio_net_ctrl_hdr ctrl;
> virtio_net_ctrl_ack status = ~0;
> - unsigned out_num = 0, in_num = 0, tmp;
> + unsigned out_num = 0, tmp;
>
> /* Caller should know better */
> BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
> + BUG_ON(out_cnt > 2);
>
> ctrl.class = class;
> ctrl.cmd = cmd;
> @@ -894,17 +894,14 @@ static bool virtnet_send_command(struct
> sg_init_one(&hdr, &ctrl, sizeof(ctrl));
> sgs[out_num++] = &hdr;
>
> - if (out)
> - sgs[out_num++] = out;
> - if (in)
> - sgs[out_num + in_num++] = in;
> + while (out_cnt-- > 0)
> + sgs[out_num++] = out++;
>
> /* Add return status. */
> sg_init_one(&stat, &status, sizeof(status));
> - sgs[out_num + in_num++] = &stat;
> + sgs[out_num] = &stat;
>
> - BUG_ON(out_num + in_num > ARRAY_SIZE(sgs));
> - BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi, GFP_ATOMIC)
> + BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC)
> < 0);
>
> if (unlikely(!virtqueue_kick(vi->cvq)))
> @@ -936,7 +933,7 @@ static int virtnet_set_mac_address(struc
> sg_init_one(&sg, addr->sa_data, dev->addr_len);
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> VIRTIO_NET_CTRL_MAC_ADDR_SET,
> - &sg, NULL)) {
> + &sg, 1)) {
> dev_warn(&vdev->dev,
> "Failed to set mac address by vq command.\n");
> return -EINVAL;
> @@ -1009,7 +1006,7 @@ static void virtnet_ack_link_announce(st
> {
> rtnl_lock();
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
> - VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL, NULL))
> + VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL, 0))
> dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
> rtnl_unlock();
> }
> @@ -1027,7 +1024,7 @@ static int virtnet_set_queues(struct vir
> sg_init_one(&sg, &s, sizeof(s));
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
> - VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, NULL)) {
> + VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, 1)) {
> dev_warn(&dev->dev, "Fail to set num of queue pairs to %d\n",
> queue_pairs);
> return -EINVAL;
> @@ -1078,7 +1075,7 @@ static void virtnet_set_rx_mode(struct n
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> VIRTIO_NET_CTRL_RX_PROMISC,
> - sg, NULL))
> + sg, 1))
> dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
> promisc ? "en" : "dis");
>
> @@ -1086,7 +1083,7 @@ static void virtnet_set_rx_mode(struct n
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> VIRTIO_NET_CTRL_RX_ALLMULTI,
> - sg, NULL))
> + sg, 1))
> dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
> allmulti ? "en" : "dis");
>
> @@ -1123,7 +1120,7 @@ static void virtnet_set_rx_mode(struct n
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> VIRTIO_NET_CTRL_MAC_TABLE_SET,
> - sg, NULL))
> + sg, 2))
> dev_warn(&dev->dev, "Failed to set MAC filter table.\n");
>
> kfree(buf);
> @@ -1138,7 +1135,7 @@ static int virtnet_vlan_rx_add_vid(struc
> sg_init_one(&sg, &vid, sizeof(vid));
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> - VIRTIO_NET_CTRL_VLAN_ADD, &sg, NULL))
> + VIRTIO_NET_CTRL_VLAN_ADD, &sg, 1))
> dev_warn(&dev->dev, "Failed to add VLAN ID %d.\n", vid);
> return 0;
> }
> @@ -1152,7 +1149,7 @@ static int virtnet_vlan_rx_kill_vid(stru
> sg_init_one(&sg, &vid, sizeof(vid));
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> - VIRTIO_NET_CTRL_VLAN_DEL, &sg, NULL))
> + VIRTIO_NET_CTRL_VLAN_DEL, &sg, 1))
> dev_warn(&dev->dev, "Failed to kill VLAN ID %d.\n", vid);
> return 0;
> }
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC] virtio_net: multicast address list never sent to host
2013-12-05 23:20 [RFC] virtio_net: multicast address list never sent to host Stephen Hemminger
2013-12-06 14:38 ` Vlad Yasevich
2013-12-07 0:42 ` Rusty Russell
@ 2013-12-08 12:35 ` Michael S. Tsirkin
2013-12-08 19:43 ` Stephen Hemminger
2013-12-10 8:22 ` Amos Kong
3 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2013-12-08 12:35 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Alex Williamson, Rusty Russell, netdev
On Thu, Dec 05, 2013 at 03:20:52PM -0800, Stephen Hemminger wrote:
>
> The virtio_net driver never sends the second address list to
> the host. This is because send command takes a pointer to scatter list
> to send but only inserts that one entry into the scatter list that
> is sent to the host.
>
> This bug has been there since:
> commit f565a7c259d71cc186753653d978c646d2354b36
> Author: Alex Williamson <alex.williamson@hp.com>
> Date: Wed Feb 4 09:02:45 2009 +0000
>
> virtio_net: Add a MAC filter table
>
>
> I also dropped the in argument since it is never used.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Indeed. Good catch.
Acked-by: Michael S. Tsirkin <mst@redhat.com>
but I think this needs to be submitted formally
without the RFC tag for dave to include it.
It's also needed for stable I think?
>
> --- a/drivers/net/virtio_net.c 2013-12-05 15:11:34.000000000 -0800
> +++ b/drivers/net/virtio_net.c 2013-12-05 15:17:16.211568831 -0800
> @@ -877,16 +877,16 @@ static netdev_tx_t start_xmit(struct sk_
> * never fail unless improperly formated.
> */
> static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> - struct scatterlist *out,
> - struct scatterlist *in)
> + struct scatterlist *out, unsigned out_cnt)
> {
> struct scatterlist *sgs[4], hdr, stat;
> struct virtio_net_ctrl_hdr ctrl;
> virtio_net_ctrl_ack status = ~0;
> - unsigned out_num = 0, in_num = 0, tmp;
> + unsigned out_num = 0, tmp;
>
> /* Caller should know better */
> BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
> + BUG_ON(out_cnt > 2);
>
> ctrl.class = class;
> ctrl.cmd = cmd;
> @@ -894,17 +894,14 @@ static bool virtnet_send_command(struct
> sg_init_one(&hdr, &ctrl, sizeof(ctrl));
> sgs[out_num++] = &hdr;
>
> - if (out)
> - sgs[out_num++] = out;
> - if (in)
> - sgs[out_num + in_num++] = in;
> + while (out_cnt-- > 0)
> + sgs[out_num++] = out++;
>
> /* Add return status. */
> sg_init_one(&stat, &status, sizeof(status));
> - sgs[out_num + in_num++] = &stat;
> + sgs[out_num] = &stat;
>
> - BUG_ON(out_num + in_num > ARRAY_SIZE(sgs));
> - BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi, GFP_ATOMIC)
> + BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC)
> < 0);
>
> if (unlikely(!virtqueue_kick(vi->cvq)))
> @@ -936,7 +933,7 @@ static int virtnet_set_mac_address(struc
> sg_init_one(&sg, addr->sa_data, dev->addr_len);
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> VIRTIO_NET_CTRL_MAC_ADDR_SET,
> - &sg, NULL)) {
> + &sg, 1)) {
> dev_warn(&vdev->dev,
> "Failed to set mac address by vq command.\n");
> return -EINVAL;
> @@ -1009,7 +1006,7 @@ static void virtnet_ack_link_announce(st
> {
> rtnl_lock();
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
> - VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL, NULL))
> + VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL, 0))
> dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
> rtnl_unlock();
> }
> @@ -1027,7 +1024,7 @@ static int virtnet_set_queues(struct vir
> sg_init_one(&sg, &s, sizeof(s));
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
> - VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, NULL)) {
> + VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, 1)) {
> dev_warn(&dev->dev, "Fail to set num of queue pairs to %d\n",
> queue_pairs);
> return -EINVAL;
> @@ -1078,7 +1075,7 @@ static void virtnet_set_rx_mode(struct n
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> VIRTIO_NET_CTRL_RX_PROMISC,
> - sg, NULL))
> + sg, 1))
> dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
> promisc ? "en" : "dis");
>
> @@ -1086,7 +1083,7 @@ static void virtnet_set_rx_mode(struct n
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> VIRTIO_NET_CTRL_RX_ALLMULTI,
> - sg, NULL))
> + sg, 1))
> dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
> allmulti ? "en" : "dis");
>
> @@ -1123,7 +1120,7 @@ static void virtnet_set_rx_mode(struct n
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> VIRTIO_NET_CTRL_MAC_TABLE_SET,
> - sg, NULL))
> + sg, 2))
> dev_warn(&dev->dev, "Failed to set MAC filter table.\n");
>
> kfree(buf);
> @@ -1138,7 +1135,7 @@ static int virtnet_vlan_rx_add_vid(struc
> sg_init_one(&sg, &vid, sizeof(vid));
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> - VIRTIO_NET_CTRL_VLAN_ADD, &sg, NULL))
> + VIRTIO_NET_CTRL_VLAN_ADD, &sg, 1))
> dev_warn(&dev->dev, "Failed to add VLAN ID %d.\n", vid);
> return 0;
> }
> @@ -1152,7 +1149,7 @@ static int virtnet_vlan_rx_kill_vid(stru
> sg_init_one(&sg, &vid, sizeof(vid));
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> - VIRTIO_NET_CTRL_VLAN_DEL, &sg, NULL))
> + VIRTIO_NET_CTRL_VLAN_DEL, &sg, 1))
> dev_warn(&dev->dev, "Failed to kill VLAN ID %d.\n", vid);
> return 0;
> }
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC] virtio_net: multicast address list never sent to host
2013-12-08 12:35 ` Michael S. Tsirkin
@ 2013-12-08 19:43 ` Stephen Hemminger
0 siblings, 0 replies; 9+ messages in thread
From: Stephen Hemminger @ 2013-12-08 19:43 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Alex Williamson, Rusty Russell, netdev
On Sun, 8 Dec 2013 14:35:26 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Dec 05, 2013 at 03:20:52PM -0800, Stephen Hemminger wrote:
> >
> > The virtio_net driver never sends the second address list to
> > the host. This is because send command takes a pointer to scatter list
> > to send but only inserts that one entry into the scatter list that
> > is sent to the host.
> >
> > This bug has been there since:
> > commit f565a7c259d71cc186753653d978c646d2354b36
> > Author: Alex Williamson <alex.williamson@hp.com>
> > Date: Wed Feb 4 09:02:45 2009 +0000
> >
> > virtio_net: Add a MAC filter table
> >
> >
> > I also dropped the in argument since it is never used.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>
> Indeed. Good catch.
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> but I think this needs to be submitted formally
> without the RFC tag for dave to include it.
>
> It's also needed for stable I think?
>
Not sure if anyone was actually using it. Don't like to put things to stable
unless people are seeing bug. It could be that doing this would breaks some
incorrect host implementation of virtio that was assuming original behaviour.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] virtio_net: multicast address list never sent to host
2013-12-05 23:20 [RFC] virtio_net: multicast address list never sent to host Stephen Hemminger
` (2 preceding siblings ...)
2013-12-08 12:35 ` Michael S. Tsirkin
@ 2013-12-10 8:22 ` Amos Kong
2013-12-16 7:52 ` Amos Kong
3 siblings, 1 reply; 9+ messages in thread
From: Amos Kong @ 2013-12-10 8:22 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Alex Williamson, Rusty Russell, Michael S. Tsirkin, netdev,
Jason Wang
On Thu, Dec 05, 2013 at 03:20:52PM -0800, Stephen Hemminger wrote:
>
> The virtio_net driver never sends the second address list to
> the host. This is because send command takes a pointer to scatter list
> to send but only inserts that one entry into the scatter list that
> is sent to the host.
>
> This bug has been there since:
> commit f565a7c259d71cc186753653d978c646d2354b36
> Author: Alex Williamson <alex.williamson@hp.com>
> Date: Wed Feb 4 09:02:45 2009 +0000
>
> virtio_net: Add a MAC filter table
>
>
> I also dropped the in argument since it is never used.
Hi Stephen,
I don't think this patch is necessary, did you touch some real
problem?
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>
> --- a/drivers/net/virtio_net.c 2013-12-05 15:11:34.000000000 -0800
> +++ b/drivers/net/virtio_net.c 2013-12-05 15:17:16.211568831 -0800
> @@ -877,16 +877,16 @@ static netdev_tx_t start_xmit(struct sk_
> * never fail unless improperly formated.
> */
> static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> - struct scatterlist *out,
> - struct scatterlist *in)
> + struct scatterlist *out, unsigned out_cnt)
> {
> struct scatterlist *sgs[4], hdr, stat;
We save scatterlist point in sgs[], one list can have 0, 1 or more than 1 items.
In virtio_rng.c: virtqueue_add_sgs()
/* Count them first. */
for (i = total_out = total_in = 0; i < out_sgs; i++) {
struct scatterlist *sg;
for (sg = sgs[i]; sg; sg = sg_next(sg))
total_out++;
}
Each item of sgs[] will be visited, all the scatter entries of one scatterlist
will also be visited.
In my testing (w/o this patch), multicast addresses can be sent to qemu.
guest) # ip maddr show
1: lo
inet 224.0.0.1
inet6 ff02::1
inet6 ff01::1
2: eth0
link 33:33:00:00:00:01
link 01:00:5e:00:00:01
link 33:33:ff:12:34:56
link 33:33:00:00:02:02
link 01:00:5e:00:00:fb
inet 224.0.0.251
inet 224.0.0.1
inet6 ff02::202
inet6 ff02::1:ff12:3456
inet6 ff02::1
inet6 ff01::1
qmp) { 'execute': 'query-rx-filter'}
{
"return": [
{
"promiscuous": false,
"name": "vnet0",
"main-mac": "52:54:00:12:34:56",
"unicast": "normal",
"vlan-table": [
],
"unicast-table": [
],
"multicast": "normal",
"multicast-overflow": false,
"unicast-overflow": false,
"multicast-table": [
"01:00:5e:00:00:fb",
"33:33:00:00:02:02",
"33:33:ff:12:34:56",
"01:00:5e:00:00:01",
"33:33:00:00:00:01"
],
"broadcast-allowed": false
}
]
}
Thanks, Amos
> struct virtio_net_ctrl_hdr ctrl;
> virtio_net_ctrl_ack status = ~0;
> - unsigned out_num = 0, in_num = 0, tmp;
> + unsigned out_num = 0, tmp;
>
> /* Caller should know better */
> BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
> + BUG_ON(out_cnt > 2);
>
> ctrl.class = class;
> ctrl.cmd = cmd;
> @@ -894,17 +894,14 @@ static bool virtnet_send_command(struct
> sg_init_one(&hdr, &ctrl, sizeof(ctrl));
> sgs[out_num++] = &hdr;
>
> - if (out)
> - sgs[out_num++] = out;
> - if (in)
> - sgs[out_num + in_num++] = in;
> + while (out_cnt-- > 0)
> + sgs[out_num++] = out++;
>
> /* Add return status. */
> sg_init_one(&stat, &status, sizeof(status));
> - sgs[out_num + in_num++] = &stat;
> + sgs[out_num] = &stat;
>
> - BUG_ON(out_num + in_num > ARRAY_SIZE(sgs));
> - BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi, GFP_ATOMIC)
> + BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC)
> < 0);
>
> if (unlikely(!virtqueue_kick(vi->cvq)))
> @@ -936,7 +933,7 @@ static int virtnet_set_mac_address(struc
> sg_init_one(&sg, addr->sa_data, dev->addr_len);
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> VIRTIO_NET_CTRL_MAC_ADDR_SET,
> - &sg, NULL)) {
> + &sg, 1)) {
> dev_warn(&vdev->dev,
> "Failed to set mac address by vq command.\n");
> return -EINVAL;
> @@ -1009,7 +1006,7 @@ static void virtnet_ack_link_announce(st
> {
> rtnl_lock();
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
> - VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL, NULL))
> + VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL, 0))
> dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
> rtnl_unlock();
> }
> @@ -1027,7 +1024,7 @@ static int virtnet_set_queues(struct vir
> sg_init_one(&sg, &s, sizeof(s));
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
> - VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, NULL)) {
> + VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, 1)) {
> dev_warn(&dev->dev, "Fail to set num of queue pairs to %d\n",
> queue_pairs);
> return -EINVAL;
> @@ -1078,7 +1075,7 @@ static void virtnet_set_rx_mode(struct n
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> VIRTIO_NET_CTRL_RX_PROMISC,
> - sg, NULL))
> + sg, 1))
> dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
> promisc ? "en" : "dis");
>
> @@ -1086,7 +1083,7 @@ static void virtnet_set_rx_mode(struct n
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> VIRTIO_NET_CTRL_RX_ALLMULTI,
> - sg, NULL))
> + sg, 1))
> dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
> allmulti ? "en" : "dis");
>
> @@ -1123,7 +1120,7 @@ static void virtnet_set_rx_mode(struct n
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> VIRTIO_NET_CTRL_MAC_TABLE_SET,
> - sg, NULL))
> + sg, 2))
> dev_warn(&dev->dev, "Failed to set MAC filter table.\n");
>
> kfree(buf);
> @@ -1138,7 +1135,7 @@ static int virtnet_vlan_rx_add_vid(struc
> sg_init_one(&sg, &vid, sizeof(vid));
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> - VIRTIO_NET_CTRL_VLAN_ADD, &sg, NULL))
> + VIRTIO_NET_CTRL_VLAN_ADD, &sg, 1))
> dev_warn(&dev->dev, "Failed to add VLAN ID %d.\n", vid);
> return 0;
> }
> @@ -1152,7 +1149,7 @@ static int virtnet_vlan_rx_kill_vid(stru
> sg_init_one(&sg, &vid, sizeof(vid));
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> - VIRTIO_NET_CTRL_VLAN_DEL, &sg, NULL))
> + VIRTIO_NET_CTRL_VLAN_DEL, &sg, 1))
> dev_warn(&dev->dev, "Failed to kill VLAN ID %d.\n", vid);
> return 0;
> }
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC] virtio_net: multicast address list never sent to host
2013-12-10 8:22 ` Amos Kong
@ 2013-12-16 7:52 ` Amos Kong
0 siblings, 0 replies; 9+ messages in thread
From: Amos Kong @ 2013-12-16 7:52 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Alex Williamson, Rusty Russell, Michael S. Tsirkin, netdev,
Jason Wang
On Tue, Dec 10, 2013 at 04:22:01PM +0800, Amos Kong wrote:
> On Thu, Dec 05, 2013 at 03:20:52PM -0800, Stephen Hemminger wrote:
> >
> > The virtio_net driver never sends the second address list to
> > the host. This is because send command takes a pointer to scatter list
> > to send but only inserts that one entry into the scatter list that
> > is sent to the host.
> >
> > This bug has been there since:
> > commit f565a7c259d71cc186753653d978c646d2354b36
> > Author: Alex Williamson <alex.williamson@hp.com>
> > Date: Wed Feb 4 09:02:45 2009 +0000
> >
> > virtio_net: Add a MAC filter table
> >
> >
> > I also dropped the in argument since it is never used.
>
> Hi Stephen,
>
> I don't think this patch is necessary, did you touch some real
> problem?
>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >
> > --- a/drivers/net/virtio_net.c 2013-12-05 15:11:34.000000000 -0800
> > +++ b/drivers/net/virtio_net.c 2013-12-05 15:17:16.211568831 -0800
> > @@ -877,16 +877,16 @@ static netdev_tx_t start_xmit(struct sk_
> > * never fail unless improperly formated.
> > */
> > static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > - struct scatterlist *out,
> > - struct scatterlist *in)
> > + struct scatterlist *out, unsigned out_cnt)
> > {
> > struct scatterlist *sgs[4], hdr, stat;
>
> We save scatterlist point in sgs[], one list can have 0, 1 or more than 1 items.
>
> In virtio_rng.c: virtqueue_add_sgs()
>
> /* Count them first. */
> for (i = total_out = total_in = 0; i < out_sgs; i++) {
> struct scatterlist *sg;
> for (sg = sgs[i]; sg; sg = sg_next(sg))
> total_out++;
> }
>
> Each item of sgs[] will be visited, all the scatter entries of one scatterlist
> will also be visited.
>
> In my testing (w/o this patch), multicast addresses can be sent to qemu.
>
> guest) # ip maddr show
> 1: lo
> inet 224.0.0.1
> inet6 ff02::1
> inet6 ff01::1
> 2: eth0
> link 33:33:00:00:00:01
> link 01:00:5e:00:00:01
> link 33:33:ff:12:34:56
> link 33:33:00:00:02:02
> link 01:00:5e:00:00:fb
> inet 224.0.0.251
> inet 224.0.0.1
> inet6 ff02::202
> inet6 ff02::1:ff12:3456
> inet6 ff02::1
> inet6 ff01::1
>
> qmp) { 'execute': 'query-rx-filter'}
>
> {
> "return": [
> {
> "promiscuous": false,
> "name": "vnet0",
> "main-mac": "52:54:00:12:34:56",
> "unicast": "normal",
> "vlan-table": [
> ],
> "unicast-table": [
> ],
> "multicast": "normal",
> "multicast-overflow": false,
> "unicast-overflow": false,
> "multicast-table": [
> "01:00:5e:00:00:fb",
> "33:33:00:00:02:02",
> "33:33:ff:12:34:56",
> "01:00:5e:00:00:01",
> "33:33:00:00:00:01"
> ],
> "broadcast-allowed": false
> }
> ]
> }
Michael, Jason, any thought?
> Thanks, Amos
^ permalink raw reply [flat|nested] 9+ messages in thread