Prepare virtio for upstream QEMU merging

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 <[email protected]>

qemu/hw/virtio-blk.c +2 -0

index 727119b..fb703eb 100644
@@ -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,

qemu/hw/virtio-blk.c +0 -0

 static int virtio_net_can_receive(void *opaque)
 {

     VirtIONet *n = opaque;

qemu/hw/virtio-blk.c +1 -2

-    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) {

qemu/hw/virtio-blk.c +16 -0

     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,

Quoted 38 lines · Anthony Liguori

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 <[email protected]>

qemu/hw/virtio-blk.c +2 -0

index 727119b..fb703eb 100644
@@ -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;
 

qemu/hw/virtio-blk.c void *virtio_blk_init(PCIBus *bus, uint16_t vendor, uint16_t device, · line 302

Quoted 13 lines

qemu/hw/virtio-blk.c +3 -0

     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.

Quoted 17 lines

qemu/hw/virtio-blk.c +0 -0

 static int virtio_net_can_receive(void *opaque)

 {
     VirtIONet *n = opaque;

qemu/hw/virtio-blk.c +3 -3

-    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;
     }

qemu/hw/virtio-blk.c

Quoted 6 lines

qemu/hw/virtio-blk.c +1 -1

-    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.

qemu/hw/virtio-blk.c static void work_around_broken_dhclient(struct virtio_net_hdr *hdr, · line 195

Quoted 12 lines

qemu/hw/virtio-blk.c +2 -0

+#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?

Quoted 7 lines

qemu/hw/virtio-blk.c +0 -0

 static void virtio_net_receive(void *opaque, const uint8_t *buf, int size)

{

qemu/hw/virtio-blk.c +0 -0

@@ -190,6 +200,11 @@ static void virtio_net_receive(void *opaque, const uint8_t *buf, int size)
     int offset, i;
     int total;
 

qemu/hw/virtio-blk.c static void virtio_net_receive(void *opaque, const uint8_t *buf, int size) · line 209

Quoted 7 lines

qemu/hw/virtio-blk.c +5 -0

+#ifndef USE_KVM
+    if (!virtio_queue_ready(n->rx_vq))
+        return;
+#endif
+
 
 

Or this?

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

Quoted 6 lines

qemu/hw/virtio-blk.c +6 -0

+#ifdef USE_KVM
+    VRingDesc *desc;
+    VRingAvail *avail;
+    VRingUsed *used;
+#else
+    target_phys_addr_t desc;

qemu/hw/virtio-blk.c

Quoted 6 lines

qemu/hw/virtio-blk.c +4 -0

+    target_phys_addr_t avail;
+    target_phys_addr_t used;
+#endif
+} VRing;

Stumped. Why?

Quoted 4 lines

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

I'd rather have a consumer of an interface before merging the actual infrastructure.

Quoted 16 lines

qemu/hw/virtio-net.c static void work_around_broken_dhclient(struct · line 195

Quoted 13 lines

+#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

qemu/hw/virtio-net.c +1 -0

@@ -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?

It is because the GSO stuff has not gone into upstream QEMU yet. I'd rather merge a crippled virtio implementation, and then merge each of the optimizations than vice-versa. Primarily because I'm concerned some of these optimizations will require refactoring.

Quoted 15 lines

qemu/hw/virtio.c line 100

Quoted 12 lines

qemu/hw/virtio.c +10 -0

+#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?

In KVM, desc points directly to guest memory. The non-KVM accessors use stl/ldl et al so they only need the physical address base.

If you agree with this approach, I'll clean up the patch and send again.

Quoted 4 lines · Anthony Liguori

Quoted 4 lines

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

Quoted 4 lines

I'd rather have a consumer of an interface before merging the actual infrastructure.

So merge them all into qemu at the same time (as separate patches, if you like).

The amount of code duplication is frightening.

Oh, and we need to set the dirty bit so live migration works. Or do we have a hack in place to force copying of the ring at the last stage?

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

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

I'd rather have a consumer of an interface before merging the actual infrastructure.

Quoted 3 lines

So merge them all into qemu at the same time (as separate patches, if you like).

But that will require refactoring a lot of these optimizations. In order to do that right, they need to be presented on qemu-devel. It's a whole lot easier to do that incrementally so that people can digest it all instead of blasting a big series.

Quoted 3 lines

The amount of code duplication is frightening.

I've already got that worked out. I need to prepare and commit a patch to fix stw/lduw in upstream QEMU and then we can switch to always using those functions in KVM. I've done some performance testing and that seems to be enough. With that, there is no longer any scary code duplication.

Quoted 8 lines · Anthony Liguori

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

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

I'd rather have a consumer of an interface before merging the actual infrastructure.

Quoted 5 lines

Quoted 3 lines

So merge them all into qemu at the same time (as separate patches, if you like).

But that will require refactoring a lot of these optimizations. In order to do that right, they need to be presented on qemu-devel. It's a whole lot easier to do that incrementally so that people can digest it all instead of blasting a big series.

Can you elaborate? Which interfaces will need rework, and why?

Quoted 5 lines

Quoted 3 lines

The amount of code duplication is frightening.

I've already got that worked out. I need to prepare and commit a patch to fix stw/lduw in upstream QEMU and then we can switch to always using those functions in KVM. I've done some performance testing and that seems to be enough. With that, there is no longer any scary code duplication.

Yes, your patch on qemu-devel looks good. Even if we do have a performance problem (which may well turn out after we optimize things some more), it's easy to have a cached pointer along with the phys address.

Quoted 4 lines

Quoted 2 lines

But that will require refactoring a lot of these optimizations. In order to do that right, they need to be presented on qemu-devel. It's a whole lot easier to do that incrementally so that people can digest it all instead of blasting a big series.

Can you elaborate? Which interfaces will need rework, and why?

Last time I tried, virtio-net doesn't work with slirp. I believe it's either because of the GSO changes (unlikely) or because of the can_receive changes (more likely). The can_receive changes probably need some refactoring to be more slirp friendly. The GSO changes are a bit vlan unfriendly.

Right now, you could construct something like -net tap -net nic,model=virtio -net model=e1000. e1000 doesn't support GSO and bad things will happen from this. It's very centric to the single-nic, single-host driver model. Also, exposing something like tap_has_vnet_hdr() to the actual network cards violates the layering. The network cards shouldn't have any knowledge of what types of host drivers there are, just what features a particular VLAN supports.

It's also unclear how you handle things like NIC hot-plug. What if you add a nic that doesn't support GSO to a VLAN that is using GSO? What about migration? What if you migrate from a host that has GSO support to a host that doesn't support GSO? This later problem is hard and would require either a feature renegotiation mechanism in virtio or software implementation of GSO within QEMU.