diff mbox

[RFC,2/2] virtio-net: introduce a new control to set macaddr

Message ID 1357829141-25455-3-git-send-email-akong@redhat.com
State New
Headers show

Commit Message

Amos Kong Jan. 10, 2013, 2:45 p.m. UTC 
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        | 16 +++++++++++++++-
 include/uapi/linux/virtio_net.h |  8 +++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

Comments

Jason Wang Jan. 10, 2013, 2:57 p.m. UTC | #1 
On 01/10/2013 10:45 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        | 16 +++++++++++++++-
>  include/uapi/linux/virtio_net.h |  8 +++++++-
>  2 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 395ab4f..ff22bcd 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -803,13 +803,26 @@ static int virtnet_set_mac_address(struct net_device *dev, void *p)
>  	struct virtio_device *vdev = vi->vdev;
>  	int ret;
>  
> +	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)) {
> +		/* Set MAC address by sending vq command */
> +		sg_init_one(&sg, dev->dev_addr, dev->addr_len);
> +		virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> +				     VIRTIO_NET_CTRL_MAC_ADDR_SET,
> +				     &sg, 1, 0);
> +		return 0;
> +	}
> +

Better to check the return of virtnet_send_command() and give a warn
like other command. Btw, need we fail back to try the old way then?
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
> +		/* Set MAC address by writing config space */
>  		vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
>  		                  dev->dev_addr, dev->addr_len);
> +	}
>  
>  	return 0;
>  }
> @@ -1627,6 +1640,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
Michael S. Tsirkin Jan. 10, 2013, 3:26 p.m. UTC | #2 
On Thu, Jan 10, 2013 at 10:45:41PM +0800, 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        | 16 +++++++++++++++-
>  include/uapi/linux/virtio_net.h |  8 +++++++-
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 395ab4f..ff22bcd 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -803,13 +803,26 @@ static int virtnet_set_mac_address(struct net_device *dev, void *p)
>  	struct virtio_device *vdev = vi->vdev;
>  	int ret;
>  
> +	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)) {
> +		/* Set MAC address by sending vq command */
> +		sg_init_one(&sg, dev->dev_addr, dev->addr_len);
> +		virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> +				     VIRTIO_NET_CTRL_MAC_ADDR_SET,
> +				     &sg, 1, 0);

I think we should check status and return an error if the command failed.
Also, delay eth_mac_addr until after it passes.

> +		return 0;
> +	}
> +
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
> +		/* Set MAC address by writing config space */
>  		vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
>  		                  dev->dev_addr, dev->addr_len);
> +	}
>  

By the way, why don't we fail the command if VIRTIO_NET_F_MAC is off?
Rusty?

>  	return 0;
>  }
> @@ -1627,6 +1640,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
Rusty Russell Jan. 11, 2013, 12:43 a.m. UTC | #3 
"Michael S. Tsirkin" <mst@redhat.com> writes:
>> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
>> +		/* Set MAC address by writing config space */
>>  		vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
>>  		                  dev->dev_addr, dev->addr_len);
>> +	}
>>  
>
> By the way, why don't we fail the command if VIRTIO_NET_F_MAC is off?
> Rusty?

Looked back through the history for this one.  I think the theory is that
if the guest doesn't set VIRTIO_NET_F_MAC, it means it doesn't care:
it's up to the guest.

Cheers,
Rusty.
Amos Kong Jan. 16, 2013, 5:23 a.m. UTC | #4 
On Thu, Jan 10, 2013 at 10:57:05PM +0800, Jason Wang wrote:
> On 01/10/2013 10:45 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        | 16 +++++++++++++++-
> >  include/uapi/linux/virtio_net.h |  8 +++++++-
> >  2 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 395ab4f..ff22bcd 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -803,13 +803,26 @@ static int virtnet_set_mac_address(struct net_device *dev, void *p)
> >  	struct virtio_device *vdev = vi->vdev;
> >  	int ret;
> >  
> > +	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)) {
> > +		/* Set MAC address by sending vq command */
> > +		sg_init_one(&sg, dev->dev_addr, dev->addr_len);
> > +		virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> > +				     VIRTIO_NET_CTRL_MAC_ADDR_SET,
> > +				     &sg, 1, 0);
> > +		return 0;
> > +	}
> > +
> 
> Better to check the return of virtnet_send_command() and give a warn
> like other command. Btw, need we fail back to try the old way then?

Yes, it's necessary to check the return value of
virtnet_send_command().

In fail case, I like to return -EINVAL to userspace, because we don't
only want to set mac successfully, we also want to resolve the
addr inconsistent issue by this feature (vq cmd).
Michael S. Tsirkin Jan. 16, 2013, 9:17 a.m. UTC | #5 
On Wed, Jan 16, 2013 at 01:23:26PM +0800, Amos Kong wrote:
> On Thu, Jan 10, 2013 at 10:57:05PM +0800, Jason Wang wrote:
> > On 01/10/2013 10:45 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        | 16 +++++++++++++++-
> > >  include/uapi/linux/virtio_net.h |  8 +++++++-
> > >  2 files changed, 22 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 395ab4f..ff22bcd 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -803,13 +803,26 @@ static int virtnet_set_mac_address(struct net_device *dev, void *p)
> > >  	struct virtio_device *vdev = vi->vdev;
> > >  	int ret;
> > >  
> > > +	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)) {
> > > +		/* Set MAC address by sending vq command */
> > > +		sg_init_one(&sg, dev->dev_addr, dev->addr_len);
> > > +		virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> > > +				     VIRTIO_NET_CTRL_MAC_ADDR_SET,
> > > +				     &sg, 1, 0);
> > > +		return 0;
> > > +	}
> > > +
> > 
> > Better to check the return of virtnet_send_command() and give a warn
> > like other command. Btw, need we fail back to try the old way then?
> 
> Yes, it's necessary to check the return value of
> virtnet_send_command().
> 
> In fail case, I like to return -EINVAL to userspace, because we don't
> only want to set mac successfully, we also want to resolve the
> addr inconsistent issue by this feature (vq cmd).

It's really a device error but I guess we can.
We probably should print a warning too.
diff mbox

Patch

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 395ab4f..ff22bcd 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -803,13 +803,26 @@  static int virtnet_set_mac_address(struct net_device *dev, void *p)
 	struct virtio_device *vdev = vi->vdev;
 	int ret;
 
+	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)) {
+		/* Set MAC address by sending vq command */
+		sg_init_one(&sg, dev->dev_addr, dev->addr_len);
+		virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
+				     VIRTIO_NET_CTRL_MAC_ADDR_SET,
+				     &sg, 1, 0);
+		return 0;
+	}
+
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
+		/* Set MAC address by writing config space */
 		vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
 		                  dev->dev_addr, dev->addr_len);
+	}
 
 	return 0;
 }
@@ -1627,6 +1640,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