Re: [RFC 1/3] iommu/arm-smmu: Add support to opt-in to stalling

On 05/01/17 16:07, Will Deacon wrote:

On Thu, Jan 05, 2017 at 03:32:50PM +0000, Robin Murphy wrote:

quoted

On 05/01/17 14:47, Will Deacon wrote:

quoted

On Thu, Jan 05, 2017 at 02:07:31PM +0000, Mark Rutland wrote:

quoted

Ok. It would be good to elaborate on what "stalling is useable" means in
the property description. i.e. what specificallty the implementation and
integration need to ensure.

We can describe some of those guarantees in the property description, but
it's difficult to enumerate them exhaustively. For example, you wouldn't
want stalling to lead to data corruption, denial of service, or for the
thing to catch fire, but having those as explicit requirements is a bit
daft. It's also impossible to check that you thought of everything.

Aside from renaming the option, I'm really after an opinion on whether
it's better to have one property or combine it with the compatible
string, because I can see benefits of both and don't much care either
way.

The SMMU implementation side of the decision (i.e. independence of IRQ
assertion vs. SS) seems like exactly the sort of stuff the compatible
string already has covered. The integration side I'm less confident can
be described this way at all - the "this device definitely won't go
wrong if stalled for an indefinite length of time" is inherently a
per-master thing, so a single property on the SMMU implying that for
every device connected to it seems a bit optimistic, and breaks down as
soon as you have one device in the system for which that isn't true (a
PCI root complex, say), even if that guy's traffic never crosses paths
with whichever few devices you actually care about using stalls with.

I think this needs to be some kind of "arm,smmu-stall-safe" property
placed on individual master device nodes (mad idea: or even an extra
cell of flags in the IOMMU specifier) to encapsulate both that the given
device itself is OK with being stalled, and that it's integrated in such
a way that its stalled transactions cannot disrupt any *other* device
(e.g. it has a TBU all to itself). Then upon initialising a context bank
on a suitable SMMU implementation, we set CFCFG based on whatever device
is being attached to the corresponding domain, and refuse any subsequent
attempts to attach a non-stallable device to a stalling domain (and
possibly even vice-versa).

If we're going to add per-master properties, I'd *really* like them to be
independent of the IOMMU in use. That is, we should be able to re-use this
property as part of supporting SVM for platform devices in future.

I'd argue that they are still fairly separate things despite the
overlap: stalling is a specific ARM SMMU architecture thing (in both
architectures) which may be used for purposes unrelated to SVM;
conversely SVM implemented via PRI or similar mechanisms should be
pretty much oblivious to the transaction fault model.

But I think we agree that we need:

  1. A compatible string for the SMMU that can be used to infer the SS
     behaviour in the driver

  2. A property on the SMMU to say that it's been integrated in such a
     way that stalling is safe (doesn't deadlock)

That's still got to be a per-master property, not a SMMU property, I
think. To illustrate:

  [A]         [B]   [C]
   |           |_____|
 __|______________|___
| TBU |        | TBU |
|_____|  SMMU  |_____|
|__|______________|__|
   |              |

Say A and B are instances of some device happy to be stalled, and C is a
PCIe RC, and each is attached to their own context bank - enabling
stalls for A is definitely fine. However even though B and C are using
different context banks, enabling stalls for B might deadlock C if it
results in more total outstanding transactions than the TBU's slave port
supports. Therefore A can happily claim to be stall-safe, but B cannot
due to its integration with respect to C.

And yes, I can point you at some existing hardware which really does
posess a topology like that.

  3. A generic master property that says that the device can DMA to
     unpinned memory

That sounds a bit *too* generic to me, given that there are multiple
incompatible ways that could be implemented. I'm not the biggest fan of
properties with heavily context-specific interpretations, especially
when there's more than a hint of software implementation details in the mix.

Robin.

Anything else?

Will