| Message ID | 1403815790-8548-2-git-send-email-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 26, 2014 at 10:49:41PM +0200, Thierry Reding wrote: > From: Thierry Reding <treding@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 <treding@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
> -----Original Message----- > From: iommu-bounces@lists.linux-foundation.org [mailto:iommu- > bounces@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; devicetree@vger.kernel.org; Grant Grundler; Rhyland > Klein; iommu@lists.linux-foundation.org; linux-kernel@vger.kernel.org; > Marc Zyngier; Allen Martin; Paul Walmsley; linux-tegra@vger.kernel.org; > Cho KyongHo; Dave Martin; linux-arm-kernel@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 <treding@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 <treding@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 (in add_device) if the device is attached to a particular IOMMU. If the device is attached to that IOMMU then it can create the corresponding IOMMU group. IOMMU information can be stored in archdata. -Varun
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. Joerg
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). There are other ways around this, but I think we'll need to eventually come up with something like this anyway. Consider for example what happens when a device has master interfaces on two different IOMMUs. Not only does the current model of having one and one only IOMMU per struct bus_type break down, but also IOMMU masters will need a way to specify which IOMMU they're talking to. Thierry
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. Will
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. 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. 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. 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. Arnd
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 of a domain. For that we'll need a structure that represents the IOMMU device's context (which is what this patch introduces as struct iommu). The only functions in struct iommu_ops that deal with an IOMMU directly are .add_device(), .remove_device() and .device_group(), although they may become obsolete with the new APIs. Currently .add_device() and .remove_device() are only used to register devices from a bus notifier and that would be replaced by something more explicit like above. As for device_group(), I don't see it being used at all currently. Now for DMA mapping API integration we could make that use the first (or only) IOMMU device registered. Perhaps we could even reject using this layer of integration for multi-master devices, since it would be difficult to tell whether or not the selected device is the correct one. We still have the option to handle things mostly transparently with the above by moving calls to iommu_get() into the core. But we also gain the flexibility to work with multiple IOMMU contexts explicitly if required. Thierry
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 -EINVAL; } +static inline int iommu_attach(struct device *dev) +{ + return 0; +} + +static inline int iommu_detach(struct device *dev) +{ + return 0; +} #endif /* CONFIG_IOMMU_API */ #endif /* __LINUX_IOMMU_H */