[PATCH 0/2] Introduce iommu_commit() function

[PATCH 0/2] Introduce iommu_commit() function

[Joerg Roedel]

Hi,

this patch-set introduces a new function into the iommu-api:

	iommu_commit(domain);

It needs to be called whenever a some code changed a domain (either by
attaching/detaching devices or by mapping/unmapping pages in
the domain).

IOMMU drivers can implement the new commit-callback to coalesce any
cache flushing on the CPU and in the IOMMU hardware which is necessary
after changes have been made to a domain.

David, I think especially VT-d can benefit from such a callback. I will
implement support for it in the AMD IOMMU driver and post a patch-set
soon.

Any comments, thoughts?

Thanks,

	Joerg

[PATCH 1/2] iommu-api: Introduce iommu_comit function

[Joerg Roedel]

This function will be used to coalesce cache-flushes
required after a domain was changed. This should
significantly improve performance in some cases.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/iommu/iommu.c |    7 +++++++
 include/linux/iommu.h |    6 ++++++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6e6b6a1..4099cc6 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -122,3 +122,10 @@ int iommu_unmap(struct iommu_domain *domain, unsigned long iova, int gfp_order)
 	return iommu_ops->unmap(domain, iova, gfp_order);
 }
 EXPORT_SYMBOL_GPL(iommu_unmap);
+
+void iommu_commit(struct iommu_domain *domain)
+{
+	if (iommu_ops->commit)
+		iommu_ops->commit(domain);
+}
+EXPORT_SYMBOL_GPL(iommu_commit);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9940319..dbb4324 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -47,6 +47,7 @@ struct iommu_ops {
 				    unsigned long iova);
 	int (*domain_has_cap)(struct iommu_domain *domain,
 			      unsigned long cap);
+	void (*commit)(struct iommu_domain *domain);
 };
 
 #ifdef CONFIG_IOMMU_API
@@ -67,6 +68,7 @@ extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain,
 				      unsigned long iova);
 extern int iommu_domain_has_cap(struct iommu_domain *domain,
 				unsigned long cap);
+extern void iommu_commit(struct iommu_domain *domain);
 
 #else /* CONFIG_IOMMU_API */
 
@@ -123,6 +125,10 @@ static inline int domain_has_cap(struct iommu_domain *domain,
 	return 0;
 }
 
+static inline void iommu_commit(struct iommu_domain *domain)
+{
+}
+
 #endif /* CONFIG_IOMMU_API */
 
 #endif /* __LINUX_IOMMU_H */
-- 
1.7.4.1

[PATCH 2/2] KVM: IOMMU: Use iommu_commit() in device-passthrough code

[Joerg Roedel]

Use the new iommu_commit() function in the KVM device
passthrough code.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 virt/kvm/iommu.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
index 62a9caf..87ef85a 100644
--- a/virt/kvm/iommu.c
+++ b/virt/kvm/iommu.c
@@ -187,6 +187,8 @@ int kvm_assign_device(struct kvm *kvm,
 		PCI_SLOT(assigned_dev->host_devfn),
 		PCI_FUNC(assigned_dev->host_devfn));
 
+	iommu_commit(domain);
+
 	return 0;
 out_unmap:
 	kvm_iommu_unmap_memslots(kvm);
@@ -215,6 +217,8 @@ int kvm_deassign_device(struct kvm *kvm,
 		PCI_SLOT(assigned_dev->host_devfn),
 		PCI_FUNC(assigned_dev->host_devfn));
 
+	iommu_commit(domain);
+
 	return 0;
 }
 
@@ -235,6 +239,8 @@ int kvm_iommu_map_guest(struct kvm *kvm)
 	if (r)
 		goto out_unmap;
 
+	iommu_commit(kvm->arch.iommu_domain);
+
 	return 0;
 
 out_unmap:
@@ -283,6 +289,8 @@ static void kvm_iommu_put_pages(struct kvm *kvm,
 
 		gfn += unmap_pages;
 	}
+
+	iommu_commit(domain);
 }
 
 static int kvm_iommu_unmap_memslots(struct kvm *kvm)
-- 
1.7.4.1

Re: [PATCH 0/2] Introduce iommu_commit() function

[David Woodhouse]

On Thu, 2011-06-23 at 17:31 +0200, Joerg Roedel wrote:
> David, I think especially VT-d can benefit from such a callback. I will
> implement support for it in the AMD IOMMU driver and post a patch-set
> soon.
> 
> Any comments, thoughts?

Ick. We *already* do the flushes as appropriate while we're filling the
page tables. So every time we move on from one page table page to the
next, we'll flush the old one. And when we've *done* filling the page
tables for the range we've been asked to map, we flush the last writes
too.

The problem with KVM is that it calls us over and over again to map a
single 4KiB page.

It doesn't seem simple to make use of a 'commit' function, because we'd
have to keep track of *which* page tables are dirty.

I'd much rather KVM just gave us a list of the pages to map, in a single
call. Or even a 'translation' callback we could call to get the physical
address for each page in the range.

-- 
dwmw2

Re: [PATCH 0/2] Introduce iommu_commit() function

[Roedel, Joerg]

On Thu, Jun 23, 2011 at 11:38:54AM -0400, David Woodhouse wrote:
> On Thu, 2011-06-23 at 17:31 +0200, Joerg Roedel wrote:
> > David, I think especially VT-d can benefit from such a callback. I will
> > implement support for it in the AMD IOMMU driver and post a patch-set
> > soon.
> > 
> > Any comments, thoughts?
> 
> Ick. We *already* do the flushes as appropriate while we're filling the
> page tables. So every time we move on from one page table page to the
> next, we'll flush the old one. And when we've *done* filling the page
> tables for the range we've been asked to map, we flush the last writes
> too.

It doesn't sound too complicated to make this work with iommu_commit.
All the VT-d driver needs to do is to keep track of the last page-table
page a map/unmap request was targeted to (per domain). Subsequent
map/unmap calls check if the same page is targeted and flushes the old
one if not.  The last writes are flushed in the iommu_commit() call
(together with the IOMMU cache flushes).

It is basically the same algorithm you use now except that it works
accross iommu_map/iommu_unmap calls.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 0/2] Introduce iommu_commit() function

[Chris Wright]

* David Woodhouse (dwmw2@infradead.org) wrote:
> I'd much rather KVM just gave us a list of the pages to map, in a single
> call.

This makes most sense to me.

Re: [PATCH 0/2] Introduce iommu_commit() function

[Roedel, Joerg]

On Thu, Jun 23, 2011 at 01:20:53PM -0400, Chris Wright wrote:
> * David Woodhouse (dwmw2@infradead.org) wrote:
> > I'd much rather KVM just gave us a list of the pages to map, in a single
> > call.
> 
> This makes most sense to me.

But how is this supposed to work with arbitrary page-sizes? When we
define the interface as

	int iommu_map(struct iommu_domain *domain,
		      unsigned long iova,
		      struct page **pages,
		      int num_pages);

how does the IOMMU driver know which page-sizes are the best to use?
Sure, it can check if the pages in the list are physically contiguous,
but that is really bad design.
The information about the right page-sizes is already available in the
caller and the best is to pass this information down to the iommu
driver (which then maps the region with the best-fitting page-sizes it
supports). I would rather change the interface to something like

	int iommu_map(struct iommu_domain *domain,
		      unsigned long iova,
		      phys_addr_t phys_addr,
		      size_t len);

In this interface the caller specifies that the system physical region
starting at phys_addr, ending at phys_addr+len-1 should be mapped at
iova. The IOMMU driver already knows that this region is physically
contiguous and can apply the right page-sizes itself.

This interface is also much friendlier to the caller because there is no
need to build a list of pages. And yes, there will be more users of the
iommu-api than KVM in the future.

The page-table page cache-flushing problem on VT-d can be solved with
the iommu_commit() function like I described in an previous email. This
one can be used for a number of things necessary on other iommus too
(like flushing io/tlbs) so that this interface makes sense for all iommu
hardware supported by the iommu-api.

Regards,

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 0/2] Introduce iommu_commit() function

[KyongHo Cho]

Hi.

On Fri, Jun 24, 2011 at 12:31 AM, Joerg Roedel <joerg.roedel@amd.com> wrote:
> It needs to be called whenever a some code changed a domain (either by
> attaching/detaching devices or by mapping/unmapping pages in
> the domain).

Do you mean we can invalidate IOTLB with this iommu_commit()?
We need to invalidate IOTLB without updating page table for some
optimized solutions.

If a device in one domain is moved to other domain, IOTLB of the
device must be invalidated
because it contains translation information of the previous domain.

Regards,
KyongHo Cho.

Re: [PATCH 0/2] Introduce iommu_commit() function

[Joerg Roedel]

On Wed, Jun 29, 2011 at 02:02:02PM +0900, KyongHo Cho wrote:
> Hi.
> 
> On Fri, Jun 24, 2011 at 12:31 AM, Joerg Roedel <joerg.roedel@amd.com> wrote:
> > It needs to be called whenever a some code changed a domain (either by
> > attaching/detaching devices or by mapping/unmapping pages in
> > the domain).
> 
> Do you mean we can invalidate IOTLB with this iommu_commit()?
> We need to invalidate IOTLB without updating page table for some
> optimized solutions.
> 
> If a device in one domain is moved to other domain, IOTLB of the
> device must be invalidated
> because it contains translation information of the previous domain.

The classic use-case is to do a single iommu-tlb flush after a set of
page-table updates, but flushing iotlbs after moving devices is a
potential use-case too, so the answer is yes.

Regards,

	Joerg

Re: [PATCH 0/2] Introduce iommu_commit() function

[KyongHo Cho]

On Wed, Jun 29, 2011 at 2:51 PM, Joerg Roedel <joro@8bytes.org> wrote:
> On Wed, Jun 29, 2011 at 02:02:02PM +0900, KyongHo Cho wrote:
>> Hi.
>>
>> On Fri, Jun 24, 2011 at 12:31 AM, Joerg Roedel <joerg.roedel@amd.com> wrote:
>> > It needs to be called whenever a some code changed a domain (either by
>> > attaching/detaching devices or by mapping/unmapping pages in
>> > the domain).
>>
>> Do you mean we can invalidate IOTLB with this iommu_commit()?
>> We need to invalidate IOTLB without updating page table for some
>> optimized solutions.
>>
>> If a device in one domain is moved to other domain, IOTLB of the
>> device must be invalidated
>> because it contains translation information of the previous domain.
>
> The classic use-case is to do a single iommu-tlb flush after a set of
> page-table updates, but flushing iotlbs after moving devices is a
> potential use-case too, so the answer is yes.
>

Hi.

In the 'next' branch of
http://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git,
I found that iommu_commit() is removed.

Why is it removed?

BTW, the comment of struct iommu_ops still contains description about commit.

Regards,

  KyongHo

Re: [PATCH 0/2] Introduce iommu_commit() function

[Avi Kivity]

On 06/23/2011 06:38 PM, David Woodhouse wrote:
> On Thu, 2011-06-23 at 17:31 +0200, Joerg Roedel wrote:
> > David, I think especially VT-d can benefit from such a callback. I will
> > implement support for it in the AMD IOMMU driver and post a patch-set
> > soon.
> > 
> > Any comments, thoughts?
>
> Ick. We *already* do the flushes as appropriate while we're filling the
> page tables. So every time we move on from one page table page to the
> next, we'll flush the old one. And when we've *done* filling the page
> tables for the range we've been asked to map, we flush the last writes
> too.

For the current kvm use case flushing just once on commit is most
efficient.  If/when we get resumable io faults, per-page flushing
becomes worthwhile.

> The problem with KVM is that it calls us over and over again to map a
> single 4KiB page.
>
> It doesn't seem simple to make use of a 'commit' function, because we'd
> have to keep track of *which* page tables are dirty.

You could easily do that by using a free bit in the pte as a dirty bit. 
You can then choose whether to use per-page flush or a full flush.

> I'd much rather KVM just gave us a list of the pages to map, in a single
> call. 

The list can easily be several million pages long.

> Or even a 'translation' callback we could call to get the physical
> address for each page in the range.

This is doable, and is probably most flexible.  If the translation also
returns ranges, then you don't have to figure out large mappings yourself.

Not that there's a huge difference between

   iommu_begin(iommu_transaction, domain)
   for (page in range)
         iommu_map(iommu_transaction, page, translate(page))
    iommu_commit(iommu_transaction)

and

   iommu_map(domain, range, translate)

- one can be converted to the other.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/2] Introduce iommu_commit() function

[Joerg Roedel]

On Wed, Nov 16, 2011 at 11:00:56AM +0900, KyongHo Cho wrote:
> On Wed, Jun 29, 2011 at 2:51 PM, Joerg Roedel <joro@8bytes.org> wrote:

> In the 'next' branch of
> http://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git,
> I found that iommu_commit() is removed.
> 
> Why is it removed?

It was never in the next-branch. It actually is in the master-branch,
but that happened accidentially :)
The reason is that there is not enough consensus about this interface
yet. This is also the reason I havn't pushed it upstream yet.


	Joerg