On 26/08/14 23:17, 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 this fix as well as a corresponding > fix to virtio_pci or to another driver. > > With this patch, virtfs survives kmemleak and CONFIG_DMA_API_DEBUG. > virtio-net warns (correctly) about DMA from the stack in > virtnet_set_rx_mode. > > This breaks s390's defconfig. The default configuration for s390 > does virtio through a KVM-specific interface, but that configuration > does not support DMA. I could modify this patch to stub out the DMA > API calls if !CONFIG_HAS_DMA, but it seems to me that it would be > much nicer to make s390 support DMA unconditionally.s390 has no DMA per se. Newest systems have a PCI-like I/O attach in addition to the classic channel I/O, and for that we enable the DMA code just for that transport to be able to reuse some of the existing PCI drivers. (only some because, we differ in some aspects from how PCI looks like) But the architecture itself (and the virtio interface) does not provide the DMA interface as you know it: In essence this patch is a no-go for s390. I am also aksing myself, if this makes virtio-ring more expensive? Christian> > It's actually unclear to me whether this can be fixed without either > s390 arch support or a special case for s390 in virtio_ring: on > brief inspection of s390's DMA code, it seems to assume that > everything goes through a PCI IOMMU, which is presumably not the > case for virtio.> > Cc: Cornelia Huck <cornelia.huck at de.ibm.com> > Signed-off-by: Andy Lutomirski <luto at amacapital.net> > --- > drivers/virtio/Kconfig | 1 + > drivers/virtio/virtio_ring.c | 116 +++++++++++++++++++++++++++++++++++-------- > 2 files changed, 95 insertions(+), 22 deletions(-) > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig > index c6683f2e396c..b6f3acc61153 100644 > --- a/drivers/virtio/Kconfig > +++ b/drivers/virtio/Kconfig > @@ -1,5 +1,6 @@ > config VIRTIO > tristate > + depends on HAS_DMA > ---help--- > This option is selected by any driver which implements the virtio > bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_LGUEST, > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index d356a701c9c2..6a78e2fd8e63 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -24,6 +24,7 @@ > #include <linux/module.h> > #include <linux/hrtimer.h> > #include <linux/kmemleak.h> > +#include <linux/dma-mapping.h> > > #ifdef DEBUG > /* For development, we want to crash whenever the ring is screwed. */ > @@ -54,6 +55,12 @@ > #define END_USE(vq) > #endif > > +struct vring_desc_state > +{ > + void *data; /* Data for callback. */ > + struct vring_desc *indir_desc; /* Indirect descriptor, if any. */ > +}; > + > struct vring_virtqueue > { > struct virtqueue vq; > @@ -93,12 +100,45 @@ struct vring_virtqueue > ktime_t last_add_time; > #endif > > - /* Tokens for callbacks. */ > - void *data[]; > + /* Per-descriptor state. */ > + struct vring_desc_state desc_state[]; > }; > > #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq) > > +/* Helper to map one sg entry, since we can't use dma_map_sg. */ > +static dma_addr_t dma_map_one_sg(struct vring_virtqueue *vq, > + struct scatterlist *sg, > + enum dma_data_direction direction) > +{ > + return dma_map_page(vq->vq.vdev->dev.parent, > + sg_page(sg), sg->offset, sg->length, direction); > +} > + > +static void unmap_one(struct vring_virtqueue *vq, struct vring_desc *desc) > +{ > + if (desc->flags & VRING_DESC_F_INDIRECT) { > + dma_unmap_single(vq->vq.vdev->dev.parent, > + desc->addr, desc->len, > + (desc->flags & VRING_DESC_F_WRITE) ? > + DMA_FROM_DEVICE : DMA_TO_DEVICE); > + } else { > + dma_unmap_page(vq->vq.vdev->dev.parent, > + desc->addr, desc->len, > + (desc->flags & VRING_DESC_F_WRITE) ? > + DMA_FROM_DEVICE : DMA_TO_DEVICE); > + } > +} > + > +static void unmap_indirect(struct vring_virtqueue *vq, struct vring_desc *desc, > + int total) > +{ > + int i; > + > + for (i = 0; i < total; i++) > + unmap_one(vq, &desc[i]); > +} > + > /* Set up an indirect table of descriptors and add it to the queue. */ > static inline int vring_add_indirect(struct vring_virtqueue *vq, > struct scatterlist *sgs[], > @@ -132,7 +172,11 @@ static inline int vring_add_indirect(struct vring_virtqueue *vq, > if (!sg) > break; > desc[i].flags = VRING_DESC_F_NEXT; > - desc[i].addr = sg_phys(sg); > + desc[i].addr > + dma_map_one_sg(vq, sg, DMA_TO_DEVICE); > + if (dma_mapping_error(vq->vq.vdev->dev.parent, > + desc[i].addr)) > + goto unmap_free; > desc[i].len = sg->length; > desc[i].next = i+1; > i++; > @@ -143,7 +187,11 @@ static inline int vring_add_indirect(struct vring_virtqueue *vq, > if (!sg) > break; > desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE; > - desc[i].addr = sg_phys(sg); > + desc[i].addr > + dma_map_one_sg(vq, sg, DMA_FROM_DEVICE); > + if (dma_mapping_error(vq->vq.vdev->dev.parent, > + desc[i].addr)) > + goto unmap_free; > desc[i].len = sg->length; > desc[i].next = i+1; > i++; > @@ -161,15 +209,27 @@ static inline int vring_add_indirect(struct vring_virtqueue *vq, > /* Use a single buffer which doesn't continue */ > head = vq->free_head; > vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT; > - vq->vring.desc[head].addr = virt_to_phys(desc); > - /* kmemleak gives a false positive, as it's hidden by virt_to_phys */ > - kmemleak_ignore(desc); > + vq->vring.desc[head].addr > + dma_map_single(vq->vq.vdev->dev.parent, > + desc, i * sizeof(struct vring_desc), > + DMA_TO_DEVICE); > + if (dma_mapping_error(vq->vq.vdev->dev.parent, > + vq->vring.desc[head].addr)) > + goto unmap_free; > vq->vring.desc[head].len = i * sizeof(struct vring_desc); > > /* Update free pointer */ > vq->free_head = vq->vring.desc[head].next; > > + /* Save the indirect block */ > + vq->desc_state[head].indir_desc = desc; > + > return head; > + > +unmap_free: > + unmap_indirect(vq, desc, i); > + kfree(desc); > + return -ENOMEM; > } > > static inline int virtqueue_add(struct virtqueue *_vq, > @@ -244,7 +304,8 @@ static inline int virtqueue_add(struct virtqueue *_vq, > if (!sg) > break; > vq->vring.desc[i].flags = VRING_DESC_F_NEXT; > - vq->vring.desc[i].addr = sg_phys(sg); > + vq->vring.desc[i].addr > + dma_map_one_sg(vq, sg, DMA_TO_DEVICE); > vq->vring.desc[i].len = sg->length; > prev = i; > i = vq->vring.desc[i].next; > @@ -255,7 +316,8 @@ static inline int virtqueue_add(struct virtqueue *_vq, > if (!sg) > break; > vq->vring.desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE; > - vq->vring.desc[i].addr = sg_phys(sg); > + vq->vring.desc[i].addr > + dma_map_one_sg(vq, sg, DMA_FROM_DEVICE); > vq->vring.desc[i].len = sg->length; > prev = i; > i = vq->vring.desc[i].next; > @@ -269,7 +331,7 @@ static inline int virtqueue_add(struct virtqueue *_vq, > > add_head: > /* Set token. */ > - vq->data[head] = data; > + vq->desc_state[head].data = data; > > /* Put entry in available array (but don't update avail->idx until they > * do sync). */ > @@ -470,22 +532,33 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head) > unsigned int i; > > /* Clear data ptr. */ > - vq->data[head] = NULL; > + vq->desc_state[head].data = NULL; > > /* Put back on free list: find end */ > i = head; > > /* Free the indirect table */ > - if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT) > - kfree(phys_to_virt(vq->vring.desc[i].addr)); > + if (vq->desc_state[head].indir_desc) { > + u32 len = vq->vring.desc[i].len; > + > + BUG_ON(!(vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT)); > + BUG_ON(len == 0 || len % sizeof(struct vring_desc)); > + unmap_indirect(vq, vq->desc_state[head].indir_desc, > + len / sizeof(struct vring_desc)); > + kfree(vq->desc_state[head].indir_desc); > + vq->desc_state[head].indir_desc = NULL; > + } > > while (vq->vring.desc[i].flags & VRING_DESC_F_NEXT) { > + unmap_one(vq, &vq->vring.desc[i]); > i = vq->vring.desc[i].next; > vq->vq.num_free++; > } > > + unmap_one(vq, &vq->vring.desc[i]); > vq->vring.desc[i].next = vq->free_head; > vq->free_head = head; > + > /* Plus final descriptor */ > vq->vq.num_free++; > } > @@ -542,13 +615,13 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len) > BAD_RING(vq, "id %u out of range\n", i); > return NULL; > } > - if (unlikely(!vq->data[i])) { > + if (unlikely(!vq->desc_state[i].data)) { > BAD_RING(vq, "id %u is not a head!\n", i); > return NULL; > } > > /* detach_buf clears data, so grab it now. */ > - ret = vq->data[i]; > + ret = vq->desc_state[i].data; > detach_buf(vq, i); > vq->last_used_idx++; > /* If we expect an interrupt for the next entry, tell host > @@ -709,10 +782,10 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq) > START_USE(vq); > > for (i = 0; i < vq->vring.num; i++) { > - if (!vq->data[i]) > + if (!vq->desc_state[i].data) > continue; > /* detach_buf clears data, so grab it now. */ > - buf = vq->data[i]; > + buf = vq->desc_state[i].data; > detach_buf(vq, i); > vq->vring.avail->idx--; > END_USE(vq); > @@ -765,7 +838,8 @@ struct virtqueue *vring_new_virtqueue(unsigned int index, > return NULL; > } > > - vq = kmalloc(sizeof(*vq) + sizeof(void *)*num, GFP_KERNEL); > + vq = kmalloc(sizeof(*vq) + num * sizeof(struct vring_desc_state), > + GFP_KERNEL); > if (!vq) > return NULL; > > @@ -795,11 +869,9 @@ struct virtqueue *vring_new_virtqueue(unsigned int index, > > /* Put everything in free lists. */ > vq->free_head = 0; > - for (i = 0; i < num-1; i++) { > + for (i = 0; i < num-1; i++) > vq->vring.desc[i].next = i+1; > - vq->data[i] = NULL; > - } > - vq->data[i] = NULL; > + memset(vq->desc_state, 0, num * sizeof(struct vring_desc_state)); > > return &vq->vq; > } >
On Wed, Aug 27, 2014 at 09:29:36AM +0200, Christian Borntraeger wrote:> On 26/08/14 23:17, 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 this fix as well as a corresponding > > fix to virtio_pci or to another driver. > > > > With this patch, virtfs survives kmemleak and CONFIG_DMA_API_DEBUG. > > virtio-net warns (correctly) about DMA from the stack in > > virtnet_set_rx_mode. > > > > This breaks s390's defconfig. The default configuration for s390 > > does virtio through a KVM-specific interface, but that configuration > > does not support DMA. I could modify this patch to stub out the DMA > > API calls if !CONFIG_HAS_DMA, but it seems to me that it would be > > much nicer to make s390 support DMA unconditionally. > > s390 has no DMA per se. Newest systems have a PCI-like I/O attach in > addition to the classic channel I/O, and for that we enable the DMA code > just for that transport to be able to reuse some of the existing PCI > drivers. (only some because, we differ in some aspects from how PCI > looks like) But the architecture itself (and the virtio interface) does > not provide the DMA interface as you know it:Don't most of those DMA_API end up then being nops? As in, we do have in the dma-api file the #ifdef case when a platform does not do DMA which ends up with all functions stubbed out.> > In essence this patch is a no-go for s390.Is that due to possible compilation?> > I am also aksing myself, if this makes virtio-ring more expensive? > > Christian > > > > > > > It's actually unclear to me whether this can be fixed without either > > s390 arch support or a special case for s390 in virtio_ring: on > > brief inspection of s390's DMA code, it seems to assume that > > everything goes through a PCI IOMMU, which is presumably not the > > case for virtio. > > > > > > Cc: Cornelia Huck <cornelia.huck at de.ibm.com> > > Signed-off-by: Andy Lutomirski <luto at amacapital.net> > > --- > > drivers/virtio/Kconfig | 1 + > > drivers/virtio/virtio_ring.c | 116 +++++++++++++++++++++++++++++++++++-------- > > 2 files changed, 95 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig > > index c6683f2e396c..b6f3acc61153 100644 > > --- a/drivers/virtio/Kconfig > > +++ b/drivers/virtio/Kconfig > > @@ -1,5 +1,6 @@ > > config VIRTIO > > tristate > > + depends on HAS_DMA > > ---help--- > > This option is selected by any driver which implements the virtio > > bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_LGUEST, > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index d356a701c9c2..6a78e2fd8e63 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -24,6 +24,7 @@ > > #include <linux/module.h> > > #include <linux/hrtimer.h> > > #include <linux/kmemleak.h> > > +#include <linux/dma-mapping.h> > > > > #ifdef DEBUG > > /* For development, we want to crash whenever the ring is screwed. */ > > @@ -54,6 +55,12 @@ > > #define END_USE(vq) > > #endif > > > > +struct vring_desc_state > > +{ > > + void *data; /* Data for callback. */ > > + struct vring_desc *indir_desc; /* Indirect descriptor, if any. */ > > +}; > > + > > struct vring_virtqueue > > { > > struct virtqueue vq; > > @@ -93,12 +100,45 @@ struct vring_virtqueue > > ktime_t last_add_time; > > #endif > > > > - /* Tokens for callbacks. */ > > - void *data[]; > > + /* Per-descriptor state. */ > > + struct vring_desc_state desc_state[]; > > }; > > > > #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq) > > > > +/* Helper to map one sg entry, since we can't use dma_map_sg. */ > > +static dma_addr_t dma_map_one_sg(struct vring_virtqueue *vq, > > + struct scatterlist *sg, > > + enum dma_data_direction direction) > > +{ > > + return dma_map_page(vq->vq.vdev->dev.parent, > > + sg_page(sg), sg->offset, sg->length, direction); > > +} > > + > > +static void unmap_one(struct vring_virtqueue *vq, struct vring_desc *desc) > > +{ > > + if (desc->flags & VRING_DESC_F_INDIRECT) { > > + dma_unmap_single(vq->vq.vdev->dev.parent, > > + desc->addr, desc->len, > > + (desc->flags & VRING_DESC_F_WRITE) ? > > + DMA_FROM_DEVICE : DMA_TO_DEVICE); > > + } else { > > + dma_unmap_page(vq->vq.vdev->dev.parent, > > + desc->addr, desc->len, > > + (desc->flags & VRING_DESC_F_WRITE) ? > > + DMA_FROM_DEVICE : DMA_TO_DEVICE); > > + } > > +} > > + > > +static void unmap_indirect(struct vring_virtqueue *vq, struct vring_desc *desc, > > + int total) > > +{ > > + int i; > > + > > + for (i = 0; i < total; i++) > > + unmap_one(vq, &desc[i]); > > +} > > + > > /* Set up an indirect table of descriptors and add it to the queue. */ > > static inline int vring_add_indirect(struct vring_virtqueue *vq, > > struct scatterlist *sgs[], > > @@ -132,7 +172,11 @@ static inline int vring_add_indirect(struct vring_virtqueue *vq, > > if (!sg) > > break; > > desc[i].flags = VRING_DESC_F_NEXT; > > - desc[i].addr = sg_phys(sg); > > + desc[i].addr > > + dma_map_one_sg(vq, sg, DMA_TO_DEVICE); > > + if (dma_mapping_error(vq->vq.vdev->dev.parent, > > + desc[i].addr)) > > + goto unmap_free; > > desc[i].len = sg->length; > > desc[i].next = i+1; > > i++; > > @@ -143,7 +187,11 @@ static inline int vring_add_indirect(struct vring_virtqueue *vq, > > if (!sg) > > break; > > desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE; > > - desc[i].addr = sg_phys(sg); > > + desc[i].addr > > + dma_map_one_sg(vq, sg, DMA_FROM_DEVICE); > > + if (dma_mapping_error(vq->vq.vdev->dev.parent, > > + desc[i].addr)) > > + goto unmap_free; > > desc[i].len = sg->length; > > desc[i].next = i+1; > > i++; > > @@ -161,15 +209,27 @@ static inline int vring_add_indirect(struct vring_virtqueue *vq, > > /* Use a single buffer which doesn't continue */ > > head = vq->free_head; > > vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT; > > - vq->vring.desc[head].addr = virt_to_phys(desc); > > - /* kmemleak gives a false positive, as it's hidden by virt_to_phys */ > > - kmemleak_ignore(desc); > > + vq->vring.desc[head].addr > > + dma_map_single(vq->vq.vdev->dev.parent, > > + desc, i * sizeof(struct vring_desc), > > + DMA_TO_DEVICE); > > + if (dma_mapping_error(vq->vq.vdev->dev.parent, > > + vq->vring.desc[head].addr)) > > + goto unmap_free; > > vq->vring.desc[head].len = i * sizeof(struct vring_desc); > > > > /* Update free pointer */ > > vq->free_head = vq->vring.desc[head].next; > > > > + /* Save the indirect block */ > > + vq->desc_state[head].indir_desc = desc; > > + > > return head; > > + > > +unmap_free: > > + unmap_indirect(vq, desc, i); > > + kfree(desc); > > + return -ENOMEM; > > } > > > > static inline int virtqueue_add(struct virtqueue *_vq, > > @@ -244,7 +304,8 @@ static inline int virtqueue_add(struct virtqueue *_vq, > > if (!sg) > > break; > > vq->vring.desc[i].flags = VRING_DESC_F_NEXT; > > - vq->vring.desc[i].addr = sg_phys(sg); > > + vq->vring.desc[i].addr > > + dma_map_one_sg(vq, sg, DMA_TO_DEVICE); > > vq->vring.desc[i].len = sg->length; > > prev = i; > > i = vq->vring.desc[i].next; > > @@ -255,7 +316,8 @@ static inline int virtqueue_add(struct virtqueue *_vq, > > if (!sg) > > break; > > vq->vring.desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE; > > - vq->vring.desc[i].addr = sg_phys(sg); > > + vq->vring.desc[i].addr > > + dma_map_one_sg(vq, sg, DMA_FROM_DEVICE); > > vq->vring.desc[i].len = sg->length; > > prev = i; > > i = vq->vring.desc[i].next; > > @@ -269,7 +331,7 @@ static inline int virtqueue_add(struct virtqueue *_vq, > > > > add_head: > > /* Set token. */ > > - vq->data[head] = data; > > + vq->desc_state[head].data = data; > > > > /* Put entry in available array (but don't update avail->idx until they > > * do sync). */ > > @@ -470,22 +532,33 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head) > > unsigned int i; > > > > /* Clear data ptr. */ > > - vq->data[head] = NULL; > > + vq->desc_state[head].data = NULL; > > > > /* Put back on free list: find end */ > > i = head; > > > > /* Free the indirect table */ > > - if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT) > > - kfree(phys_to_virt(vq->vring.desc[i].addr)); > > + if (vq->desc_state[head].indir_desc) { > > + u32 len = vq->vring.desc[i].len; > > + > > + BUG_ON(!(vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT)); > > + BUG_ON(len == 0 || len % sizeof(struct vring_desc)); > > + unmap_indirect(vq, vq->desc_state[head].indir_desc, > > + len / sizeof(struct vring_desc)); > > + kfree(vq->desc_state[head].indir_desc); > > + vq->desc_state[head].indir_desc = NULL; > > + } > > > > while (vq->vring.desc[i].flags & VRING_DESC_F_NEXT) { > > + unmap_one(vq, &vq->vring.desc[i]); > > i = vq->vring.desc[i].next; > > vq->vq.num_free++; > > } > > > > + unmap_one(vq, &vq->vring.desc[i]); > > vq->vring.desc[i].next = vq->free_head; > > vq->free_head = head; > > + > > /* Plus final descriptor */ > > vq->vq.num_free++; > > } > > @@ -542,13 +615,13 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len) > > BAD_RING(vq, "id %u out of range\n", i); > > return NULL; > > } > > - if (unlikely(!vq->data[i])) { > > + if (unlikely(!vq->desc_state[i].data)) { > > BAD_RING(vq, "id %u is not a head!\n", i); > > return NULL; > > } > > > > /* detach_buf clears data, so grab it now. */ > > - ret = vq->data[i]; > > + ret = vq->desc_state[i].data; > > detach_buf(vq, i); > > vq->last_used_idx++; > > /* If we expect an interrupt for the next entry, tell host > > @@ -709,10 +782,10 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq) > > START_USE(vq); > > > > for (i = 0; i < vq->vring.num; i++) { > > - if (!vq->data[i]) > > + if (!vq->desc_state[i].data) > > continue; > > /* detach_buf clears data, so grab it now. */ > > - buf = vq->data[i]; > > + buf = vq->desc_state[i].data; > > detach_buf(vq, i); > > vq->vring.avail->idx--; > > END_USE(vq); > > @@ -765,7 +838,8 @@ struct virtqueue *vring_new_virtqueue(unsigned int index, > > return NULL; > > } > > > > - vq = kmalloc(sizeof(*vq) + sizeof(void *)*num, GFP_KERNEL); > > + vq = kmalloc(sizeof(*vq) + num * sizeof(struct vring_desc_state), > > + GFP_KERNEL); > > if (!vq) > > return NULL; > > > > @@ -795,11 +869,9 @@ struct virtqueue *vring_new_virtqueue(unsigned int index, > > > > /* Put everything in free lists. */ > > vq->free_head = 0; > > - for (i = 0; i < num-1; i++) { > > + for (i = 0; i < num-1; i++) > > vq->vring.desc[i].next = i+1; > > - vq->data[i] = NULL; > > - } > > - vq->data[i] = NULL; > > + memset(vq->desc_state, 0, num * sizeof(struct vring_desc_state)); > > > > return &vq->vq; > > } > > >
On Wed, Aug 27, 2014 at 10:35 AM, Konrad Rzeszutek Wilk <konrad.wilk at oracle.com> wrote:> On Wed, Aug 27, 2014 at 09:29:36AM +0200, Christian Borntraeger wrote: >> On 26/08/14 23:17, 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 this fix as well as a corresponding >> > fix to virtio_pci or to another driver. >> > >> > With this patch, virtfs survives kmemleak and CONFIG_DMA_API_DEBUG. >> > virtio-net warns (correctly) about DMA from the stack in >> > virtnet_set_rx_mode. >> > >> > This breaks s390's defconfig. The default configuration for s390 >> > does virtio through a KVM-specific interface, but that configuration >> > does not support DMA. I could modify this patch to stub out the DMA >> > API calls if !CONFIG_HAS_DMA, but it seems to me that it would be >> > much nicer to make s390 support DMA unconditionally. >> >> s390 has no DMA per se. Newest systems have a PCI-like I/O attach in >> addition to the classic channel I/O, and for that we enable the DMA code >> just for that transport to be able to reuse some of the existing PCI >> drivers. (only some because, we differ in some aspects from how PCI >> looks like) But the architecture itself (and the virtio interface) does >> not provide the DMA interface as you know it: > > Don't most of those DMA_API end up then being nops? As in, we do > have in the dma-api file the #ifdef case when a platform does not do > DMA which ends up with all functions stubbed out. >This doesn't work on s390. It fails to link. This was my initial thought, too -- it would be nice if s390 set up no-op DMA ops its virtio device. But this might be overkill, if virtio is the *only* device on s390 that does anything that looks like DMA. Anyway, v2 (coming soon) adds a switch in virtio, so s390 should continue to work. --Andy