Re: [PATCH v7 9/9] vring: Use the DMA API on Xen

[David Vrabel]

On 03/02/16 05:46, Andy Lutomirski wrote:
> Signed-off-by: Andy Lutomirski 

You forgot the previous Reviewed-by tags.

David
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v7 5/9] virtio_ring: Support DMA APIs

[Michael S. Tsirkin]

On Tue, Feb 02, 2016 at 09:46:36PM -0800, 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.
> 
> Signed-off-by: Andy Lutomirski 
> ---
>  drivers/virtio/Kconfig   |   2 +-
>  drivers/virtio/virtio_ring.c | 200 
> ---
>  tools/virtio/linux/dma-mapping.h |  17 
>  3 files changed, 183 insertions(+), 36 deletions(-)
>  create mode 100644 tools/virtio/linux/dma-mapping.h
> 
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index cab9f3f63a38..77590320d44c 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -60,7 +60,7 @@ config VIRTIO_INPUT
>  
>   config VIRTIO_MMIO
>   tristate "Platform bus driver for memory mapped virtio devices"
> - depends on HAS_IOMEM
> + depends on HAS_IOMEM && HAS_DMA
>   select VIRTIO
>   ---help---
>This drivers provides support for memory mapped virtio

What's this chunk doing here btw? Should be part of the mmio patch?

> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index ab0be6c084f6..9abc008ff7ea 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -24,6 +24,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #ifdef DEBUG
>  /* For development, we want to crash whenever the ring is screwed. */
> @@ -54,6 +55,11 @@
>  #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;
>  
> @@ -98,8 +104,8 @@ 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)
> @@ -128,6 +134,79 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
>   return false;
>  }
>  
> +/*
> + * The DMA ops on various arches are rather gnarly right now, and
> + * making all of the arch DMA ops work on the vring device itself
> + * is a mess.  For now, we use the parent device for DMA ops.
> + */
> +struct device *vring_dma_dev(const struct vring_virtqueue *vq)
> +{
> + return vq->vq.vdev->dev.parent;
> +}
> +
> +/* Map one sg entry. */
> +static dma_addr_t vring_map_one_sg(const struct vring_virtqueue *vq,
> +struct scatterlist *sg,
> +enum dma_data_direction direction)
> +{
> + if (!vring_use_dma_api(vq->vq.vdev))
> + return (dma_addr_t)sg_phys(sg);
> +
> + /*
> +  * We can't use dma_map_sg, because we don't use scatterlists in
> +  * the way it expects (we don't guarantee that the scatterlist
> +  * will exist for the lifetime of the mapping).
> +  */
> + return dma_map_page(vring_dma_dev(vq),
> + sg_page(sg), sg->offset, sg->length,
> + direction);
> +}
> +
> +static dma_addr_t vring_map_single(const struct vring_virtqueue *vq,
> +void *cpu_addr, size_t size,
> +enum dma_data_direction direction)
> +{
> + if (!vring_use_dma_api(vq->vq.vdev))
> + return (dma_addr_t)virt_to_phys(cpu_addr);
> +
> + return dma_map_single(vring_dma_dev(vq),
> +   cpu_addr, size, direction);
> +}
> +
> +static void vring_unmap_one(const struct vring_virtqueue *vq,
> + struct vring_desc *desc)
> +{
> + u16 flags;
> +
> + if (!vring_use_dma_api(vq->vq.vdev))
> + return;
> +
> + flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);
> +
> + if (flags & VRING_DESC_F_INDIRECT) {
> + dma_unmap_single(vring_dma_dev(vq),
> +  virtio64_to_cpu(vq->vq.vdev, desc->addr),
> +  virtio32_to_cpu(vq->vq.vdev, desc->len),
> +  (flags & VRING_DESC_F_WRITE) ?
> +  DMA_FROM_DEVICE : DMA_TO_DEVICE);
> + } else {
> + dma_unmap_page(vring_dma_dev(vq),
> +virtio64_to_cpu(vq->vq.vdev, desc->addr),
> +virtio32_to_cpu(vq->vq.vdev, desc->len),
> +   

Re: [PATCH net-next] virtio_net: add ethtool support for set and get of settings

[Stephen Hemminger]

On Tue,  2 Feb 2016 13:51:20 +0100
Nikolay Aleksandrov  wrote:

> +static bool virtnet_validate_speed(u32 speed)
> +{
> + switch (speed) {
> + case SPEED_10:
> + case SPEED_100:
> + case SPEED_1000:
> + case SPEED_2500:
> + case SPEED_5000:
> + case SPEED_1:
> + case SPEED_2:
> + case SPEED_25000:
> + case SPEED_4:
> + case SPEED_5:
> + case SPEED_56000:
> + case SPEED_10:
> + case SPEED_UNKNOWN:
> + return true;
> + }
> +
> + return false;
> +}

Why limit to only known values. This switch() will get out of
date when some vendor introduces 64G or some other weird value.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v7 0/9] virtio DMA API, yet again

[Stefano Stabellini]

On Tue, 2 Feb 2016, Andy Lutomirski wrote:
> This switches virtio to use the DMA API on Xen and if requested by
> module option.
> 
> This fixes virtio on Xen, and it should break anything because it's
> off by default on everything except Xen PV on x86.
> 
> To the Xen people: is this okay?  If it doesn't work on other Xen
> variants (PVH? HVM?), can you submit follow-up patches to fix it?

I have been waiting for something like this for a long time: up to now
it wasn't possible to use Xen inside a VM with virtio devices.

You can add my:

Tested-by: Stefano Stabellini 


> To everyone else: we've waffled on this for way too long.  I think
> we should to get DMA API implementation in with a conservative
> policy like this rather than waiting until we achieve perfection.
> I'm tired of carrying these patches around.
> 
> I changed queue allocation around a bit in this version.  Per Michael's
> request, we no longer use dma_zalloc_coherent in the !dma_api case.
> Instead we use alloc_pages_exact, just like the current code does.
> This simplifies the ring address accessors, because they can always
> load from the dma addr rather than depending on vring_use_dma_api
> themselves.
> 
> There's an odd warning in here if the ring's physical address
> doesn't fit in a dma_addr_t.  This could only possible happen on
> really weird configurations in which phys_addr_t is wider than
> dma_addr_t.  AFAICT this is only possible on i386 PAE systems and on
> MIPS, and even there it only happens if highmem is off.  But that
> means we're safe, since we should never end up with high allocations
> on non-highmem systems unless we explicitly ask for them, which we
> don't.
> 
> If this is too scary, I can add yet more cruft to avoid it, but
> it seems harmless enough to me, and it means that the driver will
> be totally clean once all the vring_use_dma_api calls go away.
> 
> Michael, if these survive review, can you stage these in your tree?
> Can you also take a look at tools/virtio?  I probably broke it, but I
> couldn't get it to build without these patches either, so I'm stuck.
> 
> Changes from v6:
>  - Remove HAVE_DMA_ATTRS and add Acked-by (Cornelia)
>  - Add some missing signed-off-by lines from me (whoops)
>  - Rework queue allocation (Michael)
> 
> Changes from v5:
>  - Typo fixes (David Woodhouse)
>  - Use xen_domain() to detect Xen (David Vrabel)
>  - Pass struct vring_virtqueue * into vring_use_dma_api for future proofing
>  - Removed module parameter (Michael)
> 
> Changes from v4:
>  - Bake vring_use_dma_api in from the beginning.
>  - Automatically enable only on Xen.
>  - Add module parameter.
>  - Add s390 and alpha DMA API implementations.
>  - Rebase to 4.5-rc1.
> 
> Changes from v3:
>  - More big-endian fixes.
>  - Added better virtio-ring APIs that handle allocation and use them in
>virtio-mmio and virtio-pci.
>  - Switch to Michael's virtio-net patch.
> 
> Changes from v2:
>  - Fix vring_mapping_error incorrect argument
> 
> Changes from v1:
>  - Fix an endian conversion error causing a BUG to hit.
>  - Fix a DMA ordering issue (swiotlb=force works now).
>  - Minor cleanups.
> 
> Andy Lutomirski (6):
>   vring: Introduce vring_use_dma_api()
>   virtio_ring: Support DMA APIs
>   virtio: Add improved queue allocation API
>   virtio_mmio: Use the DMA API if enabled
>   virtio_pci: Use the DMA API if enabled
>   vring: Use the DMA API on Xen
> 
> Christian Borntraeger (3):
>   dma: Provide simple noop dma ops
>   alpha/dma: use common noop dma ops
>   s390/dma: Allow per device dma ops
> 
>  arch/alpha/kernel/pci-noop.c|  46 +---
>  arch/s390/Kconfig   |   5 +-
>  arch/s390/include/asm/device.h  |   6 +-
>  arch/s390/include/asm/dma-mapping.h |   6 +-
>  arch/s390/pci/pci.c |   1 +
>  arch/s390/pci/pci_dma.c |   4 +-
>  drivers/virtio/Kconfig  |   2 +-
>  drivers/virtio/virtio_mmio.c|  67 ++
>  drivers/virtio/virtio_pci_common.h  |   6 -
>  drivers/virtio/virtio_pci_legacy.c  |  42 ++--
>  drivers/virtio/virtio_pci_modern.c  |  61 ++---
>  drivers/virtio/virtio_ring.c| 439 
> +++-
>  include/linux/dma-mapping.h |   2 +
>  include/linux/virtio.h  |  23 +-
>  include/linux/virtio_ring.h |  35 +++
>  lib/Makefile|   1 +
>  lib/dma-noop.c  |  75 ++
>  tools/virtio/linux/dma-mapping.h|  17 ++
>  18 files changed, 594 insertions(+), 244 deletions(-)
>  create mode 100644 lib/dma-noop.c
>  create mode 100644 tools/virtio/linux/dma-mapping.h
> 
> -- 
> 2.5.0
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v7 5/9] virtio_ring: Support DMA APIs

[Andy Lutomirski]

On Feb 3, 2016 5:52 AM, "Michael S. Tsirkin"  wrote:
>
> On Tue, Feb 02, 2016 at 09:46:36PM -0800, 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.
> >
> > Signed-off-by: Andy Lutomirski 
> > ---
> >  drivers/virtio/Kconfig   |   2 +-
> >  drivers/virtio/virtio_ring.c | 200 
> > ---
> >  tools/virtio/linux/dma-mapping.h |  17 
> >  3 files changed, 183 insertions(+), 36 deletions(-)
> >  create mode 100644 tools/virtio/linux/dma-mapping.h
> >
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > index cab9f3f63a38..77590320d44c 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -60,7 +60,7 @@ config VIRTIO_INPUT
> >
> >   config VIRTIO_MMIO
> >   tristate "Platform bus driver for memory mapped virtio devices"
> > - depends on HAS_IOMEM
> > + depends on HAS_IOMEM && HAS_DMA
> >   select VIRTIO
> >   ---help---
> >This drivers provides support for memory mapped virtio
>
> What's this chunk doing here btw? Should be part of the mmio patch?
>

IIRC it was deliberate.  Making virtio depend on HAS_DMA didn't work
right because kconfig doesn't propagate dependencies through select
intelligently.  This patch makes core virtio depend on HAS_DMA, so I
added the dependency here, too.

--Andy
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization