From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Jg6Bj-0007LQ-TP for qemu-devel@nongnu.org; Sun, 30 Mar 2008 18:39:43 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Jg6Bj-0007L8-2O for qemu-devel@nongnu.org; Sun, 30 Mar 2008 18:39:43 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Jg6Bi-0007Kz-Si for qemu-devel@nongnu.org; Sun, 30 Mar 2008 18:39:42 -0400 Received: from fg-out-1718.google.com ([72.14.220.154]) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Jg6Bi-0002DZ-H0 for qemu-devel@nongnu.org; Sun, 30 Mar 2008 18:39:42 -0400 Received: by fg-out-1718.google.com with SMTP id e12so1126360fga.8 for ; Sun, 30 Mar 2008 15:39:41 -0700 (PDT) Subject: Re: [Qemu-devel] [PATCH 3/6] virtio for QEMU In-Reply-To: <1206827760-4566-3-git-send-email-aliguori@us.ibm.com> References: <1206827760-4566-1-git-send-email-aliguori@us.ibm.com> <1206827760-4566-3-git-send-email-aliguori@us.ibm.com> Content-Type: text/plain Date: Sun, 30 Mar 2008 20:25:41 +0300 Message-Id: <1206897941.3357.15.camel@localhost.localdomain> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit From: Dor Laor Reply-To: dor.laor@qumranet.com, qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: kvm-devel@lists.sourceforge.net, Marcelo Tosatti , Anthony Liguori , Aurelien Jarno On Sat, 2008-03-29 at 16:55 -0500, Anthony Liguori wrote: > This patch introduces virtio support over PCI. virtio is a generic virtual IO > framework for Linux first introduced in 2.6.23. Since 2.6.25, virtio has > supported a PCI transport which this patch implements. > > Since the last time these patches were posted to qemu-devel, I've reworked it > to use the proper access functions to manipulate guest memory. > > Signed-off-by: Anthony Liguori It's will be great to drop the nasty hacks :) Do you still get 1G net performance using the extra copy from tap (memcpy_to_iovector)? [snip] > +static uint32_t vring_desc_len(VirtQueue *vq, unsigned int i) > +{ Below there were place you did use offsetof(vq->vring.desc[i], len) so we better be consistent + its nicer > + return ldl_phys(vq->vring.desc + i * sizeof(VRingDesc) + > + offsetof(VRingDesc, len)); > +} > + [snip] > +VirtQueueElement *virtqueue_pop(VirtQueue *vq) > +{ > + unsigned int i, head; > + unsigned int position; > + VirtQueueElement *elem; > + > + /* Check it isn't doing very strange things with descriptor numbers. */ > + if ((uint16_t)(vring_avail_idx(vq) - vq->last_avail_idx) > vq->vring.num) > + errx(1, "Guest moved used index from %u to %u", > + vq->last_avail_idx, vring_avail_idx(vq)); > + > + /* If there's nothing new since last we looked, return invalid. */ > + if (vring_avail_idx(vq) == vq->last_avail_idx) > + return NULL; > + > + /* Grab the next descriptor number they're advertising, and increment > + * the index we've seen. */ > + head = vring_avail_ring(vq, vq->last_avail_idx++ % vq->vring.num); > + > + /* If their number is silly, that's a fatal mistake. */ > + if (head >= vq->vring.num) > + errx(1, "Guest says index %u is available", head); > + > + /* When we start there are none of either input nor output. */ > + position = 0; > + > + elem = qemu_mallocz(sizeof(VirtQueueElement)); > + > + elem->phys_in = qemu_mallocz(sizeof(PhysIOVector) + > + vq->vring.num * sizeof(PhysIOVectorElement)); > + elem->phys_out = qemu_mallocz(sizeof(PhysIOVector) + > + vq->vring.num * sizeof(PhysIOVectorElement)); I was wondering whether it can be optimized since vring.num is sometimes 512 so and we can either use a pool of these or calculate the vring.num from the descriptors but it seems like your way is the best. > + > + i = head; > + do { > + PhysIOVectorElement *sge; > + > + if (vring_desc_flags(vq, i) & VRING_DESC_F_WRITE) > + sge = &elem->phys_in->sg[elem->phys_in->num++]; > + else > + sge = &elem->phys_out->sg[elem->phys_out->num++]; > + > + /* Grab the first descriptor, and check it's OK. */ > + sge->len = vring_desc_len(vq, i); > + sge->base = vring_desc_addr(vq, i); > + > + /* If we've got too many, that implies a descriptor loop. */ > + if ((elem->phys_in->num + elem->phys_out->num) > vq->vring.num) > + errx(1, "Looped descriptor"); > + } while ((i = virtqueue_next_desc(vq, i)) != vq->vring.num); > + > + elem->virt_in = pci_device_dma_map(&vq->vdev->pci_dev, elem->phys_in); > + elem->virt_out = pci_device_dma_map(&vq->vdev->pci_dev, elem->phys_out); > + elem->index = head; > + > + if (elem->virt_in == NULL || elem->virt_out == NULL) > + errx(1, "Bad DMA"); > + > + return elem; > +} > + > + The name below is a bit misleading since when enable is true you actually set no_notify. So I name it something like virtio_vring_set_no_notify(...) or similar. > +void virtio_ring_set_used_notify(VirtQueue *vq, int enable) > +{ > + if (enable) > + vring_used_set_flag(vq, VRING_USED_F_NO_NOTIFY); > + else > + vring_used_unset_flag(vq, VRING_USED_F_NO_NOTIFY); > +} > + Cheers, Dor