From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anthony Liguori Subject: Re: [PATCH][RFC] Prepare virtio for upstream QEMU merging Date: Tue, 14 Oct 2008 11:21:19 -0500 Message-ID: <48F4C6FF.4010801@codemonkey.ws> References: <48F3934E.9040809@codemonkey.ws> <48F4C4FF.8020207@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm-devel To: Avi Kivity Return-path: Received: from an-out-0708.google.com ([209.85.132.251]:5165 "EHLO an-out-0708.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751481AbYJNQVX (ORCPT ); Tue, 14 Oct 2008 12:21:23 -0400 Received: by an-out-0708.google.com with SMTP id d40so169145and.103 for ; Tue, 14 Oct 2008 09:21:22 -0700 (PDT) In-Reply-To: <48F4C4FF.8020207@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 >> >> 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