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

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

[Will Deacon]

On Tue, Jan 03, 2017 at 04:30:54PM -0500, Rob Clark wrote:
> TODO maybe we want two options, one to enable stalling, and 2nd to punt
> handling to wq?  I haven't needed to use mm APIs from fault handler yet
> (although it is something that I think we'll want some day).  Perhaps
> stalling support is limited to just letting driver dump some extra
> debugging information otherwise.  Threaded handling probably only useful
> with stalling, but inverse may not always be true.

I'd actually like to see this stuck on a worker thread, because I think
that's more generally useful and I don't want to have a situation where
sometimes the IOMMU fault notifier is run in IRQ context and sometimes it's
not.

> 
> Signed-off-by: Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/iommu/arm,smmu.txt         |  3 ++
>  drivers/iommu/arm-smmu.c                           | 42 ++++++++++++++++++----
>  2 files changed, 39 insertions(+), 6 deletions(-)
> 
> 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
> +
>  ** Deprecated properties:
>  
>  - mmu-masters (deprecated in favour of the generic "iommus" binding) :
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index d505432..a71cb8f 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -350,6 +350,7 @@ struct arm_smmu_device {
>  	u32				features;
>  
>  #define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)
> +#define ARM_SMMU_OPT_ENABLE_STALL      (1 << 1)
>  	u32				options;
>  	enum arm_smmu_arch_version	version;
>  	enum arm_smmu_implementation	model;
> @@ -425,6 +426,7 @@ static bool using_legacy_binding, using_generic_binding;
>  
>  static struct arm_smmu_option_prop arm_smmu_options[] = {
>  	{ ARM_SMMU_OPT_SECURE_CFG_ACCESS, "calxeda,smmu-secure-config-access" },
> +	{ ARM_SMMU_OPT_ENABLE_STALL,      "arm,smmu-enable-stall" },
>  	{ 0, NULL},
>  };
>  
> @@ -676,7 +678,8 @@ static struct iommu_gather_ops arm_smmu_gather_ops = {
>  
>  static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
>  {
> -	u32 fsr, fsynr;
> +	int flags, ret;
> +	u32 fsr, fsynr, resume;
>  	unsigned long iova;
>  	struct iommu_domain *domain = dev;
>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> @@ -690,15 +693,40 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
>  	if (!(fsr & FSR_FAULT))
>  		return IRQ_NONE;
>  
> +	if (fsr & FSR_IGN)
> +		dev_err_ratelimited(smmu->dev,
> +				    "Unexpected context fault (fsr 0x%x)\n",
> +				    fsr);
> +
>  	fsynr = readl_relaxed(cb_base + ARM_SMMU_CB_FSYNR0);
> -	iova = readq_relaxed(cb_base + ARM_SMMU_CB_FAR);
> +	flags = fsynr & FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ;
>  
> -	dev_err_ratelimited(smmu->dev,
> -	"Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cb=%d\n",
> -			    fsr, iova, fsynr, cfg->cbndx);
> +	iova = readq_relaxed(cb_base + ARM_SMMU_CB_FAR);
> +	if (!report_iommu_fault(domain, smmu->dev, iova, flags)) {
> +		ret = IRQ_HANDLED;
> +		resume = RESUME_RETRY;
> +	} else {
> +		dev_err_ratelimited(smmu->dev,
> +		    "Unhandled context fault: iova=0x%08lx, fsynr=0x%x, cb=%d\n",
> +		    iova, fsynr, cfg->cbndx);
> +		ret = IRQ_NONE;
> +		resume = RESUME_TERMINATE;
> +	}
>  
> +	/* Clear the faulting FSR */
>  	writel(fsr, cb_base + ARM_SMMU_CB_FSR);
> -	return IRQ_HANDLED;
> +
> +	/* Retry or terminate any stalled transactions */
> +	if (fsr & FSR_SS) {
> +		/* Should we care about ending up w/ a stalled transaction
> +		 * when we didn't ask for it?  I guess for now best to call
> +		 * attention to it and resume anyways.
> +		 */
> +		WARN_ON(!(smmu->options & ARM_SMMU_OPT_ENABLE_STALL));

I don't think we need to care about this. If we're getting stall faults
with CFCFG clear, then something has gone drastically wrong in the hardware
and we'll probably see "Unhandled context fault" anyway.

> +		writel_relaxed(resume, cb_base + ARM_SMMU_CB_RESUME);
> +	}
> +
> +	return ret;
>  }
>  
>  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.

I added the devicetree folks to CC for an opinion..

Will

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

[Mark Rutland]

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:
> > TODO maybe we want two options, one to enable stalling, and 2nd to punt
> > handling to wq?  I haven't needed to use mm APIs from fault handler yet
> > (although it is something that I think we'll want some day).  Perhaps
> > stalling support is limited to just letting driver dump some extra
> > debugging information otherwise.  Threaded handling probably only useful
> > with stalling, but inverse may not always be true.
> 
> I'd actually like to see this stuck on a worker thread, because I think
> that's more generally useful and I don't want to have a situation where
> sometimes the IOMMU fault notifier is run in IRQ context and sometimes it's
> not.
> 
> > 
> > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > ---
> >  .../devicetree/bindings/iommu/arm,smmu.txt         |  3 ++
> >  drivers/iommu/arm-smmu.c                           | 42 ++++++++++++++++++----
> >  2 files changed, 39 insertions(+), 6 deletions(-)
> > 
> > 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?

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

Thanks,
Mark.

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

[Will Deacon]

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

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

[Mark Rutland]

On Thu, Jan 05, 2017 at 02:00:05PM +0000, Will Deacon wrote:
> 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

Ok. My point was that the wording above is an imperative -- it tells
the kernel to enable stall mode, not if/why it is safe to do so (i.e. it
is a policy, not a property).

It sounds like that's just a wording issue. Something like
"arm,stalling-is-usable" (along with a descrition of when that
can/should be in the DT) would be vastly better.

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

Ok.

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

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.

Thanks,
Mark.

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

[Will Deacon]

On Thu, Jan 05, 2017 at 02:07:31PM +0000, Mark Rutland wrote:
> On Thu, Jan 05, 2017 at 02:00:05PM +0000, Will Deacon wrote:
> > 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
> 
> Ok. My point was that the wording above is an imperative -- it tells
> the kernel to enable stall mode, not if/why it is safe to do so (i.e. it
> is a policy, not a property).
> 
> It sounds like that's just a wording issue. Something like
> "arm,stalling-is-usable" (along with a descrition of when that
> can/should be in the DT) would be vastly better.

Why does it need a vendor prefix? I'm not down on the convention there.
"stalling-safe" or "stalling-supported" are alternative strings.

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

Will

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

[Robin Murphy]

On 05/01/17 14:47, Will Deacon wrote:
> On Thu, Jan 05, 2017 at 02:07:31PM +0000, Mark Rutland wrote:
>> On Thu, Jan 05, 2017 at 02:00:05PM +0000, Will Deacon wrote:
>>> 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
>>
>> Ok. My point was that the wording above is an imperative -- it tells
>> the kernel to enable stall mode, not if/why it is safe to do so (i.e. it
>> is a policy, not a property).
>>
>> It sounds like that's just a wording issue. Something like
>> "arm,stalling-is-usable" (along with a descrition of when that
>> can/should be in the DT) would be vastly better.
> 
> Why does it need a vendor prefix? I'm not down on the convention there.
> "stalling-safe" or "stalling-supported" are alternative strings.
> 
>>>>>>  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.
>>
>> 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).

Robin.

> 
> Will
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
> 

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

[Will Deacon]

On Thu, Jan 05, 2017 at 03:32:50PM +0000, Robin Murphy wrote:
> On 05/01/17 14:47, Will Deacon wrote:
> > On Thu, Jan 05, 2017 at 02:07:31PM +0000, Mark Rutland wrote:
> >> 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.

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)

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

Anything else?

Will

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

[Robin Murphy]

On 05/01/17 16:07, Will Deacon wrote:
> On Thu, Jan 05, 2017 at 03:32:50PM +0000, Robin Murphy wrote:
>> On 05/01/17 14:47, Will Deacon wrote:
>>> On Thu, Jan 05, 2017 at 02:07:31PM +0000, Mark Rutland wrote:
>>>> 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
> 

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

[Will Deacon]

On Thu, Jan 05, 2017 at 05:03:30PM +0000, Robin Murphy wrote:
> On 05/01/17 16:07, Will Deacon wrote:
> > On Thu, Jan 05, 2017 at 03:32:50PM +0000, Robin Murphy wrote:
> >> 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 SVM for a platform device is likely to require working stalls, so
they're highly related concepts. Having the former be a generic property
but the second tied to the SMMU means we have to duplicate information
(that is, we end up with a generic "SVM-capable" property that implies
that stalling works). I don't think we want that at all.

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

So in this case, don't say that B and C can DMA to unpinned memory. You
need the third property. This property (property 2) is concerned with the
SMMU itself because, e.g. the way the walker has been integrated can
cause a deadlock.

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

So what would you propose for SVM? I really want that to be opt-in, so a
per-master flag seems like the right way forward to me.

Will

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

[Rob Clark]

On Thu, Jan 5, 2017 at 12:25 PM, Will Deacon <will.deacon@arm.com> wrote:
>> 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.
>
> So in this case, don't say that B and C can DMA to unpinned memory. You
> need the third property. This property (property 2) is concerned with the
> SMMU itself because, e.g. the way the walker has been integrated can
> cause a deadlock.


fwiw, I guess I'm mostly thinking about case (A)..  but I guess in the
(B) case amend my suggestion about adding param to
iommu_set_fault_handler() slightly to consider the enable_stall param
passed in when both (B) and (C) register their fault handlers?

Or I guess the idea about increasing extra cell (which IIUC would let
us add an extra param in dt in the devices iommus property) could also
work.  Unless maybe there could be some cases where whether a device
can do stalling is also a function of the driver as well (ie. some
feature needs to be implemented type thing)..


BR,
-R

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

[Rob Clark]

On Thu, Jan 5, 2017 at 6:55 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> On Tue, Jan 03, 2017 at 04:30:54PM -0500, Rob Clark wrote:
>> TODO maybe we want two options, one to enable stalling, and 2nd to punt
>> handling to wq?  I haven't needed to use mm APIs from fault handler yet
>> (although it is something that I think we'll want some day).  Perhaps
>> stalling support is limited to just letting driver dump some extra
>> debugging information otherwise.  Threaded handling probably only useful
>> with stalling, but inverse may not always be true.
>
> I'd actually like to see this stuck on a worker thread, because I think
> that's more generally useful and I don't want to have a situation where
> sometimes the IOMMU fault notifier is run in IRQ context and sometimes it's
> not.

So I was talking a bit w/ Jordan on IRC yesterday..  and we also have
the GPU's hw hang-detect to contend with.  So I *suspect* that when we
get to the point of using this to do things like page in things from
swap and resume the faulting transaction, we probably want to get
called immediately from the IRQ handler so we can disable the hw
hang-detect.

I'm not sure if the better solution then would be to have two fault
callbacks, one immediately from the IRQ and a later one from wq.  Or
let the driver handle the wq business and give it a way to tell the
IOMMU when to resume.

I kinda think we should punt on the worker thread for now until we are
ready to resume faulting transactions, because I guess a strong chance
that whatever way we do it now will be wrong ;-)

>>
>> Signed-off-by: Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  .../devicetree/bindings/iommu/arm,smmu.txt         |  3 ++
>>  drivers/iommu/arm-smmu.c                           | 42 ++++++++++++++++++----
>>  2 files changed, 39 insertions(+), 6 deletions(-)
>>
>> 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
>> +
>>  ** Deprecated properties:
>>
>>  - mmu-masters (deprecated in favour of the generic "iommus" binding) :
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index d505432..a71cb8f 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -350,6 +350,7 @@ struct arm_smmu_device {
>>       u32                             features;
>>
>>  #define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)
>> +#define ARM_SMMU_OPT_ENABLE_STALL      (1 << 1)
>>       u32                             options;
>>       enum arm_smmu_arch_version      version;
>>       enum arm_smmu_implementation    model;
>> @@ -425,6 +426,7 @@ static bool using_legacy_binding, using_generic_binding;
>>
>>  static struct arm_smmu_option_prop arm_smmu_options[] = {
>>       { ARM_SMMU_OPT_SECURE_CFG_ACCESS, "calxeda,smmu-secure-config-access" },
>> +     { ARM_SMMU_OPT_ENABLE_STALL,      "arm,smmu-enable-stall" },
>>       { 0, NULL},
>>  };
>>
>> @@ -676,7 +678,8 @@ static struct iommu_gather_ops arm_smmu_gather_ops = {
>>
>>  static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
>>  {
>> -     u32 fsr, fsynr;
>> +     int flags, ret;
>> +     u32 fsr, fsynr, resume;
>>       unsigned long iova;
>>       struct iommu_domain *domain = dev;
>>       struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> @@ -690,15 +693,40 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
>>       if (!(fsr & FSR_FAULT))
>>               return IRQ_NONE;
>>
>> +     if (fsr & FSR_IGN)
>> +             dev_err_ratelimited(smmu->dev,
>> +                                 "Unexpected context fault (fsr 0x%x)\n",
>> +                                 fsr);
>> +
>>       fsynr = readl_relaxed(cb_base + ARM_SMMU_CB_FSYNR0);
>> -     iova = readq_relaxed(cb_base + ARM_SMMU_CB_FAR);
>> +     flags = fsynr & FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ;
>>
>> -     dev_err_ratelimited(smmu->dev,
>> -     "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cb=%d\n",
>> -                         fsr, iova, fsynr, cfg->cbndx);
>> +     iova = readq_relaxed(cb_base + ARM_SMMU_CB_FAR);
>> +     if (!report_iommu_fault(domain, smmu->dev, iova, flags)) {
>> +             ret = IRQ_HANDLED;
>> +             resume = RESUME_RETRY;
>> +     } else {
>> +             dev_err_ratelimited(smmu->dev,
>> +                 "Unhandled context fault: iova=0x%08lx, fsynr=0x%x, cb=%d\n",
>> +                 iova, fsynr, cfg->cbndx);
>> +             ret = IRQ_NONE;
>> +             resume = RESUME_TERMINATE;
>> +     }
>>
>> +     /* Clear the faulting FSR */
>>       writel(fsr, cb_base + ARM_SMMU_CB_FSR);
>> -     return IRQ_HANDLED;
>> +
>> +     /* Retry or terminate any stalled transactions */
>> +     if (fsr & FSR_SS) {
>> +             /* Should we care about ending up w/ a stalled transaction
>> +              * when we didn't ask for it?  I guess for now best to call
>> +              * attention to it and resume anyways.
>> +              */
>> +             WARN_ON(!(smmu->options & ARM_SMMU_OPT_ENABLE_STALL));
>
> I don't think we need to care about this. If we're getting stall faults
> with CFCFG clear, then something has gone drastically wrong in the hardware
> and we'll probably see "Unhandled context fault" anyway.

ok, I can drop the WARN_ON()

>> +             writel_relaxed(resume, cb_base + ARM_SMMU_CB_RESUME);
>> +     }
>> +
>> +     return ret;
>>  }
>>
>>  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.

We could also drop the property and key the behavior on specific
compat strings I guess.  Having both seems a bit odd.  Anyways, I'll
defer to DT folks about what the cleaner approach is.

BR,
-R

> I added the devicetree folks to CC for an opinion..
>
> Will
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Will Deacon]

On Thu, Jan 05, 2017 at 10:27:27AM -0500, Rob Clark wrote:
> On Thu, Jan 5, 2017 at 6:55 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Tue, Jan 03, 2017 at 04:30:54PM -0500, Rob Clark wrote:
> >> TODO maybe we want two options, one to enable stalling, and 2nd to punt
> >> handling to wq?  I haven't needed to use mm APIs from fault handler yet
> >> (although it is something that I think we'll want some day).  Perhaps
> >> stalling support is limited to just letting driver dump some extra
> >> debugging information otherwise.  Threaded handling probably only useful
> >> with stalling, but inverse may not always be true.
> >
> > I'd actually like to see this stuck on a worker thread, because I think
> > that's more generally useful and I don't want to have a situation where
> > sometimes the IOMMU fault notifier is run in IRQ context and sometimes it's
> > not.
> 
> So I was talking a bit w/ Jordan on IRC yesterday..  and we also have
> the GPU's hw hang-detect to contend with.  So I *suspect* that when we
> get to the point of using this to do things like page in things from
> swap and resume the faulting transaction, we probably want to get
> called immediately from the IRQ handler so we can disable the hw
> hang-detect.

Well, if you want to use an SMMU for paging, then the GPU driver would
need to request that explicitly when allocating its DMA buffers, to that
would be the time to either delay or disable the hang detection.

> I'm not sure if the better solution then would be to have two fault
> callbacks, one immediately from the IRQ and a later one from wq.  Or
> let the driver handle the wq business and give it a way to tell the
> IOMMU when to resume.
> 
> I kinda think we should punt on the worker thread for now until we are
> ready to resume faulting transactions, because I guess a strong chance
> that whatever way we do it now will be wrong ;-)

I guess what I'm after is for you to change the interrupt handlers to be
threaded, like they are for SMMUv3. I *think* you can do that with a NULL
thread_fn for now, and just call report_iommu_fault from the handler.
The return value of that could, in theory, be used to queued the paging
request and wake the paging thread in future.

> > 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.
> 
> We could also drop the property and key the behavior on specific
> compat strings I guess.  Having both seems a bit odd.  Anyways, I'll
> defer to DT folks about what the cleaner approach is.

As Robin pointed out, we do need to be able to distinguish the integration
of the device from the device itself. For example, MMU-9000 might be capable
of stalling, but if it's bolted to a PCI RC, it's not safe to do so.

Will

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

[Rob Clark]

On Thu, Jan 5, 2017 at 10:49 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> On Thu, Jan 05, 2017 at 10:27:27AM -0500, Rob Clark wrote:
>> On Thu, Jan 5, 2017 at 6:55 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
>> > On Tue, Jan 03, 2017 at 04:30:54PM -0500, Rob Clark wrote:
>> >> TODO maybe we want two options, one to enable stalling, and 2nd to punt
>> >> handling to wq?  I haven't needed to use mm APIs from fault handler yet
>> >> (although it is something that I think we'll want some day).  Perhaps
>> >> stalling support is limited to just letting driver dump some extra
>> >> debugging information otherwise.  Threaded handling probably only useful
>> >> with stalling, but inverse may not always be true.
>> >
>> > I'd actually like to see this stuck on a worker thread, because I think
>> > that's more generally useful and I don't want to have a situation where
>> > sometimes the IOMMU fault notifier is run in IRQ context and sometimes it's
>> > not.
>>
>> So I was talking a bit w/ Jordan on IRC yesterday..  and we also have
>> the GPU's hw hang-detect to contend with.  So I *suspect* that when we
>> get to the point of using this to do things like page in things from
>> swap and resume the faulting transaction, we probably want to get
>> called immediately from the IRQ handler so we can disable the hw
>> hang-detect.
>
> Well, if you want to use an SMMU for paging, then the GPU driver would
> need to request that explicitly when allocating its DMA buffers, to that
> would be the time to either delay or disable the hang detection.

If userspace is using SVM, for example, it is pretty impossible to
know when to expect a fault.  The best you could do is keep track that
*some* process which has active work queued up for gpu is using SVM
and disable hang detect for *everyone*.. which is kind of sad.

>> I'm not sure if the better solution then would be to have two fault
>> callbacks, one immediately from the IRQ and a later one from wq.  Or
>> let the driver handle the wq business and give it a way to tell the
>> IOMMU when to resume.
>>
>> I kinda think we should punt on the worker thread for now until we are
>> ready to resume faulting transactions, because I guess a strong chance
>> that whatever way we do it now will be wrong ;-)
>
> I guess what I'm after is for you to change the interrupt handlers to be
> threaded, like they are for SMMUv3. I *think* you can do that with a NULL
> thread_fn for now, and just call report_iommu_fault from the handler.
> The return value of that could, in theory, be used to queued the paging
> request and wake the paging thread in future.

If we only pass in the non-threaded irq fxn, I'm not really sure how
that changes anything.. or maybe I'm not understanding what you mean.

But yeah, I guess we could use request_threaded_irq() to get both IRQ
context notification and a later thread context notification rather
than doing the wq thing.  Either way the iommu API has to change
slightly.

>> > 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.
>>
>> We could also drop the property and key the behavior on specific
>> compat strings I guess.  Having both seems a bit odd.  Anyways, I'll
>> defer to DT folks about what the cleaner approach is.
>
> As Robin pointed out, we do need to be able to distinguish the integration
> of the device from the device itself. For example, MMU-9000 might be capable
> of stalling, but if it's bolted to a PCI RC, it's not safe to do so.

Hmm, well we install the fault handler on the iommu_domain..  perhaps
maybe a combo of dts property (or deciding based on more specific
compat string), plus extra param passed in to
iommu_set_fault_hander().  The dts property or compat string to
indicate whether the iommu (and how it is wired up) can handle stalls,
and enable_stall param when fault handler is registered to indicate
whether the device itself can cope.. if either can't do stalling, then
don't set CFCFG.

BR,
-R

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

[Will Deacon]

Hi Rob,

On Fri, Jan 06, 2017 at 11:26:49AM -0500, Rob Clark wrote:
> On Thu, Jan 5, 2017 at 10:49 AM, Will Deacon <will.deacon@arm.com> wrote:
> > On Thu, Jan 05, 2017 at 10:27:27AM -0500, Rob Clark wrote:
> >> I'm not sure if the better solution then would be to have two fault
> >> callbacks, one immediately from the IRQ and a later one from wq.  Or
> >> let the driver handle the wq business and give it a way to tell the
> >> IOMMU when to resume.
> >>
> >> I kinda think we should punt on the worker thread for now until we are
> >> ready to resume faulting transactions, because I guess a strong chance
> >> that whatever way we do it now will be wrong ;-)
> >
> > I guess what I'm after is for you to change the interrupt handlers to be
> > threaded, like they are for SMMUv3. I *think* you can do that with a NULL
> > thread_fn for now, and just call report_iommu_fault from the handler.
> > The return value of that could, in theory, be used to queued the paging
> > request and wake the paging thread in future.
> 
> If we only pass in the non-threaded irq fxn, I'm not really sure how
> that changes anything.. or maybe I'm not understanding what you mean.
> 
> But yeah, I guess we could use request_threaded_irq() to get both IRQ
> context notification and a later thread context notification rather
> than doing the wq thing.  Either way the iommu API has to change
> slightly.
> 
> >> > 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.
> >>
> >> We could also drop the property and key the behavior on specific
> >> compat strings I guess.  Having both seems a bit odd.  Anyways, I'll
> >> defer to DT folks about what the cleaner approach is.
> >
> > As Robin pointed out, we do need to be able to distinguish the integration
> > of the device from the device itself. For example, MMU-9000 might be capable
> > of stalling, but if it's bolted to a PCI RC, it's not safe to do so.
> 
> Hmm, well we install the fault handler on the iommu_domain..  perhaps
> maybe a combo of dts property (or deciding based on more specific
> compat string), plus extra param passed in to
> iommu_set_fault_hander().  The dts property or compat string to
> indicate whether the iommu (and how it is wired up) can handle stalls,
> and enable_stall param when fault handler is registered to indicate
> whether the device itself can cope.. if either can't do stalling, then
> don't set CFCFG.

I thought about this some more, and I think you're right. Having
iommu_set_fault_handler take a flags parameter indicating that, for example,
the fault handler can deal with paging, is all we need to implement the
per-master opt-in functionality for stalling faults. There's no real
requirement to standardise a generic firmware property for that (but
we still need *something* that says stalling is usable on the SMMU --
perhaps just the compatible string is ok).

Taking this further, there's then no need for the threaded IRQ function
in the SMMUv2 driver after all. Instead, we pass a continuation function
pointer and opaque token from the SMMU driver to the fault handler in
IRQ context (this will be in thread context for SMMUv3, but that should
be fine). The fault handler can then stash these someplace, and signal
a wakeup for its own threaded handler, which ultimately calls the SMMU
continuation function with the opaque token as a parameter when it's done
with the fault. I think that's enough to get things rolling without adding
lots of infrastructure to the SMMU driver initially. If a pattern emerges
amongst users of the interface, then we could consolidate some of the work
handling back into IOMMU core.

What do you think? It should all be pretty straightforward for what you
want to do.

Will

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

[Rob Clark]

On Tue, Jan 10, 2017 at 12:52 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi Rob,
>
> On Fri, Jan 06, 2017 at 11:26:49AM -0500, Rob Clark wrote:
>> On Thu, Jan 5, 2017 at 10:49 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
>> > On Thu, Jan 05, 2017 at 10:27:27AM -0500, Rob Clark wrote:
>> >> I'm not sure if the better solution then would be to have two fault
>> >> callbacks, one immediately from the IRQ and a later one from wq.  Or
>> >> let the driver handle the wq business and give it a way to tell the
>> >> IOMMU when to resume.
>> >>
>> >> I kinda think we should punt on the worker thread for now until we are
>> >> ready to resume faulting transactions, because I guess a strong chance
>> >> that whatever way we do it now will be wrong ;-)
>> >
>> > I guess what I'm after is for you to change the interrupt handlers to be
>> > threaded, like they are for SMMUv3. I *think* you can do that with a NULL
>> > thread_fn for now, and just call report_iommu_fault from the handler.
>> > The return value of that could, in theory, be used to queued the paging
>> > request and wake the paging thread in future.
>>
>> If we only pass in the non-threaded irq fxn, I'm not really sure how
>> that changes anything.. or maybe I'm not understanding what you mean.
>>
>> But yeah, I guess we could use request_threaded_irq() to get both IRQ
>> context notification and a later thread context notification rather
>> than doing the wq thing.  Either way the iommu API has to change
>> slightly.
>>
>> >> > 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.
>> >>
>> >> We could also drop the property and key the behavior on specific
>> >> compat strings I guess.  Having both seems a bit odd.  Anyways, I'll
>> >> defer to DT folks about what the cleaner approach is.
>> >
>> > As Robin pointed out, we do need to be able to distinguish the integration
>> > of the device from the device itself. For example, MMU-9000 might be capable
>> > of stalling, but if it's bolted to a PCI RC, it's not safe to do so.
>>
>> Hmm, well we install the fault handler on the iommu_domain..  perhaps
>> maybe a combo of dts property (or deciding based on more specific
>> compat string), plus extra param passed in to
>> iommu_set_fault_hander().  The dts property or compat string to
>> indicate whether the iommu (and how it is wired up) can handle stalls,
>> and enable_stall param when fault handler is registered to indicate
>> whether the device itself can cope.. if either can't do stalling, then
>> don't set CFCFG.
>
> I thought about this some more, and I think you're right. Having
> iommu_set_fault_handler take a flags parameter indicating that, for example,
> the fault handler can deal with paging, is all we need to implement the
> per-master opt-in functionality for stalling faults. There's no real
> requirement to standardise a generic firmware property for that (but
> we still need *something* that says stalling is usable on the SMMU --
> perhaps just the compatible string is ok).

btw, it occurred to me that maybe it should be flags param to
iommu_attach_device() (just in case fault handler not installed?)
otoh stalling without a fault handler is silly, but I guess we need it
to infer whether stalling can be supported by other devices on same
iommu.. tbh I'm on a bit shaky ground when it comes to multiple
devices per iommu since the SoC's I'm familiar with do it the other
way around.  But I guess you have thought more about the multi-device
case, so figured I should suggest it..

> Taking this further, there's then no need for the threaded IRQ function
> in the SMMUv2 driver after all. Instead, we pass a continuation function
> pointer and opaque token from the SMMU driver to the fault handler in
> IRQ context (this will be in thread context for SMMUv3, but that should
> be fine). The fault handler can then stash these someplace, and signal
> a wakeup for its own threaded handler, which ultimately calls the SMMU
> continuation function with the opaque token as a parameter when it's done
> with the fault. I think that's enough to get things rolling without adding
> lots of infrastructure to the SMMU driver initially. If a pattern emerges
> amongst users of the interface, then we could consolidate some of the work
> handling back into IOMMU core.
>
> What do you think? It should all be pretty straightforward for what you
> want to do.

yeah, that makes sense to me..  I can give it a try.

BR,
-R

> Will

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

[Will Deacon]

On Tue, Jan 10, 2017 at 02:20:13PM -0500, Rob Clark wrote:
> On Tue, Jan 10, 2017 at 12:52 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Fri, Jan 06, 2017 at 11:26:49AM -0500, Rob Clark wrote:
> >> Hmm, well we install the fault handler on the iommu_domain..  perhaps
> >> maybe a combo of dts property (or deciding based on more specific
> >> compat string), plus extra param passed in to
> >> iommu_set_fault_hander().  The dts property or compat string to
> >> indicate whether the iommu (and how it is wired up) can handle stalls,
> >> and enable_stall param when fault handler is registered to indicate
> >> whether the device itself can cope.. if either can't do stalling, then
> >> don't set CFCFG.
> >
> > I thought about this some more, and I think you're right. Having
> > iommu_set_fault_handler take a flags parameter indicating that, for example,
> > the fault handler can deal with paging, is all we need to implement the
> > per-master opt-in functionality for stalling faults. There's no real
> > requirement to standardise a generic firmware property for that (but
> > we still need *something* that says stalling is usable on the SMMU --
> > perhaps just the compatible string is ok).
> 
> btw, it occurred to me that maybe it should be flags param to
> iommu_attach_device() (just in case fault handler not installed?)
> otoh stalling without a fault handler is silly, but I guess we need it
> to infer whether stalling can be supported by other devices on same
> iommu.. tbh I'm on a bit shaky ground when it comes to multiple
> devices per iommu since the SoC's I'm familiar with do it the other
> way around.  But I guess you have thought more about the multi-device
> case, so figured I should suggest it..

I don't think it works at attach time, because the stalling property belongs
to the domain, rather than the individual devices within it. Similarly, I
don't think we should allow this property to be toggled once devices have
been attached.

Will
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rob Clark]

On Wed, Jan 11, 2017 at 4:36 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> On Tue, Jan 10, 2017 at 02:20:13PM -0500, Rob Clark wrote:
>> On Tue, Jan 10, 2017 at 12:52 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
>> > On Fri, Jan 06, 2017 at 11:26:49AM -0500, Rob Clark wrote:
>> >> Hmm, well we install the fault handler on the iommu_domain..  perhaps
>> >> maybe a combo of dts property (or deciding based on more specific
>> >> compat string), plus extra param passed in to
>> >> iommu_set_fault_hander().  The dts property or compat string to
>> >> indicate whether the iommu (and how it is wired up) can handle stalls,
>> >> and enable_stall param when fault handler is registered to indicate
>> >> whether the device itself can cope.. if either can't do stalling, then
>> >> don't set CFCFG.
>> >
>> > I thought about this some more, and I think you're right. Having
>> > iommu_set_fault_handler take a flags parameter indicating that, for example,
>> > the fault handler can deal with paging, is all we need to implement the
>> > per-master opt-in functionality for stalling faults. There's no real
>> > requirement to standardise a generic firmware property for that (but
>> > we still need *something* that says stalling is usable on the SMMU --
>> > perhaps just the compatible string is ok).
>>
>> btw, it occurred to me that maybe it should be flags param to
>> iommu_attach_device() (just in case fault handler not installed?)
>> otoh stalling without a fault handler is silly, but I guess we need it
>> to infer whether stalling can be supported by other devices on same
>> iommu.. tbh I'm on a bit shaky ground when it comes to multiple
>> devices per iommu since the SoC's I'm familiar with do it the other
>> way around.  But I guess you have thought more about the multi-device
>> case, so figured I should suggest it..
>
> I don't think it works at attach time, because the stalling property belongs
> to the domain, rather than the individual devices within it. Similarly, I
> don't think we should allow this property to be toggled once devices have
> been attached.
>

hmm, I was more thinking of cases where drivers for particular devices
need some work (ie. like potentially disabling hw hang detect during
faults).. I guess we could have three levels, that all have to be true
in order to enable stall: smmu, domain (pass flags in to
iommu_domain_alloc()??), and device (iommu_attach_device())?

BR,
-R
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Will Deacon]

On Wed, Jan 11, 2017 at 03:59:30PM -0500, Rob Clark wrote:
> On Wed, Jan 11, 2017 at 4:36 AM, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Jan 10, 2017 at 02:20:13PM -0500, Rob Clark wrote:
> >> On Tue, Jan 10, 2017 at 12:52 PM, Will Deacon <will.deacon@arm.com> wrote:
> >> > On Fri, Jan 06, 2017 at 11:26:49AM -0500, Rob Clark wrote:
> >> >> Hmm, well we install the fault handler on the iommu_domain..  perhaps
> >> >> maybe a combo of dts property (or deciding based on more specific
> >> >> compat string), plus extra param passed in to
> >> >> iommu_set_fault_hander().  The dts property or compat string to
> >> >> indicate whether the iommu (and how it is wired up) can handle stalls,
> >> >> and enable_stall param when fault handler is registered to indicate
> >> >> whether the device itself can cope.. if either can't do stalling, then
> >> >> don't set CFCFG.
> >> >
> >> > I thought about this some more, and I think you're right. Having
> >> > iommu_set_fault_handler take a flags parameter indicating that, for example,
> >> > the fault handler can deal with paging, is all we need to implement the
> >> > per-master opt-in functionality for stalling faults. There's no real
> >> > requirement to standardise a generic firmware property for that (but
> >> > we still need *something* that says stalling is usable on the SMMU --
> >> > perhaps just the compatible string is ok).
> >>
> >> btw, it occurred to me that maybe it should be flags param to
> >> iommu_attach_device() (just in case fault handler not installed?)
> >> otoh stalling without a fault handler is silly, but I guess we need it
> >> to infer whether stalling can be supported by other devices on same
> >> iommu.. tbh I'm on a bit shaky ground when it comes to multiple
> >> devices per iommu since the SoC's I'm familiar with do it the other
> >> way around.  But I guess you have thought more about the multi-device
> >> case, so figured I should suggest it..
> >
> > I don't think it works at attach time, because the stalling property belongs
> > to the domain, rather than the individual devices within it. Similarly, I
> > don't think we should allow this property to be toggled once devices have
> > been attached.
> >
> 
> hmm, I was more thinking of cases where drivers for particular devices
> need some work (ie. like potentially disabling hw hang detect during
> faults).. I guess we could have three levels, that all have to be true
> in order to enable stall: smmu, domain (pass flags in to
> iommu_domain_alloc()??), and device (iommu_attach_device())?

Hooking iommu_set_fault_handler, as you originally suggested, is the best
way to set the flag on the domain. I think we just need to enforce that
iommu_set_fault_handler is called prior to attaching devices to a domain,
so that the IOMMU driver can configure the domain appropriately on the
first attach.

Will

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

[Rob Clark]

On Thu, Jan 12, 2017 at 10:17 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> On Wed, Jan 11, 2017 at 03:59:30PM -0500, Rob Clark wrote:
>> On Wed, Jan 11, 2017 at 4:36 AM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
>> > On Tue, Jan 10, 2017 at 02:20:13PM -0500, Rob Clark wrote:
>> >> On Tue, Jan 10, 2017 at 12:52 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
>> >> > On Fri, Jan 06, 2017 at 11:26:49AM -0500, Rob Clark wrote:
>> >> >> Hmm, well we install the fault handler on the iommu_domain..  perhaps
>> >> >> maybe a combo of dts property (or deciding based on more specific
>> >> >> compat string), plus extra param passed in to
>> >> >> iommu_set_fault_hander().  The dts property or compat string to
>> >> >> indicate whether the iommu (and how it is wired up) can handle stalls,
>> >> >> and enable_stall param when fault handler is registered to indicate
>> >> >> whether the device itself can cope.. if either can't do stalling, then
>> >> >> don't set CFCFG.
>> >> >
>> >> > I thought about this some more, and I think you're right. Having
>> >> > iommu_set_fault_handler take a flags parameter indicating that, for example,
>> >> > the fault handler can deal with paging, is all we need to implement the
>> >> > per-master opt-in functionality for stalling faults. There's no real
>> >> > requirement to standardise a generic firmware property for that (but
>> >> > we still need *something* that says stalling is usable on the SMMU --
>> >> > perhaps just the compatible string is ok).
>> >>
>> >> btw, it occurred to me that maybe it should be flags param to
>> >> iommu_attach_device() (just in case fault handler not installed?)
>> >> otoh stalling without a fault handler is silly, but I guess we need it
>> >> to infer whether stalling can be supported by other devices on same
>> >> iommu.. tbh I'm on a bit shaky ground when it comes to multiple
>> >> devices per iommu since the SoC's I'm familiar with do it the other
>> >> way around.  But I guess you have thought more about the multi-device
>> >> case, so figured I should suggest it..
>> >
>> > I don't think it works at attach time, because the stalling property belongs
>> > to the domain, rather than the individual devices within it. Similarly, I
>> > don't think we should allow this property to be toggled once devices have
>> > been attached.
>> >
>>
>> hmm, I was more thinking of cases where drivers for particular devices
>> need some work (ie. like potentially disabling hw hang detect during
>> faults).. I guess we could have three levels, that all have to be true
>> in order to enable stall: smmu, domain (pass flags in to
>> iommu_domain_alloc()??), and device (iommu_attach_device())?
>
> Hooking iommu_set_fault_handler, as you originally suggested, is the best
> way to set the flag on the domain. I think we just need to enforce that
> iommu_set_fault_handler is called prior to attaching devices to a domain,
> so that the IOMMU driver can configure the domain appropriately on the
> first attach.

Hi Will, just (finally) revisiting this..

So I started working on a patch to add can_stall to
iommu_set_fault_handler() (fortunately not many callers).  And then
adding an iommu_domain_resume(domain, resume/terminate).  (Ie.
iommu_domain_resume() would be called by the IOMMU user either
directly from fault handler callback, or indirectly from a thread
context..  that seemed a bit less clunky than passing a callback to
the callback..)

But is there any good way to iterate all the domains associated w/ the
arm_smmu_device?  Unfortunately we don't pass in the device ptr to
iommu_domain_alloc() no I'm not entirely sure at what point we know
whether all the associated domains can stall..

BR,
-R