* [PATCH] vdpa_sim: get rid of DMA ops
@ 2022-12-23 6:00 ` Jason Wang
0 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2022-12-23 6:00 UTC (permalink / raw)
To: mst, jasowang
Cc: sgarzare, eperezma, virtualization, linux-kernel, xieyongji, hch
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@redhat.com>
---
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]) {
+ 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] vdpa_sim: get rid of DMA ops
@ 2022-12-23 6:00 ` Jason Wang
0 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2022-12-23 6:00 UTC (permalink / raw)
To: mst, jasowang; +Cc: xieyongji, linux-kernel, virtualization, eperezma, hch
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@redhat.com>
---
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]) {
+ 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@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] vdpa_sim: get rid of DMA ops
2022-12-23 6:00 ` Jason Wang
@ 2022-12-23 6:15 ` Christoph Hellwig
-1 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2022-12-23 6:15 UTC (permalink / raw)
To: Jason Wang
Cc: mst, sgarzare, eperezma, virtualization, linux-kernel, xieyongji, hch
Looks good from the DMA subsysten POV:
Acked-by: Christoph Hellwig <hch@lst.de>
Thanks for doing the work!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] vdpa_sim: get rid of DMA ops
@ 2022-12-23 6:15 ` Christoph Hellwig
0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2022-12-23 6:15 UTC (permalink / raw)
To: Jason Wang; +Cc: xieyongji, mst, linux-kernel, virtualization, eperezma, hch
Looks good from the DMA subsysten POV:
Acked-by: Christoph Hellwig <hch@lst.de>
Thanks for doing the work!
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] vdpa_sim: get rid of DMA ops
2022-12-23 6:00 ` Jason Wang
@ 2022-12-23 9:26 ` Stefano Garzarella
-1 siblings, 0 replies; 12+ messages in thread
From: Stefano Garzarella @ 2022-12-23 9:26 UTC (permalink / raw)
To: Jason Wang; +Cc: xieyongji, mst, linux-kernel, virtualization, eperezma, hch
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@redhat.com>
>---
>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?
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@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] vdpa_sim: get rid of DMA ops
@ 2022-12-23 9:26 ` Stefano Garzarella
0 siblings, 0 replies; 12+ messages in thread
From: Stefano Garzarella @ 2022-12-23 9:26 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, eperezma, virtualization, linux-kernel, xieyongji, hch
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@redhat.com>
>---
>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?
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] vdpa_sim: get rid of DMA ops
2022-12-23 9:26 ` Stefano Garzarella
@ 2022-12-26 4:12 ` Jason Wang
-1 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2022-12-26 4:12 UTC (permalink / raw)
To: Stefano Garzarella
Cc: xieyongji, mst, linux-kernel, virtualization, eperezma, hch
On Fri, Dec 23, 2022 at 5:27 PM Stefano Garzarella <sgarzare@redhat.com> 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@redhat.com>
> >---
> >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@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] vdpa_sim: get rid of DMA ops
@ 2022-12-26 4:12 ` Jason Wang
0 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2022-12-26 4:12 UTC (permalink / raw)
To: Stefano Garzarella
Cc: mst, eperezma, virtualization, linux-kernel, xieyongji, hch
On Fri, Dec 23, 2022 at 5:27 PM Stefano Garzarella <sgarzare@redhat.com> 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@redhat.com>
> >---
> >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
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] vdpa_sim: get rid of DMA ops
2022-12-26 4:12 ` Jason Wang
@ 2023-01-27 10:29 ` Michael S. Tsirkin
-1 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2023-01-27 10:29 UTC (permalink / raw)
To: Jason Wang; +Cc: xieyongji, linux-kernel, virtualization, eperezma, hch
On Mon, Dec 26, 2022 at 12:12:42PM +0800, Jason Wang wrote:
> > >@@ -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
OK I picked as is. Do we want WARN_ON maybe?
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] vdpa_sim: get rid of DMA ops
@ 2023-01-27 10:29 ` Michael S. Tsirkin
0 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2023-01-27 10:29 UTC (permalink / raw)
To: Jason Wang
Cc: Stefano Garzarella, eperezma, virtualization, linux-kernel,
xieyongji, hch
On Mon, Dec 26, 2022 at 12:12:42PM +0800, Jason Wang wrote:
> > >@@ -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
OK I picked as is. Do we want WARN_ON maybe?
--
MST
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] vdpa_sim: get rid of DMA ops
2023-01-27 10:29 ` Michael S. Tsirkin
@ 2023-01-29 5:47 ` Jason Wang
-1 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2023-01-29 5:47 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: xieyongji, linux-kernel, virtualization, eperezma, hch
On Fri, Jan 27, 2023 at 6:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Dec 26, 2022 at 12:12:42PM +0800, Jason Wang wrote:
> > > >@@ -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
>
> OK I picked as is. Do we want WARN_ON maybe?
This could be triggered by the userspace, so I'm not sure it's worth it.
Thanks
>
> --
> MST
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] vdpa_sim: get rid of DMA ops
@ 2023-01-29 5:47 ` Jason Wang
0 siblings, 0 replies; 12+ messages in thread
From: Jason Wang @ 2023-01-29 5:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefano Garzarella, eperezma, virtualization, linux-kernel,
xieyongji, hch
On Fri, Jan 27, 2023 at 6:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Dec 26, 2022 at 12:12:42PM +0800, Jason Wang wrote:
> > > >@@ -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
>
> OK I picked as is. Do we want WARN_ON maybe?
This could be triggered by the userspace, so I'm not sure it's worth it.
Thanks
>
> --
> MST
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-01-29 5:48 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-23 6:00 [PATCH] vdpa_sim: get rid of DMA ops Jason Wang
2022-12-23 6:00 ` Jason Wang
2022-12-23 6:15 ` Christoph Hellwig
2022-12-23 6:15 ` Christoph Hellwig
2022-12-23 9:26 ` Stefano Garzarella
2022-12-23 9:26 ` Stefano Garzarella
2022-12-26 4:12 ` Jason Wang
2022-12-26 4:12 ` Jason Wang
2023-01-27 10:29 ` Michael S. Tsirkin
2023-01-27 10:29 ` Michael S. Tsirkin
2023-01-29 5:47 ` Jason Wang
2023-01-29 5:47 ` Jason Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.