[QEMU PATCH v4 0/3] virtio-net: fix of ctrl commands

[QEMU PATCH v4 0/3] virtio-net: fix of ctrl commands

[akong]

From: Amos Kong <akong@redhat.com>

Currently virtio-net code relys on the layout of descriptor,
this patchset removed the assumptions and introduced a control
command to set mac address. Last patch is a trivial renaming.

V2: check guest's iov_len
V3: fix of migration compatibility
    make mac field in config space read-only when new feature is acked
V4: add fix of descriptor layout assumptions, trivial rename

Amos Kong (2):
  virtio-net: introduce a new macaddr control
  virtio-net: rename ctrl rx commands

Michael S. Tsirkin (1):
  virtio-net: remove layout assumptions for ctrl vq

 hw/pc_piix.c    |   4 ++
 hw/virtio-net.c | 143 ++++++++++++++++++++++++++++++++++----------------------
 hw/virtio-net.h |  26 +++++++----
 3 files changed, 109 insertions(+), 64 deletions(-)

-- 
1.7.11.7

[QEMU PATCH v4 1/3] virtio-net: remove layout assumptions for ctrl vq

[akong]

From: "Michael S. Tsirkin" <mst@redhat.com>

Virtio-net code makes assumption about virtqueue descriptor layout
(e.g. sg[0] is the header, sg[1] is the data buffer).

This patch makes code not rely on the layout of descriptors.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Amos Kong <akong@redhat.com>
---
 hw/virtio-net.c | 128 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 74 insertions(+), 54 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 3bb01b1..113e194 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -315,44 +315,44 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
 }
 
 static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
-                                     VirtQueueElement *elem)
+                                     struct iovec *iov, unsigned int iov_cnt)
 {
     uint8_t on;
+    size_t s;
 
-    if (elem->out_num != 2 || elem->out_sg[1].iov_len != sizeof(on)) {
-        error_report("virtio-net ctrl invalid rx mode command");
-        exit(1);
+    s = iov_to_buf(iov, iov_cnt, 0, &on, sizeof(on));
+    if (s != sizeof(on)) {
+        return VIRTIO_NET_ERR;
     }
 
-    on = ldub_p(elem->out_sg[1].iov_base);
-
-    if (cmd == VIRTIO_NET_CTRL_RX_MODE_PROMISC)
+    if (cmd == VIRTIO_NET_CTRL_RX_MODE_PROMISC) {
         n->promisc = on;
-    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI)
+    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI) {
         n->allmulti = on;
-    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI)
+    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI) {
         n->alluni = on;
-    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI)
+    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI) {
         n->nomulti = on;
-    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI)
+    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI) {
         n->nouni = on;
-    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST)
+    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST) {
         n->nobcast = on;
-    else
+    } else {
         return VIRTIO_NET_ERR;
+    }
 
     return VIRTIO_NET_OK;
 }
 
 static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
-                                 VirtQueueElement *elem)
+                                 struct iovec *iov, unsigned int iov_cnt)
 {
     struct virtio_net_ctrl_mac mac_data;
+    size_t s;
 
-    if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET || elem->out_num != 3 ||
-        elem->out_sg[1].iov_len < sizeof(mac_data) ||
-        elem->out_sg[2].iov_len < sizeof(mac_data))
+    if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET) {
         return VIRTIO_NET_ERR;
+    }
 
     n->mac_table.in_use = 0;
     n->mac_table.first_multi = 0;
@@ -360,54 +360,71 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
     n->mac_table.multi_overflow = 0;
     memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
 
-    mac_data.entries = ldl_p(elem->out_sg[1].iov_base);
+    s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
+                   sizeof(mac_data.entries));
 
-    if (sizeof(mac_data.entries) +
-        (mac_data.entries * ETH_ALEN) > elem->out_sg[1].iov_len)
+    if (s != sizeof(mac_data.entries)) {
         return VIRTIO_NET_ERR;
+    }
+    iov_discard_front(&iov, &iov_cnt, s);
+
+    if (mac_data.entries * ETH_ALEN > iov_size(iov, iov_cnt)) {
+        return VIRTIO_NET_ERR;
+    }
 
     if (mac_data.entries <= MAC_TABLE_ENTRIES) {
-        memcpy(n->mac_table.macs, elem->out_sg[1].iov_base + sizeof(mac_data),
-               mac_data.entries * ETH_ALEN);
+        s = iov_to_buf(iov, iov_cnt, 0, n->mac_table.macs,
+                       mac_data.entries * ETH_ALEN);
+        if (s != mac_data.entries * ETH_ALEN) {
+            return VIRTIO_NET_OK;
+        }
         n->mac_table.in_use += mac_data.entries;
     } else {
         n->mac_table.uni_overflow = 1;
     }
 
+    iov_discard_front(&iov, &iov_cnt, mac_data.entries * ETH_ALEN);
+
     n->mac_table.first_multi = n->mac_table.in_use;
 
-    mac_data.entries = ldl_p(elem->out_sg[2].iov_base);
+    s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
+                   sizeof(mac_data.entries));
 
-    if (sizeof(mac_data.entries) +
-        (mac_data.entries * ETH_ALEN) > elem->out_sg[2].iov_len)
+    if (s != sizeof(mac_data.entries)) {
         return VIRTIO_NET_ERR;
+    }
 
-    if (mac_data.entries) {
-        if (n->mac_table.in_use + mac_data.entries <= MAC_TABLE_ENTRIES) {
-            memcpy(n->mac_table.macs + (n->mac_table.in_use * ETH_ALEN),
-                   elem->out_sg[2].iov_base + sizeof(mac_data),
-                   mac_data.entries * ETH_ALEN);
-            n->mac_table.in_use += mac_data.entries;
-        } else {
-            n->mac_table.multi_overflow = 1;
+    iov_discard_front(&iov, &iov_cnt, s);
+
+    if (mac_data.entries * ETH_ALEN != iov_size(iov, iov_cnt)) {
+        return VIRTIO_NET_ERR;
+    }
+
+    if (n->mac_table.in_use + mac_data.entries <= MAC_TABLE_ENTRIES) {
+        s = iov_to_buf(iov, iov_cnt, 0, n->mac_table.macs,
+                       mac_data.entries * ETH_ALEN);
+        if (s != mac_data.entries * ETH_ALEN) {
+            return VIRTIO_NET_OK;
         }
+        n->mac_table.in_use += mac_data.entries;
+    } else {
+        n->mac_table.multi_overflow = 1;
     }
 
     return VIRTIO_NET_OK;
 }
 
 static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
-                                        VirtQueueElement *elem)
+                                        struct iovec *iov, unsigned int iov_cnt)
 {
     uint16_t vid;
+    size_t s;
 
-    if (elem->out_num != 2 || elem->out_sg[1].iov_len != sizeof(vid)) {
-        error_report("virtio-net ctrl invalid vlan command");
+    s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
+    if (s != sizeof(vid)) {
         return VIRTIO_NET_ERR;
     }
 
-    vid = lduw_p(elem->out_sg[1].iov_base);
-
     if (vid >= MAX_VLAN)
         return VIRTIO_NET_ERR;
 
@@ -427,30 +444,33 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     struct virtio_net_ctrl_hdr ctrl;
     virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
     VirtQueueElement elem;
+    size_t s;
+    struct iovec *iov;
+    unsigned int iov_cnt;
 
     while (virtqueue_pop(vq, &elem)) {
-        if ((elem.in_num < 1) || (elem.out_num < 1)) {
+        if (iov_size(elem.in_sg, elem.in_num) < sizeof(status) ||
+            iov_size(elem.out_sg, elem.out_num) < sizeof(ctrl)) {
             error_report("virtio-net ctrl missing headers");
             exit(1);
         }
 
-        if (elem.out_sg[0].iov_len < sizeof(ctrl) ||
-            elem.in_sg[elem.in_num - 1].iov_len < sizeof(status)) {
-            error_report("virtio-net ctrl header not in correct element");
-            exit(1);
+        iov = elem.out_sg;
+        iov_cnt = elem.out_num;
+        s = iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl));
+        iov_discard_front(&iov, &iov_cnt, sizeof(ctrl));
+        if (s != sizeof(ctrl)) {
+            status = VIRTIO_NET_ERR;
+        } else if (ctrl.class == VIRTIO_NET_CTRL_RX_MODE) {
+            status = virtio_net_handle_rx_mode(n, ctrl.cmd, iov, iov_cnt);
+        } else if (ctrl.class == VIRTIO_NET_CTRL_MAC) {
+            status = virtio_net_handle_mac(n, ctrl.cmd, iov, iov_cnt);
+        } else if (ctrl.class == VIRTIO_NET_CTRL_VLAN) {
+            status = virtio_net_handle_vlan_table(n, ctrl.cmd, iov, iov_cnt);
         }
 
-        ctrl.class = ldub_p(elem.out_sg[0].iov_base);
-        ctrl.cmd = ldub_p(elem.out_sg[0].iov_base + sizeof(ctrl.class));
-
-        if (ctrl.class == VIRTIO_NET_CTRL_RX_MODE)
-            status = virtio_net_handle_rx_mode(n, ctrl.cmd, &elem);
-        else if (ctrl.class == VIRTIO_NET_CTRL_MAC)
-            status = virtio_net_handle_mac(n, ctrl.cmd, &elem);
-        else if (ctrl.class == VIRTIO_NET_CTRL_VLAN)
-            status = virtio_net_handle_vlan_table(n, ctrl.cmd, &elem);
-
-        stb_p(elem.in_sg[elem.in_num - 1].iov_base, status);
+        s = iov_from_buf(elem.in_sg, elem.in_num, 0, &status, sizeof(status));
+        assert(s == sizeof(status));
 
         virtqueue_push(vq, &elem, sizeof(status));
         virtio_notify(vdev, vq);
-- 
1.7.11.7

Re: [Qemu-devel] [QEMU PATCH v4 1/3] virtio-net: remove layout assumptions for ctrl vq

[Amos Kong]

On Sat, Jan 19, 2013 at 09:54:26AM +0800, akong@redhat.com wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> 
> Virtio-net code makes assumption about virtqueue descriptor layout
> (e.g. sg[0] is the header, sg[1] is the data buffer).
> 
> This patch makes code not rely on the layout of descriptors.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  hw/virtio-net.c | 128 ++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 74 insertions(+), 54 deletions(-)

Had tested this patch with following scenarios:

VIRTIO_NET_CTRL_RX_MODE
1) ip link eth0 promisc on/off
2) ip link set eth0 allmulticast on/off
3) ip link set eth0 multicast on/off

VIRTIO_NET_CTRL_MAC
4) ifconfig eth0 hw ether 52:54:00:12:34:57
5) ping guest after joined guest into multicast
   group (225.0.0.1 ~ 225.0.0.10)

VIRTIO_NET_CTRL_VLAN
6) vconfig add eth0 2; vconfig rem eth0.2

Re: [QEMU PATCH v4 1/3] virtio-net: remove layout assumptions for ctrl vq

[Stefan Hajnoczi]

On Sat, Jan 19, 2013 at 09:54:26AM +0800, akong@redhat.com wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> 
> Virtio-net code makes assumption about virtqueue descriptor layout
> (e.g. sg[0] is the header, sg[1] is the data buffer).
> 
> This patch makes code not rely on the layout of descriptors.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  hw/virtio-net.c | 128 ++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 74 insertions(+), 54 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 3bb01b1..113e194 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -315,44 +315,44 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
>  }
>  
>  static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> -                                     VirtQueueElement *elem)
> +                                     struct iovec *iov, unsigned int iov_cnt)
>  {
>      uint8_t on;
> +    size_t s;
>  
> -    if (elem->out_num != 2 || elem->out_sg[1].iov_len != sizeof(on)) {
> -        error_report("virtio-net ctrl invalid rx mode command");
> -        exit(1);
> +    s = iov_to_buf(iov, iov_cnt, 0, &on, sizeof(on));
> +    if (s != sizeof(on)) {
> +        return VIRTIO_NET_ERR;
>      }
>  
> -    on = ldub_p(elem->out_sg[1].iov_base);
> -
> -    if (cmd == VIRTIO_NET_CTRL_RX_MODE_PROMISC)
> +    if (cmd == VIRTIO_NET_CTRL_RX_MODE_PROMISC) {
>          n->promisc = on;
> -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI)
> +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI) {
>          n->allmulti = on;
> -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI)
> +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI) {
>          n->alluni = on;
> -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI)
> +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI) {
>          n->nomulti = on;
> -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI)
> +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI) {
>          n->nouni = on;
> -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST)
> +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST) {
>          n->nobcast = on;
> -    else
> +    } else {
>          return VIRTIO_NET_ERR;
> +    }
>  
>      return VIRTIO_NET_OK;
>  }
>  
>  static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> -                                 VirtQueueElement *elem)
> +                                 struct iovec *iov, unsigned int iov_cnt)
>  {
>      struct virtio_net_ctrl_mac mac_data;
> +    size_t s;
>  
> -    if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET || elem->out_num != 3 ||
> -        elem->out_sg[1].iov_len < sizeof(mac_data) ||
> -        elem->out_sg[2].iov_len < sizeof(mac_data))
> +    if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET) {
>          return VIRTIO_NET_ERR;
> +    }
>  
>      n->mac_table.in_use = 0;
>      n->mac_table.first_multi = 0;
> @@ -360,54 +360,71 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>      n->mac_table.multi_overflow = 0;
>      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
>  
> -    mac_data.entries = ldl_p(elem->out_sg[1].iov_base);
> +    s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
> +                   sizeof(mac_data.entries));
>  
> -    if (sizeof(mac_data.entries) +
> -        (mac_data.entries * ETH_ALEN) > elem->out_sg[1].iov_len)
> +    if (s != sizeof(mac_data.entries)) {
>          return VIRTIO_NET_ERR;
> +    }
> +    iov_discard_front(&iov, &iov_cnt, s);
> +
> +    if (mac_data.entries * ETH_ALEN > iov_size(iov, iov_cnt)) {

The (possible) byteswap was lost.  ldl_p() copies from target endianness
to host endianness.

> +        return VIRTIO_NET_ERR;
> +    }
>  
>      if (mac_data.entries <= MAC_TABLE_ENTRIES) {
> -        memcpy(n->mac_table.macs, elem->out_sg[1].iov_base + sizeof(mac_data),
> -               mac_data.entries * ETH_ALEN);
> +        s = iov_to_buf(iov, iov_cnt, 0, n->mac_table.macs,
> +                       mac_data.entries * ETH_ALEN);
> +        if (s != mac_data.entries * ETH_ALEN) {
> +            return VIRTIO_NET_OK;

s/VIRTIO_NET_OK/VIRTIO_NET_ERR/

> +        }
>          n->mac_table.in_use += mac_data.entries;
>      } else {
>          n->mac_table.uni_overflow = 1;
>      }
>  
> +    iov_discard_front(&iov, &iov_cnt, mac_data.entries * ETH_ALEN);
> +
>      n->mac_table.first_multi = n->mac_table.in_use;
>  
> -    mac_data.entries = ldl_p(elem->out_sg[2].iov_base);
> +    s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
> +                   sizeof(mac_data.entries));

Same deal with mac_data.entries byteswap.

>  
> -    if (sizeof(mac_data.entries) +
> -        (mac_data.entries * ETH_ALEN) > elem->out_sg[2].iov_len)
> +    if (s != sizeof(mac_data.entries)) {
>          return VIRTIO_NET_ERR;
> +    }
>  
> -    if (mac_data.entries) {
> -        if (n->mac_table.in_use + mac_data.entries <= MAC_TABLE_ENTRIES) {
> -            memcpy(n->mac_table.macs + (n->mac_table.in_use * ETH_ALEN),
> -                   elem->out_sg[2].iov_base + sizeof(mac_data),
> -                   mac_data.entries * ETH_ALEN);
> -            n->mac_table.in_use += mac_data.entries;
> -        } else {
> -            n->mac_table.multi_overflow = 1;
> +    iov_discard_front(&iov, &iov_cnt, s);
> +
> +    if (mac_data.entries * ETH_ALEN != iov_size(iov, iov_cnt)) {
> +        return VIRTIO_NET_ERR;
> +    }
> +
> +    if (n->mac_table.in_use + mac_data.entries <= MAC_TABLE_ENTRIES) {
> +        s = iov_to_buf(iov, iov_cnt, 0, n->mac_table.macs,
> +                       mac_data.entries * ETH_ALEN);
> +        if (s != mac_data.entries * ETH_ALEN) {
> +            return VIRTIO_NET_OK;

VIRTIO_NET_ERR

>          }
> +        n->mac_table.in_use += mac_data.entries;
> +    } else {
> +        n->mac_table.multi_overflow = 1;
>      }
>  
>      return VIRTIO_NET_OK;
>  }
>  
>  static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
> -                                        VirtQueueElement *elem)
> +                                        struct iovec *iov, unsigned int iov_cnt)
>  {
>      uint16_t vid;
> +    size_t s;
>  
> -    if (elem->out_num != 2 || elem->out_sg[1].iov_len != sizeof(vid)) {
> -        error_report("virtio-net ctrl invalid vlan command");
> +    s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
> +    if (s != sizeof(vid)) {
>          return VIRTIO_NET_ERR;
>      }
>  
> -    vid = lduw_p(elem->out_sg[1].iov_base);
> -

Byteswap missing.

Re: [QEMU PATCH v4 1/3] virtio-net: remove layout assumptions for ctrl vq

[Amos Kong]

On Mon, Jan 21, 2013 at 05:03:30PM +0100, Stefan Hajnoczi wrote:
> On Sat, Jan 19, 2013 at 09:54:26AM +0800, akong@redhat.com wrote:
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > 
> > Virtio-net code makes assumption about virtqueue descriptor layout
> > (e.g. sg[0] is the header, sg[1] is the data buffer).
> > 
> > This patch makes code not rely on the layout of descriptors.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  hw/virtio-net.c | 128 ++++++++++++++++++++++++++++++++------------------------
> >  1 file changed, 74 insertions(+), 54 deletions(-)
> > 
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index 3bb01b1..113e194 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -315,44 +315,44 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
> >  }
> >  
> >  static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> > -                                     VirtQueueElement *elem)
> > +                                     struct iovec *iov, unsigned int iov_cnt)
> >  {
> >      uint8_t on;
> > +    size_t s;
> >  
> > -    if (elem->out_num != 2 || elem->out_sg[1].iov_len != sizeof(on)) {
> > -        error_report("virtio-net ctrl invalid rx mode command");
> > -        exit(1);
> > +    s = iov_to_buf(iov, iov_cnt, 0, &on, sizeof(on));
> > +    if (s != sizeof(on)) {
> > +        return VIRTIO_NET_ERR;
> >      }
> >  
> > -    on = ldub_p(elem->out_sg[1].iov_base);
> > -
> > -    if (cmd == VIRTIO_NET_CTRL_RX_MODE_PROMISC)
> > +    if (cmd == VIRTIO_NET_CTRL_RX_MODE_PROMISC) {
> >          n->promisc = on;
> > -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI)
> > +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI) {
> >          n->allmulti = on;
> > -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI)
> > +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI) {
> >          n->alluni = on;
> > -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI)
> > +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI) {
> >          n->nomulti = on;
> > -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI)
> > +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI) {
> >          n->nouni = on;
> > -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST)
> > +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST) {
> >          n->nobcast = on;
> > -    else
> > +    } else {
> >          return VIRTIO_NET_ERR;
> > +    }
> >  
> >      return VIRTIO_NET_OK;
> >  }
> >  
> >  static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > -                                 VirtQueueElement *elem)
> > +                                 struct iovec *iov, unsigned int iov_cnt)
> >  {
> >      struct virtio_net_ctrl_mac mac_data;
> > +    size_t s;
> >  
> > -    if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET || elem->out_num != 3 ||
> > -        elem->out_sg[1].iov_len < sizeof(mac_data) ||
> > -        elem->out_sg[2].iov_len < sizeof(mac_data))
> > +    if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET) {
> >          return VIRTIO_NET_ERR;
> > +    }
> >  
> >      n->mac_table.in_use = 0;
> >      n->mac_table.first_multi = 0;
> > @@ -360,54 +360,71 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >      n->mac_table.multi_overflow = 0;
> >      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
> >  
> > -    mac_data.entries = ldl_p(elem->out_sg[1].iov_base);
> > +    s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
> > +                   sizeof(mac_data.entries));

Hi Stefan, can we adjust the endianness after each iov_to_buf() copy?


diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 72d7857..0088d6c 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -321,6 +321,7 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
     size_t s;
 
     s = iov_to_buf(iov, iov_cnt, 0, &on, sizeof(on));
+    on = ldub_p(&on);
     if (s != sizeof(on)) {
         return VIRTIO_NET_ERR;
     }
@@ -362,7 +363,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
 
     s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
                    sizeof(mac_data.entries));
-
+    mac_data.entries = ldl_p(&mac_data.entries);
     if (s != sizeof(mac_data.entries)) {
         return VIRTIO_NET_ERR;
     }
@@ -389,7 +390,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
 
     s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
                    sizeof(mac_data.entries));
-
+    mac_data.entries = ldl_p(&mac_data.entries);
     if (s != sizeof(mac_data.entries)) {
         return VIRTIO_NET_ERR;
     }
@@ -421,6 +422,7 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
     size_t s;
 
     s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
+    vid = lduw_p(&vid);
     if (s != sizeof(vid)) {
         return VIRTIO_NET_ERR;
     }
@@ -458,6 +460,8 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
         iov = elem.out_sg;
         iov_cnt = elem.out_num;
         s = iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl));
+        ctrl.class = ldub_p(&ctrl.class);
+        ctrl.cmd = ldub_p(&ctrl.class + sizeof(ctrl.class));
         iov_discard_front(&iov, &iov_cnt, sizeof(ctrl));
         if (s != sizeof(ctrl)) {
             status = VIRTIO_NET_ERR;

Re: [QEMU PATCH v4 1/3] virtio-net: remove layout assumptions for ctrl vq

[Stefan Hajnoczi]

On Tue, Jan 22, 2013 at 10:38:14PM +0800, Amos Kong wrote:
> On Mon, Jan 21, 2013 at 05:03:30PM +0100, Stefan Hajnoczi wrote:
> > On Sat, Jan 19, 2013 at 09:54:26AM +0800, akong@redhat.com wrote:
> > > From: "Michael S. Tsirkin" <mst@redhat.com>
> > > 
> > > Virtio-net code makes assumption about virtqueue descriptor layout
> > > (e.g. sg[0] is the header, sg[1] is the data buffer).
> > > 
> > > This patch makes code not rely on the layout of descriptors.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > ---
> > >  hw/virtio-net.c | 128 ++++++++++++++++++++++++++++++++------------------------
> > >  1 file changed, 74 insertions(+), 54 deletions(-)
> > > 
> > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > > index 3bb01b1..113e194 100644
> > > --- a/hw/virtio-net.c
> > > +++ b/hw/virtio-net.c
> > > @@ -315,44 +315,44 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
> > >  }
> > >  
> > >  static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> > > -                                     VirtQueueElement *elem)
> > > +                                     struct iovec *iov, unsigned int iov_cnt)
> > >  {
> > >      uint8_t on;
> > > +    size_t s;
> > >  
> > > -    if (elem->out_num != 2 || elem->out_sg[1].iov_len != sizeof(on)) {
> > > -        error_report("virtio-net ctrl invalid rx mode command");
> > > -        exit(1);
> > > +    s = iov_to_buf(iov, iov_cnt, 0, &on, sizeof(on));
> > > +    if (s != sizeof(on)) {
> > > +        return VIRTIO_NET_ERR;
> > >      }
> > >  
> > > -    on = ldub_p(elem->out_sg[1].iov_base);
> > > -
> > > -    if (cmd == VIRTIO_NET_CTRL_RX_MODE_PROMISC)
> > > +    if (cmd == VIRTIO_NET_CTRL_RX_MODE_PROMISC) {
> > >          n->promisc = on;
> > > -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI)
> > > +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI) {
> > >          n->allmulti = on;
> > > -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI)
> > > +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI) {
> > >          n->alluni = on;
> > > -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI)
> > > +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI) {
> > >          n->nomulti = on;
> > > -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI)
> > > +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI) {
> > >          n->nouni = on;
> > > -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST)
> > > +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST) {
> > >          n->nobcast = on;
> > > -    else
> > > +    } else {
> > >          return VIRTIO_NET_ERR;
> > > +    }
> > >  
> > >      return VIRTIO_NET_OK;
> > >  }
> > >  
> > >  static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > > -                                 VirtQueueElement *elem)
> > > +                                 struct iovec *iov, unsigned int iov_cnt)
> > >  {
> > >      struct virtio_net_ctrl_mac mac_data;
> > > +    size_t s;
> > >  
> > > -    if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET || elem->out_num != 3 ||
> > > -        elem->out_sg[1].iov_len < sizeof(mac_data) ||
> > > -        elem->out_sg[2].iov_len < sizeof(mac_data))
> > > +    if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET) {
> > >          return VIRTIO_NET_ERR;
> > > +    }
> > >  
> > >      n->mac_table.in_use = 0;
> > >      n->mac_table.first_multi = 0;
> > > @@ -360,54 +360,71 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > >      n->mac_table.multi_overflow = 0;
> > >      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
> > >  
> > > -    mac_data.entries = ldl_p(elem->out_sg[1].iov_base);
> > > +    s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
> > > +                   sizeof(mac_data.entries));
> 
> Hi Stefan, can we adjust the endianness after each iov_to_buf() copy?

Yes.

It's only necessary for uint16_t and larger types since a single byte
cannot be swapped (so ldub_p() is not needed).

Stefan

[QEMU PATCH v4 2/3] virtio-net: introduce a new macaddr control

[akong]

From: Amos Kong <akong@redhat.com>

In virtio-net guest driver, currently we write MAC address to
pci config space byte by byte, this means that we have an
intermediate step where mac is wrong. This patch introduced
a new control command to set MAC address, it's atomic.

VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.

"mac" field will be set to read-only when VIRTIO_NET_F_CTRL_MAC_ADDR
is acked.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hw/pc_piix.c    |  4 ++++
 hw/virtio-net.c | 15 ++++++++++++++-
 hw/virtio-net.h | 12 ++++++++++--
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 0a6923d..6218350 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -297,6 +297,10 @@ static QEMUMachine pc_i440fx_machine_v1_4 = {
             .driver   = "usb-tablet",\
             .property = "usb_version",\
             .value    = stringify(1),\
+        },{\
+            .driver   = "virtio-net-pci",\
+            .property = "ctrl_mac_addr",\
+            .value    = "off",      \
         }
 
 static QEMUMachine pc_machine_v1_3 = {
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 113e194..cf40ff2 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -93,7 +93,8 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
 
     memcpy(&netcfg, config, sizeof(netcfg));
 
-    if (memcmp(netcfg.mac, n->mac, ETH_ALEN)) {
+    if (!(n->vdev.guest_features >> VIRTIO_NET_F_CTRL_MAC_ADDR & 1) &&
+        memcmp(netcfg.mac, n->mac, ETH_ALEN)) {
         memcpy(n->mac, netcfg.mac, ETH_ALEN);
         qemu_format_nic_info_str(&n->nic->nc, n->mac);
     }
@@ -350,6 +351,18 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
     struct virtio_net_ctrl_mac mac_data;
     size_t s;
 
+    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
+        if (iov_size(iov, iov_cnt) != ETH_ALEN) {
+            return VIRTIO_NET_ERR;
+        }
+        s = iov_to_buf(iov, iov_cnt, 0, &n->mac, sizeof(n->mac));
+        if (s != sizeof(n->mac)) {
+            return VIRTIO_NET_ERR;
+        }
+        qemu_format_nic_info_str(&n->nic->nc, n->mac);
+        return VIRTIO_NET_OK;
+    }
+
     if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET) {
         return VIRTIO_NET_ERR;
     }
diff --git a/hw/virtio-net.h b/hw/virtio-net.h
index d46fb98..1ec632f 100644
--- a/hw/virtio-net.h
+++ b/hw/virtio-net.h
@@ -44,6 +44,8 @@
 #define VIRTIO_NET_F_CTRL_VLAN  19      /* Control channel VLAN filtering */
 #define VIRTIO_NET_F_CTRL_RX_EXTRA 20   /* Extra RX mode control support */
 
+#define VIRTIO_NET_F_CTRL_MAC_ADDR   23 /* Set MAC address */
+
 #define VIRTIO_NET_S_LINK_UP    1       /* Link is up */
 
 #define TX_TIMER_INTERVAL 150000 /* 150 us */
@@ -106,7 +108,7 @@ typedef uint8_t virtio_net_ctrl_ack;
  #define VIRTIO_NET_CTRL_RX_MODE_NOBCAST      5
 
 /*
- * Control the MAC filter table.
+ * Control the MAC
  *
  * The MAC filter table is managed by the hypervisor, the guest should
  * assume the size is infinite.  Filtering should be considered
@@ -119,6 +121,10 @@ typedef uint8_t virtio_net_ctrl_ack;
  * first sg list contains unicast addresses, the second is for multicast.
  * This functionality is present if the VIRTIO_NET_F_CTRL_RX feature
  * is available.
+ *
+ * The ADDR_SET command requests one out scatterlist, it contains a
+ * 6 bytes MAC address. This functionality is present if the
+ * VIRTIO_NET_F_CTRL_MAC_ADDR feature is available.
  */
 struct virtio_net_ctrl_mac {
     uint32_t entries;
@@ -126,6 +132,7 @@ struct virtio_net_ctrl_mac {
 };
 #define VIRTIO_NET_CTRL_MAC    1
  #define VIRTIO_NET_CTRL_MAC_TABLE_SET        0
+ #define VIRTIO_NET_CTRL_MAC_ADDR_SET         1
 
 /*
  * Control VLAN filtering
@@ -158,5 +165,6 @@ struct virtio_net_ctrl_mac {
         DEFINE_PROP_BIT("ctrl_vq", _state, _field, VIRTIO_NET_F_CTRL_VQ, true), \
         DEFINE_PROP_BIT("ctrl_rx", _state, _field, VIRTIO_NET_F_CTRL_RX, true), \
         DEFINE_PROP_BIT("ctrl_vlan", _state, _field, VIRTIO_NET_F_CTRL_VLAN, true), \
-        DEFINE_PROP_BIT("ctrl_rx_extra", _state, _field, VIRTIO_NET_F_CTRL_RX_EXTRA, true)
+        DEFINE_PROP_BIT("ctrl_rx_extra", _state, _field, VIRTIO_NET_F_CTRL_RX_EXTRA, true), \
+        DEFINE_PROP_BIT("ctrl_mac_addr", _state, _field, VIRTIO_NET_F_CTRL_MAC_ADDR, true)
 #endif
-- 
1.7.11.7

[QEMU PATCH v4 3/3] virtio-net: rename ctrl rx commands

[akong]

From: Amos Kong <akong@redhat.com>

This patch makes rx commands consistent with specification.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hw/virtio-net.c | 14 +++++++-------
 hw/virtio-net.h | 14 +++++++-------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index cf40ff2..5700e22 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -326,17 +326,17 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
         return VIRTIO_NET_ERR;
     }
 
-    if (cmd == VIRTIO_NET_CTRL_RX_MODE_PROMISC) {
+    if (cmd == VIRTIO_NET_CTRL_RX_PROMISC) {
         n->promisc = on;
-    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI) {
+    } else if (cmd == VIRTIO_NET_CTRL_RX_ALLMULTI) {
         n->allmulti = on;
-    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI) {
+    } else if (cmd == VIRTIO_NET_CTRL_RX_ALLUNI) {
         n->alluni = on;
-    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI) {
+    } else if (cmd == VIRTIO_NET_CTRL_RX_NOMULTI) {
         n->nomulti = on;
-    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI) {
+    } else if (cmd == VIRTIO_NET_CTRL_RX_NOUNI) {
         n->nouni = on;
-    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST) {
+    } else if (cmd == VIRTIO_NET_CTRL_RX_NOBCAST) {
         n->nobcast = on;
     } else {
         return VIRTIO_NET_ERR;
@@ -474,7 +474,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
         iov_discard_front(&iov, &iov_cnt, sizeof(ctrl));
         if (s != sizeof(ctrl)) {
             status = VIRTIO_NET_ERR;
-        } else if (ctrl.class == VIRTIO_NET_CTRL_RX_MODE) {
+        } else if (ctrl.class == VIRTIO_NET_CTRL_RX) {
             status = virtio_net_handle_rx_mode(n, ctrl.cmd, iov, iov_cnt);
         } else if (ctrl.class == VIRTIO_NET_CTRL_MAC) {
             status = virtio_net_handle_mac(n, ctrl.cmd, iov, iov_cnt);
diff --git a/hw/virtio-net.h b/hw/virtio-net.h
index 1ec632f..c0bb284 100644
--- a/hw/virtio-net.h
+++ b/hw/virtio-net.h
@@ -99,13 +99,13 @@ typedef uint8_t virtio_net_ctrl_ack;
  * 0 and 1 are supported with the VIRTIO_NET_F_CTRL_RX feature.
  * Commands 2-5 are added with VIRTIO_NET_F_CTRL_RX_EXTRA.
  */
-#define VIRTIO_NET_CTRL_RX_MODE    0
- #define VIRTIO_NET_CTRL_RX_MODE_PROMISC      0
- #define VIRTIO_NET_CTRL_RX_MODE_ALLMULTI     1
- #define VIRTIO_NET_CTRL_RX_MODE_ALLUNI       2
- #define VIRTIO_NET_CTRL_RX_MODE_NOMULTI      3
- #define VIRTIO_NET_CTRL_RX_MODE_NOUNI        4
- #define VIRTIO_NET_CTRL_RX_MODE_NOBCAST      5
+#define VIRTIO_NET_CTRL_RX    0
+ #define VIRTIO_NET_CTRL_RX_PROMISC      0
+ #define VIRTIO_NET_CTRL_RX_ALLMULTI     1
+ #define VIRTIO_NET_CTRL_RX_ALLUNI       2
+ #define VIRTIO_NET_CTRL_RX_NOMULTI      3
+ #define VIRTIO_NET_CTRL_RX_NOUNI        4
+ #define VIRTIO_NET_CTRL_RX_NOBCAST      5
 
 /*
  * Control the MAC
-- 
1.7.11.7

Re: [QEMU PATCH v4 2/3] virtio-net: introduce a new macaddr control

[Stefan Hajnoczi]

On Sat, Jan 19, 2013 at 09:54:27AM +0800, akong@redhat.com wrote:
> @@ -350,6 +351,18 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>      struct virtio_net_ctrl_mac mac_data;
>      size_t s;
>  
> +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
> +        if (iov_size(iov, iov_cnt) != ETH_ALEN) {
> +            return VIRTIO_NET_ERR;
> +        }
> +        s = iov_to_buf(iov, iov_cnt, 0, &n->mac, sizeof(n->mac));
> +        if (s != sizeof(n->mac)) {
> +            return VIRTIO_NET_ERR;
> +        }

Since iov_size() was checked before iov_to_buf(), we never hit this
error.  And if we did n->mac would be trashed (i.e. error handling is
not complete).

I think assert(s == sizeof(n->mac)) is more appropriate appropriate.
Also, please change ETH_ALEN to sizeof(n->mac) to make the relationship
between the check and the copy clear.

Stefan

Re: [QEMU PATCH v4 2/3] virtio-net: introduce a new macaddr control

[Amos Kong]

On Mon, Jan 21, 2013 at 05:08:26PM +0100, Stefan Hajnoczi wrote:
> On Sat, Jan 19, 2013 at 09:54:27AM +0800, akong@redhat.com wrote:
> > @@ -350,6 +351,18 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >      struct virtio_net_ctrl_mac mac_data;
> >      size_t s;
> >  
> > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
> > +        if (iov_size(iov, iov_cnt) != ETH_ALEN) {
> > +            return VIRTIO_NET_ERR;
> > +        }
> > +        s = iov_to_buf(iov, iov_cnt, 0, &n->mac, sizeof(n->mac));
> > +        if (s != sizeof(n->mac)) {
> > +            return VIRTIO_NET_ERR;
> > +        }

     
> Since iov_size() was checked before iov_to_buf(), we never hit this
> error.  And if we did n->mac would be trashed (i.e. error handling is
> not complete).

You are right.
iov_size() computes the size by accounting iov[].iov_lens, the first
check is enough.
 
> I think assert(s == sizeof(n->mac)) is more appropriate appropriate.
> Also, please change ETH_ALEN to sizeof(n->mac) to make the relationship
> between the check and the copy clear.
> 

Will update this patch.

         if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
             if (iov_size(iov, iov_cnt) != sizeof(n->mac)) {
                 return VIRTIO_NET_ERR;
             }
             s = iov_to_buf(iov, iov_cnt, 0, &n->mac, sizeof(n->mac));
             assert(s == sizeof(n->mac));
             qemu_format_nic_info_str(&n->nic->nc, n->mac);
             return VIRTIO_NET_OK;
         }

> Stefan