RFC: extend iommu-map binding to support #iommu-cells

-----Original Message-----
From: Stuart Yoder
Sent: Friday, December 16, 2016 8:07 AM
foundation.org
Subject: RFC: extend iommu-map binding to support #iommu-cells > 1
https://www.spinics.net/lists/arm-kernel/msg539066.html
The existing iommu-map binding did not account for the situation where
#iommu-cells == 2, as permitted in the ARM SMMU binding. The 2nd cell of
the IOMMU specifier being the SMR mask. The existing binding defines the
Any RID r in the interval [rid-base, rid-base + length) is associated with
the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
...and that does not work if iommu-base is 2 cells, the second being the SMR
mask.
While this can be worked around by always having length=1, it seems we can
get this cleaned up by updating the binding definition for iommu-map.
See patch below. Thoughts?
Thanks,
Stuart
-------------------------------------------------------------------------
diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt
b/Documentation/devicetree/bindings/pci/pci-iommu.txt
index 56c8296..e81b461 100644
--- a/Documentation/devicetree/bindings/pci/pci-iommu.txt
+++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
@@ -38,8 +38,20 @@ Optional properties
The property is an arbitrary number of tuples of
(rid-base,iommu,iommu-base,length).
- Any RID r in the interval [rid-base, rid-base + length) is associated with
- the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
+ If the associated IOMMU has an #iommu-cells value of 1, any RID r in
+ the interval [rid-base, rid-base + length) is associated with the
+ listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
+
+ The ARM SMMU binding permits an #iommu-cells value of 2 and in this
+ case defines an IOMMU specifier to be: (stream-id,smr-mask)
+
+ (rid-base,iommu,[stream-id,smr-mask],length).
+
+ any RID r in the interval [rid-base, rid-base + length) is associated
+ with the listed IOMMU, with the iommu-specifier (r - rid-base + stream-
id).

Should not this be (r - rid-base + (stream-id & smr-mask)) ?

So basically stream-id ranges from (stream-id & smr-mask) - (stream-id & smr-mask + (length - 1) )

Thanks
-Bharat

- iommu-map-mask: A mask to be applied to each Requester ID prior to being
mapped to an iommu-specifier per the iommu-map property.

Stuart Yoder

2016-12-16 15:06:04 UTC

-----Original Message-----
From: Bharat Bhushan
Sent: Thursday, December 15, 2016 9:46 PM
Subject: RE: RFC: extend iommu-map binding to support #iommu-cells > 1

Should not this be (r - rid-base + (stream-id & smr-mask)) ?

No, the SMR mask is not applied here-- that is programmed in the SMMU
hardware and not applied by software. The SMR mask in the case of NXP
is 0x7C00, and so is the inverse of a typical mask of 1 bits.

If the map was defined as: <0x0 &smmu 0x10 0x7c00 10>;

An RID value of 0x2 would get mapped to the 2-cell specifier: <0x12 0x7c00>

Stuart

Robin Murphy

2016-12-16 11:34:46 UTC

Post by Stuart Yoder
https://www.spinics.net/lists/arm-kernel/msg539066.html
The existing iommu-map binding did not account for the situation where
#iommu-cells == 2, as permitted in the ARM SMMU binding. The 2nd cell
of the IOMMU specifier being the SMR mask. The existing binding defines
Any RID r in the interval [rid-base, rid-base + length) is associated with
the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
...and that does not work if iommu-base is 2 cells, the second being the
SMR mask.
While this can be worked around by always having length=1, it seems we
can get this cleaned up by updating the binding definition for iommu-map.
See patch below. Thoughts?

I really don't think defining a generic binding to have a magic
non-standard meaning for one specific use-case is the right way to go.

Give me a moment to spin the patch I reckon you actually want...

Robin.

Post by Stuart Yoder
Thanks,
Stuart
-------------------------------------------------------------------------
diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt b/Documentation/devicetree/bindings/pci/pci-iommu.txt
index 56c8296..e81b461 100644
--- a/Documentation/devicetree/bindings/pci/pci-iommu.txt
+++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
@@ -38,8 +38,20 @@ Optional properties
The property is an arbitrary number of tuples of
(rid-base,iommu,iommu-base,length).
- Any RID r in the interval [rid-base, rid-base + length) is associated with
- the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
+ If the associated IOMMU has an #iommu-cells value of 1, any RID r in the
+ interval [rid-base, rid-base + length) is associated with the listed IOMMU,
+ with the iommu-specifier (r - rid-base + iommu-base).
+
+ The ARM SMMU binding permits an #iommu-cells value of 2 and in this
+ case defines an IOMMU specifier to be: (stream-id,smr-mask)
+
+ (rid-base,iommu,[stream-id,smr-mask],length).
+
+ any RID r in the interval [rid-base, rid-base + length) is associated
+ with the listed IOMMU, with the iommu-specifier (r - rid-base + stream-id).
- iommu-map-mask: A mask to be applied to each Requester ID prior to being
mapped to an iommu-specifier per the iommu-map property.
_______________________________________________
iommu mailing list
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Mark Rutland

2016-12-16 11:39:14 UTC

To reiterate, I'm very much not keen on the pci-iommu binding having
knowledge of the decomposition of iommu-specifiers from other bindings.

As mentioned previously, there's an intended interpretation [1] that we
need to fix up the pci-iommu binding with. With that, I don't think it's
even necessary to bodge iommu-cells = <1> AFAICT.

I'm sorry that the patch I suggested never materialized; let me figure
out the wording now...

Thanks,
Mark.

[1] https://www.spinics.net/lists/arm-kernel/msg539357.html

Post by Stuart Yoder
See patch below. Thoughts?
Thanks,
Stuart
-------------------------------------------------------------------------
diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt b/Documentation/devicetree/bindings/pci/pci-iommu.txt
index 56c8296..e81b461 100644
--- a/Documentation/devicetree/bindings/pci/pci-iommu.txt
+++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
@@ -38,8 +38,20 @@ Optional properties
The property is an arbitrary number of tuples of
(rid-base,iommu,iommu-base,length).
- Any RID r in the interval [rid-base, rid-base + length) is associated with
- the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
+ If the associated IOMMU has an #iommu-cells value of 1, any RID r in the
+ interval [rid-base, rid-base + length) is associated with the listed IOMMU,
+ with the iommu-specifier (r - rid-base + iommu-base).
+
+ The ARM SMMU binding permits an #iommu-cells value of 2 and in this
+ case defines an IOMMU specifier to be: (stream-id,smr-mask)
+
+ (rid-base,iommu,[stream-id,smr-mask],length).
+
+ any RID r in the interval [rid-base, rid-base + length) is associated
+ with the listed IOMMU, with the iommu-specifier (r - rid-base + stream-id).
- iommu-map-mask: A mask to be applied to each Requester ID prior to being
mapped to an iommu-specifier per the iommu-map property.

Stuart Yoder

2016-12-16 14:21:14 UTC

-----Original Message-----
Sent: Friday, December 16, 2016 5:39 AM
Subject: Re: RFC: extend iommu-map binding to support #iommu-cells > 1

https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Farm-

kernel%2Fmsg539066.html&data=01%7C01%7Cstuart.yoder%40nxp.com%7C0464e12bddfd42e0f0a508d425a847cb%7C686ea
1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=KnzeOlbts271GwD1cpx2FLMsKnxw%2FOdiGVTp6%2BKFrbM%3D&reserved=0

The existing iommu-map binding did not account for the situation where
#iommu-cells == 2, as permitted in the ARM SMMU binding. The 2nd cell
of the IOMMU specifier being the SMR mask. The existing binding defines
Any RID r in the interval [rid-base, rid-base + length) is associated with
the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
...and that does not work if iommu-base is 2 cells, the second being the
SMR mask.
While this can be worked around by always having length=1, it seems we
can get this cleaned up by updating the binding definition for iommu-map.

To reiterate, I'm very much not keen on the pci-iommu binding having
knowledge of the decomposition of iommu-specifiers from other bindings.

With the current definition of iommu-map we already have baked in an
assumption that an iommu-specifier is a value that can be incremented
by 1 to get to the next sequential specifier. So the binding already
has "knowledge" that applies in most, but not all cases.

The generic iommu binding also defines a case where #iommu-cells=4
for some IOMMUs.

Since the ARM SMMU is a special case, perhaps the intepretation
of an iommu-specifier in the context of iommu-map should be moved
into the SMMU binding.

As mentioned previously, there's an intended interpretation [1] that we
need to fix up the pci-iommu binding with. With that, I don't think it's
even necessary to bodge iommu-cells = <1> AFAICT.
I had intended that the offset was added to the final cell of the
iommu-specifier (i.e. that the iommu-specifier was treated as a large
number).
You can handle this case by adding additional entries in the map table,
with a single entry length.

I understand that, and it works, but I don't see why the definition has
to be that the offset is added to the "final cell". Why can't it be
an iommu specific definition that makes sense for a given IOMMU?

Stuart

Robin Murphy

2016-12-16 14:50:43 UTC

-----Original Message-----
Sent: Friday, December 16, 2016 5:39 AM
Subject: Re: RFC: extend iommu-map binding to support #iommu-cells > 1

https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Farm-

To reiterate, I'm very much not keen on the pci-iommu binding having
knowledge of the decomposition of iommu-specifiers from other bindings.

With the current definition of iommu-map we already have baked in an
assumption that an iommu-specifier is a value that can be incremented
by 1 to get to the next sequential specifier. So the binding already
has "knowledge" that applies in most, but not all cases.
The generic iommu binding also defines a case where #iommu-cells=4
for some IOMMUs.
Since the ARM SMMU is a special case, perhaps the intepretation
of an iommu-specifier in the context of iommu-map should be moved
into the SMMU binding.

I understand that, and it works, but I don't see why the definition has
to be that the offset is added to the "final cell".

Because the cells of a specifier form a single opaque big-endian value.
Were DT little-endian it would be the "first cell". To be pedantic, both
of those descriptions are technically wrong because they fail to account
for overflow and carry up into the next cell (in whichever direction).

Post by Stuart Yoder
Why can't it be
an iommu specific definition that makes sense for a given IOMMU?

Because the implementation would then become a nightmarish rabbit-warren
of special-cases, largely defeating the point of a *generic* binding. At
the very least it'd have to chase every phandle and determine its
compatible just to work out what to do with any given specifier - merely
thinking about the complexity of the error handling for the number of
additional failure modes that introduces is enough to put me off.

Robin.

Post by Stuart Yoder
Stuart

Bharat Bhushan

2016-12-16 15:07:44 UTC

-----Original Message-----
Sent: Friday, December 16, 2016 8:21 PM
Subject: Re: RFC: extend iommu-map binding to support #iommu-cells > 1

-----Original Message-----
Sent: Friday, December 16, 2016 5:39 AM
Subject: Re: RFC: extend iommu-map binding to support #iommu-cells > 1

https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw

ww.spinics.net%2Flists%2Farm-

kernel%2Fmsg539066.html&data=01%7C01%7Cstuart.yoder%40nxp.com%7C
0464e

12bddfd42e0f0a508d425a847cb%7C686ea

1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=KnzeOlbts271GwD1cpx2FLMsKn
xw%2F

OdiGVTp6%2BKFrbM%3D&reserved=0

base).

...and that does not work if iommu-base is 2 cells, the second being
the SMR mask.
While this can be worked around by always having length=1, it seems
we can get this cleaned up by updating the binding definition for iommu-

map.

To reiterate, I'm very much not keen on the pci-iommu binding having
knowledge of the decomposition of iommu-specifiers from other

bindings.

Post by Stuart Yoder
With the current definition of iommu-map we already have baked in an
assumption that an iommu-specifier is a value that can be incremented
by 1 to get to the next sequential specifier. So the binding already
has "knowledge" that applies in most, but not all cases.
The generic iommu binding also defines a case where #iommu-cells=4 for
some IOMMUs.
Since the ARM SMMU is a special case, perhaps the intepretation of an
iommu-specifier in the context of iommu-map should be moved into the
SMMU binding.

As mentioned previously, there's an intended interpretation [1] that
we need to fix up the pci-iommu binding with. With that, I don't
think it's even necessary to bodge iommu-cells = <1> AFAICT.
I had intended that the offset was added to the final cell of the
iommu-specifier (i.e. that the iommu-specifier was treated as a large
number).
You can handle this case by adding additional entries in the map table,
with a single entry length.

I understand that, and it works, but I don't see why the definition
has to be that the offset is added to the "final cell".

Because the cells of a specifier form a single opaque big-endian value.
Were DT little-endian it would be the "first cell". To be pedantic, both of
those descriptions are technically wrong because they fail to account for
overflow and carry up into the next cell (in whichever direction).

Post by Stuart Yoder
Why can't it be
an iommu specific definition that makes sense for a given IOMMU?

Because the implementation would then become a nightmarish rabbit-
warren of special-cases, largely defeating the point of a *generic* binding. At
the very least it'd have to chase every phandle and determine its compatible
just to work out what to do with any given specifier - merely thinking about
the complexity of the error handling for the number of additional failure
modes that introduces is enough to put me off.

Currently if iommu-cells = 2 then SMMU treats the 2 cells as stream-id + stream-id mask. While DT binding is silent on how 2 cells, which means that actually stream-id could be more than 32bit, but SMMU uses it in the way it wants to, which is not correct. If we be more explicit on defining the meaning of two cells than it will avoid confusion and implementation will match the DT binding definition.

Thanks
-Bharat

Robin.

Post by Stuart Yoder
Stuart

Stuart Yoder

2016-12-16 15:56:21 UTC

Post by Mark Rutland

Post by Stuart Yoder
The existing iommu-map binding did not account for the situation where
#iommu-cells == 2, as permitted in the ARM SMMU binding. The 2nd cell
of the IOMMU specifier being the SMR mask. The existing binding defines
Any RID r in the interval [rid-base, rid-base + length) is associated with
the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
...and that does not work if iommu-base is 2 cells, the second being the
SMR mask.
While this can be worked around by always having length=1, it seems we
can get this cleaned up by updating the binding definition for iommu-map.

To reiterate, I'm very much not keen on the pci-iommu binding having
knowledge of the decomposition of iommu-specifiers from other bindings.

With the current definition of iommu-map we already have baked in an
assumption that an iommu-specifier is a value that can be incremented
by 1 to get to the next sequential specifier. So the binding already
has "knowledge" that applies in most, but not all cases.
The generic iommu binding also defines a case where #iommu-cells=4
for some IOMMUs.
Since the ARM SMMU is a special case, perhaps the intepretation
of an iommu-specifier in the context of iommu-map should be moved
into the SMMU binding.

Post by Mark Rutland
As mentioned previously, there's an intended interpretation [1] that we
need to fix up the pci-iommu binding with. With that, I don't think it's
even necessary to bodge iommu-cells = <1> AFAICT.
I had intended that the offset was added to the final cell of the
iommu-specifier (i.e. that the iommu-specifier was treated as a large
number).
You can handle this case by adding additional entries in the map table,
with a single entry length.

I understand that, and it works, but I don't see why the definition has
to be that the offset is added to the "final cell".

If it is really opaque how can we reliably add 1 to it?

Post by Stuart Yoder
Why can't it be
an iommu specific definition that makes sense for a given IOMMU?

In order to decode an iommu-map at all we have to chase every phandle, no?
Isn't that how we know how many cells an iommu-map entry has?

I have not thought through the implementation, but my thought was that
the code could use a callback to handle the IOMMU-specific case. If
callback==NULL then the default case is what we have today. An IOMMU like
the SMMU can implement a simple callback that can return the mapping.

Not sure how this is that much harder than any other map, such as interrupt-map.

Stuart

Robin Murphy

2016-12-16 16:49:36 UTC

Post by Mark Rutland

To reiterate, I'm very much not keen on the pci-iommu binding having
knowledge of the decomposition of iommu-specifiers from other bindings.

With the current definition of iommu-map we already have baked in an
assumption that an iommu-specifier is a value that can be incremented
by 1 to get to the next sequential specifier. So the binding already
has "knowledge" that applies in most, but not all cases.
The generic iommu binding also defines a case where #iommu-cells=4
for some IOMMUs.
Since the ARM SMMU is a special case, perhaps the intepretation
of an iommu-specifier in the context of iommu-map should be moved
into the SMMU binding.

I understand that, and it works, but I don't see why the definition has
to be that the offset is added to the "final cell".

If it is really opaque how can we reliably add 1 to it?

The pci-iommu binding isn't adding 1 to an opaque value. It is
*generating* an IOMMU specifier of the form of a single numeric value,
as defined by some linear translation of a PCI RID relative to a numeric
base value appropriate for the IOMMU topology. It is explicit therein
that a single numeric value must be the appropriate interpretation of
that specifier. That happens to be true of a 1-cell arm-smmu specifier,
therefore iommu-map can be used with arm-smmu with #iommu-cells=1. It is
also true of a 2-cell some-other-iommu specifier, therefore iommu-map
can be used with some-other-iommu with #iommu-cells=2 (if we fix the
fact that the current implementation fails to consider more than one
cell). It is not, however, true of a 2-cell arm-smmu specifier,
therefore iommu-map cannot be used with arm-smmu with #iommu-cells=2,
although the fact that of_pci_iommu_configure() unconditionally
generates a 1-cell specifier at the moment does happen to sidestep that.

The point of the 2-cell arm-smmu specifier is really to handle the
devices (which exist) with dozens or hundreds of stream IDs, which are
*only* usable via SMR masking, and particularly with a hand-crafted mask
that is able to assume the non-existence of overlapping IDs (that aspect
being actually quite significant for optimal allocation - one of the
reasons my automatic-mask-generation prototype is now gathering dust).

The MMU-500 TBU use-case is really an entirely different kettle of fish,
hence the RFC I posted earlier.

Robin.

Post by Stuart Yoder
Why can't it be
an iommu specific definition that makes sense for a given IOMMU?

In order to decode an iommu-map at all we have to chase every phandle, no?
Isn't that how we know how many cells an iommu-map entry has?
I have not thought through the implementation, but my thought was that
the code could use a callback to handle the IOMMU-specific case. If
callback==NULL then the default case is what we have today. An IOMMU like
the SMMU can implement a simple callback that can return the mapping.
Not sure how this is that much harder than any other map, such as interrupt-map.
Stuart

Stuart Yoder

2016-12-16 17:05:43 UTC

-----Original Message-----
Sent: Friday, December 16, 2016 10:50 AM
Subject: Re: RFC: extend iommu-map binding to support #iommu-cells > 1