From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031092AbbJ3MFl (ORCPT ); Fri, 30 Oct 2015 08:05:41 -0400 Received: from e06smtp09.uk.ibm.com ([195.75.94.105]:52972 "EHLO e06smtp09.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030908AbbJ3MFg (ORCPT ); Fri, 30 Oct 2015 08:05:36 -0400 X-IBM-Helo: d06dlp02.portsmouth.uk.ibm.com X-IBM-MailFrom: borntraeger@de.ibm.com X-IBM-RcptTo: kvm@vger.kernel.org;linux-kernel@vger.kernel.org;linux-s390@vger.kernel.org;sparclinux@vger.kernel.org Subject: Re: [PATCH v4 2/6] virtio_ring: Support DMA APIs To: Cornelia Huck , Andy Lutomirski References: <7c590bf685f5cbc3f01e42bdbc1dbe3ffd83420f.1446162273.git.luto@kernel.org> <20151030130116.52a87922.cornelia.huck@de.ibm.com> Cc: linux-kernel@vger.kernel.org, "David S. Miller" , sparclinux@vger.kernel.org, Joerg Roedel , Sebastian Ott , Paolo Bonzini , Christoph Hellwig , benh@kernel.crashing.org, KVM , dwmw2@infradead.org, Martin Schwidefsky , linux-s390 , "Michael S. Tsirkin" , virtualization@lists.linux-foundation.org From: Christian Borntraeger Message-ID: <56335CF7.2050101@de.ibm.com> Date: Fri, 30 Oct 2015 13:05:11 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <20151030130116.52a87922.cornelia.huck@de.ibm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15103012-0037-0000-0000-0000047C3FF5 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Am 30.10.2015 um 13:01 schrieb Cornelia Huck: > On Thu, 29 Oct 2015 18:09:47 -0700 > Andy Lutomirski wrote: > >> virtio_ring currently sends the device (usually a hypervisor) >> physical addresses of its I/O buffers. This is okay when DMA >> addresses and physical addresses are the same thing, but this isn't >> always the case. For example, this never works on Xen guests, and >> it is likely to fail if a physical "virtio" device ever ends up >> behind an IOMMU or swiotlb. >> >> The immediate use case for me is to enable virtio on Xen guests. >> For that to work, we need vring to support DMA address translation >> as well as a corresponding change to virtio_pci or to another >> driver. >> >> With this patch, if enabled, virtfs survives kmemleak and >> CONFIG_DMA_API_DEBUG. >> >> Signed-off-by: Andy Lutomirski >> --- >> drivers/virtio/Kconfig | 2 +- >> drivers/virtio/virtio_ring.c | 190 +++++++++++++++++++++++++++++++-------- >> tools/virtio/linux/dma-mapping.h | 17 ++++ >> 3 files changed, 172 insertions(+), 37 deletions(-) >> create mode 100644 tools/virtio/linux/dma-mapping.h > >> static void detach_buf(struct vring_virtqueue *vq, unsigned int head) >> { >> - unsigned int i; >> + unsigned int i, j; >> + u16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT); >> >> /* Clear data ptr. */ >> - vq->data[head] = NULL; >> + vq->desc_state[head].data = NULL; >> >> - /* Put back on free list: find end */ >> + /* Put back on free list: unmap first-level descriptors and find end */ >> i = head; >> >> - /* Free the indirect table */ >> - if (vq->vring.desc[i].flags & cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT)) >> - kfree(phys_to_virt(virtio64_to_cpu(vq->vq.vdev, vq->vring.desc[i].addr))); >> - >> - while (vq->vring.desc[i].flags & cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT)) { >> + while (vq->vring.desc[i].flags & nextflag) { >> + vring_unmap_one(vq, &vq->vring.desc[i]); >> i = virtio16_to_cpu(vq->vq.vdev, vq->vring.desc[i].next); >> vq->vq.num_free++; >> } >> >> + vring_unmap_one(vq, &vq->vring.desc[i]); >> vq->vring.desc[i].next = cpu_to_virtio16(vq->vq.vdev, vq->free_head); >> vq->free_head = head; >> + >> /* Plus final descriptor */ >> vq->vq.num_free++; >> + >> + /* Free the indirect table, if any, now that it's unmapped. */ >> + if (vq->desc_state[head].indir_desc) { >> + struct vring_desc *indir_desc = vq->desc_state[head].indir_desc; >> + u32 len = vq->vring.desc[head].len; > > This one needs to be virtio32_to_cpu(...) as well. Yes, just did the exact same change diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index f269e1c..f2249df 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -556,7 +556,7 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head) /* Free the indirect table, if any, now that it's unmapped. */ if (vq->desc_state[head].indir_desc) { struct vring_desc *indir_desc = vq->desc_state[head].indir_desc; - u32 len = vq->vring.desc[head].len; + u32 len = virtio32_to_cpu(vq->vq.vdev, vq->vring.desc[head].len); BUG_ON(!(vq->vring.desc[head].flags & cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT))); now it boots. > >> + >> + BUG_ON(!(vq->vring.desc[head].flags & >> + cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT))); >> + BUG_ON(len == 0 || len % sizeof(struct vring_desc)); >> + >> + for (j = 0; j < len / sizeof(struct vring_desc); j++) >> + vring_unmap_one(vq, &indir_desc[j]); >> + >> + kfree(vq->desc_state[head].indir_desc); >> + vq->desc_state[head].indir_desc = NULL; >> + } >> } > > With that change on top of your current branch, I can boot (root on > virtio-blk, either virtio-1 or legacy virtio) on current qemu master > with kvm enabled on s390. Haven't tried anything further. > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Borntraeger Subject: Re: [PATCH v4 2/6] virtio_ring: Support DMA APIs Date: Fri, 30 Oct 2015 13:05:11 +0100 Message-ID: <56335CF7.2050101@de.ibm.com> References: <7c590bf685f5cbc3f01e42bdbc1dbe3ffd83420f.1446162273.git.luto@kernel.org> <20151030130116.52a87922.cornelia.huck@de.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20151030130116.52a87922.cornelia.huck@de.ibm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org List-Archive: List-Post: To: Cornelia Huck , Andy Lutomirski Cc: linux-s390 , Joerg Roedel , KVM , "Michael S. Tsirkin" , benh@kernel.crashing.org, Sebastian Ott , linux-kernel@vger.kernel.org, Christoph Hellwig , Martin Schwidefsky , sparclinux@vger.kernel.org, Paolo Bonzini , virtualization@lists.linux-foundation.org, dwmw2@infradead.org, "David S. Miller" List-ID: Am 30.10.2015 um 13:01 schrieb Cornelia Huck: > On Thu, 29 Oct 2015 18:09:47 -0700 > Andy Lutomirski wrote: > >> virtio_ring currently sends the device (usually a hypervisor) >> physical addresses of its I/O buffers. This is okay when DMA >> addresses and physical addresses are the same thing, but this isn't >> always the case. For example, this never works on Xen guests, and >> it is likely to fail if a physical "virtio" device ever ends up >> behind an IOMMU or swiotlb. >> >> The immediate use case for me is to enable virtio on Xen guests. >> For that to work, we need vring to support DMA address translation >> as well as a corresponding change to virtio_pci or to another >> driver. >> >> With this patch, if enabled, virtfs survives kmemleak and >> CONFIG_DMA_API_DEBUG. >> >> Signed-off-by: Andy Lutomirski >> --- >> drivers/virtio/Kconfig | 2 +- >> drivers/virtio/virtio_ring.c | 190 +++++++++++++++++++++++++++++++-------- >> tools/virtio/linux/dma-mapping.h | 17 ++++ >> 3 files changed, 172 insertions(+), 37 deletions(-) >> create mode 100644 tools/virtio/linux/dma-mapping.h > >> static void detach_buf(struct vring_virtqueue *vq, unsigned int head) >> { >> - unsigned int i; >> + unsigned int i, j; >> + u16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT); >> >> /* Clear data ptr. */ >> - vq->data[head] = NULL; >> + vq->desc_state[head].data = NULL; >> >> - /* Put back on free list: find end */ >> + /* Put back on free list: unmap first-level descriptors and find end */ >> i = head; >> >> - /* Free the indirect table */ >> - if (vq->vring.desc[i].flags & cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT)) >> - kfree(phys_to_virt(virtio64_to_cpu(vq->vq.vdev, vq->vring.desc[i].addr))); >> - >> - while (vq->vring.desc[i].flags & cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT)) { >> + while (vq->vring.desc[i].flags & nextflag) { >> + vring_unmap_one(vq, &vq->vring.desc[i]); >> i = virtio16_to_cpu(vq->vq.vdev, vq->vring.desc[i].next); >> vq->vq.num_free++; >> } >> >> + vring_unmap_one(vq, &vq->vring.desc[i]); >> vq->vring.desc[i].next = cpu_to_virtio16(vq->vq.vdev, vq->free_head); >> vq->free_head = head; >> + >> /* Plus final descriptor */ >> vq->vq.num_free++; >> + >> + /* Free the indirect table, if any, now that it's unmapped. */ >> + if (vq->desc_state[head].indir_desc) { >> + struct vring_desc *indir_desc = vq->desc_state[head].indir_desc; >> + u32 len = vq->vring.desc[head].len; > > This one needs to be virtio32_to_cpu(...) as well. Yes, just did the exact same change diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index f269e1c..f2249df 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -556,7 +556,7 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head) /* Free the indirect table, if any, now that it's unmapped. */ if (vq->desc_state[head].indir_desc) { struct vring_desc *indir_desc = vq->desc_state[head].indir_desc; - u32 len = vq->vring.desc[head].len; + u32 len = virtio32_to_cpu(vq->vq.vdev, vq->vring.desc[head].len); BUG_ON(!(vq->vring.desc[head].flags & cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT))); now it boots. > >> + >> + BUG_ON(!(vq->vring.desc[head].flags & >> + cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT))); >> + BUG_ON(len == 0 || len % sizeof(struct vring_desc)); >> + >> + for (j = 0; j < len / sizeof(struct vring_desc); j++) >> + vring_unmap_one(vq, &indir_desc[j]); >> + >> + kfree(vq->desc_state[head].indir_desc); >> + vq->desc_state[head].indir_desc = NULL; >> + } >> } > > With that change on top of your current branch, I can boot (root on > virtio-blk, either virtio-1 or legacy virtio) on current qemu master > with kvm enabled on s390. Haven't tried anything further. > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Borntraeger Date: Fri, 30 Oct 2015 12:05:11 +0000 Subject: Re: [PATCH v4 2/6] virtio_ring: Support DMA APIs Message-Id: <56335CF7.2050101@de.ibm.com> List-Id: References: <7c590bf685f5cbc3f01e42bdbc1dbe3ffd83420f.1446162273.git.luto@kernel.org> <20151030130116.52a87922.cornelia.huck@de.ibm.com> In-Reply-To: <20151030130116.52a87922.cornelia.huck@de.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Cornelia Huck , Andy Lutomirski Cc: linux-s390 , Joerg Roedel , KVM , "Michael S. Tsirkin" , benh@kernel.crashing.org, Sebastian Ott , linux-kernel@vger.kernel.org, Christoph Hellwig , Martin Schwidefsky , sparclinux@vger.kernel.org, Paolo Bonzini , virtualization@lists.linux-foundation.org, dwmw2@infradead.org, "David S. Miller" Am 30.10.2015 um 13:01 schrieb Cornelia Huck: > On Thu, 29 Oct 2015 18:09:47 -0700 > Andy Lutomirski wrote: > >> virtio_ring currently sends the device (usually a hypervisor) >> physical addresses of its I/O buffers. This is okay when DMA >> addresses and physical addresses are the same thing, but this isn't >> always the case. For example, this never works on Xen guests, and >> it is likely to fail if a physical "virtio" device ever ends up >> behind an IOMMU or swiotlb. >> >> The immediate use case for me is to enable virtio on Xen guests. >> For that to work, we need vring to support DMA address translation >> as well as a corresponding change to virtio_pci or to another >> driver. >> >> With this patch, if enabled, virtfs survives kmemleak and >> CONFIG_DMA_API_DEBUG. >> >> Signed-off-by: Andy Lutomirski >> --- >> drivers/virtio/Kconfig | 2 +- >> drivers/virtio/virtio_ring.c | 190 +++++++++++++++++++++++++++++++-------- >> tools/virtio/linux/dma-mapping.h | 17 ++++ >> 3 files changed, 172 insertions(+), 37 deletions(-) >> create mode 100644 tools/virtio/linux/dma-mapping.h > >> static void detach_buf(struct vring_virtqueue *vq, unsigned int head) >> { >> - unsigned int i; >> + unsigned int i, j; >> + u16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT); >> >> /* Clear data ptr. */ >> - vq->data[head] = NULL; >> + vq->desc_state[head].data = NULL; >> >> - /* Put back on free list: find end */ >> + /* Put back on free list: unmap first-level descriptors and find end */ >> i = head; >> >> - /* Free the indirect table */ >> - if (vq->vring.desc[i].flags & cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT)) >> - kfree(phys_to_virt(virtio64_to_cpu(vq->vq.vdev, vq->vring.desc[i].addr))); >> - >> - while (vq->vring.desc[i].flags & cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT)) { >> + while (vq->vring.desc[i].flags & nextflag) { >> + vring_unmap_one(vq, &vq->vring.desc[i]); >> i = virtio16_to_cpu(vq->vq.vdev, vq->vring.desc[i].next); >> vq->vq.num_free++; >> } >> >> + vring_unmap_one(vq, &vq->vring.desc[i]); >> vq->vring.desc[i].next = cpu_to_virtio16(vq->vq.vdev, vq->free_head); >> vq->free_head = head; >> + >> /* Plus final descriptor */ >> vq->vq.num_free++; >> + >> + /* Free the indirect table, if any, now that it's unmapped. */ >> + if (vq->desc_state[head].indir_desc) { >> + struct vring_desc *indir_desc = vq->desc_state[head].indir_desc; >> + u32 len = vq->vring.desc[head].len; > > This one needs to be virtio32_to_cpu(...) as well. Yes, just did the exact same change diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index f269e1c..f2249df 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -556,7 +556,7 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head) /* Free the indirect table, if any, now that it's unmapped. */ if (vq->desc_state[head].indir_desc) { struct vring_desc *indir_desc = vq->desc_state[head].indir_desc; - u32 len = vq->vring.desc[head].len; + u32 len = virtio32_to_cpu(vq->vq.vdev, vq->vring.desc[head].len); BUG_ON(!(vq->vring.desc[head].flags & cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT))); now it boots. > >> + >> + BUG_ON(!(vq->vring.desc[head].flags & >> + cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT))); >> + BUG_ON(len = 0 || len % sizeof(struct vring_desc)); >> + >> + for (j = 0; j < len / sizeof(struct vring_desc); j++) >> + vring_unmap_one(vq, &indir_desc[j]); >> + >> + kfree(vq->desc_state[head].indir_desc); >> + vq->desc_state[head].indir_desc = NULL; >> + } >> } > > With that change on top of your current branch, I can boot (root on > virtio-blk, either virtio-1 or legacy virtio) on current qemu master > with kvm enabled on s390. Haven't tried anything further. >