[v7,4/6] iommu: Add bootup option "iommu.non_strict"

Message ID 41f81c46348db4acd9a542184f10e7ca24f6c219.1536935328.git.robin.murphy@arm.com
State New
Headers show
Series
  • [v7,1/6] iommu/arm-smmu-v3: Implement flush_iotlb_all hook
Related show

Commit Message

Robin Murphy Sept. 14, 2018, 2:30 p.m.
From: Zhen Lei <thunder.leizhen@huawei.com>


Add a bootup option to make the system manager can choose which mode to
be used. The default mode is strict.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

[rm: move handling out of SMMUv3 driver]
Signed-off-by: Robin Murphy <robin.murphy@arm.com>

---
 .../admin-guide/kernel-parameters.txt         | 13 ++++++++++
 drivers/iommu/iommu.c                         | 26 +++++++++++++++++++
 2 files changed, 39 insertions(+)

-- 
2.19.0.dirty

Comments

Will Deacon Sept. 18, 2018, 5:10 p.m. | #1
On Fri, Sep 14, 2018 at 03:30:22PM +0100, Robin Murphy wrote:
> From: Zhen Lei <thunder.leizhen@huawei.com>

> 

> Add a bootup option to make the system manager can choose which mode to

> be used. The default mode is strict.

> 

> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

> [rm: move handling out of SMMUv3 driver]

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

> ---

>  .../admin-guide/kernel-parameters.txt         | 13 ++++++++++

>  drivers/iommu/iommu.c                         | 26 +++++++++++++++++++

>  2 files changed, 39 insertions(+)

> 

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt

> index 9871e649ffef..406b91759b62 100644

> --- a/Documentation/admin-guide/kernel-parameters.txt

> +++ b/Documentation/admin-guide/kernel-parameters.txt

> @@ -1749,6 +1749,19 @@

>  		nobypass	[PPC/POWERNV]

>  			Disable IOMMU bypass, using IOMMU for PCI devices.

>  

> +	iommu.non_strict=	[ARM64]

> +			Format: { "0" | "1" }

> +			0 - strict mode, default.

> +			    Release IOVAs after the related TLBs are invalid

> +			    completely.

> +			1 - non-strict mode.

> +			    Put off TLBs invalidation and release memory first.

> +			    It's good for scatter-gather performance but lacks

> +			    full isolation, an untrusted device can access the

> +			    reused memory because the TLBs may still valid.

> +			    Please take	full consideration before choosing this

> +			    mode. Note that, VFIO will always use strict mode.


This text needs help. How about something like:

	0 - strict mode, default.
	    Invalidate the TLB of the IOMMU hardware as part of every
	    unmap() operation.
	1 - lazy mode.
	    Defer TLB invalidation so that the TLB of the IOMMU hardware
	    is invalidated periodically, rather than as part of every
	    unmap() operation.

(generally, I think I'd s/non strict/lazy/ in this patch to avoid the double
negatives)

> +

>  	iommu.passthrough=

>  			[ARM64] Configure DMA to bypass the IOMMU by default.

>  			Format: { "0" | "1" }

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c

> index 8c15c5980299..2cabd0c0a4f3 100644

> --- a/drivers/iommu/iommu.c

> +++ b/drivers/iommu/iommu.c

> @@ -41,6 +41,7 @@ static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;

>  #else

>  static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;

>  #endif

> +static bool iommu_dma_non_strict __read_mostly;

>  

>  struct iommu_callback_data {

>  	const struct iommu_ops *ops;

> @@ -131,6 +132,24 @@ static int __init iommu_set_def_domain_type(char *str)

>  }

>  early_param("iommu.passthrough", iommu_set_def_domain_type);

>  

> +static int __init iommu_dma_setup(char *str)

> +{

> +	int ret;

> +

> +	ret = kstrtobool(str, &iommu_dma_non_strict);

> +	if (ret)

> +		return ret;

> +

> +	if (iommu_dma_non_strict) {

> +		pr_warn("WARNING: iommu non-strict mode is chosen.\n"

> +			"It's good for scatter-gather performance but lacks full isolation\n");


Hmm, not sure about this message either and tainting is probably over the
top. Maybe drop the taint and just pr_info something like "IOMMU DMA ops
using lazy TLB invalidation: unable to protect against malicious devices"

> +		add_taint(TAINT_WARN, LOCKDEP_STILL_OK);

> +	}

> +

> +	return 0;

> +}

> +early_param("iommu.non_strict", iommu_dma_setup);

> +

>  static ssize_t iommu_group_attr_show(struct kobject *kobj,

>  				     struct attribute *__attr, char *buf)

>  {

> @@ -1072,6 +1091,13 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)

>  		group->default_domain = dom;

>  		if (!group->domain)

>  			group->domain = dom;

> +

> +		if (dom && iommu_dma_non_strict) {

> +			int attr = 1;

> +			iommu_domain_set_attr(dom,

> +					      DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,

> +					      &attr);

> +		}


Hmm, I don't think we can guarantee that we're working with the DMA domain
here. Does this all fall out in the wash for the identity domain?

Will
Robin Murphy Sept. 18, 2018, 7:01 p.m. | #2
On 2018-09-18 6:10 PM, Will Deacon wrote:
> On Fri, Sep 14, 2018 at 03:30:22PM +0100, Robin Murphy wrote:

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

>>

>> Add a bootup option to make the system manager can choose which mode to

>> be used. The default mode is strict.

>>

>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

>> [rm: move handling out of SMMUv3 driver]

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

>> ---

>>   .../admin-guide/kernel-parameters.txt         | 13 ++++++++++

>>   drivers/iommu/iommu.c                         | 26 +++++++++++++++++++

>>   2 files changed, 39 insertions(+)

>>

>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt

>> index 9871e649ffef..406b91759b62 100644

>> --- a/Documentation/admin-guide/kernel-parameters.txt

>> +++ b/Documentation/admin-guide/kernel-parameters.txt

>> @@ -1749,6 +1749,19 @@

>>   		nobypass	[PPC/POWERNV]

>>   			Disable IOMMU bypass, using IOMMU for PCI devices.

>>   

>> +	iommu.non_strict=	[ARM64]

>> +			Format: { "0" | "1" }

>> +			0 - strict mode, default.

>> +			    Release IOVAs after the related TLBs are invalid

>> +			    completely.

>> +			1 - non-strict mode.

>> +			    Put off TLBs invalidation and release memory first.

>> +			    It's good for scatter-gather performance but lacks

>> +			    full isolation, an untrusted device can access the

>> +			    reused memory because the TLBs may still valid.

>> +			    Please take	full consideration before choosing this

>> +			    mode. Note that, VFIO will always use strict mode.

> 

> This text needs help. How about something like:

> 

> 	0 - strict mode, default.

> 	    Invalidate the TLB of the IOMMU hardware as part of every

> 	    unmap() operation.

> 	1 - lazy mode.

> 	    Defer TLB invalidation so that the TLB of the IOMMU hardware

> 	    is invalidated periodically, rather than as part of every

> 	    unmap() operation.

> 

> (generally, I think I'd s/non strict/lazy/ in this patch to avoid the double

> negatives)

> 

>> +

>>   	iommu.passthrough=

>>   			[ARM64] Configure DMA to bypass the IOMMU by default.

>>   			Format: { "0" | "1" }

>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c

>> index 8c15c5980299..2cabd0c0a4f3 100644

>> --- a/drivers/iommu/iommu.c

>> +++ b/drivers/iommu/iommu.c

>> @@ -41,6 +41,7 @@ static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;

>>   #else

>>   static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;

>>   #endif

>> +static bool iommu_dma_non_strict __read_mostly;

>>   

>>   struct iommu_callback_data {

>>   	const struct iommu_ops *ops;

>> @@ -131,6 +132,24 @@ static int __init iommu_set_def_domain_type(char *str)

>>   }

>>   early_param("iommu.passthrough", iommu_set_def_domain_type);

>>   

>> +static int __init iommu_dma_setup(char *str)

>> +{

>> +	int ret;

>> +

>> +	ret = kstrtobool(str, &iommu_dma_non_strict);

>> +	if (ret)

>> +		return ret;

>> +

>> +	if (iommu_dma_non_strict) {

>> +		pr_warn("WARNING: iommu non-strict mode is chosen.\n"

>> +			"It's good for scatter-gather performance but lacks full isolation\n");

> 

> Hmm, not sure about this message either and tainting is probably over the

> top. Maybe drop the taint and just pr_info something like "IOMMU DMA ops

> using lazy TLB invalidation: unable to protect against malicious devices"

> 

>> +		add_taint(TAINT_WARN, LOCKDEP_STILL_OK);

>> +	}

>> +

>> +	return 0;

>> +}

>> +early_param("iommu.non_strict", iommu_dma_setup);

>> +

>>   static ssize_t iommu_group_attr_show(struct kobject *kobj,

>>   				     struct attribute *__attr, char *buf)

>>   {

>> @@ -1072,6 +1091,13 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)

>>   		group->default_domain = dom;

>>   		if (!group->domain)

>>   			group->domain = dom;

>> +

>> +		if (dom && iommu_dma_non_strict) {

>> +			int attr = 1;

>> +			iommu_domain_set_attr(dom,

>> +					      DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,

>> +					      &attr);

>> +		}

> 

> Hmm, I don't think we can guarantee that we're working with the DMA domain

> here. Does this all fall out in the wash for the identity domain?


Indeed so - for one, I expect drivers to reject it for anything that 
isn't their own default DMA ops domain type (as #5 and #6 do), and 
furthermore it only has any effect once iommu_dma_init_domain() reads it 
back if it stuck, and other domain types should never be getting passed 
into there anyway.

Robin.

Patch

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 9871e649ffef..406b91759b62 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1749,6 +1749,19 @@ 
 		nobypass	[PPC/POWERNV]
 			Disable IOMMU bypass, using IOMMU for PCI devices.
 
+	iommu.non_strict=	[ARM64]
+			Format: { "0" | "1" }
+			0 - strict mode, default.
+			    Release IOVAs after the related TLBs are invalid
+			    completely.
+			1 - non-strict mode.
+			    Put off TLBs invalidation and release memory first.
+			    It's good for scatter-gather performance but lacks
+			    full isolation, an untrusted device can access the
+			    reused memory because the TLBs may still valid.
+			    Please take	full consideration before choosing this
+			    mode. Note that, VFIO will always use strict mode.
+
 	iommu.passthrough=
 			[ARM64] Configure DMA to bypass the IOMMU by default.
 			Format: { "0" | "1" }
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8c15c5980299..2cabd0c0a4f3 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -41,6 +41,7 @@  static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;
 #else
 static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;
 #endif
+static bool iommu_dma_non_strict __read_mostly;
 
 struct iommu_callback_data {
 	const struct iommu_ops *ops;
@@ -131,6 +132,24 @@  static int __init iommu_set_def_domain_type(char *str)
 }
 early_param("iommu.passthrough", iommu_set_def_domain_type);
 
+static int __init iommu_dma_setup(char *str)
+{
+	int ret;
+
+	ret = kstrtobool(str, &iommu_dma_non_strict);
+	if (ret)
+		return ret;
+
+	if (iommu_dma_non_strict) {
+		pr_warn("WARNING: iommu non-strict mode is chosen.\n"
+			"It's good for scatter-gather performance but lacks full isolation\n");
+		add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
+	}
+
+	return 0;
+}
+early_param("iommu.non_strict", iommu_dma_setup);
+
 static ssize_t iommu_group_attr_show(struct kobject *kobj,
 				     struct attribute *__attr, char *buf)
 {
@@ -1072,6 +1091,13 @@  struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 		group->default_domain = dom;
 		if (!group->domain)
 			group->domain = dom;
+
+		if (dom && iommu_dma_non_strict) {
+			int attr = 1;
+			iommu_domain_set_attr(dom,
+					      DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
+					      &attr);
+		}
 	}
 
 	ret = iommu_group_add_device(group, dev);