| Message ID | 1409327670-3495-5-git-send-email-will.deacon@arm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 29, 2014 at 04:54:27PM +0100, Will Deacon wrote: > The generic IOMMU device-tree bindings can be used to add arbitrary OF > masters to an IOMMU with a compliant binding. > > This patch introduces of_iommu_configure, which does exactly that. > > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > drivers/iommu/Kconfig | 2 +- > drivers/iommu/of_iommu.c | 36 ++++++++++++++++++++++++++++++++++++ > include/linux/of_iommu.h | 6 ++++++ > 3 files changed, 43 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > index dd5112265cc9..6d13f962f156 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -15,7 +15,7 @@ if IOMMU_SUPPORT > > config OF_IOMMU > def_bool y > - depends on OF > + depends on OF && IOMMU_API > > config FSL_PAMU > bool "Freescale IOMMU support" > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index f9209666157c..a7d3b3a13b83 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -19,6 +19,7 @@ > > #include <linux/export.h> > #include <linux/limits.h> > +#include <linux/iommu.h> > #include <linux/of.h> > #include <linux/of_iommu.h> > > @@ -90,6 +91,41 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index, > } > EXPORT_SYMBOL_GPL(of_get_dma_window); > > +int of_iommu_configure(struct device *dev) > +{ > + struct of_phandle_args iommu_spec; > + struct bus_type *bus = dev->bus; > + const struct iommu_ops *ops = bus->iommu_ops; > + int ret = -EINVAL, idx = 0; > + > + if (!iommu_present(bus)) > + return -ENODEV; > + > + /* > + * We don't currently walk up the tree looking for a parent IOMMU. > + * See the `Notes:' section of > + * Documentation/devicetree/bindings/iommu/iommu.txt > + */ > + while (!of_parse_phandle_with_args(dev->of_node, "iommus", > + "#iommu-cells", idx, > + &iommu_spec)) { > + void *data = of_iommu_get_data(iommu_spec.np); > + > + of_node_put(iommu_spec.np); > + if (!ops->add_device_master_ids) > + return -ENODEV; > + > + ret = ops->add_device_master_ids(dev, iommu_spec.args_count, > + iommu_spec.args, data); > + if (ret) > + break; I think this could use a bit more formalization. As I said in another reply earlier, there's very little standardization in the IOMMU API. That certainly gives us a lot of flexibility but it also has the downside that it's difficult to handle these abstractions in the core, which is really what the core is all about, isn't it? One method that worked really well for this in the past for other subsystems is to allow drivers to specify an .of_xlate() function that takes the controller device and a struct of_phandle_args. It is that function's responsibility to take the information in an of_phandle_args structure and use that to create some subsystem specific handle that represents this information in a way that it can readily be used. So I think it would really be helpful if IOMMU gained support for something similar. We could create a struct iommu to represent an instance of an IOMMU. IOMMU drivers can embed this structure and add device-specific fields that they need. That way we can easily pass around instances and upcast in the driver in a type-safe way. At the same time, a struct iommu_master could provide the basis to represent a single master interface on an IOMMU. Drivers can again embed that in driver-specific structures with additional fields required for the particular IOMMU implementation. .of_xlate() could return such an IOMMU master for the core to use. With such structures in place we should be able to eliminate many of the loops in IOMMU drivers that serve no other purpose than to find the master context from a struct device * and some parameters. It will also allow us to keep a central registry of IOMMUs and masters rather than duplicating that in every driver. Thierry
On Monday 01 September 2014 10:29:40 Thierry Reding wrote: > > I think this could use a bit more formalization. As I said in another > reply earlier, there's very little standardization in the IOMMU API. > That certainly gives us a lot of flexibility but it also has the > downside that it's difficult to handle these abstractions in the core, > which is really what the core is all about, isn't it? > > One method that worked really well for this in the past for other > subsystems is to allow drivers to specify an .of_xlate() function that > takes the controller device and a struct of_phandle_args. It is that > function's responsibility to take the information in an of_phandle_args > structure and use that to create some subsystem specific handle that > represents this information in a way that it can readily be used. Yes, good idea. > So I think it would really be helpful if IOMMU gained support for > something similar. We could create a struct iommu to represent an > instance of an IOMMU. IOMMU drivers can embed this structure and add > device-specific fields that they need. That way we can easily pass > around instances and upcast in the driver in a type-safe way. Right. > At the same time, a struct iommu_master could provide the basis to > represent a single master interface on an IOMMU. Drivers can again embed > that in driver-specific structures with additional fields required for > the particular IOMMU implementation. .of_xlate() could return such an > IOMMU master for the core to use. I'm not convinced it's necessary. Could this just be a 'struct device' instead of 'struct iommu_master'? > With such structures in place we should be able to eliminate many of the > loops in IOMMU drivers that serve no other purpose than to find the > master context from a struct device * and some parameters. It will also > allow us to keep a central registry of IOMMUs and masters rather than > duplicating that in every driver. Yes, we should be able to identify an iommu context in a generic way, but why do you want to break it down to individual masters within one context? Arnd
On Mon, Sep 01, 2014 at 03:46:18PM +0100, Arnd Bergmann wrote: > On Monday 01 September 2014 10:29:40 Thierry Reding wrote: > > > > I think this could use a bit more formalization. As I said in another > > reply earlier, there's very little standardization in the IOMMU API. > > That certainly gives us a lot of flexibility but it also has the > > downside that it's difficult to handle these abstractions in the core, > > which is really what the core is all about, isn't it? > > > > One method that worked really well for this in the past for other > > subsystems is to allow drivers to specify an .of_xlate() function that > > takes the controller device and a struct of_phandle_args. It is that > > function's responsibility to take the information in an of_phandle_args > > structure and use that to create some subsystem specific handle that > > represents this information in a way that it can readily be used. > > Yes, good idea. Hmm, how does this work for PCI devices? The current RFC takes care to ensure that the core changes work just as well for OF devices as PCI devices, and the of-specific functions and data structures are not part of it. Will
On Monday 01 September 2014 17:40:00 Will Deacon wrote: > On Mon, Sep 01, 2014 at 03:46:18PM +0100, Arnd Bergmann wrote: > > On Monday 01 September 2014 10:29:40 Thierry Reding wrote: > > > > > > I think this could use a bit more formalization. As I said in another > > > reply earlier, there's very little standardization in the IOMMU API. > > > That certainly gives us a lot of flexibility but it also has the > > > downside that it's difficult to handle these abstractions in the core, > > > which is really what the core is all about, isn't it? > > > > > > One method that worked really well for this in the past for other > > > subsystems is to allow drivers to specify an .of_xlate() function that > > > takes the controller device and a struct of_phandle_args. It is that > > > function's responsibility to take the information in an of_phandle_args > > > structure and use that to create some subsystem specific handle that > > > represents this information in a way that it can readily be used. > > > > Yes, good idea. > > Hmm, how does this work for PCI devices? The current RFC takes care to > ensure that the core changes work just as well for OF devices as PCI > devices, and the of-specific functions and data structures are not part of > it. I don't mind handling PCI devices separately. They are different in a number of ways already, in particular the way that they don't normally have an of_node attached to them but actually have a PCI bus/dev/function number. Arnd
On Mon, Sep 01, 2014 at 09:18:26PM +0100, Arnd Bergmann wrote: > On Monday 01 September 2014 17:40:00 Will Deacon wrote: > > On Mon, Sep 01, 2014 at 03:46:18PM +0100, Arnd Bergmann wrote: > > > On Monday 01 September 2014 10:29:40 Thierry Reding wrote: > > > > > > > > I think this could use a bit more formalization. As I said in another > > > > reply earlier, there's very little standardization in the IOMMU API. > > > > That certainly gives us a lot of flexibility but it also has the > > > > downside that it's difficult to handle these abstractions in the core, > > > > which is really what the core is all about, isn't it? > > > > > > > > One method that worked really well for this in the past for other > > > > subsystems is to allow drivers to specify an .of_xlate() function that > > > > takes the controller device and a struct of_phandle_args. It is that > > > > function's responsibility to take the information in an of_phandle_args > > > > structure and use that to create some subsystem specific handle that > > > > represents this information in a way that it can readily be used. > > > > > > Yes, good idea. > > > > Hmm, how does this work for PCI devices? The current RFC takes care to > > ensure that the core changes work just as well for OF devices as PCI > > devices, and the of-specific functions and data structures are not part of > > it. > > I don't mind handling PCI devices separately. They are different in a number > of ways already, in particular the way that they don't normally have an > of_node attached to them but actually have a PCI bus/dev/function number. Sure, but at the point when we call back into the iommu_ops structure we really don't want bus specific functions. That's why I avoided any OF data structures being passed to add_device_master_ids. Anyway, I'll try to hack something together shortly. I think the proposal is: - Make add_device_master_ids take a generic structure (struct iommu) - Add an of_xlate callback into iommu_ops which returns a populated struct iommu based on the of_node Sound about right? Will
On Monday 01 September 2014 16:46:18 Arnd Bergmann wrote: > On Monday 01 September 2014 10:29:40 Thierry Reding wrote: > > I think this could use a bit more formalization. As I said in another > > reply earlier, there's very little standardization in the IOMMU API. > > That certainly gives us a lot of flexibility but it also has the > > downside that it's difficult to handle these abstractions in the core, > > which is really what the core is all about, isn't it? > > > > One method that worked really well for this in the past for other > > subsystems is to allow drivers to specify an .of_xlate() function that > > takes the controller device and a struct of_phandle_args. It is that > > function's responsibility to take the information in an of_phandle_args > > structure and use that to create some subsystem specific handle that > > represents this information in a way that it can readily be used. > > Yes, good idea. > > > So I think it would really be helpful if IOMMU gained support for > > something similar. We could create a struct iommu to represent an > > instance of an IOMMU. IOMMU drivers can embed this structure and add > > device-specific fields that they need. That way we can easily pass > > around instances and upcast in the driver in a type-safe way. > > Right. > > > At the same time, a struct iommu_master could provide the basis to > > represent a single master interface on an IOMMU. Drivers can again embed > > that in driver-specific structures with additional fields required for > > the particular IOMMU implementation. .of_xlate() could return such an > > IOMMU master for the core to use. > > I'm not convinced it's necessary. Could this just be a 'struct device' > instead of 'struct iommu_master'? As master drivers should in general be unaware of the IOMMU that serves them, embedding a struct iommu_master inside driver-specific structures is a no-go. Let's also not forget that a device can be connected to multiple IOMMU ports. The only cases I've seen had multiple connections between a bus master device and a single IOMMU, all of them potentially sharing a TLB, so a simple implementation was possible. I'm not sure how we could handle connections from one device to different IOMMUs, or to multiple ports of the same IOMMU with different TLBs. Splitting the device in multiple children struct device might be required. > > With such structures in place we should be able to eliminate many of the > > loops in IOMMU drivers that serve no other purpose than to find the > > master context from a struct device * and some parameters. It will also > > allow us to keep a central registry of IOMMUs and masters rather than > > duplicating that in every driver. > > Yes, we should be able to identify an iommu context in a generic way, > but why do you want to break it down to individual masters within > one context?
Hi Will, Thank you for the patch. On Friday 29 August 2014 16:54:27 Will Deacon wrote: > The generic IOMMU device-tree bindings can be used to add arbitrary OF > masters to an IOMMU with a compliant binding. > > This patch introduces of_iommu_configure, which does exactly that. > > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > drivers/iommu/Kconfig | 2 +- > drivers/iommu/of_iommu.c | 36 ++++++++++++++++++++++++++++++++++++ > include/linux/of_iommu.h | 6 ++++++ > 3 files changed, 43 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > index dd5112265cc9..6d13f962f156 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -15,7 +15,7 @@ if IOMMU_SUPPORT > > config OF_IOMMU > def_bool y > - depends on OF > + depends on OF && IOMMU_API > > config FSL_PAMU > bool "Freescale IOMMU support" > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index f9209666157c..a7d3b3a13b83 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -19,6 +19,7 @@ > > #include <linux/export.h> > #include <linux/limits.h> > +#include <linux/iommu.h> Pet peeve of mine, how about keeping the headers alphabetically sorted ? > #include <linux/of.h> > #include <linux/of_iommu.h> > > @@ -90,6 +91,41 @@ int of_get_dma_window(struct device_node *dn, const char > *prefix, int index, } > EXPORT_SYMBOL_GPL(of_get_dma_window); > > +int of_iommu_configure(struct device *dev) > +{ > + struct of_phandle_args iommu_spec; > + struct bus_type *bus = dev->bus; > + const struct iommu_ops *ops = bus->iommu_ops; > + int ret = -EINVAL, idx = 0; > + > + if (!iommu_present(bus)) > + return -ENODEV; > + > + /* > + * We don't currently walk up the tree looking for a parent IOMMU. > + * See the `Notes:' section of > + * Documentation/devicetree/bindings/iommu/iommu.txt > + */ > + while (!of_parse_phandle_with_args(dev->of_node, "iommus", > + "#iommu-cells", idx, > + &iommu_spec)) { > + void *data = of_iommu_get_data(iommu_spec.np); > + > + of_node_put(iommu_spec.np); > + if (!ops->add_device_master_ids) > + return -ENODEV; Can't you move this outside of the loop ? > + > + ret = ops->add_device_master_ids(dev, iommu_spec.args_count, > + iommu_spec.args, data); I'm curious, how do you envision support for devices connected to multiple IOMMUs ? IOMMU drivers usually allocate a per-device archdata structure in the add_device operation and store it in the dev->archdata.iommu field. How would that work with multiple invocations of add_device or add_device_master_ids ? > + if (ret) > + break; > + > + idx++; > + } > + > + return ret; > +} > + > void __init of_iommu_init(void) > { > struct device_node *np; > diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h > index 29f2f3f88d6a..85c6d1152624 100644 > --- a/include/linux/of_iommu.h > +++ b/include/linux/of_iommu.h > @@ -1,6 +1,7 @@ > #ifndef __OF_IOMMU_H > #define __OF_IOMMU_H > > +#include <linux/device.h> > #include <linux/of.h> > > #ifdef CONFIG_OF_IOMMU > @@ -10,6 +11,7 @@ extern int of_get_dma_window(struct device_node *dn, const > char *prefix, size_t *size); > > extern void of_iommu_init(void); > +extern int of_iommu_configure(struct device *dev); > > #else > > @@ -21,6 +23,10 @@ static inline int of_get_dma_window(struct device_node > *dn, const char *prefix, } > > static inline void of_iommu_init(void) { } > +static inline int of_iommu_configure(struct device *dev) > +{ > + return -ENODEV; > +} > > #endif /* CONFIG_OF_IOMMU */
On Tue, Sep 02, 2014 at 11:51:54AM +0100, Laurent Pinchart wrote: > Hi Will, Hi Laurent, > On Friday 29 August 2014 16:54:27 Will Deacon wrote: > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > > index dd5112265cc9..6d13f962f156 100644 > > --- a/drivers/iommu/Kconfig > > +++ b/drivers/iommu/Kconfig > > @@ -15,7 +15,7 @@ if IOMMU_SUPPORT > > > > config OF_IOMMU > > def_bool y > > - depends on OF > > + depends on OF && IOMMU_API > > > > config FSL_PAMU > > bool "Freescale IOMMU support" > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > > index f9209666157c..a7d3b3a13b83 100644 > > --- a/drivers/iommu/of_iommu.c > > +++ b/drivers/iommu/of_iommu.c > > @@ -19,6 +19,7 @@ > > > > #include <linux/export.h> > > #include <linux/limits.h> > > +#include <linux/iommu.h> > > Pet peeve of mine, how about keeping the headers alphabetically sorted ? D'oh, I'm an idiot. Will fix. > > #include <linux/of.h> > > #include <linux/of_iommu.h> > > > > @@ -90,6 +91,41 @@ int of_get_dma_window(struct device_node *dn, const char > > *prefix, int index, } > > EXPORT_SYMBOL_GPL(of_get_dma_window); > > > > +int of_iommu_configure(struct device *dev) > > +{ > > + struct of_phandle_args iommu_spec; > > + struct bus_type *bus = dev->bus; > > + const struct iommu_ops *ops = bus->iommu_ops; > > + int ret = -EINVAL, idx = 0; > > + > > + if (!iommu_present(bus)) > > + return -ENODEV; > > + > > + /* > > + * We don't currently walk up the tree looking for a parent IOMMU. > > + * See the `Notes:' section of > > + * Documentation/devicetree/bindings/iommu/iommu.txt > > + */ > > + while (!of_parse_phandle_with_args(dev->of_node, "iommus", > > + "#iommu-cells", idx, > > + &iommu_spec)) { > > + void *data = of_iommu_get_data(iommu_spec.np); > > + > > + of_node_put(iommu_spec.np); > > + if (!ops->add_device_master_ids) > > + return -ENODEV; > > Can't you move this outside of the loop ? Yup, done. > > + > > + ret = ops->add_device_master_ids(dev, iommu_spec.args_count, > > + iommu_spec.args, data); > > I'm curious, how do you envision support for devices connected to multiple > IOMMUs ? IOMMU drivers usually allocate a per-device archdata structure in the > add_device operation and store it in the dev->archdata.iommu field. How would > that work with multiple invocations of add_device or add_device_master_ids ? Even devices with a single IOMMU could have multiple callbacks, since the IOMMU gets called back once per ID in reality. That means the IOMMU drivers will need to be tolerant of adding a master they already know about (by looking up the device and adding the additional IDs). Once I've reworked the data argument to be a struct iommu, we can add fields there if they're generally useful. Will
On Tuesday 02 September 2014 11:03:42 Will Deacon wrote: > On Mon, Sep 01, 2014 at 09:18:26PM +0100, Arnd Bergmann wrote: > > On Monday 01 September 2014 17:40:00 Will Deacon wrote: > > > On Mon, Sep 01, 2014 at 03:46:18PM +0100, Arnd Bergmann wrote: > > > > On Monday 01 September 2014 10:29:40 Thierry Reding wrote: > > > > > > > > > > I think this could use a bit more formalization. As I said in another > > > > > reply earlier, there's very little standardization in the IOMMU API. > > > > > That certainly gives us a lot of flexibility but it also has the > > > > > downside that it's difficult to handle these abstractions in the core, > > > > > which is really what the core is all about, isn't it? > > > > > > > > > > One method that worked really well for this in the past for other > > > > > subsystems is to allow drivers to specify an .of_xlate() function that > > > > > takes the controller device and a struct of_phandle_args. It is that > > > > > function's responsibility to take the information in an of_phandle_args > > > > > structure and use that to create some subsystem specific handle that > > > > > represents this information in a way that it can readily be used. > > > > > > > > Yes, good idea. > > > > > > Hmm, how does this work for PCI devices? The current RFC takes care to > > > ensure that the core changes work just as well for OF devices as PCI > > > devices, and the of-specific functions and data structures are not part of > > > it. > > > > I don't mind handling PCI devices separately. They are different in a number > > of ways already, in particular the way that they don't normally have an > > of_node attached to them but actually have a PCI bus/dev/function number. > > Sure, but at the point when we call back into the iommu_ops structure we > really don't want bus specific functions. That's why I avoided any OF > data structures being passed to add_device_master_ids. Well, we clearly need some format that the caller and the callee agree on. It can't be a completely opaque pointer because it's something that has to be filled out by someone who knows the format. Using the DT format has the advantage that the caller does not have to know anything about the underlying driver except for #size-cells, and it just passes the data it gets from DT into the driver. This is how we do the association in a lot of other subsystems. > Anyway, I'll try to hack something together shortly. I think the proposal > is: > > - Make add_device_master_ids take a generic structure (struct iommu) > - Add an of_xlate callback into iommu_ops which returns a populated > struct iommu based on the of_node We may have been talking past one another. What I meant with 'struct iommu' is something that identifies the iommu instance, not the connection to a particular master. What you describe here would work, but then I think the structure should have a different name. However, it seems easier to not have the add_device_master_ids at and just do the association in the xlate callback instead. We still need to figure out how to do it for PCI of course. One possibility would be to add another argument to the xlate function and have that called by the PCI device probing method with the iommus property of the PCI host controller along with the a u64 number that is generated by the host bridge driver based on the bus/device/function number of the device. This means that the new callback function for the iommu API remains DT specific, but is not really bus specific. It does however not solve the embedded x86 use case, which may need some other callback. We might be lucky there if we are able to just use the PCI b/d/f number as a unique identifier and have a NULL argument for the respective iommus property. Arnd
On Tue, Sep 02, 2014 at 01:15:06PM +0100, Arnd Bergmann wrote: > On Tuesday 02 September 2014 11:03:42 Will Deacon wrote: > > On Mon, Sep 01, 2014 at 09:18:26PM +0100, Arnd Bergmann wrote: > > > I don't mind handling PCI devices separately. They are different in a number > > > of ways already, in particular the way that they don't normally have an > > > of_node attached to them but actually have a PCI bus/dev/function number. > > > > Sure, but at the point when we call back into the iommu_ops structure we > > really don't want bus specific functions. That's why I avoided any OF > > data structures being passed to add_device_master_ids. > > Well, we clearly need some format that the caller and the callee agree > on. It can't be a completely opaque pointer because it's something > that has to be filled out by someone who knows the format. > > Using the DT format has the advantage that the caller does not have > to know anything about the underlying driver except for #size-cells, > and it just passes the data it gets from DT into the driver. This is > how we do the association in a lot of other subsystems. Ok. More below. > > Anyway, I'll try to hack something together shortly. I think the proposal > > is: > > > > - Make add_device_master_ids take a generic structure (struct iommu) > > - Add an of_xlate callback into iommu_ops which returns a populated > > struct iommu based on the of_node > > We may have been talking past one another. What I meant with 'struct iommu' > is something that identifies the iommu instance, not the connection to > a particular master. What you describe here would work, but then I think > the structure should have a different name. However, it seems easier to > not have the add_device_master_ids at and just do the association in the > xlate callback instead. Yes, I think we were talking about two different things. If we move all of the master handling into the xlate callback, then we can just use of_phandle_args as the generic master representation (using the PCI host controller node for PCI devices, as you mentioned). However, xlate is a bit of a misnomer then, as it's not actually translating anything; the handle used for DMA masters is still struct device, and we have that already in of_dma_configure. I'm still unsure about what to put inside struct iommu other than a private data pointer. You previously suggested: struct iommu { struct device *dev; const struct iommu_ops *ops; struct list_head domains; void *private; }; but that has the following problems: - We don't have a struct device * for the IOMMU until it's been probed via the standard driver probing mechanisms, which may well be after we've started registering masters - The ops are still registered on a per-bus basis, and each domain has a pointer to them anyway - The IOMMU driver really doesn't care about the domains, as the domain in question is always passed back to the functions that need it (e.g. attach, map, ...). The only useful field I can think of is something like a tree of masters, but then we have to define a generic wrapper around struct device, which is at odds with the rest of the IOMMU API. One alternative is having the xlate call populate device->archdata.iommu, but that's arch-specific and is essentially another opaque pointer. Will
On Tuesday 02 September 2014 14:05:08 Will Deacon wrote: > On Tue, Sep 02, 2014 at 01:15:06PM +0100, Arnd Bergmann wrote: > > > Anyway, I'll try to hack something together shortly. I think the proposal > > > is: > > > > > > - Make add_device_master_ids take a generic structure (struct iommu) > > > - Add an of_xlate callback into iommu_ops which returns a populated > > > struct iommu based on the of_node > > > > We may have been talking past one another. What I meant with 'struct iommu' > > is something that identifies the iommu instance, not the connection to > > a particular master. What you describe here would work, but then I think > > the structure should have a different name. However, it seems easier to > > not have the add_device_master_ids at and just do the association in the > > xlate callback instead. > > Yes, I think we were talking about two different things. If we move all > of the master handling into the xlate callback, then we can just use > of_phandle_args as the generic master representation (using the PCI host > controller node for PCI devices, as you mentioned). However, xlate is a > bit of a misnomer then, as it's not actually translating anything; the > handle used for DMA masters is still struct device, and we have that > already in of_dma_configure. > > I'm still unsure about what to put inside struct iommu other than a private > data pointer. You previously suggested: > > struct iommu { > struct device *dev; > const struct iommu_ops *ops; > struct list_head domains; > void *private; > }; > > but that has the following problems: > > - We don't have a struct device * for the IOMMU until it's been probed > via the standard driver probing mechanisms, which may well be after > we've started registering masters Right, this may have to be the device_node pointer. I also realized that if we have this structure, we can stop using the device_node->data field and instead walk a list of iommu structures. > - The ops are still registered on a per-bus basis, and each domain has > a pointer to them anyway I think that has to change in the long run: we may well require distinct IOMMU operations if we have multiple IOMMUs used on the same bus_type. > - The IOMMU driver really doesn't care about the domains, as the domain > in question is always passed back to the functions that need it (e.g. > attach, map, ...). This is an artifact of the API being single-instance at the moment. We might not in fact need it, I was just trying to think of things that naturally fit in there and that are probably already linked together in the individual iommu drivers. > The only useful field I can think of is something like a tree of masters, > but then we have to define a generic wrapper around struct device, which > is at odds with the rest of the IOMMU API. Maybe a list of the groups instead? I don't think you should need a list of every single master here. > One alternative is having the xlate call populate device->archdata.iommu, > but that's arch-specific and is essentially another opaque pointer. I think every architecture that supports IOMMUs needs to have this archdata pointer though, so that is still a good place to put private stuff. Arnd
Hi Will, I am not clear on the functionality we want to achieve with this new API. Is this a way to link devices to a particular IOMMU? Would this be used to filter out add_device invocations i.e. iommu group creations just for the devices attached to a particular IOMMU? What is the purpose of the data pointer, that is being passed to this API? Also, how would this be relevant to PCIe devices (also other hot plug devices). Regards Varun > -----Original Message----- > From: Will Deacon [mailto:will.deacon@arm.com] > Sent: Friday, August 29, 2014 9:24 PM > To: linux-arm-kernel@lists.infradead.org; iommu@lists.linux-foundation.org > Cc: arnd@arndb.de; dwmw2@infradead.org; jroedel@suse.de; > hdoyu@nvidia.com; Sethi Varun-B16395; thierry.reding@gmail.com; > laurent.pinchart@ideasonboard.com; Will Deacon > Subject: [RFC PATCH 4/7] iommu: provide helper function to configure an > IOMMU for an of master > > The generic IOMMU device-tree bindings can be used to add arbitrary OF > masters to an IOMMU with a compliant binding. > > This patch introduces of_iommu_configure, which does exactly that. > > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > drivers/iommu/Kconfig | 2 +- > drivers/iommu/of_iommu.c | 36 > ++++++++++++++++++++++++++++++++++++ > include/linux/of_iommu.h | 6 ++++++ > 3 files changed, 43 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index > dd5112265cc9..6d13f962f156 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -15,7 +15,7 @@ if IOMMU_SUPPORT > > config OF_IOMMU > def_bool y > - depends on OF > + depends on OF && IOMMU_API > > config FSL_PAMU > bool "Freescale IOMMU support" > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index > f9209666157c..a7d3b3a13b83 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -19,6 +19,7 @@ > > #include <linux/export.h> > #include <linux/limits.h> > +#include <linux/iommu.h> > #include <linux/of.h> > #include <linux/of_iommu.h> > > @@ -90,6 +91,41 @@ int of_get_dma_window(struct device_node *dn, const > char *prefix, int index, } EXPORT_SYMBOL_GPL(of_get_dma_window); > > +int of_iommu_configure(struct device *dev) { > + struct of_phandle_args iommu_spec; > + struct bus_type *bus = dev->bus; > + const struct iommu_ops *ops = bus->iommu_ops; > + int ret = -EINVAL, idx = 0; > + > + if (!iommu_present(bus)) > + return -ENODEV; > + > + /* > + * We don't currently walk up the tree looking for a parent IOMMU. > + * See the `Notes:' section of > + * Documentation/devicetree/bindings/iommu/iommu.txt > + */ > + while (!of_parse_phandle_with_args(dev->of_node, "iommus", > + "#iommu-cells", idx, > + &iommu_spec)) { > + void *data = of_iommu_get_data(iommu_spec.np); > + > + of_node_put(iommu_spec.np); > + if (!ops->add_device_master_ids) > + return -ENODEV; > + > + ret = ops->add_device_master_ids(dev, > iommu_spec.args_count, > + iommu_spec.args, data); > + if (ret) > + break; > + > + idx++; > + } > + > + return ret; > +} > + > void __init of_iommu_init(void) > { > struct device_node *np; > diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h index > 29f2f3f88d6a..85c6d1152624 100644 > --- a/include/linux/of_iommu.h > +++ b/include/linux/of_iommu.h > @@ -1,6 +1,7 @@ > #ifndef __OF_IOMMU_H > #define __OF_IOMMU_H > > +#include <linux/device.h> > #include <linux/of.h> > > #ifdef CONFIG_OF_IOMMU > @@ -10,6 +11,7 @@ extern int of_get_dma_window(struct device_node *dn, > const char *prefix, > size_t *size); > > extern void of_iommu_init(void); > +extern int of_iommu_configure(struct device *dev); > > #else > > @@ -21,6 +23,10 @@ static inline int of_get_dma_window(struct device_node > *dn, const char *prefix, } > > static inline void of_iommu_init(void) { } > +static inline int of_iommu_configure(struct device *dev) { > + return -ENODEV; > +} > > #endif /* CONFIG_OF_IOMMU */ > > -- > 2.1.0
Hi Thierry/Will/Arnd, Where would the of_xlate callback reside and what would be its function? Regards Varun > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: Tuesday, September 02, 2014 5:45 PM > To: linux-arm-kernel@lists.infradead.org > Cc: Will Deacon; jroedel@suse.de; iommu@lists.linux-foundation.org; Thierry > Reding; laurent.pinchart@ideasonboard.com; Sethi Varun-B16395; > dwmw2@infradead.org; hdoyu@nvidia.com > Subject: Re: [RFC PATCH 4/7] iommu: provide helper function to configure an > IOMMU for an of master > > On Tuesday 02 September 2014 11:03:42 Will Deacon wrote: > > On Mon, Sep 01, 2014 at 09:18:26PM +0100, Arnd Bergmann wrote: > > > On Monday 01 September 2014 17:40:00 Will Deacon wrote: > > > > On Mon, Sep 01, 2014 at 03:46:18PM +0100, Arnd Bergmann wrote: > > > > > On Monday 01 September 2014 10:29:40 Thierry Reding wrote: > > > > > > > > > > > > I think this could use a bit more formalization. As I said in > > > > > > another reply earlier, there's very little standardization in the IOMMU > API. > > > > > > That certainly gives us a lot of flexibility but it also has > > > > > > the downside that it's difficult to handle these abstractions > > > > > > in the core, which is really what the core is all about, isn't it? > > > > > > > > > > > > One method that worked really well for this in the past for > > > > > > other subsystems is to allow drivers to specify an .of_xlate() > > > > > > function that takes the controller device and a struct > > > > > > of_phandle_args. It is that function's responsibility to take > > > > > > the information in an of_phandle_args structure and use that > > > > > > to create some subsystem specific handle that represents this > information in a way that it can readily be used. > > > > > > > > > > Yes, good idea. > > > > > > > > Hmm, how does this work for PCI devices? The current RFC takes > > > > care to ensure that the core changes work just as well for OF > > > > devices as PCI devices, and the of-specific functions and data > > > > structures are not part of it. > > > > > > I don't mind handling PCI devices separately. They are different in > > > a number of ways already, in particular the way that they don't > > > normally have an of_node attached to them but actually have a PCI > bus/dev/function number. > > > > Sure, but at the point when we call back into the iommu_ops structure > > we really don't want bus specific functions. That's why I avoided any > > OF data structures being passed to add_device_master_ids. > > Well, we clearly need some format that the caller and the callee agree on. It > can't be a completely opaque pointer because it's something that has to be > filled out by someone who knows the format. > > Using the DT format has the advantage that the caller does not have to know > anything about the underlying driver except for #size-cells, and it just passes > the data it gets from DT into the driver. This is how we do the association in a > lot of other subsystems. > > > Anyway, I'll try to hack something together shortly. I think the > > proposal > > is: > > > > - Make add_device_master_ids take a generic structure (struct iommu) > > - Add an of_xlate callback into iommu_ops which returns a populated > > struct iommu based on the of_node > > We may have been talking past one another. What I meant with 'struct iommu' > is something that identifies the iommu instance, not the connection to a > particular master. What you describe here would work, but then I think the > structure should have a different name. However, it seems easier to not have > the add_device_master_ids at and just do the association in the xlate callback > instead. > > We still need to figure out how to do it for PCI of course. One possibility would > be to add another argument to the xlate function and have that called by the > PCI device probing method with the iommus property of the PCI host controller > along with the a u64 number that is generated by the host bridge driver based > on the bus/device/function number of the device. > > This means that the new callback function for the iommu API remains DT > specific, but is not really bus specific. It does however not solve the embedded > x86 use case, which may need some other callback. > > We might be lucky there if we are able to just use the PCI b/d/f number as a > unique identifier and have a NULL argument for the respective iommus > property.
On Tuesday 02 September 2014 15:03:14 Varun Sethi wrote: > Hi Thierry/Will/Arnd, > Where would the of_xlate callback reside and what would be its function? > It is an additional function pointer in iommu_ops and replaces the add_device callback for IOMMUs that are DT-enabled. The idea is that we can have a single function that sets up DMA for the device properly, including iommus when they are configured, and it lets us pass the necessary parameters describing the setup of the master device into the iommu driver. Arnd
Hi Will, On Tuesday 02 September 2014 12:03:40 Will Deacon wrote: > On Tue, Sep 02, 2014 at 11:51:54AM +0100, Laurent Pinchart wrote: > > On Friday 29 August 2014 16:54:27 Will Deacon wrote: [snip] > > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > > > index f9209666157c..a7d3b3a13b83 100644 > > > --- a/drivers/iommu/of_iommu.c > > > +++ b/drivers/iommu/of_iommu.c [snip] > > > @@ -90,6 +91,41 @@ int of_get_dma_window(struct device_node *dn, const > > > char > > > *prefix, int index, } > > > > > > EXPORT_SYMBOL_GPL(of_get_dma_window); > > > > > > +int of_iommu_configure(struct device *dev) > > > +{ > > > + struct of_phandle_args iommu_spec; > > > + struct bus_type *bus = dev->bus; > > > + const struct iommu_ops *ops = bus->iommu_ops; > > > + int ret = -EINVAL, idx = 0; > > > + > > > + if (!iommu_present(bus)) > > > + return -ENODEV; > > > + > > > + /* > > > + * We don't currently walk up the tree looking for a parent IOMMU. > > > + * See the `Notes:' section of > > > + * Documentation/devicetree/bindings/iommu/iommu.txt > > > + */ > > > + while (!of_parse_phandle_with_args(dev->of_node, "iommus", > > > + "#iommu-cells", idx, > > > + &iommu_spec)) { > > > + void *data = of_iommu_get_data(iommu_spec.np); > > > + > > > + of_node_put(iommu_spec.np); > > > + if (!ops->add_device_master_ids) > > > + return -ENODEV; > > > > Can't you move this outside of the loop ? > > Yup, done. > > > > + > > > + ret = ops->add_device_master_ids(dev, iommu_spec.args_count, > > > + iommu_spec.args, data); > > > > I'm curious, how do you envision support for devices connected to multiple > > IOMMUs ? IOMMU drivers usually allocate a per-device archdata structure in > > the add_device operation and store it in the dev->archdata.iommu field. > > How would that work with multiple invocations of add_device or > > add_device_master_ids ? > > Even devices with a single IOMMU could have multiple callbacks, since the > IOMMU gets called back once per ID in reality. That means the IOMMU drivers > will need to be tolerant of adding a master they already know about (by > looking up the device and adding the additional IDs). That wouldn't be too difficult to implement in IOMMU drivers, but I have a feeling we would duplicate the same logic in several drivers. Adding fields to struct iommu (possibly with a couple of helper functions) as mentioned below might help. Another option would be to call the add_device_master_ids a single time with a list of all ids, but the core code might become pretty ugly. > Once I've reworked the data argument to be a struct iommu, we can add fields > there if they're generally useful.
On Tue, Sep 02, 2014 at 04:01:32PM +0200, Arnd Bergmann wrote: > This is an artifact of the API being single-instance at the moment. > We might not in fact need it, I was just trying to think of things > that naturally fit in there and that are probably already linked > together in the individual iommu drivers. I am not sure what you mean by single-instance. Is it that currently the API only supports one type of iommu_ops per bus? That should be fine as long as there is only one type of IOMMU on the bus. Besides that, it is a feature of the IOMMU-API to hide the details about all the hardware IOMMUs in the system from its users. Joerg
On Tue, Sep 02, 2014 at 09:59:41PM +0100, jroedel@suse.de wrote: > On Tue, Sep 02, 2014 at 04:01:32PM +0200, Arnd Bergmann wrote: > > This is an artifact of the API being single-instance at the moment. > > We might not in fact need it, I was just trying to think of things > > that naturally fit in there and that are probably already linked > > together in the individual iommu drivers. > > I am not sure what you mean by single-instance. Is it that currently the > API only supports one type of iommu_ops per bus? That should be fine as > long as there is only one type of IOMMU on the bus. The problem is really with the platform_bus, which is used as a catch-all for all the non-probable memory-mapped buses on an SoC. We really want to have different IOMMU types there. > Besides that, it is a feature of the IOMMU-API to hide the details about > all the hardware IOMMUs in the system from its users. Sure, but even if we have multiple instances of the same IOMMU type, the complexity in dealing with that is currently pushed down into the drivers, with each one trying to construct its own notion of topology and master linkages. Moving some of this into generic code would ease the burden on IOMMU drivers. Will
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index dd5112265cc9..6d13f962f156 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -15,7 +15,7 @@ if IOMMU_SUPPORT config OF_IOMMU def_bool y - depends on OF + depends on OF && IOMMU_API config FSL_PAMU bool "Freescale IOMMU support" diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index f9209666157c..a7d3b3a13b83 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -19,6 +19,7 @@ #include <linux/export.h> #include <linux/limits.h> +#include <linux/iommu.h> #include <linux/of.h> #include <linux/of_iommu.h> @@ -90,6 +91,41 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index, } EXPORT_SYMBOL_GPL(of_get_dma_window); +int of_iommu_configure(struct device *dev) +{ + struct of_phandle_args iommu_spec; + struct bus_type *bus = dev->bus; + const struct iommu_ops *ops = bus->iommu_ops; + int ret = -EINVAL, idx = 0; + + if (!iommu_present(bus)) + return -ENODEV; + + /* + * We don't currently walk up the tree looking for a parent IOMMU. + * See the `Notes:' section of + * Documentation/devicetree/bindings/iommu/iommu.txt + */ + while (!of_parse_phandle_with_args(dev->of_node, "iommus", + "#iommu-cells", idx, + &iommu_spec)) { + void *data = of_iommu_get_data(iommu_spec.np); + + of_node_put(iommu_spec.np); + if (!ops->add_device_master_ids) + return -ENODEV; + + ret = ops->add_device_master_ids(dev, iommu_spec.args_count, + iommu_spec.args, data); + if (ret) + break; + + idx++; + } + + return ret; +} + void __init of_iommu_init(void) { struct device_node *np; diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h index 29f2f3f88d6a..85c6d1152624 100644 --- a/include/linux/of_iommu.h +++ b/include/linux/of_iommu.h @@ -1,6 +1,7 @@ #ifndef __OF_IOMMU_H #define __OF_IOMMU_H +#include <linux/device.h> #include <linux/of.h> #ifdef CONFIG_OF_IOMMU @@ -10,6 +11,7 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix, size_t *size); extern void of_iommu_init(void); +extern int of_iommu_configure(struct device *dev); #else @@ -21,6 +23,10 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix, } static inline void of_iommu_init(void) { } +static inline int of_iommu_configure(struct device *dev) +{ + return -ENODEV; +} #endif /* CONFIG_OF_IOMMU */
The generic IOMMU device-tree bindings can be used to add arbitrary OF masters to an IOMMU with a compliant binding. This patch introduces of_iommu_configure, which does exactly that. Signed-off-by: Will Deacon <will.deacon@arm.com> --- drivers/iommu/Kconfig | 2 +- drivers/iommu/of_iommu.c | 36 ++++++++++++++++++++++++++++++++++++ include/linux/of_iommu.h | 6 ++++++ 3 files changed, 43 insertions(+), 1 deletion(-)