Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thu, Dec 04, 2014 at 07:42:28PM +, Robin Murphy wrote:
On 04/12/14 17:58, Grant Likely wrote:
[...]
+struct of_iommu_node {
+ struct list_head list;
+ struct device_node *np;
+ struct iommu_ops *ops;
Why can't this be const? Why would anyone ever need to modify it? Also
drivers do define their iommu_ops structures const these days, so you'll
either still have to cast away at some point or the compiler will
complain once you start calling this from drivers.
...whereas if we make everything const then the drivers that _do_ want to
use the private data introduced by this series and thus pass mutable
per-instance iommu_ops will be the ones having to cast. We have no users
either way until this series is merged, and the need to stash the
per-instance data somewhere other than np-data is in the way of getting it
merged - this is just a quick hack to address that. I think we've already
agreed that mutable per-instance iommu_ops holding private data aren't
particularly pleasant and will (hopefully) go away in the next
iteration[1],
at which point this all changes anyway.
Do you expect drivers to modify that *priv pointer after the ops
structure is registered? I'd be very surprised if that was the use
case. It's fine for the driver to register a non-const version, but
once it is registered, the infrastructure can treat it as const from
then on.
Possibly not - certainly my current port of the ARM SMMU which makes use
of *priv is only ever reading it - although we did also wave around
reasons for mutable ops like dynamically changing the pgsize_bitmap and
possibly even swizzling individual ops for runtime reconfiguration. On
consideration though, I'd agree that things like that are mad enough to
stay well within individual drivers if they did ever happen, and
certainly shouldn't apply to this bit of the infrastructure at any rate.
I certainly need to update the pgsize_bitmap at runtime because I don't
know the supported page sizes until I've both (a) probed the hardware
and (b) allocated page tables for a domain. We've already discussed
moving the pgsize_bitmap out of the ops, but moving it somewhere where
it remains const doesn't really help.
Can I just take the patch that Grant acked, in the interest of getting
something merged? As you say, there's plenty of planned changes in this
area anyway. I plan to send Olof a pull request this afternoon.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Friday 05 December 2014 12:10:37 Will Deacon wrote:
On Thu, Dec 04, 2014 at 07:42:28PM +, Robin Murphy wrote:
On 04/12/14 17:58, Grant Likely wrote:
[...]
+struct of_iommu_node {
+ struct list_head list;
+ struct device_node *np;
+ struct iommu_ops *ops;
Why can't this be const? Why would anyone ever need to modify it? Also
drivers do define their iommu_ops structures const these days, so you'll
either still have to cast away at some point or the compiler will
complain once you start calling this from drivers.
...whereas if we make everything const then the drivers that _do_ want to
use the private data introduced by this series and thus pass mutable
per-instance iommu_ops will be the ones having to cast. We have no users
either way until this series is merged, and the need to stash the
per-instance data somewhere other than np-data is in the way of getting
it
merged - this is just a quick hack to address that. I think we've already
agreed that mutable per-instance iommu_ops holding private data aren't
particularly pleasant and will (hopefully) go away in the next
iteration[1],
at which point this all changes anyway.
Do you expect drivers to modify that *priv pointer after the ops
structure is registered? I'd be very surprised if that was the use
case. It's fine for the driver to register a non-const version, but
once it is registered, the infrastructure can treat it as const from
then on.
Possibly not - certainly my current port of the ARM SMMU which makes use
of *priv is only ever reading it - although we did also wave around
reasons for mutable ops like dynamically changing the pgsize_bitmap and
possibly even swizzling individual ops for runtime reconfiguration. On
consideration though, I'd agree that things like that are mad enough to
stay well within individual drivers if they did ever happen, and
certainly shouldn't apply to this bit of the infrastructure at any rate.
I certainly need to update the pgsize_bitmap at runtime because I don't
know the supported page sizes until I've both (a) probed the hardware
and (b) allocated page tables for a domain. We've already discussed
moving the pgsize_bitmap out of the ops, but moving it somewhere where
it remains const doesn't really help.
Can I just take the patch that Grant acked, in the interest of getting
something merged? As you say, there's plenty of planned changes in this
area anyway. I plan to send Olof a pull request this afternoon.
I think that would be ok. The fix later should be to move the
private data pointer into of_iommu_node, but I think that will
require a larger set of changes, so let's defer that.
Arnd
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on. Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy robin.mur...@arm.com wrote: Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on. Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D I've already acked it. Why are we still talking about it? :-D g. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Fri, Dec 05, 2014 at 01:06:52PM +, Grant Likely wrote: On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy robin.mur...@arm.com wrote: Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on. Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D I've already acked it. Why are we still talking about it? :-D Am I missing something? Why is there a need to rush things? Are there actually drivers that depend on this that will be merged during the 3.19 merge window? It seems like that'd be cutting it really close given where we are in the release cycle. If that's not the case, why even bother getting this hack into 3.19 if nobody uses it and we're going to change it in 3.20 anyway? Thierry pgpP_DLSxlZYQ.pgp Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Fri, Dec 5, 2014 at 1:18 PM, Thierry Reding thierry.red...@gmail.com wrote: On Fri, Dec 05, 2014 at 01:06:52PM +, Grant Likely wrote: On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy robin.mur...@arm.com wrote: Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on. Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D I've already acked it. Why are we still talking about it? :-D Am I missing something? Why is there a need to rush things? Are there actually drivers that depend on this that will be merged during the 3.19 merge window? It seems like that'd be cutting it really close given where we are in the release cycle. If that's not the case, why even bother getting this hack into 3.19 if nobody uses it and we're going to change it in 3.20 anyway? I also acked the non-hack version, the patch that doesn't try to make everything const. I assumed that was the one that we are talking about merging. g. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Fri, Dec 05, 2014 at 01:21:31PM +, Grant Likely wrote: On Fri, Dec 5, 2014 at 1:18 PM, Thierry Reding thierry.red...@gmail.com wrote: On Fri, Dec 05, 2014 at 01:06:52PM +, Grant Likely wrote: On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy robin.mur...@arm.com wrote: Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on. Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D I've already acked it. Why are we still talking about it? :-D Am I missing something? Why is there a need to rush things? Are there actually drivers that depend on this that will be merged during the 3.19 merge window? It seems like that'd be cutting it really close given where we are in the release cycle. If that's not the case, why even bother getting this hack into 3.19 if nobody uses it and we're going to change it in 3.20 anyway? I also acked the non-hack version, the patch that doesn't try to make everything const. I assumed that was the one that we are talking about merging. Actually not making everything const would be a hack. Drivers already mark their struct iommu_ops as const. But I'm more referring to the series as a whole. It seems like there are various issues that still need to be ironed out, and there's committment to do that before 3.20, so unless there are drivers that need any of the unfinished patches for 3.19 I don't see why we should be merging them in the first place. If getting them into 3.19 is merely to resolve dependencies then it's not going to work well anyway. Since this is all going to change in 3.20 anyway we'd likely have new dependencies that need to be handled, so might just as well do it properly at that time. Thierry pgp7c9IuvJxq5.pgp Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
Hello, On 2014-12-05 14:18, Thierry Reding wrote: On Fri, Dec 05, 2014 at 01:06:52PM +, Grant Likely wrote: On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy robin.mur...@arm.com wrote: Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on. Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D I've already acked it. Why are we still talking about it? :-D Am I missing something? Why is there a need to rush things? Are there actually drivers that depend on this that will be merged during the 3.19 merge window? It seems like that'd be cutting it really close given where we are in the release cycle. If that's not the case, why even bother getting this hack into 3.19 if nobody uses it and we're going to change it in 3.20 anyway? There are Exynos SYSMMU patches ready waiting for this gets merged... Best regards -- Marek Szyprowski, PhD Samsung RD Institute Poland ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Wed, Dec 03, 2014 at 07:57:50PM +, Arnd Bergmann wrote:
On Tuesday 02 December 2014 14:16:57 Grant Likely wrote:
On Mon, Dec 1, 2014 at 11:54 PM, Rob Herring robherri...@gmail.com wrote:
On Mon, Dec 1, 2014 at 10:57 AM, Will Deacon will.dea...@arm.com wrote:
+static inline void of_iommu_set_ops(struct device_node *np,
+ const struct iommu_ops *ops)
+{
+ np-data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+ return np-data;
+}
This may collide with other users. While use of it is rare, PPC uses
it in its PCI code. The OF_DYNAMIC code frees it but never actually
sets it. There may be some coming usage with the DT overlay code or
that's just a bug. Pantelis or Grant can comment. If not, I think we
really should try to get rid of this pointer rather than expand it's
usage.
I didn't see a user of this. I'm guessing that is coming in a SMMU patch?
Good catch. This is not good. The data pointer should be avoided since
there are no controls over its use. Until a better solution can be
implemented, probably the safest thing to do is add a struct iommu_ops
pointer to struct device_node. However, assuming that only a small
portion of nodes will actually have iommu_ops set, I'd rather see a
separate registry that matches device_nodes to iommu_ops.
Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and
adapt it as needed? It should be exactly what we need to start
out and can be extended and generalized later.
Sure, I'll add this to my list of stuff to do for 3.20.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thursday 04 December 2014 09:49:53 Will Deacon wrote:
On Wed, Dec 03, 2014 at 07:57:50PM +, Arnd Bergmann wrote:
On Tuesday 02 December 2014 14:16:57 Grant Likely wrote:
On Mon, Dec 1, 2014 at 11:54 PM, Rob Herring robherri...@gmail.com
wrote:
On Mon, Dec 1, 2014 at 10:57 AM, Will Deacon will.dea...@arm.com
wrote:
+static inline void of_iommu_set_ops(struct device_node *np,
+ const struct iommu_ops *ops)
+{
+ np-data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node
*np)
+{
+ return np-data;
+}
This may collide with other users. While use of it is rare, PPC uses
it in its PCI code. The OF_DYNAMIC code frees it but never actually
sets it. There may be some coming usage with the DT overlay code or
that's just a bug. Pantelis or Grant can comment. If not, I think we
really should try to get rid of this pointer rather than expand it's
usage.
I didn't see a user of this. I'm guessing that is coming in a SMMU
patch?
Good catch. This is not good. The data pointer should be avoided since
there are no controls over its use. Until a better solution can be
implemented, probably the safest thing to do is add a struct iommu_ops
pointer to struct device_node. However, assuming that only a small
portion of nodes will actually have iommu_ops set, I'd rather see a
separate registry that matches device_nodes to iommu_ops.
Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and
adapt it as needed? It should be exactly what we need to start
out and can be extended and generalized later.
Sure, I'll add this to my list of stuff to do for 3.20.
Does that mean the we don't get any of the patches for 3.19 despite the
Acks?
Arnd
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thu, Dec 04, 2014 at 10:10:17AM +, Arnd Bergmann wrote:
On Thursday 04 December 2014 09:49:53 Will Deacon wrote:
On Wed, Dec 03, 2014 at 07:57:50PM +, Arnd Bergmann wrote:
On Tuesday 02 December 2014 14:16:57 Grant Likely wrote:
On Mon, Dec 1, 2014 at 11:54 PM, Rob Herring robherri...@gmail.com
wrote:
On Mon, Dec 1, 2014 at 10:57 AM, Will Deacon will.dea...@arm.com
wrote:
+static inline void of_iommu_set_ops(struct device_node *np,
+ const struct iommu_ops *ops)
+{
+ np-data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node
*np)
+{
+ return np-data;
+}
This may collide with other users. While use of it is rare, PPC uses
it in its PCI code. The OF_DYNAMIC code frees it but never actually
sets it. There may be some coming usage with the DT overlay code or
that's just a bug. Pantelis or Grant can comment. If not, I think we
really should try to get rid of this pointer rather than expand it's
usage.
I didn't see a user of this. I'm guessing that is coming in a SMMU
patch?
Good catch. This is not good. The data pointer should be avoided since
there are no controls over its use. Until a better solution can be
implemented, probably the safest thing to do is add a struct iommu_ops
pointer to struct device_node. However, assuming that only a small
portion of nodes will actually have iommu_ops set, I'd rather see a
separate registry that matches device_nodes to iommu_ops.
Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and
adapt it as needed? It should be exactly what we need to start
out and can be extended and generalized later.
Sure, I'll add this to my list of stuff to do for 3.20.
Does that mean the we don't get any of the patches for 3.19 despite the
Acks?
Hmm, I don't know how useful they are without the get/set ops and I don't
think I can get those ready for 3.19 given where we currently are.
Grant's suggestion of adding an iommu_ops pointer to device_node would work
as a temporary hack, but anything more advanced is going to need proper
review.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thursday 04 December 2014 10:21:27 Will Deacon wrote:
On Thu, Dec 04, 2014 at 10:10:17AM +, Arnd Bergmann wrote:
On Thursday 04 December 2014 09:49:53 Will Deacon wrote:
On Wed, Dec 03, 2014 at 07:57:50PM +, Arnd Bergmann wrote:
On Tuesday 02 December 2014 14:16:57 Grant Likely wrote:
On Mon, Dec 1, 2014 at 11:54 PM, Rob Herring robherri...@gmail.com
wrote:
On Mon, Dec 1, 2014 at 10:57 AM, Will Deacon will.dea...@arm.com
wrote:
+static inline void of_iommu_set_ops(struct device_node *np,
+ const struct iommu_ops *ops)
+{
+ np-data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct
device_node *np)
+{
+ return np-data;
+}
This may collide with other users. While use of it is rare, PPC uses
it in its PCI code. The OF_DYNAMIC code frees it but never actually
sets it. There may be some coming usage with the DT overlay code or
that's just a bug. Pantelis or Grant can comment. If not, I think we
really should try to get rid of this pointer rather than expand it's
usage.
I didn't see a user of this. I'm guessing that is coming in a SMMU
patch?
Good catch. This is not good. The data pointer should be avoided since
there are no controls over its use. Until a better solution can be
implemented, probably the safest thing to do is add a struct iommu_ops
pointer to struct device_node. However, assuming that only a small
portion of nodes will actually have iommu_ops set, I'd rather see a
separate registry that matches device_nodes to iommu_ops.
Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and
adapt it as needed? It should be exactly what we need to start
out and can be extended and generalized later.
Sure, I'll add this to my list of stuff to do for 3.20.
Does that mean the we don't get any of the patches for 3.19 despite the
Acks?
Hmm, I don't know how useful they are without the get/set ops and I don't
think I can get those ready for 3.19 given where we currently are.
Grant's suggestion of adding an iommu_ops pointer to device_node would work
as a temporary hack, but anything more advanced is going to need proper
review.
Right. I guess it doesn't hurt much if we put the new pointer inside
#ifdef CONFIG_OF_IOMMU, then at least there is no significant size
increase in most DT based platforms.
Arnd
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thu, Dec 4, 2014 at 11:19 AM, Arnd Bergmann a...@arndb.de wrote:
On Thursday 04 December 2014 10:21:27 Will Deacon wrote:
On Thu, Dec 04, 2014 at 10:10:17AM +, Arnd Bergmann wrote:
On Thursday 04 December 2014 09:49:53 Will Deacon wrote:
On Wed, Dec 03, 2014 at 07:57:50PM +, Arnd Bergmann wrote:
On Tuesday 02 December 2014 14:16:57 Grant Likely wrote:
On Mon, Dec 1, 2014 at 11:54 PM, Rob Herring robherri...@gmail.com
wrote:
On Mon, Dec 1, 2014 at 10:57 AM, Will Deacon will.dea...@arm.com
wrote:
+static inline void of_iommu_set_ops(struct device_node *np,
+ const struct iommu_ops *ops)
+{
+ np-data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct
device_node *np)
+{
+ return np-data;
+}
This may collide with other users. While use of it is rare, PPC
uses
it in its PCI code. The OF_DYNAMIC code frees it but never actually
sets it. There may be some coming usage with the DT overlay code or
that's just a bug. Pantelis or Grant can comment. If not, I think
we
really should try to get rid of this pointer rather than expand
it's
usage.
I didn't see a user of this. I'm guessing that is coming in a SMMU
patch?
Good catch. This is not good. The data pointer should be avoided
since
there are no controls over its use. Until a better solution can be
implemented, probably the safest thing to do is add a struct
iommu_ops
pointer to struct device_node. However, assuming that only a small
portion of nodes will actually have iommu_ops set, I'd rather see a
separate registry that matches device_nodes to iommu_ops.
Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and
adapt it as needed? It should be exactly what we need to start
out and can be extended and generalized later.
Sure, I'll add this to my list of stuff to do for 3.20.
Does that mean the we don't get any of the patches for 3.19 despite the
Acks?
Hmm, I don't know how useful they are without the get/set ops and I don't
think I can get those ready for 3.19 given where we currently are.
Grant's suggestion of adding an iommu_ops pointer to device_node would work
as a temporary hack, but anything more advanced is going to need proper
review.
Right. I guess it doesn't hurt much if we put the new pointer inside
#ifdef CONFIG_OF_IOMMU, then at least there is no significant size
increase in most DT based platforms.
Yes, I can live with that hack on the proviso that it will be removed by v3.20
Oh, and please put an ugly /* */ comment block in the #ifdef
CONFIG_OF_IOMMU section that makes it really clear that it is an ugly
hack and will be removed in the next release. I don't want anyone
getting ideas that adding pointers to struct device_node is a good
idea.
g.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thu, Dec 04, 2014 at 11:25:35AM +, Grant Likely wrote:
On Thu, Dec 4, 2014 at 11:19 AM, Arnd Bergmann a...@arndb.de wrote:
On Thursday 04 December 2014 10:21:27 Will Deacon wrote:
Sure, I'll add this to my list of stuff to do for 3.20.
Does that mean the we don't get any of the patches for 3.19 despite the
Acks?
Hmm, I don't know how useful they are without the get/set ops and I don't
think I can get those ready for 3.19 given where we currently are.
Grant's suggestion of adding an iommu_ops pointer to device_node would work
as a temporary hack, but anything more advanced is going to need proper
review.
Right. I guess it doesn't hurt much if we put the new pointer inside
#ifdef CONFIG_OF_IOMMU, then at least there is no significant size
increase in most DT based platforms.
Yes, I can live with that hack on the proviso that it will be removed by v3.20
Oh, and please put an ugly /* */ comment block in the #ifdef
CONFIG_OF_IOMMU section that makes it really clear that it is an ugly
hack and will be removed in the next release. I don't want anyone
getting ideas that adding pointers to struct device_node is a good
idea.
Something like the mess below?
Will
---8
diff --git a/include/linux/of.h b/include/linux/of.h
index 29f0adc5f3e4..6f85c02bc1a6 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -43,6 +43,9 @@ struct property {
#if defined(CONFIG_SPARC)
struct of_irq_controller;
#endif
+#ifdef CONFIG_OF_IOMMU
+struct iommu_ops;
+#endif
struct device_node {
const char *name;
@@ -65,6 +68,19 @@ struct device_node {
unsigned int unique_id;
struct of_irq_controller *irq_trans;
#endif
+#ifdef CONFIG_OF_IOMMU
+/*
+ * HACK! HACK! HACK!
+ *
+ * This is a temporary hack to associate a device_node for an
+ * IOMMU with its set of iommu_ops so that we can probe its upstream DMA
+ * masters on the platform bus by parsing the iommus property directly.
+ *
+ * This is going away in 3.20. Please use the of_iommu_{get,set}_ops
+ * functions to get hold of this data.
+ */
+ struct iommu_ops *__iommu_ops_use_accessors;
+#endif
};
#define MAX_PHANDLE_ARGS 16
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index d03abbb11c34..392ec5f212db 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -14,6 +14,17 @@ extern int of_get_dma_window(struct device_node *dn, const
char *prefix,
extern void of_iommu_init(void);
extern struct iommu_ops *of_iommu_configure(struct device *dev);
+static inline void of_iommu_set_ops(struct device_node *np,
+ const struct iommu_ops *ops)
+{
+ np-__iommu_ops_use_accessors = ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+ return np-__iommu_ops_use_accessors;
+}
+
#else
static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
@@ -29,19 +40,15 @@ static inline struct iommu_ops *of_iommu_configure(struct
device *dev)
return NULL;
}
-#endif /* CONFIG_OF_IOMMU */
-
static inline void of_iommu_set_ops(struct device_node *np,
- const struct iommu_ops *ops)
-{
- np-data = (struct iommu_ops *)ops;
-}
-
+ const struct iommu_ops *ops) { }
static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
{
- return np-data;
+ return NULL;
}
+#endif /* CONFIG_OF_IOMMU */
+
extern struct of_device_id __iommu_of_table;
typedef int (*of_iommu_init_fn)(struct device_node *);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
Hi Arnd,
On 03/12/14 19:57, Arnd Bergmann wrote:
[...]
Good catch. This is not good. The data pointer should be avoided since
there are no controls over its use. Until a better solution can be
implemented, probably the safest thing to do is add a struct iommu_ops
pointer to struct device_node. However, assuming that only a small
portion of nodes will actually have iommu_ops set, I'd rather see a
separate registry that matches device_nodes to iommu_ops.
Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and
adapt it as needed? It should be exactly what we need to start
out and can be extended and generalized later.
I'm quite keen to see this series go in, since I'm depending on it to
make arm64 IOMMU DMA ops just work. Will and I came to the conclusion
the other day that we pretty much need to build up some kind of bus
abstraction based on the probe data in order to be able to assign IOMMU
groups correctly, which can also subsume this particular problem in the
long run. Since I've made a start on that already, I've hacked the
following short-term fix out of it. Tested on my Juno - admittedly with
only two SMMUs and one master (EHCI) being probed, but it didn't blow up
or regress anything.
Regards,
Robin.
---8---
From 1f3d2612682c239e53f2c20e2ac5d19ef3f5387c Mon Sep 17 00:00:00 2001
Message-Id:
1f3d2612682c239e53f2c20e2ac5d19ef3f5387c.1417695078.git.robin.mur...@arm.com
From: Robin Murphy robin.mur...@arm.com
Date: Thu, 4 Dec 2014 11:53:13 +
Subject: [PATCH] iommu: store DT-probed IOMMU data privately
Since the data pointer in the DT node is public and may be overwritten
by conflicting code, move the DT-probed IOMMU ops to a private list
where they will be safe.
Signed-off-by: Robin Murphy robin.mur...@arm.com
---
drivers/iommu/of_iommu.c | 38 ++
include/linux/of_iommu.h | 12 ++--
2 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 73236d3..5cd451c 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -94,6 +94,44 @@ 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 hlist_node list;
+ struct device_node *np;
+ const struct iommu_ops *ops;
+};
+static HLIST_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 = kmalloc(sizeof(*iommu), GFP_KERNEL);
+
+ if (!iommu)
+ return;
+
+ INIT_HLIST_NODE(iommu-list);
+ iommu-np = np;
+ iommu-ops = ops;
+ spin_lock(of_iommu_lock);
+ hlist_add_head(iommu-list, of_iommu_list);
+ spin_unlock(of_iommu_lock);
+}
+
+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);
+ hlist_for_each_entry(node, of_iommu_list, list)
+ if (node-np == np) {
+ ops = node-ops;
+ break;
+ }
+ spin_unlock(of_iommu_lock);
+ return (struct iommu_ops *)ops;
+}
+
struct iommu_ops *of_iommu_configure(struct device *dev)
{
struct of_phandle_args iommu_spec;
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index d03abbb..e27c53a 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -31,16 +31,8 @@ static inline struct iommu_ops
*of_iommu_configure(struct device *dev)
#endif /* CONFIG_OF_IOMMU */
-static inline void of_iommu_set_ops(struct device_node *np,
- const struct iommu_ops *ops)
-{
- np-data = (struct iommu_ops *)ops;
-}
-
-static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
-{
- return np-data;
-}
+void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
+struct iommu_ops *of_iommu_get_ops(struct device_node *np);
extern struct of_device_id __iommu_of_table;
--
1.9.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thu, Dec 4, 2014 at 12:26 PM, Robin Murphy robin.mur...@arm.com wrote:
Hi Arnd,
On 03/12/14 19:57, Arnd Bergmann wrote:
[...]
Good catch. This is not good. The data pointer should be avoided since
there are no controls over its use. Until a better solution can be
implemented, probably the safest thing to do is add a struct iommu_ops
pointer to struct device_node. However, assuming that only a small
portion of nodes will actually have iommu_ops set, I'd rather see a
separate registry that matches device_nodes to iommu_ops.
Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and
adapt it as needed? It should be exactly what we need to start
out and can be extended and generalized later.
I'm quite keen to see this series go in, since I'm depending on it to make
arm64 IOMMU DMA ops just work. Will and I came to the conclusion the other
day that we pretty much need to build up some kind of bus abstraction based
on the probe data in order to be able to assign IOMMU groups correctly,
which can also subsume this particular problem in the long run. Since I've
made a start on that already, I've hacked the following short-term fix out
of it. Tested on my Juno - admittedly with only two SMMUs and one master
(EHCI) being probed, but it didn't blow up or regress anything.
Regards,
Robin.
---8---
From 1f3d2612682c239e53f2c20e2ac5d19ef3f5387c Mon Sep 17 00:00:00 2001
Message-Id:
1f3d2612682c239e53f2c20e2ac5d19ef3f5387c.1417695078.git.robin.mur...@arm.com
From: Robin Murphy robin.mur...@arm.com
Date: Thu, 4 Dec 2014 11:53:13 +
Subject: [PATCH] iommu: store DT-probed IOMMU data privately
Since the data pointer in the DT node is public and may be overwritten
by conflicting code, move the DT-probed IOMMU ops to a private list
where they will be safe.
Signed-off-by: Robin Murphy robin.mur...@arm.com
Looks reasonable to me. Comments below...
---
drivers/iommu/of_iommu.c | 38 ++
include/linux/of_iommu.h | 12 ++--
2 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 73236d3..5cd451c 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -94,6 +94,44 @@ 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 hlist_node list;
+ struct device_node *np;
+ const struct iommu_ops *ops;
+};
+static HLIST_HEAD(of_iommu_list);
Just use a list_head. hlist_head merely saves one pointer in this case.
+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 = kmalloc(sizeof(*iommu), GFP_KERNEL);
kzalloc()
+
+ if (!iommu)
+ return;
Shouldn't there be a WARN() here on failure? I don't think failing
silently is desired.
+
+ INIT_HLIST_NODE(iommu-list);
+ iommu-np = np;
+ iommu-ops = ops;
+ spin_lock(of_iommu_lock);
+ hlist_add_head(iommu-list, of_iommu_list);
+ spin_unlock(of_iommu_lock);
+}
+
+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);
+ hlist_for_each_entry(node, of_iommu_list, list)
+ if (node-np == np) {
+ ops = node-ops;
+ break;
+ }
+ spin_unlock(of_iommu_lock);
+ return (struct iommu_ops *)ops;
The cast looks fishy. If you need to use a cast, then the data types
are probably wrong. If you drop the const from *ops here and in the
structure then it should probably work fine. Due to the way it is
being used, there isn't any advantage to using const (unless you
changes of_iommu_get_ops() to return a const pointer, then const would
make sense).
+}
+
struct iommu_ops *of_iommu_configure(struct device *dev)
{
struct of_phandle_args iommu_spec;
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index d03abbb..e27c53a 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -31,16 +31,8 @@ static inline struct iommu_ops *of_iommu_configure(struct
device *dev)
#endif /* CONFIG_OF_IOMMU */
-static inline void of_iommu_set_ops(struct device_node *np,
- const struct iommu_ops *ops)
-{
- np-data = (struct iommu_ops *)ops;
-}
-
-static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
-{
- return np-data;
-}
+void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
+struct iommu_ops *of_iommu_get_ops(struct device_node *np);
extern struct of_device_id __iommu_of_table;
--
1.9.1
___
iommu mailing list
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thu, Dec 4, 2014 at 11:52 AM, Will Deacon will.dea...@arm.com wrote:
On Thu, Dec 04, 2014 at 11:25:35AM +, Grant Likely wrote:
On Thu, Dec 4, 2014 at 11:19 AM, Arnd Bergmann a...@arndb.de wrote:
On Thursday 04 December 2014 10:21:27 Will Deacon wrote:
Sure, I'll add this to my list of stuff to do for 3.20.
Does that mean the we don't get any of the patches for 3.19 despite the
Acks?
Hmm, I don't know how useful they are without the get/set ops and I don't
think I can get those ready for 3.19 given where we currently are.
Grant's suggestion of adding an iommu_ops pointer to device_node would
work
as a temporary hack, but anything more advanced is going to need proper
review.
Right. I guess it doesn't hurt much if we put the new pointer inside
#ifdef CONFIG_OF_IOMMU, then at least there is no significant size
increase in most DT based platforms.
Yes, I can live with that hack on the proviso that it will be removed by
v3.20
Oh, and please put an ugly /* */ comment block in the #ifdef
CONFIG_OF_IOMMU section that makes it really clear that it is an ugly
hack and will be removed in the next release. I don't want anyone
getting ideas that adding pointers to struct device_node is a good
idea.
Something like the mess below?
Yes... Although it looks like Robin's patch does what is needed
without the hack.
g.
Will
---8
diff --git a/include/linux/of.h b/include/linux/of.h
index 29f0adc5f3e4..6f85c02bc1a6 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -43,6 +43,9 @@ struct property {
#if defined(CONFIG_SPARC)
struct of_irq_controller;
#endif
+#ifdef CONFIG_OF_IOMMU
+struct iommu_ops;
+#endif
struct device_node {
const char *name;
@@ -65,6 +68,19 @@ struct device_node {
unsigned int unique_id;
struct of_irq_controller *irq_trans;
#endif
+#ifdef CONFIG_OF_IOMMU
+/*
+ * HACK! HACK! HACK!
+ *
+ * This is a temporary hack to associate a device_node for an
+ * IOMMU with its set of iommu_ops so that we can probe its upstream DMA
+ * masters on the platform bus by parsing the iommus property directly.
+ *
+ * This is going away in 3.20. Please use the of_iommu_{get,set}_ops
+ * functions to get hold of this data.
+ */
+ struct iommu_ops *__iommu_ops_use_accessors;
+#endif
};
#define MAX_PHANDLE_ARGS 16
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index d03abbb11c34..392ec5f212db 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -14,6 +14,17 @@ extern int of_get_dma_window(struct device_node *dn, const
char *prefix,
extern void of_iommu_init(void);
extern struct iommu_ops *of_iommu_configure(struct device *dev);
+static inline void of_iommu_set_ops(struct device_node *np,
+ const struct iommu_ops *ops)
+{
+ np-__iommu_ops_use_accessors = ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+ return np-__iommu_ops_use_accessors;
+}
+
#else
static inline int of_get_dma_window(struct device_node *dn, const char
*prefix,
@@ -29,19 +40,15 @@ static inline struct iommu_ops *of_iommu_configure(struct
device *dev)
return NULL;
}
-#endif /* CONFIG_OF_IOMMU */
-
static inline void of_iommu_set_ops(struct device_node *np,
- const struct iommu_ops *ops)
-{
- np-data = (struct iommu_ops *)ops;
-}
-
+ const struct iommu_ops *ops) { }
static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
{
- return np-data;
+ return NULL;
}
+#endif /* CONFIG_OF_IOMMU */
+
extern struct of_device_id __iommu_of_table;
typedef int (*of_iommu_init_fn)(struct device_node *);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thursday 04 December 2014 12:26:58 Robin Murphy wrote:
+struct of_iommu_node {
+ struct hlist_node list;
+ struct device_node *np;
+ const struct iommu_ops *ops;
+};
+static HLIST_HEAD(of_iommu_list);
+static DEFINE_SPINLOCK(of_iommu_lock);
Looks good to me. For 3.20, I would hope to get consensus on renaming this
structure to 'struct iommmu' and adding additional members into it as needed,
but let's do that another day. Please address Grant's feedback and send
a new version.
Arnd
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
Hi Grant, thanks for the advice - silly micro-optimisations removed, and
I'll make a note to do so from my in-development code, too ;)
I didn't much like the casting either, so rather than push it elsewhere
or out to the caller I've just changed the prototype to obviate it
completely. Since we're also expecting to churn this again to use
something more suitable than iommu_ops as the private data, I think
keeping things simple wins over const-correctness for now.
Thanks,
Robin
---8---
From b2e8c91ac49bef4008661e4628cd6b7249d84af5 Mon Sep 17 00:00:00 2001
Message-Id:
b2e8c91ac49bef4008661e4628cd6b7249d84af5.1417698001.git.robin.mur...@arm.com
From: Robin Murphy robin.mur...@arm.com
Date: Thu, 4 Dec 2014 11:53:13 +
Subject: [PATCH v2] iommu: store DT-probed IOMMU data privately
Since the data pointer in the DT node is public and may be overwritten
by conflicting code, move the DT-probed IOMMU ops to a private list
where they will be safe.
Signed-off-by: Robin Murphy robin.mur...@arm.com
---
drivers/iommu/of_iommu.c | 40
include/linux/of_iommu.h | 12 ++--
2 files changed, 42 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 73236d3..c7078f6 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -94,6 +94,46 @@ 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;
+ 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, struct iommu_ops *ops)
+{
+ struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
+
+ if (!iommu) {
+ __WARN();
+ return;
+ }
+
+ 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);
+}
+
+struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+ struct of_iommu_node *node;
+ 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;
+}
+
struct iommu_ops *of_iommu_configure(struct device *dev)
{
struct of_phandle_args iommu_spec;
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index d03abbb..16c7554 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -31,16 +31,8 @@ static inline struct iommu_ops
*of_iommu_configure(struct device *dev)
#endif /* CONFIG_OF_IOMMU */
-static inline void of_iommu_set_ops(struct device_node *np,
- const struct iommu_ops *ops)
-{
- np-data = (struct iommu_ops *)ops;
-}
-
-static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
-{
- return np-data;
-}
+void of_iommu_set_ops(struct device_node *np, struct iommu_ops *ops);
+struct iommu_ops *of_iommu_get_ops(struct device_node *np);
extern struct of_device_id __iommu_of_table;
--
1.9.1
On 04/12/14 12:42, Grant Likely wrote:
On Thu, Dec 4, 2014 at 12:26 PM, Robin Murphy robin.mur...@arm.com wrote:
Hi Arnd,
On 03/12/14 19:57, Arnd Bergmann wrote:
[...]
Good catch. This is not good. The data pointer should be avoided since
there are no controls over its use. Until a better solution can be
implemented, probably the safest thing to do is add a struct iommu_ops
pointer to struct device_node. However, assuming that only a small
portion of nodes will actually have iommu_ops set, I'd rather see a
separate registry that matches device_nodes to iommu_ops.
Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and
adapt it as needed? It should be exactly what we need to start
out and can be extended and generalized later.
I'm quite keen to see this series go in, since I'm depending on it to make
arm64 IOMMU DMA ops just work. Will and I came to the conclusion the other
day that we pretty much need to build up some kind of bus abstraction based
on the probe data in order to be able to assign IOMMU groups correctly,
which can also subsume this particular problem in the long run. Since I've
made a start on that already, I've hacked the following short-term fix out
of it. Tested on my Juno - admittedly with only two SMMUs and one master
(EHCI) being probed, but it didn't blow up or regress anything.
Regards,
Robin.
---8---
From 1f3d2612682c239e53f2c20e2ac5d19ef3f5387c Mon Sep 17 00:00:00 2001
Message-Id:
1f3d2612682c239e53f2c20e2ac5d19ef3f5387c.1417695078.git.robin.mur...@arm.com
From: Robin Murphy robin.mur...@arm.com
Date: Thu, 4 Dec 2014
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Thu, Dec 4, 2014 at 1:43 PM, Robin Murphy robin.mur...@arm.com wrote:
Hi Grant, thanks for the advice - silly micro-optimisations removed, and
I'll make a note to do so from my in-development code, too ;)
I didn't much like the casting either, so rather than push it elsewhere or
out to the caller I've just changed the prototype to obviate it completely.
Since we're also expecting to churn this again to use something more
suitable than iommu_ops as the private data, I think keeping things simple
wins over const-correctness for now.
Thanks,
Robin
---8---
From b2e8c91ac49bef4008661e4628cd6b7249d84af5 Mon Sep 17 00:00:00 2001
Message-Id:
b2e8c91ac49bef4008661e4628cd6b7249d84af5.1417698001.git.robin.mur...@arm.com
From: Robin Murphy robin.mur...@arm.com
Date: Thu, 4 Dec 2014 11:53:13 +
Subject: [PATCH v2] iommu: store DT-probed IOMMU data privately
Since the data pointer in the DT node is public and may be overwritten
by conflicting code, move the DT-probed IOMMU ops to a private list
where they will be safe.
Signed-off-by: Robin Murphy robin.mur...@arm.com
Acked-by: Grant Likely grant.lik...@linaro.org
---
drivers/iommu/of_iommu.c | 40
include/linux/of_iommu.h | 12 ++--
2 files changed, 42 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 73236d3..c7078f6 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -94,6 +94,46 @@ 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;
+ 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, struct iommu_ops *ops)
+{
+ struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
+
+ if (!iommu) {
+ __WARN();
+ return;
+ }
+
+ 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);
+}
+
+struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+ struct of_iommu_node *node;
+ 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;
+}
+
struct iommu_ops *of_iommu_configure(struct device *dev)
{
struct of_phandle_args iommu_spec;
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index d03abbb..16c7554 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -31,16 +31,8 @@ static inline struct iommu_ops *of_iommu_configure(struct
device *dev)
#endif /* CONFIG_OF_IOMMU */
-static inline void of_iommu_set_ops(struct device_node *np,
- const struct iommu_ops *ops)
-{
- np-data = (struct iommu_ops *)ops;
-}
-
-static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
-{
- return np-data;
-}
+void of_iommu_set_ops(struct device_node *np, struct iommu_ops *ops);
+struct iommu_ops *of_iommu_get_ops(struct device_node *np);
extern struct of_device_id __iommu_of_table;
--
1.9.1
On 04/12/14 12:42, Grant Likely wrote:
On Thu, Dec 4, 2014 at 12:26 PM, Robin Murphy robin.mur...@arm.com
wrote:
Hi Arnd,
On 03/12/14 19:57, Arnd Bergmann wrote:
[...]
Good catch. This is not good. The data pointer should be avoided since
there are no controls over its use. Until a better solution can be
implemented, probably the safest thing to do is add a struct iommu_ops
pointer to struct device_node. However, assuming that only a small
portion of nodes will actually have iommu_ops set, I'd rather see a
separate registry that matches device_nodes to iommu_ops.
Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and
adapt it as needed? It should be exactly what we need to start
out and can be extended and generalized later.
I'm quite keen to see this series go in, since I'm depending on it to
make
arm64 IOMMU DMA ops just work. Will and I came to the conclusion the
other
day that we pretty much need to build up some kind of bus abstraction
based
on the probe data in order to be able to assign IOMMU groups correctly,
which can also subsume this particular problem in the long run. Since
I've
made a start on that already, I've hacked the following short-term fix
out
of it. Tested on my Juno - admittedly with only two SMMUs and one master
(EHCI) being probed, but it didn't blow up or regress anything.
Regards,
Robin.
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
Hi Grant,
On 04/12/14 17:58, Grant Likely wrote:
[...]
+struct of_iommu_node {
+ struct list_head list;
+ struct device_node *np;
+ struct iommu_ops *ops;
Why can't this be const? Why would anyone ever need to modify it? Also
drivers do define their iommu_ops structures const these days, so you'll
either still have to cast away at some point or the compiler will
complain once you start calling this from drivers.
...whereas if we make everything const then the drivers that _do_ want to
use the private data introduced by this series and thus pass mutable
per-instance iommu_ops will be the ones having to cast. We have no users
either way until this series is merged, and the need to stash the
per-instance data somewhere other than np-data is in the way of getting it
merged - this is just a quick hack to address that. I think we've already
agreed that mutable per-instance iommu_ops holding private data aren't
particularly pleasant and will (hopefully) go away in the next iteration[1],
at which point this all changes anyway.
Do you expect drivers to modify that *priv pointer after the ops
structure is registered? I'd be very surprised if that was the use
case. It's fine for the driver to register a non-const version, but
once it is registered, the infrastructure can treat it as const from
then on.
Possibly not - certainly my current port of the ARM SMMU which makes use
of *priv is only ever reading it - although we did also wave around
reasons for mutable ops like dynamically changing the pgsize_bitmap and
possibly even swizzling individual ops for runtime reconfiguration. On
consideration though, I'd agree that things like that are mad enough to
stay well within individual drivers if they did ever happen, and
certainly shouldn't apply to this bit of the infrastructure at any rate.
Here's a respin the other way - making the get/set infrastructure fully
const and fixing up the callsite in of_iommu_configure instead to avoid
the warning. Trying to chase const-correctness beyond that quickly got
too invasive and out of scope for this fix - I'm just keen to get the
merge-blocker out of the way.
Regards,
Robin.
---8---
From 9eba5081aaf4fa8ed5158675a6e622be11a64ae2 Mon Sep 17 00:00:00 2001
Message-Id:
9eba5081aaf4fa8ed5158675a6e622be11a64ae2.1417719305.git.robin.mur...@arm.com
From: Robin Murphy robin.mur...@arm.com
Date: Thu, 4 Dec 2014 11:53:13 +
Subject: [PATCH v3] iommu: store DT-probed IOMMU data privately
Since the data pointer in the DT node is public and may be overwritten
by conflicting code, move the DT-probed IOMMU ops to a private list
where they will be safe.
Signed-off-by: Robin Murphy robin.mur...@arm.com
---
drivers/iommu/of_iommu.c | 40 +++-
include/linux/of_iommu.h | 12 ++--
2 files changed, 41 insertions(+), 11 deletions(-)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 73236d3..39f581f 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -94,6 +94,44 @@ 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;
+
+ 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;
+}
+
struct iommu_ops *of_iommu_configure(struct device *dev)
{
struct of_phandle_args iommu_spec;
@@ -110,7 +148,7 @@ struct iommu_ops *of_iommu_configure(struct device *dev)
#iommu-cells, idx,
iommu_spec)) {
np = iommu_spec.np;
- ops = of_iommu_get_ops(np);
+ ops = (struct iommu_ops *)of_iommu_get_ops(np);
if (!ops || !ops-of_xlate || ops-of_xlate(dev, iommu_spec))
goto err_put_node;
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index d03abbb..83f6d0b 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -31,16 +31,8 @@ static inline struct
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Tuesday 02 December 2014 14:16:57 Grant Likely wrote:
On Mon, Dec 1, 2014 at 11:54 PM, Rob Herring robherri...@gmail.com wrote:
On Mon, Dec 1, 2014 at 10:57 AM, Will Deacon will.dea...@arm.com wrote:
+static inline void of_iommu_set_ops(struct device_node *np,
+ const struct iommu_ops *ops)
+{
+ np-data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+ return np-data;
+}
This may collide with other users. While use of it is rare, PPC uses
it in its PCI code. The OF_DYNAMIC code frees it but never actually
sets it. There may be some coming usage with the DT overlay code or
that's just a bug. Pantelis or Grant can comment. If not, I think we
really should try to get rid of this pointer rather than expand it's
usage.
I didn't see a user of this. I'm guessing that is coming in a SMMU patch?
Good catch. This is not good. The data pointer should be avoided since
there are no controls over its use. Until a better solution can be
implemented, probably the safest thing to do is add a struct iommu_ops
pointer to struct device_node. However, assuming that only a small
portion of nodes will actually have iommu_ops set, I'd rather see a
separate registry that matches device_nodes to iommu_ops.
Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and
adapt it as needed? It should be exactly what we need to start
out and can be extended and generalized later.
ARnd
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
Hello,
On 2014-12-02 00:54, Rob Herring wrote:
Adding Grant and Pantelis...
On Mon, Dec 1, 2014 at 10:57 AM, Will Deacon will.dea...@arm.com wrote:
IOMMU drivers must be initialised before any of their upstream devices,
otherwise the relevant iommu_ops won't be configured for the bus in
question. To solve this, a number of IOMMU drivers use initcalls to
initialise the driver before anything has a chance to be probed.
Whilst this solves the immediate problem, it leaves the job of probing
the IOMMU completely separate from the iommu_ops to configure the IOMMU,
which are called on a per-bus basis and require the driver to figure out
exactly which instance of the IOMMU is being requested. In particular,
the add_device callback simply passes a struct device to the driver,
which then has to parse firmware tables or probe buses to identify the
relevant IOMMU instance.
This patch takes the first step in addressing this problem by adding an
early initialisation pass for IOMMU drivers, giving them the ability to
store some per-instance data in their iommu_ops structure and store that
in their of_node. This can later be used when parsing OF masters to
identify the IOMMU instance in question.
Acked-by: Arnd Bergmann a...@arndb.de
Acked-by: Joerg Roedel jroe...@suse.de
Acked-by: Marek Szyprowski m.szyprow...@samsung.com
Tested-by: Robin Murphy robin.mur...@arm.com
Signed-off-by: Will Deacon will.dea...@arm.com
---
drivers/iommu/of_iommu.c | 17 +
include/asm-generic/vmlinux.lds.h | 2 ++
include/linux/iommu.h | 2 ++
include/linux/of_iommu.h | 25 +
4 files changed, 46 insertions(+)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e550ccb7634e..89b903406968 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -22,6 +22,9 @@
#include linux/of.h
#include linux/of_iommu.h
+static const struct of_device_id __iommu_of_table_sentinel
+ __used __section(__iommu_of_table_end);
+
/**
* of_get_dma_window - Parse *dma-window property and returns 0 if found.
*
@@ -89,3 +92,17 @@ int of_get_dma_window(struct device_node *dn, const char
*prefix, int index,
return 0;
}
EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+void __init of_iommu_init(void)
+{
+ struct device_node *np;
+ const struct of_device_id *match, *matches = __iommu_of_table;
+
+ for_each_matching_node_and_match(np, matches, match) {
+ const of_iommu_init_fn init_fn = match-data;
+
+ if (init_fn(np))
+ pr_err(Failed to initialise IOMMU %s\n,
+ of_node_full_name(np));
+ }
+}
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index aa70cbda327c..bee5d683074d 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -164,6 +164,7 @@
#define CLKSRC_OF_TABLES() OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
#define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
#define CLK_OF_TABLES()OF_TABLE(CONFIG_COMMON_CLK, clk)
+#define IOMMU_OF_TABLES() OF_TABLE(CONFIG_OF_IOMMU, iommu)
#define RESERVEDMEM_OF_TABLES()OF_TABLE(CONFIG_OF_RESERVED_MEM,
reservedmem)
#define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method)
#define EARLYCON_OF_TABLES() OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
@@ -497,6 +498,7 @@
CLK_OF_TABLES() \
RESERVEDMEM_OF_TABLES() \
CLKSRC_OF_TABLES() \
+ IOMMU_OF_TABLES() \
CPU_METHOD_OF_TABLES() \
KERNEL_DTB()\
IRQCHIP_OF_MATCH_TABLE()\
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e6a7c9ff72f2..7b83f9f8e11d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -103,6 +103,7 @@ enum iommu_attr {
* @domain_get_attr: Query domain attributes
* @domain_set_attr: Change domain attributes
* @pgsize_bitmap: bitmap of supported page sizes
+ * @priv: per-instance data private to the iommu driver
*/
struct iommu_ops {
bool (*capable)(enum iommu_cap);
@@ -133,6 +134,7 @@ struct iommu_ops {
u32 (*domain_get_windows)(struct iommu_domain *domain);
unsigned long pgsize_bitmap;
+ void *priv;
};
#define IOMMU_GROUP_NOTIFY_ADD_DEVICE 1 /* Device added */
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 51a560f34bca..5762cdc8effe 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -1,12 +1,17 @@
#ifndef __OF_IOMMU_H
#define __OF_IOMMU_H
+#include linux/iommu.h
+#include linux/of.h
+
#ifdef CONFIG_OF_IOMMU
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Tuesday 02 December 2014 10:23:00 Marek Szyprowski wrote:
+static inline void of_iommu_set_ops(struct device_node *np,
+ const struct iommu_ops *ops)
+{
+ np-data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+ return np-data;
+}
This may collide with other users. While use of it is rare, PPC uses
it in its PCI code. The OF_DYNAMIC code frees it but never actually
sets it. There may be some coming usage with the DT overlay code or
that's just a bug. Pantelis or Grant can comment. If not, I think we
really should try to get rid of this pointer rather than expand it's
usage.
I think that for the initial version it is ok to use np-data. When
per-iommu
controller structure is introduced later, it can be reused also for
performing
of_node to iommu_ops lookup, because IOMMU framework will need to keep track
on all such iommu controllers anyway.
Agreed. I think in the long run, we will have a 'struct iommu' that is
added into a global linked list and that contains (among other things)
an iommu_ops pointer and a device_node pointer. The of_iommu_get_ops
function then walks the list to find the right iommu instance.
Arnd
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Tue, Dec 02, 2014 at 09:36:59AM +, Arnd Bergmann wrote:
On Tuesday 02 December 2014 10:23:00 Marek Szyprowski wrote:
+static inline void of_iommu_set_ops(struct device_node *np,
+ const struct iommu_ops *ops)
+{
+ np-data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+ return np-data;
+}
This may collide with other users. While use of it is rare, PPC uses
it in its PCI code. The OF_DYNAMIC code frees it but never actually
sets it. There may be some coming usage with the DT overlay code or
that's just a bug. Pantelis or Grant can comment. If not, I think we
really should try to get rid of this pointer rather than expand it's
usage.
I think that for the initial version it is ok to use np-data. When
per-iommu
controller structure is introduced later, it can be reused also for
performing
of_node to iommu_ops lookup, because IOMMU framework will need to keep track
on all such iommu controllers anyway.
Agreed. I think in the long run, we will have a 'struct iommu' that is
added into a global linked list and that contains (among other things)
an iommu_ops pointer and a device_node pointer. The of_iommu_get_ops
function then walks the list to find the right iommu instance.
Yup, I plan to look at that after Christmas because I need it for the
per-IOMMU instance pgsize_bitmap anyway.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Tue, Dec 02, 2014 at 10:36:59AM +0100, Arnd Bergmann wrote:
On Tuesday 02 December 2014 10:23:00 Marek Szyprowski wrote:
+static inline void of_iommu_set_ops(struct device_node *np,
+ const struct iommu_ops *ops)
+{
+ np-data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+ return np-data;
+}
This may collide with other users. While use of it is rare, PPC uses
it in its PCI code. The OF_DYNAMIC code frees it but never actually
sets it. There may be some coming usage with the DT overlay code or
that's just a bug. Pantelis or Grant can comment. If not, I think we
really should try to get rid of this pointer rather than expand it's
usage.
I think that for the initial version it is ok to use np-data. When
per-iommu
controller structure is introduced later, it can be reused also for
performing
of_node to iommu_ops lookup, because IOMMU framework will need to keep track
on all such iommu controllers anyway.
Agreed. I think in the long run, we will have a 'struct iommu' that is
added into a global linked list and that contains (among other things)
an iommu_ops pointer and a device_node pointer. The of_iommu_get_ops
function then walks the list to find the right iommu instance.
If we end up doing that, how is this any different from what Hiroshi and
I initially proposed over a year ago (and then again earlier this year)?
The only remaining difference would be that this current proposal needs
IOMMUs to be registered even before other devices are instantiated for
no apparent reason other than ordering. The original proposal allowed
the IOMMU driver to register just like any other driver and ordering to
be handled via deferred probing.
Thierry
pgpyAwNEFasfh.pgp
Description: PGP signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
On Mon, Dec 1, 2014 at 11:54 PM, Rob Herring robherri...@gmail.com wrote:
Adding Grant and Pantelis...
On Mon, Dec 1, 2014 at 10:57 AM, Will Deacon will.dea...@arm.com wrote:
IOMMU drivers must be initialised before any of their upstream devices,
otherwise the relevant iommu_ops won't be configured for the bus in
question. To solve this, a number of IOMMU drivers use initcalls to
initialise the driver before anything has a chance to be probed.
Whilst this solves the immediate problem, it leaves the job of probing
the IOMMU completely separate from the iommu_ops to configure the IOMMU,
which are called on a per-bus basis and require the driver to figure out
exactly which instance of the IOMMU is being requested. In particular,
the add_device callback simply passes a struct device to the driver,
which then has to parse firmware tables or probe buses to identify the
relevant IOMMU instance.
This patch takes the first step in addressing this problem by adding an
early initialisation pass for IOMMU drivers, giving them the ability to
store some per-instance data in their iommu_ops structure and store that
in their of_node. This can later be used when parsing OF masters to
identify the IOMMU instance in question.
Acked-by: Arnd Bergmann a...@arndb.de
Acked-by: Joerg Roedel jroe...@suse.de
Acked-by: Marek Szyprowski m.szyprow...@samsung.com
Tested-by: Robin Murphy robin.mur...@arm.com
Signed-off-by: Will Deacon will.dea...@arm.com
---
drivers/iommu/of_iommu.c | 17 +
include/asm-generic/vmlinux.lds.h | 2 ++
include/linux/iommu.h | 2 ++
include/linux/of_iommu.h | 25 +
4 files changed, 46 insertions(+)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e550ccb7634e..89b903406968 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -22,6 +22,9 @@
#include linux/of.h
#include linux/of_iommu.h
+static const struct of_device_id __iommu_of_table_sentinel
+ __used __section(__iommu_of_table_end);
+
/**
* of_get_dma_window - Parse *dma-window property and returns 0 if found.
*
@@ -89,3 +92,17 @@ int of_get_dma_window(struct device_node *dn, const char
*prefix, int index,
return 0;
}
EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+void __init of_iommu_init(void)
+{
+ struct device_node *np;
+ const struct of_device_id *match, *matches = __iommu_of_table;
+
+ for_each_matching_node_and_match(np, matches, match) {
+ const of_iommu_init_fn init_fn = match-data;
+
+ if (init_fn(np))
+ pr_err(Failed to initialise IOMMU %s\n,
+ of_node_full_name(np));
+ }
+}
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index aa70cbda327c..bee5d683074d 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -164,6 +164,7 @@
#define CLKSRC_OF_TABLES() OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
#define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
#define CLK_OF_TABLES()OF_TABLE(CONFIG_COMMON_CLK, clk)
+#define IOMMU_OF_TABLES() OF_TABLE(CONFIG_OF_IOMMU, iommu)
#define RESERVEDMEM_OF_TABLES()OF_TABLE(CONFIG_OF_RESERVED_MEM,
reservedmem)
#define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method)
#define EARLYCON_OF_TABLES() OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
@@ -497,6 +498,7 @@
CLK_OF_TABLES() \
RESERVEDMEM_OF_TABLES() \
CLKSRC_OF_TABLES() \
+ IOMMU_OF_TABLES() \
CPU_METHOD_OF_TABLES() \
KERNEL_DTB()\
IRQCHIP_OF_MATCH_TABLE()\
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e6a7c9ff72f2..7b83f9f8e11d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -103,6 +103,7 @@ enum iommu_attr {
* @domain_get_attr: Query domain attributes
* @domain_set_attr: Change domain attributes
* @pgsize_bitmap: bitmap of supported page sizes
+ * @priv: per-instance data private to the iommu driver
*/
struct iommu_ops {
bool (*capable)(enum iommu_cap);
@@ -133,6 +134,7 @@ struct iommu_ops {
u32 (*domain_get_windows)(struct iommu_domain *domain);
unsigned long pgsize_bitmap;
+ void *priv;
};
#define IOMMU_GROUP_NOTIFY_ADD_DEVICE 1 /* Device added */
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 51a560f34bca..5762cdc8effe 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -1,12 +1,17 @@
#ifndef __OF_IOMMU_H
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
Hi Rob,
On Dec 2, 2014, at 01:54 , Rob Herring robherri...@gmail.com wrote:
Adding Grant and Pantelis...
On Mon, Dec 1, 2014 at 10:57 AM, Will Deacon will.dea...@arm.com wrote:
IOMMU drivers must be initialised before any of their upstream devices,
otherwise the relevant iommu_ops won't be configured for the bus in
question. To solve this, a number of IOMMU drivers use initcalls to
initialise the driver before anything has a chance to be probed.
Whilst this solves the immediate problem, it leaves the job of probing
the IOMMU completely separate from the iommu_ops to configure the IOMMU,
which are called on a per-bus basis and require the driver to figure out
exactly which instance of the IOMMU is being requested. In particular,
the add_device callback simply passes a struct device to the driver,
which then has to parse firmware tables or probe buses to identify the
relevant IOMMU instance.
This patch takes the first step in addressing this problem by adding an
early initialisation pass for IOMMU drivers, giving them the ability to
store some per-instance data in their iommu_ops structure and store that
in their of_node. This can later be used when parsing OF masters to
identify the IOMMU instance in question.
Acked-by: Arnd Bergmann a...@arndb.de
Acked-by: Joerg Roedel jroe...@suse.de
Acked-by: Marek Szyprowski m.szyprow...@samsung.com
Tested-by: Robin Murphy robin.mur...@arm.com
Signed-off-by: Will Deacon will.dea...@arm.com
---
drivers/iommu/of_iommu.c | 17 +
include/asm-generic/vmlinux.lds.h | 2 ++
include/linux/iommu.h | 2 ++
include/linux/of_iommu.h | 25 +
4 files changed, 46 insertions(+)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e550ccb7634e..89b903406968 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -22,6 +22,9 @@
#include linux/of.h
#include linux/of_iommu.h
+static const struct of_device_id __iommu_of_table_sentinel
+ __used __section(__iommu_of_table_end);
+
/**
* of_get_dma_window - Parse *dma-window property and returns 0 if found.
*
@@ -89,3 +92,17 @@ int of_get_dma_window(struct device_node *dn, const char
*prefix, int index,
return 0;
}
EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+void __init of_iommu_init(void)
+{
+ struct device_node *np;
+ const struct of_device_id *match, *matches = __iommu_of_table;
+
+ for_each_matching_node_and_match(np, matches, match) {
+ const of_iommu_init_fn init_fn = match-data;
+
+ if (init_fn(np))
+ pr_err(Failed to initialise IOMMU %s\n,
+ of_node_full_name(np));
+ }
+}
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index aa70cbda327c..bee5d683074d 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -164,6 +164,7 @@
#define CLKSRC_OF_TABLES() OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
#define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
#define CLK_OF_TABLES()OF_TABLE(CONFIG_COMMON_CLK, clk)
+#define IOMMU_OF_TABLES() OF_TABLE(CONFIG_OF_IOMMU, iommu)
#define RESERVEDMEM_OF_TABLES()OF_TABLE(CONFIG_OF_RESERVED_MEM,
reservedmem)
#define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method)
#define EARLYCON_OF_TABLES() OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
@@ -497,6 +498,7 @@
CLK_OF_TABLES() \
RESERVEDMEM_OF_TABLES() \
CLKSRC_OF_TABLES() \
+ IOMMU_OF_TABLES() \
CPU_METHOD_OF_TABLES() \
KERNEL_DTB()\
IRQCHIP_OF_MATCH_TABLE()\
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e6a7c9ff72f2..7b83f9f8e11d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -103,6 +103,7 @@ enum iommu_attr {
* @domain_get_attr: Query domain attributes
* @domain_set_attr: Change domain attributes
* @pgsize_bitmap: bitmap of supported page sizes
+ * @priv: per-instance data private to the iommu driver
*/
struct iommu_ops {
bool (*capable)(enum iommu_cap);
@@ -133,6 +134,7 @@ struct iommu_ops {
u32 (*domain_get_windows)(struct iommu_domain *domain);
unsigned long pgsize_bitmap;
+ void *priv;
};
#define IOMMU_GROUP_NOTIFY_ADD_DEVICE 1 /* Device added */
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 51a560f34bca..5762cdc8effe 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -1,12 +1,17 @@
#ifndef __OF_IOMMU_H
#define __OF_IOMMU_H
[PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
IOMMU drivers must be initialised before any of their upstream devices,
otherwise the relevant iommu_ops won't be configured for the bus in
question. To solve this, a number of IOMMU drivers use initcalls to
initialise the driver before anything has a chance to be probed.
Whilst this solves the immediate problem, it leaves the job of probing
the IOMMU completely separate from the iommu_ops to configure the IOMMU,
which are called on a per-bus basis and require the driver to figure out
exactly which instance of the IOMMU is being requested. In particular,
the add_device callback simply passes a struct device to the driver,
which then has to parse firmware tables or probe buses to identify the
relevant IOMMU instance.
This patch takes the first step in addressing this problem by adding an
early initialisation pass for IOMMU drivers, giving them the ability to
store some per-instance data in their iommu_ops structure and store that
in their of_node. This can later be used when parsing OF masters to
identify the IOMMU instance in question.
Acked-by: Arnd Bergmann a...@arndb.de
Acked-by: Joerg Roedel jroe...@suse.de
Acked-by: Marek Szyprowski m.szyprow...@samsung.com
Tested-by: Robin Murphy robin.mur...@arm.com
Signed-off-by: Will Deacon will.dea...@arm.com
---
drivers/iommu/of_iommu.c | 17 +
include/asm-generic/vmlinux.lds.h | 2 ++
include/linux/iommu.h | 2 ++
include/linux/of_iommu.h | 25 +
4 files changed, 46 insertions(+)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e550ccb7634e..89b903406968 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -22,6 +22,9 @@
#include linux/of.h
#include linux/of_iommu.h
+static const struct of_device_id __iommu_of_table_sentinel
+ __used __section(__iommu_of_table_end);
+
/**
* of_get_dma_window - Parse *dma-window property and returns 0 if found.
*
@@ -89,3 +92,17 @@ int of_get_dma_window(struct device_node *dn, const char
*prefix, int index,
return 0;
}
EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+void __init of_iommu_init(void)
+{
+ struct device_node *np;
+ const struct of_device_id *match, *matches = __iommu_of_table;
+
+ for_each_matching_node_and_match(np, matches, match) {
+ const of_iommu_init_fn init_fn = match-data;
+
+ if (init_fn(np))
+ pr_err(Failed to initialise IOMMU %s\n,
+ of_node_full_name(np));
+ }
+}
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index aa70cbda327c..bee5d683074d 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -164,6 +164,7 @@
#define CLKSRC_OF_TABLES() OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
#define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
#define CLK_OF_TABLES()OF_TABLE(CONFIG_COMMON_CLK, clk)
+#define IOMMU_OF_TABLES() OF_TABLE(CONFIG_OF_IOMMU, iommu)
#define RESERVEDMEM_OF_TABLES()OF_TABLE(CONFIG_OF_RESERVED_MEM,
reservedmem)
#define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method)
#define EARLYCON_OF_TABLES() OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
@@ -497,6 +498,7 @@
CLK_OF_TABLES() \
RESERVEDMEM_OF_TABLES() \
CLKSRC_OF_TABLES() \
+ IOMMU_OF_TABLES() \
CPU_METHOD_OF_TABLES() \
KERNEL_DTB()\
IRQCHIP_OF_MATCH_TABLE()\
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e6a7c9ff72f2..7b83f9f8e11d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -103,6 +103,7 @@ enum iommu_attr {
* @domain_get_attr: Query domain attributes
* @domain_set_attr: Change domain attributes
* @pgsize_bitmap: bitmap of supported page sizes
+ * @priv: per-instance data private to the iommu driver
*/
struct iommu_ops {
bool (*capable)(enum iommu_cap);
@@ -133,6 +134,7 @@ struct iommu_ops {
u32 (*domain_get_windows)(struct iommu_domain *domain);
unsigned long pgsize_bitmap;
+ void *priv;
};
#define IOMMU_GROUP_NOTIFY_ADD_DEVICE 1 /* Device added */
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 51a560f34bca..5762cdc8effe 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -1,12 +1,17 @@
#ifndef __OF_IOMMU_H
#define __OF_IOMMU_H
+#include linux/iommu.h
+#include linux/of.h
+
#ifdef CONFIG_OF_IOMMU
extern int of_get_dma_window(struct device_node *dn, const char *prefix,
int index, unsigned long *busno, dma_addr_t *addr,
Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers
Adding Grant and Pantelis...
On Mon, Dec 1, 2014 at 10:57 AM, Will Deacon will.dea...@arm.com wrote:
IOMMU drivers must be initialised before any of their upstream devices,
otherwise the relevant iommu_ops won't be configured for the bus in
question. To solve this, a number of IOMMU drivers use initcalls to
initialise the driver before anything has a chance to be probed.
Whilst this solves the immediate problem, it leaves the job of probing
the IOMMU completely separate from the iommu_ops to configure the IOMMU,
which are called on a per-bus basis and require the driver to figure out
exactly which instance of the IOMMU is being requested. In particular,
the add_device callback simply passes a struct device to the driver,
which then has to parse firmware tables or probe buses to identify the
relevant IOMMU instance.
This patch takes the first step in addressing this problem by adding an
early initialisation pass for IOMMU drivers, giving them the ability to
store some per-instance data in their iommu_ops structure and store that
in their of_node. This can later be used when parsing OF masters to
identify the IOMMU instance in question.
Acked-by: Arnd Bergmann a...@arndb.de
Acked-by: Joerg Roedel jroe...@suse.de
Acked-by: Marek Szyprowski m.szyprow...@samsung.com
Tested-by: Robin Murphy robin.mur...@arm.com
Signed-off-by: Will Deacon will.dea...@arm.com
---
drivers/iommu/of_iommu.c | 17 +
include/asm-generic/vmlinux.lds.h | 2 ++
include/linux/iommu.h | 2 ++
include/linux/of_iommu.h | 25 +
4 files changed, 46 insertions(+)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e550ccb7634e..89b903406968 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -22,6 +22,9 @@
#include linux/of.h
#include linux/of_iommu.h
+static const struct of_device_id __iommu_of_table_sentinel
+ __used __section(__iommu_of_table_end);
+
/**
* of_get_dma_window - Parse *dma-window property and returns 0 if found.
*
@@ -89,3 +92,17 @@ int of_get_dma_window(struct device_node *dn, const char
*prefix, int index,
return 0;
}
EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+void __init of_iommu_init(void)
+{
+ struct device_node *np;
+ const struct of_device_id *match, *matches = __iommu_of_table;
+
+ for_each_matching_node_and_match(np, matches, match) {
+ const of_iommu_init_fn init_fn = match-data;
+
+ if (init_fn(np))
+ pr_err(Failed to initialise IOMMU %s\n,
+ of_node_full_name(np));
+ }
+}
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index aa70cbda327c..bee5d683074d 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -164,6 +164,7 @@
#define CLKSRC_OF_TABLES() OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
#define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
#define CLK_OF_TABLES()OF_TABLE(CONFIG_COMMON_CLK, clk)
+#define IOMMU_OF_TABLES() OF_TABLE(CONFIG_OF_IOMMU, iommu)
#define RESERVEDMEM_OF_TABLES()OF_TABLE(CONFIG_OF_RESERVED_MEM,
reservedmem)
#define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method)
#define EARLYCON_OF_TABLES() OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
@@ -497,6 +498,7 @@
CLK_OF_TABLES() \
RESERVEDMEM_OF_TABLES() \
CLKSRC_OF_TABLES() \
+ IOMMU_OF_TABLES() \
CPU_METHOD_OF_TABLES() \
KERNEL_DTB()\
IRQCHIP_OF_MATCH_TABLE()\
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e6a7c9ff72f2..7b83f9f8e11d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -103,6 +103,7 @@ enum iommu_attr {
* @domain_get_attr: Query domain attributes
* @domain_set_attr: Change domain attributes
* @pgsize_bitmap: bitmap of supported page sizes
+ * @priv: per-instance data private to the iommu driver
*/
struct iommu_ops {
bool (*capable)(enum iommu_cap);
@@ -133,6 +134,7 @@ struct iommu_ops {
u32 (*domain_get_windows)(struct iommu_domain *domain);
unsigned long pgsize_bitmap;
+ void *priv;
};
#define IOMMU_GROUP_NOTIFY_ADD_DEVICE 1 /* Device added */
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 51a560f34bca..5762cdc8effe 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -1,12 +1,17 @@
#ifndef __OF_IOMMU_H
#define __OF_IOMMU_H
+#include linux/iommu.h
+#include linux/of.h
+
