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

From: Jason Wang
Date: Sun Dec 25 2022 - 23:15:38 EST

Next message: Chen-Yu Tsai: "Re: [PATCH v2 3/4] media: rkvdec: Re-enable H.264 error detection"
Previous message: Xu Yilun: "Re: [PATCH v2 1/1] fpga: m10bmc-sec: Fix probe rollback"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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
> >
>

Next message: Chen-Yu Tsai: "Re: [PATCH v2 3/4] media: rkvdec: Re-enable H.264 error detection"
Previous message: Xu Yilun: "Re: [PATCH v2 1/1] fpga: m10bmc-sec: Fix probe rollback"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]