Re: [PATCH v2 00/24] iommu: Refactor DMA domain strictness

[Doug Anderson]

Hi,

On Thu, Jul 29, 2021 at 3:33 PM Doug Anderson  wrote:
>
> I was definitely getting some inconsistencies in my tests where the
> eMMC speeds were getting into a bad state, but I don't believe it's
> related to your patch series.

I think this was just me being an idiot. I forgot that I'd been
running with KASAN, so that explains why my speeds were so much slower
than usual and probably also explains how it could get in a bad state
(I guess it also explains why sugov was eating up 30% of my CPU time
since that went away too!). No mystery here aside from why it took me
this long to realize it.

I'm now getting ~213 MB/s without forcing it to lazy and ~261 MB/s
with forcing it to lazy through sysfs (and without any other cpufreq
hacks).

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 00/24] iommu: Refactor DMA domain strictness

[Doug Anderson]

Hi,

On Wed, Jul 28, 2021 at 8:59 AM Robin Murphy  wrote:
>
> Hi all,
>
> Here's v2 where things start to look more realistic, hence the expanded
> CC list. The patches are now based on the current iommu/core branch to
> take John's iommu_set_dma_strict() cleanup into account.
>
> The series remiains in two (or possibly 3) logical parts - for people
> CC'd on cookie cleanup patches, the later parts should not affect you
> since your drivers don't implement non-strict mode anyway; the cleanup
> is all pretty straightforward, but please do yell at me if I've managed
> to let a silly mistake slip through and broken your driver.
>
> This time I have also build-tested x86 as well as arm64 :)
>
> Changes in v2:
>
> - Add iommu_is_dma_domain() helper to abstract flag check (and help
>   avoid silly typos like the one in v1).
> - Tweak a few commit messages for spelling and (hopefully) clarity.
> - Move the iommu_create_device_direct_mappings() update to patch #14
>   where it should have been.
> - Rewrite patch #20 as a conversion of the now-existing option.
> - Clean up the ops->flush_iotlb_all check which is also made redundant
>   by the new domain type
> - Add patch #24, which is arguably tangential, but it was something I
>   spotted during the rebase, so...
>
> Once again, the whole lot is available on a branch here:
>
> https://gitlab.arm.com/linux-arm/linux-rm/-/tree/iommu/fq
>
> Thanks,
> Robin.
>
>
> CC: Marek Szyprowski 
> CC: Yoshihiro Shimoda 
> CC: Geert Uytterhoeven 
> CC: Yong Wu 
> CC: Heiko Stuebner 
> CC: Chunyan Zhang 
> CC: Chunyan Zhang 
> CC: Maxime Ripard 
> CC: Jean-Philippe Brucker 
>
> Robin Murphy (24):
>   iommu: Pull IOVA cookie management into the core
>   iommu/amd: Drop IOVA cookie management
>   iommu/arm-smmu: Drop IOVA cookie management
>   iommu/vt-d: Drop IOVA cookie management
>   iommu/exynos: Drop IOVA cookie management
>   iommu/ipmmu-vmsa: Drop IOVA cookie management
>   iommu/mtk: Drop IOVA cookie management
>   iommu/rockchip: Drop IOVA cookie management
>   iommu/sprd: Drop IOVA cookie management
>   iommu/sun50i: Drop IOVA cookie management
>   iommu/virtio: Drop IOVA cookie management
>   iommu/dma: Unexport IOVA cookie management
>   iommu/dma: Remove redundant "!dev" checks
>   iommu: Introduce explicit type for non-strict DMA domains
>   iommu/amd: Prepare for multiple DMA domain types
>   iommu/arm-smmu: Prepare for multiple DMA domain types
>   iommu/vt-d: Prepare for multiple DMA domain types
>   iommu: Express DMA strictness via the domain type
>   iommu: Expose DMA domain strictness via sysfs
>   iommu: Merge strictness and domain type configs
>   iommu/dma: Factor out flush queue init
>   iommu: Allow enabling non-strict mode dynamically
>   iommu/arm-smmu: Allow non-strict in pgtable_quirks interface
>   iommu: Only log strictness for DMA domains
>
>  .../ABI/testing/sysfs-kernel-iommu_groups |  2 +
>  drivers/iommu/Kconfig | 80 +--
>  drivers/iommu/amd/iommu.c | 21 +
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 25 --
>  drivers/iommu/arm/arm-smmu/arm-smmu.c | 29 ---
>  drivers/iommu/arm/arm-smmu/qcom_iommu.c   |  8 --
>  drivers/iommu/dma-iommu.c | 44 +-
>  drivers/iommu/exynos-iommu.c  | 18 +
>  drivers/iommu/intel/iommu.c   | 23 ++
>  drivers/iommu/iommu.c | 53 +++-
>  drivers/iommu/ipmmu-vmsa.c| 27 +--
>  drivers/iommu/mtk_iommu.c |  6 --
>  drivers/iommu/rockchip-iommu.c| 11 +--
>  drivers/iommu/sprd-iommu.c|  6 --
>  drivers/iommu/sun50i-iommu.c  | 12 +--
>  drivers/iommu/virtio-iommu.c  |  8 --
>  include/linux/dma-iommu.h |  9 ++-
>  include/linux/iommu.h | 15 +++-
>  18 files changed, 171 insertions(+), 226 deletions(-)

I ran with:

a) mainline Linux (at commit 4010a528219e)
b) pulled iommu/next (at commit 9be9f5580ab6)
c) picked from patchwork your series

...and I ran on sc7180-trogdor-lazor.

Things worked OK and I could transition my eMMC to non-strict mode with:

echo DMA-FQ > /sys/devices/platform/soc@0/7c4000.sdhci/iommu_group/type

I was definitely getting some inconsistencies in my tests where the
eMMC speeds were getting into a bad state, but I don't believe it's
related to your patch series. I could transition myself back to strict
DMA with this (only got one unrelated warn splat about
dev_pm_opp_put_clkname when unbinding) because I was booted up from
USB for testing:

cd /sys/bus/mmc/drivers/mmcblk
echo mmc1:0001 > unbind
cd /sys/bus/platform/drivers/sdhci_msm/
echo 7c4000.sdhci > unbind
echo DMA > /sys/devices/platform/soc@0/7c4000.sdhci/iommu_group/type
echo 7c4000.sdhci > bind

...and it was consistently faster with non-strict than with strict so
whatever bad state I sometimes managed 

Re: [PATCH v2 0/3] iommu: Enable non-strict DMA on QCom SD/MMC

[Doug Anderson]

Hi,

On Tue, Jul 13, 2021 at 11:07 AM Robin Murphy  wrote:
>
> On 2021-07-08 15:36, Doug Anderson wrote:
> [...]
> >> Or document for the users that want performance how to
> >> change the setting, so that they can decide.
> >
> > Pushing this to the users can make sense for a Linux distribution but
> > probably less sense for an embedded platform. So I'm happy to make
> > some way for a user to override this (like via kernel command line),
> > but I also strongly believe there should be a default that users don't
> > have to futz with that we think is correct.
>
> FYI I did make progress on the "punt it to userspace" approach. I'm not
> posting it even as an RFC yet because I still need to set up a machine
> to try actually testing any of it (it's almost certainly broken
> somewhere), but in the end it comes out looking surprisingly not too bad
> overall. If you're curious to take a look in the meantime I put it here:
>
> https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/fq

Being able to change this at runtime through sysfs sounds great and it
fills all the needs I'm aware of, thanks! In Chrome OS we can just use
this with some udev rules and get everything we need. I'm happy to
give this a spin when you're ready for extra testing.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/3] iommu: Enable non-strict DMA on QCom SD/MMC

[Doug Anderson]

Hi,

On Thu, Jul 8, 2021 at 1:09 AM Joerg Roedel  wrote:
>
> On Wed, Jul 07, 2021 at 01:00:13PM -0700, Doug Anderson wrote:
> > a) Nothing is inherently broken with my current approach.
> >
> > b) My current approach doesn't make anybody terribly upset even if
> > nobody is totally in love with it.
>
> Well, no, sorry :)
>
> I don't think it is a good idea to allow drivers to opt-out of the
> strict-setting. This is a platform or user decision, and the driver
> should accept whatever it gets.

Sure, I agree with you there. The driver shouldn't ever be able to
override and make things less strict than the user or platform wants.
It feels like that can be accomplished. See below.


> So the real question is still why strict is the default setting and how
> to change that.

I guess there are two strategies if we agree that there's a benefit to
running some devices in strict and others in non-strict:

* opt-in to strict: default is non-strict and we have to explicitly
list what we want to be strict.

* opt-out of strict: default is strict and we have to explicitly list
what we want to be non-strict.

I guess the question is: do we allow both strategies or only one of
them? I think you are suggesting that the kernel should support
"opt-in" to strict and that that matches the status quo with PCI on
x86. I'm pushing for some type of "opt-out" of strict support. I have
heard from security folks that they'd prefer "opt-out" of strict as
well. If we're willing to accept more complex config options we could
support both choosable by KConfig. How it'd all work in my mind:

Command line:

* iommu.strict=0 - suggest non-strict by default
* iommu.strict=1 - force strict for all drivers
* iommu.strict not specified - no opinion

Kconfig:

* IOMMU_DEFAULT_LAZY - suggest non-strict by default; drivers can
opt-in to strict
* IOMMU_DEFAULT_STRICT - force strict for all drivers
* IOMMU_DEFAULT_LOOSE_STRICT - allow explicit suggestions for laziness
but default to strict if no votes.

Drivers:
* suggest lazy - suggest non-strict
* force strict - force strict
* no vote


How the above work together:

* if _any_ of the three things wants strict then it's strict.

* if _all_ of the three things want lazy then it's lazy.

* If the KConfig is "loose strict" and the command line is set to
"lazy" then it's equivalent to the KConfig saying "lazy". In other
words drivers could still "opt-in" to strict but otherwise we'd be
lazy.

* The only way for a driver's "suggest lazy" vote to have any effect
at all is if "iommu.strict" wasn't specified on the command line _and_
if the KConfig was "loose strict". This is effectively the "opt-out"
of lazy.


If you think the strategy I describe above is garbage then would you
be OK if I re-worked my patchset to at least allow non-PCI drivers to
"opt-in" to strict? Effectively I'd change patch #3 to list all of the
peripherals on my SoC _except_ the USB and SD/MMC and request that
they all be strict. If other people expressed their preference for the
"opt-out" of strict strategy would that change your mind?


> Or document for the users that want performance how to
> change the setting, so that they can decide.

Pushing this to the users can make sense for a Linux distribution but
probably less sense for an embedded platform. So I'm happy to make
some way for a user to override this (like via kernel command line),
but I also strongly believe there should be a default that users don't
have to futz with that we think is correct.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/3] iommu: Enable non-strict DMA on QCom SD/MMC

[Doug Anderson]

Hi,

On Fri, Jun 25, 2021 at 7:42 AM Doug Anderson  wrote:
>
> Hi,
>
> On Fri, Jun 25, 2021 at 6:19 AM Joerg Roedel  wrote:
> >
> > Hi Douglas,
> >
> > On Thu, Jun 24, 2021 at 10:17:56AM -0700, Douglas Anderson wrote:
> > > The goal of this patch series is to get better SD/MMC performance on
> > > Qualcomm eMMC controllers and in generally nudge us forward on the
> > > path of allowing some devices to be in strict mode and others to be in
> > > non-strict mode.
> >
> > So if I understand it right, this patch-set wants a per-device decision
> > about setting dma-mode to strict vs. non-strict.
> >
> > I think we should get to the reason why strict mode is used by default
> > first. Is the default on ARM platforms to use iommu-strict mode by
> > default and if so, why?
> >
> > The x86 IOMMUs use non-strict mode by default (yes, it is a security
> > trade-off).
>
> It is certainly a good question. I will say that, as per usual, I'm
> fumbling around trying to solve problems in subsystems I'm not an
> expert at, so if something I'm saying sounds like nonsense it probably
> is. Please correct me.
>
> I guess I'd start out by thinking about what devices I think need to
> be in "strict" mode. Most of my thoughts on this are in the 3rd patch
> in the series. I think devices where it's important to be in strict
> mode fall into "Case 1" from that patch description, copied here:
>
> Case 1: IOMMUs prevent malicious code running on the peripheral (maybe
> a malicious peripheral or maybe someone exploited a benign peripheral)
> from turning into an exploit of the Linux kernel. This is particularly
> important if the peripheral has loadable / updatable firmware or if
> the peripheral has some type of general purpose processor and is
> processing untrusted inputs. It's also important if the device is
> something that can be easily plugged into the host and the device has
> direct DMA access itself, like a PCIe device.
>
>
> Using sc7180 as an example (searching for iommus in sc7180.dtsi), I'd
> expect these peripherals to be in strict mode:
>
> * WiFi / LTE - I'm almost certain we want this in "strict" mode. Both
> have loadable / updatable firmware and both do complex processing on
> untrusted inputs. Both have a history of being compromised over the
> air just by being near an attacker. Note that on sc7180 these are
> _not_ connected over PCI so we can't leverage any PCI mechanism for
> deciding strict / non-strict.
>
> * Video decode / encode - pretty sure we want this in strict. It's got
> loadable / updatable firmware and processing complex / untrusted
> inputs.
>
> * LPASS (low power audio subsystem) - I don't know a ton and I think
> we don't use this much on our designs, but I believe it meets the
> definitions for needing "strict".
>
> * The QUPs (handles UART, SPI, and i2c) - I'm not as sure here. These
> are much "smarter" than you'd expect. They have loadable / updatable
> firmware and certainly have a sort of general purpose processor in
> them. They also might be processing untrusted inputs, but presumably
> in a pretty simple way. At the moment we don't use a ton of DMA here
> anyway and these are pretty low speed, so I would tend to leave them
> as strict just to be on the safe side.
>
>
> I'd expect these to be non-strict:
>
> * SD/MMC - as described in this patch series.
>
> * USB - As far as I know firmware isn't updatable and has no history
> of being compromised.
>
>
> Special:
>
> * GPU - This already has a bunch of special cases, so we probably
> don't need to discuss here.
>
>
> As far as I can tell everything in sc7180.dtsi that has an "iommus"
> property is classified above. So, unless I'm wrong and it's totally
> fine to run LTE / WiFi / Video / LPASS in non-strict mode then:
>
> * We still need some way to pick strict vs. non-strict.
>
> * Since I've only identified two peripherals that I think should be
> non-strict, having "strict" the default seems like fewer special
> cases. It's also safer.
>
>
> In terms of thinking about x86 / AMD where the default is non-strict,
> I don't have any historical knowledge there. I guess the use of PCI
> for connecting WiFi is more common (so you can use the PCI special
> cases) and I'd sorta hope that WiFi is running in strict mode. For
> video encode / decode, perhaps x86 / AMD are just accepting the risk
> here because there was no kernel infrastructure for doing better? I'd
> also expect that x86/AMD don't have something quite as crazy as the
> QUPs for UART/I2C/SPI, but even if they do I wouldn't be terribly

Re: [PATCH v2 0/3] iommu: Enable non-strict DMA on QCom SD/MMC

[Doug Anderson]

Hi,

On Fri, Jun 25, 2021 at 6:19 AM Joerg Roedel  wrote:
>
> Hi Douglas,
>
> On Thu, Jun 24, 2021 at 10:17:56AM -0700, Douglas Anderson wrote:
> > The goal of this patch series is to get better SD/MMC performance on
> > Qualcomm eMMC controllers and in generally nudge us forward on the
> > path of allowing some devices to be in strict mode and others to be in
> > non-strict mode.
>
> So if I understand it right, this patch-set wants a per-device decision
> about setting dma-mode to strict vs. non-strict.
>
> I think we should get to the reason why strict mode is used by default
> first. Is the default on ARM platforms to use iommu-strict mode by
> default and if so, why?
>
> The x86 IOMMUs use non-strict mode by default (yes, it is a security
> trade-off).

It is certainly a good question. I will say that, as per usual, I'm
fumbling around trying to solve problems in subsystems I'm not an
expert at, so if something I'm saying sounds like nonsense it probably
is. Please correct me.

I guess I'd start out by thinking about what devices I think need to
be in "strict" mode. Most of my thoughts on this are in the 3rd patch
in the series. I think devices where it's important to be in strict
mode fall into "Case 1" from that patch description, copied here:

Case 1: IOMMUs prevent malicious code running on the peripheral (maybe
a malicious peripheral or maybe someone exploited a benign peripheral)
from turning into an exploit of the Linux kernel. This is particularly
important if the peripheral has loadable / updatable firmware or if
the peripheral has some type of general purpose processor and is
processing untrusted inputs. It's also important if the device is
something that can be easily plugged into the host and the device has
direct DMA access itself, like a PCIe device.


Using sc7180 as an example (searching for iommus in sc7180.dtsi), I'd
expect these peripherals to be in strict mode:

* WiFi / LTE - I'm almost certain we want this in "strict" mode. Both
have loadable / updatable firmware and both do complex processing on
untrusted inputs. Both have a history of being compromised over the
air just by being near an attacker. Note that on sc7180 these are
_not_ connected over PCI so we can't leverage any PCI mechanism for
deciding strict / non-strict.

* Video decode / encode - pretty sure we want this in strict. It's got
loadable / updatable firmware and processing complex / untrusted
inputs.

* LPASS (low power audio subsystem) - I don't know a ton and I think
we don't use this much on our designs, but I believe it meets the
definitions for needing "strict".

* The QUPs (handles UART, SPI, and i2c) - I'm not as sure here. These
are much "smarter" than you'd expect. They have loadable / updatable
firmware and certainly have a sort of general purpose processor in
them. They also might be processing untrusted inputs, but presumably
in a pretty simple way. At the moment we don't use a ton of DMA here
anyway and these are pretty low speed, so I would tend to leave them
as strict just to be on the safe side.


I'd expect these to be non-strict:

* SD/MMC - as described in this patch series.

* USB - As far as I know firmware isn't updatable and has no history
of being compromised.


Special:

* GPU - This already has a bunch of special cases, so we probably
don't need to discuss here.


As far as I can tell everything in sc7180.dtsi that has an "iommus"
property is classified above. So, unless I'm wrong and it's totally
fine to run LTE / WiFi / Video / LPASS in non-strict mode then:

* We still need some way to pick strict vs. non-strict.

* Since I've only identified two peripherals that I think should be
non-strict, having "strict" the default seems like fewer special
cases. It's also safer.


In terms of thinking about x86 / AMD where the default is non-strict,
I don't have any historical knowledge there. I guess the use of PCI
for connecting WiFi is more common (so you can use the PCI special
cases) and I'd sorta hope that WiFi is running in strict mode. For
video encode / decode, perhaps x86 / AMD are just accepting the risk
here because there was no kernel infrastructure for doing better? I'd
also expect that x86/AMD don't have something quite as crazy as the
QUPs for UART/I2C/SPI, but even if they do I wouldn't be terribly
upset if they were in non-strict mode.

...so I guess maybe the super short answer to everything above is that
I believe that at least WiFi ought to be in "strict" mode and it's not
on PCI so we need to come up with some type of per-device solution.


-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/3] mmc: sdhci-msm: Request non-strict IOMMU mode

[Doug Anderson]

Hi,

On Thu, Jun 24, 2021 at 10:18 AM Douglas Anderson  wrote:
>
> The concept of IOMMUs being in strict vs. non-strict mode is a
> pre-existing Linux concept. I've included a rough summary here to help
> evaluate this patch.
>
> IOMMUs can be run in "strict" mode or in "non-strict" mode. The
> quick-summary difference between the two is that in "strict" mode we
> wait until everything is flushed out when we unmap DMA memory. In
> "non-strict" we don't.
>
> Using the IOMMU in "strict" mode is more secure/safer but slower
> because we have to sit and wait for flushes while we're unmapping. To
> explain a bit why "non-strict" mode is unsafe, let's imagine two
> examples.
>
> An example of "non-strict" being insecure when reading from a device:
> a) Linux driver maps memory for DMA.
> b) Linux driver starts DMA on the device.
> c) Device write to RAM subject to bounds checking done by IOMMU.
> d) Device finishes writing to RAM and signals transfer is finished.
> e) Linux driver starts unmapping DMA memory but doesn't wait for the
>unmap to finish (the definition of non-strict). At this point,
>though, the Linux APIs say that the driver owns the memory and
>shouldn't expect any more scribbling from the DMA device.
> f) Linux driver validates that the data in memory looks sane and that
>accessing it won't cause the driver to, for instance, overflow a
>buffer.
> g) Device takes advantage of knowledge of how the Linux driver works
>and sneaks in a modification to the data after the validation but
>before the IOMMU unmap flush finishes.
> h) Device has now caused the Linux driver to access memory it
>shouldn't.
>
> An example of "non-strict" being insecure when writing to a device:
> a) Linux driver writes data intended for the device to RAM.
> b) Linux driver maps memory for DMA.
> c) Linux driver starts DMA on the device.
> d) Device reads from RAM subject to bounds checking done by IOMMU.
> e) Device finishes reading from RAM and signals transfer is finished.
> f) Linux driver starts unmapping DMA memory but doesn't wait for the
>unmap to finish (the definition of non-strict)
> g) Linux driver frees memory and returns it to the pool of memory
>available for other users to allocate.
> h) Memory is allocated for another purpose since it was free memory.
> i) Device takes advantage of the period of time before IOMMU flush to
>read memory that it shouldn't have had access to. What exactly the
>memory could contain depends on the randomness of who allocated
>next, though exploits have been built on flimisier holes.
>
> As you can see from the above examples, using the iommu in
> "non-strict" mode might not sound _too_ scary (the window of badness
> is small and the exposed memory is small) but there is certainly
> risk. Let's evaluate the risk by breaking it down into two problems
> that IOMMUs are supposed to be protecting us against:
>
> Case 1: IOMMUs prevent malicious code running on the peripheral (maybe
> a malicious peripheral or maybe someone exploited a benign peripheral)
> from turning into an exploit of the Linux kernel. This is particularly
> important if the peripheral has loadable / updatable firmware or if
> the peripheral has some type of general purpose processor and is
> processing untrusted inputs. It's also important if the device is
> something that can be easily plugged into the host and the device has
> direct DMA access itself, like a PCIe device.
>
> Case 2: IOMMUs limit the severity of a class of software bugs in the
> kernel. If we misconfigure a peripheral by accident then instead of
> the peripheral clobbering random memory due to a bug we might get an
> IOMMU error.
>
> Now that we understand the issue and the risks, let's evaluate whether
> we really need "strict" mode for the Qualcomm SDHCI controllers. I
> will make the argument that we don't _need_ strict mode for them. Why?
> * The SDHCI controller on Qualcomm SoCs doesn't appear to have
>   loadable / updatable firmware and, assuming it's got some firmware
>   baked into it, I see no evidence that the firmware could be
>   compromised.
> * Even though, for external SD cards in particular, the controller is
>   dealing with "untrusted" inputs, it's dealing with them in a very
>   controlled way.  It seems unlikely that a rogue SD card would be
>   able to present something to the SDHCI controller that would cause
>   it to DMA to/from an address other than one the kernel told it
>   about.
> * Although it would be nice to catch more software bugs, once the
>   Linux driver has been debugged and stressed the value is not very
>   high. If the IOMMU caught something like this the system would be in
>   a pretty bad shape anyway (we don't really recover from IOMMU
>   errors) and the only benefit would be a better spotlight on what
>   went wrong.
>
> Now we have a good understanding of the benefits of "strict" mode for
> our SDHCI controllers, let's look at some performance 

Re: [PATCH 0/6] iommu: Enable devices to request non-strict DMA, starting with QCom SD/MMC

[Doug Anderson]

Hi,

On Wed, Jun 23, 2021 at 10:29 AM Doug Anderson  wrote:
>
> * Instead of putting the details in per-device nodes, maybe it makes
> sense to accept that we should be specifying these things at the IOMMU
> level? If specifying things at the device tree level then the
> device-tree node of the IOMMU itself would just have a list of things
> that should be strict/non-strict. ...this could potentially be merged
> with a hardcoded list of things in the IOMMU driver based on the IOMMU
> compatible string.
>
> Do those sound right?
>
> I still haven't totally grokked the ideal way to identify devices. I
> guess on Qualcomm systems each device is in its own group and so could
> have its own strictness levels? ...or would it be better to list by
> "stream ID" or something like that?
>
> If we do something like this then maybe that's a solution that could
> land short-ish term? We would know right at init time whether a given
> domain should be strict or non-strict and there'd be no requirements
> to transition it.

OK, so I have attempted to implement this in the Qualcomm IOMMU driver
in v2 of this series:

https://lore.kernel.org/r/20210624171759.4125094-1-diand...@chromium.org/

Hopefully that doesn't fragment the discussion too much, but it seemed
like it might help move us forward to see what this would look like in
code.

I'll also note that I removed a few people from the CC list on v2 of
the series because I'm no longer touching any code outside of the
IOMMU subsystem and I thought folks would appreciate less noise in
their inboxes. I've CCed a boatload of mailing lists though so it
should be easy to find. If I dropped you from the CC list of v2 and
you really want back on then I'm more than happy to re-add you.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 6/6] mmc: sdhci-msm: Request non-strict IOMMU mode

[Doug Anderson]

Hi,

On Thu, Jun 24, 2021 at 6:43 AM Greg KH  wrote:
>
> On Mon, Jun 21, 2021 at 04:52:48PM -0700, Douglas Anderson wrote:
> > IOMMUs can be run in "strict" mode or in "non-strict" mode. The
> > quick-summary difference between the two is that in "strict" mode we
> > wait until everything is flushed out when we unmap DMA memory. In
> > "non-strict" we don't.
> >
> > Using the IOMMU in "strict" mode is more secure/safer but slower
> > because we have to sit and wait for flushes while we're unmapping. To
> > explain a bit why "non-strict" mode is unsafe, let's imagine two
> > examples.
> >
> > An example of "non-strict" being insecure when reading from a device:
> > a) Linux driver maps memory for DMA.
> > b) Linux driver starts DMA on the device.
> > c) Device write to RAM subject to bounds checking done by IOMMU.
> > d) Device finishes writing to RAM and signals transfer is finished.
> > e) Linux driver starts unmapping DMA memory but doesn't flush.
>
> Why doesn't it "flush"?

This is just how the pre-existing "iommu.strict=0" command line parameter works.


> > f) Linux driver validates that the data in memory looks sane and that
> >accessing it won't cause the driver to, for instance, overflow a
> >buffer.
> > g) Device takes advantage of knowledge of how the Linux driver works
> >and sneaks in a modification to the data after the validation but
> >before the IOMMU unmap flush finishes.
> > h) Device has now caused the Linux driver to access memory it
> >shouldn't.
>
> So you are now saying we need to not trust hardware?  If so, that's a
> massive switch for how the kernel needs to work right?

This is a pre-existing concept in the kernel and is in fact so
prevalent that there are a bunch of inconsistent ways to configure it
(though it's being made better [1])

* On ARM64, default is strict and you can configure it with iommu.strict

* On AMD, default is non-strict and you can configure it with
amd_iommu=fullflush

* On Intel, default is non-strict and you can configure it with
intel_iommu=strict

...also pre-existing is that the kernel has special cases for
"external" PCI devices where it forces them to strict mode even if the
default is non-strict (like on Intel and AMD). I was pointed
specifically at  for an example of why this
was important.


> And what driver does f) and allows g) to happen?  That would be a normal
> bug anyway, why not just fix the driver?

This one would be possible to workaround in the driver by copying the
memory somewhere else, but it violates the DMA model. Specifically
step "e)" above is supposed to mean that the driver is now in full
control of the memory, so it should be perfectly justified in assuming
that nobody else is scribbling on it.


> > An example of "non-strict" being insecure when writing to a device:
> > a) Linux driver writes data intended for the device to RAM.
> > b) Linux driver maps memory for DMA.
> > c) Linux driver starts DMA on the device.
> > d) Device reads from RAM subject to bounds checking done by IOMMU.
> > e) Device finishes reading from RAM and signals transfer is finished.
> > f) Linux driver starts unmapping DMA memory but doesn't flush.
>
> Why does it not flush?
>
> What do you mean by "flush"

"flush" means force / wait for the IOMMU unmap to fully take effect.


> > g) Linux driver frees memory and returns it to the pool.
>
> What pool?

The normal Linux memory pool.


> > h) Memory is allocated for another purpose.
>
> Allocated by what?

Someone else that wanted memory.


> We have memory allocators that write over the data when freed, why not
> just use this if you are concerned about this type of issue?
>
> > i) Device takes advantage of the period of time before IOMMU flush to
> >read memory that it shouldn't have had access to.
>
> What memory would that be?

Depends on who got it. This could be hard to predict unless a
peripheral was trying to exploit a very specific version of Linux
where maybe it was predictable who got the memory next.


> And if you really care about these issues, are you not able to take the
> "hit" for the flush all the time as that is a hardware thing, not a
> software thing.  Why not just always take advantage of that, no driver
> changes needed?

The whole concept of strict vs. non-strict is definitely not new to my
series and I'm mostly just trying to configure it properly.


> And this isn't going to work, again, because the "pre_probe" isn't going
> to be acceptable, sorry.

Right. As discussed in the cover letter, I'm going to try to solve
this in other ways that doesn't involve pre_probe.

[1] 
https://lore.kernel.org/linux-iommu/1624016058-189713-1-git-send-email-john.ga...@huawei.com/T/#m21bc07b9353b3ba85f2a40557645c2bcc13cbb3e

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/6] PCI: Indicate that we want to force strict DMA for untrusted devices

[Doug Anderson]

Hi,

On Thu, Jun 24, 2021 at 6:38 AM Greg KH  wrote:
>
> On Mon, Jun 21, 2021 at 04:52:45PM -0700, Douglas Anderson wrote:
> > At the moment the generic IOMMU framework reaches into the PCIe device
> > to check the "untrusted" state and uses this information to figure out
> > if it should be running the IOMMU in strict or non-strict mode. Let's
> > instead set the new boolean in "struct device" to indicate when we
> > want forced strictness.
> >
> > NOTE: we still continue to set the "untrusted" bit in PCIe since that
> > apparently is used for more than just IOMMU strictness. It probably
> > makes sense for a later patchset to clarify all of the other needs we
> > have for "untrusted" PCIe devices (perhaps add more booleans into the
> > "struct device") so we can fully eliminate the need for the IOMMU
> > framework to reach into a PCIe device.
>
> It feels like the iommu code should not be messing with pci devices at
> all, please don't do this.

I think it's generally agreed that having the IOMMU code reach into
the PCIe code is pretty non-ideal, but that's not something that my
patch series added. The IOMMU code already has special cases to reach
into PCIe devices to decide strictness. I was actually trying to
reduce the amount of it.

> Why does this matter?  Why wouldn't a pci device use "strict" iommu at
> all times?  What happens if it does not?  Why are PCI devices special?

This is something pre-existing in Linux. In my patch series I was
trying to make PCI devices less special and take some of the concepts
from there and expand them, but in a cleaner way. It sounds like in my
v2 I should steer away from this and leave the existing PCI hacks
alone.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/6] drivers: base: Add bits to struct device to control iommu strictness

[Doug Anderson]

Hi,

On Thu, Jun 24, 2021 at 6:37 AM Greg KH  wrote:
>
> On Mon, Jun 21, 2021 at 04:52:44PM -0700, Douglas Anderson wrote:
> > How to control the "strictness" of an IOMMU is a bit of a mess right
> > now. As far as I can tell, right now:
> > * You can set the default to "non-strict" and some devices (right now,
> >   only PCI devices) can request to run in "strict" mode.
> > * You can set the default to "strict" and no devices in the system are
> >   allowed to run as "non-strict".
> >
> > I believe this needs to be improved a bit. Specifically:
> > * We should be able to default to "strict" mode but let devices that
> >   claim to be fairly low risk request that they be run in "non-strict"
> >   mode.
> > * We should allow devices outside of PCIe to request "strict" mode if
> >   the system default is "non-strict".
> >
> > I believe the correct way to do this is two bits in "struct
> > device". One allows a device to force things to "strict" mode and the
> > other allows a device to _request_ "non-strict" mode. The asymmetry
> > here is on purpose. Generally if anything in the system makes a
> > request for strictness of something then we want it strict. Thus
> > drivers can only request (but not force) non-strictness.
> >
> > It's expected that the strictness fields can be filled in by the bus
> > code like in the patch ("PCI: Indicate that we want to force strict
> > DMA for untrusted devices") or by using the new pre_probe concept
> > introduced in the patch ("drivers: base: Add the concept of
> > "pre_probe" to drivers").
> >
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> >  include/linux/device.h | 11 +++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index f1a00040fa53..c1b985e10c47 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -449,6 +449,15 @@ struct dev_links_info {
> >   *   and optionall (if the coherent mask is large enough) also
> >   *   for dma allocations.  This flag is managed by the dma ops
> >   *   instance from ->dma_supported.
> > + * @force_strict_iommu: If set to %true then we should force this device to
> > + *   iommu.strict regardless of the other defaults in the
> > + *   system. Only has an effect if an IOMMU is in place.
>
> Why would you ever NOT want to do this?
>
> > + * @request_non_strict_iommu: If set to %true and there are no other known
> > + * reasons to make the iommu.strict for this 
> > device,
> > + * then default to non-strict mode. This implies
> > + * some belief that the DMA master for this device
> > + * won't abuse the DMA path to compromise the 
> > kernel.
> > + * Only has an effect if an IOMMU is in place.
>
> This feels in contrast to the previous field you just added, how do they
> both interact?  Would an enum work better?

Right that it never makes sense to set both bits so an enum could work
better, essentially it would be { dont_care, request_non_strict,
force_strict }.

In any case the whole idea of doing this at the device level looks
like it's not the right way to go anyway, so this patch and the
previous pre_probe one are essentially abandoned and I need to send
out a v2 with some different approaches.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/6] iommu: Enable devices to request non-strict DMA, starting with QCom SD/MMC

[Doug Anderson]

Hi,

On Tue, Jun 22, 2021 at 3:10 PM Robin Murphy  wrote:
>
> On 2021-06-22 17:06, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Jun 22, 2021 at 4:35 AM Robin Murphy  wrote:
> >>
> >> Hi Doug,
> >>
> >> On 2021-06-22 00:52, Douglas Anderson wrote:
> >>>
> >>> This patch attempts to put forward a proposal for enabling non-strict
> >>> DMA on a device-by-device basis. The patch series requests non-strict
> >>> DMA for the Qualcomm SDHCI controller as a first device to enable,
> >>> getting a nice bump in performance with what's believed to be a very
> >>> small drop in security / safety (see the patch for the full argument).
> >>>
> >>> As part of this patch series I am end up slightly cleaning up some of
> >>> the interactions between the PCI subsystem and the IOMMU subsystem but
> >>> I don't go all the way to fully remove all the tentacles. Specifically
> >>> this patch series only concerns itself with a single aspect: strict
> >>> vs. non-strict mode for the IOMMU. I'm hoping that this will be easier
> >>> to talk about / reason about for more subsystems compared to overall
> >>> deciding what it means for a device to be "external" or "untrusted".
> >>>
> >>> If something like this patch series ends up being landable, it will
> >>> undoubtedly need coordination between many maintainers to land. I
> >>> believe it's fully bisectable but later patches in the series
> >>> definitely depend on earlier ones. Sorry for the long CC list. :(
> >>
> >> Unfortunately, this doesn't work. In normal operation, the default
> >> domains should be established long before individual drivers are even
> >> loaded (if they are modules), let alone anywhere near probing. The fact
> >> that iommu_probe_device() sometimes gets called far too late off the
> >> back of driver probe is an unfortunate artefact of the original
> >> probe-deferral scheme, and causes other problems like potentially
> >> malformed groups - I've been forming a plan to fix that for a while now,
> >> so I for one really can't condone anything trying to rely on it.
> >> Non-deterministic behaviour based on driver probe order for multi-device
> >> groups is part of the existing problem, and your proposal seems equally
> >> vulnerable to that too.
> >
> > Doh! :( I definitely can't say I understand the iommu subsystem
> > amazingly well. It was working for me, but I could believe that I was
> > somehow violating a rule somewhere.
> >
> > I'm having a bit of a hard time understanding where the problem is
> > though. Is there any chance that you missed the part of my series
> > where I introduced a "pre_probe" step? Specifically, I see this:
> >
> > * really_probe() is called w/ a driver and a device.
> > * -> calls dev->bus->dma_configure() w/ a "struct device *"
> > * -> eventually calls iommu_probe_device() w/ the device.
>
> This...
>
> > * -> calls iommu_alloc_default_domain() w/ the device
> > * -> calls iommu_group_alloc_default_domain()
> > * -> always allocates a new domain
> >
> > ...so we always have a "struct device" when a domain is allocated if
> > that domain is going to be associated with a device.
> >
> > I will agree that iommu_probe_device() is called before the driver
> > probe, but unless I missed something it's after the device driver is
> > loaded.  ...and assuming something like patch #1 in this series looks
> > OK then iommu_probe_device() will be called after "pre_probe".
> >
> > So assuming I'm not missing something, I'm not actually relying the
> > IOMMU getting init off the back of driver probe.
>
> ...is implicitly that. Sorry that it's not obvious.
>
> The "proper" flow is that iommu_probe_device() is called for everything
> which already exists during the IOMMU driver's own probe when it calls
> bus_set_iommu(), then at BUS_NOTIFY_ADD_DEVICE time for everything which
> appears subsequently. The only trouble is, to observe it in action on a
> DT-based system you'd currently have to go back at least 4 years, before
> 09515ef5ddad...
>
> Basically there were two issues: firstly we need the of_xlate step
> before add_device (now probe_device) for a DT-based IOMMU driver to know
> whether it should care about the given device or not. When -EPROBE_DEFER
> was the only tool we had to ensure probe ordering, and resolving the
> "iommus" D

Re: [PATCH 0/6] iommu: Enable devices to request non-strict DMA, starting with QCom SD/MMC

[Doug Anderson]

Hi,

On Tue, Jun 22, 2021 at 1:06 PM Saravana Kannan  wrote:
>
> On Tue, Jun 22, 2021 at 1:02 PM Rob Herring  wrote:
> >
> > On Tue, Jun 22, 2021 at 09:06:02AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Tue, Jun 22, 2021 at 4:35 AM Robin Murphy  wrote:
> > > >
> > > > Hi Doug,
> > > >
> > > > On 2021-06-22 00:52, Douglas Anderson wrote:
> > > > >
> > > > > This patch attempts to put forward a proposal for enabling non-strict
> > > > > DMA on a device-by-device basis. The patch series requests non-strict
> > > > > DMA for the Qualcomm SDHCI controller as a first device to enable,
> > > > > getting a nice bump in performance with what's believed to be a very
> > > > > small drop in security / safety (see the patch for the full argument).
> > > > >
> > > > > As part of this patch series I am end up slightly cleaning up some of
> > > > > the interactions between the PCI subsystem and the IOMMU subsystem but
> > > > > I don't go all the way to fully remove all the tentacles. Specifically
> > > > > this patch series only concerns itself with a single aspect: strict
> > > > > vs. non-strict mode for the IOMMU. I'm hoping that this will be easier
> > > > > to talk about / reason about for more subsystems compared to overall
> > > > > deciding what it means for a device to be "external" or "untrusted".
> > > > >
> > > > > If something like this patch series ends up being landable, it will
> > > > > undoubtedly need coordination between many maintainers to land. I
> > > > > believe it's fully bisectable but later patches in the series
> > > > > definitely depend on earlier ones. Sorry for the long CC list. :(
> > > >
> > > > Unfortunately, this doesn't work. In normal operation, the default
> > > > domains should be established long before individual drivers are even
> > > > loaded (if they are modules), let alone anywhere near probing. The fact
> > > > that iommu_probe_device() sometimes gets called far too late off the
> > > > back of driver probe is an unfortunate artefact of the original
> > > > probe-deferral scheme, and causes other problems like potentially
> > > > malformed groups - I've been forming a plan to fix that for a while now,
> > > > so I for one really can't condone anything trying to rely on it.
> > > > Non-deterministic behaviour based on driver probe order for multi-device
> > > > groups is part of the existing problem, and your proposal seems equally
> > > > vulnerable to that too.
> > >
> > > Doh! :( I definitely can't say I understand the iommu subsystem
> > > amazingly well. It was working for me, but I could believe that I was
> > > somehow violating a rule somewhere.
> > >
> > > I'm having a bit of a hard time understanding where the problem is
> > > though. Is there any chance that you missed the part of my series
> > > where I introduced a "pre_probe" step? Specifically, I see this:
> > >
> > > * really_probe() is called w/ a driver and a device.
> > > * -> calls dev->bus->dma_configure() w/ a "struct device *"
> > > * -> eventually calls iommu_probe_device() w/ the device.
> > > * -> calls iommu_alloc_default_domain() w/ the device
> > > * -> calls iommu_group_alloc_default_domain()
> > > * -> always allocates a new domain
> > >
> > > ...so we always have a "struct device" when a domain is allocated if
> > > that domain is going to be associated with a device.
> > >
> > > I will agree that iommu_probe_device() is called before the driver
> > > probe, but unless I missed something it's after the device driver is
> > > loaded.  ...and assuming something like patch #1 in this series looks
> > > OK then iommu_probe_device() will be called after "pre_probe".
> > >
> > > So assuming I'm not missing something, I'm not actually relying the
> > > IOMMU getting init off the back of driver probe.
> > >
> > >
> > > > FWIW we already have a go-faster knob for people who want to tweak the
> > > > security/performance compromise for specific devices, namely the sysfs
> > > > interface for changing a group's domain type before binding the relevant
> > > > driver(s). Is that something you could use in your

Re: [PATCH 0/6] iommu: Enable devices to request non-strict DMA, starting with QCom SD/MMC

[Doug Anderson]

Hi,

On Tue, Jun 22, 2021 at 10:46 AM John Garry  wrote:
>
> On 22/06/2021 00:52, Douglas Anderson wrote:
> >
> > This patch attempts to put forward a proposal for enabling non-strict
> > DMA on a device-by-device basis. The patch series requests non-strict
> > DMA for the Qualcomm SDHCI controller as a first device to enable,
> > getting a nice bump in performance with what's believed to be a very
> > small drop in security / safety (see the patch for the full argument).
> >
> > As part of this patch series I am end up slightly cleaning up some of
> > the interactions between the PCI subsystem and the IOMMU subsystem but
> > I don't go all the way to fully remove all the tentacles. Specifically
> > this patch series only concerns itself with a single aspect: strict
> > vs. non-strict mode for the IOMMU. I'm hoping that this will be easier
> > to talk about / reason about for more subsystems compared to overall
> > deciding what it means for a device to be "external" or "untrusted".
> >
> > If something like this patch series ends up being landable, it will
> > undoubtedly need coordination between many maintainers to land. I
> > believe it's fully bisectable but later patches in the series
> > definitely depend on earlier ones. Sorry for the long CC list. :(
> >
>
> JFYI, In case to missed it, and I know it's not the same thing as you
> want, above, but the following series will allow you to build the kernel
> to default to lazy mode:
>
> https://lore.kernel.org/linux-iommu/1624016058-189713-1-git-send-email-john.ga...@huawei.com/T/#m21bc07b9353b3ba85f2a40557645c2bcc13cbb3e
>
> So iommu.strict=0 would be no longer always required for arm64.

Excellent! I'm never a fan of command line parameters as a replacement
for Kconfig. They are great for debugging or for cases where the user
of the kernel and the person compiling the kernel are not the same
(like with off-the-shelf Linux distros) but aren't great for setting a
default for embedded environments.

I actually think that something like my patchset may be even more
important atop yours. Do you agree? If the default is non-strict it
could be extra important to be able to mark a certain device to be in
"strict" mode.

...also, unfortunately I probably won't be able to use your patchest
for my use case. I think we want the extra level of paranoia by
default and really only want to allow non-strict mode for devices that
we're really convinced are safe.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default

[Doug Anderson]

Hi,

On Tue, Jun 22, 2021 at 11:45 AM Rajat Jain  wrote:
>
> On Mon, Jun 21, 2021 at 4:53 PM Douglas Anderson  
> wrote:
> >
> > In the patch ("drivers: base: Add bits to struct device to control
> > iommu strictness") we add the ability for devices to tell us about
> > their IOMMU strictness requirements. Let's now take that into account
> > in the IOMMU layer.
> >
> > A few notes here:
> > * Presumably this is always how iommu_get_dma_strict() was intended to
> >   behave. Had this not been the intention then it never would have
> >   taken a domain as a parameter.
> > * The iommu_set_dma_strict() feels awfully non-symmetric now. That
> >   function sets the _default_ strictness globally in the system
> >   whereas iommu_get_dma_strict() returns the value for a given domain
> >   (falling back to the default). Presumably, at least, the fact that
> >   iommu_set_dma_strict() doesn't take a domain makes this obvious.
> >
> > The function iommu_get_dma_strict() should now make it super obvious
> > where strictness comes from and who overides who. Though the function
> > changed a bunch to make the logic clearer, the only two new rules
> > should be:
> > * Devices can force strictness for themselves, overriding the cmdline
> >   "iommu.strict=0" or a call to iommu_set_dma_strict(false)).
> > * Devices can request non-strictness for themselves, assuming there
> >   was no cmdline "iommu.strict=1" or a call to
> >   iommu_set_dma_strict(true).
>
> Along the same lines, I believe a platform (device tree / ACPI) should
> also be able to have a say in this. I assume in your proposal, a
> platform would expose a property in device tree which the device
> driver would need to parse and then use it to set these bits in the
> "struct device"?

Nothing would prevent creating a device tree or ACPI property that
caused either "force-strict" or "request-non-strict" from being set if
everyone agrees that it's a good idea. I wouldn't reject the idea
myself, but I do worry that we'd devolve into the usual bikeshed for
exactly how this should look. I talked about this a bit in my response
to Saravana, but basically:

* If there was some generic property, would we call it "untrusted",
"external", or something else?

* How do you describe "trust" in a generic "objective" way? It's not
really boolean and trying to describe exactly how trustworthy
something should be considered is hard.

* At least for the device tree there's a general requirement that it
describes the hardware and not so much how the software should
configure the hardware. As I understand it there is _some_ leeway here
where it's OK to describe how the hardware was designed for the OS to
configure it, but it's a pretty high bar and a hard sell. In general
the device tree isn't supposed to be used to describe "policy". In
other words: if one OS might decide on one setting and another OS on
another then it doesn't really belong in the device tree.

* In general the kernel is also not really supposed to have policy
hardcoded in either, though it feels like we can get away with having
a good default/sane policy and allowing overriding the policy with
command line parameters (like iommu.strict). In the case where
something has to be configured at bootup there's not many ways to do
better.


tl;dr: I have no plans to try to make an overarching property, but my
patch series does allow subsystems to come up with and easily
implement their own rules as it makes sense. While this might seem
hodgepodge I prefer to see it as "flexible" since I'm not convinced
that we're going to be able to come up with an overarching trust
framework.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default

[Doug Anderson]

Hi,

On Tue, Jun 22, 2021 at 9:53 AM Doug Anderson  wrote:
>
> Hi,
>
> On Mon, Jun 21, 2021 at 7:05 PM Lu Baolu  wrote:
> >
> > On 6/22/21 7:52 AM, Douglas Anderson wrote:
> > > @@ -1519,7 +1542,8 @@ static int iommu_get_def_domain_type(struct device 
> > > *dev)
> > >
> > >   static int iommu_group_alloc_default_domain(struct bus_type *bus,
> > >   struct iommu_group *group,
> > > - unsigned int type)
> > > + unsigned int type,
> > > + struct device *dev)
> > >   {
> > >   struct iommu_domain *dom;
> > >
> > > @@ -1534,6 +1558,12 @@ static int iommu_group_alloc_default_domain(struct 
> > > bus_type *bus,
> > >   if (!dom)
> > >   return -ENOMEM;
> > >
> > > + /* Save the strictness requests from the device */
> > > + if (dev && type == IOMMU_DOMAIN_DMA) {
> > > + dom->request_non_strict = dev->request_non_strict_iommu;
> > > + dom->force_strict = dev->force_strict_iommu;
> > > + }
> > > +
> >
> > An iommu default domain might be used by multiple devices which might
> > have different "strict" attributions. Then who could override who?
>
> My gut instinct would be that if multiple devices were part of a given
> domain that it would be combined like this:
>
> 1. Any device that requests strict makes the domain strict force strict.
>
> 2. To request non-strict all of the devices in the domain would have
> to request non-strict.
>
> To do that I'd have to change my patchset obviously, but I don't think
> it should be hard. We can just keep a count of devices and a count of
> the strict vs. non-strict requests? If there are no other blockers
> I'll try to do that in my v2.

One issue, I guess, is that we might need to transition a non-strict
domain to become strict. Is that possible? We'd end up with an extra
"flush queue" that we didn't need to allocate, but are there other
problems?

Actually, in general would it be possible to transition between strict
and non-strict at runtime as long as we called
init_iova_flush_queue()? Maybe that's a better solution than all
this--we just boot in strict mode and can just transition to
non-strict mode after-the-fact?


-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default

[Doug Anderson]

Hi,

On Mon, Jun 21, 2021 at 7:05 PM Lu Baolu  wrote:
>
> On 6/22/21 7:52 AM, Douglas Anderson wrote:
> > @@ -1519,7 +1542,8 @@ static int iommu_get_def_domain_type(struct device 
> > *dev)
> >
> >   static int iommu_group_alloc_default_domain(struct bus_type *bus,
> >   struct iommu_group *group,
> > - unsigned int type)
> > + unsigned int type,
> > + struct device *dev)
> >   {
> >   struct iommu_domain *dom;
> >
> > @@ -1534,6 +1558,12 @@ static int iommu_group_alloc_default_domain(struct 
> > bus_type *bus,
> >   if (!dom)
> >   return -ENOMEM;
> >
> > + /* Save the strictness requests from the device */
> > + if (dev && type == IOMMU_DOMAIN_DMA) {
> > + dom->request_non_strict = dev->request_non_strict_iommu;
> > + dom->force_strict = dev->force_strict_iommu;
> > + }
> > +
>
> An iommu default domain might be used by multiple devices which might
> have different "strict" attributions. Then who could override who?

My gut instinct would be that if multiple devices were part of a given
domain that it would be combined like this:

1. Any device that requests strict makes the domain strict force strict.

2. To request non-strict all of the devices in the domain would have
to request non-strict.

To do that I'd have to change my patchset obviously, but I don't think
it should be hard. We can just keep a count of devices and a count of
the strict vs. non-strict requests? If there are no other blockers
I'll try to do that in my v2.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default

[Doug Anderson]

Hi,

On Mon, Jun 21, 2021 at 7:56 PM Saravana Kannan  wrote:
>
> On Mon, Jun 21, 2021 at 4:53 PM Douglas Anderson  
> wrote:
> >
> > In the patch ("drivers: base: Add bits to struct device to control
> > iommu strictness") we add the ability for devices to tell us about
> > their IOMMU strictness requirements. Let's now take that into account
> > in the IOMMU layer.
> >
> > A few notes here:
> > * Presumably this is always how iommu_get_dma_strict() was intended to
> >   behave. Had this not been the intention then it never would have
> >   taken a domain as a parameter.
> > * The iommu_set_dma_strict() feels awfully non-symmetric now. That
> >   function sets the _default_ strictness globally in the system
> >   whereas iommu_get_dma_strict() returns the value for a given domain
> >   (falling back to the default). Presumably, at least, the fact that
> >   iommu_set_dma_strict() doesn't take a domain makes this obvious.
> >
> > The function iommu_get_dma_strict() should now make it super obvious
> > where strictness comes from and who overides who. Though the function
> > changed a bunch to make the logic clearer, the only two new rules
> > should be:
> > * Devices can force strictness for themselves, overriding the cmdline
> >   "iommu.strict=0" or a call to iommu_set_dma_strict(false)).
> > * Devices can request non-strictness for themselves, assuming there
> >   was no cmdline "iommu.strict=1" or a call to
> >   iommu_set_dma_strict(true).
> >
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> >  drivers/iommu/iommu.c | 56 +--
> >  include/linux/iommu.h |  2 ++
> >  2 files changed, 45 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 808ab70d5df5..0c84a4c06110 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -28,8 +28,19 @@
> >  static struct kset *iommu_group_kset;
> >  static DEFINE_IDA(iommu_group_ida);
> >
> > +enum iommu_strictness {
> > +   IOMMU_DEFAULT_STRICTNESS = -1,
> > +   IOMMU_NOT_STRICT = 0,
> > +   IOMMU_STRICT = 1,
> > +};
> > +static inline enum iommu_strictness bool_to_strictness(bool strictness)
> > +{
> > +   return (enum iommu_strictness)strictness;
> > +}
> > +
> >  static unsigned int iommu_def_domain_type __read_mostly;
> > -static bool iommu_dma_strict __read_mostly = true;
> > +static enum iommu_strictness cmdline_dma_strict __read_mostly = 
> > IOMMU_DEFAULT_STRICTNESS;
> > +static enum iommu_strictness driver_dma_strict __read_mostly = 
> > IOMMU_DEFAULT_STRICTNESS;
> >  static u32 iommu_cmd_line __read_mostly;
> >
> >  struct iommu_group {
> > @@ -69,7 +80,6 @@ static const char * const iommu_group_resv_type_string[] 
> > = {
> >  };
> >
> >  #define IOMMU_CMD_LINE_DMA_API BIT(0)
> > -#define IOMMU_CMD_LINE_STRICT  BIT(1)
> >
> >  static int iommu_alloc_default_domain(struct iommu_group *group,
> >   struct device *dev);
> > @@ -336,25 +346,38 @@ early_param("iommu.passthrough", 
> > iommu_set_def_domain_type);
> >
> >  static int __init iommu_dma_setup(char *str)
> >  {
> > -   int ret = kstrtobool(str, _dma_strict);
> > +   bool strict;
> > +   int ret = kstrtobool(str, );
> >
> > if (!ret)
> > -   iommu_cmd_line |= IOMMU_CMD_LINE_STRICT;
> > +   cmdline_dma_strict = bool_to_strictness(strict);
> > return ret;
> >  }
> >  early_param("iommu.strict", iommu_dma_setup);
> >
> >  void iommu_set_dma_strict(bool strict)
> >  {
> > -   if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
> > -   iommu_dma_strict = strict;
> > +   /* A driver can request strictness but not the other way around */
> > +   if (driver_dma_strict != IOMMU_STRICT)
> > +   driver_dma_strict = bool_to_strictness(strict);
> >  }
> >
> >  bool iommu_get_dma_strict(struct iommu_domain *domain)
> >  {
> > -   /* only allow lazy flushing for DMA domains */
> > -   if (domain->type == IOMMU_DOMAIN_DMA)
> > -   return iommu_dma_strict;
> > +   /* Non-DMA domains or anyone forcing it to strict makes it strict */
> > +   if (domain->type != IOMMU_DOMAIN_DMA ||
> > +   cmdline_dma_strict == IOMMU_STRICT ||
> > +   driver_dma_strict == IOMMU_STRICT ||
> > +   domain->force_strict)
> > +   return true;
> > +
> > +   /* Anyone requesting non-strict (if no forces) makes it non-strict 
> > */
> > +   if (cmdline_dma_strict == IOMMU_NOT_STRICT ||
> > +   driver_dma_strict == IOMMU_NOT_STRICT ||
> > +   domain->request_non_strict)
> > +   return false;
> > +
> > +   /* Nobody said anything, so it's strict by default */
>
> If iommu.strict is not set in the command line, upstream treats it as
> iommu.strict=1. Meaning, no drivers can override it.
>
> If I understand it correctly, with your series, if iommu.strict=1 is
> not set, 

Re: [PATCH 0/6] iommu: Enable devices to request non-strict DMA, starting with QCom SD/MMC

[Doug Anderson]

Hi,

On Tue, Jun 22, 2021 at 4:35 AM Robin Murphy  wrote:
>
> Hi Doug,
>
> On 2021-06-22 00:52, Douglas Anderson wrote:
> >
> > This patch attempts to put forward a proposal for enabling non-strict
> > DMA on a device-by-device basis. The patch series requests non-strict
> > DMA for the Qualcomm SDHCI controller as a first device to enable,
> > getting a nice bump in performance with what's believed to be a very
> > small drop in security / safety (see the patch for the full argument).
> >
> > As part of this patch series I am end up slightly cleaning up some of
> > the interactions between the PCI subsystem and the IOMMU subsystem but
> > I don't go all the way to fully remove all the tentacles. Specifically
> > this patch series only concerns itself with a single aspect: strict
> > vs. non-strict mode for the IOMMU. I'm hoping that this will be easier
> > to talk about / reason about for more subsystems compared to overall
> > deciding what it means for a device to be "external" or "untrusted".
> >
> > If something like this patch series ends up being landable, it will
> > undoubtedly need coordination between many maintainers to land. I
> > believe it's fully bisectable but later patches in the series
> > definitely depend on earlier ones. Sorry for the long CC list. :(
>
> Unfortunately, this doesn't work. In normal operation, the default
> domains should be established long before individual drivers are even
> loaded (if they are modules), let alone anywhere near probing. The fact
> that iommu_probe_device() sometimes gets called far too late off the
> back of driver probe is an unfortunate artefact of the original
> probe-deferral scheme, and causes other problems like potentially
> malformed groups - I've been forming a plan to fix that for a while now,
> so I for one really can't condone anything trying to rely on it.
> Non-deterministic behaviour based on driver probe order for multi-device
> groups is part of the existing problem, and your proposal seems equally
> vulnerable to that too.

Doh! :( I definitely can't say I understand the iommu subsystem
amazingly well. It was working for me, but I could believe that I was
somehow violating a rule somewhere.

I'm having a bit of a hard time understanding where the problem is
though. Is there any chance that you missed the part of my series
where I introduced a "pre_probe" step? Specifically, I see this:

* really_probe() is called w/ a driver and a device.
* -> calls dev->bus->dma_configure() w/ a "struct device *"
* -> eventually calls iommu_probe_device() w/ the device.
* -> calls iommu_alloc_default_domain() w/ the device
* -> calls iommu_group_alloc_default_domain()
* -> always allocates a new domain

...so we always have a "struct device" when a domain is allocated if
that domain is going to be associated with a device.

I will agree that iommu_probe_device() is called before the driver
probe, but unless I missed something it's after the device driver is
loaded.  ...and assuming something like patch #1 in this series looks
OK then iommu_probe_device() will be called after "pre_probe".

So assuming I'm not missing something, I'm not actually relying the
IOMMU getting init off the back of driver probe.


> FWIW we already have a go-faster knob for people who want to tweak the
> security/performance compromise for specific devices, namely the sysfs
> interface for changing a group's domain type before binding the relevant
> driver(s). Is that something you could use in your application, say from
> an initramfs script?

We've never had an initramfs script in Chrome OS. I don't know all the
history of why (I'm trying to check), but I'm nearly certain it was a
conscious decision. Probably it has to do with the fact that we're not
trying to build a generic distribution where a single boot source can
boot a huge variety of hardware. We generally have one kernel for a
class of devices. I believe avoiding the initramfs just keeps things
simpler.

I think trying to revamp Chrome OS to switch to an initramfs type
system would be a pretty big undertaking since (as I understand it)
you can't just run a little command and then return to the normal boot
flow. Once you switch to initramfs you're committing to finding /
setting up the rootfs yourself and on Chrome OS I believe that means a
whole bunch of dm-verity work.


...so probably the initramfs is a no-go for me, but I'm still crossing
my fingers that the pre_probe() might be legit if you take a second
look at it?

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCHv2 2/3] iommu/io-pgtable: Optimize partial walk flush for large scatter-gather list

[Doug Anderson]

Hi,

On Thu, Jun 17, 2021 at 7:51 PM Sai Prakash Ranjan
 wrote:
>
> Currently for iommu_unmap() of large scatter-gather list with page size
> elements, the majority of time is spent in flushing of partial walks in
> __arm_lpae_unmap() which is a VA based TLB invalidation invalidating
> page-by-page on iommus like arm-smmu-v2 (TLBIVA) which do not support
> range based invalidations like on arm-smmu-v3.2.
>
> For example: to unmap a 32MB scatter-gather list with page size elements
> (8192 entries), there are 16->2MB buffer unmaps based on the pgsize (2MB
> for 4K granule) and each of 2MB will further result in 512 TLBIVAs (2MB/4K)
> resulting in a total of 8192 TLBIVAs (512*16) for 16->2MB causing a huge
> overhead.
>
> So instead use tlb_flush_all() callback (TLBIALL/TLBIASID) to invalidate
> the entire context for partial walk flush on select few platforms where
> cost of over-invalidation is less than unmap latency

It would probably be worth punching this description up a little bit.
Elsewhere you said in more detail why this over-invalidation is less
of a big deal for the Qualcomm SMMU. It's probably worth saying
something like that here, too. Like this bit paraphrased from your
other email:

On qcom impl, we have several performance improvements for TLB cache
invalidations in HW like wait-for-safe (for realtime clients such as
camera and display) and few others to allow for cache lookups/updates
when TLBI is in progress for the same context bank.


> using the newly
> introduced quirk IO_PGTABLE_QUIRK_TLB_INV_ALL. We also do this for
> non-strict mode given its all about over-invalidation saving time on
> individual unmaps and non-deterministic generally.

As per usual I'm mostly clueless, but I don't quite understand why you
want this new behavior for non-strict mode. To me it almost seems like
the opposite? Specifically, non-strict mode is already outside the
critical path today and so there's no need to optimize it. I'm
probably not explaining myself clearly, but I guess i'm thinking:

a) today for strict, unmap is in the critical path and it's important
to get it out of there. Getting it out of the critical path is so
important that we're willing to over-invalidate to speed up the
critical path.

b) today for non-strict, unmap is not in the critical path.

So I would almost expect your patch to _disable_ your new feature for
non-strict mappings, not auto-enable your new feature for non-strict
mappings.

If I'm babbling, feel free to ignore. ;-) Looking back, I guess Robin
was the one that suggested the behavior you're implementing, so it's
more likely he's right than I am. ;-)


-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/7] iommu: Move iotlb_sync_map out from __iommu_map

[Doug Anderson]

Hi,

On Thu, Jan 7, 2021 at 4:31 AM Yong Wu  wrote:
>
> @@ -2438,18 +2435,31 @@ static int __iommu_map(struct iommu_domain *domain, 
> unsigned long iova,
> return ret;
>  }
>
> +static int _iommu_map(struct iommu_domain *domain, unsigned long iova,
> + phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
> +{
> +   const struct iommu_ops *ops = domain->ops;
> +   int ret;
> +
> +   ret = __iommu_map(domain, iova, paddr, size, prot, GFP_KERNEL);

The above is broken.  Instead of GFP_KERNEL it should be passing "gfp".


> +   if (ret == 0 && ops->iotlb_sync_map)
> +   ops->iotlb_sync_map(domain);
> +
> +   return ret;
> +}
> +
>  int iommu_map(struct iommu_domain *domain, unsigned long iova,
>   phys_addr_t paddr, size_t size, int prot)
>  {
> might_sleep();
> -   return __iommu_map(domain, iova, paddr, size, prot, GFP_KERNEL);
> +   return _iommu_map(domain, iova, paddr, size, prot, GFP_KERNEL);
>  }
>  EXPORT_SYMBOL_GPL(iommu_map);
>
>  int iommu_map_atomic(struct iommu_domain *domain, unsigned long iova,
>   phys_addr_t paddr, size_t size, int prot)
>  {
> -   return __iommu_map(domain, iova, paddr, size, prot, GFP_ATOMIC);
> +   return _iommu_map(domain, iova, paddr, size, prot, GFP_ATOMIC);

Specifically the above bug means we drop the "GFP_ATOMIC" here.

It means we trigger a warning, like this (on a downstream kernel with
the patch backported):

 BUG: sleeping function called from invalid context at mm/page_alloc.c:4726
 in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 9, name: ksoftirqd/0
 CPU: 0 PID: 9 Comm: ksoftirqd/0 Not tainted 5.4.93-12508-gc10c93e28e39 #1
 Call trace:
  dump_backtrace+0x0/0x154
  show_stack+0x20/0x2c
  dump_stack+0xa0/0xfc
  ___might_sleep+0x11c/0x12c
  __might_sleep+0x50/0x84
  __alloc_pages_nodemask+0xf8/0x2bc
  __arm_lpae_alloc_pages+0x48/0x1b4
  __arm_lpae_map+0x124/0x274
  __arm_lpae_map+0x1cc/0x274
  arm_lpae_map+0x140/0x170
  arm_smmu_map+0x78/0xbc
  __iommu_map+0xd4/0x210
  _iommu_map+0x4c/0x84
  iommu_map_atomic+0x44/0x58
  __iommu_dma_map+0x8c/0xc4
  iommu_dma_map_page+0xac/0xf0

---

A quick (but not very tested) fix at:

https://lore.kernel.org/r/20210201170611.1.I64a7b62579287d668d7c89e105dcedf45d641063@changeid/


-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names

[Doug Anderson]

Hi,

On Wed, Aug 26, 2020 at 8:01 AM Sai Prakash Ranjan
 wrote:
>
> On 2020-08-26 19:21, Robin Murphy wrote:
> > On 2020-08-26 13:17, Sai Prakash Ranjan wrote:
> >> On 2020-08-26 17:07, Robin Murphy wrote:
> >>> On 2020-08-25 16:42, Sai Prakash Ranjan wrote:
>  Currently the non-strict or lazy mode of TLB invalidation can only
>  be set
>  for all or no domains. This works well for development platforms
>  where
>  setting to non-strict/lazy mode is fine for performance reasons but
>  on
>  production devices, we need a more fine grained control to allow
>  only
>  certain peripherals to support this mode where we can be sure that
>  it is
>  safe. So add support to filter non-strict/lazy mode based on the
>  device
>  names that are passed via cmdline parameter
>  "iommu.nonstrict_device".
> >>>
> >>> There seems to be considerable overlap here with both the existing
> >>> patches for per-device default domain control [1], and the broader
> >>> ongoing development on how to define, evaluate and handle "trusted"
> >>> vs. "untrusted" devices (e.g. [2],[3]). I'd rather see work done to
> >>> make sure those integrate properly together and work well for
> >>> everyone's purposes, than add more disjoint mechanisms that only
> >>> address small pieces of the overall issue.
> >>>
> >>> Robin.
> >>>
> >>> [1]
> >>> https://lore.kernel.org/linux-iommu/20200824051726.7xaJRTTszJuzdFWGJ8YNsshCtfNR0BNeMrlILAyqt_0@z/
> >>> [2]
> >>> https://lore.kernel.org/linux-iommu/20200630044943.3425049-1-raja...@google.com/
> >>> [3]
> >>> https://lore.kernel.org/linux-iommu/20200626002710.110200-2-raja...@google.com/
> >>
> >> Thanks for the links, [1] definitely sounds interesting, I was under
> >> the impression
> >> that changing such via sysfs is late, but seems like other Sai has got
> >> it working
> >> for the default domain type. So we can extend that and add a strict
> >> attribute as well,
> >> we should be definitely OK with system booting with default strict
> >> mode for all
> >> peripherals as long as we have an option to change that later, Doug?
> >
> > Right, IIRC there was initially a proposal of a command line option
> > there too, and it faced the same criticism around not being very
> > generic or scalable. I believe sysfs works as a reasonable compromise
> > since in many cases it can be tweaked relatively early from an initrd,
> > and non-essential devices can effectively be switched at any time by
> > removing and reprobing their driver.
> >
>
> Ah I see, so the catch is that device must not be bound to the driver
> and won't work for the internal devices or builtin drivers probed early.

Hrm, that wouldn't work so well for us for eMMC.  I don't think I'm
going to manage to convince folks that we need an initrd just for
this.  I'm probably being naive and I haven't looked at the code, but
it does seem a little weird that this isn't the kind of thing that
could just be tweaked for transfers going forward...


> > As for a general approach for internal devices where you do believe
> > the hardware is honest but don't necessarily trust whatever firmware
> > it happens to be running, I'm pretty sure that's come up already, but
> > I'll be sure to mention it at Rajat's imminent LPC talk if nobody else
> > does.

I'll at least attend.  We'll see how useful my contributions are
since, as per usual, I'm wandering into an area I'm not an expert in
here.  ;-)

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names

[Doug Anderson]

Hi,

On Tue, Aug 25, 2020 at 3:54 PM Rob Clark  wrote:
>
> On Tue, Aug 25, 2020 at 3:23 PM Doug Anderson  wrote:
> >
> > Hi,
> >
> > On Tue, Aug 25, 2020 at 12:01 PM Sai Prakash Ranjan
> >  wrote:
> > >
> > > Hi,
> > >
> > > On 2020-08-25 21:40, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Tue, Aug 25, 2020 at 8:43 AM Sai Prakash Ranjan
> > > >  wrote:
> > > >>
> > > >> Currently the non-strict or lazy mode of TLB invalidation can only be
> > > >> set
> > > >> for all or no domains. This works well for development platforms where
> > > >> setting to non-strict/lazy mode is fine for performance reasons but on
> > > >> production devices, we need a more fine grained control to allow only
> > > >> certain peripherals to support this mode where we can be sure that it
> > > >> is
> > > >> safe. So add support to filter non-strict/lazy mode based on the
> > > >> device
> > > >> names that are passed via cmdline parameter "iommu.nonstrict_device".
> > > >>
> > > >> Example:
> > > >> iommu.nonstrict_device="7c4000.sdhci,a60.dwc3,6048000.etr"
> >
> > Just curious: are device names like this really guaranteed to be
> > stable across versions?
> >
> >
> > > > I have an inherent dislike of jamming things like this onto the
> > > > command line.  IMHO the command line is the last resort for specifying
> > > > configuration and generally should be limited to some specialized
> > > > debug options and cases where the person running the kernel needs to
> > > > override a config that was set by the person (or company) compiling
> > > > the kernel.  Specifically, having a long/unwieldy command line makes
> > > > it harder to use for the case when an end user actually wants to use
> > > > it to override something.  It's also just another place to look for
> > > > config.
> > > >
> > >
> > > Good thing about command line parameters are that they are optional,
> > > they do
> > > not specify any default behaviour (I mean they are not mandatory to be
> > > set
> > > for the system to be functional), so I would like to view it as an
> > > optional
> > > config. And this command line parameter (nonstrict_device) is strictly
> > > optional
> > > with default being strict already set in the driver.
> > >
> > > They can be passed from the bootloader via chosen node for DT platforms
> > > or choose
> > > a new *bootconfig* as a way to pass the cmdline but finally it does boil
> > > down to
> > > just another config.
> >
> > Never looked at bootconfig.  Unfortunately it seems to require
> > initramfs so that pretty much means it's out for my usage.  :(
> >
> >
> > > I agree with general boolean or single value command line parameters
> > > being just
> > > more messy which could just be Kconfigs instead but for multiple value
> > > parameters
> > > like these do not fit in Kconfig.
> > >
> > > As you might already know, command line also gives an advantage to the
> > > end user
> > > to configure system without building kernel, for this specific command
> > > line its
> > > very useful because the performance bump is quite noticeable when the
> > > iommu.strict
> > > is off. Now for end user who would not be interested in building entire
> > > kernel(majority)
> > > and just cares about good speeds or throughput can find this very
> > > beneficial.
> > > I am not talking about one specific OS usecase here but more in general
> > > term.
> > >
> > > > The other problem is that this doesn't necessarily scale very well.
> > > > While it works OK for embedded cases it doesn't work terribly well for
> > > > distributions.  I know that in an out-of-band thread you indicated
> > > > that it doesn't break anything that's not already broken (AKA this
> > > > doesn't fix the distro case but it doesn't make it worse), it would be
> > > > better to come up with a more universal solution.
> > > >
> > >
> > > Is the universal solution here referring to fix all the command line
> > > parameters
> > > in the kernel or this specific command line? Are we going to remove any
> > > mor

Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names

[Doug Anderson]

Hi,

On Tue, Aug 25, 2020 at 12:01 PM Sai Prakash Ranjan
 wrote:
>
> Hi,
>
> On 2020-08-25 21:40, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Aug 25, 2020 at 8:43 AM Sai Prakash Ranjan
> >  wrote:
> >>
> >> Currently the non-strict or lazy mode of TLB invalidation can only be
> >> set
> >> for all or no domains. This works well for development platforms where
> >> setting to non-strict/lazy mode is fine for performance reasons but on
> >> production devices, we need a more fine grained control to allow only
> >> certain peripherals to support this mode where we can be sure that it
> >> is
> >> safe. So add support to filter non-strict/lazy mode based on the
> >> device
> >> names that are passed via cmdline parameter "iommu.nonstrict_device".
> >>
> >> Example:
> >> iommu.nonstrict_device="7c4000.sdhci,a60.dwc3,6048000.etr"

Just curious: are device names like this really guaranteed to be
stable across versions?


> > I have an inherent dislike of jamming things like this onto the
> > command line.  IMHO the command line is the last resort for specifying
> > configuration and generally should be limited to some specialized
> > debug options and cases where the person running the kernel needs to
> > override a config that was set by the person (or company) compiling
> > the kernel.  Specifically, having a long/unwieldy command line makes
> > it harder to use for the case when an end user actually wants to use
> > it to override something.  It's also just another place to look for
> > config.
> >
>
> Good thing about command line parameters are that they are optional,
> they do
> not specify any default behaviour (I mean they are not mandatory to be
> set
> for the system to be functional), so I would like to view it as an
> optional
> config. And this command line parameter (nonstrict_device) is strictly
> optional
> with default being strict already set in the driver.
>
> They can be passed from the bootloader via chosen node for DT platforms
> or choose
> a new *bootconfig* as a way to pass the cmdline but finally it does boil
> down to
> just another config.

Never looked at bootconfig.  Unfortunately it seems to require
initramfs so that pretty much means it's out for my usage.  :(


> I agree with general boolean or single value command line parameters
> being just
> more messy which could just be Kconfigs instead but for multiple value
> parameters
> like these do not fit in Kconfig.
>
> As you might already know, command line also gives an advantage to the
> end user
> to configure system without building kernel, for this specific command
> line its
> very useful because the performance bump is quite noticeable when the
> iommu.strict
> is off. Now for end user who would not be interested in building entire
> kernel(majority)
> and just cares about good speeds or throughput can find this very
> beneficial.
> I am not talking about one specific OS usecase here but more in general
> term.
>
> > The other problem is that this doesn't necessarily scale very well.
> > While it works OK for embedded cases it doesn't work terribly well for
> > distributions.  I know that in an out-of-band thread you indicated
> > that it doesn't break anything that's not already broken (AKA this
> > doesn't fix the distro case but it doesn't make it worse), it would be
> > better to come up with a more universal solution.
> >
>
> Is the universal solution here referring to fix all the command line
> parameters
> in the kernel or this specific command line? Are we going to remove any
> more
> addition to the cmdline ;)

There are very few cases where a kernel command line parameter is the
only way to configure something.  Most of the time it's just there to
override a config.  I wouldn't suggest removing those.  I just don't
want a kernel command line parameter to be the primary way to enable
something.


> So possible other solution is the *bootconfig* which is again just
> another place
> to look for a config. So thing is that this universal solution would
> result in
> just more new fancy ways of passing configs or adding such configs to
> the drivers
> or subsystems in kernel which is pretty much similar to implementing
> policy in
> kernel which I think is frowned upon and mentioned in the other thread.
>
> > Ideally it feels like we should figure out how to tag devices in a
> > generic manner automatically (hardcode at the driver or in the device
> > tree).  I think the out-of-band discussions talked about "external
> > facing" and the like.  We co

Re: [PATCH] iommu: Add support to filter non-strict/lazy mode based on device names

[Doug Anderson]

Hi,

On Tue, Aug 25, 2020 at 8:43 AM Sai Prakash Ranjan
 wrote:
>
> Currently the non-strict or lazy mode of TLB invalidation can only be set
> for all or no domains. This works well for development platforms where
> setting to non-strict/lazy mode is fine for performance reasons but on
> production devices, we need a more fine grained control to allow only
> certain peripherals to support this mode where we can be sure that it is
> safe. So add support to filter non-strict/lazy mode based on the device
> names that are passed via cmdline parameter "iommu.nonstrict_device".
>
> Example: iommu.nonstrict_device="7c4000.sdhci,a60.dwc3,6048000.etr"

I have an inherent dislike of jamming things like this onto the
command line.  IMHO the command line is the last resort for specifying
configuration and generally should be limited to some specialized
debug options and cases where the person running the kernel needs to
override a config that was set by the person (or company) compiling
the kernel.  Specifically, having a long/unwieldy command line makes
it harder to use for the case when an end user actually wants to use
it to override something.  It's also just another place to look for
config.

The other problem is that this doesn't necessarily scale very well.
While it works OK for embedded cases it doesn't work terribly well for
distributions.  I know that in an out-of-band thread you indicated
that it doesn't break anything that's not already broken (AKA this
doesn't fix the distro case but it doesn't make it worse), it would be
better to come up with a more universal solution.

Ideally it feels like we should figure out how to tag devices in a
generic manner automatically (hardcode at the driver or in the device
tree).  I think the out-of-band discussions talked about "external
facing" and the like.  We could also, perhaps, tag devices that have
"binary blob" firmware if we wanted.  Then we'd have a policy (set by
Kconfig, perhaps overridable via commandline) that indicated the
strictness level for the various classes of devices.  So policy would
be decided by KConfig and/or command line.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 10/20] dt-bindings: arm-smmu: Add compatible string for Adreno GPU SMMU

[Doug Anderson]

Hi,

On Wed, Aug 19, 2020 at 10:36 AM Rob Clark  wrote:
>
> On Wed, Aug 19, 2020 at 10:03 AM Doug Anderson  wrote:
> >
> > Hi,
> >
> > On Mon, Aug 17, 2020 at 3:03 PM Rob Clark  wrote:
> > >
> > > From: Jordan Crouse 
> > >
> > > Every Qcom Adreno GPU has an embedded SMMU for its own use. These
> > > devices depend on unique features such as split pagetables,
> > > different stall/halt requirements and other settings. Identify them
> > > with a compatible string so that they can be identified in the
> > > arm-smmu implementation specific code.
> > >
> > > Signed-off-by: Jordan Crouse 
> > > Reviewed-by: Rob Herring 
> > > Signed-off-by: Rob Clark 
> > > ---
> > >  Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 4 
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml 
> > > b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > index 503160a7b9a0..5ec5d0d691f6 100644
> > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > @@ -40,6 +40,10 @@ properties:
> > >- qcom,sm8150-smmu-500
> > >- qcom,sm8250-smmu-500
> > >- const: arm,mmu-500
> > > +  - description: Qcom Adreno GPUs implementing "arm,smmu-v2"
> > > +items:
> > > +  - const: qcom,adreno-smmu
> > > +  - const: qcom,smmu-v2
> >
> > I know I'm kinda late to the game, but this seems weird to me,
> > especially given the later patches in the series like:
> >
> > https://lore.kernel.org/r/20200817220238.603465-19-robdcl...@gmail.com
> >
> > Specifically in that patch you can see that this IOMMU already had a
> > compatible string and we're changing it and throwing away the
> > model-specific string?  I'm guessing that you're just trying to make
> > it easier for code to identify the adreno iommu, but it seems like a
> > better way would have been to just add the adreno compatible in the
> > middle, like:
> >
> >   - description: Qcom Adreno GPUs implementing "arm,smmu-v2"
> > items:
> >   - enum:
> >   - qcom,msm8996-smmu-v2
> >   - qcom,msm8998-smmu-v2
> >   - qcom,sc7180-smmu-v2
> >   - qcom,sdm845-smmu-v2
> > - const: qcom,adreno-smmu
> > - const: qcom,smmu-v2
> >
> > Then we still have the SoC-specific compatible string in case we need
> > it but we also have the generic one?  It also means that we're not
> > deleting the old compatible string...
>
> I did bring up the thing about removing the compat string in an
> earlier revision of the series.. but then we realized that
> qcom,sc7180-smmu-v2 was never actually used anywhere.

Right, so at least there's not going to be weird issues where landing
the dts before the code change will break anything.


> But I guess we could:  compatible = "qcom,sc7180-smmu-v2",
> "qcom,adreno-smmu", "qcom,smmu-v2";

Yeah, that was what I was suggesting.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 10/20] dt-bindings: arm-smmu: Add compatible string for Adreno GPU SMMU

[Doug Anderson]

Hi,

On Mon, Aug 17, 2020 at 3:03 PM Rob Clark  wrote:
>
> From: Jordan Crouse 
>
> Every Qcom Adreno GPU has an embedded SMMU for its own use. These
> devices depend on unique features such as split pagetables,
> different stall/halt requirements and other settings. Identify them
> with a compatible string so that they can be identified in the
> arm-smmu implementation specific code.
>
> Signed-off-by: Jordan Crouse 
> Reviewed-by: Rob Herring 
> Signed-off-by: Rob Clark 
> ---
>  Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml 
> b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> index 503160a7b9a0..5ec5d0d691f6 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> @@ -40,6 +40,10 @@ properties:
>- qcom,sm8150-smmu-500
>- qcom,sm8250-smmu-500
>- const: arm,mmu-500
> +  - description: Qcom Adreno GPUs implementing "arm,smmu-v2"
> +items:
> +  - const: qcom,adreno-smmu
> +  - const: qcom,smmu-v2

I know I'm kinda late to the game, but this seems weird to me,
especially given the later patches in the series like:

https://lore.kernel.org/r/20200817220238.603465-19-robdcl...@gmail.com

Specifically in that patch you can see that this IOMMU already had a
compatible string and we're changing it and throwing away the
model-specific string?  I'm guessing that you're just trying to make
it easier for code to identify the adreno iommu, but it seems like a
better way would have been to just add the adreno compatible in the
middle, like:

  - description: Qcom Adreno GPUs implementing "arm,smmu-v2"
items:
  - enum:
  - qcom,msm8996-smmu-v2
  - qcom,msm8998-smmu-v2
  - qcom,sc7180-smmu-v2
  - qcom,sdm845-smmu-v2
- const: qcom,adreno-smmu
- const: qcom,smmu-v2

Then we still have the SoC-specific compatible string in case we need
it but we also have the generic one?  It also means that we're not
deleting the old compatible string...

-Doug


>- description: Marvell SoCs implementing "arm,mmu-500"
>  items:
>- const: marvell,ap806-smmu-500
> --
> 2.26.2
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/2] dt-bindings: arm-smmu: Add sc7180 compatible string

[Doug Anderson]

Hi,

On Mon, May 18, 2020 at 7:39 AM Will Deacon  wrote:
>
> On Fri, May 15, 2020 at 12:05:39PM -0700, Doug Anderson wrote:
> > On Fri, May 1, 2020 at 3:30 AM Sharat Masetty  
> > wrote:
> > >
> > > This patch simply adds a new compatible string for SC7180 platform.
> > >
> > > Signed-off-by: Sharat Masetty 
> > > ---
> > >  Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml 
> > > b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > index 6515dbe..986098b 100644
> > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > @@ -28,6 +28,7 @@ properties:
> > >- enum:
> > >- qcom,msm8996-smmu-v2
> > >- qcom,msm8998-smmu-v2
> > > +  - qcom,sc7180-smmu-v2
> > >- qcom,sdm845-smmu-v2
> > >- const: qcom,smmu-v2
> >
> > Is anything blocking this patch from landing now?
>
> I thought updates to the bindings usually went via Rob and the device-tree
> tree, but neither of those are on cc.
>
> Perhaps resend with that fixed?

Ah, I guess I wasn't familiar with how things worked for this file, or
maybe things have changed recently?  I'm used to most bindings going
through the same tree as the drivers that use them.  Usually if things
are at all complicated maintainers wait for an Ack from Rob (so he
should have been CCed for sure) and then land.

In this case it actually looks like Bjorn landed it in the Qualcomm
and I just didn't realize it.  That seems like it should be fine since
it's in the middle of a clause that's all Qualcomm and the change
shouldn't be controversial in any way.  :-)

Thanks!

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/2] dt-bindings: arm-smmu: Add sc7180 compatible string

[Doug Anderson]

Hi,

On Fri, May 1, 2020 at 3:30 AM Sharat Masetty  wrote:
>
> This patch simply adds a new compatible string for SC7180 platform.
>
> Signed-off-by: Sharat Masetty 
> ---
>  Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml 
> b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> index 6515dbe..986098b 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> @@ -28,6 +28,7 @@ properties:
>- enum:
>- qcom,msm8996-smmu-v2
>- qcom,msm8998-smmu-v2
> +  - qcom,sc7180-smmu-v2
>- qcom,sdm845-smmu-v2
>- const: qcom,smmu-v2

Is anything blocking this patch from landing now?

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCHv2] iommu/arm-smmu: Make remove callback message more informative

[Doug Anderson]

Hi,

On Thu, Apr 23, 2020 at 7:35 AM Doug Anderson  wrote:
>
> Hi,
>
> On Thu, Apr 23, 2020 at 2:55 AM Sai Prakash Ranjan
>  wrote:
> >
> > Currently on reboot/shutdown, the following messages are
> > displayed on the console as error messages before the
> > system reboots/shutdown as part of remove callback.
> >
> > On SC7180:
> >
> >   arm-smmu 1500.iommu: removing device with active domains!
> >   arm-smmu 504.iommu: removing device with active domains!
> >
> > Make this error message more informative and less scary.
> >
> > Reported-by: Douglas Anderson 
> > Suggested-by: Robin Murphy 
> > Signed-off-by: Sai Prakash Ranjan 
> > ---
> >  drivers/iommu/arm-smmu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Reviewed-by: Douglas Anderson 

Is this patch waiting on anything in particular now?  Do we need
reviews from Robin and/or Will?

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] arm64: dts: qcom: sc7180: Add A618 gpu dt blob

[Doug Anderson]

Hi,

On Fri, May 1, 2020 at 3:30 AM Sharat Masetty  wrote:
>
> This patch adds the required dt nodes and properties
> to enabled A618 GPU.
>
> Signed-off-by: Sharat Masetty 
> ---
> * Remove GCC_DDRSS_GPU_AXI_CLK clock reference from gpu smmu node.
>
>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 102 
> +++
>  1 file changed, 102 insertions(+)

This is the newer version of the patch:

https://lore.kernel.org/r/1581320465-15854-2-git-send-email-smase...@codeaurora.org

The change to remove the extra IOMMU clock matches our discussions and
there's no longer anything blocking this from landing.

Reviewed-by: Douglas Anderson 
Tested-by: Douglas Anderson 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/2] dt-bindings: arm-smmu: Add sc7180 compatible string

[Doug Anderson]

Hi,

On Fri, May 1, 2020 at 3:30 AM Sharat Masetty  wrote:
>
> This patch simply adds a new compatible string for SC7180 platform.
>
> Signed-off-by: Sharat Masetty 
> ---
>  Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Douglas Anderson 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCHv2] iommu/arm-smmu: Make remove callback message more informative

[Doug Anderson]

Hi,

On Thu, Apr 23, 2020 at 2:55 AM Sai Prakash Ranjan
 wrote:
>
> Currently on reboot/shutdown, the following messages are
> displayed on the console as error messages before the
> system reboots/shutdown as part of remove callback.
>
> On SC7180:
>
>   arm-smmu 1500.iommu: removing device with active domains!
>   arm-smmu 504.iommu: removing device with active domains!
>
> Make this error message more informative and less scary.
>
> Reported-by: Douglas Anderson 
> Suggested-by: Robin Murphy 
> Signed-off-by: Sai Prakash Ranjan 
> ---
>  drivers/iommu/arm-smmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Douglas Anderson 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/arm-smmu: Demote error messages to debug in shutdown callback

[Doug Anderson]

Hi,

On Tue, Mar 31, 2020 at 12:53 AM Sai Prakash Ranjan
 wrote:
>
> Hi Will,
>
> On 2020-03-31 13:14, Will Deacon wrote:
> > On Tue, Mar 31, 2020 at 01:06:11PM +0530, Sai Prakash Ranjan wrote:
> >> On 2020-03-30 23:54, Doug Anderson wrote:
> >> > On Sat, Mar 28, 2020 at 12:35 AM Sai Prakash Ranjan
> >> >  wrote:
> >> > >
> >> > > > Of course the fact that in practice we'll *always* see the warning
> >> > > > because there's no way to tear down the default DMA domains, and even
> >> > > > if all devices *have* been nicely quiesced there's no way to tell, is
> >> > > > certainly less than ideal. Like I say, it's not entirely clear-cut
> >> > > > either way...
> >> > > >
> >> > >
> >> > > Thanks for these examples, good to know these scenarios in case we
> >> > > come
> >> > > across these.
> >> > > However, if we see these error/warning messages appear everytime then
> >> > > what will be
> >> > > the credibility of these messages? We will just ignore these messages
> >> > > when
> >> > > these issues you mention actually appears because we see them
> >> > > everytime
> >> > > on
> >> > > reboot or shutdown.
> >> >
> >> > I would agree that if these messages are expected to be seen every
> >> > time, there's no way to fix them, and they're not indicative of any
> >> > problem then something should be done.  Seeing something printed at
> >> > "dev_error" level with an exclamation point (!) at the end makes me
> >> > feel like this is something that needs immediate action on my part.
> >> >
> >> > If we really can't do better but feel that the messages need to be
> >> > there, at least make them dev_info and less scary like:
> >> >
> >> >   arm-smmu 1500.iommu: turning off; DMA should be quiesced before
> >> > now
> >> >
> >> > ...that would still give you a hint in the logs that if you saw a DMA
> >> > transaction after the message that it was a bug but also wouldn't
> >> > sound scary to someone who wasn't seeing any other problems.
> >> >
> >>
> >> We can do this if Robin is OK?
> >
> > It would be nice if you could figure out which domains are still live
> > when
> > the SMMU is being shut down in your case and verify that it *is* infact
> > benign before we start making the message more friendly. As Robin said
> > earlier, rogue DMA is a real nightmare to debug.
> >
>
> I could see this error message for all the clients of apps_smmu.
> I checked manually enabling bypass and removing iommus dt property
> for each client of apps_smmu.

Any update on the status here?  If I'm reading the conversation above,
Robin said: "we'll *always* see the warning because there's no way to
tear down the default DMA domains, and even if all devices *have* been
nicely quiesced there's no way to tell".  Did I understand that
properly?  If so, it seems like it's fully expected to see this
message on every reboot and it doesn't necessarily signify anything
bad.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/arm-smmu: Demote error messages to debug in shutdown callback

[Doug Anderson]

Hi,

On Sat, Mar 28, 2020 at 12:35 AM Sai Prakash Ranjan
 wrote:
>
> > Of course the fact that in practice we'll *always* see the warning
> > because there's no way to tear down the default DMA domains, and even
> > if all devices *have* been nicely quiesced there's no way to tell, is
> > certainly less than ideal. Like I say, it's not entirely clear-cut
> > either way...
> >
>
> Thanks for these examples, good to know these scenarios in case we come
> across these.
> However, if we see these error/warning messages appear everytime then
> what will be
> the credibility of these messages? We will just ignore these messages
> when
> these issues you mention actually appears because we see them everytime
> on
> reboot or shutdown.

I would agree that if these messages are expected to be seen every
time, there's no way to fix them, and they're not indicative of any
problem then something should be done.  Seeing something printed at
"dev_error" level with an exclamation point (!) at the end makes me
feel like this is something that needs immediate action on my part.

If we really can't do better but feel that the messages need to be
there, at least make them dev_info and less scary like:

  arm-smmu 1500.iommu: turning off; DMA should be quiesced before now

...that would still give you a hint in the logs that if you saw a DMA
transaction after the message that it was a bug but also wouldn't
sound scary to someone who wasn't seeing any other problems.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu/arm-smmu: Report USF more clearly

[Doug Anderson]

Hi,

On Tue, Sep 17, 2019 at 7:45 AM Robin Murphy  wrote:
>
> Although CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT is a welcome tool
> for smoking out inadequate firmware, the failure mode is non-obvious
> and can be confusing for end users. Add some special-case reporting of
> Unidentified Stream Faults to help clarify this particular symptom.
> Since we're adding yet another print to the mix, also break out an
> explicit ratelimit state to make sure everything stays together (and
> reduce the static storage footprint a little).
>
> CC: Douglas Anderson 

nit: Cc, not CC.


> Signed-off-by: Robin Murphy 
> ---
>  drivers/iommu/arm-smmu.c | 21 -
>  drivers/iommu/arm-smmu.h |  2 ++
>  2 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index b7cf24402a94..b27020fd6c90 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -36,6 +36,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>
>  #include 
> @@ -485,6 +486,8 @@ static irqreturn_t arm_smmu_global_fault(int irq, void 
> *dev)
>  {
> u32 gfsr, gfsynr0, gfsynr1, gfsynr2;
> struct arm_smmu_device *smmu = dev;
> +   static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
> + DEFAULT_RATELIMIT_BURST);
>
> gfsr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sGFSR);
> gfsynr0 = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sGFSYNR0);
> @@ -494,11 +497,19 @@ static irqreturn_t arm_smmu_global_fault(int irq, void 
> *dev)
> if (!gfsr)
> return IRQ_NONE;
>
> -   dev_err_ratelimited(smmu->dev,
> -   "Unexpected global fault, this could be serious\n");
> -   dev_err_ratelimited(smmu->dev,
> -   "\tGFSR 0x%08x, GFSYNR0 0x%08x, GFSYNR1 0x%08x, GFSYNR2 
> 0x%08x\n",
> -   gfsr, gfsynr0, gfsynr1, gfsynr2);
> +   if (__ratelimit()) {
> +   if (IS_ENABLED(CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT) &&
> +   (gfsr & sGFSR_USF))
> +   dev_err(smmu->dev,
> +   "Blocked unknown Stream ID 0x%hx; boot with 
> \"arm-smmu.disable_bypass=0\" to allow, but this may have security 
> implications\n",

optional nit: "%#hx" instead of "0x%hx"

Reviewed-by: Douglas Anderson 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/arm-smmu: Report USF more clearly

[Doug Anderson]

Hi,

On Mon, Sep 16, 2019 at 11:00 AM Will Deacon  wrote:
>
> On Fri, Sep 13, 2019 at 03:44:12PM -0700, Doug Anderson wrote:
> > On Fri, Sep 13, 2019 at 4:48 AM Robin Murphy  wrote:
> > >
> > > Although CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT is a welcome tool
> > > for smoking out inadequate firmware, the failure mode is non-obvious
> > > and can be confusing for end users. Add some special-case reporting of
> > > Unidentified Stream Faults to help clarify this particular symptom.
> > >
> > > CC: Douglas Anderson 
> >
> > nit that I believe that "Cc" (lowercase 2nd c) is correct.
> >
> > > Signed-off-by: Robin Murphy 
> > > ---
> > >  drivers/iommu/arm-smmu.c | 5 +
> > >  drivers/iommu/arm-smmu.h | 2 ++
> > >  2 files changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > > index b7cf24402a94..76ac8c180695 100644
> > > --- a/drivers/iommu/arm-smmu.c
> > > +++ b/drivers/iommu/arm-smmu.c
> > > @@ -499,6 +499,11 @@ static irqreturn_t arm_smmu_global_fault(int irq, 
> > > void *dev)
> > > dev_err_ratelimited(smmu->dev,
> > > "\tGFSR 0x%08x, GFSYNR0 0x%08x, GFSYNR1 0x%08x, GFSYNR2 
> > > 0x%08x\n",
> > > gfsr, gfsynr0, gfsynr1, gfsynr2);
> > > +   if (IS_ENABLED(CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT) &&
> > > +   (gfsr & sGFSR_USF))
> > > +   dev_err_ratelimited(smmu->dev,
> > > +   "Stream ID %hu may not be described by firmware, 
> > > try booting with \"arm-smmu.disable_bypass=0\"\n",
> > > +   (u16)gfsynr1);
> >
> > In general it seems like a sane idea to surface an error like this.  I
> > guess a few nits:
> >
> > 1. "By firmware" might be a bit misleading.  In most cases I'm aware
> > of the problem is in the device tree that was bundled together with
> > the kernel.  If there are actually cases where firmware has baked in a
> > device tree and it got this wrong then we might want to spend time
> > figuring out what to do about it.
>
> I thought that was usually the way UEFI systems worked, where the kernel
> is updated independently of the device-tree? Either way, that should be
> what we're aiming for, even if many platforms require the two to be tied
> together.

It's my opinion that until there is a place in the kernel to "fixup"
broken device trees that were baked in firmware that it's a bad idea
to ship device trees separate from the kernel except if the device
trees are exceedingly simple.  We'll run into too many problems
otherwise, either because the kernel the device tree was written for
had downstream patches or someone just made a mistake in them and
nobody noticed.  I know device trees are supposed to be ABI, but
people make mistakes and we need a way to fix them up.

...but that's getting pretty far afield from Robin's patch.


> > 2. Presumably booting with "arm-smmu.disable_bypass=0" is in most
> > cases the least desirable option available.  I always consider kernel
> > command line parameters as something of a last resort for
> > configuration and would only be something that and end user might do
> > if they were given a kernel compiled by someone else (like if someone
> > where taking a prebuilt Linux distro and trying to install it onto a
> > generic PC).  Are you seeing cases where this is happening?  If people
> > are compiling their own kernel I'd argue that telling them to set
> > "CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT" to "no" is much better
> > than trying to jam a command line option on.  Command line options
> > don't scale well.
>
> Hmm. Recompiling seems like even more of a last resort to me!

Depends on what you're doing.  If you're not in the habit of compiling
a kernel and you're just trying to make one work then the command line
is great.  If you're trying to manage configuration for a whole bunch
of different hardware products then the command line is a terrible
place to store config.

...but I guess the summary is that we wouldn't want someone to
actually ship a kernel with this option on anyway.  ;-)


> > 3. Any chance you could make it more obvious that this change is
> > undesirable and a last resort?  AKA:
> >
> > "Stream ID x blocked for security reasons; allow anyway by booting
> > with arm-smmu.disable_bypass=0"
>
> How about:
>
>   "Blocked transaction from unknown Stream ID x; boot with
>\"arm-smmu.disable_bypass=0\" to allow these transactions, although this
>may have security implications."

Fine with me if it's not too long for an error message.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/arm-smmu: Report USF more clearly

[Doug Anderson]

Hi,

On Fri, Sep 13, 2019 at 4:48 AM Robin Murphy  wrote:
>
> Although CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT is a welcome tool
> for smoking out inadequate firmware, the failure mode is non-obvious
> and can be confusing for end users. Add some special-case reporting of
> Unidentified Stream Faults to help clarify this particular symptom.
>
> CC: Douglas Anderson 

nit that I believe that "Cc" (lowercase 2nd c) is correct.

> Signed-off-by: Robin Murphy 
> ---
>  drivers/iommu/arm-smmu.c | 5 +
>  drivers/iommu/arm-smmu.h | 2 ++
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index b7cf24402a94..76ac8c180695 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -499,6 +499,11 @@ static irqreturn_t arm_smmu_global_fault(int irq, void 
> *dev)
> dev_err_ratelimited(smmu->dev,
> "\tGFSR 0x%08x, GFSYNR0 0x%08x, GFSYNR1 0x%08x, GFSYNR2 
> 0x%08x\n",
> gfsr, gfsynr0, gfsynr1, gfsynr2);
> +   if (IS_ENABLED(CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT) &&
> +   (gfsr & sGFSR_USF))
> +   dev_err_ratelimited(smmu->dev,
> +   "Stream ID %hu may not be described by firmware, try 
> booting with \"arm-smmu.disable_bypass=0\"\n",
> +   (u16)gfsynr1);

In general it seems like a sane idea to surface an error like this.  I
guess a few nits:

1. "By firmware" might be a bit misleading.  In most cases I'm aware
of the problem is in the device tree that was bundled together with
the kernel.  If there are actually cases where firmware has baked in a
device tree and it got this wrong then we might want to spend time
figuring out what to do about it.

2. Presumably booting with "arm-smmu.disable_bypass=0" is in most
cases the least desirable option available.  I always consider kernel
command line parameters as something of a last resort for
configuration and would only be something that and end user might do
if they were given a kernel compiled by someone else (like if someone
where taking a prebuilt Linux distro and trying to install it onto a
generic PC).  Are you seeing cases where this is happening?  If people
are compiling their own kernel I'd argue that telling them to set
"CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT" to "no" is much better
than trying to jam a command line option on.  Command line options
don't scale well.

3. Any chance you could make it more obvious that this change is
undesirable and a last resort?  AKA:

"Stream ID x blocked for security reasons; allow anyway by booting
with arm-smmu.disable_bypass=0"

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/arm-smmu: Allow disabling bypass via kernel config

[Doug Anderson]

Hi,

On Fri, Feb 15, 2019 at 2:37 PM Rob Clark  wrote:
>
> On Thu, Feb 14, 2019 at 7:40 PM Doug Anderson  wrote:
> >
> > Hi,
> >
> > On Thu, Feb 14, 2019 at 1:32 PM Robin Murphy  wrote:
> > >
> > > Hi Doug,
> > >
> > > On 2019-02-14 8:44 pm, Douglas Anderson wrote:
> > > > Right now the only way to disable the iommu bypass for the ARM SMMU is
> > > > with the kernel command line parameter 'arm-smmu.disable_bypass'.
> > > >
> > > > In general kernel command line parameters make sense for things that
> > > > someone would like to tweak without rebuilding the kernel or for very
> > > > basic communication between the bootloader and the kernel, but are
> > > > awkward for other things.  Specifically:
> > > > * Human parsing of the kernel command line can be difficult since it's
> > > >just a big runon space separated line of text.
> > > > * If every bit of the system was configured via the kernel command
> > > >line the kernel command line would get very large and even more
> > > >unwieldly.
> > > > * Typically there are not easy ways in build systems to adjust the
> > > >kernel command line for config-like options.
> > > >
> > > > Let's introduce a new config option that allows us to disable the
> > > > iommu bypass without affecting the existing default nor the existing
> > > > ability to adjust the configuration via kernel command line.
> > >
> > > I say let's just flip the default - for a while now it's been one of
> > > those "oh yeah, we should probably do that" things that gets instantly
> > > forgotten again, so some 3rd-party demand is plenty to convince me :)
> > >
> > > There are few reasons to allow unmatched stream bypass, and even fewer
> > > good ones, so I'd be happy to shift the command-line burden over to the
> > > esoteric cases at this point, and consider the config option in future
> > > if anyone from that camp pops up and screams hard enough.
> >
> > Sure, I can submit that patch if we want.  I presume I'll get lots of
> > screaming but I'm used to that.  ;-)
> >
> > ...specifically I found that when I turned on "disably bypass" on my
> > board (sdm845-cheza, which is not yet upstream) that a bunch of things
> > that used to work broke.  That's a good thing because all the things
> > that broke need to be fixed properly (by adding the IOMMUs) but
> > presumably my board is not special in relying on the old insecure
> > behavior.
>
> So one niggly bit, beyond the drivers that aren't but should be using
> iommu, is the case where bootloader lights up the display.  We
> actually still have a lot of work to do for this (in that clk and
> genpd drivers need to be aware of handover, in addition to just
> iommu)..  But I'd rather not make a hard problem harder just yet.
>
> (it gets worse, afaict so far on the windows-arm laptops since in that
> case uefi/edk is actually taking the iommu out of bypass and things go
> badly when arm-smmu disables clks after probe..)
>
> But I might recommend making the default a kconfig option for now, so
> in the distro kernel case, where display driver is a kernel module,
> you aren't making a hard problem harder.  For cases where bootloader
> isn't enabling display, things are much easier, and I think we could
> switch the default sooner.  But I fear for cases where bootloader is
> enabling display it is a much harder problem :-(

OK, so after thinking about this and playing with it a bit, here's
what I'm planning to do: I'm going to send out a v2 of my patch where
I basically just flip it to "default y".

Originally I was going to post a patch like Robin suggested that just
changed the default in the code without introducing a KConfig option.
Then I looked at all the things I needed to do on my own board to get
things working again once the IOMMU disallowed bypass and I just
couldn't bring myself to respond to everyone else I was about to break
"now figure out how to add a new kernel command line option until you
fix this more correctly".

In my mind a change that by default breaks all these insecure people
(effectively notifying them that they were insecure) but that allows
them to get back to the old state quickly seems like a good first
step.  In my commit message I'll mention that in a kernel version or
two we should probably fully take out the KConfig option since people
will have (presumably) had a chance to wean themselves off it.

One last thought on all of this is the question about the whole
"device tree as a stabl

Re: [PATCH] iommu/arm-smmu: Allow disabling bypass via kernel config

[Doug Anderson]

Hi,

On Thu, Feb 14, 2019 at 1:32 PM Robin Murphy  wrote:
>
> Hi Doug,
>
> On 2019-02-14 8:44 pm, Douglas Anderson wrote:
> > Right now the only way to disable the iommu bypass for the ARM SMMU is
> > with the kernel command line parameter 'arm-smmu.disable_bypass'.
> >
> > In general kernel command line parameters make sense for things that
> > someone would like to tweak without rebuilding the kernel or for very
> > basic communication between the bootloader and the kernel, but are
> > awkward for other things.  Specifically:
> > * Human parsing of the kernel command line can be difficult since it's
> >just a big runon space separated line of text.
> > * If every bit of the system was configured via the kernel command
> >line the kernel command line would get very large and even more
> >unwieldly.
> > * Typically there are not easy ways in build systems to adjust the
> >kernel command line for config-like options.
> >
> > Let's introduce a new config option that allows us to disable the
> > iommu bypass without affecting the existing default nor the existing
> > ability to adjust the configuration via kernel command line.
>
> I say let's just flip the default - for a while now it's been one of
> those "oh yeah, we should probably do that" things that gets instantly
> forgotten again, so some 3rd-party demand is plenty to convince me :)
>
> There are few reasons to allow unmatched stream bypass, and even fewer
> good ones, so I'd be happy to shift the command-line burden over to the
> esoteric cases at this point, and consider the config option in future
> if anyone from that camp pops up and screams hard enough.

Sure, I can submit that patch if we want.  I presume I'll get lots of
screaming but I'm used to that.  ;-)

...specifically I found that when I turned on "disably bypass" on my
board (sdm845-cheza, which is not yet upstream) that a bunch of things
that used to work broke.  That's a good thing because all the things
that broke need to be fixed properly (by adding the IOMMUs) but
presumably my board is not special in relying on the old insecure
behavior.

I'm about to head on vacation for a week so I'm not sure I'll get to
re-post before then.  If not I'll post this sometime after I get back
unless someone beats me to it.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/3] dts: arm64/sdm845: Add node for arm,mmu-500

[Doug Anderson via iommu]

Hi,

On Thu, Jul 19, 2018 at 10:53 AM, Vivek Gautam
 wrote:
> Add device node for arm,mmu-500 available on sdm845.
> This MMU-500 with single TCU and multiple TBU architecture
> is shared among all the peripherals except gpu on sdm845.
>
> Signed-off-by: Vivek Gautam 
> ---
>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts |  4 ++
>  arch/arm64/boot/dts/qcom/sdm845.dtsi| 73 
> +
>  2 files changed, 77 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts 
> b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index 6d651f314193..13b50dff440f 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> @@ -58,3 +58,7 @@
> bias-pull-up;
> };
>  };
> +
> +_smmu {
> +   status = "okay";
> +};

When you spin this patch please put the above in the correct place.
Since "a" sorts alphabetically before "i" then this should be just
before the line:

 {

Thanks!

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/8] iommu/rockchip: Fix allocation of bases array in driver probe

[Doug Anderson]

Hi,

On Mon, Jun 20, 2016 at 9:34 PM, Tomasz Figa  wrote:
> From: Shunqian Zheng 
>
> In .probe(), devm_kzalloc() is called with size == 0 and works only
> by luck, due to internal behavior of the allocator and the fact
> that the proper allocation size is small. Let's use proper value for
> calculating the size.
>
> Fixes: cd6438c5f844 ("iommu/rockchip: Reconstruct to support multi slaves")
>
> Signed-off-by: Shunqian Zheng 
> Signed-off-by: Tomasz Figa 
> ---
>  drivers/iommu/rockchip-iommu.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Douglas Anderson 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/2] dma/iommu: Add pgsize_bitmap confirmation in __iommu_dma_alloc_pages

[Doug Anderson]

Hi,

On Fri, Apr 8, 2016 at 10:30 AM, Will Deacon  wrote:
>> > Am I barking up the wrong tree?
>>
>> I don't think min_order can be negative.  Certainly we could enter the
>> loop with order == 0 and min_order == 0, though.
>
> ... and in that case, PageCompound will be false, and we'll call split_page
> which won't do anything, so we break out.
>
>>
>> Some examples:
>>
>> order = 0, min_order = 0
>> -> Want alloc_pages _without_ __GFP_NORETRY.  OK
>> -> If alloc_pages fails, return NULL.  OK
>> -> If alloc pages succeeds, don't need splitting since single page.  OK
>
> [...]
>
>> I think those are all right.  Did I mess up?  You could certainly
>> structure the loop in a different way but you need to make sure you
>> handle all of those cases.  If you have an alternate structure that
>> handles all those, let's consider it.
>
> Right, I don't think the code is broken, I just think the !order check is
> confusing and not needed.

Ah ha!  Got it.  I didn't dig into split_page() to see that it was a
no-op when "order == 0".  I just know that the old code didn't call
split_page() with order == 0 so I assumed that was wise to keep.  If
we don't need to keep that then agreed that the "if" test can simply
be removed.  :)

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/2] dma/iommu: Add pgsize_bitmap confirmation in __iommu_dma_alloc_pages

[Doug Anderson]

Will,

On Fri, Apr 8, 2016 at 6:07 AM, Will Deacon <will.dea...@arm.com> wrote:
> On Tue, Apr 05, 2016 at 10:03:32AM -0700, Doug Anderson wrote:
>> On Tue, Mar 29, 2016 at 10:02 AM, Will Deacon <will.dea...@arm.com> wrote:
>> > On Mon, Mar 28, 2016 at 02:32:11PM +0800, Yong Wu wrote:
>> >> @@ -213,13 +215,16 @@ static struct page 
>> >> **__iommu_dma_alloc_pages(unsigned int count, gfp_t gfp)
>> >>   /*
>> >>* Higher-order allocations are a convenience rather
>> >>* than a necessity, hence using __GFP_NORETRY until
>> >> -  * falling back to single-page allocations.
>> >> +  * falling back to min size allocations.
>> >>*/
>> >> - for (order = min_t(unsigned int, order, __fls(count));
>> >> -  order > 0; order--) {
>> >> - page = alloc_pages(gfp | __GFP_NORETRY, order);
>> >> + for (order = min_t(int, order, __fls(count));
>> >> +  order >= min_order; order--) {
>> >> + page = alloc_pages((order == min_order) ? gfp :
>> >> +gfp | __GFP_NORETRY, order);
>> >>   if (!page)
>> >>   continue;
>> >> + if (!order)
>> >> + break;
>> >
>> > Isn't this handled by the loop condition?
>>
>> He changed the loop condition to be ">= min_order" instead of "> 0",
>> so now we can get here with an order == 0.  This makes sense because
>> when min_order is not 0 you still want to run the code to split the
>> pages and it is sane not to duplicate that below.
>>
>> Maybe I'm misunderstanding, though.  Perhaps you can explain how you
>> think this code should look?
>
> My reading of the code was that we require order >= min_order to enter
> the loop. Given that order doesn't change between the loop header and the
> if (!order) check, then that must mean we can enter the loop body with
> order == 0 and order >= min_order, which means that min_order is allowed
> to be negative. That feels weird.
>
> Am I barking up the wrong tree?

I don't think min_order can be negative.  Certainly we could enter the
loop with order == 0 and min_order == 0, though.


Some examples:

order = 0, min_order = 0
-> Want alloc_pages _without_ __GFP_NORETRY.  OK
-> If alloc_pages fails, return NULL.  OK
-> If alloc pages succeeds, don't need splitting since single page.  OK

order = 1, min_order = 1
-> Want alloc_pages _without_ __GFP_NORETRY.  OK
-> If alloc_pages fails, return NULL.  OK
-> If alloc pages succeeds, DO need splitting.  OK

order = 1, min_order = 0
-> Want alloc_pages with __GFP_NORETRY.  OK
-> If alloc_pages fails, try order = 0.  OK
-> If alloc pages succeeds, DO need splitting.  OK

order = 2, min_order = 1
-> Want alloc_pages with __GFP_NORETRY.  OK
-> If alloc_pages fails, try order = 1.  OK
-> If alloc pages succeeds, DO need splitting.  OK


I think those are all right.  Did I mess up?  You could certainly
structure the loop in a different way but you need to make sure you
handle all of those cases.  If you have an alternate structure that
handles all those, let's consider it.

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/2] dma/iommu: Add pgsize_bitmap confirmation in __iommu_dma_alloc_pages

[Doug Anderson]

Will,

On Tue, Mar 29, 2016 at 10:02 AM, Will Deacon  wrote:
> On Mon, Mar 28, 2016 at 02:32:11PM +0800, Yong Wu wrote:
>> Currently __iommu_dma_alloc_pages assumes that all the IOMMU support
>> the granule of PAGE_SIZE. It call alloc_page to try allocating memory
>> in the last time. Fortunately the mininum pagesize in all the
>> current IOMMU is SZ_4K, so this works well.
>>
>> But there may be a case in which the mininum granule of IOMMU may be
>> larger than PAGE_SIZE, then it will abort as the IOMMU cann't map the
>> discontinuous memory within a granule. For example, the pgsize_bitmap
>> of the IOMMU only has SZ_16K while the PAGE_SIZE is SZ_4K, then we
>> have to prepare SZ_16K continuous memory at least for each a granule
>> iommu mapping.
>
> Did you find a driver/platform that actually needs this? I certainly
> think it's possible, but we don't necessarily need to fix it if it's
> not broken yet! Just adding a comment would be enough.

I think Yong Wu was adding this patch in response to your request from
v1 of .  Fixing this
seemed like a separate issue so I asked Yong Wu to make two patches
rather than sticking this fix in the patch as his original.  Hopefully
this makes sense.

> Either way, a couple of review comments inline.
>
>>
>> Signed-off-by: Yong Wu 
>>
>> ---
>> v2:
>>  -rebase on v4.6-rc1.
>>  -add a new patch([1/2] add pgsize_bitmap) here.
>>
>>  drivers/iommu/dma-iommu.c | 22 +-
>>  1 file changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 72d6182..75ce71e 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -190,11 +190,13 @@ static void __iommu_dma_free_pages(struct page 
>> **pages, int count)
>>   kvfree(pages);
>>  }
>>
>> -static struct page **__iommu_dma_alloc_pages(unsigned int count, gfp_t gfp)
>> +static struct page **__iommu_dma_alloc_pages(unsigned int count, gfp_t gfp,
>> +  unsigned long pgsize_bitmap)
>>  {
>>   struct page **pages;
>>   unsigned int i = 0, array_size = count * sizeof(*pages);
>> - unsigned int order = MAX_ORDER;
>> + int min_order = get_order(1 << __ffs(pgsize_bitmap));
>
> 1UL

Yong: presumably you will spin with this fix?


>> + int order = MAX_ORDER;
>>
>>   if (array_size <= PAGE_SIZE)
>>   pages = kzalloc(array_size, GFP_KERNEL);
>> @@ -213,13 +215,16 @@ static struct page **__iommu_dma_alloc_pages(unsigned 
>> int count, gfp_t gfp)
>>   /*
>>* Higher-order allocations are a convenience rather
>>* than a necessity, hence using __GFP_NORETRY until
>> -  * falling back to single-page allocations.
>> +  * falling back to min size allocations.
>>*/
>> - for (order = min_t(unsigned int, order, __fls(count));
>> -  order > 0; order--) {
>> - page = alloc_pages(gfp | __GFP_NORETRY, order);
>> + for (order = min_t(int, order, __fls(count));
>> +  order >= min_order; order--) {
>> + page = alloc_pages((order == min_order) ? gfp :
>> +gfp | __GFP_NORETRY, order);
>>   if (!page)
>>   continue;
>> + if (!order)
>> + break;
>
> Isn't this handled by the loop condition?

He changed the loop condition to be ">= min_order" instead of "> 0",
so now we can get here with an order == 0.  This makes sense because
when min_order is not 0 you still want to run the code to split the
pages and it is sane not to duplicate that below.

Maybe I'm misunderstanding, though.  Perhaps you can explain how you
think this code should look?

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] arm64/dma-mapping: Add DMA_ATTR_ALLOC_SINGLE_PAGES support

[Doug Anderson]

Hi,

On Thu, Mar 24, 2016 at 4:50 AM, Will Deacon  wrote:
>> > I have a slight snag with this, in that you don't consult the IOMMU
>> > pgsize_bitmap at any point, and assume that it can map pages at the
>> > same granularity as the CPU. The documentation for
>> > DMA_ATTR_ALLOC_SINGLE_PAGES seems to be weaker than that.
>>
>> Interesting.  Is that something that exists in the real world?  ...or
>> something you think is coming soon?
>
> All it would take is for an IOMMU driver to choose a granule size that
> differs from the CPU. For example, if the SMMU driver chose 64k pages
> and the CPU was using 4k pages, then you'd have this problem.
>
>> I'd argue that such a case existed in the real world then probably
>> we're already broken.  Unless I'm misreading, existing code will
>> already fall all the way back to order 0 allocations.  ...so while
>> existing code might could work if it was called on a totally
>> unfragmented system, it would already break once some fragmentation
>> was introduced.
>
> I disagree. For example, in the case I described previously, they may
> well settle on a common supported granule (e.g. 2M), assuming that
> contiguous pages were implemented in the page table code.

I'm still a little confused how existing code could have worked if
IOMMU has 64K pages and CPU has 4K pages if memory is fragmented.
Presumably existing code in __iommu_dma_alloc_pages() would keep
failing the "alloc_pages(gfp | __GFP_NORETRY, order);" call until
order got down to 0.  Then we'd allocate order 0 (4K) pages and we'd
hit a bug.


>> I'm not saying that we shouldn't fix the code to handle this, I'm just
>> saying that Yong Wu's patch doesn't appear to break any code that
>> wasn't already broken.  That might be reason to land his code first,
>> then debate the finer points of whether IOMMUs with less granularity
>> are likely to exist and whether we need to add complexity to the code
>> to handle them (or just detect this case and return an error).
>>
>> From looking at a WIP patch provided to me by Yong Wu, it looks as if
>> he thinks several more functions need to change to handle this need
>> for IOMMUs that can't handle small pages.  That seems to be further
>> evidence that the work should be done in a separate patch.
>
> Sure, my observations weren't intended to hold up this patch, but we
> should double-check that we're no regressing any of the existing IOMMU
> drivers/platforms by going straight to order 0 allocations.

Argh.  I see why I was confused and thought it got complicated.  When
I looked at diffs I thought Yong Wu's patch was more complicated
because he rebased it and my diff showed some other unrelated patches.
Dumb.

OK, you're right that this is pretty simple.

In any case, you're right that we should fix this.  ...though assuming
my argument above isn't wrong, then existing code is already broken or
has a latent bug if you've got an IOMMU that can't map at least as
granular as the CPU.  To me that means adding that new feature (or
fixing that latent bug) should be done in a separate patch.  It could
come in sequence either before or after this one.  Of course, if
everyone else thinks it should be one patch I won't block that...

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] arm64/dma-mapping: Add DMA_ATTR_ALLOC_SINGLE_PAGES support

[Doug Anderson]

Will,

On Mon, Mar 21, 2016 at 11:01 AM, Will Deacon  wrote:
> On Thu, Mar 03, 2016 at 02:54:26AM +0800, Yong Wu wrote:
>> Sometimes it is not worth for the iommu allocating big chunks.
>> Here we enable DMA_ATTR_ALLOC_SINGLE_PAGES which could help avoid to
>> allocate big chunks while iommu allocating buffer.
>>
>> More information about this attribute, please check Doug's commit[1].
>>
>> [1]: https://lkml.org/lkml/2016/1/11/720
>>
>> Cc: Robin Murphy 
>> Suggested-by: Douglas Anderson 
>> Signed-off-by: Yong Wu 
>> ---
>>
>> Our video drivers may soon use this.
>>
>>  arch/arm64/mm/dma-mapping.c |  4 ++--
>>  drivers/iommu/dma-iommu.c   | 14 ++
>>  include/linux/dma-iommu.h   |  4 ++--
>>  3 files changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>> index 331c4ca..3225e3ca 100644
>> --- a/arch/arm64/mm/dma-mapping.c
>> +++ b/arch/arm64/mm/dma-mapping.c
>> @@ -562,8 +562,8 @@ static void *__iommu_alloc_attrs(struct device *dev, 
>> size_t size,
>>   struct page **pages;
>>   pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
>>
>> - pages = iommu_dma_alloc(dev, iosize, gfp, ioprot, handle,
>> - flush_page);
>> + pages = iommu_dma_alloc(dev, iosize, gfp, ioprot, attrs,
>> + handle, flush_page);
>>   if (!pages)
>>   return NULL;
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 72d6182..3569cb6 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -190,7 +190,8 @@ static void __iommu_dma_free_pages(struct page **pages, 
>> int count)
>>   kvfree(pages);
>>  }
>>
>> -static struct page **__iommu_dma_alloc_pages(unsigned int count, gfp_t gfp)
>> +static struct page **__iommu_dma_alloc_pages(unsigned int count, gfp_t gfp,
>> +  struct dma_attrs *attrs)
>>  {
>>   struct page **pages;
>>   unsigned int i = 0, array_size = count * sizeof(*pages);
>> @@ -203,6 +204,10 @@ static struct page **__iommu_dma_alloc_pages(unsigned 
>> int count, gfp_t gfp)
>>   if (!pages)
>>   return NULL;
>>
>> + /* Go straight to 4K chunks if caller says it's OK. */
>> + if (dma_get_attr(DMA_ATTR_ALLOC_SINGLE_PAGES, attrs))
>> + order = 0;
>
> I have a slight snag with this, in that you don't consult the IOMMU
> pgsize_bitmap at any point, and assume that it can map pages at the
> same granularity as the CPU. The documentation for
> DMA_ATTR_ALLOC_SINGLE_PAGES seems to be weaker than that.

Interesting.  Is that something that exists in the real world?  ...or
something you think is coming soon?

I'd argue that such a case existed in the real world then probably
we're already broken.  Unless I'm misreading, existing code will
already fall all the way back to order 0 allocations.  ...so while
existing code might could work if it was called on a totally
unfragmented system, it would already break once some fragmentation
was introduced.

I'm not saying that we shouldn't fix the code to handle this, I'm just
saying that Yong Wu's patch doesn't appear to break any code that
wasn't already broken.  That might be reason to land his code first,
then debate the finer points of whether IOMMUs with less granularity
are likely to exist and whether we need to add complexity to the code
to handle them (or just detect this case and return an error).

>From looking at a WIP patch provided to me by Yong Wu, it looks as if
he thinks several more functions need to change to handle this need
for IOMMUs that can't handle small pages.  That seems to be further
evidence that the work should be done in a separate patch.


Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] arm64/dma-mapping: Add DMA_ATTR_ALLOC_SINGLE_PAGES support

[Doug Anderson]

Hi,

On Wed, Mar 2, 2016 at 10:54 AM, Yong Wu  wrote:
> Sometimes it is not worth for the iommu allocating big chunks.
> Here we enable DMA_ATTR_ALLOC_SINGLE_PAGES which could help avoid to
> allocate big chunks while iommu allocating buffer.
>
> More information about this attribute, please check Doug's commit[1].
>
> [1]: https://lkml.org/lkml/2016/1/11/720
>
> Cc: Robin Murphy 
> Suggested-by: Douglas Anderson 
> Signed-off-by: Yong Wu 
> ---
>
> Our video drivers may soon use this.
>
>  arch/arm64/mm/dma-mapping.c |  4 ++--
>  drivers/iommu/dma-iommu.c   | 14 ++
>  include/linux/dma-iommu.h   |  4 ++--
>  3 files changed, 14 insertions(+), 8 deletions(-)

It should also be mentioned that this depends on commit df05c6f6e0bb
("ARM: 8506/1: common: DMA-mapping: add DMA_ATTR_ALLOC_SINGLE_PAGES
attribute") which is not in mainline quite yet.

Also please CC Marek Szyprowski on any future patches in this area
since I saw a patch series from him that was touching this same area.

Reviewed-by: Douglas Anderson 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/3] iommu/dma: Avoid unlikely high-order allocations

[Doug Anderson]

Robin,

On Fri, Dec 18, 2015 at 9:01 AM, Robin Murphy  wrote:
> Doug reports that the equivalent page allocator on 32-bit ARM exhibits
> particularly pathalogical behaviour under memory pressure when
> fragmentation is high, where allocating a 4MB buffer takes tens of
> seconds and the number of calls to alloc_pages() is over 9000![1]
>
> We can drastically improve that situation without losing the other
> benefits of high-order allocations when they would succeed, by assuming
> memory pressure is relatively constant over the course of an allocation,
> and not retrying allocations at orders we know to have failed before.
> This way, the best-case behaviour remains unchanged, and in the worst
> case we should see at most a dozen or so (MAX_ORDER - 1) failed attempts
> before falling back to single pages for the remainder of the buffer.
>
> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/394660.html
>
> Reported-by: Douglas Anderson 
> Signed-off-by: Robin Murphy 
> ---
>  drivers/iommu/dma-iommu.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

Presumably it makes sense to update this based on v2 of my patch to
the original code?  AKA: 
?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v12 30/31] ARM: dts: add System MMU nodes of exynos5250

[Doug Anderson]

Vikas,


On Sun, Apr 27, 2014 at 10:39 AM, Vikas Sajjan sajjan.li...@gmail.com wrote:
 Hi shaik,

 +Doug, Abhilash,

 On Sun, Apr 27, 2014 at 1:08 PM, Shaik Ameer Basha
 shaik.am...@samsung.com wrote:
 From: Cho KyongHo pullip@samsung.com

 Signed-off-by: Cho KyongHo pullip@samsung.com
 ---
  arch/arm/boot/dts/exynos5250.dtsi |  270 
 -
  1 file changed, 267 insertions(+), 3 deletions(-)

 diff --git a/arch/arm/boot/dts/exynos5250.dtsi 
 b/arch/arm/boot/dts/exynos5250.dtsi
 index 3742331..eebd397 100644
 --- a/arch/arm/boot/dts/exynos5250.dtsi
 +++ b/arch/arm/boot/dts/exynos5250.dtsi
 @@ -82,6 +82,16 @@
 reg = 0x10044040 0x20;
 };

 +   pd_isp: isp-power-domain@0x10044020 {
 +   compatible = samsung,exynos4210-pd;
 +   reg = 0x10044020 0x20;
 +   };
 +
 +   pd_disp1: disp1-power-domain@0x100440A0 {
 +   compatible = samsung,exynos4210-pd;
 +   reg = 0x100440A0 0x20;
 +   };
 +

 As per subject add System MMU nodes of exynos5250, it should only
 add SysMMU node.
 So, I think adding power domain nodes should go in a separate patch.

 Adding power domain nodes can break the system, if powering ON/OFF of
 the given power domain is NOT taken care well.
 I can see ISP is one such case. With this series I can see S2R breaks
 [1] on 5250 chromebook with current mainline kernel (same applies for
 arndale and smdk5250, but I have tested on these boards yet)

Thanks for catching that!  Let's make sure not to break suspend/resume.

Given that these power domains are actually used as the domains for
some of the System MMU nodes I don't totally object to adding them in
the same patch, but if they're breaking things then I agree that we
could leave them out and add them in a later patch (once issues are
sorted out).

-Doug
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu