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

[Anthony Liguori <anthony@codemonkey.ws>]

Avi Kivity wrote:
> 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.
>   

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

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

Sure.

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

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.

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

This is necessary because of the net optimizations, which yes, should go 
upstream into QEMU.

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

I will, but I'd rather have a consumer first.

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

Regards,

Anthony Liguori