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

[Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>]

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
>