From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51320) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XHdZv-0008EL-Vz for qemu-devel@nongnu.org; Wed, 13 Aug 2014 14:47:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XHdZp-0006iG-Q1 for qemu-devel@nongnu.org; Wed, 13 Aug 2014 14:47:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:4494) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XHdZp-0006iA-Hf for qemu-devel@nongnu.org; Wed, 13 Aug 2014 14:47:13 -0400 Date: Wed, 13 Aug 2014 20:47:45 +0200 From: "Michael S. Tsirkin" Message-ID: <20140813184745.GC25973@redhat.com> References: <1407937660-1952-1-git-send-email-hjxiaohust@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1407937660-1952-1-git-send-email-hjxiaohust@gmail.com> Subject: Re: [Qemu-devel] [PATCH 1/1] add loopback for virtio-net List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hengjinxiao Cc: virtio-dev@lists.oasis-open.org, jasowang@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, aliguori@amazon.com On Wed, Aug 13, 2014 at 09:47:40PM +0800, Hengjinxiao wrote: > Sometimes it is necessary to test whether virtio-net can send and receive packets > normally, just as e1000 does. This patch adds loopback for virtio-net, when the > command 'VIRTIO_NET_CTRL_LOOPBACK_SET' is sent from front-end driver(such as > virtio-net driver in linux.git), virtio-net goes into loopback mode, and the test > above can be executed, then command 'VIRTIO_NET_CTRL_LOOPBACK_UNSET' can make > virtio-net go back to previous state. > > Signed-off-by: Hengjinxiao Using two commands here seems unjustified, when using one would do. Capability to handle these commands needs a feature bit. The patches have style issues. But besides all this, there are bigger questions: How is this capability useful? Your description is kind of vague. It might be useful for e1000 as a driver development tool. For virtio it seems just as easy to connect to another VM alternatively you can write a utility - external to qemu - that gets all packets and bounces them back on the same fd. In fact, won't something like netcat do this more or less for free? It's hard to judge whether it's worth the performance and maintainance costs, without this info. > --- > hw/net/virtio-net.c | 26 +++++++++++++++++++++++--- > include/hw/virtio/virtio-net.h | 9 +++++++++ > include/hw/virtio/virtio.h | 1 + > include/net/net.h | 1 + > net/net.c | 7 +++++-- > 5 files changed, 39 insertions(+), 5 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 268eff9..ca313f4 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -778,6 +778,22 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd, > > return VIRTIO_NET_OK; > } > + > +static int virtio_net_handle_loopback(VirtIONet *n, uint8_t cmd, > + struct iovec *iov, unsigned int iov_cnt) > +{ > + VirtIODevice *vdev = VIRTIO_DEVICE(n); > + if (cmd == VIRTIO_NET_CTRL_LOOPBACK_SET) { > + vdev->loopback = 1; > + return VIRTIO_NET_OK; > + } else if (cmd == VIRTIO_NET_CTRL_LOOPBACK_UNSET) { > + vdev->loopback = 0; > + return VIRTIO_NET_OK; > + } else { > + return VIRTIO_NET_ERR; > + } > +} > + > static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq) > { > VirtIONet *n = VIRTIO_NET(vdev); > @@ -813,6 +829,8 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq) > status = virtio_net_handle_mq(n, ctrl.cmd, iov, iov_cnt); > } else if (ctrl.class == VIRTIO_NET_CTRL_GUEST_OFFLOADS) { > status = virtio_net_handle_offloads(n, ctrl.cmd, iov, iov_cnt); > + } else if (ctrl.class == VIRTIO_NET_CTRL_LOOPBACK) { > + status = virtio_net_handle_loopback(n, ctrl.cmd, iov, iov_cnt); > } > > s = iov_from_buf(elem.in_sg, elem.in_num, 0, &status, sizeof(status)); > @@ -1108,6 +1126,9 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q) > VirtQueueElement elem; > int32_t num_packets = 0; > int queue_index = vq2q(virtio_get_queue_index(q->tx_vq)); > + NetClientState *sender = qemu_get_subqueue(n->nic, queue_index); > + > + sender->loopback = vdev->loopback; > if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) { > return num_packets; > } > @@ -1156,9 +1177,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q) > } > > len = n->guest_hdr_len; > - > - ret = qemu_sendv_packet_async(qemu_get_subqueue(n->nic, queue_index), > - out_sg, out_num, virtio_net_tx_complete); > + > + ret = qemu_sendv_packet_async(sender, out_sg, out_num, > + virtio_net_tx_complete); > if (ret == 0) { > virtio_queue_set_notification(q->tx_vq, 0); > q->async_tx.elem = elem; > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h > index 6ceb5aa..24e56bf 100644 > --- a/include/hw/virtio/virtio-net.h > +++ b/include/hw/virtio/virtio-net.h > @@ -257,6 +257,15 @@ struct virtio_net_ctrl_mq { > #define VIRTIO_NET_CTRL_GUEST_OFFLOADS 5 > #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET 0 > > + /* > + * Control Loopback > + * > + * The command VIRTIO_NET_CTRL_LOOPBACK_SET is used to require the device > + * come into loopback state. > + */ > +#define VIRTIO_NET_CTRL_LOOPBACK 6 > + #define VIRTIO_NET_CTRL_LOOPBACK_SET 0 > + #define VIRTIO_NET_CTRL_LOOPBACK_UNSET 1 > #define DEFINE_VIRTIO_NET_FEATURES(_state, _field) \ > DEFINE_PROP_BIT("any_layout", _state, _field, VIRTIO_F_ANY_LAYOUT, true), \ > DEFINE_PROP_BIT("csum", _state, _field, VIRTIO_NET_F_CSUM, true), \ > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index a60104c..219e0d2 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -128,6 +128,7 @@ struct VirtIODevice > VMChangeStateEntry *vmstate; > char *bus_name; > uint8_t device_endian; > + uint8_t loopback; > }; > > typedef struct VirtioDeviceClass { > diff --git a/include/net/net.h b/include/net/net.h > index ed594f9..ce66b86 100644 > --- a/include/net/net.h > +++ b/include/net/net.h > @@ -89,6 +89,7 @@ struct NetClientState { > NetClientDestructor *destructor; > unsigned int queue_index; > unsigned rxfilter_notify_enabled:1; > + int loopback; > }; > > typedef struct NICState { > diff --git a/net/net.c b/net/net.c > index 6d930ea..f4c1ef4 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -611,8 +611,11 @@ ssize_t qemu_sendv_packet_async(NetClientState *sender, > if (sender->link_down || !sender->peer) { > return iov_size(iov, iovcnt); > } > - > - queue = sender->peer->incoming_queue; > + if (sender->loopback) { > + queue = sender->incoming_queue; > + } else { > + queue = sender->peer->incoming_queue; > + } > > return qemu_net_queue_send_iov(queue, sender, > QEMU_NET_PACKET_FLAG_NONE, > -- > 1.8.3.2