Re: [Qemu-devel] [PATCH 1/1] add loopback for virtio-net

["Michael S. Tsirkin" <mst@redhat.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 <hjxiaohust@gmail.com>

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