Re: [RFC 01/10] iommu: Add IOMMU device registry
On Sun, Jul 06, 2014 at 08:17:22PM +0200, Arnd Bergmann wrote:
On Friday 04 July 2014, Will Deacon wrote:
On Fri, Jul 04, 2014 at 02:47:10PM +0100, Thierry Reding wrote:
On Fri, Jul 04, 2014 at 01:05:30PM +0200, Joerg Roedel wrote:
On Thu, Jun 26, 2014 at 10:49:41PM +0200, Thierry Reding wrote:
Add an IOMMU device registry for drivers to register with and
implement
a method for users of the IOMMU API to attach to an IOMMU device. This
allows to support deferred probing and gives the IOMMU API a
convenient
hook to perform early initialization of a device if necessary.
Can you elaborate on why exactly you need this? The IOMMU-API is
designed to hide any details from the user about the available IOMMUs in
the system and which IOMMU handles which device. This looks like it is
going in a completly different direction from that.
I need this primarily to properly serialize device probing order.
Without it the IOMMU may be probed later than its clients, in which case
the client drivers will assume that there is no IOMMU (iommu_present()
for the parent bus fails).
I can also vouch for needing a solution to this problem. The ARM SMMU (and
I think others) rely on initcall ordering rather than the driver probing
model to ensure the IOMMU is probed before any of its masters.
I think it would be best to attach platform devices to IOMMUs from the
of_dma_configure() we just introduced. That still requires handling
IOMMUs special though, and I don't know how we should best deal
with that. It would not be too hard to scan for IOMMUs in DT first
and register them all in a way that we can later look them up
by phandle, but that would break down if we ever get nested IOMMUs.
But even for nested IOMMUs each will have an associated device node, so
we could scan the tree up front. But given that it only solves the
problem partially I don't think that's a big advantage.
Another possibility might be to register all devices as we do today,
including IOMMU devices, but return -EPROBE_DEFER from
platform_drv_probe() before we call into the driver's probe function
if the IOMMU has not been set up at that point.
Right, Hiroshi already proposed a patch for that, but it was more or
less NAK'ed because people didn't want to have that functionality in the
device driver core.
For PCI devices, we need a different way of dealing with the IOMMUs,
some generic PCI code needs to be added to attach the correct IOMMU
to a newly added PCI device based on how the host bridge is configured.
I'm curious. Without device tree, how do we find out what IOMMU a device
is connected to? Will it always be an ancestor of the device in the PCI
hierarchy?
We can probably for now get away with not worrying about any bus type
other than platform, amba or PCI: we don't use any other DMA master
capable bus on ARM, and other architectures can probably rely on
having only a single IOMMU implementation in the system.
Neither of the above proposals will work for cases where more than a
single IOMMU exists in the system. Currently we can only register one
IOMMU per bus and if we try to register a second IOMMU it will fail
(bus_set_iommu() returns -EBUSY).
Also, struct bus_type has only a pointer to a struct iommu_ops, but no
associated context. Hence my proposal, which I only posted partially
here since it didn't seem immediately relevant. But I guess to better
illustrate how I envisioned this to work, here goes:
The idea was to allow each device to have zero or more master on zero or
more IOMMUs. That's as general a case as it gets. Now to make this work
we'd need something like this:
struct iommu_master {
struct device *dev; /* the master device */
struct iommu *iommu; /* the IOMMU that dev masters */
struct list_head list; /* link in a list of all master
interfaces of dev */
};
Then we could store a list in struct device:
struct device {
...
struct list_head iommu_masters;
...
};
It was already mentioned in other threads that if a device does indeed
have more than one master interface, then it needs to control access to
them explicitly via the IOMMU API. Since we only have an API to allocate
an IRQ domain (which automatically forwards calls to the global IOMMU)
we'd need something new, such as:
master = iommu_get(dev, foo);
or
master = iommu_get(dev, 0);
Or whichever variant we prefer. That could return a pointer to a struct
iommu_master, which could then be used to obtain a domain, like so:
domain = iommu_master_alloc_domain(master);
To make that work, as far as I can tell only very minimal changes would
have to be done to iommu_ops. Most of the functions take a pointer to a
struct iommu_domain anyway, we could extend it with a reference to the
parent
RE: [RFC 01/10] iommu: Add IOMMU device registry
-Original Message-
From: iommu-boun...@lists.linux-foundation.org [mailto:iommu-
boun...@lists.linux-foundation.org] On Behalf Of Thierry Reding
Sent: Friday, June 27, 2014 12:29 PM
To: Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell; Kumar Gala;
Stephen Warren; Arnd Bergmann; Will Deacon; Joerg Roedel
Cc: Olav Haugan; devicet...@vger.kernel.org; Grant Grundler; Rhyland
Klein; iommu@lists.linux-foundation.org; linux-ker...@vger.kernel.org;
Marc Zyngier; Allen Martin; Paul Walmsley; linux-te...@vger.kernel.org;
Cho KyongHo; Dave Martin; linux-arm-ker...@lists.infradead.org
Subject: Re: [RFC 01/10] iommu: Add IOMMU device registry
On Thu, Jun 26, 2014 at 10:49:41PM +0200, Thierry Reding wrote:
From: Thierry Reding tred...@nvidia.com
Add an IOMMU device registry for drivers to register with and
implement a method for users of the IOMMU API to attach to an IOMMU
device. This allows to support deferred probing and gives the IOMMU
API a convenient hook to perform early initialization of a device if
necessary.
Signed-off-by: Thierry Reding tred...@nvidia.com
---
drivers/iommu/iommu.c | 93
+++
include/linux/iommu.h | 27 +++
2 files changed, 120 insertions(+)
I thought that perhaps I should elaborate on this a bit since I have a
few ideas on how the API could be enhanced.
+static int of_iommu_attach(struct device *dev) {
+ struct of_phandle_iter iter;
+ struct iommu *iommu;
+
+ mutex_lock(iommus_lock);
+
+ of_property_for_each_phandle_with_args(iter, dev-of_node,
iommus,
+ #iommu-cells, 0) {
+ bool found = false;
+ int err;
+
+ /* skip disabled IOMMUs */
+ if (!of_device_is_available(iter.out_args.np))
+ continue;
+
+ list_for_each_entry(iommu, iommus, list) {
+ if (iommu-dev-of_node == iter.out_args.np) {
+ err = iommu-ops-attach(iommu, dev);
+ if (err 0) {
+ }
+
+ found = true;
+ }
+ }
+
+ if (!found) {
+ mutex_unlock(iommus_lock);
+ return -EPROBE_DEFER;
+ }
+ }
+
+ mutex_unlock(iommus_lock);
+
+ return 0;
+}
+
+static int of_iommu_detach(struct device *dev) {
+ /* TODO: implement */
+ return -ENOSYS;
+}
+
+int iommu_attach(struct device *dev)
+{
+ int err = 0;
+
+ if (IS_ENABLED(CONFIG_OF) dev-of_node) {
+ err = of_iommu_attach(dev);
+ if (!err)
+ return 0;
+ }
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(iommu_attach);
I think it might make sense to introduce an explicit object for an IOMMU
master attachment. Maybe something like:
struct iommu_master {
struct iommu *iommu;
struct device *dev;
...
};
iommu_attach() could then return a pointer to that attachment and the
IOMMU user driver could subsequently use that as a handle to access other
parts of the API.
The reason is that if we ever need to support more than a single master
interface (and perhaps even multiple master interfaces on different
IOMMUs) for a single device, then we need a way for the IOMMU user to
differentiate between its master interfaces.
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
284a4683fdc1..ac2ceef194d4 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -43,6 +43,17 @@ struct notifier_block; typedef int
(*iommu_fault_handler_t)(struct iommu_domain *,
struct device *, unsigned long, int, void *);
+struct iommu {
+ struct device *dev;
+
+ struct list_head list;
+
+ const struct iommu_ops *ops;
+};
For reasons explained above, I also think that it would be a good idea to
modify the iommu_ops functions to take a struct iommu * as their first
argument. This may become important when one driver needs to support
multiple IOMMU devices. With the current API drivers have to rely on
global variables to track the driver-specific context. As far as I can
tell, only .domain_init(), .add_device(), .remove_device() and
.device_group(). .domain_init() could set up a pointer to struct iommu in
struct iommu_domain so the functions dealing with domains could gain
access to the IOMMU device via that pointer.
Would the proposed interface be an alternate to the add_device interface? Also,
how would the iommu group creation work? We are dependent on device driver
initialization to attach device an IOMMU, but the add_device allows creation of
iommu_groups during bus probing.
Can't the same thing be achieved using the add device interface where an IOMMU
driver can determine
Re: [RFC 01/10] iommu: Add IOMMU device registry
On Thu, Jun 26, 2014 at 10:49:41PM +0200, Thierry Reding wrote:
From: Thierry Reding tred...@nvidia.com
Add an IOMMU device registry for drivers to register with and implement
a method for users of the IOMMU API to attach to an IOMMU device. This
allows to support deferred probing and gives the IOMMU API a convenient
hook to perform early initialization of a device if necessary.
Signed-off-by: Thierry Reding tred...@nvidia.com
---
drivers/iommu/iommu.c | 93
+++
include/linux/iommu.h | 27 +++
2 files changed, 120 insertions(+)
I thought that perhaps I should elaborate on this a bit since I have a
few ideas on how the API could be enhanced.
+static int of_iommu_attach(struct device *dev)
+{
+ struct of_phandle_iter iter;
+ struct iommu *iommu;
+
+ mutex_lock(iommus_lock);
+
+ of_property_for_each_phandle_with_args(iter, dev-of_node, iommus,
+#iommu-cells, 0) {
+ bool found = false;
+ int err;
+
+ /* skip disabled IOMMUs */
+ if (!of_device_is_available(iter.out_args.np))
+ continue;
+
+ list_for_each_entry(iommu, iommus, list) {
+ if (iommu-dev-of_node == iter.out_args.np) {
+ err = iommu-ops-attach(iommu, dev);
+ if (err 0) {
+ }
+
+ found = true;
+ }
+ }
+
+ if (!found) {
+ mutex_unlock(iommus_lock);
+ return -EPROBE_DEFER;
+ }
+ }
+
+ mutex_unlock(iommus_lock);
+
+ return 0;
+}
+
+static int of_iommu_detach(struct device *dev)
+{
+ /* TODO: implement */
+ return -ENOSYS;
+}
+
+int iommu_attach(struct device *dev)
+{
+ int err = 0;
+
+ if (IS_ENABLED(CONFIG_OF) dev-of_node) {
+ err = of_iommu_attach(dev);
+ if (!err)
+ return 0;
+ }
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(iommu_attach);
I think it might make sense to introduce an explicit object for an IOMMU
master attachment. Maybe something like:
struct iommu_master {
struct iommu *iommu;
struct device *dev;
...
};
iommu_attach() could then return a pointer to that attachment and the
IOMMU user driver could subsequently use that as a handle to access
other parts of the API.
The reason is that if we ever need to support more than a single master
interface (and perhaps even multiple master interfaces on different
IOMMUs) for a single device, then we need a way for the IOMMU user to
differentiate between its master interfaces.
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 284a4683fdc1..ac2ceef194d4 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -43,6 +43,17 @@ struct notifier_block;
typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
struct device *, unsigned long, int, void *);
+struct iommu {
+ struct device *dev;
+
+ struct list_head list;
+
+ const struct iommu_ops *ops;
+};
For reasons explained above, I also think that it would be a good idea
to modify the iommu_ops functions to take a struct iommu * as their
first argument. This may become important when one driver needs to
support multiple IOMMU devices. With the current API drivers have to
rely on global variables to track the driver-specific context. As far as
I can tell, only .domain_init(), .add_device(), .remove_device() and
.device_group(). .domain_init() could set up a pointer to struct iommu
in struct iommu_domain so the functions dealing with domains could gain
access to the IOMMU device via that pointer.
Thierry
pgpNj9lNw1jL4.pgp
Description: PGP signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[RFC 01/10] iommu: Add IOMMU device registry
From: Thierry Reding tred...@nvidia.com
Add an IOMMU device registry for drivers to register with and implement
a method for users of the IOMMU API to attach to an IOMMU device. This
allows to support deferred probing and gives the IOMMU API a convenient
hook to perform early initialization of a device if necessary.
Signed-off-by: Thierry Reding tred...@nvidia.com
---
drivers/iommu/iommu.c | 93 +++
include/linux/iommu.h | 27 +++
2 files changed, 120 insertions(+)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 806b55d056b7..5e9e82c73bbf 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -29,8 +29,12 @@
#include linux/idr.h
#include linux/notifier.h
#include linux/err.h
+#include linux/of.h
#include trace/events/iommu.h
+static DEFINE_MUTEX(iommus_lock);
+static LIST_HEAD(iommus);
+
static struct kset *iommu_group_kset;
static struct ida iommu_group_ida;
static struct mutex iommu_group_mutex;
@@ -1004,3 +1008,92 @@ int iommu_domain_set_attr(struct iommu_domain *domain,
return ret;
}
EXPORT_SYMBOL_GPL(iommu_domain_set_attr);
+
+int iommu_add(struct iommu *iommu)
+{
+ mutex_lock(iommus_lock);
+ list_add_tail(iommu-list, iommus);
+ mutex_unlock(iommus_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(iommu_add);
+
+void iommu_remove(struct iommu *iommu)
+{
+ mutex_lock(iommus_lock);
+ list_del_init(iommu-list);
+ mutex_unlock(iommus_lock);
+}
+EXPORT_SYMBOL_GPL(iommu_remove);
+
+static int of_iommu_attach(struct device *dev)
+{
+ struct of_phandle_iter iter;
+ struct iommu *iommu;
+
+ mutex_lock(iommus_lock);
+
+ of_property_for_each_phandle_with_args(iter, dev-of_node, iommus,
+ #iommu-cells, 0) {
+ bool found = false;
+ int err;
+
+ /* skip disabled IOMMUs */
+ if (!of_device_is_available(iter.out_args.np))
+ continue;
+
+ list_for_each_entry(iommu, iommus, list) {
+ if (iommu-dev-of_node == iter.out_args.np) {
+ err = iommu-ops-attach(iommu, dev);
+ if (err 0) {
+ }
+
+ found = true;
+ }
+ }
+
+ if (!found) {
+ mutex_unlock(iommus_lock);
+ return -EPROBE_DEFER;
+ }
+ }
+
+ mutex_unlock(iommus_lock);
+
+ return 0;
+}
+
+static int of_iommu_detach(struct device *dev)
+{
+ /* TODO: implement */
+ return -ENOSYS;
+}
+
+int iommu_attach(struct device *dev)
+{
+ int err = 0;
+
+ if (IS_ENABLED(CONFIG_OF) dev-of_node) {
+ err = of_iommu_attach(dev);
+ if (!err)
+ return 0;
+ }
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(iommu_attach);
+
+int iommu_detach(struct device *dev)
+{
+ int err = 0;
+
+ if (IS_ENABLED(CONFIG_OF) dev-of_node) {
+ err = of_iommu_detach(dev);
+ if (!err)
+ return 0;
+ }
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(iommu_detach);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 284a4683fdc1..ac2ceef194d4 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -43,6 +43,17 @@ struct notifier_block;
typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
struct device *, unsigned long, int, void *);
+struct iommu {
+ struct device *dev;
+
+ struct list_head list;
+
+ const struct iommu_ops *ops;
+};
+
+int iommu_add(struct iommu *iommu);
+void iommu_remove(struct iommu *iommu);
+
struct iommu_domain_geometry {
dma_addr_t aperture_start; /* First address that can be mapped*/
dma_addr_t aperture_end; /* Last address that can be mapped */
@@ -130,6 +141,9 @@ struct iommu_ops {
/* Get the numer of window per domain */
u32 (*domain_get_windows)(struct iommu_domain *domain);
+ int (*attach)(struct iommu *iommu, struct device *dev);
+ int (*detach)(struct iommu *iommu, struct device *dev);
+
unsigned long pgsize_bitmap;
};
@@ -192,6 +206,10 @@ extern int iommu_domain_window_enable(struct iommu_domain
*domain, u32 wnd_nr,
phys_addr_t offset, u64 size,
int prot);
extern void iommu_domain_window_disable(struct iommu_domain *domain, u32
wnd_nr);
+
+int iommu_attach(struct device *dev);
+int iommu_detach(struct device *dev);
+
/**
* report_iommu_fault() - report about an IOMMU fault to the IOMMU framework
* @domain: the iommu domain where the fault has happened
@@ -396,6 +414,15 @@ static inline int iommu_domain_set_attr(struct
iommu_domain *domain,
return
