[PATCH v2 0/2] make mac programming for virtio net more robust

[PATCH v2 0/2] make mac programming for virtio net more robust

[akong]

From: Amos Kong <akong@redhat.com>

Currenly mac is programmed byte by byte. This means that we
have an intermediate step where mac is wrong. 

Second patch introduced a new vq control command to set mac
address in one time.

V2: check return of sending command, delay eth_mac_addr()

Amos Kong (2):
  move virtnet_send_command() above virtnet_set_mac_address()
  virtio-net: introduce a new control to set macaddr

 drivers/net/virtio_net.c        | 113 ++++++++++++++++++++++------------------
 include/uapi/linux/virtio_net.h |   8 ++-
 2 files changed, 68 insertions(+), 53 deletions(-)

-- 
1.7.11.7

[PATCH v2 1/2] move virtnet_send_command() above virtnet_set_mac_address()

[akong]

From: Amos Kong <akong@redhat.com>

We will send vq command to set mac address in virtnet_set_mac_address()
a little fix of coding style

Signed-off-by: Amos Kong <akong@redhat.com>
---
 drivers/net/virtio_net.c | 89 ++++++++++++++++++++++++------------------------
 1 file changed, 44 insertions(+), 45 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a6fcf15..395ab4f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -753,6 +753,50 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NETDEV_TX_OK;
 }
 
+/*
+ * Send command via the control virtqueue and check status.  Commands
+ * supported by the hypervisor, as indicated by feature bits, should
+ * never fail unless improperly formated.
+ */
+static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
+				 struct scatterlist *data, int out, int in)
+{
+	struct scatterlist *s, sg[VIRTNET_SEND_COMMAND_SG_MAX + 2];
+	struct virtio_net_ctrl_hdr ctrl;
+	virtio_net_ctrl_ack status = ~0;
+	unsigned int tmp;
+	int i;
+
+	/* Caller should know better */
+	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ||
+		(out + in > VIRTNET_SEND_COMMAND_SG_MAX));
+
+	out++; /* Add header */
+	in++; /* Add return status */
+
+	ctrl.class = class;
+	ctrl.cmd = cmd;
+
+	sg_init_table(sg, out + in);
+
+	sg_set_buf(&sg[0], &ctrl, sizeof(ctrl));
+	for_each_sg(data, s, out + in - 2, i)
+		sg_set_buf(&sg[i + 1], sg_virt(s), s->length);
+	sg_set_buf(&sg[out + in - 1], &status, sizeof(status));
+
+	BUG_ON(virtqueue_add_buf(vi->cvq, sg, out, in, vi, GFP_ATOMIC) < 0);
+
+	virtqueue_kick(vi->cvq);
+
+	/* Spin for a response, the kick causes an ioport write, trapping
+	 * into the hypervisor, so the request should be handled immediately.
+	 */
+	while (!virtqueue_get_buf(vi->cvq, &tmp))
+		cpu_relax();
+
+	return status == VIRTIO_NET_OK;
+}
+
 static int virtnet_set_mac_address(struct net_device *dev, void *p)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -819,51 +863,6 @@ static void virtnet_netpoll(struct net_device *dev)
 }
 #endif
 
-/*
- * Send command via the control virtqueue and check status.  Commands
- * supported by the hypervisor, as indicated by feature bits, should
- * never fail unless improperly formated.
- */
-static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
-				 struct scatterlist *data, int out, int in)
-{
-	struct scatterlist *s, sg[VIRTNET_SEND_COMMAND_SG_MAX + 2];
-	struct virtio_net_ctrl_hdr ctrl;
-	virtio_net_ctrl_ack status = ~0;
-	unsigned int tmp;
-	int i;
-
-	/* Caller should know better */
-	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ||
-		(out + in > VIRTNET_SEND_COMMAND_SG_MAX));
-
-	out++; /* Add header */
-	in++; /* Add return status */
-
-	ctrl.class = class;
-	ctrl.cmd = cmd;
-
-	sg_init_table(sg, out + in);
-
-	sg_set_buf(&sg[0], &ctrl, sizeof(ctrl));
-	for_each_sg(data, s, out + in - 2, i)
-		sg_set_buf(&sg[i + 1], sg_virt(s), s->length);
-	sg_set_buf(&sg[out + in - 1], &status, sizeof(status));
-
-	BUG_ON(virtqueue_add_buf(vi->cvq, sg, out, in, vi, GFP_ATOMIC) < 0);
-
-	virtqueue_kick(vi->cvq);
-
-	/*
-	 * Spin for a response, the kick causes an ioport write, trapping
-	 * into the hypervisor, so the request should be handled immediately.
-	 */
-	while (!virtqueue_get_buf(vi->cvq, &tmp))
-		cpu_relax();
-
-	return status == VIRTIO_NET_OK;
-}
-
 static void virtnet_ack_link_announce(struct virtnet_info *vi)
 {
 	rtnl_lock();
-- 
1.7.11.7

[PATCH v2 2/2] virtio-net: introduce a new control to set macaddr

[akong]

From: Amos Kong <akong@redhat.com>

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
in one time.

VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 drivers/net/virtio_net.c        | 24 +++++++++++++++++-------
 include/uapi/linux/virtio_net.h |  8 +++++++-
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 395ab4f..c8901b6 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -802,16 +802,25 @@ 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_mac_addr(dev, p);
-	if (ret)
-		return ret;
-
-	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC))
+	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, 1, 0)) {
+			dev_warn(&vdev->dev,
+				 "Failed to set mac address by vq command.\n");
+			return -EINVAL;
+		}
+	} else if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
 		vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
-		                  dev->dev_addr, dev->addr_len);
+				  addr->sa_data, dev->addr_len);
+	}
+	ret = eth_mac_addr(dev, p);
 
-	return 0;
+	return ret;
 }
 
 static struct rtnl_link_stats64 *virtnet_stats(struct net_device *dev,
@@ -1627,6 +1636,7 @@ static unsigned int features[] = {
 	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
 	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
 	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ,
+	VIRTIO_NET_F_CTRL_MAC_ADDR,
 };
 
 static struct virtio_driver virtio_net_driver = {
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 848e358..a5a8c88 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -53,6 +53,7 @@
 					 * network */
 #define VIRTIO_NET_F_MQ	22	/* Device supports Receive Flow
 					 * Steering */
+#define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
 
 #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
 #define VIRTIO_NET_S_ANNOUNCE	2	/* Announcement is needed */
@@ -127,7 +128,7 @@ typedef __u8 virtio_net_ctrl_ack;
  #define VIRTIO_NET_CTRL_RX_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
@@ -140,6 +141,10 @@ typedef __u8 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 {
 	__u32 entries;
@@ -148,6 +153,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
-- 
1.7.11.7

[QEMU PATCH v2] 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 in one time.

VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.

Signed-off-by: Amos Kong <akong@redhat.com>
---
V2: check guest's iov_len before memcpy
---
 hw/virtio-net.c | 10 ++++++++++
 hw/virtio-net.h |  9 ++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index dc7c6d6..d05f98f 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -247,6 +247,7 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
     VirtIONet *n = to_virtio_net(vdev);
 
     features |= (1 << VIRTIO_NET_F_MAC);
+    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
 
     if (!peer_has_vnet_hdr(n)) {
         features &= ~(0x1 << VIRTIO_NET_F_CSUM);
@@ -282,6 +283,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
     /* Linux kernel 2.6.25.  It understood MAC (as everyone must),
      * but also these: */
     features |= (1 << VIRTIO_NET_F_MAC);
+    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
     features |= (1 << VIRTIO_NET_F_CSUM);
     features |= (1 << VIRTIO_NET_F_HOST_TSO4);
     features |= (1 << VIRTIO_NET_F_HOST_TSO6);
@@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
 {
     struct virtio_net_ctrl_mac mac_data;
 
+    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
+        elem->out_sg[1].iov_len == ETH_ALEN) {
+        /* Set MAC address */
+        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
+        qemu_format_nic_info_str(&n->nic->nc, n->mac);
+        return VIRTIO_NET_OK;
+    }
+
     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))
diff --git a/hw/virtio-net.h b/hw/virtio-net.h
index d46fb98..9394cc0 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
-- 
1.7.11.7

Re: [PATCH v2 2/2] virtio-net: introduce a new control to set macaddr

[Jason Wang]

On Wednesday, January 16, 2013 01:57:01 PM akong@redhat.com wrote:
> From: Amos Kong <akong@redhat.com>
> 
> 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
> in one time.
> 
> VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  drivers/net/virtio_net.c        | 24 +++++++++++++++++-------
>  include/uapi/linux/virtio_net.h |  8 +++++++-
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 395ab4f..c8901b6 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -802,16 +802,25 @@ 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_mac_addr(dev, p);
> -	if (ret)
> -		return ret;
> -
> -	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC))
> +	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, 1, 0)) {
> +			dev_warn(&vdev->dev,
> +				 "Failed to set mac address by vq command.\n");
> +			return -EINVAL;
> +		}
> +	} else if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
>  		vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
> -		                  dev->dev_addr, dev->addr_len);
> +				  addr->sa_data, dev->addr_len);
> +	}
> +	ret = eth_mac_addr(dev, p);
> 

The you will the validity check in eth_mac_addr which may result a wrong mac 
address to be set in the hardware (or is there any check in qemu) and a 
inconsistency bettween what kernel assumes and qemu has.

You can take a look at netvsc driver that calls eth_mac_addr() first and 
restore the software mac address when fail to enforce it to hardware.

Thanks
> -	return 0;
> +	return ret;
>  }
> 
>  static struct rtnl_link_stats64 *virtnet_stats(struct net_device *dev,
> @@ -1627,6 +1636,7 @@ static unsigned int features[] = {
>  	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
>  	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ,
> +	VIRTIO_NET_F_CTRL_MAC_ADDR,
>  };
> 
>  static struct virtio_driver virtio_net_driver = {
> diff --git a/include/uapi/linux/virtio_net.h
> b/include/uapi/linux/virtio_net.h index 848e358..a5a8c88 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -53,6 +53,7 @@
>  					 * network */
>  #define VIRTIO_NET_F_MQ	22	/* Device supports Receive Flow
>  					 * Steering */
> +#define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
> 
>  #define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
>  #define VIRTIO_NET_S_ANNOUNCE	2	/* Announcement is needed */
> @@ -127,7 +128,7 @@ typedef __u8 virtio_net_ctrl_ack;
>   #define VIRTIO_NET_CTRL_RX_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
> @@ -140,6 +141,10 @@ typedef __u8 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 {
>  	__u32 entries;
> @@ -148,6 +153,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

Re: [PATCH v2 2/2] virtio-net: introduce a new control to set macaddr

[Amos Kong]

On Wed, Jan 16, 2013 at 02:20:39PM +0800, Jason Wang wrote:
> On Wednesday, January 16, 2013 01:57:01 PM akong@redhat.com wrote:
> > From: Amos Kong <akong@redhat.com>
> > 
> > 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
> > in one time.
> > 
> > VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  drivers/net/virtio_net.c        | 24 +++++++++++++++++-------
> >  include/uapi/linux/virtio_net.h |  8 +++++++-
> >  2 files changed, 24 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 395ab4f..c8901b6 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -802,16 +802,25 @@ 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_mac_addr(dev, p);
> > -	if (ret)
> > -		return ret;
> > -
> > -	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC))
> > +	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, 1, 0)) {
> > +			dev_warn(&vdev->dev,
> > +				 "Failed to set mac address by vq command.\n");
> > +			return -EINVAL;
> > +		}
> > +	} else if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
> >  		vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
> > -		                  dev->dev_addr, dev->addr_len);
> > +				  addr->sa_data, dev->addr_len);
> > +	}
> > +	ret = eth_mac_addr(dev, p);
> > 
> 
> The you will the validity check in eth_mac_addr which may result a wrong mac 
> address to be set in the hardware (or is there any check in qemu) and a 
> inconsistency bettween what kernel assumes and qemu has.
> 
> You can take a look at netvsc driver that calls eth_mac_addr() first and 
> restore the software mac address when fail to enforce it to hardware.

Thanks for the catching, I will move eth_mac_addr() back to above,
just restore addr if fail to send command.

I will also use DEFINE_PROP_BIT to fix migration issue, thanks.
 
> Thanks
> > -	return 0;
> > +	return ret;
> >  }

Re: [PATCH v2 2/2] virtio-net: introduce a new control to set macaddr

[Michael S. Tsirkin]

On Wed, Jan 16, 2013 at 04:24:47PM +0800, Amos Kong wrote:
> On Wed, Jan 16, 2013 at 02:20:39PM +0800, Jason Wang wrote:
> > On Wednesday, January 16, 2013 01:57:01 PM akong@redhat.com wrote:
> > > From: Amos Kong <akong@redhat.com>
> > > 
> > > 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
> > > in one time.
> > > 
> > > VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.
> > > 
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > ---
> > >  drivers/net/virtio_net.c        | 24 +++++++++++++++++-------
> > >  include/uapi/linux/virtio_net.h |  8 +++++++-
> > >  2 files changed, 24 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 395ab4f..c8901b6 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -802,16 +802,25 @@ 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_mac_addr(dev, p);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > -	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC))
> > > +	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, 1, 0)) {
> > > +			dev_warn(&vdev->dev,
> > > +				 "Failed to set mac address by vq command.\n");
> > > +			return -EINVAL;
> > > +		}
> > > +	} else if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
> > >  		vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
> > > -		                  dev->dev_addr, dev->addr_len);
> > > +				  addr->sa_data, dev->addr_len);
> > > +	}
> > > +	ret = eth_mac_addr(dev, p);
> > > 
> > 
> > The you will the validity check in eth_mac_addr which may result a wrong mac 
> > address to be set in the hardware (or is there any check in qemu) and a 
> > inconsistency bettween what kernel assumes and qemu has.
> > 
> > You can take a look at netvsc driver that calls eth_mac_addr() first and 
> > restore the software mac address when fail to enforce it to hardware.
> 
> Thanks for the catching, I will move eth_mac_addr() back to above,
> just restore addr if fail to send command.
> 
> I will also use DEFINE_PROP_BIT to fix migration issue, thanks.

And clear it if running with a compat machine type.

> > Thanks
> > > -	return 0;
> > > +	return ret;
> > >  }
> 

Re: [QEMU PATCH v2] virtio-net: introduce a new macaddr control

[Jason Wang]

On Wednesday, January 16, 2013 02:16:47 PM akong@redhat.com wrote:
> 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 in one time.
> 
> VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
> V2: check guest's iov_len before memcpy
> ---
>  hw/virtio-net.c | 10 ++++++++++
>  hw/virtio-net.h |  9 ++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index dc7c6d6..d05f98f 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -247,6 +247,7 @@ static uint32_t virtio_net_get_features(VirtIODevice
> *vdev, uint32_t features) VirtIONet *n = to_virtio_net(vdev);
> 
>      features |= (1 << VIRTIO_NET_F_MAC);
> +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
> 
>      if (!peer_has_vnet_hdr(n)) {
>          features &= ~(0x1 << VIRTIO_NET_F_CSUM);
> @@ -282,6 +283,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice
> *vdev) /* Linux kernel 2.6.25.  It understood MAC (as everyone must), * but
> also these: */
>      features |= (1 << VIRTIO_NET_F_MAC);
> +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
>      features |= (1 << VIRTIO_NET_F_CSUM);
>      features |= (1 << VIRTIO_NET_F_HOST_TSO4);
>      features |= (1 << VIRTIO_NET_F_HOST_TSO6);
> @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t
> cmd, {
>      struct virtio_net_ctrl_mac mac_data;
> 
> +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> +        elem->out_sg[1].iov_len == ETH_ALEN) {
> +        /* Set MAC address */
> +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> +        return VIRTIO_NET_OK;
> +    }
> +
>      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))
> diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> index d46fb98..9394cc0 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 */
> +

I wonder whether we need a DEFINE_PROP_BIT to disable and compat this feature. 
Consider we may migrate from a new version to an old version.
>  #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

Re: [QEMU PATCH v2] virtio-net: introduce a new macaddr control

[Michael S. Tsirkin]

On Wed, Jan 16, 2013 at 02:37:34PM +0800, Jason Wang wrote:
> On Wednesday, January 16, 2013 02:16:47 PM akong@redhat.com wrote:
> > 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 in one time.
> > 
> > VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> > V2: check guest's iov_len before memcpy
> > ---
> >  hw/virtio-net.c | 10 ++++++++++
> >  hw/virtio-net.h |  9 ++++++++-
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index dc7c6d6..d05f98f 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -247,6 +247,7 @@ static uint32_t virtio_net_get_features(VirtIODevice
> > *vdev, uint32_t features) VirtIONet *n = to_virtio_net(vdev);
> > 
> >      features |= (1 << VIRTIO_NET_F_MAC);
> > +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
> > 
> >      if (!peer_has_vnet_hdr(n)) {
> >          features &= ~(0x1 << VIRTIO_NET_F_CSUM);
> > @@ -282,6 +283,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice
> > *vdev) /* Linux kernel 2.6.25.  It understood MAC (as everyone must), * but
> > also these: */
> >      features |= (1 << VIRTIO_NET_F_MAC);
> > +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
> >      features |= (1 << VIRTIO_NET_F_CSUM);
> >      features |= (1 << VIRTIO_NET_F_HOST_TSO4);
> >      features |= (1 << VIRTIO_NET_F_HOST_TSO6);
> > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t
> > cmd, {
> >      struct virtio_net_ctrl_mac mac_data;
> > 
> > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > +        /* Set MAC address */
> > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > +        return VIRTIO_NET_OK;
> > +    }
> > +
> >      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))
> > diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> > index d46fb98..9394cc0 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 */
> > +
> 
> I wonder whether we need a DEFINE_PROP_BIT to disable and compat this feature. 
> Consider we may migrate from a new version to an old version.

Yes I think we do.

> >  #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

Re: [QEMU PATCH v2] virtio-net: introduce a new macaddr control

[Stefan Hajnoczi]

On Wed, Jan 16, 2013 at 02:37:34PM +0800, Jason Wang wrote:
> On Wednesday, January 16, 2013 02:16:47 PM akong@redhat.com wrote:
> > 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 in one time.
> > 
> > VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> > V2: check guest's iov_len before memcpy
> > ---
> >  hw/virtio-net.c | 10 ++++++++++
> >  hw/virtio-net.h |  9 ++++++++-
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index dc7c6d6..d05f98f 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -247,6 +247,7 @@ static uint32_t virtio_net_get_features(VirtIODevice
> > *vdev, uint32_t features) VirtIONet *n = to_virtio_net(vdev);
> > 
> >      features |= (1 << VIRTIO_NET_F_MAC);
> > +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
> > 
> >      if (!peer_has_vnet_hdr(n)) {
> >          features &= ~(0x1 << VIRTIO_NET_F_CSUM);
> > @@ -282,6 +283,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice
> > *vdev) /* Linux kernel 2.6.25.  It understood MAC (as everyone must), * but
> > also these: */
> >      features |= (1 << VIRTIO_NET_F_MAC);
> > +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
> >      features |= (1 << VIRTIO_NET_F_CSUM);
> >      features |= (1 << VIRTIO_NET_F_HOST_TSO4);
> >      features |= (1 << VIRTIO_NET_F_HOST_TSO6);
> > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t
> > cmd, {
> >      struct virtio_net_ctrl_mac mac_data;
> > 
> > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > +        /* Set MAC address */
> > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > +        return VIRTIO_NET_OK;
> > +    }
> > +
> >      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))
> > diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> > index d46fb98..9394cc0 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 */
> > +
> 
> I wonder whether we need a DEFINE_PROP_BIT to disable and compat this feature. 
> Consider we may migrate from a new version to an old version.

I agree, migration needs to be handled.  The bit should never change
while the device is initialized and running.  We should also never start
rejecting or ignoring the command if it was available before.

Stefan

Re: [QEMU PATCH v2] virtio-net: introduce a new macaddr control

[Michael S. Tsirkin]

On Wed, Jan 16, 2013 at 09:59:14AM +0100, Stefan Hajnoczi wrote:
> On Wed, Jan 16, 2013 at 02:37:34PM +0800, Jason Wang wrote:
> > On Wednesday, January 16, 2013 02:16:47 PM akong@redhat.com wrote:
> > > 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 in one time.
> > > 
> > > VIRTIO_NET_F_CTRL_MAC_ADDR is a new feature bit for compatibility.
> > > 
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > ---
> > > V2: check guest's iov_len before memcpy
> > > ---
> > >  hw/virtio-net.c | 10 ++++++++++
> > >  hw/virtio-net.h |  9 ++++++++-
> > >  2 files changed, 18 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > > index dc7c6d6..d05f98f 100644
> > > --- a/hw/virtio-net.c
> > > +++ b/hw/virtio-net.c
> > > @@ -247,6 +247,7 @@ static uint32_t virtio_net_get_features(VirtIODevice
> > > *vdev, uint32_t features) VirtIONet *n = to_virtio_net(vdev);
> > > 
> > >      features |= (1 << VIRTIO_NET_F_MAC);
> > > +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
> > > 
> > >      if (!peer_has_vnet_hdr(n)) {
> > >          features &= ~(0x1 << VIRTIO_NET_F_CSUM);
> > > @@ -282,6 +283,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice
> > > *vdev) /* Linux kernel 2.6.25.  It understood MAC (as everyone must), * but
> > > also these: */
> > >      features |= (1 << VIRTIO_NET_F_MAC);
> > > +    features |= (1 << VIRTIO_NET_F_CTRL_MAC_ADDR);
> > >      features |= (1 << VIRTIO_NET_F_CSUM);
> > >      features |= (1 << VIRTIO_NET_F_HOST_TSO4);
> > >      features |= (1 << VIRTIO_NET_F_HOST_TSO6);
> > > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t
> > > cmd, {
> > >      struct virtio_net_ctrl_mac mac_data;
> > > 
> > > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > > +        /* Set MAC address */
> > > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > > +        return VIRTIO_NET_OK;
> > > +    }
> > > +
> > >      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))
> > > diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> > > index d46fb98..9394cc0 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 */
> > > +
> > 
> > I wonder whether we need a DEFINE_PROP_BIT to disable and compat this feature. 
> > Consider we may migrate from a new version to an old version.
> 
> I agree, migration needs to be handled.  The bit should never change
> while the device is initialized and running.  We should also never start
> rejecting or ignoring the command if it was available before.
> 
> Stefan

It's the same for all feature bits. Soo all we need to do is
clear them when running with a compat machine type.

Re: [QEMU PATCH v2] virtio-net: introduce a new macaddr control

[Rusty Russell]

akong@redhat.com writes:
> @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>  {
>      struct virtio_net_ctrl_mac mac_data;
>  
> +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> +        elem->out_sg[1].iov_len == ETH_ALEN) {
> +        /* Set MAC address */
> +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> +        return VIRTIO_NET_OK;
> +    }

Does the rest of the net device still rely on the layout of descriptors?
If so, OK, we'll fix them all together.  If not, this introduces a new
one.

Cheers,
Rusty.

Re: [Qemu-devel] [QEMU PATCH v2] virtio-net: introduce a new macaddr control

[Amos Kong]

On Thu, Jan 17, 2013 at 11:49:20AM +1030, Rusty Russell wrote:
> akong@redhat.com writes:
> > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >  {
> >      struct virtio_net_ctrl_mac mac_data;
> >  
> > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > +        /* Set MAC address */
> > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > +        return VIRTIO_NET_OK;
> > +    }
> 
> Does the rest of the net device still rely on the layout of descriptors?

No, only info string of net client relies on n->mac

> If so, OK, we'll fix them all together.  If not, this introduces a new
> one.
> 
> Cheers,
> Rusty.

Re: [Qemu-devel] [QEMU PATCH v2] virtio-net: introduce a new macaddr control

[Amos Kong]

On Thu, Jan 17, 2013 at 01:45:11PM +0800, Amos Kong wrote:
> On Thu, Jan 17, 2013 at 11:49:20AM +1030, Rusty Russell wrote:
> > akong@redhat.com writes:
> > > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > >  {
> > >      struct virtio_net_ctrl_mac mac_data;
> > >  
> > > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > > +        /* Set MAC address */
> > > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > > +        return VIRTIO_NET_OK;
> > > +    }
> > 
> > Does the rest of the net device still rely on the layout of descriptors?
> 
> No, only info string of net client relies on n->mac

I misunderstood. There is no clear limitation of how much descriptor are
used for each vq command, but many commands rely on the layout of
descriptiors. eg:

virtio-net:
   VIRTIO_NET_CTRL_RX_PROMISC
   VIRTIO_NET_CTRL_RX_ALLMULTI
   VIRTIO_NET_CTRL_MAC_TABLE_SET
   etc
 
> > If so, OK, we'll fix them all together.  If not, this introduces a new
> > one.
> > 
> > Cheers,
> > Rusty.

Re: [Qemu-devel] [QEMU PATCH v2] virtio-net: introduce a new macaddr control

[Stefan Hajnoczi]

On Thu, Jan 17, 2013 at 01:45:11PM +0800, Amos Kong wrote:
> On Thu, Jan 17, 2013 at 11:49:20AM +1030, Rusty Russell wrote:
> > akong@redhat.com writes:
> > > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > >  {
> > >      struct virtio_net_ctrl_mac mac_data;
> > >  
> > > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > > +        /* Set MAC address */
> > > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > > +        return VIRTIO_NET_OK;
> > > +    }
> > 
> > Does the rest of the net device still rely on the layout of descriptors?
> 
> No, only info string of net client relies on n->mac

I think the question is whether the hw/virtio-net.c code makes
assumptions about virtqueue descriptor layout (e.g. sg[0] is the header,
sg[1] is the data buffer).

The answer is yes, the control virtqueue function directly accesses
iov[n].

Additional patches would be required to convert the existing
hw/virtio-net.c code to make no assumptions about virtqueue descriptor
layout.  It's outside the scope of this series.

Stefan

Re: [Qemu-devel] [QEMU PATCH v2] virtio-net: introduce a new macaddr control

[Michael S. Tsirkin]

On Thu, Jan 17, 2013 at 09:39:54AM +0100, Stefan Hajnoczi wrote:
> On Thu, Jan 17, 2013 at 01:45:11PM +0800, Amos Kong wrote:
> > On Thu, Jan 17, 2013 at 11:49:20AM +1030, Rusty Russell wrote:
> > > akong@redhat.com writes:
> > > > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > > >  {
> > > >      struct virtio_net_ctrl_mac mac_data;
> > > >  
> > > > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > > > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > > > +        /* Set MAC address */
> > > > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > > > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > > > +        return VIRTIO_NET_OK;
> > > > +    }
> > > 
> > > Does the rest of the net device still rely on the layout of descriptors?
> > 
> > No, only info string of net client relies on n->mac
> 
> I think the question is whether the hw/virtio-net.c code makes
> assumptions about virtqueue descriptor layout (e.g. sg[0] is the header,
> sg[1] is the data buffer).
> 
> The answer is yes, the control virtqueue function directly accesses
> iov[n].
> 
> Additional patches would be required to convert the existing
> hw/virtio-net.c code to make no assumptions about virtqueue descriptor
> layout.  It's outside the scope of this series.
> 
> Stefan

It's not hard at all though - the harder part is data path
processing, this has been done already. Will send a
patch shortly.

Re: [QEMU PATCH v2] virtio-net: introduce a new macaddr control

[Michael S. Tsirkin]

On Thu, Jan 17, 2013 at 11:49:20AM +1030, Rusty Russell wrote:
> akong@redhat.com writes:
> > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >  {
> >      struct virtio_net_ctrl_mac mac_data;
> >  
> > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > +        /* Set MAC address */
> > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > +        return VIRTIO_NET_OK;
> > +    }
> 
> Does the rest of the net device still rely on the layout of descriptors?
> If so, OK, we'll fix them all together.  If not, this introduces a new
> one.
> 
> Cheers,
> Rusty.

The following fixes all existing users.
Got to deal with some urgent stuff so did not test yet -
Amos, would you like to include this in your patchset
and build on it, test it all together?
If not I'll get to it next week.

--->

virtio-net: remove layout assumptions for ctrl vq

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 4d80a25..5d1e084 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -316,44 +316,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;
@@ -361,54 +361,64 @@ 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 (s != sizeof mac_data.entries) {
+        return VIRTIO_NET_ERR;
+    }
+
+    iov_discard_front(&iov, &iov_cnt, s);
+    assert(s == sizeof mac_data.entries);
 
-    if (sizeof(mac_data.entries) +
-        (mac_data.entries * ETH_ALEN) > elem->out_sg[1].iov_len)
+    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);
         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 (s != sizeof mac_data.entries) {
+        return VIRTIO_NET_ERR;
+    }
+
+    iov_discard_front(&iov, &iov_cnt, s);
+    assert(s == sizeof mac_data.entries);
 
-    if (sizeof(mac_data.entries) +
-        (mac_data.entries * ETH_ALEN) > elem->out_sg[2].iov_len)
+    if (mac_data.entries * ETH_ALEN > iov_size(iov, iov_cnt)) {
         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;
-        }
+    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);
+        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;
 
@@ -428,30 +438,32 @@ 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) < 1) {
             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);

Re: [QEMU PATCH v2] virtio-net: introduce a new macaddr control

[Amos Kong]

On Thu, Jan 17, 2013 at 02:13:08PM +0200, Michael S. Tsirkin wrote:
> On Thu, Jan 17, 2013 at 11:49:20AM +1030, Rusty Russell wrote:
> > akong@redhat.com writes:
> > > @@ -349,6 +351,14 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> > >  {
> > >      struct virtio_net_ctrl_mac mac_data;
> > >  
> > > +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET && elem->out_num == 2 &&
> > > +        elem->out_sg[1].iov_len == ETH_ALEN) {
> > > +        /* Set MAC address */
> > > +        memcpy(n->mac, elem->out_sg[1].iov_base, elem->out_sg[1].iov_len);
> > > +        qemu_format_nic_info_str(&n->nic->nc, n->mac);
> > > +        return VIRTIO_NET_OK;
> > > +    }
> > 
> > Does the rest of the net device still rely on the layout of descriptors?
> > If so, OK, we'll fix them all together.  If not, this introduces a new
> > one.
> > 
> > Cheers,
> > Rusty.
> 
> The following fixes all existing users.
> Got to deal with some urgent stuff so did not test yet -
> Amos, would you like to include this in your patchset
> and build on it, test it all together?

No problem.

> If not I'll get to it next week.
> 
> --->
> 
> virtio-net: remove layout assumptions for ctrl vq
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

...