linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v13 0/6] iommu: Enhance IOMMU default DMA mode build options
@ 2021-06-16 11:03 John Garry
  2021-06-16 11:03 ` [PATCH v13 1/6] iommu: Deprecate Intel and AMD cmdline methods to enable strict mode John Garry
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: John Garry @ 2021-06-16 11:03 UTC (permalink / raw)
  To: joro, will, dwmw2, baolu.lu, robin.murphy, corbet
  Cc: linux-kernel, iommu, linuxarm, thunder.leizhen, chenxiang66,
	linux-doc, John Garry

This is a reboot of Zhen Lei's series from a couple of years ago, which
never made it across the line.

I still think that it has some value, so taking up the mantle.

Motivation:
Allow lazy mode be default mode for DMA domains for all ARCHs, and not
only those who hardcode it (to be lazy). For ARM64, currently we must use
a kernel command line parameter to use lazy mode, which is less than
ideal.

I have now included the print for strict/lazy mode, which I originally
sent in:
https://lore.kernel.org/linux-iommu/72eb3de9-1d1c-ae46-c5a9-95f26525d435@huawei.com/

There was some concern there about drivers and their custom prints
conflicting with the print in that patch, but I think that it
should be ok.

Differences to v12:
- Rebase to next-20210611 and include patch "iommu: Update "iommu.strict"
  documentation" as a baseline
- Add Robin's RB tags (thanks!)
	- Please let me know if not ok with kernel-parameters.txt update
	  in 3/6
- Add a patch to mark x86 strict cmdline params as deprecated
- Improve wording in Kconfig change and tweak iommu_dma_strict declaration

Differences to v11:
- Rebase to next-20210610
- Drop strict mode globals in Intel and AMD drivers
- Include patch to print strict vs lazy mode
- Include patch to remove argument from iommu_set_dma_strict()

Differences to v10:
- Rebase to v5.13-rc4
- Correct comment and typo in Kconfig (Randy)
- Make Kconfig choice depend on relevant architectures
- Update kernel-parameters.txt for CONFIG_IOMMU_DEFAULT_{LAZY,STRICT}

John Garry (3):
  iommu: Deprecate Intel and AMD cmdline methods to enable strict mode
  iommu: Print strict or lazy mode at init time
  iommu: Remove mode argument from iommu_set_dma_strict()

Zhen Lei (3):
  iommu: Enhance IOMMU default DMA mode build options
  iommu/vt-d: Add support for IOMMU default DMA mode build options
  iommu/amd: Add support for IOMMU default DMA mode build options

 .../admin-guide/kernel-parameters.txt         |  8 ++--
 drivers/iommu/Kconfig                         | 41 +++++++++++++++++++
 drivers/iommu/amd/amd_iommu_types.h           |  6 ---
 drivers/iommu/amd/init.c                      |  7 ++--
 drivers/iommu/amd/iommu.c                     |  6 ---
 drivers/iommu/intel/iommu.c                   | 16 ++++----
 drivers/iommu/iommu.c                         | 12 ++++--
 include/linux/iommu.h                         |  2 +-
 8 files changed, 66 insertions(+), 32 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v13 1/6] iommu: Deprecate Intel and AMD cmdline methods to enable strict mode
  2021-06-16 11:03 [PATCH v13 0/6] iommu: Enhance IOMMU default DMA mode build options John Garry
@ 2021-06-16 11:03 ` John Garry
  2021-06-17 19:01   ` Robin Murphy
  2021-06-16 11:03 ` [PATCH v13 2/6] iommu: Print strict or lazy mode at init time John Garry
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: John Garry @ 2021-06-16 11:03 UTC (permalink / raw)
  To: joro, will, dwmw2, baolu.lu, robin.murphy, corbet
  Cc: linux-kernel, iommu, linuxarm, thunder.leizhen, chenxiang66,
	linux-doc, John Garry

Now that the x86 drivers support iommu.strict, deprecate the custom
methods.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 5 +++--
 drivers/iommu/amd/init.c                        | 4 +++-
 drivers/iommu/intel/iommu.c                     | 1 +
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 30e9dd52464e..fcbb36d6eea7 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -290,7 +290,8 @@
 	amd_iommu=	[HW,X86-64]
 			Pass parameters to the AMD IOMMU driver in the system.
 			Possible values are:
-			fullflush - enable flushing of IO/TLB entries when
+			fullflush   [Deprecated, use iommu.strict instead]
+				  - enable flushing of IO/TLB entries when
 				    they are unmapped. Otherwise they are
 				    flushed before they will be reused, which
 				    is a lot of faster
@@ -1947,7 +1948,7 @@
 			bypassed by not enabling DMAR with this option. In
 			this case, gfx device will use physical address for
 			DMA.
-		strict [Default Off]
+		strict [Default Off] [Deprecated, use iommu.strict instead]
 			With this option on every unmap_single operation will
 			result in a hardware IOTLB flush operation as opposed
 			to batching them for performance.
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 46280e6e1535..9f3096d650aa 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3098,8 +3098,10 @@ static int __init parse_amd_iommu_intr(char *str)
 static int __init parse_amd_iommu_options(char *str)
 {
 	for (; *str; ++str) {
-		if (strncmp(str, "fullflush", 9) == 0)
+		if (strncmp(str, "fullflush", 9) == 0) {
+			pr_warn("amd_iommu=fullflush deprecated; use iommu.strict instead\n");
 			amd_iommu_unmap_flush = true;
+		}
 		if (strncmp(str, "force_enable", 12) == 0)
 			amd_iommu_force_enable = true;
 		if (strncmp(str, "off", 3) == 0)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index bd93c7ec879e..821d8227a4e6 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -454,6 +454,7 @@ static int __init intel_iommu_setup(char *str)
 			pr_warn("intel_iommu=forcedac deprecated; use iommu.forcedac instead\n");
 			iommu_dma_forcedac = true;
 		} else if (!strncmp(str, "strict", 6)) {
+			pr_warn("intel_iommu=strict deprecated; use iommu.strict instead\n");
 			pr_info("Disable batched IOTLB flush\n");
 			intel_iommu_strict = 1;
 		} else if (!strncmp(str, "sp_off", 6)) {
-- 
2.26.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v13 2/6] iommu: Print strict or lazy mode at init time
  2021-06-16 11:03 [PATCH v13 0/6] iommu: Enhance IOMMU default DMA mode build options John Garry
  2021-06-16 11:03 ` [PATCH v13 1/6] iommu: Deprecate Intel and AMD cmdline methods to enable strict mode John Garry
@ 2021-06-16 11:03 ` John Garry
  2021-06-16 11:03 ` [PATCH v13 3/6] iommu: Enhance IOMMU default DMA mode build options John Garry
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: John Garry @ 2021-06-16 11:03 UTC (permalink / raw)
  To: joro, will, dwmw2, baolu.lu, robin.murphy, corbet
  Cc: linux-kernel, iommu, linuxarm, thunder.leizhen, chenxiang66,
	linux-doc, John Garry

As well as the default domain type, it's useful to know whether strict
or lazy for DMA domains, so add this info in a separate print.

The (stict/lazy) mode may be also set via iommu.strict earlyparm, but
this will be processed prior to iommu_subsys_init(), so that print will be
accurate for drivers which don't set the mode via custom means.

For the drivers which set the mode via custom means - AMD and Intel drivers
- they maintain prints to inform a change in policy or that custom cmdline
methods to change policy are deprecated.

Signed-off-by: John Garry <john.garry@huawei.com>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/iommu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 5419c4b9f27a..cf58949cc2f3 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -138,6 +138,11 @@ static int __init iommu_subsys_init(void)
 		(iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ?
 			"(set via kernel command line)" : "");
 
+	pr_info("DMA domain TLB invalidation policy: %s mode %s\n",
+		iommu_dma_strict ? "strict" : "lazy",
+		(iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
+			"(set via kernel command line)" : "");
+
 	return 0;
 }
 subsys_initcall(iommu_subsys_init);
-- 
2.26.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v13 3/6] iommu: Enhance IOMMU default DMA mode build options
  2021-06-16 11:03 [PATCH v13 0/6] iommu: Enhance IOMMU default DMA mode build options John Garry
  2021-06-16 11:03 ` [PATCH v13 1/6] iommu: Deprecate Intel and AMD cmdline methods to enable strict mode John Garry
  2021-06-16 11:03 ` [PATCH v13 2/6] iommu: Print strict or lazy mode at init time John Garry
@ 2021-06-16 11:03 ` John Garry
  2021-06-16 11:03 ` [PATCH v13 4/6] iommu/vt-d: Add support for " John Garry
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: John Garry @ 2021-06-16 11:03 UTC (permalink / raw)
  To: joro, will, dwmw2, baolu.lu, robin.murphy, corbet
  Cc: linux-kernel, iommu, linuxarm, thunder.leizhen, chenxiang66,
	linux-doc, John Garry

From: Zhen Lei <thunder.leizhen@huawei.com>

First, add build options IOMMU_DEFAULT_{LAZY|STRICT}, so that we have the
opportunity to set {lazy|strict} mode as default at build time. Then put
the two config options in an choice, as they are mutually exclusive.

[jpg: Make choice between strict and lazy only (and not passthrough)]
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
Signed-off-by: John Garry <john.garry@huawei.com>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
 .../admin-guide/kernel-parameters.txt         |  3 +-
 drivers/iommu/Kconfig                         | 40 +++++++++++++++++++
 drivers/iommu/iommu.c                         |  2 +-
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index fcbb36d6eea7..d8fb36363be0 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2052,9 +2052,10 @@
 			  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.
+			unset - Use value of CONFIG_IOMMU_DEFAULT_{LAZY,STRICT}.
 			Note: on x86, the default behaviour depends on the
 			equivalent driver-specific parameters, but a strict
 			mode explicitly specified by either method takes
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 1f111b399bca..0327a942fdb7 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -90,6 +90,46 @@ config IOMMU_DEFAULT_PASSTHROUGH
 
 	  If unsure, say N here.
 
+choice
+	prompt "IOMMU default DMA IOTLB invalidation mode"
+	depends on IOMMU_DMA
+
+	default IOMMU_DEFAULT_STRICT
+	help
+	  This option allows an IOMMU DMA IOTLB invalidation mode to be
+	  chosen at build time, to override the default mode of each ARCH,
+	  removing the need to pass in kernel parameters through command line.
+	  It is still possible to provide common boot params to override this
+	  config.
+
+	  If unsure, keep the default.
+
+config IOMMU_DEFAULT_STRICT
+	bool "strict"
+	help
+	  For every IOMMU DMA unmap operation, the flush operation of IOTLB and
+	  the free operation of IOVA are guaranteed to be done in the unmap
+	  function.
+
+config IOMMU_DEFAULT_LAZY
+	bool "lazy"
+	help
+	  Support lazy mode, where for every IOMMU DMA unmap operation, the
+	  flush operation of IOTLB and the free operation of IOVA are deferred.
+	  They are only guaranteed to be done before the related IOVA will be
+	  reused.
+
+	  The isolation provided in this mode is not as secure as STRICT mode,
+	  such that a vulnerable time window may be created between the DMA
+	  unmap and the mappings cached in the IOMMU IOTLB or device TLB
+	  finally being invalidated, where the device could still access the
+	  memory which has already been unmapped by the device driver.
+	  However this mode may provide better performance in high throughput
+	  scenarios, and is still considerably more secure than passthrough
+	  mode or no IOMMU.
+
+endchoice
+
 config OF_IOMMU
 	def_bool y
 	depends on OF && IOMMU_API
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index cf58949cc2f3..60b1ec42e73b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -29,7 +29,7 @@ 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 bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_STRICT);
 static u32 iommu_cmd_line __read_mostly;
 
 struct iommu_group {
-- 
2.26.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v13 4/6] iommu/vt-d: Add support for IOMMU default DMA mode build options
  2021-06-16 11:03 [PATCH v13 0/6] iommu: Enhance IOMMU default DMA mode build options John Garry
                   ` (2 preceding siblings ...)
  2021-06-16 11:03 ` [PATCH v13 3/6] iommu: Enhance IOMMU default DMA mode build options John Garry
@ 2021-06-16 11:03 ` John Garry
  2021-06-17  7:32   ` Lu Baolu
  2021-06-16 11:03 ` [PATCH v13 5/6] iommu/amd: " John Garry
  2021-06-16 11:03 ` [PATCH v13 6/6] iommu: Remove mode argument from iommu_set_dma_strict() John Garry
  5 siblings, 1 reply; 19+ messages in thread
From: John Garry @ 2021-06-16 11:03 UTC (permalink / raw)
  To: joro, will, dwmw2, baolu.lu, robin.murphy, corbet
  Cc: linux-kernel, iommu, linuxarm, thunder.leizhen, chenxiang66,
	linux-doc, John Garry

From: Zhen Lei <thunder.leizhen@huawei.com>

Make IOMMU_DEFAULT_LAZY default for when INTEL_IOMMU config is set,
as is current behaviour.

Also delete global flag intel_iommu_strict:
- In intel_iommu_setup(), call iommu_set_dma_strict(true) directly. Also
  remove the print, as iommu_subsys_init() prints the mode and we have
  already marked this param as deprecated.

- For cap_caching_mode() check in intel_iommu_setup(), call
  iommu_set_dma_strict(true) directly, and reword the accompanying print
  and add the missing '\n'.

- For Ironlake GPU, again call iommu_set_dma_strict(true) directly and
  keep the accompanying print.

[jpg: Remove intel_iommu_strict]
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/iommu/Kconfig       |  1 +
 drivers/iommu/intel/iommu.c | 15 ++++++---------
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 0327a942fdb7..c214a36eb2dc 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -94,6 +94,7 @@ choice
 	prompt "IOMMU default DMA IOTLB invalidation mode"
 	depends on IOMMU_DMA
 
+	default IOMMU_DEFAULT_LAZY if INTEL_IOMMU
 	default IOMMU_DEFAULT_STRICT
 	help
 	  This option allows an IOMMU DMA IOTLB invalidation mode to be
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 821d8227a4e6..d586990fa751 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -361,7 +361,6 @@ int intel_iommu_enabled = 0;
 EXPORT_SYMBOL_GPL(intel_iommu_enabled);
 
 static int dmar_map_gfx = 1;
-static int intel_iommu_strict;
 static int intel_iommu_superpage = 1;
 static int iommu_identity_mapping;
 static int iommu_skip_te_disable;
@@ -455,8 +454,7 @@ static int __init intel_iommu_setup(char *str)
 			iommu_dma_forcedac = true;
 		} else if (!strncmp(str, "strict", 6)) {
 			pr_warn("intel_iommu=strict deprecated; use iommu.strict instead\n");
-			pr_info("Disable batched IOTLB flush\n");
-			intel_iommu_strict = 1;
+			iommu_set_dma_strict(true);
 		} else if (!strncmp(str, "sp_off", 6)) {
 			pr_info("Disable supported super page\n");
 			intel_iommu_superpage = 0;
@@ -4382,9 +4380,9 @@ int __init intel_iommu_init(void)
 		 * is likely to be much lower than the overhead of synchronizing
 		 * the virtual and physical IOMMU page-tables.
 		 */
-		if (!intel_iommu_strict && cap_caching_mode(iommu->cap)) {
-			pr_warn("IOMMU batching is disabled due to virtualization");
-			intel_iommu_strict = 1;
+		if (cap_caching_mode(iommu->cap)) {
+			pr_warn("IOMMU batching disallowed due to virtualization\n");
+			iommu_set_dma_strict(true);
 		}
 		iommu_device_sysfs_add(&iommu->iommu, NULL,
 				       intel_iommu_groups,
@@ -4393,7 +4391,6 @@ int __init intel_iommu_init(void)
 	}
 	up_read(&dmar_global_lock);
 
-	iommu_set_dma_strict(intel_iommu_strict);
 	bus_set_iommu(&pci_bus_type, &intel_iommu_ops);
 	if (si_domain && !hw_pass_through)
 		register_memory_notifier(&intel_iommu_memory_nb);
@@ -5702,8 +5699,8 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
 	} else if (dmar_map_gfx) {
 		/* we have to ensure the gfx device is idle before we flush */
 		pci_info(dev, "Disabling batched IOTLB flush on Ironlake\n");
-		intel_iommu_strict = 1;
-       }
+		iommu_set_dma_strict(true);
+	}
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0040, quirk_calpella_no_shadow_gtt);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0044, quirk_calpella_no_shadow_gtt);
-- 
2.26.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v13 5/6] iommu/amd: Add support for IOMMU default DMA mode build options
  2021-06-16 11:03 [PATCH v13 0/6] iommu: Enhance IOMMU default DMA mode build options John Garry
                   ` (3 preceding siblings ...)
  2021-06-16 11:03 ` [PATCH v13 4/6] iommu/vt-d: Add support for " John Garry
@ 2021-06-16 11:03 ` John Garry
  2021-06-16 11:03 ` [PATCH v13 6/6] iommu: Remove mode argument from iommu_set_dma_strict() John Garry
  5 siblings, 0 replies; 19+ messages in thread
From: John Garry @ 2021-06-16 11:03 UTC (permalink / raw)
  To: joro, will, dwmw2, baolu.lu, robin.murphy, corbet
  Cc: linux-kernel, iommu, linuxarm, thunder.leizhen, chenxiang66,
	linux-doc, John Garry

From: Zhen Lei <thunder.leizhen@huawei.com>

Make IOMMU_DEFAULT_LAZY default for when AMD_IOMMU config is set, which
matches current behaviour.

For "fullflush" param, just call iommu_set_dma_strict(true) directly.

Since we get a strict vs lazy mode print already in iommu_subsys_init(),
and maintain a deprecation print when "fullflush" param is passed, drop the
prints in amd_iommu_init_dma_ops().

Finally drop global flag amd_iommu_unmap_flush, as it has no longer has any
purpose.

[jpg: Rebase for relocated file and drop amd_iommu_unmap_flush]
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/iommu/Kconfig               | 2 +-
 drivers/iommu/amd/amd_iommu_types.h | 6 ------
 drivers/iommu/amd/init.c            | 3 +--
 drivers/iommu/amd/iommu.c           | 6 ------
 4 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index c214a36eb2dc..fd1ad28dd5ee 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -94,7 +94,7 @@ choice
 	prompt "IOMMU default DMA IOTLB invalidation mode"
 	depends on IOMMU_DMA
 
-	default IOMMU_DEFAULT_LAZY if INTEL_IOMMU
+	default IOMMU_DEFAULT_LAZY if (AMD_IOMMU || INTEL_IOMMU)
 	default IOMMU_DEFAULT_STRICT
 	help
 	  This option allows an IOMMU DMA IOTLB invalidation mode to be
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 94c1a7a9876d..8dbe61e2b3c1 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -779,12 +779,6 @@ extern u16 amd_iommu_last_bdf;
 /* allocation bitmap for domain ids */
 extern unsigned long *amd_iommu_pd_alloc_bitmap;
 
-/*
- * If true, the addresses will be flushed on unmap time, not when
- * they are reused
- */
-extern bool amd_iommu_unmap_flush;
-
 /* Smallest max PASID supported by any IOMMU in the system */
 extern u32 amd_iommu_max_pasid;
 
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 9f3096d650aa..fb3618af643b 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -161,7 +161,6 @@ u16 amd_iommu_last_bdf;			/* largest PCI device id we have
 					   to handle */
 LIST_HEAD(amd_iommu_unity_map);		/* a list of required unity mappings
 					   we find in ACPI */
-bool amd_iommu_unmap_flush;		/* if true, flush on every unmap */
 
 LIST_HEAD(amd_iommu_list);		/* list of all AMD IOMMUs in the
 					   system */
@@ -3100,7 +3099,7 @@ static int __init parse_amd_iommu_options(char *str)
 	for (; *str; ++str) {
 		if (strncmp(str, "fullflush", 9) == 0) {
 			pr_warn("amd_iommu=fullflush deprecated; use iommu.strict instead\n");
-			amd_iommu_unmap_flush = true;
+			iommu_set_dma_strict(true);
 		}
 		if (strncmp(str, "force_enable", 12) == 0)
 			amd_iommu_force_enable = true;
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index b1fbf2c83df5..32b541ee2e11 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1775,12 +1775,6 @@ void amd_iommu_domain_update(struct protection_domain *domain)
 static void __init amd_iommu_init_dma_ops(void)
 {
 	swiotlb = (iommu_default_passthrough() || sme_me_mask) ? 1 : 0;
-
-	if (amd_iommu_unmap_flush)
-		pr_info("IO/TLB flush on unmap enabled\n");
-	else
-		pr_info("Lazy IO/TLB flushing enabled\n");
-	iommu_set_dma_strict(amd_iommu_unmap_flush);
 }
 
 int __init amd_iommu_init_api(void)
-- 
2.26.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v13 6/6] iommu: Remove mode argument from iommu_set_dma_strict()
  2021-06-16 11:03 [PATCH v13 0/6] iommu: Enhance IOMMU default DMA mode build options John Garry
                   ` (4 preceding siblings ...)
  2021-06-16 11:03 ` [PATCH v13 5/6] iommu/amd: " John Garry
@ 2021-06-16 11:03 ` John Garry
  2021-06-17  7:36   ` Lu Baolu
  5 siblings, 1 reply; 19+ messages in thread
From: John Garry @ 2021-06-16 11:03 UTC (permalink / raw)
  To: joro, will, dwmw2, baolu.lu, robin.murphy, corbet
  Cc: linux-kernel, iommu, linuxarm, thunder.leizhen, chenxiang66,
	linux-doc, John Garry

We only ever now set strict mode enabled in iommu_set_dma_strict(), so
just remove the argument.

Signed-off-by: John Garry <john.garry@huawei.com>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/amd/init.c    | 2 +-
 drivers/iommu/intel/iommu.c | 6 +++---
 drivers/iommu/iommu.c       | 5 ++---
 include/linux/iommu.h       | 2 +-
 4 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index fb3618af643b..7bc460052678 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3099,7 +3099,7 @@ static int __init parse_amd_iommu_options(char *str)
 	for (; *str; ++str) {
 		if (strncmp(str, "fullflush", 9) == 0) {
 			pr_warn("amd_iommu=fullflush deprecated; use iommu.strict instead\n");
-			iommu_set_dma_strict(true);
+			iommu_set_dma_strict();
 		}
 		if (strncmp(str, "force_enable", 12) == 0)
 			amd_iommu_force_enable = true;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index d586990fa751..0618c35cfb51 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -454,7 +454,7 @@ static int __init intel_iommu_setup(char *str)
 			iommu_dma_forcedac = true;
 		} else if (!strncmp(str, "strict", 6)) {
 			pr_warn("intel_iommu=strict deprecated; use iommu.strict instead\n");
-			iommu_set_dma_strict(true);
+			iommu_set_dma_strict();
 		} else if (!strncmp(str, "sp_off", 6)) {
 			pr_info("Disable supported super page\n");
 			intel_iommu_superpage = 0;
@@ -4382,7 +4382,7 @@ int __init intel_iommu_init(void)
 		 */
 		if (cap_caching_mode(iommu->cap)) {
 			pr_warn("IOMMU batching disallowed due to virtualization\n");
-			iommu_set_dma_strict(true);
+			iommu_set_dma_strict();
 		}
 		iommu_device_sysfs_add(&iommu->iommu, NULL,
 				       intel_iommu_groups,
@@ -5699,7 +5699,7 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
 	} else if (dmar_map_gfx) {
 		/* we have to ensure the gfx device is idle before we flush */
 		pci_info(dev, "Disabling batched IOTLB flush on Ironlake\n");
-		iommu_set_dma_strict(true);
+		iommu_set_dma_strict();
 	}
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0040, quirk_calpella_no_shadow_gtt);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 60b1ec42e73b..ff221d3ddcbc 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -349,10 +349,9 @@ static int __init iommu_dma_setup(char *str)
 }
 early_param("iommu.strict", iommu_dma_setup);
 
-void iommu_set_dma_strict(bool strict)
+void iommu_set_dma_strict(void)
 {
-	if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
-		iommu_dma_strict = strict;
+	iommu_dma_strict = true;
 }
 
 bool iommu_get_dma_strict(struct iommu_domain *domain)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 32d448050bf7..754f67d6dd90 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -476,7 +476,7 @@ int iommu_enable_nesting(struct iommu_domain *domain);
 int iommu_set_pgtable_quirks(struct iommu_domain *domain,
 		unsigned long quirks);
 
-void iommu_set_dma_strict(bool val);
+void iommu_set_dma_strict(void);
 bool iommu_get_dma_strict(struct iommu_domain *domain);
 
 extern int report_iommu_fault(struct iommu_domain *domain, struct device *dev,
-- 
2.26.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v13 4/6] iommu/vt-d: Add support for IOMMU default DMA mode build options
  2021-06-16 11:03 ` [PATCH v13 4/6] iommu/vt-d: Add support for " John Garry
@ 2021-06-17  7:32   ` Lu Baolu
  2021-06-17  8:00     ` John Garry
  0 siblings, 1 reply; 19+ messages in thread
From: Lu Baolu @ 2021-06-17  7:32 UTC (permalink / raw)
  To: John Garry, joro, will, dwmw2, robin.murphy, corbet
  Cc: baolu.lu, linux-kernel, iommu, linuxarm, thunder.leizhen,
	chenxiang66, linux-doc

On 6/16/21 7:03 PM, John Garry wrote:
> @@ -4382,9 +4380,9 @@ int __init intel_iommu_init(void)
>   		 * is likely to be much lower than the overhead of synchronizing
>   		 * the virtual and physical IOMMU page-tables.
>   		 */
> -		if (!intel_iommu_strict && cap_caching_mode(iommu->cap)) {
> -			pr_warn("IOMMU batching is disabled due to virtualization");
> -			intel_iommu_strict = 1;
> +		if (cap_caching_mode(iommu->cap)) {
> +			pr_warn("IOMMU batching disallowed due to virtualization\n");
> +			iommu_set_dma_strict(true);

With this change, VM guest will always show this warning. How about
removing this message? Users could get the same information through the
kernel message added by "[PATCH v13 2/6] iommu: Print strict or lazy
mode at init time".

Best regards,
baolu

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v13 6/6] iommu: Remove mode argument from iommu_set_dma_strict()
  2021-06-16 11:03 ` [PATCH v13 6/6] iommu: Remove mode argument from iommu_set_dma_strict() John Garry
@ 2021-06-17  7:36   ` Lu Baolu
  2021-06-17  7:41     ` John Garry
  2021-06-17 18:56     ` Robin Murphy
  0 siblings, 2 replies; 19+ messages in thread
From: Lu Baolu @ 2021-06-17  7:36 UTC (permalink / raw)
  To: John Garry, joro, will, dwmw2, robin.murphy, corbet
  Cc: baolu.lu, linux-kernel, iommu, linuxarm, thunder.leizhen,
	chenxiang66, linux-doc

On 6/16/21 7:03 PM, John Garry wrote:
> We only ever now set strict mode enabled in iommu_set_dma_strict(), so
> just remove the argument.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> ---
>   drivers/iommu/amd/init.c    | 2 +-
>   drivers/iommu/intel/iommu.c | 6 +++---
>   drivers/iommu/iommu.c       | 5 ++---
>   include/linux/iommu.h       | 2 +-
>   4 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index fb3618af643b..7bc460052678 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3099,7 +3099,7 @@ static int __init parse_amd_iommu_options(char *str)
>   	for (; *str; ++str) {
>   		if (strncmp(str, "fullflush", 9) == 0) {
>   			pr_warn("amd_iommu=fullflush deprecated; use iommu.strict instead\n");
> -			iommu_set_dma_strict(true);
> +			iommu_set_dma_strict();
>   		}
>   		if (strncmp(str, "force_enable", 12) == 0)
>   			amd_iommu_force_enable = true;
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index d586990fa751..0618c35cfb51 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -454,7 +454,7 @@ static int __init intel_iommu_setup(char *str)
>   			iommu_dma_forcedac = true;
>   		} else if (!strncmp(str, "strict", 6)) {
>   			pr_warn("intel_iommu=strict deprecated; use iommu.strict instead\n");
> -			iommu_set_dma_strict(true);
> +			iommu_set_dma_strict();
>   		} else if (!strncmp(str, "sp_off", 6)) {
>   			pr_info("Disable supported super page\n");
>   			intel_iommu_superpage = 0;
> @@ -4382,7 +4382,7 @@ int __init intel_iommu_init(void)
>   		 */
>   		if (cap_caching_mode(iommu->cap)) {
>   			pr_warn("IOMMU batching disallowed due to virtualization\n");
> -			iommu_set_dma_strict(true);
> +			iommu_set_dma_strict();
>   		}
>   		iommu_device_sysfs_add(&iommu->iommu, NULL,
>   				       intel_iommu_groups,
> @@ -5699,7 +5699,7 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
>   	} else if (dmar_map_gfx) {
>   		/* we have to ensure the gfx device is idle before we flush */
>   		pci_info(dev, "Disabling batched IOTLB flush on Ironlake\n");
> -		iommu_set_dma_strict(true);
> +		iommu_set_dma_strict();
>   	}
>   }
>   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0040, quirk_calpella_no_shadow_gtt);
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 60b1ec42e73b..ff221d3ddcbc 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -349,10 +349,9 @@ static int __init iommu_dma_setup(char *str)
>   }
>   early_param("iommu.strict", iommu_dma_setup);
>   
> -void iommu_set_dma_strict(bool strict)
> +void iommu_set_dma_strict(void)
>   {
> -	if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
> -		iommu_dma_strict = strict;
> +	iommu_dma_strict = true;

Sorry, I still can't get how iommu.strict kernel option works.

static int __init iommu_dma_setup(char *str)
{
         int ret = kstrtobool(str, &iommu_dma_strict);

         if (!ret)
                 iommu_cmd_line |= IOMMU_CMD_LINE_STRICT;
         return ret;
}
early_param("iommu.strict", iommu_dma_setup);

The bit IOMMU_CMD_LINE_STRICT is only set, but not used anywhere. Hence,
I am wondering how could it work? A bug or I missed anything?

Best regards,
baolu

>   }
>   
>   bool iommu_get_dma_strict(struct iommu_domain *domain)
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 32d448050bf7..754f67d6dd90 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -476,7 +476,7 @@ int iommu_enable_nesting(struct iommu_domain *domain);
>   int iommu_set_pgtable_quirks(struct iommu_domain *domain,
>   		unsigned long quirks);
>   
> -void iommu_set_dma_strict(bool val);
> +void iommu_set_dma_strict(void);
>   bool iommu_get_dma_strict(struct iommu_domain *domain);
>   
>   extern int report_iommu_fault(struct iommu_domain *domain, struct device *dev,
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v13 6/6] iommu: Remove mode argument from iommu_set_dma_strict()
  2021-06-17  7:36   ` Lu Baolu
@ 2021-06-17  7:41     ` John Garry
  2021-06-18  1:52       ` Lu Baolu
  2021-06-17 18:56     ` Robin Murphy
  1 sibling, 1 reply; 19+ messages in thread
From: John Garry @ 2021-06-17  7:41 UTC (permalink / raw)
  To: Lu Baolu, joro, will, dwmw2, robin.murphy, corbet
  Cc: linux-kernel, iommu, linuxarm, thunder.leizhen, chenxiang66, linux-doc


>> @@ -349,10 +349,9 @@ static int __init iommu_dma_setup(char *str)
>>   }
>>   early_param("iommu.strict", iommu_dma_setup);
>> -void iommu_set_dma_strict(bool strict)
>> +void iommu_set_dma_strict(void)
>>   {
>> -    if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>> -        iommu_dma_strict = strict;
>> +    iommu_dma_strict = true;
> 
> Sorry, I still can't get how iommu.strict kernel option works.
> 
> static int __init iommu_dma_setup(char *str)
> {
>          int ret = kstrtobool(str, &iommu_dma_strict);
> 
>          if (!ret)
>                  iommu_cmd_line |= IOMMU_CMD_LINE_STRICT;
>          return ret;
> }
> early_param("iommu.strict", iommu_dma_setup);
> 
> The bit IOMMU_CMD_LINE_STRICT is only set, but not used anywhere.

It is used in patch 2/6:

+	pr_info("DMA domain TLB invalidation policy: %s mode %s\n",
+		iommu_dma_strict ? "strict" : "lazy",
+		(iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
+			"(set via kernel command line)" : "");

> Hence,
> I am wondering how could it work? A bug or I missed anything?

It is really just used for informative purpose now.

Thanks,
john

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v13 4/6] iommu/vt-d: Add support for IOMMU default DMA mode build options
  2021-06-17  7:32   ` Lu Baolu
@ 2021-06-17  8:00     ` John Garry
  2021-06-17 19:03       ` Robin Murphy
  2021-06-18  1:46       ` Lu Baolu
  0 siblings, 2 replies; 19+ messages in thread
From: John Garry @ 2021-06-17  8:00 UTC (permalink / raw)
  To: Lu Baolu, joro, will, dwmw2, robin.murphy, corbet
  Cc: linux-kernel, iommu, linuxarm, thunder.leizhen, chenxiang66, linux-doc

On 17/06/2021 08:32, Lu Baolu wrote:
> On 6/16/21 7:03 PM, John Garry wrote:
>> @@ -4382,9 +4380,9 @@ int __init intel_iommu_init(void)
>>            * is likely to be much lower than the overhead of 
>> synchronizing
>>            * the virtual and physical IOMMU page-tables.
>>            */
>> -        if (!intel_iommu_strict && cap_caching_mode(iommu->cap)) {
>> -            pr_warn("IOMMU batching is disabled due to virtualization");
>> -            intel_iommu_strict = 1;
>> +        if (cap_caching_mode(iommu->cap)) {
>> +            pr_warn("IOMMU batching disallowed due to 
>> virtualization\n");
>> +            iommu_set_dma_strict(true);
> 
> With this change, VM guest will always show this warning.

Would they have got it before also normally?

I mean, default is intel_iommu_strict=0, so if 
cap_caching_mode(iommu->cap) is true and intel_iommu_strict not set to 1 
elsewhere previously, then we would get this print.

> How about
> removing this message? Users could get the same information through the
> kernel message added by "[PATCH v13 2/6] iommu: Print strict or lazy
> mode at init time".

I think that the print from 2/6 should occur before this print.

Regardless I would think that you would still like to be notified of 
this change in policy, right?

However I now realize that the print is in a loop per iommu, so we would 
get it per iommu:

for_each_active_iommu(iommu, drhd) {
	/*
	 * The flush queue implementation does not perform
	 * page-selective invalidations that are required for efficient
	 * TLB flushes in virtual environments.  The benefit of batching
	 * is likely to be much lower than the overhead of synchronizing
	 * the virtual and physical IOMMU page-tables.
	 */
	if (!intel_iommu_strict && cap_caching_mode(iommu->cap)) {
		pr_warn("IOMMU batching is disabled due to virtualization");
		intel_iommu_strict = 1;
	}
	...
}

I need to change that. How about this:

bool print_warning = false;

for_each_active_iommu(iommu, drhd) {
	/*
	 * The flush queue implementation does not perform
	 * page-selective invalidations that are required for efficient
	 * TLB flushes in virtual environments.  The benefit of batching
	 * is likely to be much lower than the overhead of synchronizing
	 * the virtual and physical IOMMU page-tables.
	 */
	if (!print_warning && cap_caching_mode(iommu->cap)) {
		pr_warn("IOMMU batching disallowed due to virtualization\n");
		iommu_set_dma_strict(true);
		print_warning = true;
	}
	...
}

or use pr_warn_once().

Thanks,
John

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v13 6/6] iommu: Remove mode argument from iommu_set_dma_strict()
  2021-06-17  7:36   ` Lu Baolu
  2021-06-17  7:41     ` John Garry
@ 2021-06-17 18:56     ` Robin Murphy
  2021-06-18  1:51       ` Lu Baolu
  1 sibling, 1 reply; 19+ messages in thread
From: Robin Murphy @ 2021-06-17 18:56 UTC (permalink / raw)
  To: Lu Baolu, John Garry, joro, will, dwmw2, corbet
  Cc: linux-kernel, iommu, linuxarm, thunder.leizhen, chenxiang66, linux-doc

On 2021-06-17 08:36, Lu Baolu wrote:
> On 6/16/21 7:03 PM, John Garry wrote:
>> We only ever now set strict mode enabled in iommu_set_dma_strict(), so
>> just remove the argument.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/amd/init.c    | 2 +-
>>   drivers/iommu/intel/iommu.c | 6 +++---
>>   drivers/iommu/iommu.c       | 5 ++---
>>   include/linux/iommu.h       | 2 +-
>>   4 files changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>> index fb3618af643b..7bc460052678 100644
>> --- a/drivers/iommu/amd/init.c
>> +++ b/drivers/iommu/amd/init.c
>> @@ -3099,7 +3099,7 @@ static int __init parse_amd_iommu_options(char 
>> *str)
>>       for (; *str; ++str) {
>>           if (strncmp(str, "fullflush", 9) == 0) {
>>               pr_warn("amd_iommu=fullflush deprecated; use 
>> iommu.strict instead\n");
>> -            iommu_set_dma_strict(true);
>> +            iommu_set_dma_strict();
>>           }
>>           if (strncmp(str, "force_enable", 12) == 0)
>>               amd_iommu_force_enable = true;
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index d586990fa751..0618c35cfb51 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -454,7 +454,7 @@ static int __init intel_iommu_setup(char *str)
>>               iommu_dma_forcedac = true;
>>           } else if (!strncmp(str, "strict", 6)) {
>>               pr_warn("intel_iommu=strict deprecated; use iommu.strict 
>> instead\n");
>> -            iommu_set_dma_strict(true);
>> +            iommu_set_dma_strict();
>>           } else if (!strncmp(str, "sp_off", 6)) {
>>               pr_info("Disable supported super page\n");
>>               intel_iommu_superpage = 0;
>> @@ -4382,7 +4382,7 @@ int __init intel_iommu_init(void)
>>            */
>>           if (cap_caching_mode(iommu->cap)) {
>>               pr_warn("IOMMU batching disallowed due to 
>> virtualization\n");
>> -            iommu_set_dma_strict(true);
>> +            iommu_set_dma_strict();
>>           }
>>           iommu_device_sysfs_add(&iommu->iommu, NULL,
>>                          intel_iommu_groups,
>> @@ -5699,7 +5699,7 @@ static void quirk_calpella_no_shadow_gtt(struct 
>> pci_dev *dev)
>>       } else if (dmar_map_gfx) {
>>           /* we have to ensure the gfx device is idle before we flush */
>>           pci_info(dev, "Disabling batched IOTLB flush on Ironlake\n");
>> -        iommu_set_dma_strict(true);
>> +        iommu_set_dma_strict();
>>       }
>>   }
>>   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0040, 
>> quirk_calpella_no_shadow_gtt);
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 60b1ec42e73b..ff221d3ddcbc 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -349,10 +349,9 @@ static int __init iommu_dma_setup(char *str)
>>   }
>>   early_param("iommu.strict", iommu_dma_setup);
>> -void iommu_set_dma_strict(bool strict)
>> +void iommu_set_dma_strict(void)
>>   {
>> -    if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>> -        iommu_dma_strict = strict;
>> +    iommu_dma_strict = true;
> 
> Sorry, I still can't get how iommu.strict kernel option works.
> 
> static int __init iommu_dma_setup(char *str)
> {
>          int ret = kstrtobool(str, &iommu_dma_strict);

Note that this is the bit that does the real work - if the argument 
parses OK then iommu_dma_strict is reassigned with the appropriate 
value. The iommu_cmd_line stuff is a bit of additional bookkeeping, 
basically just so we can see whether default values have been overridden.

Robin.

> 
>          if (!ret)
>                  iommu_cmd_line |= IOMMU_CMD_LINE_STRICT;
>          return ret;
> }
> early_param("iommu.strict", iommu_dma_setup);
> 
> The bit IOMMU_CMD_LINE_STRICT is only set, but not used anywhere. Hence,
> I am wondering how could it work? A bug or I missed anything?
> 
> Best regards,
> baolu
> 
>>   }
>>   bool iommu_get_dma_strict(struct iommu_domain *domain)
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 32d448050bf7..754f67d6dd90 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -476,7 +476,7 @@ int iommu_enable_nesting(struct iommu_domain 
>> *domain);
>>   int iommu_set_pgtable_quirks(struct iommu_domain *domain,
>>           unsigned long quirks);
>> -void iommu_set_dma_strict(bool val);
>> +void iommu_set_dma_strict(void);
>>   bool iommu_get_dma_strict(struct iommu_domain *domain);
>>   extern int report_iommu_fault(struct iommu_domain *domain, struct 
>> device *dev,
>>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v13 1/6] iommu: Deprecate Intel and AMD cmdline methods to enable strict mode
  2021-06-16 11:03 ` [PATCH v13 1/6] iommu: Deprecate Intel and AMD cmdline methods to enable strict mode John Garry
@ 2021-06-17 19:01   ` Robin Murphy
  2021-06-18  7:43     ` John Garry
  0 siblings, 1 reply; 19+ messages in thread
From: Robin Murphy @ 2021-06-17 19:01 UTC (permalink / raw)
  To: John Garry, joro, will, dwmw2, baolu.lu, corbet
  Cc: linux-kernel, iommu, linuxarm, thunder.leizhen, chenxiang66, linux-doc

On 2021-06-16 12:03, John Garry wrote:
> Now that the x86 drivers support iommu.strict, deprecate the custom
> methods.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   Documentation/admin-guide/kernel-parameters.txt | 5 +++--
>   drivers/iommu/amd/init.c                        | 4 +++-
>   drivers/iommu/intel/iommu.c                     | 1 +
>   3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 30e9dd52464e..fcbb36d6eea7 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -290,7 +290,8 @@
>   	amd_iommu=	[HW,X86-64]
>   			Pass parameters to the AMD IOMMU driver in the system.
>   			Possible values are:
> -			fullflush - enable flushing of IO/TLB entries when
> +			fullflush   [Deprecated, use iommu.strict instead]
> +				  - enable flushing of IO/TLB entries when
>   				    they are unmapped. Otherwise they are
>   				    flushed before they will be reused, which
>   				    is a lot of faster
> @@ -1947,7 +1948,7 @@
>   			bypassed by not enabling DMAR with this option. In
>   			this case, gfx device will use physical address for
>   			DMA.
> -		strict [Default Off]
> +		strict [Default Off] [Deprecated, use iommu.strict instead]
>   			With this option on every unmap_single operation will
>   			result in a hardware IOTLB flush operation as opposed
>   			to batching them for performance.

FWIW I'd be inclined to replace both whole descriptions with just 
something like "Deprecated, equivalent to iommu.strict=1".

> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 46280e6e1535..9f3096d650aa 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3098,8 +3098,10 @@ static int __init parse_amd_iommu_intr(char *str)
>   static int __init parse_amd_iommu_options(char *str)
>   {
>   	for (; *str; ++str) {
> -		if (strncmp(str, "fullflush", 9) == 0)
> +		if (strncmp(str, "fullflush", 9) == 0) {
> +			pr_warn("amd_iommu=fullflush deprecated; use iommu.strict instead\n");

Nit: maybe we should spell out "...use <option>=1 instead" in all of 
these messages just in case anyone takes them literally? (I'm not sure 
the options parse correctly with no argument)

Either way,

Acked-by: Robin Murphy <robin.murphy@arm.com>

Thanks,
Robin.

>   			amd_iommu_unmap_flush = true;
> +		}
>   		if (strncmp(str, "force_enable", 12) == 0)
>   			amd_iommu_force_enable = true;
>   		if (strncmp(str, "off", 3) == 0)
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index bd93c7ec879e..821d8227a4e6 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -454,6 +454,7 @@ static int __init intel_iommu_setup(char *str)
>   			pr_warn("intel_iommu=forcedac deprecated; use iommu.forcedac instead\n");
>   			iommu_dma_forcedac = true;
>   		} else if (!strncmp(str, "strict", 6)) {
> +			pr_warn("intel_iommu=strict deprecated; use iommu.strict instead\n");
>   			pr_info("Disable batched IOTLB flush\n");
>   			intel_iommu_strict = 1;
>   		} else if (!strncmp(str, "sp_off", 6)) {
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v13 4/6] iommu/vt-d: Add support for IOMMU default DMA mode build options
  2021-06-17  8:00     ` John Garry
@ 2021-06-17 19:03       ` Robin Murphy
  2021-06-18  1:46       ` Lu Baolu
  1 sibling, 0 replies; 19+ messages in thread
From: Robin Murphy @ 2021-06-17 19:03 UTC (permalink / raw)
  To: John Garry, Lu Baolu, joro, will, dwmw2, corbet
  Cc: linux-kernel, iommu, linuxarm, thunder.leizhen, chenxiang66, linux-doc

On 2021-06-17 09:00, John Garry wrote:
> On 17/06/2021 08:32, Lu Baolu wrote:
>> On 6/16/21 7:03 PM, John Garry wrote:
>>> @@ -4382,9 +4380,9 @@ int __init intel_iommu_init(void)
>>>            * is likely to be much lower than the overhead of 
>>> synchronizing
>>>            * the virtual and physical IOMMU page-tables.
>>>            */
>>> -        if (!intel_iommu_strict && cap_caching_mode(iommu->cap)) {
>>> -            pr_warn("IOMMU batching is disabled due to 
>>> virtualization");
>>> -            intel_iommu_strict = 1;
>>> +        if (cap_caching_mode(iommu->cap)) {
>>> +            pr_warn("IOMMU batching disallowed due to 
>>> virtualization\n");
>>> +            iommu_set_dma_strict(true);
>>
>> With this change, VM guest will always show this warning.
> 
> Would they have got it before also normally?
> 
> I mean, default is intel_iommu_strict=0, so if 
> cap_caching_mode(iommu->cap) is true and intel_iommu_strict not set to 1 
> elsewhere previously, then we would get this print.
> 
>> How about
>> removing this message? Users could get the same information through the
>> kernel message added by "[PATCH v13 2/6] iommu: Print strict or lazy
>> mode at init time".
> 
> I think that the print from 2/6 should occur before this print.
> 
> Regardless I would think that you would still like to be notified of 
> this change in policy, right?
> 
> However I now realize that the print is in a loop per iommu, so we would 
> get it per iommu:
> 
> for_each_active_iommu(iommu, drhd) {
>      /*
>       * The flush queue implementation does not perform
>       * page-selective invalidations that are required for efficient
>       * TLB flushes in virtual environments.  The benefit of batching
>       * is likely to be much lower than the overhead of synchronizing
>       * the virtual and physical IOMMU page-tables.
>       */
>      if (!intel_iommu_strict && cap_caching_mode(iommu->cap)) {
>          pr_warn("IOMMU batching is disabled due to virtualization");
>          intel_iommu_strict = 1;
>      }
>      ...
> }
> 
> I need to change that. How about this:
> 
> bool print_warning = false;
> 
> for_each_active_iommu(iommu, drhd) {
>      /*
>       * The flush queue implementation does not perform
>       * page-selective invalidations that are required for efficient
>       * TLB flushes in virtual environments.  The benefit of batching
>       * is likely to be much lower than the overhead of synchronizing
>       * the virtual and physical IOMMU page-tables.
>       */
>      if (!print_warning && cap_caching_mode(iommu->cap)) {
>          pr_warn("IOMMU batching disallowed due to virtualization\n");
>          iommu_set_dma_strict(true);
>          print_warning = true;
>      }
>      ...
> }
> 
> or use pr_warn_once().

Maybe even downgrade it to pr_info_once(), since AIUI it's not really 
anything scary?

I suppose you could technically fake up a domain on the stack to get the 
global setting out of iommu_get_dma_strict(), or perhaps give 
iommu_set_dma_strict() a cheeky return value to indicate what the 
previous setting was, in order to suppress the message entirely if 
strict is already set, but I'm not at all convinced it's worth the bother.

Robin.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v13 4/6] iommu/vt-d: Add support for IOMMU default DMA mode build options
  2021-06-17  8:00     ` John Garry
  2021-06-17 19:03       ` Robin Murphy
@ 2021-06-18  1:46       ` Lu Baolu
  2021-06-18  7:31         ` John Garry
  1 sibling, 1 reply; 19+ messages in thread
From: Lu Baolu @ 2021-06-18  1:46 UTC (permalink / raw)
  To: John Garry, joro, will, dwmw2, robin.murphy, corbet
  Cc: baolu.lu, linux-kernel, iommu, linuxarm, thunder.leizhen,
	chenxiang66, linux-doc

Hi John,

On 6/17/21 4:00 PM, John Garry wrote:
> On 17/06/2021 08:32, Lu Baolu wrote:
>> On 6/16/21 7:03 PM, John Garry wrote:
>>> @@ -4382,9 +4380,9 @@ int __init intel_iommu_init(void)
>>>            * is likely to be much lower than the overhead of 
>>> synchronizing
>>>            * the virtual and physical IOMMU page-tables.
>>>            */
>>> -        if (!intel_iommu_strict && cap_caching_mode(iommu->cap)) {
>>> -            pr_warn("IOMMU batching is disabled due to 
>>> virtualization");
>>> -            intel_iommu_strict = 1;
>>> +        if (cap_caching_mode(iommu->cap)) {
>>> +            pr_warn("IOMMU batching disallowed due to 
>>> virtualization\n");
>>> +            iommu_set_dma_strict(true);
>>
>> With this change, VM guest will always show this warning.
> 
> Would they have got it before also normally?
> 
> I mean, default is intel_iommu_strict=0, so if 
> cap_caching_mode(iommu->cap) is true and intel_iommu_strict not set to 1 
> elsewhere previously, then we would get this print.

Yes. You are right.

> 
>> How about
>> removing this message? Users could get the same information through the
>> kernel message added by "[PATCH v13 2/6] iommu: Print strict or lazy
>> mode at init time".
> 
> I think that the print from 2/6 should occur before this print.
> 
> Regardless I would think that you would still like to be notified of 
> this change in policy, right?
> 
> However I now realize that the print is in a loop per iommu, so we would 
> get it per iommu:
> 
> for_each_active_iommu(iommu, drhd) {
>      /*
>       * The flush queue implementation does not perform
>       * page-selective invalidations that are required for efficient
>       * TLB flushes in virtual environments.  The benefit of batching
>       * is likely to be much lower than the overhead of synchronizing
>       * the virtual and physical IOMMU page-tables.
>       */
>      if (!intel_iommu_strict && cap_caching_mode(iommu->cap)) {
>          pr_warn("IOMMU batching is disabled due to virtualization");
>          intel_iommu_strict = 1;
>      }
>      ...
> }
> 
> I need to change that. How about this:
> 
> bool print_warning = false;
> 
> for_each_active_iommu(iommu, drhd) {
>      /*
>       * The flush queue implementation does not perform
>       * page-selective invalidations that are required for efficient
>       * TLB flushes in virtual environments.  The benefit of batching
>       * is likely to be much lower than the overhead of synchronizing
>       * the virtual and physical IOMMU page-tables.
>       */
>      if (!print_warning && cap_caching_mode(iommu->cap)) {
>          pr_warn("IOMMU batching disallowed due to virtualization\n");
>          iommu_set_dma_strict(true);
>          print_warning = true;
>      }
>      ...
> }
> 
> or use pr_warn_once().

 From my p.o.v, pr_xxxx_once() is better.

How about using a pr_info_once()? I don't think it's a warning, it's
just a policy choice in VM environment.

Best regards,
baolu

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v13 6/6] iommu: Remove mode argument from iommu_set_dma_strict()
  2021-06-17 18:56     ` Robin Murphy
@ 2021-06-18  1:51       ` Lu Baolu
  0 siblings, 0 replies; 19+ messages in thread
From: Lu Baolu @ 2021-06-18  1:51 UTC (permalink / raw)
  To: Robin Murphy, John Garry, joro, will, dwmw2, corbet
  Cc: baolu.lu, linux-kernel, iommu, linuxarm, thunder.leizhen,
	chenxiang66, linux-doc

Hi Robin,

On 6/18/21 2:56 AM, Robin Murphy wrote:
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 60b1ec42e73b..ff221d3ddcbc 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -349,10 +349,9 @@ static int __init iommu_dma_setup(char *str)
>>>   }
>>>   early_param("iommu.strict", iommu_dma_setup);
>>> -void iommu_set_dma_strict(bool strict)
>>> +void iommu_set_dma_strict(void)
>>>   {
>>> -    if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>>> -        iommu_dma_strict = strict;
>>> +    iommu_dma_strict = true;
>>
>> Sorry, I still can't get how iommu.strict kernel option works.
>>
>> static int __init iommu_dma_setup(char *str)
>> {
>>          int ret = kstrtobool(str, &iommu_dma_strict);
> 
> Note that this is the bit that does the real work - if the argument 
> parses OK then iommu_dma_strict is reassigned with the appropriate 
> value. The iommu_cmd_line stuff is a bit of additional bookkeeping, 
> basically just so we can see whether default values have been overridden.

Ah, get it. Thanks a lot. I missed this part and naively thought it just
converts a string to integer.

Best regards,
baolu

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v13 6/6] iommu: Remove mode argument from iommu_set_dma_strict()
  2021-06-17  7:41     ` John Garry
@ 2021-06-18  1:52       ` Lu Baolu
  0 siblings, 0 replies; 19+ messages in thread
From: Lu Baolu @ 2021-06-18  1:52 UTC (permalink / raw)
  To: John Garry, joro, will, dwmw2, robin.murphy, corbet
  Cc: baolu.lu, linux-kernel, iommu, linuxarm, thunder.leizhen,
	chenxiang66, linux-doc

On 6/17/21 3:41 PM, John Garry wrote:
> 
>>> @@ -349,10 +349,9 @@ static int __init iommu_dma_setup(char *str)
>>>   }
>>>   early_param("iommu.strict", iommu_dma_setup);
>>> -void iommu_set_dma_strict(bool strict)
>>> +void iommu_set_dma_strict(void)
>>>   {
>>> -    if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>>> -        iommu_dma_strict = strict;
>>> +    iommu_dma_strict = true;
>>
>> Sorry, I still can't get how iommu.strict kernel option works.
>>
>> static int __init iommu_dma_setup(char *str)
>> {
>>          int ret = kstrtobool(str, &iommu_dma_strict);
>>
>>          if (!ret)
>>                  iommu_cmd_line |= IOMMU_CMD_LINE_STRICT;
>>          return ret;
>> }
>> early_param("iommu.strict", iommu_dma_setup);
>>
>> The bit IOMMU_CMD_LINE_STRICT is only set, but not used anywhere.
> 
> It is used in patch 2/6:
> 
> +    pr_info("DMA domain TLB invalidation policy: %s mode %s\n",
> +        iommu_dma_strict ? "strict" : "lazy",
> +        (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
> +            "(set via kernel command line)" : "");
> 
>> Hence,
>> I am wondering how could it work? A bug or I missed anything?
> 
> It is really just used for informative purpose now.

I am clear now. Thanks!

Best regards,
baolu

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v13 4/6] iommu/vt-d: Add support for IOMMU default DMA mode build options
  2021-06-18  1:46       ` Lu Baolu
@ 2021-06-18  7:31         ` John Garry
  0 siblings, 0 replies; 19+ messages in thread
From: John Garry @ 2021-06-18  7:31 UTC (permalink / raw)
  To: Lu Baolu, joro, will, dwmw2, robin.murphy, corbet
  Cc: linux-kernel, iommu, linuxarm, thunder.leizhen, chenxiang66, linux-doc

On 18/06/2021 02:46, Lu Baolu wrote:

Hi baolu,

>> I need to change that. How about this:
>>
>> bool print_warning = false;
>>
>> for_each_active_iommu(iommu, drhd) {
>>      /*
>>       * The flush queue implementation does not perform
>>       * page-selective invalidations that are required for efficient
>>       * TLB flushes in virtual environments.  The benefit of batching
>>       * is likely to be much lower than the overhead of synchronizing
>>       * the virtual and physical IOMMU page-tables.
>>       */
>>      if (!print_warning && cap_caching_mode(iommu->cap)) {
>>          pr_warn("IOMMU batching disallowed due to virtualization\n");
>>          iommu_set_dma_strict(true);
>>          print_warning = true;
>>      }
>>      ...
>> }
>>
>> or use pr_warn_once().
> 
>  From my p.o.v, pr_xxxx_once() is better.
> 
> How about using a pr_info_once()? I don't think it's a warning, it's
> just a policy choice in VM environment.

ok, I can go with that, which Robin mostly agrees with.

Thanks,
John

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v13 1/6] iommu: Deprecate Intel and AMD cmdline methods to enable strict mode
  2021-06-17 19:01   ` Robin Murphy
@ 2021-06-18  7:43     ` John Garry
  0 siblings, 0 replies; 19+ messages in thread
From: John Garry @ 2021-06-18  7:43 UTC (permalink / raw)
  To: Robin Murphy, joro, will, dwmw2, baolu.lu, corbet
  Cc: linux-kernel, iommu, linuxarm, thunder.leizhen, chenxiang66, linux-doc

On 17/06/2021 20:01, Robin Murphy wrote:
>>               DMA.
>> -        strict [Default Off]
>> +        strict [Default Off] [Deprecated, use iommu.strict instead]
>>               With this option on every unmap_single operation will
>>               result in a hardware IOTLB flush operation as opposed
>>               to batching them for performance.
> 
> FWIW I'd be inclined to replace both whole descriptions with just 
> something like "Deprecated, equivalent to iommu.strict=1".
> 
>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>> index 46280e6e1535..9f3096d650aa 100644
>> --- a/drivers/iommu/amd/init.c
>> +++ b/drivers/iommu/amd/init.c
>> @@ -3098,8 +3098,10 @@ static int __init parse_amd_iommu_intr(char *str)
>>   static int __init parse_amd_iommu_options(char *str)
>>   {
>>       for (; *str; ++str) {
>> -        if (strncmp(str, "fullflush", 9) == 0)
>> +        if (strncmp(str, "fullflush", 9) == 0) {
>> +            pr_warn("amd_iommu=fullflush deprecated; use iommu.strict 
>> instead\n");
> 
> Nit: maybe we should spell out "...use <option>=1 instead" in all of 
> these messages just in case anyone takes them literally?


> (I'm not sure 
> the options parse correctly with no argument)

I don't think they do.

But I'll take both suggestions on board.

> 
> Either way,
> 
> Acked-by: Robin Murphy <robin.murphy@arm.com>

Cheers!


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-06-18  7:49 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-16 11:03 [PATCH v13 0/6] iommu: Enhance IOMMU default DMA mode build options John Garry
2021-06-16 11:03 ` [PATCH v13 1/6] iommu: Deprecate Intel and AMD cmdline methods to enable strict mode John Garry
2021-06-17 19:01   ` Robin Murphy
2021-06-18  7:43     ` John Garry
2021-06-16 11:03 ` [PATCH v13 2/6] iommu: Print strict or lazy mode at init time John Garry
2021-06-16 11:03 ` [PATCH v13 3/6] iommu: Enhance IOMMU default DMA mode build options John Garry
2021-06-16 11:03 ` [PATCH v13 4/6] iommu/vt-d: Add support for " John Garry
2021-06-17  7:32   ` Lu Baolu
2021-06-17  8:00     ` John Garry
2021-06-17 19:03       ` Robin Murphy
2021-06-18  1:46       ` Lu Baolu
2021-06-18  7:31         ` John Garry
2021-06-16 11:03 ` [PATCH v13 5/6] iommu/amd: " John Garry
2021-06-16 11:03 ` [PATCH v13 6/6] iommu: Remove mode argument from iommu_set_dma_strict() John Garry
2021-06-17  7:36   ` Lu Baolu
2021-06-17  7:41     ` John Garry
2021-06-18  1:52       ` Lu Baolu
2021-06-17 18:56     ` Robin Murphy
2021-06-18  1:51       ` Lu Baolu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).