Re: [PATCH V2 1/2] Add new flush_iotlb_range and handle freelists when using iommu_unmap_fast

2020-08-18 Thread Tom Murphy
On Tue, 18 Aug 2020 at 16:17, Robin Murphy  wrote:
>
> On 2020-08-18 07:04, Tom Murphy wrote:
> > Add a flush_iotlb_range to allow flushing of an iova range instead of a
> > full flush in the dma-iommu path.
> >
> > Allow the iommu_unmap_fast to return newly freed page table pages and
> > pass the freelist to queue_iova in the dma-iommu ops path.
> >
> > This patch is useful for iommu drivers (in this case the intel iommu
> > driver) which need to wait for the ioTLB to be flushed before newly
> > free/unmapped page table pages can be freed. This way we can still batch
> > ioTLB free operations and handle the freelists.
>
> It sounds like the freelist is something that logically belongs in the
> iommu_iotlb_gather structure. And even if it's not a perfect fit I'd be
> inclined to jam it in there anyway just to avoid this giant argument
> explosion ;)

Good point, I'll do that.

>
> Why exactly do we need to introduce a new flush_iotlb_range() op? Can't
> the AMD driver simply use the gather mechanism like everyone else?

No, there's no reason it can't simply use the gather mechanism. I will
use the gather mechanism.
I think I wrote this patch way back before the gather mechanism was
introduced and I've been rebasing/slightly updating it since then
without paying proper attention to the code.

>
> Robin.
>
> > Change-log:
> > V2:
> > -fix missing parameter in mtk_iommu_v1.c
> >
> > Signed-off-by: Tom Murphy 
> > ---
> >   drivers/iommu/amd/iommu.c   | 14 -
> >   drivers/iommu/arm-smmu-v3.c |  3 +-
> >   drivers/iommu/arm-smmu.c|  3 +-
> >   drivers/iommu/dma-iommu.c   | 45 ---
> >   drivers/iommu/exynos-iommu.c|  3 +-
> >   drivers/iommu/intel/iommu.c | 54 +
> >   drivers/iommu/iommu.c   | 25 +++
> >   drivers/iommu/ipmmu-vmsa.c  |  3 +-
> >   drivers/iommu/msm_iommu.c   |  3 +-
> >   drivers/iommu/mtk_iommu.c   |  3 +-
> >   drivers/iommu/mtk_iommu_v1.c|  3 +-
> >   drivers/iommu/omap-iommu.c  |  3 +-
> >   drivers/iommu/qcom_iommu.c  |  3 +-
> >   drivers/iommu/rockchip-iommu.c  |  3 +-
> >   drivers/iommu/s390-iommu.c  |  3 +-
> >   drivers/iommu/sun50i-iommu.c|  3 +-
> >   drivers/iommu/tegra-gart.c  |  3 +-
> >   drivers/iommu/tegra-smmu.c  |  3 +-
> >   drivers/iommu/virtio-iommu.c|  3 +-
> >   drivers/vfio/vfio_iommu_type1.c |  2 +-
> >   include/linux/iommu.h   | 21 +++--
> >   21 files changed, 150 insertions(+), 56 deletions(-)
> >
> > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> > index 2f22326ee4df..25fbacab23c3 100644
> > --- a/drivers/iommu/amd/iommu.c
> > +++ b/drivers/iommu/amd/iommu.c
> > @@ -2513,7 +2513,8 @@ static int amd_iommu_map(struct iommu_domain *dom, 
> > unsigned long iova,
> >
> >   static size_t amd_iommu_unmap(struct iommu_domain *dom, unsigned long 
> > iova,
> > size_t page_size,
> > -   struct iommu_iotlb_gather *gather)
> > +   struct iommu_iotlb_gather *gather,
> > +   struct page **freelist)
> >   {
> >   struct protection_domain *domain = to_pdomain(dom);
> >   struct domain_pgtable pgtable;
> > @@ -2636,6 +2637,16 @@ static void amd_iommu_flush_iotlb_all(struct 
> > iommu_domain *domain)
> >   spin_unlock_irqrestore(>lock, flags);
> >   }
> >
> > +static void amd_iommu_flush_iotlb_range(struct iommu_domain *domain,
> > + unsigned long iova, size_t size,
> > + struct page *freelist)
> > +{
> > + struct protection_domain *dom = to_pdomain(domain);
> > +
> > + domain_flush_pages(dom, iova, size);
> > + domain_flush_complete(dom);
> > +}
> > +
> >   static void amd_iommu_iotlb_sync(struct iommu_domain *domain,
> >struct iommu_iotlb_gather *gather)
> >   {
> > @@ -2675,6 +2686,7 @@ const struct iommu_ops amd_iommu_ops = {
> >   .is_attach_deferred = amd_iommu_is_attach_deferred,
> >   .pgsize_bitmap  = AMD_IOMMU_PGSIZES,
> >   .flush_iotlb_all = amd_iommu_flush_iotlb_all,
> > + .flush_iotlb_range = amd_iommu_flush_iotlb_range,
> >   .iotlb_sync = amd_iommu_iotlb_sync,
> >   .def_domain_type = amd_iommu_def_domain_type,
> >   };
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index f578677a5c41..8d328dc25326 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -2854,7 +2854,8 @@ static int arm_smmu_map(struct iommu_domain *domain, 
> > unsigned long iova,
> >   }
> >
> >   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long 
> > iova,
> > -  size_t size, struct iommu_iotlb_gather *gather)
> > +  size_t size, struct iommu_iotlb_gather *gather,
> > +  struct page **freelist)
> >   

Re: [PATCH V2 1/2] Add new flush_iotlb_range and handle freelists when using iommu_unmap_fast

2020-08-18 Thread Robin Murphy

On 2020-08-18 07:04, Tom Murphy wrote:

Add a flush_iotlb_range to allow flushing of an iova range instead of a
full flush in the dma-iommu path.

Allow the iommu_unmap_fast to return newly freed page table pages and
pass the freelist to queue_iova in the dma-iommu ops path.

This patch is useful for iommu drivers (in this case the intel iommu
driver) which need to wait for the ioTLB to be flushed before newly
free/unmapped page table pages can be freed. This way we can still batch
ioTLB free operations and handle the freelists.


It sounds like the freelist is something that logically belongs in the 
iommu_iotlb_gather structure. And even if it's not a perfect fit I'd be 
inclined to jam it in there anyway just to avoid this giant argument 
explosion ;)


Why exactly do we need to introduce a new flush_iotlb_range() op? Can't 
the AMD driver simply use the gather mechanism like everyone else?


Robin.


Change-log:
V2:
-fix missing parameter in mtk_iommu_v1.c

Signed-off-by: Tom Murphy 
---
  drivers/iommu/amd/iommu.c   | 14 -
  drivers/iommu/arm-smmu-v3.c |  3 +-
  drivers/iommu/arm-smmu.c|  3 +-
  drivers/iommu/dma-iommu.c   | 45 ---
  drivers/iommu/exynos-iommu.c|  3 +-
  drivers/iommu/intel/iommu.c | 54 +
  drivers/iommu/iommu.c   | 25 +++
  drivers/iommu/ipmmu-vmsa.c  |  3 +-
  drivers/iommu/msm_iommu.c   |  3 +-
  drivers/iommu/mtk_iommu.c   |  3 +-
  drivers/iommu/mtk_iommu_v1.c|  3 +-
  drivers/iommu/omap-iommu.c  |  3 +-
  drivers/iommu/qcom_iommu.c  |  3 +-
  drivers/iommu/rockchip-iommu.c  |  3 +-
  drivers/iommu/s390-iommu.c  |  3 +-
  drivers/iommu/sun50i-iommu.c|  3 +-
  drivers/iommu/tegra-gart.c  |  3 +-
  drivers/iommu/tegra-smmu.c  |  3 +-
  drivers/iommu/virtio-iommu.c|  3 +-
  drivers/vfio/vfio_iommu_type1.c |  2 +-
  include/linux/iommu.h   | 21 +++--
  21 files changed, 150 insertions(+), 56 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 2f22326ee4df..25fbacab23c3 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2513,7 +2513,8 @@ static int amd_iommu_map(struct iommu_domain *dom, 
unsigned long iova,
  
  static size_t amd_iommu_unmap(struct iommu_domain *dom, unsigned long iova,

  size_t page_size,
- struct iommu_iotlb_gather *gather)
+ struct iommu_iotlb_gather *gather,
+ struct page **freelist)
  {
struct protection_domain *domain = to_pdomain(dom);
struct domain_pgtable pgtable;
@@ -2636,6 +2637,16 @@ static void amd_iommu_flush_iotlb_all(struct 
iommu_domain *domain)
spin_unlock_irqrestore(>lock, flags);
  }
  
+static void amd_iommu_flush_iotlb_range(struct iommu_domain *domain,

+   unsigned long iova, size_t size,
+   struct page *freelist)
+{
+   struct protection_domain *dom = to_pdomain(domain);
+
+   domain_flush_pages(dom, iova, size);
+   domain_flush_complete(dom);
+}
+
  static void amd_iommu_iotlb_sync(struct iommu_domain *domain,
 struct iommu_iotlb_gather *gather)
  {
@@ -2675,6 +2686,7 @@ const struct iommu_ops amd_iommu_ops = {
.is_attach_deferred = amd_iommu_is_attach_deferred,
.pgsize_bitmap  = AMD_IOMMU_PGSIZES,
.flush_iotlb_all = amd_iommu_flush_iotlb_all,
+   .flush_iotlb_range = amd_iommu_flush_iotlb_range,
.iotlb_sync = amd_iommu_iotlb_sync,
.def_domain_type = amd_iommu_def_domain_type,
  };
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index f578677a5c41..8d328dc25326 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2854,7 +2854,8 @@ static int arm_smmu_map(struct iommu_domain *domain, 
unsigned long iova,
  }
  
  static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,

-size_t size, struct iommu_iotlb_gather *gather)
+size_t size, struct iommu_iotlb_gather *gather,
+struct page **freelist)
  {
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 243bc4cb2705..0cd0dfc89875 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1234,7 +1234,8 @@ static int arm_smmu_map(struct iommu_domain *domain, 
unsigned long iova,
  }
  
  static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,

-size_t size, struct iommu_iotlb_gather *gather)
+size_t size, struct iommu_iotlb_gather *gather,
+struct page **freelist)

[PATCH V2 1/2] Add new flush_iotlb_range and handle freelists when using iommu_unmap_fast

2020-08-18 Thread Tom Murphy
Add a flush_iotlb_range to allow flushing of an iova range instead of a
full flush in the dma-iommu path.

Allow the iommu_unmap_fast to return newly freed page table pages and
pass the freelist to queue_iova in the dma-iommu ops path.

This patch is useful for iommu drivers (in this case the intel iommu
driver) which need to wait for the ioTLB to be flushed before newly
free/unmapped page table pages can be freed. This way we can still batch
ioTLB free operations and handle the freelists.

Change-log:
V2:
-fix missing parameter in mtk_iommu_v1.c

Signed-off-by: Tom Murphy 
---
 drivers/iommu/amd/iommu.c   | 14 -
 drivers/iommu/arm-smmu-v3.c |  3 +-
 drivers/iommu/arm-smmu.c|  3 +-
 drivers/iommu/dma-iommu.c   | 45 ---
 drivers/iommu/exynos-iommu.c|  3 +-
 drivers/iommu/intel/iommu.c | 54 +
 drivers/iommu/iommu.c   | 25 +++
 drivers/iommu/ipmmu-vmsa.c  |  3 +-
 drivers/iommu/msm_iommu.c   |  3 +-
 drivers/iommu/mtk_iommu.c   |  3 +-
 drivers/iommu/mtk_iommu_v1.c|  3 +-
 drivers/iommu/omap-iommu.c  |  3 +-
 drivers/iommu/qcom_iommu.c  |  3 +-
 drivers/iommu/rockchip-iommu.c  |  3 +-
 drivers/iommu/s390-iommu.c  |  3 +-
 drivers/iommu/sun50i-iommu.c|  3 +-
 drivers/iommu/tegra-gart.c  |  3 +-
 drivers/iommu/tegra-smmu.c  |  3 +-
 drivers/iommu/virtio-iommu.c|  3 +-
 drivers/vfio/vfio_iommu_type1.c |  2 +-
 include/linux/iommu.h   | 21 +++--
 21 files changed, 150 insertions(+), 56 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 2f22326ee4df..25fbacab23c3 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2513,7 +2513,8 @@ static int amd_iommu_map(struct iommu_domain *dom, 
unsigned long iova,
 
 static size_t amd_iommu_unmap(struct iommu_domain *dom, unsigned long iova,
  size_t page_size,
- struct iommu_iotlb_gather *gather)
+ struct iommu_iotlb_gather *gather,
+ struct page **freelist)
 {
struct protection_domain *domain = to_pdomain(dom);
struct domain_pgtable pgtable;
@@ -2636,6 +2637,16 @@ static void amd_iommu_flush_iotlb_all(struct 
iommu_domain *domain)
spin_unlock_irqrestore(>lock, flags);
 }
 
+static void amd_iommu_flush_iotlb_range(struct iommu_domain *domain,
+   unsigned long iova, size_t size,
+   struct page *freelist)
+{
+   struct protection_domain *dom = to_pdomain(domain);
+
+   domain_flush_pages(dom, iova, size);
+   domain_flush_complete(dom);
+}
+
 static void amd_iommu_iotlb_sync(struct iommu_domain *domain,
 struct iommu_iotlb_gather *gather)
 {
@@ -2675,6 +2686,7 @@ const struct iommu_ops amd_iommu_ops = {
.is_attach_deferred = amd_iommu_is_attach_deferred,
.pgsize_bitmap  = AMD_IOMMU_PGSIZES,
.flush_iotlb_all = amd_iommu_flush_iotlb_all,
+   .flush_iotlb_range = amd_iommu_flush_iotlb_range,
.iotlb_sync = amd_iommu_iotlb_sync,
.def_domain_type = amd_iommu_def_domain_type,
 };
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index f578677a5c41..8d328dc25326 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2854,7 +2854,8 @@ static int arm_smmu_map(struct iommu_domain *domain, 
unsigned long iova,
 }
 
 static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
-size_t size, struct iommu_iotlb_gather *gather)
+size_t size, struct iommu_iotlb_gather *gather,
+struct page **freelist)
 {
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 243bc4cb2705..0cd0dfc89875 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1234,7 +1234,8 @@ static int arm_smmu_map(struct iommu_domain *domain, 
unsigned long iova,
 }
 
 static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
-size_t size, struct iommu_iotlb_gather *gather)
+size_t size, struct iommu_iotlb_gather *gather,
+struct page **freelist)
 {
struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 4959f5df21bd..7433f74d921a 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -50,6 +50,19 @@ struct iommu_dma_cookie {
struct iommu_domain *fq_domain;
 };
 
+
+static void iommu_dma_entry_dtor(unsigned