* [PATCH v2 0/2] make mac programming for virtio net more robust @ 2013-01-16 5:56 akong 2013-01-16 5:57 ` [PATCH v2 1/2] move virtnet_send_command() above virtnet_set_mac_address() akong 2013-01-16 5:57 ` [PATCH v2 2/2] virtio-net: introduce a new control to set macaddr akong 0 siblings, 2 replies; 6+ messages in thread From: akong @ 2013-01-16 5:56 UTC (permalink / raw) To: mst; +Cc: kvm, netdev, qemu-devel, virtualization, davem 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] move virtnet_send_command() above virtnet_set_mac_address() 2013-01-16 5:56 [PATCH v2 0/2] make mac programming for virtio net more robust akong @ 2013-01-16 5:57 ` akong 2013-01-16 5:57 ` [PATCH v2 2/2] virtio-net: introduce a new control to set macaddr akong 1 sibling, 0 replies; 6+ messages in thread From: akong @ 2013-01-16 5:57 UTC (permalink / raw) To: mst; +Cc: rusty, qemu-devel, kvm, virtualization, netdev, davem 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] virtio-net: introduce a new control to set macaddr 2013-01-16 5:56 [PATCH v2 0/2] make mac programming for virtio net more robust akong 2013-01-16 5:57 ` [PATCH v2 1/2] move virtnet_send_command() above virtnet_set_mac_address() akong @ 2013-01-16 5:57 ` akong 2013-01-16 6:20 ` Jason Wang 1 sibling, 1 reply; 6+ messages in thread From: akong @ 2013-01-16 5:57 UTC (permalink / raw) To: mst; +Cc: rusty, qemu-devel, kvm, virtualization, netdev, davem 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] virtio-net: introduce a new control to set macaddr 2013-01-16 5:57 ` [PATCH v2 2/2] virtio-net: introduce a new control to set macaddr akong @ 2013-01-16 6:20 ` Jason Wang 2013-01-16 8:24 ` Amos Kong 0 siblings, 1 reply; 6+ messages in thread From: Jason Wang @ 2013-01-16 6:20 UTC (permalink / raw) To: virtualization; +Cc: kvm, mst, netdev, qemu-devel, davem 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] virtio-net: introduce a new control to set macaddr 2013-01-16 6:20 ` Jason Wang @ 2013-01-16 8:24 ` Amos Kong 2013-01-16 8:36 ` Michael S. Tsirkin 0 siblings, 1 reply; 6+ messages in thread From: Amos Kong @ 2013-01-16 8:24 UTC (permalink / raw) To: Jason Wang; +Cc: virtualization, mst, kvm, netdev, qemu-devel, davem 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; > > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] virtio-net: introduce a new control to set macaddr 2013-01-16 8:24 ` Amos Kong @ 2013-01-16 8:36 ` Michael S. Tsirkin 0 siblings, 0 replies; 6+ messages in thread From: Michael S. Tsirkin @ 2013-01-16 8:36 UTC (permalink / raw) To: Amos Kong; +Cc: kvm, netdev, qemu-devel, virtualization, davem 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; > > > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-01-16 8:36 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-16 5:56 [PATCH v2 0/2] make mac programming for virtio net more robust akong 2013-01-16 5:57 ` [PATCH v2 1/2] move virtnet_send_command() above virtnet_set_mac_address() akong 2013-01-16 5:57 ` [PATCH v2 2/2] virtio-net: introduce a new control to set macaddr akong 2013-01-16 6:20 ` Jason Wang 2013-01-16 8:24 ` Amos Kong 2013-01-16 8:36 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).