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

[Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>]

On Thu, Jan 05, 2017 at 12:08:57PM +0000, Mark Rutland wrote:
> On Thu, Jan 05, 2017 at 11:55:29AM +0000, Will Deacon wrote:
> > On Tue, Jan 03, 2017 at 04:30:54PM -0500, Rob Clark wrote:
> > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> > > index ef465b0..5f405a6 100644
> > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> > > @@ -68,6 +68,9 @@ conditions.
> > >                    aliases of secure registers have to be used during
> > >                    SMMU configuration.
> > >  
> > > +- arm,smmu-enable-stall : Enable stall mode to stall memory transactions
> > > +                  and resume after fault is handled
> 
> The wording here seems to describe a policy rather than a property.
> 
> Can you elaborate on when/why this is required/preferred/valid?

It's not a policy, it's a hardware capability. There are some non-probeable
reasons why stalling mode is unsafe or unusable:

  http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/474530.html

Some of these are specific to the SMMU implementation (e.g. whether or not
the SS bit can remain set without reasserting the IRQ) and some are specific
to the integration (e.g. whether or not stalling an endpoint can deadlock
the SoC). The key point is that, without support from both the
implementation and the integration, stalls are unusable.

> > >  static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
> > > @@ -824,6 +852,8 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
> > >  
> > >  	/* SCTLR */
> > >  	reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
> > > +	if (smmu->options & ARM_SMMU_OPT_ENABLE_STALL)
> > > +		reg |= SCTLR_CFCFG;
> > 
> > I wonder if this should also be predicated on the compatible string, so
> > that the "arm,smmu-enable-stall" property is ignored (with a warning) if
> > the compatible string isn't specific enough to identify an implementation
> > with the required SS behaviour? On the other hand, it feels pretty
> > redundant and a single "stalling works" property is all we need.
> 
> Can you elaborate on what "stalling works" entails? Is that just the SS
> bit behaviour? are there integration or endpoint-specific things that we
> need to care about?

See above. The "stalling works" property (arm,smmu-enable-stall) would
indicate that both the implementation *and* the integration are such
that stalling is usable for demand paging. I suspect there are endpoints
that can't deal with stalls (e.g. they might timeout and signal a RAS
event), but in that case their respective device drivers should ensure
that any DMA buffers are pinned and/or register a fault handler to
request termination of the faulting transaction.

Will