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
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 <[email protected]>
---
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
Use the new iommu_commit() function in the KVM device
passthrough code.
Signed-off-by: Joerg Roedel <[email protected]>
---
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
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
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
* David Woodhouse ([email protected]) 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.
On Thu, Jun 23, 2011 at 01:20:53PM -0400, Chris Wright wrote:
> * David Woodhouse ([email protected]) 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
Hi.
On Fri, Jun 24, 2011 at 12:31 AM, Joerg Roedel <[email protected]> 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.
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 <[email protected]> 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