Re: [PATCH] vdpa_sim: get rid of DMA ops
On Fri, Jan 27, 2023 at 6:29 PM Michael S. Tsirkin 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
Re: [PATCH] vdpa_sim: get rid of DMA ops
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
Re: [PATCH] vdpa_sim: get rid of DMA ops
On Fri, Dec 23, 2022 at 5:27 PM Stefano Garzarella 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
> >---
> >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
> > #include
> > #include
> >-#include
> > #include
> >
> > #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)
> >>iommu_lock);
> > }
> >
> >- for (i = 0; i < vdpasim->dev_attr.nas; i++)
> >+ for (i = 0; i < vdpasim->dev_attr.nas; i++) {
> > vhost_iotlb_reset(>iommu[i]);
> >+ vhost_iotlb_add_range(>iommu[i], 0, ULONG_MAX,
> >+0, VHOST_MAP_RW);
> >+ vdpasim->iommu_pt[i] = true;
> >+ }
> >
> > vdpasim->running = true;
> > spin_unlock(>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(>iova, size >> iova_shift(>iova),
> >-ULONG_MAX - 1, true);
> >- if (!iova)
> >- return DMA_MAPPING_ERROR;
> >-
> >- dma_addr = iova_dma_addr(>iova, iova);
> >-
> >- spin_lock(>iommu_lock);
> >- ret = vhost_iotlb_add_range(>iommu[0], (u64)dma_addr,
> >- (u64)dma_addr + size - 1, (u64)paddr,
> >perm);
> >- spin_unlock(>iommu_lock);
> >-
> >- if (ret) {
> >- __free_iova(>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(>iommu_lock);
> >- vhost_iotlb_del_range(>iommu[0], (u64)dma_addr,
> >-(u64)dma_addr + size - 1);
> >- spin_unlock(>iommu_lock);
> >-
> >- free_iova(>iova, iova_pfn(>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)
>
Re: [PATCH] vdpa_sim: get rid of DMA ops
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
---
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
#include
#include
-#include
#include
#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)
>iommu_lock);
}
- for (i = 0; i < vdpasim->dev_attr.nas; i++)
+ for (i = 0; i < vdpasim->dev_attr.nas; i++) {
vhost_iotlb_reset(>iommu[i]);
+ vhost_iotlb_add_range(>iommu[i], 0, ULONG_MAX,
+ 0, VHOST_MAP_RW);
+ vdpasim->iommu_pt[i] = true;
+ }
vdpasim->running = true;
spin_unlock(>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(>iova, size >> iova_shift(>iova),
- ULONG_MAX - 1, true);
- if (!iova)
- return DMA_MAPPING_ERROR;
-
- dma_addr = iova_dma_addr(>iova, iova);
-
- spin_lock(>iommu_lock);
- ret = vhost_iotlb_add_range(>iommu[0], (u64)dma_addr,
- (u64)dma_addr + size - 1, (u64)paddr, perm);
- spin_unlock(>iommu_lock);
-
- if (ret) {
- __free_iova(>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(>iommu_lock);
- vhost_iotlb_del_range(>iommu[0], (u64)dma_addr,
- (u64)dma_addr + size - 1);
- spin_unlock(>iommu_lock);
-
- free_iova(>iova, iova_pfn(>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,
Re: [PATCH] vdpa_sim: get rid of DMA ops
Looks good from the DMA subsysten POV: Acked-by: Christoph Hellwig Thanks for doing the work! ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] vdpa_sim: get rid of DMA ops
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
---
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
#include
#include
-#include
#include
#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)
>iommu_lock);
}
- for (i = 0; i < vdpasim->dev_attr.nas; i++)
+ for (i = 0; i < vdpasim->dev_attr.nas; i++) {
vhost_iotlb_reset(>iommu[i]);
+ vhost_iotlb_add_range(>iommu[i], 0, ULONG_MAX,
+ 0, VHOST_MAP_RW);
+ vdpasim->iommu_pt[i] = true;
+ }
vdpasim->running = true;
spin_unlock(>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(>iova, size >> iova_shift(>iova),
- ULONG_MAX - 1, true);
- if (!iova)
- return DMA_MAPPING_ERROR;
-
- dma_addr = iova_dma_addr(>iova, iova);
-
- spin_lock(>iommu_lock);
- ret = vhost_iotlb_add_range(>iommu[0], (u64)dma_addr,
- (u64)dma_addr + size - 1, (u64)paddr, perm);
- spin_unlock(>iommu_lock);
-
- if (ret) {
- __free_iova(>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(>iommu_lock);
- vhost_iotlb_del_range(>iommu[0], (u64)dma_addr,
- (u64)dma_addr + size - 1);
- spin_unlock(>iommu_lock);
-
- free_iova(>iova, iova_pfn(>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) {
-
