Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1714013 > unrolled thread

[PATCH 00/13] Introduce IOMMU-API TLB Flushing Interface

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/13] Introduce IOMMU-API TLB Flushing Interface Joerg Roedel <joro@8bytes.org> - 2017-08-17 15:10 +0200
    [PATCH 03/13] vfio/type1: Use sychronized interface of the IOMMU-API Joerg Roedel <joro@8bytes.org> - 2017-08-17 15:10 +0200
    Re: [PATCH 00/13] Introduce IOMMU-API TLB Flushing Interface Alex Williamson <alex.williamson@redhat.com> - 2017-08-17 16:40 +0200
      Re: [PATCH 00/13] Introduce IOMMU-API TLB Flushing Interface Joerg Roedel <joro@8bytes.org> - 2017-08-17 16:50 +0200
        Re: [PATCH 00/13] Introduce IOMMU-API TLB Flushing Interface Alex Williamson <alex.williamson@redhat.com> - 2017-08-17 17:00 +0200
          Re: [PATCH 00/13] Introduce IOMMU-API TLB Flushing Interface Joerg Roedel <joro@8bytes.org> - 2017-08-17 17:30 +0200
            Re: [PATCH 00/13] Introduce IOMMU-API TLB Flushing Interface Joerg Roedel <joro@8bytes.org> - 2017-08-23 14:10 +0200

#1714013 — [PATCH 00/13] Introduce IOMMU-API TLB Flushing Interface

Hi,

here is a patch-set to introduce an explicit interface to
the IOMMU-API to flush IOMMU and device IO/TLBs. Currently
the iommu_map(), iommu_map_sg(), and iommu_unmap() functions
have to make sure all IO/TLBs in the system are synchronized
with the page-table updates they made.

This is very inefficient in some scenarios, for example when
a large address space is unmapped and an IO/TLB flush has to
be done in every call of iommu_unmap(). Or in a scenario
where it makes sense to queue up some changes to the
page-tables and flush them together.

To optimize these scenarios, the need to synchronize with
the IOMMU and device TLBs has been removed from the
map/unmap functions of the IOMMU-API and an interface to
explicitly do the flushes has been introduced.

To make the conversion of existing users of the IOMMU-API
easier, new functions - iommu_map_sync(), iommu_map_sg_sync(),
and iommu_unmap_sync() - have been introduced. These
functions guarantee that the IO/TLBs are synchronized with
any page-table update when they return. The optimizations
possible with the new interface are subject to separate
patch-sets.

Patch 1 just renames a few functions in the AMD-Vi driver
that would otherwise collide with the new TLB-flush
functions from the IOMMU-API.

Patch 2 introduces the new IO/TLB Flush-Interface.

Patch 3-13 convert existing users of the IOMMU-API to use
the *_sync functions for now.

Please review.

Thanks,

	Joerg

Joerg Roedel (13):
  iommu/amd: Rename a few flush functions
  iommu: Introduce Interface for IOMMU TLB Flushing
  vfio/type1: Use sychronized interface of the IOMMU-API
  iommu/dma: Use sychronized interface of the IOMMU-API
  arm: dma-mapping: Use sychronized interface of the IOMMU-API
  drm/etnaviv: Use sychronized interface of the IOMMU-API
  drm/msm: Use sychronized interface of the IOMMU-API
  drm/nouveau/imem/gk20a: Use sychronized interface of the IOMMU-API
  drm/rockchip: Use sychronized interface of the IOMMU-API
  drm/tegra: Use sychronized interface of the IOMMU-API
  gpu: host1x: Use sychronized interface of the IOMMU-API
  IB/usnic: Use sychronized interface of the IOMMU-API
  remoteproc: Use sychronized interface of the IOMMU-API

 arch/arm/mm/dma-mapping.c                          | 21 +++---
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c              | 10 +--
 drivers/gpu/drm/msm/msm_iommu.c                    |  5 +-
 .../gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c    | 12 ++--
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c        |  6 +-
 drivers/gpu/drm/tegra/drm.c                        |  6 +-
 drivers/gpu/drm/tegra/gem.c                        |  6 +-
 drivers/gpu/host1x/cdma.c                          |  6 +-
 drivers/gpu/host1x/job.c                           |  6 +-
 drivers/infiniband/hw/usnic/usnic_uiom.c           | 10 +--
 drivers/iommu/amd_iommu.c                          | 16 ++---
 drivers/iommu/dma-iommu.c                          |  8 +--
 drivers/iommu/iommu.c                              | 26 +++++++
 drivers/remoteproc/remoteproc_core.c               | 10 +--
 drivers/vfio/vfio_iommu_type1.c                    | 38 +++++-----
 include/linux/iommu.h                              | 80 +++++++++++++++++++++-
 16 files changed, 189 insertions(+), 77 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1714014 — [PATCH 03/13] vfio/type1: Use sychronized interface of the IOMMU-API

From: Joerg Roedel <jroedel@suse.de>

The map and unmap functions of the IOMMU-API changed their
semantics: They do no longer guarantee that the hardware
TLBs are synchronized with the page-table updates they made.

To make conversion easier, new synchronized functions have
been introduced which give these guarantees again until the
code is converted to use the new TLB-flush interface of the
IOMMU-API, which allows certain optimizations.

But for now, just convert this code to use the synchronized
functions so that it will behave as before.

Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: kvm@vger.kernel.org
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/vfio/vfio_iommu_type1.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 8549cb1..4ad83d4 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -672,7 +672,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
 				      struct vfio_domain, next);
 
 	list_for_each_entry_continue(d, &iommu->domain_list, next) {
-		iommu_unmap(d->domain, dma->iova, dma->size);
+		iommu_unmap_sync(d->domain, dma->iova, dma->size);
 		cond_resched();
 	}
 
@@ -687,9 +687,9 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
 		}
 
 		/*
-		 * To optimize for fewer iommu_unmap() calls, each of which
-		 * may require hardware cache flushing, try to find the
-		 * largest contiguous physical memory chunk to unmap.
+		 * To optimize for fewer iommu_unmap_sync() calls, each of which
+		 * may require hardware cache flushing, try to find the largest
+		 * contiguous physical memory chunk to unmap.
 		 */
 		for (len = PAGE_SIZE;
 		     !domain->fgsp && iova + len < end; len += PAGE_SIZE) {
@@ -698,7 +698,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
 				break;
 		}
 
-		unmapped = iommu_unmap(domain->domain, iova, len);
+		unmapped = iommu_unmap_sync(domain->domain, iova, len);
 		if (WARN_ON(!unmapped))
 			break;
 
@@ -877,15 +877,15 @@ static int map_try_harder(struct vfio_domain *domain, dma_addr_t iova,
 	int ret = 0;
 
 	for (i = 0; i < npage; i++, pfn++, iova += PAGE_SIZE) {
-		ret = iommu_map(domain->domain, iova,
-				(phys_addr_t)pfn << PAGE_SHIFT,
-				PAGE_SIZE, prot | domain->prot);
+		ret = iommu_map_sync(domain->domain, iova,
+				     (phys_addr_t)pfn << PAGE_SHIFT,
+				     PAGE_SIZE, prot | domain->prot);
 		if (ret)
 			break;
 	}
 
 	for (; i < npage && i > 0; i--, iova -= PAGE_SIZE)
-		iommu_unmap(domain->domain, iova, PAGE_SIZE);
+		iommu_unmap_sync(domain->domain, iova, PAGE_SIZE);
 
 	return ret;
 }
@@ -897,8 +897,9 @@ static int vfio_iommu_map(struct vfio_iommu *iommu, dma_addr_t iova,
 	int ret;
 
 	list_for_each_entry(d, &iommu->domain_list, next) {
-		ret = iommu_map(d->domain, iova, (phys_addr_t)pfn << PAGE_SHIFT,
-				npage << PAGE_SHIFT, prot | d->prot);
+		ret = iommu_map_sync(d->domain, iova,
+				     (phys_addr_t)pfn << PAGE_SHIFT,
+				     npage << PAGE_SHIFT, prot | d->prot);
 		if (ret) {
 			if (ret != -EBUSY ||
 			    map_try_harder(d, iova, pfn, npage, prot))
@@ -912,7 +913,7 @@ static int vfio_iommu_map(struct vfio_iommu *iommu, dma_addr_t iova,
 
 unwind:
 	list_for_each_entry_continue_reverse(d, &iommu->domain_list, next)
-		iommu_unmap(d->domain, iova, npage << PAGE_SHIFT);
+		iommu_unmap_sync(d->domain, iova, npage << PAGE_SHIFT);
 
 	return ret;
 }
@@ -1102,8 +1103,8 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
 				size = npage << PAGE_SHIFT;
 			}
 
-			ret = iommu_map(domain->domain, iova, phys,
-					size, dma->prot | domain->prot);
+			ret = iommu_map_sync(domain->domain, iova, phys,
+					     size, dma->prot | domain->prot);
 			if (ret)
 				return ret;
 
@@ -1133,13 +1134,14 @@ static void vfio_test_domain_fgsp(struct vfio_domain *domain)
 	if (!pages)
 		return;
 
-	ret = iommu_map(domain->domain, 0, page_to_phys(pages), PAGE_SIZE * 2,
-			IOMMU_READ | IOMMU_WRITE | domain->prot);
+	ret = iommu_map_sync(domain->domain, 0, page_to_phys(pages),
+			     PAGE_SIZE * 2,
+			     IOMMU_READ | IOMMU_WRITE | domain->prot);
 	if (!ret) {
-		size_t unmapped = iommu_unmap(domain->domain, 0, PAGE_SIZE);
+		size_t unmapped = iommu_unmap_sync(domain->domain, 0, PAGE_SIZE);
 
 		if (unmapped == PAGE_SIZE)
-			iommu_unmap(domain->domain, PAGE_SIZE, PAGE_SIZE);
+			iommu_unmap_sync(domain->domain, PAGE_SIZE, PAGE_SIZE);
 		else
 			domain->fgsp = true;
 	}
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1714087

On Thu, 17 Aug 2017 14:56:23 +0200
Joerg Roedel <joro@8bytes.org> wrote:

> Hi,
> 
> here is a patch-set to introduce an explicit interface to
> the IOMMU-API to flush IOMMU and device IO/TLBs. Currently
> the iommu_map(), iommu_map_sg(), and iommu_unmap() functions
> have to make sure all IO/TLBs in the system are synchronized
> with the page-table updates they made.
> 
> This is very inefficient in some scenarios, for example when
> a large address space is unmapped and an IO/TLB flush has to
> be done in every call of iommu_unmap(). Or in a scenario
> where it makes sense to queue up some changes to the
> page-tables and flush them together.
> 
> To optimize these scenarios, the need to synchronize with
> the IOMMU and device TLBs has been removed from the
> map/unmap functions of the IOMMU-API and an interface to
> explicitly do the flushes has been introduced.
> 
> To make the conversion of existing users of the IOMMU-API
> easier, new functions - iommu_map_sync(), iommu_map_sg_sync(),
> and iommu_unmap_sync() - have been introduced. These
> functions guarantee that the IO/TLBs are synchronized with
> any page-table update when they return. The optimizations
> possible with the new interface are subject to separate
> patch-sets.

Hi Joerg,

Wouldn't it be much more friendly to downstreams and out-of-tree
drivers to introduce new functions for the async semantics?  ie.
iommu_map_async(), etc.  The API also seems a little cleaner that
iommu_map() stands alone, it's synchronous, iommu_map_async() is
explicitly asynchronous and a _flush() call is needed to finalize it.
What do you see as the advantage to the approach here?  Thanks,

Alex


> Patch 1 just renames a few functions in the AMD-Vi driver
> that would otherwise collide with the new TLB-flush
> functions from the IOMMU-API.
> 
> Patch 2 introduces the new IO/TLB Flush-Interface.
> 
> Patch 3-13 convert existing users of the IOMMU-API to use
> the *_sync functions for now.
> 
> Please review.
> 
> Thanks,
> 
> 	Joerg
> 
> Joerg Roedel (13):
>   iommu/amd: Rename a few flush functions
>   iommu: Introduce Interface for IOMMU TLB Flushing
>   vfio/type1: Use sychronized interface of the IOMMU-API
>   iommu/dma: Use sychronized interface of the IOMMU-API
>   arm: dma-mapping: Use sychronized interface of the IOMMU-API
>   drm/etnaviv: Use sychronized interface of the IOMMU-API
>   drm/msm: Use sychronized interface of the IOMMU-API
>   drm/nouveau/imem/gk20a: Use sychronized interface of the IOMMU-API
>   drm/rockchip: Use sychronized interface of the IOMMU-API
>   drm/tegra: Use sychronized interface of the IOMMU-API
>   gpu: host1x: Use sychronized interface of the IOMMU-API
>   IB/usnic: Use sychronized interface of the IOMMU-API
>   remoteproc: Use sychronized interface of the IOMMU-API
> 
>  arch/arm/mm/dma-mapping.c                          | 21 +++---
>  drivers/gpu/drm/etnaviv/etnaviv_mmu.c              | 10 +--
>  drivers/gpu/drm/msm/msm_iommu.c                    |  5 +-
>  .../gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c    | 12 ++--
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c        |  6 +-
>  drivers/gpu/drm/tegra/drm.c                        |  6 +-
>  drivers/gpu/drm/tegra/gem.c                        |  6 +-
>  drivers/gpu/host1x/cdma.c                          |  6 +-
>  drivers/gpu/host1x/job.c                           |  6 +-
>  drivers/infiniband/hw/usnic/usnic_uiom.c           | 10 +--
>  drivers/iommu/amd_iommu.c                          | 16 ++---
>  drivers/iommu/dma-iommu.c                          |  8 +--
>  drivers/iommu/iommu.c                              | 26 +++++++
>  drivers/remoteproc/remoteproc_core.c               | 10 +--
>  drivers/vfio/vfio_iommu_type1.c                    | 38 +++++-----
>  include/linux/iommu.h                              | 80 +++++++++++++++++++++-
>  16 files changed, 189 insertions(+), 77 deletions(-)
> 

[toc] | [prev] | [next] | [standalone]

#1714098

Hi Alex,

On Thu, Aug 17, 2017 at 08:35:20AM -0600, Alex Williamson wrote:
> Wouldn't it be much more friendly to downstreams and out-of-tree
> drivers to introduce new functions for the async semantics?  ie.
> iommu_map_async(), etc.  The API also seems a little cleaner that
> iommu_map() stands alone, it's synchronous, iommu_map_async() is
> explicitly asynchronous and a _flush() call is needed to finalize it.
> What do you see as the advantage to the approach here?  Thanks,

The reason I did it this way was that I want the iommu_map(),
iommu_unmap(), and iomu_map_sg() functions be considered the _default_
to chose when using the IOMMU-API, because their use is faster than
using the _sync() variants. Or in other words, I want the _sync function
names to imply that they are slower versions of the default ones.


Regards,

	Joerg

[toc] | [prev] | [next] | [standalone]

#1714104

On Thu, 17 Aug 2017 16:43:08 +0200
Joerg Roedel <joro@8bytes.org> wrote:

> Hi Alex,
> 
> On Thu, Aug 17, 2017 at 08:35:20AM -0600, Alex Williamson wrote:
> > Wouldn't it be much more friendly to downstreams and out-of-tree
> > drivers to introduce new functions for the async semantics?  ie.
> > iommu_map_async(), etc.  The API also seems a little cleaner that
> > iommu_map() stands alone, it's synchronous, iommu_map_async() is
> > explicitly asynchronous and a _flush() call is needed to finalize it.
> > What do you see as the advantage to the approach here?  Thanks,  
> 
> The reason I did it this way was that I want the iommu_map(),
> iommu_unmap(), and iomu_map_sg() functions be considered the _default_
> to chose when using the IOMMU-API, because their use is faster than
> using the _sync() variants. Or in other words, I want the _sync function
> names to imply that they are slower versions of the default ones.

So _sync() does imply that they're slower, but iommu_map() does not
imply that a _flush() is required.  One is a performance issue, the
other is an API usability issue.  If the sync version is used
sub-optimally, it's a performance issue, not a correctness issue.  If
the async version is used without an explicit flush, it's a correctness
issue.  Therefore, I would lean towards making the asynchronous mode
explicit and providing good documentation and comments to steer
developers to the async version.  I think it makes the API harder to
use incorrectly.  Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

#1714135

On Thu, Aug 17, 2017 at 08:54:07AM -0600, Alex Williamson wrote:
> So _sync() does imply that they're slower, but iommu_map() does not
> imply that a _flush() is required.  One is a performance issue, the
> other is an API usability issue.  If the sync version is used
> sub-optimally, it's a performance issue, not a correctness issue.  If
> the async version is used without an explicit flush, it's a correctness
> issue.  Therefore, I would lean towards making the asynchronous mode
> explicit and providing good documentation and comments to steer
> developers to the async version.  I think it makes the API harder to
> use incorrectly.

I agree that it makes the API a bit more complicated to use. But that
can be solved by documenting it and by converting the main users (for me
that is VFIO and dma-iommu.c) of it to the unsynchronized interface,
because people will look at existing users and just do what they do.

Introducing _sync functions instead of _async ones also has the
side-effect that it puts pressure on the maintainers of the code to
convert it to the async interface, because they now see explicitly that
they use the slow version and start looking for ways to make things
faster.

What I absolutly don't want is that the whole explicit TLB flushing
of the IOMMU-API (as introduced in this patch-set) is considered some
optional part of the API, as it would be when I just introduce _async
versions of map/unmap/map_sg.


Regards,

	Joerg

[toc] | [prev] | [next] | [standalone]

#1718284

On Thu, Aug 17, 2017 at 05:22:20PM +0200, Joerg Roedel wrote:
> What I absolutly don't want is that the whole explicit TLB flushing
> of the IOMMU-API (as introduced in this patch-set) is considered some
> optional part of the API, as it would be when I just introduce _async
> versions of map/unmap/map_sg.

Okay, forget that :)

The discussions I had around this interface made me change it a little
bit in the version 2 of the patch-set which I will post soon.

I thought a bit more about the iommu_map() code-path. It really doesn't
make any sense to remove the tlb-sync requirement from it, because in
almost all cases the hardware doesn't require any flushes after a map
operation anyway. And in the rare cases where it does - because the
hardware is emulated and slow - the iommu-driver can handle
that by doing a flush in its iommu_ops->map() call-back.

So I removed the iommu_map_sync() and iommu_map_sg_sync() functions from
this series. With those changes it also doesn't make sense anymore
to have different tlb-sync semantics between iommu_map() and
iommu_unmap(). So I ended up introducing a new iommu_unmap_fast()
function which can unmap ranges and return with dirty io-tlbs.

This makes the extension of couse look somewhat optional, which I tried
to avoid, but I hope the '_fast' part of the name is enough motivation for
iommu-api users to look into ways to use it in their code.


Regards,

	Joerg

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web