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

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

[Douglas Anderson]

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. This patch series doesn't save the world but
hopefully at least moves us in the right direction while accomplishing
something useful. Specifically:
- No attempt is made to touch the PCI subsystem or cleanup the way
  that it requests strict vs. non-strict.
- No fully generic mechanism is come up with that makes it super easy
  for everyone to be in non-strict mode.

This patch conflicts with a few other patch series that are in
flight. I've tried to call them out "after the cut" in patches. I
assume other in flight patches will land before this one, so I'd
expect to send a rebased version when that happens, assuming that this
series isn't NAKed into the ground.

Changes in v2:
- No longer based on changes adding strictness to "struct device"
- Updated kernel-parameters docs.
- Patch moving check for strictness in arm-smmu new for v2.
- Now accomplish the goal by putting rules in the IOMMU driver.
- Reworded commit message to clarify things pointed out by Greg.

Douglas Anderson (3):
  iommu: Add per-domain strictness and combine with the global default
  iommu/arm-smmu: Check for strictness after calling
    impl->init_context()
  mmc: sdhci-msm: Request non-strict IOMMU mode

 .../admin-guide/kernel-parameters.txt         |  5 ++-
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c    | 19 ++++++++
 drivers/iommu/arm/arm-smmu/arm-smmu.c         |  6 +--
 drivers/iommu/iommu.c                         | 43 +++++++++++++++----
 include/linux/iommu.h                         |  7 +++
 5 files changed, 67 insertions(+), 13 deletions(-)

-- 
2.32.0.93.g670b81a890-goog

[PATCH v2 1/3] iommu: Add per-domain strictness and combine with the global default

[Douglas Anderson]

Strictness has the semantic of being a per-domain property. This is
why iommu_get_dma_strict() takes a "struct iommu_domain" as a
parameter. Let's add knowledge to the "struct iommu_domain" so we can
know whether we'd like each domain to be strict.

In this patch nothing sets the per-domain strictness, it just paves
the way for future patches to do so.

Prior to this patch we could only affect strictness at a global
level. We'll still honor the global strictness level if it has been
explicitly set and it's stricter than the one requested per-domain.

NOTE: it's even more obvious that iommu_set_dma_strict() and
iommu_get_dma_strict() are non-symmetric after this change. However,
they have always been asymmetric by design [0].

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

[0] https://lore.kernel.org/r/a023af85-5060-0a3c-4648-b00f8b8c0430@arm.com/

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This patch clearly will cause conflicts if John Garry's patches [1]
land before it. It shouldn't be too hard to rebase,
though. Essentially with John's patches it'll be impossible for what's
called `cmdline_dma_strict` in my patch to be "default". It'll
probably make sense to rearrange the logic/names a bit though just to
make things clearer.

[1] https://lore.kernel.org/r/1624016058-189713-1-git-send-email-john.garry@huawei.com/

Changes in v2:
- No longer based on changes adding strictness to "struct device"
- Updated kernel-parameters docs.

 .../admin-guide/kernel-parameters.txt         |  5 ++-
 drivers/iommu/iommu.c                         | 43 +++++++++++++++----
 include/linux/iommu.h                         |  7 +++
 3 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index cb89dbdedc46..7675fd79f9a9 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1995,9 +1995,12 @@
 			  throughput at the cost of reduced device isolation.
 			  Will fall back to strict mode if not supported by
 			  the relevant IOMMU driver.
-			1 - Strict mode (default).
+			1 - Strict mode.
 			  DMA unmap operations invalidate IOMMU hardware TLBs
 			  synchronously.
+			NOTE: if "iommu.strict" is not specified in the command
+			line then it's up to the system to try to determine the
+			proper strictness.
 
 	iommu.passthrough=
 			[ARM64, X86] Configure DMA to bypass the IOMMU by default.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 808ab70d5df5..7943d2105b2f 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -29,7 +29,8 @@ static struct kset *iommu_group_kset;
 static DEFINE_IDA(iommu_group_ida);
 
 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;
+static enum iommu_strictness driver_dma_strict __read_mostly;
 static u32 iommu_cmd_line __read_mostly;
 
 struct iommu_group {
@@ -69,7 +70,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);
@@ -334,27 +334,52 @@ static int __init iommu_set_def_domain_type(char *str)
 }
 early_param("iommu.passthrough", iommu_set_def_domain_type);
 
+static inline enum iommu_strictness bool_to_strictness(bool strict)
+{
+	return strict ? IOMMU_STRICT : IOMMU_NOT_STRICT;
+}
+
 static int __init iommu_dma_setup(char *str)
 {
-	int ret = kstrtobool(str, &iommu_dma_strict);
+	bool strict;
+	int ret = kstrtobool(str, &strict);
 
 	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;
+	/*
+	 * Valid transitions:
+	 * - DEFAULT -> NON_STRICT
+	 * - DEFAULT -> STRICT
+	 * - NON_STRICT -> STRICT
+	 *
+	 * Everything else is ignored.
+	 */
+	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->strictness == IOMMU_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->strictness == IOMMU_NOT_STRICT)
+		return false;
+
+	/* Nobody said anything, so it's strict by default */
 	return true;
 }
 EXPORT_SYMBOL_GPL(iommu_get_dma_strict);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 32d448050bf7..2e172059c931 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -79,8 +79,15 @@ struct iommu_domain_geometry {
 #define IOMMU_DOMAIN_DMA	(__IOMMU_DOMAIN_PAGING |	\
 				 __IOMMU_DOMAIN_DMA_API)
 
+enum iommu_strictness {
+	IOMMU_DEFAULT_STRICTNESS = 0,	/* zero-init ends up at default */
+	IOMMU_NOT_STRICT,
+	IOMMU_STRICT,
+};
+
 struct iommu_domain {
 	unsigned type;
+	enum iommu_strictness strictness;
 	const struct iommu_ops *ops;
 	unsigned long pgsize_bitmap;	/* Bitmap of page sizes in use */
 	iommu_fault_handler_t handler;
-- 
2.32.0.93.g670b81a890-goog

[PATCH v2 2/3] iommu/arm-smmu: Check for strictness after calling impl->init_context()

[Douglas Anderson]

Implementations should be able to affect the strictness so reorder a
little bit so we call them before we look at the strictness.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Patch moving check for strictness in arm-smmu new for v2.

 drivers/iommu/arm/arm-smmu/arm-smmu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 6f72c4d208ca..659d3fddffa5 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -761,15 +761,15 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 		.iommu_dev	= smmu->dev,
 	};
 
-	if (!iommu_get_dma_strict(domain))
-		pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
-
 	if (smmu->impl && smmu->impl->init_context) {
 		ret = smmu->impl->init_context(smmu_domain, &pgtbl_cfg, dev);
 		if (ret)
 			goto out_clear_smmu;
 	}
 
+	if (!iommu_get_dma_strict(domain))
+		pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
+
 	if (smmu_domain->pgtbl_quirks)
 		pgtbl_cfg.quirks |= smmu_domain->pgtbl_quirks;
 
-- 
2.32.0.93.g670b81a890-goog

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

[Douglas Anderson]

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 numbers. I used
"dd" to measure read speeds from eMMC on a sc7180-trogdor-lazor
board. Basic test command (while booted from USB):
  echo 3 > /proc/sys/vm/drop_caches
  dd if=/dev/mmcblk1 of=/dev/null bs=4M count=512

I attempted to run my tests for enough iterations that results
stabilized and weren't too noisy. Tests were run with patches picked
to the chromeos-5.4 tree (sanity checked against v5.13-rc7). I also
attempted to compare to other attempts to address IOMMU problems
and/or attempts to bump the cpufreq up to solve this problem:
- eMMC datasheet spec: 300 MB/s "Typical Sequential Performance"
  NOTE: we're driving the bus at 192 MHz instead of 200 Mhz so we might
  not be able to achieve the full 300 MB/s.
- Baseline: 210.9 MB/s
- Baseline + peg cpufreq to max: 284.3 MB/s
- This patch: 279.6 MB/s
- This patch + peg cpufreq to max: 288.1 MB/s
- Joel's IO Wait fix [1]: 258.4 MB/s
- Joel's IO Wait fix [1] + peg cpufreq to max: 287.8 MB/s
- TLBIVA patches [2] + [3]: 214.7 MB/s
- TLBIVA patches [2] + [3] + peg cpufreq to max: 285.7 MB/s
- This patch plus Joel's [1]: 280.2 MB/s
- This patch plus Joel's [1] + peg...: 279.0 MB/s
  NOTE: I suspect something in the system was thermal throttling since
  there's a heat wave right now.

I also spent a little bit of time trying to see if I could get the
IOMMU flush for MMC out of the critical path but was unable to figure
out how to do this and get good performance.

Overall I'd say that the performance results above show:
* It's really not straightforward to point at "one thing" that is
  making our eMMC performance bad.
* It's certainly possible to get pretty good eMMC performance even
  without this patch.
* This patch makes it much easier to get good eMMC performance.
* No other solutions that I found resulted in quite as good eMMC
  performance as having this patch.

Given all the above (security safety concerns are minimal and it's a
nice performance win), I'm proposing that running SDHCI on Qualcomm
SoCs in non-strict mode is the right thing to do until such point in
time as someone can come up with a better solution to get good SD/eMMC
performance without it.

Now that we've decided we want the SD/MMC controller in non-strict
mode, we need to figure out how to make it happen. We will take
advantage of the fact that on Qualcomm IOMMUs we know that SD/MMC
controllers are in a domain by themselves and hook in when initting
the domain context. In response to a previous version of this series
there had been discussion [4] of having this driven from a device tree
property and this solution doesn't preclude that but is a good jumping
off point.

NOTES:
* It's likely that arguments similar to the above can be made for
  other SDHCI controllers. However, given that this is something that
  can have an impact on security it feels like we want each SDHCI
  controller to opt-in. I believe it is conceivable, for instance,
  that some SDHCI controllers might have loadable or updatable
  firmware.
* It's also likely other peripherals will want this to get the quick
  performance win. That also should be fine, though anyone landing a
  similar patch should be very careful that it is low risk for all
  users of a given peripheral.
* Conceivably if even this patch is considered too "high risk", we
  could limit this to just non-removable cards (like eMMC) by just
  checking the device tree. This is one nice advantage of using the
  pre_probe() to set this.

[1] https://lore.kernel.org/r/20210618040639.3113489-1-joel@joelfernandes.org
[2] https://lore.kernel.org/r/1623850736-389584-1-git-send-email-quic_c_gdjako@quicinc.com/
[3] https://lore.kernel.org/r/cover.1623981933.git.saiprakash.ranjan@codeaurora.org/
[4] https://lore.kernel.org/r/20210621235248.2521620-1-dianders@chromium.org

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Now accomplish the goal by putting rules in the IOMMU driver.
- Reworded commit message to clarify things pointed out by Greg.

 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index 98b3a1c2a181..bd66376d21ce 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -172,6 +172,24 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
 	{ }
 };
 
+static const struct of_device_id qcom_smmu_nonstrict_of_match[] __maybe_unused = {
+	{ .compatible = "qcom,sdhci-msm-v4" },
+	{ .compatible = "qcom,sdhci-msm-v5" },
+	{ }
+};
+
+static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
+		struct io_pgtable_cfg *pgtbl_cfg, struct device *dev)
+{
+	const struct of_device_id *match =
+		of_match_device(qcom_smmu_nonstrict_of_match, dev);
+
+	if (match)
+		smmu_domain->domain.strictness = IOMMU_NOT_STRICT;
+
+	return 0;
+}
+
 static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
 {
 	unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
@@ -295,6 +313,7 @@ static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
 }
 
 static const struct arm_smmu_impl qcom_smmu_impl = {
+	.init_context = qcom_smmu_init_context,
 	.cfg_probe = qcom_smmu_cfg_probe,
 	.def_domain_type = qcom_smmu_def_domain_type,
 	.reset = qcom_smmu500_reset,
-- 
2.32.0.93.g670b81a890-goog

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 <dianders@chromium.org> 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 numbers. I used
> "dd" to measure read speeds from eMMC on a sc7180-trogdor-lazor
> board. Basic test command (while booted from USB):
>   echo 3 > /proc/sys/vm/drop_caches
>   dd if=/dev/mmcblk1 of=/dev/null bs=4M count=512
>
> I attempted to run my tests for enough iterations that results
> stabilized and weren't too noisy. Tests were run with patches picked
> to the chromeos-5.4 tree (sanity checked against v5.13-rc7). I also
> attempted to compare to other attempts to address IOMMU problems
> and/or attempts to bump the cpufreq up to solve this problem:
> - eMMC datasheet spec: 300 MB/s "Typical Sequential Performance"
>   NOTE: we're driving the bus at 192 MHz instead of 200 Mhz so we might
>   not be able to achieve the full 300 MB/s.
> - Baseline: 210.9 MB/s
> - Baseline + peg cpufreq to max: 284.3 MB/s
> - This patch: 279.6 MB/s
> - This patch + peg cpufreq to max: 288.1 MB/s
> - Joel's IO Wait fix [1]: 258.4 MB/s
> - Joel's IO Wait fix [1] + peg cpufreq to max: 287.8 MB/s
> - TLBIVA patches [2] + [3]: 214.7 MB/s
> - TLBIVA patches [2] + [3] + peg cpufreq to max: 285.7 MB/s
> - This patch plus Joel's [1]: 280.2 MB/s
> - This patch plus Joel's [1] + peg...: 279.0 MB/s
>   NOTE: I suspect something in the system was thermal throttling since
>   there's a heat wave right now.
>
> I also spent a little bit of time trying to see if I could get the
> IOMMU flush for MMC out of the critical path but was unable to figure
> out how to do this and get good performance.
>
> Overall I'd say that the performance results above show:
> * It's really not straightforward to point at "one thing" that is
>   making our eMMC performance bad.
> * It's certainly possible to get pretty good eMMC performance even
>   without this patch.
> * This patch makes it much easier to get good eMMC performance.
> * No other solutions that I found resulted in quite as good eMMC
>   performance as having this patch.
>
> Given all the above (security safety concerns are minimal and it's a
> nice performance win), I'm proposing that running SDHCI on Qualcomm
> SoCs in non-strict mode is the right thing to do until such point in
> time as someone can come up with a better solution to get good SD/eMMC
> performance without it.
>
> Now that we've decided we want the SD/MMC controller in non-strict
> mode, we need to figure out how to make it happen. We will take
> advantage of the fact that on Qualcomm IOMMUs we know that SD/MMC
> controllers are in a domain by themselves and hook in when initting
> the domain context. In response to a previous version of this series
> there had been discussion [4] of having this driven from a device tree
> property and this solution doesn't preclude that but is a good jumping
> off point.
>
> NOTES:
> * It's likely that arguments similar to the above can be made for
>   other SDHCI controllers. However, given that this is something that
>   can have an impact on security it feels like we want each SDHCI
>   controller to opt-in. I believe it is conceivable, for instance,
>   that some SDHCI controllers might have loadable or updatable
>   firmware.
> * It's also likely other peripherals will want this to get the quick
>   performance win. That also should be fine, though anyone landing a
>   similar patch should be very careful that it is low risk for all
>   users of a given peripheral.
> * Conceivably if even this patch is considered too "high risk", we
>   could limit this to just non-removable cards (like eMMC) by just
>   checking the device tree. This is one nice advantage of using the
>   pre_probe() to set this.
>
> [1] https://lore.kernel.org/r/20210618040639.3113489-1-joel@joelfernandes.org
> [2] https://lore.kernel.org/r/1623850736-389584-1-git-send-email-quic_c_gdjako@quicinc.com/
> [3] https://lore.kernel.org/r/cover.1623981933.git.saiprakash.ranjan@codeaurora.org/
> [4] https://lore.kernel.org/r/20210621235248.2521620-1-dianders@chromium.org
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2:
> - Now accomplish the goal by putting rules in the IOMMU driver.
> - Reworded commit message to clarify things pointed out by Greg.
>
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Ugh, I really miffed up ${SUBJECT}. I thought I had fixed it to the
proper iommu prefix but clearly I didn't. :(


> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 98b3a1c2a181..bd66376d21ce 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -172,6 +172,24 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
>         { }
>  };
>
> +static const struct of_device_id qcom_smmu_nonstrict_of_match[] __maybe_unused = {
> +       { .compatible = "qcom,sdhci-msm-v4" },
> +       { .compatible = "qcom,sdhci-msm-v5" },
> +       { }
> +};

I will note that I've done more testing and I've also found that I get
a really nice USB performance win with a similar change there. I have
run as many variety of tests there, but atop the patches that are
already in the chromeos-5.4 tree (which includes Joel's io wait patch)
I saw an "dd" from a USB 3.0 SD card reader (with a fast card) go from
~74 MB/s to ~86 MB/s by adding "snps,dwc3".

I believe that the USB controller is a more complicated beast than the
SDHCI controller so perhaps it's harder to have quite as much
confidence in it, but it still does technically fulfil the
requirements above.

I'd also say that since it appears that both Intel and AMD IOMMU
drivers appear to default to non-strict for anything that's not an
external PCI device that we could probably default to non-secure for
dwc3 as well. I'll include a 4th patch in the next version of this
series assuming that the series as a whole isn't NAKed.

-Doug

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

[Joerg Roedel]

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

Regards,

	Joerg

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 <joro@8bytes.org> 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

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 <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Jun 25, 2021 at 6:19 AM Joerg Roedel <joro@8bytes.org> 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.

I guess this thread has been silent for a bit of time now. Given that
my previous version generated a whole bunch of traffic, I guess I'm
assuming this:

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.

c) Nobody has any other bright ideas for ways to solve this that would
make my patch series obsolete.

I guess I'll take that as a good sign and hope that it means that this
approach has a path forward. I suppose it could just be that everyone
is busy and/or on vacation, but I've always been an optimist!

My plan continues to be to send a v3 of my patch series atop Sai's
patch [1] and John's series [2]. I'll plan to wait a bit longer before
posting my v3 to allow for more feedback/thoughts and also to see if
either Sai's patches or John's patches land and/or have newer versions
posted. :-)

-Doug

[1] https://lore.kernel.org/r/20210623134201.16140-1-saiprakash.ranjan@codeaurora.org
[2] https://lore.kernel.org/linux-doc/1624016058-189713-1-git-send-email-john.garry@huawei.com

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

[Joerg Roedel]

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.

So the real question is still why strict is the default setting and how
to change that. Or document for the users that want performance how to
change the setting, so that they can decide.

Regards,

	Joerg

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 <joro@8bytes.org> 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

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

[Robin Murphy]

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

Cheers,
Robin.

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 <robin.murphy@arm.com> 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

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

[Rajat Jain]

Hi Robin, Doug,

On Wed, Jul 14, 2021 at 8:14 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Jul 13, 2021 at 11:07 AM Robin Murphy <robin.murphy@arm.com> 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

I was wondering if you got any closer to testing / sending it out? I
looked at the patches and am trying to understand, would they also
make it possible to convert at runtime, an existing "non-strict"
domain (for a particular device) into a "strict" domain leaving the
other devices/domains as-is? Please let me know when you think your
patches are good to be tested, and I'd also be interested in trying
them out.

>
> 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 still have another (inverse) use case where this does not work:
We have an Intel chromebook with the default domain type being
non-strict. There is an LTE modem (an internal PCI device which cannot
be marked external), which we'd like to be treated as a "Strict" DMA
domain.

Do I understand it right that using Rob's patches, I could potentially
switch the domain to "strict" *after* booting (since we don't use
initramfs), but by that time, the driver might have already attached
to the modem device (using "non-strict" domain), and thus the damage
may have already been done? So perhaps we still need a device property
that the firmware could use to indicate "strictness" for certain
devices at boot?

Thanks,
Rajat

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

[Rajat Jain]

Hi Rob,

On Mon, Aug 2, 2021 at 5:09 PM Rajat Jain <rajatja@google.com> wrote:
>
> Hi Robin, Doug,
>
> On Wed, Jul 14, 2021 at 8:14 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Tue, Jul 13, 2021 at 11:07 AM Robin Murphy <robin.murphy@arm.com> 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

BTW, is there another mirror to this? I (and another colleague) are
getting the following error when trying to clone it:

rajatja@rajat2:~/rob_iommu$ git clone
https://git.gitlab.arm.com/linux-arm/linux-rm.git
Cloning into 'linux-rm'...
remote: Enumerating objects: 125712, done.
remote: Counting objects: 100% (125712/125712), done.
remote: Compressing objects: 100% (41203/41203), done.
error: RPC failed; curl 18 transfer closed with outstanding read data remaining
error: 804 bytes of body are still expected
fetch-pack: unexpected disconnect while reading sideband packet fatal:
early EOF
fatal: fetch-pack: invalid index-pack output rajatja@rajat2:~/rob_iommu$

We've tried both git and https methods.

>
> I was wondering if you got any closer to testing / sending it out? I
> looked at the patches and am trying to understand, would they also
> make it possible to convert at runtime, an existing "non-strict"
> domain (for a particular device) into a "strict" domain leaving the
> other devices/domains as-is? Please let me know when you think your
> patches are good to be tested, and I'd also be interested in trying
> them out.
>
> >
> > 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 still have another (inverse) use case where this does not work:
> We have an Intel chromebook with the default domain type being
> non-strict. There is an LTE modem (an internal PCI device which cannot
> be marked external), which we'd like to be treated as a "Strict" DMA
> domain.
>
> Do I understand it right that using Rob's patches, I could potentially
> switch the domain to "strict" *after* booting (since we don't use
> initramfs), but by that time, the driver might have already attached
> to the modem device (using "non-strict" domain), and thus the damage
> may have already been done? So perhaps we still need a device property
> that the firmware could use to indicate "strictness" for certain
> devices at boot?
>
> Thanks,
> Rajat

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

[Robin Murphy]

On 2021-08-03 01:09, Rajat Jain wrote:
> Hi Robin, Doug,
> 
> On Wed, Jul 14, 2021 at 8:14 AM Doug Anderson <dianders@chromium.org> wrote:
>>
>> Hi,
>>
>> On Tue, Jul 13, 2021 at 11:07 AM Robin Murphy <robin.murphy@arm.com> 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
> 
> I was wondering if you got any closer to testing / sending it out? I
> looked at the patches and am trying to understand, would they also
> make it possible to convert at runtime, an existing "non-strict"
> domain (for a particular device) into a "strict" domain leaving the
> other devices/domains as-is? Please let me know when you think your
> patches are good to be tested, and I'd also be interested in trying
> them out.

Yup, most recently here:

https://lore.kernel.org/linux-iommu/cover.1627468308.git.robin.murphy@arm.com/

I'm currently getting v3 ready, so I'll try to remember to add you to 
the CC list.

>> 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 still have another (inverse) use case where this does not work:
> We have an Intel chromebook with the default domain type being
> non-strict. There is an LTE modem (an internal PCI device which cannot
> be marked external), which we'd like to be treated as a "Strict" DMA
> domain.
> 
> Do I understand it right that using Rob's patches, I could potentially
> switch the domain to "strict" *after* booting (since we don't use
> initramfs), but by that time, the driver might have already attached
> to the modem device (using "non-strict" domain), and thus the damage
> may have already been done? So perhaps we still need a device property
> that the firmware could use to indicate "strictness" for certain
> devices at boot?

Well, in my view the "external facing" firmware property *should* 
effectively be the "I don't trust this device not to misbehave" 
property, but I guess it's a bit too conflated with other aspects of 
Thunderbolt root ports (at least in the ACPI definition) to abuse in 
that manner.

Ideas off the top of my head would be to flip the default domain type 
and manually relax all the other performance-sensitive devices instead, 
or module_blacklist the modem driver to load manually later after 
tweaking its group. However, if you think it's a sufficiently general 
concern, maybe a quirk to set pci_dev->untrusted might be worth 
exploring? It may make sense to drive such a thing from a command-line 
option rather than a hard-coded list, though, since trust is really down 
to the individual use-case.

[ re gitlab.arm.com, I understand it tends not to like large transfers - 
some colleagues have reported similar issues pushing large repos as 
well. I'd suggest cloning the base mainline repo from kernel.org or 
another reliable source, then fetching my branch into that. I've just 
tried that on a different machine (outside the work network) and it 
worked fine) ]

Thanks,
Robin.

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

[Robin Murphy]

On 2021-07-08 09:08, 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.
> 
> So the real question is still why strict is the default setting and how
> to change that. Or document for the users that want performance how to
> change the setting, so that they can decide.

As I mentioned before, conceptually I think this very much belongs in 
sysfs as a user decision. We essentially have 4 levels of "strictness":

1: DMA domain with bounce pages
2: DMA domain
3: DMA domain with flush queue
4: Identity domain

The "make this device go faster because I trust it" use-case is why we 
have the sysfs interface to switch between 2 and 4, so it's entirely 
logical to have the intermediate option as well for when 3 is "faster" 
enough while still giving a bit more peace of mind than full-on bypass.

Making it a platform-specific decision that's hidden in a driver - 
arm-smmu-qcom can be considered a dumping ground of detailed platform 
knowledge ;) - happens to work as a reasonable compromise for this 
specific case, but I concur that it could be viewed as setting a 
precedent for other cases which we definitely aren't as reasonable.

I've been thinking about the sysfs thing some more, and since it's 
Friday afternoon and I can't concentrate on what I'm supposed to be 
doing anyway, let's see how far I can get by Monday...

Robin.

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

[Joerg Roedel]

Hi Robin,

On Fri, Jul 09, 2021 at 02:56:47PM +0100, Robin Murphy wrote:
> As I mentioned before, conceptually I think this very much belongs in sysfs
> as a user decision. We essentially have 4 levels of "strictness":
> 
> 1: DMA domain with bounce pages
> 2: DMA domain
> 3: DMA domain with flush queue
> 4: Identity domain

Together with reasonable defaults (influenced by compile-time
options) it seems to be a good thing to configure at runtime via
sysfs.

We already have CONFIG_IOMMU_DEFAULT_PASSTHROUGH, which can probably be
extended to be an option list:

	- CONFIG_IOMMU_DEFAULT_PASSTHROUGH: Trusted devices are identity
					    mapped

	- CONFIG_IOMMU_DEFAULT_DMA_STRICT: Trusted devices are DMA
					   mapped with strict flush
					   behavior on unmap

	- CONFIG_IOMMU_DEFAULT_DMA_LAZY: Trusted devices are DMA mapped
					 with flush queues for performance

Untrusted devices always get into the DMA domain with bounce pages by
default.

The defaults can be changed at runtime via sysfs. We already have basic
support for runtime switching of the default domain, so that can be
re-used.

Regards,

	Joerg

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

[Robin Murphy]

On 2021-07-14 11:15, Joerg Roedel wrote:
> Hi Robin,
> 
> On Fri, Jul 09, 2021 at 02:56:47PM +0100, Robin Murphy wrote:
>> As I mentioned before, conceptually I think this very much belongs in sysfs
>> as a user decision. We essentially have 4 levels of "strictness":
>>
>> 1: DMA domain with bounce pages
>> 2: DMA domain
>> 3: DMA domain with flush queue
>> 4: Identity domain
> 
> Together with reasonable defaults (influenced by compile-time
> options) it seems to be a good thing to configure at runtime via
> sysfs.
> 
> We already have CONFIG_IOMMU_DEFAULT_PASSTHROUGH, which can probably be
> extended to be an option list:
> 
> 	- CONFIG_IOMMU_DEFAULT_PASSTHROUGH: Trusted devices are identity
> 					    mapped
> 
> 	- CONFIG_IOMMU_DEFAULT_DMA_STRICT: Trusted devices are DMA
> 					   mapped with strict flush
> 					   behavior on unmap
> 
> 	- CONFIG_IOMMU_DEFAULT_DMA_LAZY: Trusted devices are DMA mapped
> 					 with flush queues for performance

Indeed, I got focused on the sysfs angle, but rearranging the Kconfig 
default that way to match makes a lot of sense, and is another thing 
which should fall out really easily from my domain type rework, so I'll 
add that to my branch now before I forget again.

> Untrusted devices always get into the DMA domain with bounce pages by
> default.
> 
> The defaults can be changed at runtime via sysfs. We already have basic
> support for runtime switching of the default domain, so that can be
> re-used.

As mentioned yesterday, already done! I'm hoping to be able to post the 
patches next week after some testing :)

Cheers,
Robin.

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

[Joerg Roedel]

On Wed, Jul 14, 2021 at 11:29:08AM +0100, Robin Murphy wrote:
> As mentioned yesterday, already done! I'm hoping to be able to post the
> patches next week after some testing :)

Great, looking forward to your patches :-)

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

[Robin Murphy]

On 2021-07-08 09:08, 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.
> 
> So the real question is still why strict is the default setting and how
> to change that.

It's occurred to me whilst hacking on the relevant area that there's an 
important point I may have somewhat glossed over there: most of the 
IOMMU drivers that are used for arm64 do not take advantage of 
non-strict mode anyway. If anything it would be detrimental, since 
iommu-dma would waste a bunch of time and memory managing flush queues 
and firing off the batch invalidations while internally the drivers are 
still invalidating each unmap synchronously.

Those IOMMUs in mobile and embedded SoCs are also mostly used for media 
devices, where the buffers are relatively large and change relatively 
infrequently, so they are less likely to gain significantly from 
supporting non-strict mode. It's primarily the Arm SMMUs which get used 
in the more "x86-like" paradigm (especially in larger systems) of being 
stuck in front of everything including networking/storage/PCIe/etc. 
where the workloads are far more varied.

Robin.

> Or document for the users that want performance how to
> change the setting, so that they can decide.
> 
> Regards,
> 
> 	Joerg
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>