* [PATCH v3 0/2] make mac programming for virtio net more robust
@ 2013-01-17 10:40 ` akong
0 siblings, 0 replies; 12+ messages in thread
From: akong @ 2013-01-17 10:40 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, it's atomic.
V2: check return of sending command, delay eth_mac_addr()
V3: restore software address when fail to set hardware address
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 | 110 +++++++++++++++++++++++-----------------
include/uapi/linux/virtio_net.h | 8 ++-
2 files changed, 71 insertions(+), 47 deletions(-)
--
1.7.11.7
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v3 0/2] make mac programming for virtio net more robust
@ 2013-01-17 10:40 ` akong
0 siblings, 0 replies; 12+ messages in thread
From: akong @ 2013-01-17 10:40 UTC (permalink / raw)
To: mst; +Cc: kvm, netdev, rusty, 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, it's atomic.
V2: check return of sending command, delay eth_mac_addr()
V3: restore software address when fail to set hardware address
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 | 110 +++++++++++++++++++++++-----------------
include/uapi/linux/virtio_net.h | 8 ++-
2 files changed, 71 insertions(+), 47 deletions(-)
--
1.7.11.7
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 1/2] move virtnet_send_command() above virtnet_set_mac_address()
2013-01-17 10:40 ` [Qemu-devel] " akong
@ 2013-01-17 10:40 ` akong
-1 siblings, 0 replies; 12+ messages in thread
From: akong @ 2013-01-17 10:40 UTC (permalink / raw)
To: mst; +Cc: kvm, netdev, qemu-devel, virtualization, 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] 12+ messages in thread
* [Qemu-devel] [PATCH v3 1/2] move virtnet_send_command() above virtnet_set_mac_address()
@ 2013-01-17 10:40 ` akong
0 siblings, 0 replies; 12+ messages in thread
From: akong @ 2013-01-17 10:40 UTC (permalink / raw)
To: mst; +Cc: kvm, netdev, rusty, qemu-devel, virtualization, 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] 12+ messages in thread
* [PATCH v3 2/2] virtio-net: introduce a new control to set macaddr
2013-01-17 10:40 ` [Qemu-devel] " akong
@ 2013-01-17 10:40 ` akong
-1 siblings, 0 replies; 12+ messages in thread
From: akong @ 2013-01-17 10:40 UTC (permalink / raw)
To: mst; +Cc: kvm, netdev, qemu-devel, virtualization, 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,
it's atomic.
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 | 21 ++++++++++++++++++++-
include/uapi/linux/virtio_net.h | 8 +++++++-
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 395ab4f..837c978 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -802,14 +802,32 @@ 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 scatterlist sg;
+ char save_addr[ETH_ALEN];
+ unsigned char save_aatype;
+
+ memcpy(save_addr, dev->dev_addr, ETH_ALEN);
+ save_aatype = dev->addr_assign_type;
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, dev->dev_addr, 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");
+ memcpy(dev->dev_addr, save_addr, ETH_ALEN);
+ dev->addr_assign_type = save_aatype;
+ 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);
+ }
return 0;
}
@@ -1627,6 +1645,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] 12+ messages in thread
* [Qemu-devel] [PATCH v3 2/2] virtio-net: introduce a new control to set macaddr
@ 2013-01-17 10:40 ` akong
0 siblings, 0 replies; 12+ messages in thread
From: akong @ 2013-01-17 10:40 UTC (permalink / raw)
To: mst; +Cc: kvm, netdev, rusty, qemu-devel, virtualization, 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,
it's atomic.
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 | 21 ++++++++++++++++++++-
include/uapi/linux/virtio_net.h | 8 +++++++-
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 395ab4f..837c978 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -802,14 +802,32 @@ 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 scatterlist sg;
+ char save_addr[ETH_ALEN];
+ unsigned char save_aatype;
+
+ memcpy(save_addr, dev->dev_addr, ETH_ALEN);
+ save_aatype = dev->addr_assign_type;
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, dev->dev_addr, 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");
+ memcpy(dev->dev_addr, save_addr, ETH_ALEN);
+ dev->addr_assign_type = save_aatype;
+ 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);
+ }
return 0;
}
@@ -1627,6 +1645,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] 12+ messages in thread
* Re: [PATCH v3 2/2] virtio-net: introduce a new control to set macaddr
2013-01-17 10:40 ` [Qemu-devel] " akong
@ 2013-01-18 11:00 ` Stefan Hajnoczi
-1 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2013-01-18 11:00 UTC (permalink / raw)
To: akong; +Cc: mst, kvm, netdev, qemu-devel, virtualization, davem
On Thu, Jan 17, 2013 at 06:40:12PM +0800, akong@redhat.com wrote:
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 395ab4f..837c978 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -802,14 +802,32 @@ 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 scatterlist sg;
> + char save_addr[ETH_ALEN];
> + unsigned char save_aatype;
> +
> + memcpy(save_addr, dev->dev_addr, ETH_ALEN);
> + save_aatype = dev->addr_assign_type;
>
> 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, dev->dev_addr, 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");
> + memcpy(dev->dev_addr, save_addr, ETH_ALEN);
> + dev->addr_assign_type = save_aatype;
> + return -EINVAL;
> + }
eth_mac_addr() doesn't allow callers to implement error handling nicely.
Although you didn't duplicate it's code directly, this patch still leaks
internals of eth_mac_addr().
How about splitting eth_mac_addr() in a separate patch:
int eth_prepare_mac_addr_change(struct net_device *dev, void *p)
{
struct sockaddr *addr = p;
if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev))
return -EBUSY;
if (!is_valid_ether_addr(addr->sa_data))
return -EADDRNOTAVAIL;
return 0;
}
void eth_commit_mac_addr_change(struct net_device *dev, void *p)
{
struct sockaddr *addr = p;
memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
/* if device marked as NET_ADDR_RANDOM, reset it */
dev->addr_assign_type &= ~NET_ADDR_RANDOM;
}
/* Default implementation of MAC address changing */
int eth_mac_addr(struct net_device *dev, void *p)
{
int ret;
ret = eth_prepare_mac_addr_change(dev, p);
if (ret < 0)
return ret;
eth_commit_mac_addr_change(dev, p);
return 0;
}
Now virtio_net.c does:
ret = eth_prepare_mac_addr_change(dev, p);
if (ret < 0)
return ret;
if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
sg_init_one(&sg, dev->dev_addr, 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;
}
} ...
eth_commit_mac_addr_change(dev, p);
return 0;
Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] virtio-net: introduce a new control to set macaddr
@ 2013-01-18 11:00 ` Stefan Hajnoczi
0 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2013-01-18 11:00 UTC (permalink / raw)
To: akong; +Cc: kvm, mst, netdev, qemu-devel, virtualization, davem
On Thu, Jan 17, 2013 at 06:40:12PM +0800, akong@redhat.com wrote:
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 395ab4f..837c978 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -802,14 +802,32 @@ 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 scatterlist sg;
> + char save_addr[ETH_ALEN];
> + unsigned char save_aatype;
> +
> + memcpy(save_addr, dev->dev_addr, ETH_ALEN);
> + save_aatype = dev->addr_assign_type;
>
> 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, dev->dev_addr, 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");
> + memcpy(dev->dev_addr, save_addr, ETH_ALEN);
> + dev->addr_assign_type = save_aatype;
> + return -EINVAL;
> + }
eth_mac_addr() doesn't allow callers to implement error handling nicely.
Although you didn't duplicate it's code directly, this patch still leaks
internals of eth_mac_addr().
How about splitting eth_mac_addr() in a separate patch:
int eth_prepare_mac_addr_change(struct net_device *dev, void *p)
{
struct sockaddr *addr = p;
if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev))
return -EBUSY;
if (!is_valid_ether_addr(addr->sa_data))
return -EADDRNOTAVAIL;
return 0;
}
void eth_commit_mac_addr_change(struct net_device *dev, void *p)
{
struct sockaddr *addr = p;
memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
/* if device marked as NET_ADDR_RANDOM, reset it */
dev->addr_assign_type &= ~NET_ADDR_RANDOM;
}
/* Default implementation of MAC address changing */
int eth_mac_addr(struct net_device *dev, void *p)
{
int ret;
ret = eth_prepare_mac_addr_change(dev, p);
if (ret < 0)
return ret;
eth_commit_mac_addr_change(dev, p);
return 0;
}
Now virtio_net.c does:
ret = eth_prepare_mac_addr_change(dev, p);
if (ret < 0)
return ret;
if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
sg_init_one(&sg, dev->dev_addr, 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;
}
} ...
eth_commit_mac_addr_change(dev, p);
return 0;
Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] virtio-net: introduce a new control to set macaddr
2013-01-17 10:40 ` [Qemu-devel] " akong
(?)
(?)
@ 2013-01-18 11:00 ` Stefan Hajnoczi
-1 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2013-01-18 11:00 UTC (permalink / raw)
To: akong; +Cc: kvm, mst, netdev, qemu-devel, virtualization, davem
On Thu, Jan 17, 2013 at 06:40:12PM +0800, akong@redhat.com wrote:
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 395ab4f..837c978 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -802,14 +802,32 @@ 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 scatterlist sg;
> + char save_addr[ETH_ALEN];
> + unsigned char save_aatype;
> +
> + memcpy(save_addr, dev->dev_addr, ETH_ALEN);
> + save_aatype = dev->addr_assign_type;
>
> 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, dev->dev_addr, 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");
> + memcpy(dev->dev_addr, save_addr, ETH_ALEN);
> + dev->addr_assign_type = save_aatype;
> + return -EINVAL;
> + }
eth_mac_addr() doesn't allow callers to implement error handling nicely.
Although you didn't duplicate it's code directly, this patch still leaks
internals of eth_mac_addr().
How about splitting eth_mac_addr() in a separate patch:
int eth_prepare_mac_addr_change(struct net_device *dev, void *p)
{
struct sockaddr *addr = p;
if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev))
return -EBUSY;
if (!is_valid_ether_addr(addr->sa_data))
return -EADDRNOTAVAIL;
return 0;
}
void eth_commit_mac_addr_change(struct net_device *dev, void *p)
{
struct sockaddr *addr = p;
memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
/* if device marked as NET_ADDR_RANDOM, reset it */
dev->addr_assign_type &= ~NET_ADDR_RANDOM;
}
/* Default implementation of MAC address changing */
int eth_mac_addr(struct net_device *dev, void *p)
{
int ret;
ret = eth_prepare_mac_addr_change(dev, p);
if (ret < 0)
return ret;
eth_commit_mac_addr_change(dev, p);
return 0;
}
Now virtio_net.c does:
ret = eth_prepare_mac_addr_change(dev, p);
if (ret < 0)
return ret;
if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
sg_init_one(&sg, dev->dev_addr, 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;
}
} ...
eth_commit_mac_addr_change(dev, p);
return 0;
Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] virtio-net: introduce a new control to set macaddr
2013-01-18 11:00 ` [Qemu-devel] " Stefan Hajnoczi
@ 2013-01-20 2:20 ` Amos Kong
-1 siblings, 0 replies; 12+ messages in thread
From: Amos Kong @ 2013-01-20 2:20 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kvm, mst, netdev, qemu-devel, virtualization, davem
On Fri, Jan 18, 2013 at 12:00:42PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jan 17, 2013 at 06:40:12PM +0800, akong@redhat.com wrote:
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 395ab4f..837c978 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -802,14 +802,32 @@ 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 scatterlist sg;
> > + char save_addr[ETH_ALEN];
> > + unsigned char save_aatype;
> > +
> > + memcpy(save_addr, dev->dev_addr, ETH_ALEN);
> > + save_aatype = dev->addr_assign_type;
> >
> > 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, dev->dev_addr, 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");
> > + memcpy(dev->dev_addr, save_addr, ETH_ALEN);
> > + dev->addr_assign_type = save_aatype;
> > + return -EINVAL;
> > + }
>
> eth_mac_addr() doesn't allow callers to implement error handling nicely.
> Although you didn't duplicate it's code directly, this patch still leaks
> internals of eth_mac_addr().
>
> How about splitting eth_mac_addr() in a separate patch:
Agree, then we can have nice error handling.
I will send a V4 and include this patch.
Thanks.
> int eth_prepare_mac_addr_change(struct net_device *dev, void *p)
> {
> struct sockaddr *addr = p;
> if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev))
> return -EBUSY;
> if (!is_valid_ether_addr(addr->sa_data))
> return -EADDRNOTAVAIL;
> return 0;
> }
...
> Now virtio_net.c does:
>
> ret = eth_prepare_mac_addr_change(dev, p);
> if (ret < 0)
> return ret;
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
> sg_init_one(&sg, dev->dev_addr, dev->addr_len);
trivial issue, %s/dev->dev_addr/addr->sa_data
> 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;
> }
> } ...
>
> eth_commit_mac_addr_change(dev, p);
> return 0;
>
> Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] virtio-net: introduce a new control to set macaddr
@ 2013-01-20 2:20 ` Amos Kong
0 siblings, 0 replies; 12+ messages in thread
From: Amos Kong @ 2013-01-20 2:20 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kvm, mst, netdev, qemu-devel, virtualization, davem
On Fri, Jan 18, 2013 at 12:00:42PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jan 17, 2013 at 06:40:12PM +0800, akong@redhat.com wrote:
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 395ab4f..837c978 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -802,14 +802,32 @@ 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 scatterlist sg;
> > + char save_addr[ETH_ALEN];
> > + unsigned char save_aatype;
> > +
> > + memcpy(save_addr, dev->dev_addr, ETH_ALEN);
> > + save_aatype = dev->addr_assign_type;
> >
> > 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, dev->dev_addr, 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");
> > + memcpy(dev->dev_addr, save_addr, ETH_ALEN);
> > + dev->addr_assign_type = save_aatype;
> > + return -EINVAL;
> > + }
>
> eth_mac_addr() doesn't allow callers to implement error handling nicely.
> Although you didn't duplicate it's code directly, this patch still leaks
> internals of eth_mac_addr().
>
> How about splitting eth_mac_addr() in a separate patch:
Agree, then we can have nice error handling.
I will send a V4 and include this patch.
Thanks.
> int eth_prepare_mac_addr_change(struct net_device *dev, void *p)
> {
> struct sockaddr *addr = p;
> if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev))
> return -EBUSY;
> if (!is_valid_ether_addr(addr->sa_data))
> return -EADDRNOTAVAIL;
> return 0;
> }
...
> Now virtio_net.c does:
>
> ret = eth_prepare_mac_addr_change(dev, p);
> if (ret < 0)
> return ret;
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
> sg_init_one(&sg, dev->dev_addr, dev->addr_len);
trivial issue, %s/dev->dev_addr/addr->sa_data
> 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;
> }
> } ...
>
> eth_commit_mac_addr_change(dev, p);
> return 0;
>
> Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] virtio-net: introduce a new control to set macaddr
2013-01-18 11:00 ` [Qemu-devel] " Stefan Hajnoczi
(?)
@ 2013-01-20 2:20 ` Amos Kong
-1 siblings, 0 replies; 12+ messages in thread
From: Amos Kong @ 2013-01-20 2:20 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kvm, mst, netdev, qemu-devel, virtualization, davem
On Fri, Jan 18, 2013 at 12:00:42PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jan 17, 2013 at 06:40:12PM +0800, akong@redhat.com wrote:
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 395ab4f..837c978 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -802,14 +802,32 @@ 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 scatterlist sg;
> > + char save_addr[ETH_ALEN];
> > + unsigned char save_aatype;
> > +
> > + memcpy(save_addr, dev->dev_addr, ETH_ALEN);
> > + save_aatype = dev->addr_assign_type;
> >
> > 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, dev->dev_addr, 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");
> > + memcpy(dev->dev_addr, save_addr, ETH_ALEN);
> > + dev->addr_assign_type = save_aatype;
> > + return -EINVAL;
> > + }
>
> eth_mac_addr() doesn't allow callers to implement error handling nicely.
> Although you didn't duplicate it's code directly, this patch still leaks
> internals of eth_mac_addr().
>
> How about splitting eth_mac_addr() in a separate patch:
Agree, then we can have nice error handling.
I will send a V4 and include this patch.
Thanks.
> int eth_prepare_mac_addr_change(struct net_device *dev, void *p)
> {
> struct sockaddr *addr = p;
> if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev))
> return -EBUSY;
> if (!is_valid_ether_addr(addr->sa_data))
> return -EADDRNOTAVAIL;
> return 0;
> }
...
> Now virtio_net.c does:
>
> ret = eth_prepare_mac_addr_change(dev, p);
> if (ret < 0)
> return ret;
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
> sg_init_one(&sg, dev->dev_addr, dev->addr_len);
trivial issue, %s/dev->dev_addr/addr->sa_data
> 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;
> }
> } ...
>
> eth_commit_mac_addr_change(dev, p);
> return 0;
>
> Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-01-20 3:15 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-17 10:40 [PATCH v3 0/2] make mac programming for virtio net more robust akong
2013-01-17 10:40 ` [Qemu-devel] " akong
2013-01-17 10:40 ` [PATCH v3 1/2] move virtnet_send_command() above virtnet_set_mac_address() akong
2013-01-17 10:40 ` [Qemu-devel] " akong
2013-01-17 10:40 ` [PATCH v3 2/2] virtio-net: introduce a new control to set macaddr akong
2013-01-17 10:40 ` [Qemu-devel] " akong
2013-01-18 11:00 ` Stefan Hajnoczi
2013-01-18 11:00 ` [Qemu-devel] " Stefan Hajnoczi
2013-01-20 2:20 ` Amos Kong
2013-01-20 2:20 ` Amos Kong
2013-01-20 2:20 ` [Qemu-devel] " Amos Kong
2013-01-18 11:00 ` Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.