Re: [PATCH] vdpa_sim: get rid of DMA ops

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Dec 23, 2022 at 5:27 PM Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote:
>
> On Fri, Dec 23, 2022 at 02:00:21PM +0800, Jason Wang wrote:
> >We used to (ab)use the DMA ops for setting up identical mappings in
> >the IOTLB. This patch tries to get rid of the those unnecessary DMA
> >ops by maintaining a simple identical/passthrough mappings by
> >default. When bound to virtio_vdpa driver, DMA API will simply use PA
> >as the IOVA and we will be all fine. When the vDPA bus tries to setup
> >customized mapping (e.g when bound to vhost-vDPA), the
> >identical/passthrough mapping will be removed.
> >
> >Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> >---
> >Note:
> >- This patch depends on the series "[PATCH V3 0/4] Vendor stats
> >  support in vdpasim_net"
> >---
> > drivers/vdpa/vdpa_sim/vdpa_sim.c | 170 ++++---------------------------
> > drivers/vdpa/vdpa_sim/vdpa_sim.h |   2 +-
> > 2 files changed, 22 insertions(+), 150 deletions(-)
> >
> >diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >index 45d3f84b7937..187fa3a0e5d5 100644
> >--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >@@ -17,7 +17,6 @@
> > #include <linux/vringh.h>
> > #include <linux/vdpa.h>
> > #include <linux/vhost_iotlb.h>
> >-#include <linux/iova.h>
> > #include <uapi/linux/vdpa.h>
> >
> > #include "vdpa_sim.h"
> >@@ -45,13 +44,6 @@ static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa)
> >       return container_of(vdpa, struct vdpasim, vdpa);
> > }
> >
> >-static struct vdpasim *dev_to_sim(struct device *dev)
> >-{
> >-      struct vdpa_device *vdpa = dev_to_vdpa(dev);
> >-
> >-      return vdpa_to_sim(vdpa);
> >-}
> >-
> > static void vdpasim_vq_notify(struct vringh *vring)
> > {
> >       struct vdpasim_virtqueue *vq =
> >@@ -104,8 +96,12 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
> >                                &vdpasim->iommu_lock);
> >       }
> >
> >-      for (i = 0; i < vdpasim->dev_attr.nas; i++)
> >+      for (i = 0; i < vdpasim->dev_attr.nas; i++) {
> >               vhost_iotlb_reset(&vdpasim->iommu[i]);
> >+              vhost_iotlb_add_range(&vdpasim->iommu[i], 0, ULONG_MAX,
> >+                                    0, VHOST_MAP_RW);
> >+              vdpasim->iommu_pt[i] = true;
> >+      }
> >
> >       vdpasim->running = true;
> >       spin_unlock(&vdpasim->iommu_lock);
> >@@ -115,133 +111,6 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
> >       ++vdpasim->generation;
> > }
> >
> >-static int dir_to_perm(enum dma_data_direction dir)
> >-{
> >-      int perm = -EFAULT;
> >-
> >-      switch (dir) {
> >-      case DMA_FROM_DEVICE:
> >-              perm = VHOST_MAP_WO;
> >-              break;
> >-      case DMA_TO_DEVICE:
> >-              perm = VHOST_MAP_RO;
> >-              break;
> >-      case DMA_BIDIRECTIONAL:
> >-              perm = VHOST_MAP_RW;
> >-              break;
> >-      default:
> >-              break;
> >-      }
> >-
> >-      return perm;
> >-}
> >-
> >-static dma_addr_t vdpasim_map_range(struct vdpasim *vdpasim, phys_addr_t paddr,
> >-                                  size_t size, unsigned int perm)
> >-{
> >-      struct iova *iova;
> >-      dma_addr_t dma_addr;
> >-      int ret;
> >-
> >-      /* We set the limit_pfn to the maximum (ULONG_MAX - 1) */
> >-      iova = alloc_iova(&vdpasim->iova, size >> iova_shift(&vdpasim->iova),
> >-                        ULONG_MAX - 1, true);
> >-      if (!iova)
> >-              return DMA_MAPPING_ERROR;
> >-
> >-      dma_addr = iova_dma_addr(&vdpasim->iova, iova);
> >-
> >-      spin_lock(&vdpasim->iommu_lock);
> >-      ret = vhost_iotlb_add_range(&vdpasim->iommu[0], (u64)dma_addr,
> >-                                  (u64)dma_addr + size - 1, (u64)paddr, perm);
> >-      spin_unlock(&vdpasim->iommu_lock);
> >-
> >-      if (ret) {
> >-              __free_iova(&vdpasim->iova, iova);
> >-              return DMA_MAPPING_ERROR;
> >-      }
> >-
> >-      return dma_addr;
> >-}
> >-
> >-static void vdpasim_unmap_range(struct vdpasim *vdpasim, dma_addr_t dma_addr,
> >-                              size_t size)
> >-{
> >-      spin_lock(&vdpasim->iommu_lock);
> >-      vhost_iotlb_del_range(&vdpasim->iommu[0], (u64)dma_addr,
> >-                            (u64)dma_addr + size - 1);
> >-      spin_unlock(&vdpasim->iommu_lock);
> >-
> >-      free_iova(&vdpasim->iova, iova_pfn(&vdpasim->iova, dma_addr));
> >-}
> >-
> >-static dma_addr_t vdpasim_map_page(struct device *dev, struct page *page,
> >-                                 unsigned long offset, size_t size,
> >-                                 enum dma_data_direction dir,
> >-                                 unsigned long attrs)
> >-{
> >-      struct vdpasim *vdpasim = dev_to_sim(dev);
> >-      phys_addr_t paddr = page_to_phys(page) + offset;
> >-      int perm = dir_to_perm(dir);
> >-
> >-      if (perm < 0)
> >-              return DMA_MAPPING_ERROR;
> >-
> >-      return vdpasim_map_range(vdpasim, paddr, size, perm);
> >-}
> >-
> >-static void vdpasim_unmap_page(struct device *dev, dma_addr_t dma_addr,
> >-                             size_t size, enum dma_data_direction dir,
> >-                             unsigned long attrs)
> >-{
> >-      struct vdpasim *vdpasim = dev_to_sim(dev);
> >-
> >-      vdpasim_unmap_range(vdpasim, dma_addr, size);
> >-}
> >-
> >-static void *vdpasim_alloc_coherent(struct device *dev, size_t size,
> >-                                  dma_addr_t *dma_addr, gfp_t flag,
> >-                                  unsigned long attrs)
> >-{
> >-      struct vdpasim *vdpasim = dev_to_sim(dev);
> >-      phys_addr_t paddr;
> >-      void *addr;
> >-
> >-      addr = kmalloc(size, flag);
> >-      if (!addr) {
> >-              *dma_addr = DMA_MAPPING_ERROR;
> >-              return NULL;
> >-      }
> >-
> >-      paddr = virt_to_phys(addr);
> >-
> >-      *dma_addr = vdpasim_map_range(vdpasim, paddr, size, VHOST_MAP_RW);
> >-      if (*dma_addr == DMA_MAPPING_ERROR) {
> >-              kfree(addr);
> >-              return NULL;
> >-      }
> >-
> >-      return addr;
> >-}
> >-
> >-static void vdpasim_free_coherent(struct device *dev, size_t size,
> >-                                void *vaddr, dma_addr_t dma_addr,
> >-                                unsigned long attrs)
> >-{
> >-      struct vdpasim *vdpasim = dev_to_sim(dev);
> >-
> >-      vdpasim_unmap_range(vdpasim, dma_addr, size);
> >-
> >-      kfree(vaddr);
> >-}
> >-
> >-static const struct dma_map_ops vdpasim_dma_ops = {
> >-      .map_page = vdpasim_map_page,
> >-      .unmap_page = vdpasim_unmap_page,
> >-      .alloc = vdpasim_alloc_coherent,
> >-      .free = vdpasim_free_coherent,
> >-};
> >-
> > static const struct vdpa_config_ops vdpasim_config_ops;
> > static const struct vdpa_config_ops vdpasim_batch_config_ops;
> >
> >@@ -289,7 +158,6 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr,
> >       dev->dma_mask = &dev->coherent_dma_mask;
> >       if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)))
> >               goto err_iommu;
> >-      set_dma_ops(dev, &vdpasim_dma_ops);
> >       vdpasim->vdpa.mdev = dev_attr->mgmt_dev;
> >
> >       vdpasim->config = kzalloc(dev_attr->config_size, GFP_KERNEL);
> >@@ -306,6 +174,11 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr,
> >       if (!vdpasim->iommu)
> >               goto err_iommu;
> >
> >+      vdpasim->iommu_pt = kmalloc_array(vdpasim->dev_attr.nas,
> >+                                        sizeof(*vdpasim->iommu_pt), GFP_KERNEL);
> >+      if (!vdpasim->iommu_pt)
> >+              goto err_iommu;
> >+
> >       for (i = 0; i < vdpasim->dev_attr.nas; i++)
> >               vhost_iotlb_init(&vdpasim->iommu[i], max_iotlb_entries, 0);
> >
> >@@ -317,13 +190,6 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr,
> >               vringh_set_iotlb(&vdpasim->vqs[i].vring, &vdpasim->iommu[0],
> >                                &vdpasim->iommu_lock);
> >
> >-      ret = iova_cache_get();
> >-      if (ret)
> >-              goto err_iommu;
> >-
> >-      /* For simplicity we use an IOVA allocator with byte granularity */
> >-      init_iova_domain(&vdpasim->iova, 1, 0);
> >-
> >       vdpasim->vdpa.dma_dev = dev;
> >
> >       return vdpasim;
> >@@ -639,6 +505,7 @@ static int vdpasim_set_map(struct vdpa_device *vdpa, unsigned int asid,
> >
> >       iommu = &vdpasim->iommu[asid];
> >       vhost_iotlb_reset(iommu);
> >+      vdpasim->iommu_pt[asid] = false;
> >
> >       for (map = vhost_iotlb_itree_first(iotlb, start, last); map;
> >            map = vhost_iotlb_itree_next(map, start, last)) {
> >@@ -667,6 +534,10 @@ static int vdpasim_dma_map(struct vdpa_device *vdpa, unsigned int asid,
> >               return -EINVAL;
> >
> >       spin_lock(&vdpasim->iommu_lock);
> >+      if (vdpasim->iommu_pt[asid]) {
> >+              vhost_iotlb_reset(&vdpasim->iommu[asid]);
> >+              vdpasim->iommu_pt[asid] = false;
> >+      }
> >       ret = vhost_iotlb_add_range_ctx(&vdpasim->iommu[asid], iova,
> >                                       iova + size - 1, pa, perm, opaque);
> >       spin_unlock(&vdpasim->iommu_lock);
> >@@ -682,6 +553,11 @@ static int vdpasim_dma_unmap(struct vdpa_device *vdpa, unsigned int asid,
> >       if (asid >= vdpasim->dev_attr.nas)
> >               return -EINVAL;
> >
> >+      if (vdpasim->iommu_pt[asid]) {
>
> We are in the vdpasim_dma_unmap, so if vdpasim->iommu_pt[asid] is true,
> should be better to return an error, since this case should not happen?

So it's a question of how to behave when unmap is called without a
map. I think we can leave the code as is or if we wish, it needs a
separate patch.

(We didn't error this previously anyhow).

Thanks

>
> The rest LGTM!
>
> Thanks,
> Stefano
>
> >+              vhost_iotlb_reset(&vdpasim->iommu[asid]);
> >+              vdpasim->iommu_pt[asid] = false;
> >+      }
> >+
> >       spin_lock(&vdpasim->iommu_lock);
> >       vhost_iotlb_del_range(&vdpasim->iommu[asid], iova, iova + size - 1);
> >       spin_unlock(&vdpasim->iommu_lock);
> >@@ -701,15 +577,11 @@ static void vdpasim_free(struct vdpa_device *vdpa)
> >               vringh_kiov_cleanup(&vdpasim->vqs[i].in_iov);
> >       }
> >
> >-      if (vdpa_get_dma_dev(vdpa)) {
> >-              put_iova_domain(&vdpasim->iova);
> >-              iova_cache_put();
> >-      }
> >-
> >       kvfree(vdpasim->buffer);
> >       for (i = 0; i < vdpasim->dev_attr.nas; i++)
> >               vhost_iotlb_reset(&vdpasim->iommu[i]);
> >       kfree(vdpasim->iommu);
> >+      kfree(vdpasim->iommu_pt);
> >       kfree(vdpasim->vqs);
> >       kfree(vdpasim->config);
> > }
> >diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> >index d2a08c0abad7..770ef3408619 100644
> >--- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
> >+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> >@@ -64,7 +64,7 @@ struct vdpasim {
> >       /* virtio config according to device type */
> >       void *config;
> >       struct vhost_iotlb *iommu;
> >-      struct iova_domain iova;
> >+      bool *iommu_pt;
> >       void *buffer;
> >       u32 status;
> >       u32 generation;
> >--
> >2.25.1
> >
>

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization



[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux