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

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

[Stuart Yoder]

For context, please see the thread:
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 mapping as:
   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).
+
+  ARM SMMU Note:
+    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)
+
+    In an iommu-map this means the iommu-base consists of 2 cells:
+        (rid-base,iommu,[stream-id,smr-mask],length).
+
+    In this case the RID to IOMMU specifier mapping is defined to be:
+    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.

RE: RFC: extend iommu-map binding to support #iommu-cells > 1

[Bharat Bhushan]

> -----Original Message-----
> From: Stuart Yoder
> Sent: Friday, December 16, 2016 8:07 AM
> To: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>; robin.murphy-5wv7dgnIgG8@public.gmane.org;
> will.deacon-5wv7dgnIgG8@public.gmane.org
> Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Bharat Bhushan <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>;
> Nipun Gupta <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; Diana Madalina Craciun
> <diana.craciun-3arQi8VN3Tc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; iommu-cunTk1MwBs/ROKNJybVBZg@public.gmane.org
> foundation.org
> Subject: RFC: extend iommu-map binding to support #iommu-cells > 1
> 
> For context, please see the thread:
> 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
> mapping as:
>    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).
> +
> +  ARM SMMU Note:
> +    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)
> +
> +    In an iommu-map this means the iommu-base consists of 2 cells:
> +        (rid-base,iommu,[stream-id,smr-mask],length).
> +
> +    In this case the RID to IOMMU specifier mapping is defined to be:
> +    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.
> 
> 

RE: RFC: extend iommu-map binding to support #iommu-cells > 1

[Stuart Yoder]

> -----Original Message-----
> From: Bharat Bhushan
> Sent: Thursday, December 15, 2016 9:46 PM
> To: Stuart Yoder <stuart.yoder-3arQi8VN3Tc@public.gmane.org>; Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>; robin.murphy-5wv7dgnIgG8@public.gmane.org;
> will.deacon-5wv7dgnIgG8@public.gmane.org
> Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Nipun Gupta <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; Diana Madalina Craciun
> <diana.craciun-3arQi8VN3Tc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Subject: RE: RFC: extend iommu-map binding to support #iommu-cells > 1
> 
> 
> 
> > -----Original Message-----
> > From: Stuart Yoder
> > Sent: Friday, December 16, 2016 8:07 AM
> > To: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>; robin.murphy-5wv7dgnIgG8@public.gmane.org;
> > will.deacon-5wv7dgnIgG8@public.gmane.org
> > Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Bharat Bhushan <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>;
> > Nipun Gupta <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; Diana Madalina Craciun
> > <diana.craciun-3arQi8VN3Tc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; iommu-cunTk1MwBs/ROKNJybVBZg@public.gmane.org
> > foundation.org
> > Subject: RFC: extend iommu-map binding to support #iommu-cells > 1
> >
> > For context, please see the thread:
> > 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
> > mapping as:
> >    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).
> > +
> > +  ARM SMMU Note:
> > +    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)
> > +
> > +    In an iommu-map this means the iommu-base consists of 2 cells:
> > +        (rid-base,iommu,[stream-id,smr-mask],length).
> > +
> > +    In this case the RID to IOMMU specifier mapping is defined to be:
> > +    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)) ?

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

Re: RFC: extend iommu-map binding to support #iommu-cells > 1

[Robin Murphy]

On 16/12/16 02:36, Stuart Yoder wrote:
> For context, please see the thread:
> 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 mapping as:
>    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.

> 
> 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).
> +
> +  ARM SMMU Note:
> +    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)
> +
> +    In an iommu-map this means the iommu-base consists of 2 cells:
> +        (rid-base,iommu,[stream-id,smr-mask],length).
> +
> +    In this case the RID to IOMMU specifier mapping is defined to be:
> +    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
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
> 

Re: RFC: extend iommu-map binding to support #iommu-cells > 1

[Mark Rutland]

On Fri, Dec 16, 2016 at 02:36:57AM +0000, Stuart Yoder wrote:
> For context, please see the thread:
> 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 mapping as:
>    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.

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

> 
> 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).
> +
> +  ARM SMMU Note:
> +    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)
> +
> +    In an iommu-map this means the iommu-base consists of 2 cells:
> +        (rid-base,iommu,[stream-id,smr-mask],length).
> +
> +    In this case the RID to IOMMU specifier mapping is defined to be:
> +    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.
> 
> 
> 

RE: RFC: extend iommu-map binding to support #iommu-cells > 1

[Stuart Yoder]

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland-5wv7dgnIgG8@public.gmane.org]
> Sent: Friday, December 16, 2016 5:39 AM
> To: Stuart Yoder <stuart.yoder-3arQi8VN3Tc@public.gmane.org>
> Cc: robin.murphy-5wv7dgnIgG8@public.gmane.org; will.deacon-5wv7dgnIgG8@public.gmane.org; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Bharat Bhushan
> <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>; Nipun Gupta <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; Diana Madalina Craciun
> <diana.craciun-3arQi8VN3Tc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Subject: Re: RFC: extend iommu-map binding to support #iommu-cells > 1
> 
> On Fri, Dec 16, 2016 at 02:36:57AM +0000, Stuart Yoder wrote:
> > For context, please see the thread:
> > 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
> > the mapping as:
> >    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.

You had said in the previous thread:

  > 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

Re: RFC: extend iommu-map binding to support #iommu-cells > 1

[Robin Murphy]

On 16/12/16 14:21, Stuart Yoder wrote:
> 
> 
>> -----Original Message-----
>> From: Mark Rutland [mailto:mark.rutland-5wv7dgnIgG8@public.gmane.org]
>> Sent: Friday, December 16, 2016 5:39 AM
>> To: Stuart Yoder <stuart.yoder-3arQi8VN3Tc@public.gmane.org>
>> Cc: robin.murphy-5wv7dgnIgG8@public.gmane.org; will.deacon-5wv7dgnIgG8@public.gmane.org; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Bharat Bhushan
>> <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>; Nipun Gupta <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; Diana Madalina Craciun
>> <diana.craciun-3arQi8VN3Tc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>> Subject: Re: RFC: extend iommu-map binding to support #iommu-cells > 1
>>
>> On Fri, Dec 16, 2016 at 02:36:57AM +0000, Stuart Yoder wrote:
>>> For context, please see the thread:
>>> 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
>>> the mapping as:
>>>    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.
> 
> You had said in the previous thread:
> 
>   > 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).

>  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.

> 
> Stuart
> 

RE: RFC: extend iommu-map binding to support #iommu-cells > 1

[Bharat Bhushan]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy-5wv7dgnIgG8@public.gmane.org]
> Sent: Friday, December 16, 2016 8:21 PM
> To: Stuart Yoder <stuart.yoder-3arQi8VN3Tc@public.gmane.org>; Mark Rutland
> <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: will.deacon-5wv7dgnIgG8@public.gmane.org; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Bharat Bhushan
> <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>; Nipun Gupta <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; Diana
> Madalina Craciun <diana.craciun-3arQi8VN3Tc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Subject: Re: RFC: extend iommu-map binding to support #iommu-cells > 1
> 
> On 16/12/16 14:21, Stuart Yoder wrote:
> >
> >
> >> -----Original Message-----
> >> From: Mark Rutland [mailto:mark.rutland-5wv7dgnIgG8@public.gmane.org]
> >> Sent: Friday, December 16, 2016 5:39 AM
> >> To: Stuart Yoder <stuart.yoder-3arQi8VN3Tc@public.gmane.org>
> >> Cc: robin.murphy-5wv7dgnIgG8@public.gmane.org; will.deacon-5wv7dgnIgG8@public.gmane.org;
> robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org;
> >> Bharat Bhushan <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>; Nipun Gupta
> >> <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; Diana Madalina Craciun
> >> <diana.craciun-3arQi8VN3Tc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> >> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> >> Subject: Re: RFC: extend iommu-map binding to support #iommu-cells >
> >> 1
> >>
> >> On Fri, Dec 16, 2016 at 02:36:57AM +0000, Stuart Yoder wrote:
> >>> For context, please see the thread:
> >>>
> 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
> >>>
> >>> 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 mapping as:
> >>>    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.
> >
> > You had said in the previous thread:
> >
> >   > 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).
> 
> >  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.
> 
> >
> > Stuart
> >

RE: RFC: extend iommu-map binding to support #iommu-cells > 1

[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
> >>> the mapping as:
> >>>    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.
> >
> > You had said in the previous thread:
> >
> >   > 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).

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

> >  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.

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

Re: RFC: extend iommu-map binding to support #iommu-cells > 1

[Robin Murphy]

On 16/12/16 15:56, Stuart Yoder wrote:
>>>>> 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 mapping as:
>>>>>    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.
>>>
>>> You had said in the previous thread:
>>>
>>>   > 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).
> 
> 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.

>>>  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.
> 
> 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
> 

RE: RFC: extend iommu-map binding to support #iommu-cells > 1

[Stuart Yoder]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy-5wv7dgnIgG8@public.gmane.org]
> Sent: Friday, December 16, 2016 10:50 AM
> To: Stuart Yoder <stuart.yoder-3arQi8VN3Tc@public.gmane.org>; Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: will.deacon-5wv7dgnIgG8@public.gmane.org; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Bharat Bhushan <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>; Nipun Gupta
> <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; Diana Madalina Craciun <diana.craciun-3arQi8VN3Tc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Subject: Re: RFC: extend iommu-map binding to support #iommu-cells > 1
> 
> On 16/12/16 15:56, Stuart Yoder wrote:
> >>>>> 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 mapping as:
> >>>>>    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.
> >>>
> >>> You had said in the previous thread:
> >>>
> >>>   > 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).
> >
> > 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.

I had not seen your RFC when I replied previously...the global SMR mask
is actually much preferred and more straightforward.  We will test it.

Thanks,
Stuart