Re: [RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

2014-09-03 Thread Will Deacon 
On Tue, Sep 02, 2014 at 09:59:41PM +0100, jroe...@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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

2014-09-02 Thread Will Deacon 
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

2014-09-02 Thread Arnd Bergmann 
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

2014-09-02 Thread Will Deacon 
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

2014-09-02 Thread Arnd Bergmann 
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

2014-09-02 Thread Varun Sethi 
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.dea...@arm.com]
 Sent: Friday, August 29, 2014 9:24 PM
 To: linux-arm-ker...@lists.infradead.org; iommu@lists.linux-foundation.org
 Cc: a...@arndb.de; dw...@infradead.org; jroe...@suse.de;
 hd...@nvidia.com; Sethi Varun-B16395; thierry.red...@gmail.com;
 laurent.pinch...@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.dea...@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

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

2014-09-02 Thread jroe...@suse.de 
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

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

2014-09-01 Thread Arnd Bergmann 
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

2014-09-01 Thread Will Deacon 
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

2014-09-01 Thread Arnd Bergmann 
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu