| Message ID | 1315410113-26608-3-git-send-email-joerg.roedel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)