[PATCH v14 0/6] iommu: Enhance IOMMU default DMA mode build options

[PATCH v14 0/6] iommu: Enhance IOMMU default DMA mode build options

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

Based on next-20210611 + "iommu: Update "iommu.strict" documentation"

Differences to v13:
- Improve strict mode deprecation messages and cut out some
  kernel-parameters.txt legacy description
- Add tag in 1/6
- use pr_info_once() for vt-d message about VM and caching

Differences to v12:
- Add Robin's RB tags (thanks!)
- 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()

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         | 12 ++----
 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, 65 insertions(+), 37 deletions(-)

-- 
2.26.2

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

[PATCH v14 1/6] iommu: Deprecate Intel and AMD cmdline methods to enable strict mode

[John Garry]

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

Signed-off-by: John Garry <john.garry@huawei.com>
Acked-by: Robin Murphy <robin.murphy@arm.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 9 ++-------
 drivers/iommu/amd/init.c                        | 4 +++-
 drivers/iommu/intel/iommu.c                     | 1 +
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 30e9dd52464e..673952379900 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -290,10 +290,7 @@
 	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
-				    they are unmapped. Otherwise they are
-				    flushed before they will be reused, which
-				    is a lot of faster
+			fullflush - Deprecated, equivalent to iommu.strict=1
 			off	  - do not initialize any AMD IOMMU found in
 				    the system
 			force_isolation - Force device isolation for all
@@ -1948,9 +1945,7 @@
 			this case, gfx device will use physical address for
 			DMA.
 		strict [Default Off]
-			With this option on every unmap_single operation will
-			result in a hardware IOTLB flush operation as opposed
-			to batching them for performance.
+			Deprecated, equivalent to iommu.strict=1.
 		sp_off [Default Off]
 			By default, super page will be supported if Intel IOMMU
 			has the capability. With this option, super page will
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 46280e6e1535..3a2fb805f11e 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=1 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..29497113d748 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=1 instead\n");
 			pr_info("Disable batched IOTLB flush\n");
 			intel_iommu_strict = 1;
 		} else if (!strncmp(str, "sp_off", 6)) {
-- 
2.26.2

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

Re: [PATCH v14 1/6] iommu: Deprecate Intel and AMD cmdline methods to enable strict mode

[Lu Baolu]

On 2021/6/18 19:34, John Garry wrote:
> Now that the x86 drivers support iommu.strict, deprecate the custom
> methods.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> Acked-by: Robin Murphy <robin.murphy@arm.com>
> ---
>   Documentation/admin-guide/kernel-parameters.txt | 9 ++-------
>   drivers/iommu/amd/init.c                        | 4 +++-
>   drivers/iommu/intel/iommu.c                     | 1 +
>   3 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 30e9dd52464e..673952379900 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -290,10 +290,7 @@
>   	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
> -				    they are unmapped. Otherwise they are
> -				    flushed before they will be reused, which
> -				    is a lot of faster
> +			fullflush - Deprecated, equivalent to iommu.strict=1
>   			off	  - do not initialize any AMD IOMMU found in
>   				    the system
>   			force_isolation - Force device isolation for all
> @@ -1948,9 +1945,7 @@
>   			this case, gfx device will use physical address for
>   			DMA.
>   		strict [Default Off]
> -			With this option on every unmap_single operation will
> -			result in a hardware IOTLB flush operation as opposed
> -			to batching them for performance.
> +			Deprecated, equivalent to iommu.strict=1.
>   		sp_off [Default Off]
>   			By default, super page will be supported if Intel IOMMU
>   			has the capability. With this option, super page will
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 46280e6e1535..3a2fb805f11e 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=1 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..29497113d748 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=1 instead\n");
>   			pr_info("Disable batched IOTLB flush\n");
>   			intel_iommu_strict = 1;
>   		} else if (!strncmp(str, "sp_off", 6)) {
> 

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v14 2/6] iommu: Print strict or lazy mode at init time

[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

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

Re: [PATCH v14 2/6] iommu: Print strict or lazy mode at init time

[Lu Baolu]

On 2021/6/18 19:34, John Garry wrote:
> 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);
> 

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v14 3/6] iommu: Enhance IOMMU default DMA mode build options

[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 673952379900..a1b7c8526bb5 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2046,9 +2046,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

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

Re: [PATCH v14 3/6] iommu: Enhance IOMMU default DMA mode build options

[Lu Baolu]

On 2021/6/18 19:34, John Garry wrote:
> 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 673952379900..a1b7c8526bb5 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2046,9 +2046,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 {
> 

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v14 4/6] iommu/vt-d: Add support for IOMMU default DMA mode build options

[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; also reword the accompanying print
  with a level downgrade and also 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 29497113d748..06666f9d8116 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=1 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_info_once("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

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

Re: [PATCH v14 4/6] iommu/vt-d: Add support for IOMMU default DMA mode build options

[Lu Baolu]

On 2021/6/18 19:34, John Garry wrote:
> 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; also reword the accompanying print
>    with a level downgrade and also 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 29497113d748..06666f9d8116 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=1 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_info_once("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);
> 

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v14 5/6] iommu/amd: Add support for IOMMU default DMA mode build options

[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 3a2fb805f11e..1e641cb6dddc 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=1 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

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

[PATCH v14 6/6] iommu: Remove mode argument from iommu_set_dma_strict()

[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 1e641cb6dddc..6e12a615117b 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=1 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 06666f9d8116..77d0834fb0df 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=1 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_info_once("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

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

Re: [PATCH v14 6/6] iommu: Remove mode argument from iommu_set_dma_strict()

[Lu Baolu]

On 2021/6/18 19:34, 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 1e641cb6dddc..6e12a615117b 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=1 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 06666f9d8116..77d0834fb0df 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=1 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_info_once("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,
> 

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v14 6/6] iommu: Remove mode argument from iommu_set_dma_strict()

[Lu Baolu]

On 2021/6/18 19:34, John Garry 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 for this late comment.

Normally the cache invalidation policy should come from the user. We
have pre-build kernel option and also a kernel boot command iommu.strict
to override it. These seem reasonable.

We also have a helper (iommu_set_dma_strict()) so that the vendor iommu
driver could squeeze in and change the previous settings mostly due to:

a) vendor iommu driver specific kernel boot command. (We are about to
    deprecate those.)

b) quirky hardware.

c) kernel optimization (e.x. strict mode in VM environment).

a) and b) are mandatory, while c) is optional. In any instance should c)
override the flush mode specified by the user. Hence, probably we should
also have another helper like:

void iommu_set_dma_strict_optional()
{
	if (!(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
		iommu_dma_strict = true;
}

Any thoughts?

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v14 6/6] iommu: Remove mode argument from iommu_set_dma_strict()

[John Garry]

On 21/06/2021 06:17, Lu Baolu wrote:
> On 2021/6/18 19:34, John Garry 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;
>>   }
> 

Hi baolu,

> Sorry for this late comment.
>  > Normally the cache invalidation policy should come from the user. We
> have pre-build kernel option and also a kernel boot command iommu.strict
> to override it. These seem reasonable.
> 
> We also have a helper (iommu_set_dma_strict()) so that the vendor iommu
> driver could squeeze in and change the previous settings mostly due to:
> 
> a) vendor iommu driver specific kernel boot command. (We are about to
>     deprecate those.)
> 
> b) quirky hardware.
> 
> c) kernel optimization (e.x. strict mode in VM environment).
> 
> a) and b) are mandatory, while c) is optional. In any instance should c)
> override the flush mode specified by the user. Hence, probably we should
> also have another helper like:
> 
> void iommu_set_dma_strict_optional()
> {
>      if (!(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>          iommu_dma_strict = true;
> }
> 
> Any thoughts?

What you are suggesting is a change in policy from mainline code. 
Currently for c) we always set strict enabled, regardless of any user 
cmdline input. But now you are saying that you want iommu.strict to 
override in particular scenario, right?

In that case I would think it's better to rework the current API, like 
adding an option to "force" strict mode:

void iommu_set_dma_strict(bool force)
{
      	if (force == true)
		iommu_dma_strict = true;
	else if (!(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
		iommu_dma_strict = true;
}

So we would use iommu_set_dma_strict(true) for a) and b), but 
iommu_set_dma_strict(false) for c).

Then I am not sure what you want to do with the accompanying print for 
c). It was:
"IOMMU batching is disabled due to virtualization"

And now is from this series:
"IOMMU batching disallowed due to virtualization"

Using iommu_get_dma_strict(domain) is not appropriate here to know the 
current mode (so we know whether to print).

Note that this change would mean that the current series would require 
non-trivial rework, which would be unfortunate so late in the cycle.

Thanks,
John
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v14 6/6] iommu: Remove mode argument from iommu_set_dma_strict()

[Lu Baolu]

On 2021/6/21 16:12, John Garry wrote:
> On 21/06/2021 06:17, Lu Baolu wrote:
>> On 2021/6/18 19:34, John Garry 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;
>>>   }
>>
> 
> Hi baolu,

Hi John,

> 
>> Sorry for this late comment.
>>  > Normally the cache invalidation policy should come from the user. We
>> have pre-build kernel option and also a kernel boot command iommu.strict
>> to override it. These seem reasonable.
>>
>> We also have a helper (iommu_set_dma_strict()) so that the vendor iommu
>> driver could squeeze in and change the previous settings mostly due to:
>>
>> a) vendor iommu driver specific kernel boot command. (We are about to
>>     deprecate those.)
>>
>> b) quirky hardware.
>>
>> c) kernel optimization (e.x. strict mode in VM environment).
>>
>> a) and b) are mandatory, while c) is optional. In any instance should c)
>> override the flush mode specified by the user. Hence, probably we should
>> also have another helper like:
>>
>> void iommu_set_dma_strict_optional()
>> {
>>      if (!(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>>          iommu_dma_strict = true;
>> }
>>
>> Any thoughts?
> 
> What you are suggesting is a change in policy from mainline code. 
> Currently for c) we always set strict enabled, regardless of any user 
> cmdline input. But now you are saying that you want iommu.strict to 
> override in particular scenario, right?
> 
> In that case I would think it's better to rework the current API, like 
> adding an option to "force" strict mode:
> 
> void iommu_set_dma_strict(bool force)
> {
>           if (force == true)
>          iommu_dma_strict = true;
>      else if (!(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>          iommu_dma_strict = true;
> }
> 
> So we would use iommu_set_dma_strict(true) for a) and b), but 
> iommu_set_dma_strict(false) for c).

Yes. We need to distinguish the "must" and "nice-to-have" cases of
setting strict mode.

> 
> Then I am not sure what you want to do with the accompanying print for 
> c). It was:
> "IOMMU batching is disabled due to virtualization"
> 
> And now is from this series:
> "IOMMU batching disallowed due to virtualization"
> 
> Using iommu_get_dma_strict(domain) is not appropriate here to know the 
> current mode (so we know whether to print).
> 
> Note that this change would mean that the current series would require 
> non-trivial rework, which would be unfortunate so late in the cycle.

This patch series looks good to me and I have added by reviewed-by.
Probably we could make another patch series to improve it so that the
kernel optimization should not override the user setting.

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v14 6/6] iommu: Remove mode argument from iommu_set_dma_strict()

[John Garry]

On 21/06/2021 11:00, Lu Baolu wrote:
>> void iommu_set_dma_strict(bool force)
>> {
>>           if (force == true)
>>          iommu_dma_strict = true;
>>      else if (!(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>>          iommu_dma_strict = true;
>> }
>>
>> So we would use iommu_set_dma_strict(true) for a) and b), but 
>> iommu_set_dma_strict(false) for c).
> 
> Yes. We need to distinguish the "must" and "nice-to-have" cases of
> setting strict mode.
> 
>>
>> Then I am not sure what you want to do with the accompanying print for 
>> c). It was:
>> "IOMMU batching is disabled due to virtualization"
>>
>> And now is from this series:
>> "IOMMU batching disallowed due to virtualization"
>>
>> Using iommu_get_dma_strict(domain) is not appropriate here to know the 
>> current mode (so we know whether to print).
>>
>> Note that this change would mean that the current series would require 
>> non-trivial rework, which would be unfortunate so late in the cycle.
> 
> This patch series looks good to me and I have added by reviewed-by.
> Probably we could make another patch series to improve it so that the
> kernel optimization should not override the user setting.

On a personal level I would be happy with that approach, but I think 
it's better to not start changing things right away in a follow-up series.

So how about we add this patch (which replaces 6/6 "iommu: Remove mode 
argument from iommu_set_dma_strict()")?

Robin, any opinion?

------->8---------

[PATCH] iommu/vt-d: Make "iommu.strict" override batching due to
  virtualization

As a change in policy, make iommu.strict cmdline argument override 
whether we disable batching due to virtualization.

The API of iommu_set_dma_strict() is changed to accept a "force" 
argument, which means that we always set iommu_dma_strict true, 
regardless of whether we already set via cmdline. Also return a boolean, 
to tell whether iommu_dma_strict was set or not.

Note that in all pre-existing callsites of iommu_set_dma_strict(), 
argument strict was true, so this argument is dropped.

Signed-off-by: John Garry <john.garry@huawei.com>

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 06666f9d8116..e8d65239b359 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4380,10 +4380,8 @@ 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 (cap_caching_mode(iommu->cap)) {
+		if (cap_caching_mode(iommu->cap) && iommu_set_dma_strict(false))
  			pr_info_once("IOMMU batching disallowed due to virtualization\n");
-			iommu_set_dma_strict(true);
-		}
  		iommu_device_sysfs_add(&iommu->iommu, NULL,
  				       intel_iommu_groups,
  				       "%s", iommu->name);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 60b1ec42e73b..1434bee64af3 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -349,10 +349,14 @@ static int __init iommu_dma_setup(char *str)
  }
  early_param("iommu.strict", iommu_dma_setup);

-void iommu_set_dma_strict(bool strict)
+/* Return true if we set iommu_dma_strict */
+bool iommu_set_dma_strict(bool force)
  {
-	if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
-		iommu_dma_strict = strict;
+	if (force || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT)) {
+		iommu_dma_strict = true;
+		return true;
+	}
+	return false;
  }

  bool iommu_get_dma_strict(struct iommu_domain *domain)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 32d448050bf7..f17b20234296 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);
+bool iommu_set_dma_strict(bool force);
  bool iommu_get_dma_strict(struct iommu_domain *domain);

  extern int report_iommu_fault(struct iommu_domain *domain, struct 
device *dev,
-- 
2.26.2
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v14 6/6] iommu: Remove mode argument from iommu_set_dma_strict()

[Robin Murphy]

On 2021-06-21 11:34, John Garry wrote:
> On 21/06/2021 11:00, Lu Baolu wrote:
>>> void iommu_set_dma_strict(bool force)
>>> {
>>>           if (force == true)
>>>          iommu_dma_strict = true;
>>>      else if (!(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>>>          iommu_dma_strict = true;
>>> }
>>>
>>> So we would use iommu_set_dma_strict(true) for a) and b), but 
>>> iommu_set_dma_strict(false) for c).
>>
>> Yes. We need to distinguish the "must" and "nice-to-have" cases of
>> setting strict mode.
>>
>>>
>>> Then I am not sure what you want to do with the accompanying print 
>>> for c). It was:
>>> "IOMMU batching is disabled due to virtualization"
>>>
>>> And now is from this series:
>>> "IOMMU batching disallowed due to virtualization"
>>>
>>> Using iommu_get_dma_strict(domain) is not appropriate here to know 
>>> the current mode (so we know whether to print).
>>>
>>> Note that this change would mean that the current series would 
>>> require non-trivial rework, which would be unfortunate so late in the 
>>> cycle.
>>
>> This patch series looks good to me and I have added by reviewed-by.
>> Probably we could make another patch series to improve it so that the
>> kernel optimization should not override the user setting.
> 
> On a personal level I would be happy with that approach, but I think 
> it's better to not start changing things right away in a follow-up series.
> 
> So how about we add this patch (which replaces 6/6 "iommu: Remove mode 
> argument from iommu_set_dma_strict()")?
> 
> Robin, any opinion?

For me it boils down to whether there are any realistic workloads where 
non-strict mode *would* still perform better under virtualisation. The 
only reason for the user to explicitly pass "iommu.strict=0" is because 
they expect it to increase unmap performance; if it's only ever going to 
lead to an unexpected performance loss, I don't see any value in 
overriding the kernel's decision purely for the sake of subservience.

If there *are* certain valid cases for allowing it for people who really 
know what they're doing, then we should arguably also log a counterpart 
message to say "we're honouring your override but beware it may have the 
opposite effect to what you expect" for the benefit of other users who 
assume it's a generic go-faster knob. At that point it starts getting 
non-trivial enough that I'd want to know for sure it's worthwhile.

The other reason this might be better to revisit later is that an AMD 
equivalent is still in flight[1], and there might be more that can 
eventually be factored out. I think both series are pretty much good to 
merge for 5.14, but time's already tight to sort out the conflicts which 
exist as-is, without making them any worse.

Robin.

[1] 
https://lore.kernel.org/linux-iommu/20210616100500.174507-3-namit@vmware.com/

> 
> ------->8---------
> 
> [PATCH] iommu/vt-d: Make "iommu.strict" override batching due to
>   virtualization
> 
> As a change in policy, make iommu.strict cmdline argument override 
> whether we disable batching due to virtualization.
> 
> The API of iommu_set_dma_strict() is changed to accept a "force" 
> argument, which means that we always set iommu_dma_strict true, 
> regardless of whether we already set via cmdline. Also return a boolean, 
> to tell whether iommu_dma_strict was set or not.
> 
> Note that in all pre-existing callsites of iommu_set_dma_strict(), 
> argument strict was true, so this argument is dropped.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 06666f9d8116..e8d65239b359 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4380,10 +4380,8 @@ 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 (cap_caching_mode(iommu->cap)) {
> +        if (cap_caching_mode(iommu->cap) && iommu_set_dma_strict(false))
>               pr_info_once("IOMMU batching disallowed due to 
> virtualization\n");
> -            iommu_set_dma_strict(true);
> -        }
>           iommu_device_sysfs_add(&iommu->iommu, NULL,
>                          intel_iommu_groups,
>                          "%s", iommu->name);
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 60b1ec42e73b..1434bee64af3 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -349,10 +349,14 @@ static int __init iommu_dma_setup(char *str)
>   }
>   early_param("iommu.strict", iommu_dma_setup);
> 
> -void iommu_set_dma_strict(bool strict)
> +/* Return true if we set iommu_dma_strict */
> +bool iommu_set_dma_strict(bool force)
>   {
> -    if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
> -        iommu_dma_strict = strict;
> +    if (force || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT)) {
> +        iommu_dma_strict = true;
> +        return true;
> +    }
> +    return false;
>   }
> 
>   bool iommu_get_dma_strict(struct iommu_domain *domain)
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 32d448050bf7..f17b20234296 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);
> +bool iommu_set_dma_strict(bool force);
>   bool iommu_get_dma_strict(struct iommu_domain *domain);
> 
>   extern int report_iommu_fault(struct iommu_domain *domain, struct 
> device *dev,
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v14 6/6] iommu: Remove mode argument from iommu_set_dma_strict()

[John Garry]

On 21/06/2021 12:59, Robin Murphy wrote:

+ Nadav

>> On a personal level I would be happy with that approach, but I think
>> it's better to not start changing things right away in a follow-up series.
>>
>> So how about we add this patch (which replaces 6/6 "iommu: Remove mode
>> argument from iommu_set_dma_strict()")?
>>
>> Robin, any opinion?
> For me it boils down to whether there are any realistic workloads where
> non-strict mode*would*  still perform better under virtualisation. The
> only reason for the user to explicitly pass "iommu.strict=0" is because
> they expect it to increase unmap performance; if it's only ever going to
> lead to an unexpected performance loss, I don't see any value in
> overriding the kernel's decision purely for the sake of subservience.
> 
> If there*are*  certain valid cases for allowing it for people who really
> know what they're doing, then we should arguably also log a counterpart
> message to say "we're honouring your override but beware it may have the
> opposite effect to what you expect" for the benefit of other users who
> assume it's a generic go-faster knob. At that point it starts getting
> non-trivial enough that I'd want to know for sure it's worthwhile.
> 
> The other reason this might be better to revisit later is that an AMD
> equivalent is still in flight[1], and there might be more that can
> eventually be factored out. I think both series are pretty much good to
> merge for 5.14, but time's already tight to sort out the conflicts which
> exist as-is, without making them any worse.

ok, fine. Can revisit.

As for getting these merged, I'll dry-run merging both of those series 
to see the conflicts. It doesn't look too problematic from a glance.

Cheers,
John

> 
> Robin.
> 
> [1]
> https://lore.kernel.org/linux-iommu/20210616100500.174507-3-namit@vmware.com/
> 
>> ------->8---------
>>
>> [PATCH] iommu/vt-d: Make "iommu.strict" override batching due to
>>    virtualization
>>
>> As a change in policy, make iommu.strict cmdline argument override
>> whether we disable batching due to virtualization.
>>
>> The API of iommu_set_dma_strict() is changed to accept a "force"
>> argument, which means that we always set iommu_dma_strict true,
>> regardless of whether we already set via cmdline. Also return a boolean,
>> to tell whether iommu_dma_strict was set or not.
>>
>> Note that in all pre-existing callsites of iommu_set_dma_strict(),
>> argument strict was true, so this argument is dropped.
>>
>> Signed-off-by: John Garry<john.garry@huawei.com>
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 06666f9d8116..e8d65239b359 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -4380,10 +4380,8 @@ 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 (cap_caching_mode(iommu->cap)) {
>> +        if (cap_caching_mode(iommu->cap) && iommu_set_dma_strict(false))
>>                pr_info_once("IOMMU batching disallowed due to
>> virtualization\n");
>> -            iommu_set_dma_strict(true);
>> -        }
>>            iommu_device_sysfs_add(&iommu->iommu, NULL,
>>                           intel_iommu_groups,
>>                           "%s", iommu->name);
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 60b1ec42e73b..1434bee64af3 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -349,10 +349,14 @@ static int __init iommu_dma_setup(char *str)
>>    }
>>    early_param("iommu.strict", iommu_dma_setup);
>>
>> -void iommu_set_dma_strict(bool strict)
>> +/* Return true if we set iommu_dma_strict */
>> +bool iommu_set_dma_strict(bool force)
>>    {
>> -    if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>> -        iommu_dma_strict = strict;
>> +    if (force || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT)) {
>> +        iommu_dma_strict = true;
>> +        return true;
>> +    }
>> +    return false;
>>    }
>>
>>    bool iommu_get_dma_strict(struct iommu_domain *domain)
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 32d448050bf7..f17b20234296 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);
>> +bool iommu_set_dma_strict(bool force);
>>    bool iommu_get_dma_strict(struct iommu_domain *domain);
>>
>>    extern int report_iommu_fault(struct iommu_domain *domain, struct
>> device *dev,

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

Re: [PATCH v14 6/6] iommu: Remove mode argument from iommu_set_dma_strict()

[Lu Baolu]

Hi Robin,

On 2021/6/21 19:59, Robin Murphy wrote:
> On 2021-06-21 11:34, John Garry wrote:
>> On 21/06/2021 11:00, Lu Baolu wrote:
>>>> void iommu_set_dma_strict(bool force)
>>>> {
>>>>           if (force == true)
>>>>          iommu_dma_strict = true;
>>>>      else if (!(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>>>>          iommu_dma_strict = true;
>>>> }
>>>>
>>>> So we would use iommu_set_dma_strict(true) for a) and b), but 
>>>> iommu_set_dma_strict(false) for c).
>>>
>>> Yes. We need to distinguish the "must" and "nice-to-have" cases of
>>> setting strict mode.
>>>
>>>>
>>>> Then I am not sure what you want to do with the accompanying print 
>>>> for c). It was:
>>>> "IOMMU batching is disabled due to virtualization"
>>>>
>>>> And now is from this series:
>>>> "IOMMU batching disallowed due to virtualization"
>>>>
>>>> Using iommu_get_dma_strict(domain) is not appropriate here to know 
>>>> the current mode (so we know whether to print).
>>>>
>>>> Note that this change would mean that the current series would 
>>>> require non-trivial rework, which would be unfortunate so late in 
>>>> the cycle.
>>>
>>> This patch series looks good to me and I have added by reviewed-by.
>>> Probably we could make another patch series to improve it so that the
>>> kernel optimization should not override the user setting.
>>
>> On a personal level I would be happy with that approach, but I think 
>> it's better to not start changing things right away in a follow-up 
>> series.
>>
>> So how about we add this patch (which replaces 6/6 "iommu: Remove mode 
>> argument from iommu_set_dma_strict()")?
>>
>> Robin, any opinion?
> 
> For me it boils down to whether there are any realistic workloads where 
> non-strict mode *would* still perform better under virtualisation. The 

At present, we see that strict mode has better performance in the
virtualization environment because it will make the shadow page table
management more efficient. When the hardware supports nested
translation, we may have to re-evaluate this since there's no need for
a shadowing page table anymore.

> only reason for the user to explicitly pass "iommu.strict=0" is because 
> they expect it to increase unmap performance; if it's only ever going to 
> lead to an unexpected performance loss, I don't see any value in 
> overriding the kernel's decision purely for the sake of subservience.
> 
> If there *are* certain valid cases for allowing it for people who really 
> know what they're doing, then we should arguably also log a counterpart 
> message to say "we're honouring your override but beware it may have the 
> opposite effect to what you expect" for the benefit of other users who 
> assume it's a generic go-faster knob. At that point it starts getting 
> non-trivial enough that I'd want to know for sure it's worthwhile.
> 
> The other reason this might be better to revisit later is that an AMD 
> equivalent is still in flight[1], and there might be more that can 
> eventually be factored out. I think both series are pretty much good to 
> merge for 5.14, but time's already tight to sort out the conflicts which 
> exist as-is, without making them any worse.

Agreed. We could revisit it later.

Best regards,
baolu


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

Re: [PATCH v14 6/6] iommu: Remove mode argument from iommu_set_dma_strict()

[Robin Murphy]

On 2021-06-21 15:32, Lu Baolu wrote:
> Hi Robin,
> 
> On 2021/6/21 19:59, Robin Murphy wrote:
>> On 2021-06-21 11:34, John Garry wrote:
>>> On 21/06/2021 11:00, Lu Baolu wrote:
>>>>> void iommu_set_dma_strict(bool force)
>>>>> {
>>>>>           if (force == true)
>>>>>          iommu_dma_strict = true;
>>>>>      else if (!(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>>>>>          iommu_dma_strict = true;
>>>>> }
>>>>>
>>>>> So we would use iommu_set_dma_strict(true) for a) and b), but 
>>>>> iommu_set_dma_strict(false) for c).
>>>>
>>>> Yes. We need to distinguish the "must" and "nice-to-have" cases of
>>>> setting strict mode.
>>>>
>>>>>
>>>>> Then I am not sure what you want to do with the accompanying print 
>>>>> for c). It was:
>>>>> "IOMMU batching is disabled due to virtualization"
>>>>>
>>>>> And now is from this series:
>>>>> "IOMMU batching disallowed due to virtualization"
>>>>>
>>>>> Using iommu_get_dma_strict(domain) is not appropriate here to know 
>>>>> the current mode (so we know whether to print).
>>>>>
>>>>> Note that this change would mean that the current series would 
>>>>> require non-trivial rework, which would be unfortunate so late in 
>>>>> the cycle.
>>>>
>>>> This patch series looks good to me and I have added by reviewed-by.
>>>> Probably we could make another patch series to improve it so that the
>>>> kernel optimization should not override the user setting.
>>>
>>> On a personal level I would be happy with that approach, but I think 
>>> it's better to not start changing things right away in a follow-up 
>>> series.
>>>
>>> So how about we add this patch (which replaces 6/6 "iommu: Remove 
>>> mode argument from iommu_set_dma_strict()")?
>>>
>>> Robin, any opinion?
>>
>> For me it boils down to whether there are any realistic workloads 
>> where non-strict mode *would* still perform better under 
>> virtualisation. The 
> 
> At present, we see that strict mode has better performance in the
> virtualization environment because it will make the shadow page table
> management more efficient. When the hardware supports nested
> translation, we may have to re-evaluate this since there's no need for
> a shadowing page table anymore.

I guess I was assuming that in most cases, proper nested mode could look 
distinct enough that we'd be able to treat it differently in the first 
place. For instance, if it's handing guest tables directly to the 
hardware, would the host have any reason to still set the "caching mode" 
ID bit?

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

Re: [PATCH v14 6/6] iommu: Remove mode argument from iommu_set_dma_strict()

[Lu Baolu]

On 6/23/21 6:25 AM, Robin Murphy wrote:
> On 2021-06-21 15:32, Lu Baolu wrote:
>> Hi Robin,
>>
>> On 2021/6/21 19:59, Robin Murphy wrote:
>>> On 2021-06-21 11:34, John Garry wrote:
>>>> On 21/06/2021 11:00, Lu Baolu wrote:
>>>>>> void iommu_set_dma_strict(bool force)
>>>>>> {
>>>>>>           if (force == true)
>>>>>>          iommu_dma_strict = true;
>>>>>>      else if (!(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>>>>>>          iommu_dma_strict = true;
>>>>>> }
>>>>>>
>>>>>> So we would use iommu_set_dma_strict(true) for a) and b), but 
>>>>>> iommu_set_dma_strict(false) for c).
>>>>>
>>>>> Yes. We need to distinguish the "must" and "nice-to-have" cases of
>>>>> setting strict mode.
>>>>>
>>>>>>
>>>>>> Then I am not sure what you want to do with the accompanying print 
>>>>>> for c). It was:
>>>>>> "IOMMU batching is disabled due to virtualization"
>>>>>>
>>>>>> And now is from this series:
>>>>>> "IOMMU batching disallowed due to virtualization"
>>>>>>
>>>>>> Using iommu_get_dma_strict(domain) is not appropriate here to know 
>>>>>> the current mode (so we know whether to print).
>>>>>>
>>>>>> Note that this change would mean that the current series would 
>>>>>> require non-trivial rework, which would be unfortunate so late in 
>>>>>> the cycle.
>>>>>
>>>>> This patch series looks good to me and I have added by reviewed-by.
>>>>> Probably we could make another patch series to improve it so that the
>>>>> kernel optimization should not override the user setting.
>>>>
>>>> On a personal level I would be happy with that approach, but I think 
>>>> it's better to not start changing things right away in a follow-up 
>>>> series.
>>>>
>>>> So how about we add this patch (which replaces 6/6 "iommu: Remove 
>>>> mode argument from iommu_set_dma_strict()")?
>>>>
>>>> Robin, any opinion?
>>>
>>> For me it boils down to whether there are any realistic workloads 
>>> where non-strict mode *would* still perform better under 
>>> virtualisation. The 
>>
>> At present, we see that strict mode has better performance in the
>> virtualization environment because it will make the shadow page table
>> management more efficient. When the hardware supports nested
>> translation, we may have to re-evaluate this since there's no need for
>> a shadowing page table anymore.
> 
> I guess I was assuming that in most cases, proper nested mode could look 
> distinct enough that we'd be able to treat it differently in the first 
> place. For instance, if it's handing guest tables directly to the 
> hardware, would the host have any reason to still set the "caching mode" 
> ID bit?

For Intel VT-d, yes, simply for compatible purpose. The guest kernel
may use page tables that are not compatible with the first level
translation. In this case, we must roll back to shadow page table.

> 
> Robin.

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v14 0/6] iommu: Enhance IOMMU default DMA mode build options

[John Garry]

On 18/06/2021 12:34, John Garry wrote:
> 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.
> 
> Based on next-20210611 + "iommu: Update "iommu.strict" documentation"

Hi Joerg, Will,

We think that this series is ready to go.

There would be a build conflict with the following:
https://lore.kernel.org/linux-iommu/20210616100500.174507-1-namit@vmware.com/

So please let us know where you stand on it, so that could be resolved.

Robin and Baolu have kindly reviewed all the patches, apart from the AMD 
one.

Thanks,
John
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v14 0/6] iommu: Enhance IOMMU default DMA mode build options

[joro]

Hi John,

On Fri, Jun 25, 2021 at 05:41:09PM +0100, John Garry wrote:
> We think that this series is ready to go.
> 
> There would be a build conflict with the following:
> https://lore.kernel.org/linux-iommu/20210616100500.174507-1-namit@vmware.com/
> 
> So please let us know where you stand on it, so that could be resolved.
> 
> Robin and Baolu have kindly reviewed all the patches, apart from the AMD
> one.

The AMD one also looks good to me, please re-send after the merge window
closes and I will take care of it then. Note that I usually start
merging new stuff after -rc3 is out.

Regards,

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