| Message ID | 20161109141948.19244-5-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/11/16 14:19, Lorenzo Pieralisi wrote: > The of_iommu_{set/get}_ops() API is used to associate a device > tree node with a specific set of IOMMU operations. The same > kernel interface is required on systems booting with ACPI, where > devices are not associated with a device tree node, therefore > the interface requires generalization. > > The struct device fwnode member represents the fwnode token > associated with the device and the struct it points at is firmware > specific; regardless, it is initialized on both ACPI and DT systems > and makes an ideal candidate to use it to associate a set of IOMMU > operations to a given device, through its struct device.fwnode member > pointer. > > Convert the DT specific of_iommu_{set/get}_ops() interface to > use struct device.fwnode as a look-up token, making the interface > usable on ACPI systems. > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Tested-by: Hanjun Guo <hanjun.guo@linaro.org> > Tested-by: Tomasz Nowicki <tn@semihalf.com> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Hanjun Guo <hanjun.guo@linaro.org> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Joerg Roedel <joro@8bytes.org> > --- > drivers/iommu/iommu.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > drivers/iommu/of_iommu.c | 39 --------------------------------------- > include/linux/iommu.h | 14 ++++++++++++++ > include/linux/of_iommu.h | 12 ++++++++++-- > 4 files changed, 66 insertions(+), 41 deletions(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 9a2f196..5c97c01 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1615,6 +1615,48 @@ int iommu_request_dm_for_dev(struct device *dev) > return ret; > } > > +struct iommu_fwentry { :) > + struct list_head list; > + struct fwnode_handle *fwnode; > + const struct iommu_ops *ops; > +}; > +static LIST_HEAD(iommu_fwentry_list); > +static DEFINE_SPINLOCK(iommu_fwentry_lock); > + > +void fwnode_iommu_set_ops(struct fwnode_handle *fwnode, > + const struct iommu_ops *ops) > +{ > + struct iommu_fwentry *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL); > + > + if (WARN_ON(!iommu)) > + return; > + > + if (is_of_node(fwnode)) Nit: this check is actually redundant, since to_of_node() already does the right thing and of_node_get() is NULL-safe - iommu_fwspec_init() already works that way. On the other hand, though, it is perhaps more intuitive to see it explicitly, and since to_of_node() is inline it ought to result in the same object code (I've not checked, though). Either way: Reviewed-by: Robin Murphy <robin.murphy@arm.com> > + of_node_get(to_of_node(fwnode)); > + > + INIT_LIST_HEAD(&iommu->list); > + iommu->fwnode = fwnode; > + iommu->ops = ops; > + spin_lock(&iommu_fwentry_lock); > + list_add_tail(&iommu->list, &iommu_fwentry_list); > + spin_unlock(&iommu_fwentry_lock); > +} > + > +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode) > +{ > + struct iommu_fwentry *node; > + const struct iommu_ops *ops = NULL; > + > + spin_lock(&iommu_fwentry_lock); > + list_for_each_entry(node, &iommu_fwentry_list, list) > + if (node->fwnode == fwnode) { > + ops = node->ops; > + break; > + } > + spin_unlock(&iommu_fwentry_lock); > + return ops; > +} > + > int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode, > const struct iommu_ops *ops) > { > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index 5b82862..0f57ddc 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -96,45 +96,6 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index, > } > EXPORT_SYMBOL_GPL(of_get_dma_window); > > -struct of_iommu_node { > - struct list_head list; > - struct device_node *np; > - const struct iommu_ops *ops; > -}; > -static LIST_HEAD(of_iommu_list); > -static DEFINE_SPINLOCK(of_iommu_lock); > - > -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops) > -{ > - struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL); > - > - if (WARN_ON(!iommu)) > - return; > - > - of_node_get(np); > - INIT_LIST_HEAD(&iommu->list); > - iommu->np = np; > - iommu->ops = ops; > - spin_lock(&of_iommu_lock); > - list_add_tail(&iommu->list, &of_iommu_list); > - spin_unlock(&of_iommu_lock); > -} > - > -const struct iommu_ops *of_iommu_get_ops(struct device_node *np) > -{ > - struct of_iommu_node *node; > - const struct iommu_ops *ops = NULL; > - > - spin_lock(&of_iommu_lock); > - list_for_each_entry(node, &of_iommu_list, list) > - if (node->np == np) { > - ops = node->ops; > - break; > - } > - spin_unlock(&of_iommu_lock); > - return ops; > -} > - > static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data) > { > struct of_phandle_args *iommu_spec = data; > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 436dc21..15d5478 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -351,6 +351,9 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode, > const struct iommu_ops *ops); > void iommu_fwspec_free(struct device *dev); > int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids); > +void fwnode_iommu_set_ops(struct fwnode_handle *fwnode, > + const struct iommu_ops *ops); > +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode); > > #else /* CONFIG_IOMMU_API */ > > @@ -580,6 +583,17 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids, > return -ENODEV; > } > > +static inline void fwnode_iommu_set_ops(struct fwnode_handle *fwnode, > + const struct iommu_ops *ops) > +{ > +} > + > +static inline > +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode) > +{ > + return NULL; > +} > + > #endif /* CONFIG_IOMMU_API */ > > #endif /* __LINUX_IOMMU_H */ > diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h > index e80b9c7..7681007 100644 > --- a/include/linux/of_iommu.h > +++ b/include/linux/of_iommu.h > @@ -31,8 +31,16 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev, > > #endif /* CONFIG_OF_IOMMU */ > > -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops); > -const struct iommu_ops *of_iommu_get_ops(struct device_node *np); > +static inline void of_iommu_set_ops(struct device_node *np, > + const struct iommu_ops *ops) > +{ > + fwnode_iommu_set_ops(&np->fwnode, ops); > +} > + > +static inline const struct iommu_ops *of_iommu_get_ops(struct device_node *np) > +{ > + return fwnode_iommu_get_ops(&np->fwnode); > +} > > extern struct of_device_id __iommu_of_table; > >
On Wed, Nov 09, 2016 at 02:40:08PM +0000, Robin Murphy wrote: [...] > > +void fwnode_iommu_set_ops(struct fwnode_handle *fwnode, > > + const struct iommu_ops *ops) > > +{ > > + struct iommu_fwentry *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL); > > + > > + if (WARN_ON(!iommu)) > > + return; > > + > > + if (is_of_node(fwnode)) > > Nit: this check is actually redundant, since to_of_node() already does > the right thing and of_node_get() is NULL-safe - iommu_fwspec_init() > already works that way. On the other hand, though, it is perhaps more > intuitive to see it explicitly, and since to_of_node() is inline it > ought to result in the same object code (I've not checked, though). I can easily fold this change in the final code and I think we should keep this consistent so I am happy to change my code and make the is_of_node() check implicit. > Either way: > > Reviewed-by: Robin Murphy <robin.murphy@arm.com> Thank you ! Lorenzo > > + of_node_get(to_of_node(fwnode)); > > + > > + INIT_LIST_HEAD(&iommu->list); > > + iommu->fwnode = fwnode; > > + iommu->ops = ops; > > + spin_lock(&iommu_fwentry_lock); > > + list_add_tail(&iommu->list, &iommu_fwentry_list); > > + spin_unlock(&iommu_fwentry_lock); > > +} > > + > > +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode) > > +{ > > + struct iommu_fwentry *node; > > + const struct iommu_ops *ops = NULL; > > + > > + spin_lock(&iommu_fwentry_lock); > > + list_for_each_entry(node, &iommu_fwentry_list, list) > > + if (node->fwnode == fwnode) { > > + ops = node->ops; > > + break; > > + } > > + spin_unlock(&iommu_fwentry_lock); > > + return ops; > > +} > > + > > int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode, > > const struct iommu_ops *ops) > > { > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > > index 5b82862..0f57ddc 100644 > > --- a/drivers/iommu/of_iommu.c > > +++ b/drivers/iommu/of_iommu.c > > @@ -96,45 +96,6 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index, > > } > > EXPORT_SYMBOL_GPL(of_get_dma_window); > > > > -struct of_iommu_node { > > - struct list_head list; > > - struct device_node *np; > > - const struct iommu_ops *ops; > > -}; > > -static LIST_HEAD(of_iommu_list); > > -static DEFINE_SPINLOCK(of_iommu_lock); > > - > > -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops) > > -{ > > - struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL); > > - > > - if (WARN_ON(!iommu)) > > - return; > > - > > - of_node_get(np); > > - INIT_LIST_HEAD(&iommu->list); > > - iommu->np = np; > > - iommu->ops = ops; > > - spin_lock(&of_iommu_lock); > > - list_add_tail(&iommu->list, &of_iommu_list); > > - spin_unlock(&of_iommu_lock); > > -} > > - > > -const struct iommu_ops *of_iommu_get_ops(struct device_node *np) > > -{ > > - struct of_iommu_node *node; > > - const struct iommu_ops *ops = NULL; > > - > > - spin_lock(&of_iommu_lock); > > - list_for_each_entry(node, &of_iommu_list, list) > > - if (node->np == np) { > > - ops = node->ops; > > - break; > > - } > > - spin_unlock(&of_iommu_lock); > > - return ops; > > -} > > - > > static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data) > > { > > struct of_phandle_args *iommu_spec = data; > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > index 436dc21..15d5478 100644 > > --- a/include/linux/iommu.h > > +++ b/include/linux/iommu.h > > @@ -351,6 +351,9 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode, > > const struct iommu_ops *ops); > > void iommu_fwspec_free(struct device *dev); > > int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids); > > +void fwnode_iommu_set_ops(struct fwnode_handle *fwnode, > > + const struct iommu_ops *ops); > > +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode); > > > > #else /* CONFIG_IOMMU_API */ > > > > @@ -580,6 +583,17 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids, > > return -ENODEV; > > } > > > > +static inline void fwnode_iommu_set_ops(struct fwnode_handle *fwnode, > > + const struct iommu_ops *ops) > > +{ > > +} > > + > > +static inline > > +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode) > > +{ > > + return NULL; > > +} > > + > > #endif /* CONFIG_IOMMU_API */ > > > > #endif /* __LINUX_IOMMU_H */ > > diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h > > index e80b9c7..7681007 100644 > > --- a/include/linux/of_iommu.h > > +++ b/include/linux/of_iommu.h > > @@ -31,8 +31,16 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev, > > > > #endif /* CONFIG_OF_IOMMU */ > > > > -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops); > > -const struct iommu_ops *of_iommu_get_ops(struct device_node *np); > > +static inline void of_iommu_set_ops(struct device_node *np, > > + const struct iommu_ops *ops) > > +{ > > + fwnode_iommu_set_ops(&np->fwnode, ops); > > +} > > + > > +static inline const struct iommu_ops *of_iommu_get_ops(struct device_node *np) > > +{ > > + return fwnode_iommu_get_ops(&np->fwnode); > > +} > > > > extern struct of_device_id __iommu_of_table; > > > > >
On Wed, Nov 09, 2016 at 02:19:36PM +0000, Lorenzo Pieralisi wrote: > +struct iommu_fwentry { > + struct list_head list; > + struct fwnode_handle *fwnode; > + const struct iommu_ops *ops; > +}; Is there a reason the iommu_ops need to be stored there? Currently it seems that the ops are only needed to get the of_xlate fn-ptr later. And the place where it is called the iommu-ops should also be available through pdev->dev->bus->iommu_ops. Joerg
On 11/11/16 15:22, Joerg Roedel wrote: > On Wed, Nov 09, 2016 at 02:19:36PM +0000, Lorenzo Pieralisi wrote: >> +struct iommu_fwentry { >> + struct list_head list; >> + struct fwnode_handle *fwnode; >> + const struct iommu_ops *ops; >> +}; > > Is there a reason the iommu_ops need to be stored there? Currently it > seems that the ops are only needed to get the of_xlate fn-ptr later. And > the place where it is called the iommu-ops should also be available > through pdev->dev->bus->iommu_ops. In the original of_iommu_configure design, the thought was that an ops structure could be IOMMU-instance-specific (hence the later-removed "priv" member), so I suppose right now it is mostly a hangover from that. However, it's also what we initialise a device's fwspec with, so becomes important again if we're ever going to get past the limitations of buses-which-are-not-actually-buses[1]. Robin. [1]:http://www.mail-archive.com/iommu@lists.linux-foundation.org/msg14576.html
On Fri, Nov 11, 2016 at 04:17:37PM +0000, Robin Murphy wrote: > In the original of_iommu_configure design, the thought was that an ops > structure could be IOMMU-instance-specific (hence the later-removed > "priv" member), so I suppose right now it is mostly a hangover from > that. However, it's also what we initialise a device's fwspec with, so > becomes important again if we're ever going to get past the limitations > of buses-which-are-not-actually-buses[1]. Yeah, I discussed this with a few others at LPC. My current idea is to tell the iommu-core which hardware-iommus exist in the system and a seperate iommu_ops ptr for each of them. Then every struct device can link to the iommu-instance it is translated by. We are not there yet, but this will give you the same per-device iommu-ops as implemented here. Joerg
On 11/11/16 16:27, Joerg Roedel wrote: > On Fri, Nov 11, 2016 at 04:17:37PM +0000, Robin Murphy wrote: >> In the original of_iommu_configure design, the thought was that an ops >> structure could be IOMMU-instance-specific (hence the later-removed >> "priv" member), so I suppose right now it is mostly a hangover from >> that. However, it's also what we initialise a device's fwspec with, so >> becomes important again if we're ever going to get past the limitations >> of buses-which-are-not-actually-buses[1]. > > Yeah, I discussed this with a few others at LPC. My current idea is to > tell the iommu-core which hardware-iommus exist in the system and a > seperate iommu_ops ptr for each of them. Then every struct device can > link to the iommu-instance it is translated by. Er, that sounds very much like a description of what we already have in 4.9-rc. Every struct device now has an iommu_fwspec which encapsulates both an iommu_ops pointer (which can perfectly well be per-instance if the IOMMU driver wants) and a place for the IOMMU-private data to replace the mess of archdata.iommu and driver-internal globals. > We are not there yet, but this will give you the same per-device > iommu-ops as implemented here. With those two patches I linked to, which make the bulk of the IOMMU core code per-device-ops-aware off the bat, I'd say we *are* already pretty much there. It's only iommu_domain_alloc() which needs a device-based alternative, and the non-of_xlate-based IOMMU drivers to either call iommu_fwspec_init() for themselves, or perhaps for x86 plumbing in DMAR/IVRS equivalents of the IORT parsing to the infrastructure provided by this series. Robin.
Hi Robin, Joerg, On Fri, Nov 11, 2016 at 05:43:39PM +0000, Robin Murphy wrote: > On 11/11/16 16:27, Joerg Roedel wrote: > > On Fri, Nov 11, 2016 at 04:17:37PM +0000, Robin Murphy wrote: > >> In the original of_iommu_configure design, the thought was that an ops > >> structure could be IOMMU-instance-specific (hence the later-removed > >> "priv" member), so I suppose right now it is mostly a hangover from > >> that. However, it's also what we initialise a device's fwspec with, so > >> becomes important again if we're ever going to get past the limitations > >> of buses-which-are-not-actually-buses[1]. > > > > Yeah, I discussed this with a few others at LPC. My current idea is to > > tell the iommu-core which hardware-iommus exist in the system and a > > seperate iommu_ops ptr for each of them. Then every struct device can > > link to the iommu-instance it is translated by. > > Er, that sounds very much like a description of what we already have in > 4.9-rc. Every struct device now has an iommu_fwspec which encapsulates > both an iommu_ops pointer (which can perfectly well be per-instance if > the IOMMU driver wants) and a place for the IOMMU-private data to > replace the mess of archdata.iommu and driver-internal globals. > > > We are not there yet, but this will give you the same per-device > > iommu-ops as implemented here. > > With those two patches I linked to, which make the bulk of the IOMMU > core code per-device-ops-aware off the bat, I'd say we *are* already > pretty much there. It's only iommu_domain_alloc() which needs a > device-based alternative, and the non-of_xlate-based IOMMU drivers to > either call iommu_fwspec_init() for themselves, or perhaps for x86 > plumbing in DMAR/IVRS equivalents of the IORT parsing to the > infrastructure provided by this series. I think it all boils down to how we end up implementing the per-device iommu_ops look-up/binding, question is what do you want me to do with this patch, it should be fine to drop it and use dev->bus->iommu_ops for the look-up but I should know sooner rather than later to make sure the series get another good round of testing. Please let me know, thank you very much. Lorenzo
On 14/11/16 10:26, Lorenzo Pieralisi wrote: > Hi Robin, Joerg, > > On Fri, Nov 11, 2016 at 05:43:39PM +0000, Robin Murphy wrote: >> On 11/11/16 16:27, Joerg Roedel wrote: >>> On Fri, Nov 11, 2016 at 04:17:37PM +0000, Robin Murphy wrote: >>>> In the original of_iommu_configure design, the thought was that an ops >>>> structure could be IOMMU-instance-specific (hence the later-removed >>>> "priv" member), so I suppose right now it is mostly a hangover from >>>> that. However, it's also what we initialise a device's fwspec with, so >>>> becomes important again if we're ever going to get past the limitations >>>> of buses-which-are-not-actually-buses[1]. >>> >>> Yeah, I discussed this with a few others at LPC. My current idea is to >>> tell the iommu-core which hardware-iommus exist in the system and a >>> seperate iommu_ops ptr for each of them. Then every struct device can >>> link to the iommu-instance it is translated by. >> >> Er, that sounds very much like a description of what we already have in >> 4.9-rc. Every struct device now has an iommu_fwspec which encapsulates >> both an iommu_ops pointer (which can perfectly well be per-instance if >> the IOMMU driver wants) and a place for the IOMMU-private data to >> replace the mess of archdata.iommu and driver-internal globals. >> >>> We are not there yet, but this will give you the same per-device >>> iommu-ops as implemented here. >> >> With those two patches I linked to, which make the bulk of the IOMMU >> core code per-device-ops-aware off the bat, I'd say we *are* already >> pretty much there. It's only iommu_domain_alloc() which needs a >> device-based alternative, and the non-of_xlate-based IOMMU drivers to >> either call iommu_fwspec_init() for themselves, or perhaps for x86 >> plumbing in DMAR/IVRS equivalents of the IORT parsing to the >> infrastructure provided by this series. > > I think it all boils down to how we end up implementing the per-device > iommu_ops look-up/binding, question is what do you want me to do with > this patch, it should be fine to drop it and use dev->bus->iommu_ops > for the look-up but I should know sooner rather than later to make > sure the series get another good round of testing. If we've already made the decision to move away from bus ops, I don't see that it makes sense to deliberately introduce new dependencies on them. Besides, as it stands, this patch literally implements "tell the iommu-core which hardware-iommus exist in the system and a seperate iommu_ops ptr for each of them" straight off. Robin. > > Please let me know, thank you very much. > > Lorenzo >
On Mon, Nov 14, 2016 at 12:00:47PM +0000, Robin Murphy wrote: > If we've already made the decision to move away from bus ops, I don't > see that it makes sense to deliberately introduce new dependencies on > them. Besides, as it stands, this patch literally implements "tell the > iommu-core which hardware-iommus exist in the system and a seperate > iommu_ops ptr for each of them" straight off. Not sure which code you are looking at, but as I see it we have only per-device iommu-ops now (with this patch). That is different from having core-visible hardware-iommu instances where devices could link to. Also the rest of iommu-core code still makes use of the per-bus ops. The per-device ops are only used for the of_xlate fn-ptr. Joerg
On Mon, Nov 14, 2016 at 04:52:23PM +0100, Joerg Roedel wrote: > On Mon, Nov 14, 2016 at 12:00:47PM +0000, Robin Murphy wrote: > > If we've already made the decision to move away from bus ops, I don't > > see that it makes sense to deliberately introduce new dependencies on > > them. Besides, as it stands, this patch literally implements "tell the > > iommu-core which hardware-iommus exist in the system and a seperate > > iommu_ops ptr for each of them" straight off. > > Not sure which code you are looking at, but as I see it we have only > per-device iommu-ops now (with this patch). That is different from > having core-visible hardware-iommu instances where devices could link > to. This patch enables the IOMMU-OF-node<->device look-up on non-OF (ie ACPI) systems by "converting" the of_node to a generic fwnode_handle, that's all it does (and move the related look-up code from drivers/iommu/of_iommu.c to drivers/iommu/iommu.c so that it does not depend on OF_IOMMU any longer). > Also the rest of iommu-core code still makes use of the per-bus ops. The > per-device ops are only used for the of_xlate fn-ptr. I can put this patch on the backburner and retrieve the iommu_ops through the dev->bus path in the IORT xlate function (iort_iommu_xlate() introduced in the last patch), the change is trivial and should work just fine but it deserves a v8 to give everyone a chance to test it. We would end-up handling the device->iommu_ops look-up differently in DT and ACPI for streamid translations though, I am not sure I see a reason why. Thanks, Lorenzo
On 14/11/16 15:52, Joerg Roedel wrote: > On Mon, Nov 14, 2016 at 12:00:47PM +0000, Robin Murphy wrote: >> If we've already made the decision to move away from bus ops, I don't >> see that it makes sense to deliberately introduce new dependencies on >> them. Besides, as it stands, this patch literally implements "tell the >> iommu-core which hardware-iommus exist in the system and a seperate >> iommu_ops ptr for each of them" straight off. > > Not sure which code you are looking at, but as I see it we have only > per-device iommu-ops now (with this patch). That is different from > having core-visible hardware-iommu instances where devices could link > to. The per-device IOMMU ops are already there since 57f98d2f61e1. This patch generalises the other end, moving the "registering an IOMMU instance" (i.e. iommu_fwentry) bit into the IOMMU core, from being OF-specific. I'd be perfectly happy if we rename iommu_fwentry to iommu_instance, fwnode_iommu_set_ops() to iommu_register_instance(), and such if that makes the design intent clearer. If you'd also prefer to replace iommu_fwspec::ops with an opaque iommu_fwspec::iommu_instance pointer so that things are a bit more centralised (and users are forced to go through the API rather then call ops directly), I'd have no major objection either. My main point is that we've been deliberately putting the relevant building blocks in place - the of_iommu_{get,set}_ops stuff was designed from the start to accommodate per-instance ops, via the ops pointer *being* the instance token; the iommu_fwspec stuff is deliberately intended to provide per-device ops on top of that. The raw functionality is either there in iommu.c already, or moving there in patches already written, so if it doesn't look right all we need to focus on is making it look right. > Also the rest of iommu-core code still makes use of the per-bus ops. The > per-device ops are only used for the of_xlate fn-ptr. Hence my aforementioned patches intended for 4.10, directly following on from introducing iommu_fwspec in 4.9: http://www.mail-archive.com/iommu@lists.linux-foundation.org/msg14576.html ...the purpose being to provide a smooth transition from per-bus ops to per-device, per-instance ops. Apply those and we're 90% of the way there for OF-based IOMMU drivers (not that any of those actually need per-instance ops, admittedly; I did prototype it for the ARM SMMU ages ago, but it didn't seem worth the bother). Lorenzo's series broadens the scope to ACPI-based systems and moves the generically-useful parts into the core where we can easily build on them further if necessary. The major remaining work is to convert external callers of the current bus-dependent functions like iommu_domain_alloc(), iommu_present(), etc. to device-based alternatives. Robin.
On Mon, Nov 14, 2016 at 06:25:16PM +0000, Robin Murphy wrote: > On 14/11/16 15:52, Joerg Roedel wrote: > > On Mon, Nov 14, 2016 at 12:00:47PM +0000, Robin Murphy wrote: > >> If we've already made the decision to move away from bus ops, I don't > >> see that it makes sense to deliberately introduce new dependencies on > >> them. Besides, as it stands, this patch literally implements "tell the > >> iommu-core which hardware-iommus exist in the system and a seperate > >> iommu_ops ptr for each of them" straight off. > > > > Not sure which code you are looking at, but as I see it we have only > > per-device iommu-ops now (with this patch). That is different from > > having core-visible hardware-iommu instances where devices could link > > to. > > The per-device IOMMU ops are already there since 57f98d2f61e1. This > patch generalises the other end, moving the "registering an IOMMU > instance" (i.e. iommu_fwentry) bit into the IOMMU core, from being > OF-specific. I'd be perfectly happy if we rename iommu_fwentry to > iommu_instance, fwnode_iommu_set_ops() to iommu_register_instance(), and > such if that makes the design intent clearer. I second that and I need to know what to do with this patch sooner rather than later so it is time we make a decision please. Joerg, what's your opinion ? Thanks, Lorenzo > If you'd also prefer to replace iommu_fwspec::ops with an opaque > iommu_fwspec::iommu_instance pointer so that things are a bit more > centralised (and users are forced to go through the API rather then call > ops directly), I'd have no major objection either. My main point is that > we've been deliberately putting the relevant building blocks in place - > the of_iommu_{get,set}_ops stuff was designed from the start to > accommodate per-instance ops, via the ops pointer *being* the instance > token; the iommu_fwspec stuff is deliberately intended to provide > per-device ops on top of that. The raw functionality is either there in > iommu.c already, or moving there in patches already written, so if it > doesn't look right all we need to focus on is making it look right. > > > Also the rest of iommu-core code still makes use of the per-bus ops. The > > per-device ops are only used for the of_xlate fn-ptr. > > Hence my aforementioned patches intended for 4.10, directly following on > from introducing iommu_fwspec in 4.9: > > http://www.mail-archive.com/iommu@lists.linux-foundation.org/msg14576.html > > ...the purpose being to provide a smooth transition from per-bus ops to > per-device, per-instance ops. Apply those and we're 90% of the way there > for OF-based IOMMU drivers (not that any of those actually need > per-instance ops, admittedly; I did prototype it for the ARM SMMU ages > ago, but it didn't seem worth the bother). Lorenzo's series broadens the > scope to ACPI-based systems and moves the generically-useful parts into > the core where we can easily build on them further if necessary. The > major remaining work is to convert external callers of the current > bus-dependent functions like iommu_domain_alloc(), iommu_present(), etc. > to device-based alternatives. > > Robin.
Hi Joerg, On Mon, Nov 14, 2016 at 06:25:16PM +0000, Robin Murphy wrote: > On 14/11/16 15:52, Joerg Roedel wrote: > > On Mon, Nov 14, 2016 at 12:00:47PM +0000, Robin Murphy wrote: > >> If we've already made the decision to move away from bus ops, I don't > >> see that it makes sense to deliberately introduce new dependencies on > >> them. Besides, as it stands, this patch literally implements "tell the > >> iommu-core which hardware-iommus exist in the system and a seperate > >> iommu_ops ptr for each of them" straight off. > > > > Not sure which code you are looking at, but as I see it we have only > > per-device iommu-ops now (with this patch). That is different from > > having core-visible hardware-iommu instances where devices could link > > to. > > The per-device IOMMU ops are already there since 57f98d2f61e1. This > patch generalises the other end, moving the "registering an IOMMU > instance" (i.e. iommu_fwentry) bit into the IOMMU core, from being > OF-specific. I'd be perfectly happy if we rename iommu_fwentry to > iommu_instance, fwnode_iommu_set_ops() to iommu_register_instance(), and > such if that makes the design intent clearer. I can easily make the changes Robin suggests above, I need to know what to do with this patch it is the last blocking point for this series and time is running out I can revert to using dev->bus to retrieve iommu_ops (even though I do not think it makes sense given what Robin outlines below) but I need to know please, we can't gate an entire series for this patch that is just syntactic sugar. Thanks ! Lorenzo > If you'd also prefer to replace iommu_fwspec::ops with an opaque > iommu_fwspec::iommu_instance pointer so that things are a bit more > centralised (and users are forced to go through the API rather then call > ops directly), I'd have no major objection either. My main point is that > we've been deliberately putting the relevant building blocks in place - > the of_iommu_{get,set}_ops stuff was designed from the start to > accommodate per-instance ops, via the ops pointer *being* the instance > token; the iommu_fwspec stuff is deliberately intended to provide > per-device ops on top of that. The raw functionality is either there in > iommu.c already, or moving there in patches already written, so if it > doesn't look right all we need to focus on is making it look right. > > > Also the rest of iommu-core code still makes use of the per-bus ops. The > > per-device ops are only used for the of_xlate fn-ptr. > > Hence my aforementioned patches intended for 4.10, directly following on > from introducing iommu_fwspec in 4.9: > > http://www.mail-archive.com/iommu@lists.linux-foundation.org/msg14576.html > > ...the purpose being to provide a smooth transition from per-bus ops to > per-device, per-instance ops. Apply those and we're 90% of the way there > for OF-based IOMMU drivers (not that any of those actually need > per-instance ops, admittedly; I did prototype it for the ARM SMMU ages > ago, but it didn't seem worth the bother). Lorenzo's series broadens the > scope to ACPI-based systems and moves the generically-useful parts into > the core where we can easily build on them further if necessary. The > major remaining work is to convert external callers of the current > bus-dependent functions like iommu_domain_alloc(), iommu_present(), etc. > to device-based alternatives. > > Robin.
On Wed, Nov 16, 2016 at 09:56:15AM +0000, Lorenzo Pieralisi wrote: > I can easily make the changes Robin suggests above, I need to know > what to do with this patch it is the last blocking point for this > series and time is running out I can revert to using dev->bus to > retrieve iommu_ops (even though I do not think it makes sense given > what Robin outlines below) but I need to know please, we can't gate > an entire series for this patch that is just syntactic sugar. Well, I didn't really object to the approach per-se, I just wanted to know the rationale behind the need for the iommu-ops pointer. So through which tree should this series be merged? I think I can live with the pointer for now, we can later convert it to an iommu-instance pointer. Joerg
On Tue, Nov 29, 2016 at 05:05:49PM +0100, Joerg Roedel wrote: > On Wed, Nov 16, 2016 at 09:56:15AM +0000, Lorenzo Pieralisi wrote: > > I can easily make the changes Robin suggests above, I need to know > > what to do with this patch it is the last blocking point for this > > series and time is running out I can revert to using dev->bus to > > retrieve iommu_ops (even though I do not think it makes sense given > > what Robin outlines below) but I need to know please, we can't gate > > an entire series for this patch that is just syntactic sugar. > > Well, I didn't really object to the approach per-se, I just wanted to > know the rationale behind the need for the iommu-ops pointer. So through > which tree should this series be merged? I was just about to send a pull request to you, since it conflicts with my ARM SMMU patches and the PCI/ACPI bits are acked by Rafael and Bjorn. My for-joerg/arm-smmu/updates is ready to go; just need to write the pull request. Will
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 9a2f196..5c97c01 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1615,6 +1615,48 @@ int iommu_request_dm_for_dev(struct device *dev) return ret; } +struct iommu_fwentry { + struct list_head list; + struct fwnode_handle *fwnode; + const struct iommu_ops *ops; +}; +static LIST_HEAD(iommu_fwentry_list); +static DEFINE_SPINLOCK(iommu_fwentry_lock); + +void fwnode_iommu_set_ops(struct fwnode_handle *fwnode, + const struct iommu_ops *ops) +{ + struct iommu_fwentry *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL); + + if (WARN_ON(!iommu)) + return; + + if (is_of_node(fwnode)) + of_node_get(to_of_node(fwnode)); + + INIT_LIST_HEAD(&iommu->list); + iommu->fwnode = fwnode; + iommu->ops = ops; + spin_lock(&iommu_fwentry_lock); + list_add_tail(&iommu->list, &iommu_fwentry_list); + spin_unlock(&iommu_fwentry_lock); +} + +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode) +{ + struct iommu_fwentry *node; + const struct iommu_ops *ops = NULL; + + spin_lock(&iommu_fwentry_lock); + list_for_each_entry(node, &iommu_fwentry_list, list) + if (node->fwnode == fwnode) { + ops = node->ops; + break; + } + spin_unlock(&iommu_fwentry_lock); + return ops; +} + int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode, const struct iommu_ops *ops) { diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index 5b82862..0f57ddc 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -96,45 +96,6 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index, } EXPORT_SYMBOL_GPL(of_get_dma_window); -struct of_iommu_node { - struct list_head list; - struct device_node *np; - const struct iommu_ops *ops; -}; -static LIST_HEAD(of_iommu_list); -static DEFINE_SPINLOCK(of_iommu_lock); - -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops) -{ - struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL); - - if (WARN_ON(!iommu)) - return; - - of_node_get(np); - INIT_LIST_HEAD(&iommu->list); - iommu->np = np; - iommu->ops = ops; - spin_lock(&of_iommu_lock); - list_add_tail(&iommu->list, &of_iommu_list); - spin_unlock(&of_iommu_lock); -} - -const struct iommu_ops *of_iommu_get_ops(struct device_node *np) -{ - struct of_iommu_node *node; - const struct iommu_ops *ops = NULL; - - spin_lock(&of_iommu_lock); - list_for_each_entry(node, &of_iommu_list, list) - if (node->np == np) { - ops = node->ops; - break; - } - spin_unlock(&of_iommu_lock); - return ops; -} - static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data) { struct of_phandle_args *iommu_spec = data; diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 436dc21..15d5478 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -351,6 +351,9 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode, const struct iommu_ops *ops); void iommu_fwspec_free(struct device *dev); int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids); +void fwnode_iommu_set_ops(struct fwnode_handle *fwnode, + const struct iommu_ops *ops); +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode); #else /* CONFIG_IOMMU_API */ @@ -580,6 +583,17 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids, return -ENODEV; } +static inline void fwnode_iommu_set_ops(struct fwnode_handle *fwnode, + const struct iommu_ops *ops) +{ +} + +static inline +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode) +{ + return NULL; +} + #endif /* CONFIG_IOMMU_API */ #endif /* __LINUX_IOMMU_H */ diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h index e80b9c7..7681007 100644 --- a/include/linux/of_iommu.h +++ b/include/linux/of_iommu.h @@ -31,8 +31,16 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev, #endif /* CONFIG_OF_IOMMU */ -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops); -const struct iommu_ops *of_iommu_get_ops(struct device_node *np); +static inline void of_iommu_set_ops(struct device_node *np, + const struct iommu_ops *ops) +{ + fwnode_iommu_set_ops(&np->fwnode, ops); +} + +static inline const struct iommu_ops *of_iommu_get_ops(struct device_node *np) +{ + return fwnode_iommu_get_ops(&np->fwnode); +} extern struct of_device_id __iommu_of_table;
The of_iommu_{set/get}_ops() API is used to associate a device tree node with a specific set of IOMMU operations. The same kernel interface is required on systems booting with ACPI, where devices are not associated with a device tree node, therefore the interface requires generalization. The struct device fwnode member represents the fwnode token associated with the device and the struct it points at is firmware specific; regardless, it is initialized on both ACPI and DT systems and makes an ideal candidate to use it to associate a set of IOMMU operations to a given device, through its struct device.fwnode member pointer. Convert the DT specific of_iommu_{set/get}_ops() interface to use struct device.fwnode as a look-up token, making the interface usable on ACPI systems. Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Tested-by: Hanjun Guo <hanjun.guo@linaro.org> Tested-by: Tomasz Nowicki <tn@semihalf.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Hanjun Guo <hanjun.guo@linaro.org> Cc: Robin Murphy <robin.murphy@arm.com> Cc: Joerg Roedel <joro@8bytes.org> --- drivers/iommu/iommu.c | 42 ++++++++++++++++++++++++++++++++++++++++++ drivers/iommu/of_iommu.c | 39 --------------------------------------- include/linux/iommu.h | 14 ++++++++++++++ include/linux/of_iommu.h | 12 ++++++++++-- 4 files changed, 66 insertions(+), 41 deletions(-)