[PATCH 0/10] IOMMU: Make iommu_ops per-bus_type

[PATCH 0/10] IOMMU: Make iommu_ops per-bus_type

[Joerg Roedel]

Hi,

here is the new version of the patch-set to make the iommu_ops used in
the iommu-api a bus_type property. This will allow us to move code out
of the iommu drivers into generic code and it simplifies the
implementation of the Alex' device-group property.

Greg, can you have a look at patch 2 please and tell me if you have any
objections?

With this version the patch-set is complete (not as the first RFC post).
It converts all iommu drivers to use the new registration interface and
completly removes the register_iommu interface.

Regards,

	Joerg

Diffstat:

 arch/ia64/kvm/kvm-ia64.c           |    3 +-
 arch/x86/kvm/x86.c                 |    3 +-
 drivers/base/bus.c                 |   16 ++++++++++
 drivers/iommu/amd_iommu.c          |    2 +-
 drivers/iommu/intel-iommu.c        |    2 +-
 drivers/iommu/iommu.c              |   58 ++++++++++++++++++++++++------------
 drivers/iommu/msm_iommu.c          |    2 +-
 drivers/iommu/omap-iommu.c         |    2 +-
 drivers/media/video/omap3isp/isp.c |    2 +-
 include/linux/device.h             |    9 +++++
 include/linux/iommu.h              |   21 +++++++------
 virt/kvm/iommu.c                   |    4 +-
 12 files changed, 86 insertions(+), 38 deletions(-)

[PATCH 01/10] iommu/core: Define iommu_ops and register_iommu only with CONFIG_IOMMU_API

[Joerg Roedel]

This makes it impossible to compile an iommu driver into the
kernel without selecting CONFIG_IOMMU_API.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 include/linux/iommu.h |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9940319..6470cd8 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -34,6 +34,8 @@ struct iommu_domain {
 #define IOMMU_CAP_CACHE_COHERENCY	0x1
 #define IOMMU_CAP_INTR_REMAP		0x2	/* isolates device intrs */
 
+#ifdef CONFIG_IOMMU_API
+
 struct iommu_ops {
 	int (*domain_init)(struct iommu_domain *domain);
 	void (*domain_destroy)(struct iommu_domain *domain);
@@ -49,8 +51,6 @@ struct iommu_ops {
 			      unsigned long cap);
 };
 
-#ifdef CONFIG_IOMMU_API
-
 extern void register_iommu(struct iommu_ops *ops);
 extern bool iommu_found(void);
 extern struct iommu_domain *iommu_domain_alloc(void);
@@ -70,9 +70,7 @@ extern int iommu_domain_has_cap(struct iommu_domain *domain,
 
 #else /* CONFIG_IOMMU_API */
 
-static inline void register_iommu(struct iommu_ops *ops)
-{
-}
+struct iommu_ops {};
 
 static inline bool iommu_found(void)
 {
-- 
1.7.4.1

[PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Joerg Roedel]

This is the starting point to make the iommu_ops used for
the iommu-api a per-bus-type structure. It is required to
easily implement bus-specific setup in the iommu-layer.
The first user will be the iommu-group attribute in sysfs.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/base/bus.c     |   16 ++++++++++++++++
 drivers/iommu/iommu.c  |    4 ++++
 include/linux/device.h |    9 +++++++++
 include/linux/iommu.h  |    2 ++
 4 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 000e7b2..34ac706 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -1028,6 +1028,22 @@ void bus_sort_breadthfirst(struct bus_type *bus,
 }
 EXPORT_SYMBOL_GPL(bus_sort_breadthfirst);
 
+#ifdef CONFIG_IOMMU_API
+int bus_set_iommu(struct bus_type *bus, struct iommu_ops *ops)
+{
+	if (bus->iommu_ops != NULL)
+		return -EBUSY;
+
+	bus->iommu_ops = ops;
+
+	/* Do IOMMU specific setup for this bus-type */
+	iommu_bus_init(bus, ops);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(bus_set_iommu);
+#endif
+
 int __init buses_init(void)
 {
 	bus_kset = kset_create_and_add("bus", &bus_uevent_ops, NULL);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 30b0644..3b24a5b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -34,6 +34,10 @@ void register_iommu(struct iommu_ops *ops)
 	iommu_ops = ops;
 }
 
+void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops)
+{
+}
+
 bool iommu_found(void)
 {
 	return iommu_ops != NULL;
diff --git a/include/linux/device.h b/include/linux/device.h
index c20dfbf..8240b2a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -22,6 +22,7 @@
 #include <linux/types.h>
 #include <linux/module.h>
 #include <linux/pm.h>
+#include <linux/iommu.h>
 #include <linux/atomic.h>
 #include <asm/device.h>
 
@@ -67,6 +68,9 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  * @resume:	Called to bring a device on this bus out of sleep mode.
  * @pm:		Power management operations of this bus, callback the specific
  *		device driver's pm-ops.
+ * @iommu_ops   IOMMU specific operations for this bus, used to attach IOMMU
+ *              driver implementations to a bus and allow the driver to do
+ *              bus-specific setup
  * @p:		The private data of the driver core, only the driver core can
  *		touch this.
  *
@@ -96,6 +100,8 @@ struct bus_type {
 
 	const struct dev_pm_ops *pm;
 
+	struct iommu_ops *iommu_ops;
+
 	struct subsys_private *p;
 };
 
@@ -148,6 +154,9 @@ extern int bus_unregister_notifier(struct bus_type *bus,
 #define BUS_NOTIFY_UNBOUND_DRIVER	0x00000006 /* driver is unbound
 						      from the device */
 
+/* IOMMU related bus functions */
+int bus_set_iommu(struct bus_type *bus, struct iommu_ops *ops);
+
 extern struct kset *bus_get_kset(struct bus_type *bus);
 extern struct klist *bus_get_device_klist(struct bus_type *bus);
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 6470cd8..4739e36 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -25,6 +25,7 @@
 #define IOMMU_WRITE	(2)
 #define IOMMU_CACHE	(4) /* DMA cache coherency */
 
+struct bus_type;
 struct device;
 
 struct iommu_domain {
@@ -52,6 +53,7 @@ struct iommu_ops {
 };
 
 extern void register_iommu(struct iommu_ops *ops);
+extern void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops);
 extern bool iommu_found(void);
 extern struct iommu_domain *iommu_domain_alloc(void);
 extern void iommu_domain_free(struct iommu_domain *domain);
-- 
1.7.4.1

Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Greg KH]

On Wed, Sep 07, 2011 at 05:41:45PM +0200, Joerg Roedel wrote:
> This is the starting point to make the iommu_ops used for
> the iommu-api a per-bus-type structure. It is required to
> easily implement bus-specific setup in the iommu-layer.
> The first user will be the iommu-group attribute in sysfs.
> 
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  drivers/base/bus.c     |   16 ++++++++++++++++
>  drivers/iommu/iommu.c  |    4 ++++
>  include/linux/device.h |    9 +++++++++
>  include/linux/iommu.h  |    2 ++
>  4 files changed, 31 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 000e7b2..34ac706 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -1028,6 +1028,22 @@ void bus_sort_breadthfirst(struct bus_type *bus,
>  }
>  EXPORT_SYMBOL_GPL(bus_sort_breadthfirst);
>  
> +#ifdef CONFIG_IOMMU_API
> +int bus_set_iommu(struct bus_type *bus, struct iommu_ops *ops)
> +{
> +	if (bus->iommu_ops != NULL)
> +		return -EBUSY;

Busy?

> +
> +	bus->iommu_ops = ops;
> +
> +	/* Do IOMMU specific setup for this bus-type */
> +	iommu_bus_init(bus, ops);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(bus_set_iommu);

I don't understand what this function is for, and who would call it.

Please provide kerneldoc that explains this.


> +#endif
> +
>  int __init buses_init(void)
>  {
>  	bus_kset = kset_create_and_add("bus", &bus_uevent_ops, NULL);
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 30b0644..3b24a5b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -34,6 +34,10 @@ void register_iommu(struct iommu_ops *ops)
>  	iommu_ops = ops;
>  }
>  
> +void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops)
> +{
> +}
> +
>  bool iommu_found(void)
>  {
>  	return iommu_ops != NULL;
> diff --git a/include/linux/device.h b/include/linux/device.h
> index c20dfbf..8240b2a 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -22,6 +22,7 @@
>  #include <linux/types.h>
>  #include <linux/module.h>
>  #include <linux/pm.h>
> +#include <linux/iommu.h>
>  #include <linux/atomic.h>
>  #include <asm/device.h>
>  
> @@ -67,6 +68,9 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
>   * @resume:	Called to bring a device on this bus out of sleep mode.
>   * @pm:		Power management operations of this bus, callback the specific
>   *		device driver's pm-ops.
> + * @iommu_ops   IOMMU specific operations for this bus, used to attach IOMMU
> + *              driver implementations to a bus and allow the driver to do
> + *              bus-specific setup

So why is this just not set by the bus itself, making the above function
not needed at all?

confused,

greg k-h

Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Joerg Roedel]

Hi Greg,

the bus_set_iommu() function will be called by the IOMMU driver. There
can be different drivers for the same bus, depending on the hardware. On
PCI for example, there can be the Intel or the AMD IOMMU driver that
implement the iommu-api and that register for that bus.

On Wed, Sep 07, 2011 at 11:47:50AM -0700, Greg KH wrote:
> > +#ifdef CONFIG_IOMMU_API
> > +int bus_set_iommu(struct bus_type *bus, struct iommu_ops *ops)
> > +{
> > +	if (bus->iommu_ops != NULL)
> > +		return -EBUSY;
> 
> Busy?

Yes, it signals to the IOMMU driver that another driver has already
registered for that bus. In the previous register_iommu() interface this
was just a BUG(), but I think returning an error to the caller is
better. It can be turned back into a BUG() if it is considered better,
though.

> > +
> > +	bus->iommu_ops = ops;
> > +
> > +	/* Do IOMMU specific setup for this bus-type */
> > +	iommu_bus_init(bus, ops);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(bus_set_iommu);
> 
> I don't understand what this function is for, and who would call it.

It is called by the IOMMU driver.

> Please provide kerneldoc that explains this.

Will do.

> > @@ -67,6 +68,9 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
> >   * @resume:	Called to bring a device on this bus out of sleep mode.
> >   * @pm:		Power management operations of this bus, callback the specific
> >   *		device driver's pm-ops.
> > + * @iommu_ops   IOMMU specific operations for this bus, used to attach IOMMU
> > + *              driver implementations to a bus and allow the driver to do
> > + *              bus-specific setup
> 
> So why is this just not set by the bus itself, making the above function
> not needed at all?

The IOMMUs are usually devices on the bus itself, so they are
initialized after the bus is set up and the devices on it are
populated.  So the function can not be called on bus initialization
because the IOMMU is not ready at this point.

Regards,

	Joerg

Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Greg KH]

On Wed, Sep 07, 2011 at 09:19:19PM +0200, Joerg Roedel wrote:
> Hi Greg,
> 
> the bus_set_iommu() function will be called by the IOMMU driver. There
> can be different drivers for the same bus, depending on the hardware. On
> PCI for example, there can be the Intel or the AMD IOMMU driver that
> implement the iommu-api and that register for that bus.

Why are you pushing this down into the driver core?  What other busses
becides PCI use/need this?

If you can have a different IOMMU driver on the same bus, then wouldn't
this be a per-device thing instead of a per-bus thing?


> On Wed, Sep 07, 2011 at 11:47:50AM -0700, Greg KH wrote:
> > > +#ifdef CONFIG_IOMMU_API
> > > +int bus_set_iommu(struct bus_type *bus, struct iommu_ops *ops)
> > > +{
> > > +	if (bus->iommu_ops != NULL)
> > > +		return -EBUSY;
> > 
> > Busy?
> 
> Yes, it signals to the IOMMU driver that another driver has already
> registered for that bus. In the previous register_iommu() interface this
> was just a BUG(), but I think returning an error to the caller is
> better. It can be turned back into a BUG() if it is considered better,
> though.

Can you ever have more than one IOMMU driver per bus?  If so, this seems
wrong (see above.)

> > > +
> > > +	bus->iommu_ops = ops;
> > > +
> > > +	/* Do IOMMU specific setup for this bus-type */
> > > +	iommu_bus_init(bus, ops);
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(bus_set_iommu);
> > 
> > I don't understand what this function is for, and who would call it.
> 
> It is called by the IOMMU driver.
> 
> > Please provide kerneldoc that explains this.
> 
> Will do.
> 
> > > @@ -67,6 +68,9 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
> > >   * @resume:	Called to bring a device on this bus out of sleep mode.
> > >   * @pm:		Power management operations of this bus, callback the specific
> > >   *		device driver's pm-ops.
> > > + * @iommu_ops   IOMMU specific operations for this bus, used to attach IOMMU
> > > + *              driver implementations to a bus and allow the driver to do
> > > + *              bus-specific setup
> > 
> > So why is this just not set by the bus itself, making the above function
> > not needed at all?
> 
> The IOMMUs are usually devices on the bus itself, so they are
> initialized after the bus is set up and the devices on it are
> populated.  So the function can not be called on bus initialization
> because the IOMMU is not ready at this point.

Ok, that makes more sense, please state as much in the documentation.

thanks,

greg k-h

Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Don Dutile]

On 09/07/2011 03:44 PM, Greg KH wrote:
> On Wed, Sep 07, 2011 at 09:19:19PM +0200, Joerg Roedel wrote:
>> Hi Greg,
>>
>> the bus_set_iommu() function will be called by the IOMMU driver. There
>> can be different drivers for the same bus, depending on the hardware. On
>> PCI for example, there can be the Intel or the AMD IOMMU driver that
>> implement the iommu-api and that register for that bus.
>
> Why are you pushing this down into the driver core?  What other busses
> becides PCI use/need this?
>
> If you can have a different IOMMU driver on the same bus, then wouldn't
> this be a per-device thing instead of a per-bus thing?
>
And given the dma api takes a struct device *, it'd be more efficient
to be tied into the device structure.
Device structure would get iommu ops set by parent(bus);
if a bus (segment) doesn't provide a unique/different/layered IOMMU
then the parent bus, it inherits the parent's iommu-ops.
setting the iommu-ops in the root bus struct, seeds the iommu-ops
for the (PCI) tree.

For intel & amd IOMMUs, in early pci (bios,root?) init, you would
seed the pci root busses with appropriate IOMMU support (based on
dmar/drhd & ivrs/ivhd data structures, respectively), and
then modify the PCI code to do the inheritence (PPB code inherits
unless specific device driver for a given PPB vid-did loads a
different iommu-ops for that segment/branch).

This would enable different types of IOMMUs for different devices
(or PCI segments, or branches of PCI trees) that are designed for
different tasks -- simple IOMMUs for legacy devices; complicated, io-page-faulting
IOMMUs for plug-in, high-end devices on virtualizing servers for PCI (SRIOV) endpoints.

and as Greg indicates, is only relevant to PCI.
The catch is that dev* has to be looked at for iommu support for dma-ops.

>
>> On Wed, Sep 07, 2011 at 11:47:50AM -0700, Greg KH wrote:
>>>> +#ifdef CONFIG_IOMMU_API
>>>> +int bus_set_iommu(struct bus_type *bus, struct iommu_ops *ops)
>>>> +{
>>>> +	if (bus->iommu_ops != NULL)
>>>> +		return -EBUSY;
>>>
>>> Busy?
>>
>> Yes, it signals to the IOMMU driver that another driver has already
>> registered for that bus. In the previous register_iommu() interface this
>> was just a BUG(), but I think returning an error to the caller is
>> better. It can be turned back into a BUG() if it is considered better,
>> though.
>
> Can you ever have more than one IOMMU driver per bus?  If so, this seems
> wrong (see above.)
>
>>>> +
>>>> +	bus->iommu_ops = ops;
>>>> +
>>>> +	/* Do IOMMU specific setup for this bus-type */
>>>> +	iommu_bus_init(bus, ops);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(bus_set_iommu);
>>>
>>> I don't understand what this function is for, and who would call it.
>>
>> It is called by the IOMMU driver.
>>
>>> Please provide kerneldoc that explains this.
>>
>> Will do.
>>
>>>> @@ -67,6 +68,9 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
>>>>    * @resume:	Called to bring a device on this bus out of sleep mode.
>>>>    * @pm:		Power management operations of this bus, callback the specific
>>>>    *		device driver's pm-ops.
>>>> + * @iommu_ops   IOMMU specific operations for this bus, used to attach IOMMU
>>>> + *              driver implementations to a bus and allow the driver to do
>>>> + *              bus-specific setup
>>>
>>> So why is this just not set by the bus itself, making the above function
>>> not needed at all?
>>
>> The IOMMUs are usually devices on the bus itself, so they are
>> initialized after the bus is set up and the devices on it are
>> populated.  So the function can not be called on bus initialization
>> because the IOMMU is not ready at this point.
>
> Ok, that makes more sense, please state as much in the documentation.
>
> thanks,
>
> greg k-h
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/iommu

Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Joerg Roedel]

On Wed, Sep 07, 2011 at 04:37:11PM -0400, Don Dutile wrote:
> On 09/07/2011 03:44 PM, Greg KH wrote:

>> Why are you pushing this down into the driver core?  What other busses
>> becides PCI use/need this?
>>
>> If you can have a different IOMMU driver on the same bus, then wouldn't
>> this be a per-device thing instead of a per-bus thing?
>>
> And given the dma api takes a struct device *, it'd be more efficient
> to be tied into the device structure.

More efficient in what way? Currently no dma-api implementation uses the
iommu-api directly. It is planned to change that, of course. But the
dma-api implementation will use the iommu_map/iommu_unmap functions which
get an iommu_domain parameter and no device structure. Thats why the
domain structure keeps an iommu_ops pointer too.

> Device structure would get iommu ops set by parent(bus);
> if a bus (segment) doesn't provide a unique/different/layered IOMMU
> then the parent bus, it inherits the parent's iommu-ops.
> setting the iommu-ops in the root bus struct, seeds the iommu-ops
> for the (PCI) tree.
>
> For intel & amd IOMMUs, in early pci (bios,root?) init, you would
> seed the pci root busses with appropriate IOMMU support (based on
> dmar/drhd & ivrs/ivhd data structures, respectively), and
> then modify the PCI code to do the inheritence (PPB code inherits
> unless specific device driver for a given PPB vid-did loads a
> different iommu-ops for that segment/branch).
>
> This would enable different types of IOMMUs for different devices
> (or PCI segments, or branches of PCI trees) that are designed for
> different tasks -- simple IOMMUs for legacy devices; complicated, io-page-faulting
> IOMMUs for plug-in, high-end devices on virtualizing servers for PCI (SRIOV) endpoints.

This only works for pci, but the iommu-api is not pci-only.

> and as Greg indicates, is only relevant to PCI.

We already have non-pci iommu drivers implementing the iommu-api. So
this is certainly relevant outside pci too.

	Joerg

Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Joerg Roedel]

On Wed, Sep 07, 2011 at 12:44:45PM -0700, Greg KH wrote:
> On Wed, Sep 07, 2011 at 09:19:19PM +0200, Joerg Roedel wrote:
> > Hi Greg,
> > 
> > the bus_set_iommu() function will be called by the IOMMU driver. There
> > can be different drivers for the same bus, depending on the hardware. On
> > PCI for example, there can be the Intel or the AMD IOMMU driver that
> > implement the iommu-api and that register for that bus.
> 
> Why are you pushing this down into the driver core?  What other busses
> becides PCI use/need this?

Currently it is the platform_bus besides pci. The pci iommus are on x86
and ia64 while all arm iommus use the platform_bus (by 'iommus' I only
mean those implementing the iommu-api). Currently there are two drivers
for arm iommus in /drivers/iommu.

> If you can have a different IOMMU driver on the same bus, then wouldn't
> this be a per-device thing instead of a per-bus thing?

Well, I havn't seen a system yet where multiple iommus are on the same
bus. Or to state it better, multiple iommus of different type that
require different drivers. There is no 1-1 mapping between real hardware
iommus and iommu_ops. There is only such a mapping for iommu drivers and
iommu_ops. An iommu driver usually handles all hardware iommus of the
same type in the system.

So having iommu_ops per-device doesn't make much sense at this point.
With this patch-set they are accessible by dev->bus->iommu_ops anyway.
But if I am wrong on this I can change this of course.

This patch-set improves the current situation where only on active
iommu-driver is allowed to be active on a system (because of the global
iommu_ops). But the main reason to put this into the bus_type structure
is that it allows to generalize the device-handling on a bus between
iommu drivers.


> 
> 
> > On Wed, Sep 07, 2011 at 11:47:50AM -0700, Greg KH wrote:
> > > > +#ifdef CONFIG_IOMMU_API
> > > > +int bus_set_iommu(struct bus_type *bus, struct iommu_ops *ops)
> > > > +{
> > > > +	if (bus->iommu_ops != NULL)
> > > > +		return -EBUSY;
> > > 
> > > Busy?
> > 
> > Yes, it signals to the IOMMU driver that another driver has already
> > registered for that bus. In the previous register_iommu() interface this
> > was just a BUG(), but I think returning an error to the caller is
> > better. It can be turned back into a BUG() if it is considered better,
> > though.
> 
> Can you ever have more than one IOMMU driver per bus?  If so, this seems
> wrong (see above.)

As I said, I havn't seen such systems. But if they exist or are planned
I am happy to redesign the whole thing.

> > The IOMMUs are usually devices on the bus itself, so they are
> > initialized after the bus is set up and the devices on it are
> > populated.  So the function can not be called on bus initialization
> > because the IOMMU is not ready at this point.
> 
> Ok, that makes more sense, please state as much in the documentation.

Will do, thanks.


	Joerg

RE: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Sethi Varun-B16395]

> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On
> Behalf Of Joerg Roedel
> Sent: Thursday, September 08, 2011 2:12 PM
> To: Greg KH
> Cc: Joerg Roedel; iommu@lists.linux-foundation.org; Alex Williamson; Ohad
> Ben-Cohen; David Woodhouse; David Brown; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type
> 
> On Wed, Sep 07, 2011 at 12:44:45PM -0700, Greg KH wrote:
> > On Wed, Sep 07, 2011 at 09:19:19PM +0200, Joerg Roedel wrote:
> > > Hi Greg,
> > >
> > > the bus_set_iommu() function will be called by the IOMMU driver.
> > > There can be different drivers for the same bus, depending on the
> > > hardware. On PCI for example, there can be the Intel or the AMD
> > > IOMMU driver that implement the iommu-api and that register for that
> bus.
> >
> > Why are you pushing this down into the driver core?  What other busses
> > becides PCI use/need this?
> 
> Currently it is the platform_bus besides pci. The pci iommus are on x86
> and ia64 while all arm iommus use the platform_bus (by 'iommus' I only
> mean those implementing the iommu-api). Currently there are two drivers
> for arm iommus in /drivers/iommu.
> 
We too have an IOMMU on Freescale Socs (Power core based) sitting on the platform bus
and are looking at integrating it with the linux IOMMU API.

-Varun

Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Roedel, Joerg]

Hi Greg,

On Wed, Sep 07, 2011 at 03:44:45PM -0400, Greg KH wrote:
> > 
> > The IOMMUs are usually devices on the bus itself, so they are
> > initialized after the bus is set up and the devices on it are
> > populated.  So the function can not be called on bus initialization
> > because the IOMMU is not ready at this point.
> 
> Ok, that makes more sense, please state as much in the documentation.

I added the kernel-doc documentation you requested. Here is the updated
patch. Please let me know what you think.

>From 23757825035f6d9309866dc142133aada0e2c1de Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joerg.roedel@amd.com>
Date: Fri, 26 Aug 2011 16:48:26 +0200
Subject: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

This is the starting point to make the iommu_ops used for
the iommu-api a per-bus-type structure. It is required to
easily implement bus-specific setup in the iommu-layer.
The first user will be the iommu-group attribute in sysfs.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/base/bus.c     |   29 +++++++++++++++++++++++++++++
 drivers/iommu/iommu.c  |    4 ++++
 include/linux/device.h |    9 +++++++++
 include/linux/iommu.h  |    2 ++
 4 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 000e7b2..b3014fe 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -1028,6 +1028,35 @@ void bus_sort_breadthfirst(struct bus_type *bus,
 }
 EXPORT_SYMBOL_GPL(bus_sort_breadthfirst);
 
+#ifdef CONFIG_IOMMU_API
+/**
+ * bus_set_iommu - set iommu-callbacks for the bus
+ * @bus: bus.
+ * @ops: the callbacks provided by the iommu-driver
+ *
+ * This function is called by an iommu driver to set the iommu methods
+ * used for a particular bus. Drivers for devices on that bus can use
+ * the iommu-api after these ops are registered.
+ * This special function is needed because IOMMUs are usually devices on
+ * the bus itself, so the iommu drivers are not initialized when the bus
+ * is set up. With this function the iommu-driver can set the iommu-ops
+ * afterwards.
+ */
+int bus_set_iommu(struct bus_type *bus, struct iommu_ops *ops)
+{
+	if (bus->iommu_ops != NULL)
+		return -EBUSY;
+
+	bus->iommu_ops = ops;
+
+	/* Do IOMMU specific setup for this bus-type */
+	iommu_bus_init(bus, ops);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(bus_set_iommu);
+#endif
+
 int __init buses_init(void)
 {
 	bus_kset = kset_create_and_add("bus", &bus_uevent_ops, NULL);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 30b0644..3b24a5b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -34,6 +34,10 @@ void register_iommu(struct iommu_ops *ops)
 	iommu_ops = ops;
 }
 
+void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops)
+{
+}
+
 bool iommu_found(void)
 {
 	return iommu_ops != NULL;
diff --git a/include/linux/device.h b/include/linux/device.h
index c20dfbf..8240b2a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -22,6 +22,7 @@
 #include <linux/types.h>
 #include <linux/module.h>
 #include <linux/pm.h>
+#include <linux/iommu.h>
 #include <linux/atomic.h>
 #include <asm/device.h>
 
@@ -67,6 +68,9 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  * @resume:	Called to bring a device on this bus out of sleep mode.
  * @pm:		Power management operations of this bus, callback the specific
  *		device driver's pm-ops.
+ * @iommu_ops   IOMMU specific operations for this bus, used to attach IOMMU
+ *              driver implementations to a bus and allow the driver to do
+ *              bus-specific setup
  * @p:		The private data of the driver core, only the driver core can
  *		touch this.
  *
@@ -96,6 +100,8 @@ struct bus_type {
 
 	const struct dev_pm_ops *pm;
 
+	struct iommu_ops *iommu_ops;
+
 	struct subsys_private *p;
 };
 
@@ -148,6 +154,9 @@ extern int bus_unregister_notifier(struct bus_type *bus,
 #define BUS_NOTIFY_UNBOUND_DRIVER	0x00000006 /* driver is unbound
 						      from the device */
 
+/* IOMMU related bus functions */
+int bus_set_iommu(struct bus_type *bus, struct iommu_ops *ops);
+
 extern struct kset *bus_get_kset(struct bus_type *bus);
 extern struct klist *bus_get_device_klist(struct bus_type *bus);
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 6470cd8..4739e36 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -25,6 +25,7 @@
 #define IOMMU_WRITE	(2)
 #define IOMMU_CACHE	(4) /* DMA cache coherency */
 
+struct bus_type;
 struct device;
 
 struct iommu_domain {
@@ -52,6 +53,7 @@ struct iommu_ops {
 };
 
 extern void register_iommu(struct iommu_ops *ops);
+extern void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops);
 extern bool iommu_found(void);
 extern struct iommu_domain *iommu_domain_alloc(void);
 extern void iommu_domain_free(struct iommu_domain *domain);
-- 
1.7.4.1

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Greg KH]

On Tue, Sep 13, 2011 at 04:54:02PM +0200, Roedel, Joerg wrote:
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -22,6 +22,7 @@
>  #include <linux/types.h>
>  #include <linux/module.h>
>  #include <linux/pm.h>
> +#include <linux/iommu.h>

Ick, please don't add new #includes to device.h, it makes the whole
build slower.  Just pre-declare the structure and all should be fine.

Don't you need to also redo the other patches in this series based on
the other comments you received?

greg k-h

Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Roedel, Joerg]

On Tue, Sep 13, 2011 at 10:58:55AM -0400, Greg KH wrote:
> On Tue, Sep 13, 2011 at 04:54:02PM +0200, Roedel, Joerg wrote:
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -22,6 +22,7 @@
> >  #include <linux/types.h>
> >  #include <linux/module.h>
> >  #include <linux/pm.h>
> > +#include <linux/iommu.h>
> 
> Ick, please don't add new #includes to device.h, it makes the whole
> build slower.  Just pre-declare the structure and all should be fine.

Okay, will do. Thanks.

> Don't you need to also redo the other patches in this series based on
> the other comments you received?

Unless I have missed something I am not aware of other comments that
need action by now.
Sethi pointed out that the KVM device assignment code needs rework to be
usable outside of PCI too. But that work is already underway with Alex
Williamson's VFIO framework and not scope of this patch-set.

Regards,

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Roedel, Joerg]

On Tue, Sep 13, 2011 at 10:58:55AM -0400, Greg KH wrote:
> On Tue, Sep 13, 2011 at 04:54:02PM +0200, Roedel, Joerg wrote:
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -22,6 +22,7 @@
> >  #include <linux/types.h>
> >  #include <linux/module.h>
> >  #include <linux/pm.h>
> > +#include <linux/iommu.h>
> 
> Ick, please don't add new #includes to device.h, it makes the whole
> build slower.  Just pre-declare the structure and all should be fine.

Hmm, since linux/iommu.h provides 'struct iommu_ops', and this patch
adds a 'struct iommu_ops' to 'struct bus_type', wouldn't a simple
forward declaration make the bus_type incomplete in most other places?
To lower the impact I can move the 'struct iommu_ops' to a seperate
header file instead.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Greg KH]

On Tue, Sep 13, 2011 at 05:38:11PM +0200, Roedel, Joerg wrote:
> On Tue, Sep 13, 2011 at 10:58:55AM -0400, Greg KH wrote:
> > On Tue, Sep 13, 2011 at 04:54:02PM +0200, Roedel, Joerg wrote:
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -22,6 +22,7 @@
> > >  #include <linux/types.h>
> > >  #include <linux/module.h>
> > >  #include <linux/pm.h>
> > > +#include <linux/iommu.h>
> > 
> > Ick, please don't add new #includes to device.h, it makes the whole
> > build slower.  Just pre-declare the structure and all should be fine.
> 
> Hmm, since linux/iommu.h provides 'struct iommu_ops', and this patch
> adds a 'struct iommu_ops' to 'struct bus_type', wouldn't a simple
> forward declaration make the bus_type incomplete in most other places?

No, just like it doesn't make iommu.h incomplete as you used a struct
bus_type there.

> To lower the impact I can move the 'struct iommu_ops' to a seperate
> header file instead.

No, that should not be necessary.

greg k-h

Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Roedel, Joerg]

On Tue, Sep 13, 2011 at 12:21:26PM -0400, Greg KH wrote:
> On Tue, Sep 13, 2011 at 05:38:11PM +0200, Roedel, Joerg wrote:
> > On Tue, Sep 13, 2011 at 10:58:55AM -0400, Greg KH wrote:
> > > On Tue, Sep 13, 2011 at 04:54:02PM +0200, Roedel, Joerg wrote:
> > > > --- a/include/linux/device.h
> > > > +++ b/include/linux/device.h
> > > > @@ -22,6 +22,7 @@
> > > >  #include <linux/types.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/pm.h>
> > > > +#include <linux/iommu.h>
> > > 
> > > Ick, please don't add new #includes to device.h, it makes the whole
> > > build slower.  Just pre-declare the structure and all should be fine.
> > 
> > Hmm, since linux/iommu.h provides 'struct iommu_ops', and this patch
> > adds a 'struct iommu_ops' to 'struct bus_type', wouldn't a simple
> > forward declaration make the bus_type incomplete in most other places?
> 
> No, just like it doesn't make iommu.h incomplete as you used a struct
> bus_type there.

Ah right, because bus->iommu_ops is just a pointer the full type
definition for iommu_ops is only needed when this pointer is actually
dereferenced. I updated the patch. Please find it below.


>From 6e0e1c3b997e06539f7bda80f46ffe9fb04aab4e Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joerg.roedel@amd.com>
Date: Fri, 26 Aug 2011 16:48:26 +0200
Subject: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

This is the starting point to make the iommu_ops used for
the iommu-api a per-bus-type structure. It is required to
easily implement bus-specific setup in the iommu-layer.
The first user will be the iommu-group attribute in sysfs.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/base/bus.c     |   29 +++++++++++++++++++++++++++++
 drivers/iommu/iommu.c  |    4 ++++
 include/linux/device.h |   10 ++++++++++
 include/linux/iommu.h  |    2 ++
 4 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 000e7b2..b3014fe 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -1028,6 +1028,35 @@ void bus_sort_breadthfirst(struct bus_type *bus,
 }
 EXPORT_SYMBOL_GPL(bus_sort_breadthfirst);
 
+#ifdef CONFIG_IOMMU_API
+/**
+ * bus_set_iommu - set iommu-callbacks for the bus
+ * @bus: bus.
+ * @ops: the callbacks provided by the iommu-driver
+ *
+ * This function is called by an iommu driver to set the iommu methods
+ * used for a particular bus. Drivers for devices on that bus can use
+ * the iommu-api after these ops are registered.
+ * This special function is needed because IOMMUs are usually devices on
+ * the bus itself, so the iommu drivers are not initialized when the bus
+ * is set up. With this function the iommu-driver can set the iommu-ops
+ * afterwards.
+ */
+int bus_set_iommu(struct bus_type *bus, struct iommu_ops *ops)
+{
+	if (bus->iommu_ops != NULL)
+		return -EBUSY;
+
+	bus->iommu_ops = ops;
+
+	/* Do IOMMU specific setup for this bus-type */
+	iommu_bus_init(bus, ops);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(bus_set_iommu);
+#endif
+
 int __init buses_init(void)
 {
 	bus_kset = kset_create_and_add("bus", &bus_uevent_ops, NULL);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 30b0644..3b24a5b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -34,6 +34,10 @@ void register_iommu(struct iommu_ops *ops)
 	iommu_ops = ops;
 }
 
+void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops)
+{
+}
+
 bool iommu_found(void)
 {
 	return iommu_ops != NULL;
diff --git a/include/linux/device.h b/include/linux/device.h
index c20dfbf..490382b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -33,6 +33,7 @@ struct class;
 struct subsys_private;
 struct bus_type;
 struct device_node;
+struct iommu_ops;
 
 struct bus_attribute {
 	struct attribute	attr;
@@ -46,6 +47,7 @@ struct bus_attribute bus_attr_##_name = __ATTR(_name, _mode, _show, _store)
 extern int __must_check bus_create_file(struct bus_type *,
 					struct bus_attribute *);
 extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
+extern void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops);
 
 /**
  * struct bus_type - The bus type of the device
@@ -67,6 +69,9 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  * @resume:	Called to bring a device on this bus out of sleep mode.
  * @pm:		Power management operations of this bus, callback the specific
  *		device driver's pm-ops.
+ * @iommu_ops   IOMMU specific operations for this bus, used to attach IOMMU
+ *              driver implementations to a bus and allow the driver to do
+ *              bus-specific setup
  * @p:		The private data of the driver core, only the driver core can
  *		touch this.
  *
@@ -96,6 +101,8 @@ struct bus_type {
 
 	const struct dev_pm_ops *pm;
 
+	struct iommu_ops *iommu_ops;
+
 	struct subsys_private *p;
 };
 
@@ -148,6 +155,9 @@ extern int bus_unregister_notifier(struct bus_type *bus,
 #define BUS_NOTIFY_UNBOUND_DRIVER	0x00000006 /* driver is unbound
 						      from the device */
 
+/* IOMMU related bus functions */
+int bus_set_iommu(struct bus_type *bus, struct iommu_ops *ops);
+
 extern struct kset *bus_get_kset(struct bus_type *bus);
 extern struct klist *bus_get_device_klist(struct bus_type *bus);
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 6470cd8..4739e36 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -25,6 +25,7 @@
 #define IOMMU_WRITE	(2)
 #define IOMMU_CACHE	(4) /* DMA cache coherency */
 
+struct bus_type;
 struct device;
 
 struct iommu_domain {
@@ -52,6 +53,7 @@ struct iommu_ops {
 };
 
 extern void register_iommu(struct iommu_ops *ops);
+extern void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops);
 extern bool iommu_found(void);
 extern struct iommu_domain *iommu_domain_alloc(void);
 extern void iommu_domain_free(struct iommu_domain *domain);
-- 
1.7.4.1


-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

RE: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Sethi Varun-B16395]

> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On
> Behalf Of Joerg Roedel
> Sent: Thursday, September 08, 2011 12:49 AM
> To: Greg KH
> Cc: Joerg Roedel; iommu@lists.linux-foundation.org; Alex Williamson; Ohad
> Ben-Cohen; David Woodhouse; David Brown; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type
> 
> Hi Greg,
> 
> the bus_set_iommu() function will be called by the IOMMU driver. There
> can be different drivers for the same bus, depending on the hardware. On
> PCI for example, there can be the Intel or the AMD IOMMU driver that
> implement the iommu-api and that register for that bus.
> 
> On Wed, Sep 07, 2011 at 11:47:50AM -0700, Greg KH wrote:
> > > +#ifdef CONFIG_IOMMU_API
> > > +int bus_set_iommu(struct bus_type *bus, struct iommu_ops *ops) {
> > > +	if (bus->iommu_ops != NULL)
> > > +		return -EBUSY;
> >
> > Busy?
> 
> Yes, it signals to the IOMMU driver that another driver has already
> registered for that bus. In the previous register_iommu() interface this
> was just a BUG(), but I think returning an error to the caller is better.
> It can be turned back into a BUG() if it is considered better, though.
> 
> > > +
> > > +	bus->iommu_ops = ops;
> > > +
> > > +	/* Do IOMMU specific setup for this bus-type */
> > > +	iommu_bus_init(bus, ops);
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(bus_set_iommu);
> >
> > I don't understand what this function is for, and who would call it.
> 
> It is called by the IOMMU driver.
> 
> > Please provide kerneldoc that explains this.
> 
> Will do.
> 
> > > @@ -67,6 +68,9 @@ extern void bus_remove_file(struct bus_type *,
> struct bus_attribute *);
> > >   * @resume:	Called to bring a device on this bus out of sleep mode.
> > >   * @pm:		Power management operations of this bus, callback
> the specific
> > >   *		device driver's pm-ops.
> > > + * @iommu_ops   IOMMU specific operations for this bus, used to
> attach IOMMU
> > > + *              driver implementations to a bus and allow the driver
> to do
> > > + *              bus-specific setup
> >
> > So why is this just not set by the bus itself, making the above
> > function not needed at all?
> 
> The IOMMUs are usually devices on the bus itself, so they are initialized
> after the bus is set up and the devices on it are populated.  So the
> function can not be called on bus initialization because the IOMMU is not
> ready at this point.
Well, at what point would the add_device_group (referring to patch set posted by Alex) call back be invoked?

-Varun

Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Roedel, Joerg]

On Mon, Sep 12, 2011 at 08:08:41AM -0400, Sethi Varun-B16395 wrote:
> > The IOMMUs are usually devices on the bus itself, so they are initialized
> > after the bus is set up and the devices on it are populated.  So the
> > function can not be called on bus initialization because the IOMMU is not
> > ready at this point.
> Well, at what point would the add_device_group (referring to patch set posted by Alex) call back be invoked?

The details are up to Alex Williamson. One option is to register a
notifier for the bus in the iommu_bus_init() function and react to its
notifications.
I think in the end we will have a number of additional call-backs in the
iommu_ops which are called by the notifier (or from the driver-core
directly) to handle actions like added or removed devices. All the
infrastructure for that which is implemented in the iommu-drivers today
will then be in the iommu-core code.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

RE: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Sethi Varun-B16395]

> -----Original Message-----
> From: Roedel, Joerg [mailto:Joerg.Roedel@amd.com]
> Sent: Monday, September 12, 2011 6:06 PM
> To: Sethi Varun-B16395
> Cc: Joerg Roedel; Greg KH; iommu@lists.linux-foundation.org; Alex
> Williamson; Ohad Ben-Cohen; David Woodhouse; David Brown;
> kvm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type
> 
> On Mon, Sep 12, 2011 at 08:08:41AM -0400, Sethi Varun-B16395 wrote:
> > > The IOMMUs are usually devices on the bus itself, so they are
> > > initialized after the bus is set up and the devices on it are
> > > populated.  So the function can not be called on bus initialization
> > > because the IOMMU is not ready at this point.
> > Well, at what point would the add_device_group (referring to patch set
> posted by Alex) call back be invoked?
> 
> The details are up to Alex Williamson. One option is to register a
> notifier for the bus in the iommu_bus_init() function and react to its
> notifications.
> I think in the end we will have a number of additional call-backs in the
> iommu_ops which are called by the notifier (or from the driver-core
> directly) to handle actions like added or removed devices. All the
> infrastructure for that which is implemented in the iommu-drivers today
> will then be in the iommu-core code.
I am not sure If I understand this, but as per your earlier statement iommu is a device on the bus
and its initialization would happen when bus is set up and devices are populated. So, when would device
notifier call an iommu call back?

-Varun
 

Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Roedel, Joerg]

On Thu, Sep 15, 2011 at 08:45:35AM -0400, Sethi Varun-B16395 wrote:
> > From: Roedel, Joerg [mailto:Joerg.Roedel@amd.com]
> > The details are up to Alex Williamson. One option is to register a
> > notifier for the bus in the iommu_bus_init() function and react to its
> > notifications.
> > I think in the end we will have a number of additional call-backs in the
> > iommu_ops which are called by the notifier (or from the driver-core
> > directly) to handle actions like added or removed devices. All the
> > infrastructure for that which is implemented in the iommu-drivers today
> > will then be in the iommu-core code.

> I am not sure If I understand this, but as per your earlier statement
> iommu is a device on the bus and its initialization would happen when
> bus is set up and devices are populated. So, when would device
> notifier call an iommu call back?

This is done in the iommu_bus_init() function. It will iterate over all
devices that are already on the bus and do the iommu specific
initialization on them. The devices added or removed later the notifier
will do the job.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH 03/10] iommu/core: Add bus_type parameter to iommu_domain_alloc

[Joerg Roedel]

This is necessary to store a pointer to the bus-specific
iommu_ops in the iommu-domain structure. It will be used
later to call into bus-specific iommu-ops.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/iommu/iommu.c              |   14 +++++++++++++-
 drivers/media/video/omap3isp/isp.c |    2 +-
 include/linux/iommu.h              |    6 ++++--
 virt/kvm/iommu.c                   |    2 +-
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3b24a5b..adaee9b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -16,6 +16,7 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
  */
 
+#include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/bug.h>
 #include <linux/types.h>
@@ -44,15 +45,26 @@ bool iommu_found(void)
 }
 EXPORT_SYMBOL_GPL(iommu_found);
 
-struct iommu_domain *iommu_domain_alloc(void)
+struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
 {
 	struct iommu_domain *domain;
+	struct iommu_ops *ops;
 	int ret;
 
+	if (bus->iommu_ops)
+		ops = bus->iommu_ops;
+	else
+		ops = iommu_ops;
+
+	if (ops == NULL)
+		return NULL;
+
 	domain = kmalloc(sizeof(*domain), GFP_KERNEL);
 	if (!domain)
 		return NULL;
 
+	domain->ops = ops;
+
 	ret = iommu_ops->domain_init(domain);
 	if (ret)
 		goto out_free;
diff --git a/drivers/media/video/omap3isp/isp.c b/drivers/media/video/omap3isp/isp.c
index a4baa61..a7ed985 100644
--- a/drivers/media/video/omap3isp/isp.c
+++ b/drivers/media/video/omap3isp/isp.c
@@ -2141,7 +2141,7 @@ static int isp_probe(struct platform_device *pdev)
 	/* to be removed once iommu migration is complete */
 	isp->iommu = to_iommu(isp->iommu_dev);
 
-	isp->domain = iommu_domain_alloc();
+	isp->domain = iommu_domain_alloc(pdev->dev.bus);
 	if (!isp->domain) {
 		dev_err(isp->dev, "can't alloc iommu domain\n");
 		ret = -ENOMEM;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 4739e36..3bd6892 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -25,10 +25,12 @@
 #define IOMMU_WRITE	(2)
 #define IOMMU_CACHE	(4) /* DMA cache coherency */
 
+struct iommu_ops;
 struct bus_type;
 struct device;
 
 struct iommu_domain {
+	struct iommu_ops *ops;
 	void *priv;
 };
 
@@ -55,7 +57,7 @@ struct iommu_ops {
 extern void register_iommu(struct iommu_ops *ops);
 extern void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops);
 extern bool iommu_found(void);
-extern struct iommu_domain *iommu_domain_alloc(void);
+extern struct iommu_domain *iommu_domain_alloc(struct bus_type *bus);
 extern void iommu_domain_free(struct iommu_domain *domain);
 extern int iommu_attach_device(struct iommu_domain *domain,
 			       struct device *dev);
@@ -79,7 +81,7 @@ static inline bool iommu_found(void)
 	return false;
 }
 
-static inline struct iommu_domain *iommu_domain_alloc(void)
+static inline struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
 {
 	return NULL;
 }
diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
index 78c80f6..20115b1 100644
--- a/virt/kvm/iommu.c
+++ b/virt/kvm/iommu.c
@@ -233,7 +233,7 @@ int kvm_iommu_map_guest(struct kvm *kvm)
 		return -ENODEV;
 	}
 
-	kvm->arch.iommu_domain = iommu_domain_alloc();
+	kvm->arch.iommu_domain = iommu_domain_alloc(&pci_bus_type);
 	if (!kvm->arch.iommu_domain)
 		return -ENOMEM;
 
-- 
1.7.4.1

RE: [PATCH 03/10] iommu/core: Add bus_type parameter to iommu_domain_alloc

[Sethi Varun-B16395]

> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On
> Behalf Of Joerg Roedel
> Sent: Wednesday, September 07, 2011 9:12 PM
> To: iommu@lists.linux-foundation.org
> Cc: Greg Kroah-Hartman; Alex Williamson; Ohad Ben-Cohen; David Woodhouse;
> David Brown; joro@8bytes.org; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; Joerg Roedel
> Subject: [PATCH 03/10] iommu/core: Add bus_type parameter to
> iommu_domain_alloc
> 
> This is necessary to store a pointer to the bus-specific iommu_ops in the
> iommu-domain structure. It will be used later to call into bus-specific
> iommu-ops.
> 
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  drivers/iommu/iommu.c              |   14 +++++++++++++-
>  drivers/media/video/omap3isp/isp.c |    2 +-
>  include/linux/iommu.h              |    6 ++++--
>  virt/kvm/iommu.c                   |    2 +-
>  4 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
> 3b24a5b..adaee9b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -16,6 +16,7 @@
>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> USA
>   */
> 
> +#include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/bug.h>
>  #include <linux/types.h>
> @@ -44,15 +45,26 @@ bool iommu_found(void)  }
> EXPORT_SYMBOL_GPL(iommu_found);
> 
> -struct iommu_domain *iommu_domain_alloc(void)
> +struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
>  {
>  	struct iommu_domain *domain;
> +	struct iommu_ops *ops;
>  	int ret;
> 
> +	if (bus->iommu_ops)
> +		ops = bus->iommu_ops;
> +	else
> +		ops = iommu_ops;
> +
> +	if (ops == NULL)
> +		return NULL;
> +
>  	domain = kmalloc(sizeof(*domain), GFP_KERNEL);
>  	if (!domain)
>  		return NULL;
> 
> +	domain->ops = ops;
> +
>  	ret = iommu_ops->domain_init(domain);
>  	if (ret)
>  		goto out_free;
> diff --git a/drivers/media/video/omap3isp/isp.c
> b/drivers/media/video/omap3isp/isp.c
> index a4baa61..a7ed985 100644
> --- a/drivers/media/video/omap3isp/isp.c
> +++ b/drivers/media/video/omap3isp/isp.c
> @@ -2141,7 +2141,7 @@ static int isp_probe(struct platform_device *pdev)
>  	/* to be removed once iommu migration is complete */
>  	isp->iommu = to_iommu(isp->iommu_dev);
> 
> -	isp->domain = iommu_domain_alloc();
> +	isp->domain = iommu_domain_alloc(pdev->dev.bus);
>  	if (!isp->domain) {
>  		dev_err(isp->dev, "can't alloc iommu domain\n");
>  		ret = -ENOMEM;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
> 4739e36..3bd6892 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -25,10 +25,12 @@
>  #define IOMMU_WRITE	(2)
>  #define IOMMU_CACHE	(4) /* DMA cache coherency */
> 
> +struct iommu_ops;
>  struct bus_type;
>  struct device;
> 
>  struct iommu_domain {
> +	struct iommu_ops *ops;
>  	void *priv;
>  };
> 
> @@ -55,7 +57,7 @@ struct iommu_ops {
>  extern void register_iommu(struct iommu_ops *ops);  extern void
> iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops);  extern bool
> iommu_found(void); -extern struct iommu_domain *iommu_domain_alloc(void);
> +extern struct iommu_domain *iommu_domain_alloc(struct bus_type *bus);
>  extern void iommu_domain_free(struct iommu_domain *domain);  extern int
> iommu_attach_device(struct iommu_domain *domain,
>  			       struct device *dev);
> @@ -79,7 +81,7 @@ static inline bool iommu_found(void)
>  	return false;
>  }
> 
> -static inline struct iommu_domain *iommu_domain_alloc(void)
> +static inline struct iommu_domain *iommu_domain_alloc(struct bus_type
> +*bus)
>  {
>  	return NULL;
>  }
> diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c index 78c80f6..20115b1
> 100644
> --- a/virt/kvm/iommu.c
> +++ b/virt/kvm/iommu.c
> @@ -233,7 +233,7 @@ int kvm_iommu_map_guest(struct kvm *kvm)
>  		return -ENODEV;
>  	}
> 
> -	kvm->arch.iommu_domain = iommu_domain_alloc();
> +	kvm->arch.iommu_domain = iommu_domain_alloc(&pci_bus_type);

Although it might require changes starting all the way from the qemu interface, but
it would certainly be nice if this could be made more extendable/generic in terms
of the bus_type usage. This interface would be used by us (Freescale )for direct
assignment of SOC devices sitting on the platform bus.

-Varun

Re: [PATCH 03/10] iommu/core: Add bus_type parameter to iommu_domain_alloc

[Roedel, Joerg]

On Mon, Sep 12, 2011 at 07:50:35AM -0400, Sethi Varun-B16395 wrote:
> > -	kvm->arch.iommu_domain = iommu_domain_alloc();
> > +	kvm->arch.iommu_domain = iommu_domain_alloc(&pci_bus_type);
> 
> Although it might require changes starting all the way from the qemu interface, but
> it would certainly be nice if this could be made more extendable/generic in terms
> of the bus_type usage. This interface would be used by us (Freescale )for direct
> assignment of SOC devices sitting on the platform bus.

Yeah, you are right on this one. Alex is working on a better and more
generic solution for kvm device passthrough (vfio). This patch only
changes the existing code which is pci-only, though.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH 04/10] iommu/core: Convert iommu_found to iommu_present

[Joerg Roedel]

With per-bus iommu_ops the iommu_found function needs to
work on a bus_type too. This patch adds a bus_type parameter
to that function and converts all call-places.
The function is also renamed to iommu_present because the
function now checks if an iommu is present for a given bus
and does not check for a global iommu anymore.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/ia64/kvm/kvm-ia64.c |    3 ++-
 arch/x86/kvm/x86.c       |    3 ++-
 drivers/iommu/iommu.c    |    9 ++++++---
 include/linux/iommu.h    |    4 ++--
 virt/kvm/iommu.c         |    2 +-
 5 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 8213efe..43f4c92 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -33,6 +33,7 @@
 #include <linux/uaccess.h>
 #include <linux/iommu.h>
 #include <linux/intel-iommu.h>
+#include <linux/pci.h>
 
 #include <asm/pgtable.h>
 #include <asm/gcc_intrin.h>
@@ -204,7 +205,7 @@ int kvm_dev_ioctl_check_extension(long ext)
 		r = KVM_COALESCED_MMIO_PAGE_OFFSET;
 		break;
 	case KVM_CAP_IOMMU:
-		r = iommu_found();
+		r = iommu_present(&pci_bus_type);
 		break;
 	default:
 		r = 0;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 84a28ea..73c6a42 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -44,6 +44,7 @@
 #include <linux/perf_event.h>
 #include <linux/uaccess.h>
 #include <linux/hash.h>
+#include <linux/pci.h>
 #include <trace/events/kvm.h>
 
 #define CREATE_TRACE_POINTS
@@ -2095,7 +2096,7 @@ int kvm_dev_ioctl_check_extension(long ext)
 		r = 0;
 		break;
 	case KVM_CAP_IOMMU:
-		r = iommu_found();
+		r = iommu_present(&pci_bus_type);
 		break;
 	case KVM_CAP_MCE:
 		r = KVM_MAX_MCE_BANKS;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index adaee9b..2270127 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -39,11 +39,14 @@ void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops)
 {
 }
 
-bool iommu_found(void)
+bool iommu_present(struct bus_type *bus)
 {
-	return iommu_ops != NULL;
+	if (bus->iommu_ops != NULL)
+		return true;
+	else
+		return iommu_ops != NULL;
 }
-EXPORT_SYMBOL_GPL(iommu_found);
+EXPORT_SYMBOL_GPL(iommu_present);
 
 struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
 {
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 3bd6892..de73219 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -56,7 +56,7 @@ struct iommu_ops {
 
 extern void register_iommu(struct iommu_ops *ops);
 extern void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops);
-extern bool iommu_found(void);
+extern bool iommu_present(struct bus_type *bus);
 extern struct iommu_domain *iommu_domain_alloc(struct bus_type *bus);
 extern void iommu_domain_free(struct iommu_domain *domain);
 extern int iommu_attach_device(struct iommu_domain *domain,
@@ -76,7 +76,7 @@ extern int iommu_domain_has_cap(struct iommu_domain *domain,
 
 struct iommu_ops {};
 
-static inline bool iommu_found(void)
+static inline bool iommu_present(struct bus_type *bus)
 {
 	return false;
 }
diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
index 20115b1..d149940 100644
--- a/virt/kvm/iommu.c
+++ b/virt/kvm/iommu.c
@@ -228,7 +228,7 @@ int kvm_iommu_map_guest(struct kvm *kvm)
 {
 	int r;
 
-	if (!iommu_found()) {
+	if (!iommu_present(&pci_bus_type)) {
 		printk(KERN_ERR "%s: iommu not found\n", __func__);
 		return -ENODEV;
 	}
-- 
1.7.4.1

[PATCH 05/10] iommu/core: Use bus->iommu_ops in the iommu-api

[Joerg Roedel]

Use the per-bus iommu-ops in the functions of the iommu-api
instead of the global iommu_ops.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/iommu/iommu.c |   34 +++++++++++++++++++++++++++-------
 1 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 2270127..f2ced4c 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -83,34 +83,48 @@ EXPORT_SYMBOL_GPL(iommu_domain_alloc);
 
 void iommu_domain_free(struct iommu_domain *domain)
 {
-	iommu_ops->domain_destroy(domain);
+	if (likely(domain->ops->domain_destroy != NULL))
+		domain->ops->domain_destroy(domain);
+
 	kfree(domain);
 }
 EXPORT_SYMBOL_GPL(iommu_domain_free);
 
 int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 {
-	return iommu_ops->attach_dev(domain, dev);
+	if (unlikely(domain->ops->attach_dev == NULL))
+		return -ENODEV;
+
+	return domain->ops->attach_dev(domain, dev);
 }
 EXPORT_SYMBOL_GPL(iommu_attach_device);
 
 void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
 {
-	iommu_ops->detach_dev(domain, dev);
+	if (unlikely(domain->ops->detach_dev == NULL))
+		return;
+
+	domain->ops->detach_dev(domain, dev);
 }
 EXPORT_SYMBOL_GPL(iommu_detach_device);
 
 phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain,
 			       unsigned long iova)
 {
-	return iommu_ops->iova_to_phys(domain, iova);
+	if (unlikely(domain->ops->iova_to_phys == NULL))
+		return 0;
+
+	return domain->ops->iova_to_phys(domain, iova);
 }
 EXPORT_SYMBOL_GPL(iommu_iova_to_phys);
 
 int iommu_domain_has_cap(struct iommu_domain *domain,
 			 unsigned long cap)
 {
-	return iommu_ops->domain_has_cap(domain, cap);
+	if (unlikely(domain->ops->domain_has_cap == NULL))
+		return 0;
+
+	return domain->ops->domain_has_cap(domain, cap);
 }
 EXPORT_SYMBOL_GPL(iommu_domain_has_cap);
 
@@ -119,11 +133,14 @@ int iommu_map(struct iommu_domain *domain, unsigned long iova,
 {
 	size_t size;
 
+	if (unlikely(domain->ops->map == NULL))
+		return -ENODEV;
+
 	size         = PAGE_SIZE << gfp_order;
 
 	BUG_ON(!IS_ALIGNED(iova | paddr, size));
 
-	return iommu_ops->map(domain, iova, paddr, gfp_order, prot);
+	return domain->ops->map(domain, iova, paddr, gfp_order, prot);
 }
 EXPORT_SYMBOL_GPL(iommu_map);
 
@@ -131,10 +148,13 @@ int iommu_unmap(struct iommu_domain *domain, unsigned long iova, int gfp_order)
 {
 	size_t size;
 
+	if (unlikely(domain->ops->unmap == NULL))
+		return -ENODEV;
+
 	size         = PAGE_SIZE << gfp_order;
 
 	BUG_ON(!IS_ALIGNED(iova, size));
 
-	return iommu_ops->unmap(domain, iova, gfp_order);
+	return domain->ops->unmap(domain, iova, gfp_order);
 }
 EXPORT_SYMBOL_GPL(iommu_unmap);
-- 
1.7.4.1

[PATCH 06/10] iommu/amd: Use bus_set_iommu instead of register_iommu

[Joerg Roedel]

Convert the AMD IOMMU driver to use the new interface for
publishing the iommu_ops.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/iommu/amd_iommu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index a14f8dc..57f6f38 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2493,7 +2493,7 @@ static unsigned device_dma_ops_init(void)
 
 void __init amd_iommu_init_api(void)
 {
-	register_iommu(&amd_iommu_ops);
+	bus_set_iommu(&pci_bus_type, &amd_iommu_ops);
 }
 
 int __init amd_iommu_init_dma_ops(void)
-- 
1.7.4.1

[PATCH 07/10] iommu/vt-d: Use bus_set_iommu instead of register_iommu

[Joerg Roedel]

Convert the Intel IOMMU driver to use the new interface for
publishing the iommu_ops.

Cc: David Woodhouse <dwmw2@infradead.org>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/iommu/intel-iommu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index c621c98..2d53c3d 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3486,7 +3486,7 @@ int __init intel_iommu_init(void)
 
 	init_iommu_pm_ops();
 
-	register_iommu(&intel_iommu_ops);
+	bus_set_iommu(&pci_bus_type, &intel_iommu_ops);
 
 	bus_register_notifier(&pci_bus_type, &device_nb);
 
-- 
1.7.4.1

[PATCH 08/10] iommu/omap: Use bus_set_iommu instead of register_iommu

[Joerg Roedel]

Convert the OMAP IOMMU driver on ARM to use the new
interface for publishing the iommu_ops.

Cc: Ohad Ben-Cohen <ohad@wizery.com>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/iommu/omap-iommu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index bd5f606..16d5b76 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1250,7 +1250,7 @@ static int __init omap_iommu_init(void)
 		return -ENOMEM;
 	iopte_cachep = p;
 
-	register_iommu(&omap_iommu_ops);
+	bus_set_iommu(&platform_bus_type, &omap_iommu_ops);
 
 	return platform_driver_register(&omap_iommu_driver);
 }
-- 
1.7.4.1

[PATCH 09/10] iommu/msm: Use bus_set_iommu instead of register_iommu

[Joerg Roedel]

Convert the MSM IOMMU driver for ARM to use the new
interface for publishing the iommu_ops.

Cc: David Brown <davidb@codeaurora.org>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/iommu/msm_iommu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index d1733f6..5865dd2 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -728,7 +728,7 @@ static void __init setup_iommu_tex_classes(void)
 static int __init msm_iommu_init(void)
 {
 	setup_iommu_tex_classes();
-	register_iommu(&msm_iommu_ops);
+	bus_set_iommu(&platform_bus_type, &msm_iommu_ops);
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH 10/10] iommu/core: Remove global iommu_ops and register_iommu

[Joerg Roedel]

With all IOMMU drivers being converted to bus_set_iommu the
global iommu_ops are no longer required. The same is true
for the deprecated register_iommu function.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/iommu/iommu.c |   27 ++++-----------------------
 include/linux/iommu.h |    1 -
 2 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f2ced4c..82178cb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -25,50 +25,31 @@
 #include <linux/errno.h>
 #include <linux/iommu.h>
 
-static struct iommu_ops *iommu_ops;
-
-void register_iommu(struct iommu_ops *ops)
-{
-	if (iommu_ops)
-		BUG();
-
-	iommu_ops = ops;
-}
-
 void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops)
 {
 }
 
 bool iommu_present(struct bus_type *bus)
 {
-	if (bus->iommu_ops != NULL)
-		return true;
-	else
-		return iommu_ops != NULL;
+	return bus->iommu_ops != NULL;
 }
 EXPORT_SYMBOL_GPL(iommu_present);
 
 struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
 {
 	struct iommu_domain *domain;
-	struct iommu_ops *ops;
 	int ret;
 
-	if (bus->iommu_ops)
-		ops = bus->iommu_ops;
-	else
-		ops = iommu_ops;
-
-	if (ops == NULL)
+	if (bus->iommu_ops == NULL)
 		return NULL;
 
 	domain = kmalloc(sizeof(*domain), GFP_KERNEL);
 	if (!domain)
 		return NULL;
 
-	domain->ops = ops;
+	domain->ops = bus->iommu_ops;
 
-	ret = iommu_ops->domain_init(domain);
+	ret = domain->ops->domain_init(domain);
 	if (ret)
 		goto out_free;
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index de73219..7014f40 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -54,7 +54,6 @@ struct iommu_ops {
 			      unsigned long cap);
 };
 
-extern void register_iommu(struct iommu_ops *ops);
 extern void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops);
 extern bool iommu_present(struct bus_type *bus);
 extern struct iommu_domain *iommu_domain_alloc(struct bus_type *bus);
-- 
1.7.4.1

Re: [PATCH 0/10] IOMMU: Make iommu_ops per-bus_type

[Greg KH]

On Wed, Sep 07, 2011 at 05:41:43PM +0200, Joerg Roedel wrote:
> Hi,
> 
> here is the new version of the patch-set to make the iommu_ops used in
> the iommu-api a bus_type property. This will allow us to move code out
> of the iommu drivers into generic code and it simplifies the
> implementation of the Alex' device-group property.
> 
> Greg, can you have a look at patch 2 please and tell me if you have any
> objections?

I object, please see my comments.

> With this version the patch-set is complete (not as the first RFC post).
> It converts all iommu drivers to use the new registration interface and
> completly removes the register_iommu interface.
> 
> Regards,
> 
> 	Joerg
> 
> Diffstat:
> 
>  arch/ia64/kvm/kvm-ia64.c           |    3 +-
>  arch/x86/kvm/x86.c                 |    3 +-
>  drivers/base/bus.c                 |   16 ++++++++++
>  drivers/iommu/amd_iommu.c          |    2 +-
>  drivers/iommu/intel-iommu.c        |    2 +-
>  drivers/iommu/iommu.c              |   58 ++++++++++++++++++++++++------------
>  drivers/iommu/msm_iommu.c          |    2 +-
>  drivers/iommu/omap-iommu.c         |    2 +-
>  drivers/media/video/omap3isp/isp.c |    2 +-
>  include/linux/device.h             |    9 +++++
>  include/linux/iommu.h              |   21 +++++++------
>  virt/kvm/iommu.c                   |    4 +-
>  12 files changed, 86 insertions(+), 38 deletions(-)

So the overall work here makes for more code, right?  I fail to see the
benifit, what am I missing?

greg k-h

Re: [PATCH 0/10] IOMMU: Make iommu_ops per-bus_type

[Joerg Roedel]

On Wed, Sep 07, 2011 at 11:48:40AM -0700, Greg KH wrote:
> On Wed, Sep 07, 2011 at 05:41:43PM +0200, Joerg Roedel wrote:
> > Diffstat:
> > 
> >  arch/ia64/kvm/kvm-ia64.c           |    3 +-
> >  arch/x86/kvm/x86.c                 |    3 +-
> >  drivers/base/bus.c                 |   16 ++++++++++
> >  drivers/iommu/amd_iommu.c          |    2 +-
> >  drivers/iommu/intel-iommu.c        |    2 +-
> >  drivers/iommu/iommu.c              |   58 ++++++++++++++++++++++++------------
> >  drivers/iommu/msm_iommu.c          |    2 +-
> >  drivers/iommu/omap-iommu.c         |    2 +-
> >  drivers/media/video/omap3isp/isp.c |    2 +-
> >  include/linux/device.h             |    9 +++++
> >  include/linux/iommu.h              |   21 +++++++------
> >  virt/kvm/iommu.c                   |    4 +-
> >  12 files changed, 86 insertions(+), 38 deletions(-)
> 
> So the overall work here makes for more code, right?  I fail to see the
> benifit, what am I missing?

Yes, the code that moves code out of the IOMMU drivers is not included
here. This is only the infrastructure for future code generalization in
the IOMMU driver. That is why the iommu_bus_init() function in patch 2
is still empty :)
For example, the IOMMU drivers on x86 implement device-notifier
callbacks to know when devices are added or removed to do iommu-specific
setup. The iommu drivers also scan over the devices on a bus to do
initialization. All this device-walking code and the notifiers can be
moved to generic IOMMU code leaving only the specific setup routines in
the iommu drivers.

Regards,

	Joerg