Re: [PATCH][RFC] Prepare virtio for upstream QEMU merging

[Avi Kivity <avi@redhat.com>]

Anthony Liguori wrote:
> For merging virtio, I thought I'd try a different approach from the
> fix out of tree and merge all at once approach I took with live migration.
>
> What I would like to do is make some minimal changes to virtio in kvm-userspace
> so that some form of virtio could be merged in upstream QEMU.  We'll have to
> maintain a small diff in the kvm-userspace tree until we work out some of the
> issues, but I'd rather work out those issues in the QEMU tree instead of fixing
> them all outside of the tree first.
>
> The attached patch uses USE_KVM guards to separate KVM specific code.  This is
> not KVM code, per say, but rather routines that aren't mergable right now.  I
> think using the guards make it more clear and easier to deal with merges.
>
> When we submit virtio to qemu-devel and eventually commit, we'll strip out any
> ifdef USE_KVM block.
>
> The only real significant change in this patch is the use of accessors for
> the virtio queues.  We've discussed patches like this before.
>
> The point of this patch is to make no functional change in the KVM tree.  I've
> confirmed that performance does not regress in virtio-net and that both
> virtio-blk and virtio-net seem to be functional.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> diff --git a/qemu/hw/virtio-blk.c b/qemu/hw/virtio-blk.c
> index 727119b..fb703eb 100644
> --- a/qemu/hw/virtio-blk.c
> +++ b/qemu/hw/virtio-blk.c
> @@ -276,7 +276,9 @@ void *virtio_blk_init(PCIBus *bus, uint16_t vendor, uint16_t device,
>  		      BlockDriverState *bs)
>  {
>      VirtIOBlock *s;
> +#ifdef USE_KVM
>      int cylinders, heads, secs;
> +#endif
>      static int virtio_blk_id;
>  
>      s = (VirtIOBlock *)virtio_init_pci(bus, "virtio-blk", vendor, device,
> @@ -290,9 +292,12 @@ void *virtio_blk_init(PCIBus *bus, uint16_t vendor, uint16_t device,
>      s->vdev.get_features = virtio_blk_get_features;
>      s->vdev.reset = virtio_blk_reset;
>      s->bs = bs;
> +#ifdef USE_KVM
>      bs->devfn = s->vdev.pci_dev.devfn;
> +    /* FIXME this can definitely go in upstream QEMU */
>      bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
>      bdrv_set_geometry_hint(s->bs, cylinders, heads, secs);
> +#endif
>   

Why not merge these bits prior to merging virtio?  They aren't kvm
specific and would be good in mainline qemu.

>  
>  static int virtio_net_can_receive(void *opaque)
>  {
>      VirtIONet *n = opaque;
>  
> -    if (n->rx_vq->vring.avail == NULL ||
> +    if (!virtio_queue_ready(n->rx_vq) ||
>  	!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK))
>  	return 0;
>  
> -    if (n->rx_vq->vring.avail->idx == n->rx_vq->last_avail_idx) {
> -	n->rx_vq->vring.used->flags &= ~VRING_USED_F_NO_NOTIFY;
> +    if (virtio_queue_empty(n->rx_vq)) {
> +        virtio_queue_set_notification(n->rx_vq, 1);
>  	return 0;
>      }
>  
> -    n->rx_vq->vring.used->flags |= VRING_USED_F_NO_NOTIFY;
> +    virtio_queue_set_notification(n->rx_vq, 0);
>      return 1;
>  }
>   

This should be in a separate patch.

>  
> +#ifdef USE_KVM
>  /* dhclient uses AF_PACKET but doesn't pass auxdata to the kernel so
>   * it never finds out that the packets don't have valid checksums.  This
>   * causes dhclient to get upset.  Fedora's carried a patch for ages to
> @@ -181,6 +190,7 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
>          hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
>      }
>  }
> +#endif
>   

Is this kvm specific?

>  
>  static void virtio_net_receive(void *opaque, const uint8_t *buf, int size)
>  {
> @@ -190,6 +200,11 @@ static void virtio_net_receive(void *opaque, const uint8_t *buf, int size)
>      int offset, i;
>      int total;
>  
> +#ifndef USE_KVM
> +    if (!virtio_queue_ready(n->rx_vq))
> +        return;
> +#endif
> +
>   

Or this?

>  
> +#ifdef USE_KVM
>      if (tap_has_vnet_hdr(n->vc->vlan->first_client)) {
>  	memcpy(hdr, buf, sizeof(*hdr));
>  	offset += total;
>          work_around_broken_dhclient(hdr, buf + offset, size - offset);
>      }
> +#endif
>   

Or this?

>  
>      /* copy in packet.  ugh */
>      i = 1;
> @@ -230,7 +247,11 @@ static void virtio_net_receive(void *opaque, const uint8_t *buf, int size)
>  static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
>  {
>      VirtQueueElement elem;
> +#ifdef USE_KVM
>      int has_vnet_hdr = tap_has_vnet_hdr(n->vc->vlan->first_client);
> +#else
> +    int has_vnet_hdr = 0;
> +#endif
>   

Or this?

>  
>      if (!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK))
>          return;
> @@ -252,7 +273,32 @@ static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
>  	    len += sizeof(struct virtio_net_hdr);
>  	}
>  
> +#ifdef USE_KVM
>  	len += qemu_sendv_packet(n->vc, out_sg, out_num);
> +#else
> +        {
> +            size_t size = 0;
> +            size_t offset = 0;
> +            int i;
> +            void *packet;
> +
> +            for (i = 0; i < out_num; i++)
> +                size += out_sg[i].iov_len;
> +
> +            packet = qemu_malloc(size);
> +            if (packet) {
> +                for (i = 0; i < out_num; i++) {
> +                    memcpy(packet + offset,
> +                           out_sg[i].iov_base,
> +                           out_sg[i].iov_len);
> +                    offset += out_sg[i].iov_len;
> +                }
> +                qemu_send_packet(n->vc, packet, size);
> +                len += size;
> +            }
> +            qemu_free(packet);
> +        }
> +#endif
>   

Why not merge qemu_sendv_packet?  Perhaps with the alternate code as the
body if some infrastructure in qemu is missing?

>  
>  	virtqueue_push(vq, &elem, len);
>  	virtio_notify(&n->vdev, vq);
> @@ -264,7 +310,7 @@ static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq)
>      VirtIONet *n = to_virtio_net(vdev);
>  
>      if (n->tx_timer_active) {
> -	vq->vring.used->flags &= ~VRING_USED_F_NO_NOTIFY;
> +        virtio_queue_set_notification(vq, 1);
>   

Separate patch.

> +#ifdef USE_KVM
> +    VRingDesc *desc;
> +    VRingAvail *avail;
> +    VRingUsed *used;
> +#else
> +    target_phys_addr_t desc;
> +    target_phys_addr_t avail;
> +    target_phys_addr_t used;
> +#endif
> +} VRing;
>   

Stumped.  Why?


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.