| Message ID | 1231881797.9095.187.camel@bling (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Tue, 2009-01-13 at 14:23 -0700, Alex Williamson wrote: > This will be used for RX mode, MAC filter table, VLAN filtering, etc... Looks very reasonable. I'm a bit wary of send_command() being synchronous, but it probably makes sense. Could do with some details in the commit log as to why this approach was chosen over increasing virtio_net_config. > Signed-off-by: Alex Williamson <alex.williamson@hp.com> > --- > > drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++++++++- > include/linux/virtio_net.h | 3 ++ > 2 files changed, 57 insertions(+), 1 deletions(-) > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index e7700de..de348de 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -39,7 +39,7 @@ module_param(gso, bool, 0444); > struct virtnet_info > { > struct virtio_device *vdev; > - struct virtqueue *rvq, *svq; > + struct virtqueue *rvq, *svq, *cvq; > struct net_device *dev; > struct napi_struct napi; > > @@ -603,6 +603,47 @@ static int virtnet_open(struct net_device *dev) > return 0; > } > > +static int virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > + void *data, unsigned int len) > +{ > + struct scatterlist sg[3]; > + struct { > + u8 class; > + u8 cmd; > + } ctrl_cmd; I'd like to see this defined in virtio_net_hdr. Why the need for class/cmd? Why not just a single 16 bit command field? > + u8 ctrl_status; > + unsigned int tmp; > + int i = 0; > + > + if (!vi->cvq) > + return -EFAULT; BUG_ON() probably makes more sense here. > + > + sg_init_table(sg, len ? 3 : 2); > + > + sg_set_buf(&sg[i++], &ctrl_cmd, sizeof(ctrl_cmd)); > + if (len) > + sg_set_buf(&sg[i++], data, len); > + sg_set_buf(&sg[i], &ctrl_status, sizeof(ctrl_status)); > + > + ctrl_cmd.class = class; > + ctrl_cmd.cmd = cmd; > + > + ctrl_status = ~0; > + > + if (vi->cvq->vq_ops->add_buf(vi->cvq, sg, i, 1, vi) != 0) > + BUG(); > + > + vi->cvq->vq_ops->kick(vi->cvq); > + > + while (!vi->cvq->vq_ops->get_buf(vi->cvq, &tmp)) > + cpu_relax(); > + > + if (ctrl_status == VIRTIO_NET_OK) > + return 0; > + else > + return -EFAULT; Could be all on one line: return ctrl_status ? -EFAULT : 0; Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2009-01-14 at 10:15 +0000, Mark McLoughlin wrote: > On Tue, 2009-01-13 at 14:23 -0700, Alex Williamson wrote: > > This will be used for RX mode, MAC filter table, VLAN filtering, etc... > > Looks very reasonable. I'm a bit wary of send_command() being > synchronous, but it probably makes sense. Yes, I tried to make it async, but quickly ran into problems sleeping when I wasn't allowed to. Using it in this way does pretty much limit it to an outbound channel though... all worthy of commit log comments. > > +static int virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > > + void *data, unsigned int len) > > +{ > > + struct scatterlist sg[3]; > > + struct { > > + u8 class; > > + u8 cmd; > > + } ctrl_cmd; > > I'd like to see this defined in virtio_net_hdr. As part of struct virtio_net_hdr? I'm not sure what that'd buy us and would likely break compatibility. Or do you simply mean defined in virtio-net.h? > Why the need for class/cmd? Why not just a single 16 bit command field? It seemed like a good way to logically divide up an address space and makes it easy for the backend to break up the code so it doesn't become a huge table. > > + u8 ctrl_status; > > + unsigned int tmp; > > + int i = 0; > > + > > + if (!vi->cvq) > > + return -EFAULT; > > BUG_ON() probably makes more sense here. This is to allow a newer virtio_net guest driver to run on an old qemu. That's why I don't generate a fatal error if we don't find the control queue. In that case the backend will be running in promiscuous mode and I think all of these commands can safely fail. Thanks for the comments. Alex
On Wed, 2009-01-14 at 09:01 -0700, Alex Williamson wrote: > On Wed, 2009-01-14 at 10:15 +0000, Mark McLoughlin wrote: > > On Tue, 2009-01-13 at 14:23 -0700, Alex Williamson wrote: > > > +static int virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, > > > + void *data, unsigned int len) > > > +{ > > > + struct scatterlist sg[3]; > > > + struct { > > > + u8 class; > > > + u8 cmd; > > > + } ctrl_cmd; > > > > I'd like to see this defined in virtio_net_hdr. > > As part of struct virtio_net_hdr? I'm not sure what that'd buy us and > would likely break compatibility. Or do you simply mean defined in > virtio-net.h? Yep, sorry, I meant virtio-net.h > > > + u8 ctrl_status; > > > + unsigned int tmp; > > > + int i = 0; > > > + > > > + if (!vi->cvq) > > > + return -EFAULT; > > > > BUG_ON() probably makes more sense here. > > This is to allow a newer virtio_net guest driver to run on an old qemu. > That's why I don't generate a fatal error if we don't find the control > queue. In that case the backend will be running in promiscuous mode and > I think all of these commands can safely fail. Thanks for the comments. Yep, but what I mean is that virtio_net_send_command() should never be called if the host doesn't have send queue support - i.e. force the calling code to think about what should be done on older hosts. Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index e7700de..de348de 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -39,7 +39,7 @@ module_param(gso, bool, 0444); struct virtnet_info { struct virtio_device *vdev; - struct virtqueue *rvq, *svq; + struct virtqueue *rvq, *svq, *cvq; struct net_device *dev; struct napi_struct napi; @@ -603,6 +603,47 @@ static int virtnet_open(struct net_device *dev) return 0; } +static int virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, + void *data, unsigned int len) +{ + struct scatterlist sg[3]; + struct { + u8 class; + u8 cmd; + } ctrl_cmd; + u8 ctrl_status; + unsigned int tmp; + int i = 0; + + if (!vi->cvq) + return -EFAULT; + + sg_init_table(sg, len ? 3 : 2); + + sg_set_buf(&sg[i++], &ctrl_cmd, sizeof(ctrl_cmd)); + if (len) + sg_set_buf(&sg[i++], data, len); + sg_set_buf(&sg[i], &ctrl_status, sizeof(ctrl_status)); + + ctrl_cmd.class = class; + ctrl_cmd.cmd = cmd; + + ctrl_status = ~0; + + if (vi->cvq->vq_ops->add_buf(vi->cvq, sg, i, 1, vi) != 0) + BUG(); + + vi->cvq->vq_ops->kick(vi->cvq); + + while (!vi->cvq->vq_ops->get_buf(vi->cvq, &tmp)) + cpu_relax(); + + if (ctrl_status == VIRTIO_NET_OK) + return 0; + else + return -EFAULT; +} + static int virtnet_close(struct net_device *dev) { struct virtnet_info *vi = netdev_priv(dev); @@ -733,6 +774,14 @@ static int virtnet_probe(struct virtio_device *vdev) goto free_recv; } + /* + * Outbound control channel virtqueue. We can live without it, + * so don't go fatal if it's not there. + */ + vi->cvq = vdev->config->find_vq(vdev, 2, NULL); + if (IS_ERR(vi->cvq)) + vi->cvq = NULL; + /* Initialize our empty receive and send queues. */ skb_queue_head_init(&vi->recv); skb_queue_head_init(&vi->send); @@ -763,6 +812,8 @@ static int virtnet_probe(struct virtio_device *vdev) unregister: unregister_netdev(dev); free_send: + if (vi->cvq) + vdev->config->del_vq(vi->cvq); vdev->config->del_vq(vi->svq); free_recv: vdev->config->del_vq(vi->rvq); @@ -793,6 +844,8 @@ static void virtnet_remove(struct virtio_device *vdev) vdev->config->del_vq(vi->svq); vdev->config->del_vq(vi->rvq); + if (vi->cvq) + vdev->config->del_vq(vi->cvq); unregister_netdev(vi->dev); while (vi->pages) diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h index 5cdd0aa..1de7c86 100644 --- a/include/linux/virtio_net.h +++ b/include/linux/virtio_net.h @@ -53,4 +53,7 @@ struct virtio_net_hdr_mrg_rxbuf { __u16 num_buffers; /* Number of merged rx buffers */ }; +#define VIRTIO_NET_OK 0 +#define VIRTIO_NET_ERR 1 + #endif /* _LINUX_VIRTIO_NET_H */
This will be used for RX mode, MAC filter table, VLAN filtering, etc... Signed-off-by: Alex Williamson <alex.williamson@hp.com> --- drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++++++++- include/linux/virtio_net.h | 3 ++ 2 files changed, 57 insertions(+), 1 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html