diff mbox series

[v5,1/6] iommu: add generic boot option iommu.dma_mode

Message ID 20190409125308.18304-2-thunder.leizhen@huawei.com
State New
Headers show
Series add generic boot option for IOMMU dma mode | expand

Commit Message

Zhen Lei April 9, 2019, 12:53 p.m. UTC
Currently the IOMMU dma contains 3 modes: passthrough, lazy, strict. The
passthrough mode bypass the IOMMU, the lazy mode defer the invalidation
of hardware TLBs, and the strict mode invalidate IOMMU hardware TLBs
synchronously. The three modes are mutually exclusive. But the current
boot options are confused, such as: iommu.passthrough and iommu.strict,
because they are no good to be coexist. So add iommu.dma_mode.

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

---
 Documentation/admin-guide/kernel-parameters.txt | 19 ++++++++
 drivers/iommu/iommu.c                           | 59 ++++++++++++++++++++-----
 include/linux/iommu.h                           |  5 +++
 3 files changed, 71 insertions(+), 12 deletions(-)

-- 
1.8.3

Comments

John Garry April 12, 2019, 10:26 a.m. UTC | #1
On 09/04/2019 13:53, Zhen Lei wrote:
> Currently the IOMMU dma contains 3 modes: passthrough, lazy, strict. The

> passthrough mode bypass the IOMMU, the lazy mode defer the invalidation

> of hardware TLBs, and the strict mode invalidate IOMMU hardware TLBs

> synchronously. The three modes are mutually exclusive. But the current

> boot options are confused, such as: iommu.passthrough and iommu.strict,

> because they are no good to be coexist. So add iommu.dma_mode.

>

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

> ---

>  Documentation/admin-guide/kernel-parameters.txt | 19 ++++++++

>  drivers/iommu/iommu.c                           | 59 ++++++++++++++++++++-----

>  include/linux/iommu.h                           |  5 +++

>  3 files changed, 71 insertions(+), 12 deletions(-)

>

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

> index 2b8ee90bb64470d..f7766f8ac8b9084 100644

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

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

> @@ -1811,6 +1811,25 @@

>  			1 - Bypass the IOMMU for DMA.

>  			unset - Use value of CONFIG_IOMMU_DEFAULT_PASSTHROUGH.

>

> +	iommu.dma_mode= Configure default dma mode. if unset, use the value

> +			of CONFIG_IOMMU_DEFAULT_PASSTHROUGH to determine

> +			passthrough or not.


To me, for unset it's unclear what we default to. So if unset and also 
CONFIG_IOMMU_DEFAULT_PASSTHROUGH is not set, do we get lazy or strict 
mode? (note: I'm ignoring backwards compatibility and interaction of 
iommu.strict and .passthorugh also, more below).

Could we considering introducing config DEFAULT_IOMMU_DMA_MODE, similar 
to DEFAULT_IOSCHED?

> +			Note: For historical reasons, ARM64/S390/PPC/X86 have

> +			their specific options. Currently, only ARM64 support

> +			this boot option, and hope other ARCHs to use this as

> +			generic boot option.

> +		passthrough

> +			Configure DMA to bypass the IOMMU by default.

> +		lazy

> +			Request that DMA unmap operations use deferred

> +			invalidation of hardware TLBs, for increased

> +			throughput at the cost of reduced device isolation.

> +			Will fall back to strict mode if not supported by

> +			the relevant IOMMU driver.

> +		strict

> +			DMA unmap operations invalidate IOMMU hardware TLBs

> +			synchronously.

> +

>  	io7=		[HW] IO7 for Marvel based alpha systems

>  			See comment before marvel_specify_io7 in

>  			arch/alpha/kernel/core_marvel.c.

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

> index 109de67d5d727c2..df1ce8e22385b48 100644

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

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

> @@ -38,12 +38,13 @@

>

>  static struct kset *iommu_group_kset;

>  static DEFINE_IDA(iommu_group_ida);

> +

>  #ifdef CONFIG_IOMMU_DEFAULT_PASSTHROUGH

> -static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;

> +#define IOMMU_DEFAULT_DMA_MODE		IOMMU_DMA_MODE_PASSTHROUGH

>  #else

> -static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;

> +#define IOMMU_DEFAULT_DMA_MODE		IOMMU_DMA_MODE_STRICT

>  #endif

> -static bool iommu_dma_strict __read_mostly = true;

> +static int iommu_default_dma_mode __read_mostly = IOMMU_DEFAULT_DMA_MODE;

>

>  struct iommu_callback_data {

>  	const struct iommu_ops *ops;

> @@ -147,20 +148,51 @@ static int __init iommu_set_def_domain_type(char *str)

>  	int ret;

>

>  	ret = kstrtobool(str, &pt);

> -	if (ret)

> -		return ret;

> +	if (!ret && pt)

> +		iommu_default_dma_mode = IOMMU_DMA_MODE_PASSTHROUGH;

>

> -	iommu_def_domain_type = pt ? IOMMU_DOMAIN_IDENTITY : IOMMU_DOMAIN_DMA;

> -	return 0;

> +	return ret;

>  }

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

>

>  static int __init iommu_dma_setup(char *str)

>  {

> -	return kstrtobool(str, &iommu_dma_strict);

> +	bool strict;

> +	int ret;

> +

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

> +	if (!ret)

> +		iommu_default_dma_mode = strict ?

> +				IOMMU_DMA_MODE_STRICT : IOMMU_DMA_MODE_LAZY;

> +

> +	return ret;

>  }

>  early_param("iommu.strict", iommu_dma_setup);

>

> +static int __init iommu_dma_mode_setup(char *str)

> +{

> +	if (!str)

> +		goto fail;

> +

> +	if (!strncmp(str, "passthrough", 11))

> +		iommu_default_dma_mode = IOMMU_DMA_MODE_PASSTHROUGH;

> +	else if (!strncmp(str, "lazy", 4))

> +		iommu_default_dma_mode = IOMMU_DMA_MODE_LAZY;

> +	else if (!strncmp(str, "strict", 6))

> +		iommu_default_dma_mode = IOMMU_DMA_MODE_STRICT;

> +	else

> +		goto fail;

> +

> +	pr_info("Force dma mode to be %d\n", iommu_default_dma_mode);


What happens if the cmdline option iommu.dma_mode is passed multiple 
times? We get mutliple - possibily conflicting - prints, right?

And do we need to have backwards compatibility, such that the setting 
for iommu.strict or iommu.passthrough trumps iommu.dma_mode, regardless 
of order?

> +

> +	return 0;

> +

> +fail:

> +	pr_debug("Boot option iommu.dma_mode is incorrect, ignored\n");

> +	return -EINVAL;

> +}

> +early_param("iommu.dma_mode", iommu_dma_mode_setup);

> +

>  static ssize_t iommu_group_attr_show(struct kobject *kobj,

>  				     struct attribute *__attr, char *buf)

>  {

> @@ -1102,14 +1134,17 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)

>  	 */

>  	if (!group->default_domain) {

>  		struct iommu_domain *dom;

> +		int def_domain_type =

> +			(iommu_default_dma_mode == IOMMU_DMA_MODE_PASSTHROUGH)

> +			? IOMMU_DOMAIN_IDENTITY : IOMMU_DOMAIN_DMA;

>

> -		dom = __iommu_domain_alloc(dev->bus, iommu_def_domain_type);

> -		if (!dom && iommu_def_domain_type != IOMMU_DOMAIN_DMA) {

> +		dom = __iommu_domain_alloc(dev->bus, def_domain_type);

> +		if (!dom && def_domain_type != IOMMU_DOMAIN_DMA) {

>  			dom = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_DMA);

>  			if (dom) {

>  				dev_warn(dev,

>  					 "failed to allocate default IOMMU domain of type %u; falling back to IOMMU_DOMAIN_DMA",

> -					 iommu_def_domain_type);

> +					 def_domain_type);

>  			}

>  		}

>

> @@ -1117,7 +1152,7 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)

>  		if (!group->domain)

>  			group->domain = dom;

>

> -		if (dom && !iommu_dma_strict) {

> +		if (dom && (iommu_default_dma_mode == IOMMU_DMA_MODE_LAZY)) {

>  			int attr = 1;

>  			iommu_domain_set_attr(dom,

>  					      DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,

> diff --git a/include/linux/iommu.h b/include/linux/iommu.h

> index ffbbc7e39ceeba3..c3f4e3416176496 100644

> --- a/include/linux/iommu.h

> +++ b/include/linux/iommu.h

> @@ -42,6 +42,11 @@

>   */

>  #define IOMMU_PRIV	(1 << 5)

>

> +

> +#define IOMMU_DMA_MODE_STRICT		0x0

> +#define IOMMU_DMA_MODE_LAZY		0x1

> +#define IOMMU_DMA_MODE_PASSTHROUGH	0x2

> +

>  struct iommu_ops;

>  struct iommu_group;

>  struct bus_type;

>
Joerg Roedel April 12, 2019, 11:16 a.m. UTC | #2
On Tue, Apr 09, 2019 at 08:53:03PM +0800, Zhen Lei wrote:
> +static int __init iommu_dma_mode_setup(char *str)

> +{

> +	if (!str)

> +		goto fail;

> +

> +	if (!strncmp(str, "passthrough", 11))

> +		iommu_default_dma_mode = IOMMU_DMA_MODE_PASSTHROUGH;

> +	else if (!strncmp(str, "lazy", 4))

> +		iommu_default_dma_mode = IOMMU_DMA_MODE_LAZY;

> +	else if (!strncmp(str, "strict", 6))

> +		iommu_default_dma_mode = IOMMU_DMA_MODE_STRICT;

> +	else

> +		goto fail;

> +

> +	pr_info("Force dma mode to be %d\n", iommu_default_dma_mode);


Printing a number is not very desriptive or helpful to the user. Please
print the name of the mode instead.


Regards,

	Joerg
Robin Murphy April 12, 2019, 1:11 p.m. UTC | #3
On 12/04/2019 11:26, John Garry wrote:
> On 09/04/2019 13:53, Zhen Lei wrote:

>> Currently the IOMMU dma contains 3 modes: passthrough, lazy, strict. The

>> passthrough mode bypass the IOMMU, the lazy mode defer the invalidation

>> of hardware TLBs, and the strict mode invalidate IOMMU hardware TLBs

>> synchronously. The three modes are mutually exclusive. But the current

>> boot options are confused, such as: iommu.passthrough and iommu.strict,

>> because they are no good to be coexist. So add iommu.dma_mode.

>>

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

>> ---

>>  Documentation/admin-guide/kernel-parameters.txt | 19 ++++++++

>>  drivers/iommu/iommu.c                           | 59 

>> ++++++++++++++++++++-----

>>  include/linux/iommu.h                           |  5 +++

>>  3 files changed, 71 insertions(+), 12 deletions(-)

>>

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

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

>> index 2b8ee90bb64470d..f7766f8ac8b9084 100644

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

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

>> @@ -1811,6 +1811,25 @@

>>              1 - Bypass the IOMMU for DMA.

>>              unset - Use value of CONFIG_IOMMU_DEFAULT_PASSTHROUGH.

>>

>> +    iommu.dma_mode= Configure default dma mode. if unset, use the value

>> +            of CONFIG_IOMMU_DEFAULT_PASSTHROUGH to determine

>> +            passthrough or not.

> 

> To me, for unset it's unclear what we default to. So if unset and also 

> CONFIG_IOMMU_DEFAULT_PASSTHROUGH is not set, do we get lazy or strict 

> mode? (note: I'm ignoring backwards compatibility and interaction of 

> iommu.strict and .passthorugh also, more below).

> 

> Could we considering introducing config DEFAULT_IOMMU_DMA_MODE, similar 

> to DEFAULT_IOSCHED?


Yes, what I was suggesting was specifically refactoring the Kconfig 
options into a single choice that controls the default (i.e. no command 
line option provided) behaviour. AFAICS it should be fairly 
straightforward to maintain the existing "strict" and "passthrough" 
options (and legacy arch-specific versions thereof) to override that 
default without introducing yet another command-line option, which I 
think we should avoid if possible.
>> +            Note: For historical reasons, ARM64/S390/PPC/X86 have

>> +            their specific options. Currently, only ARM64 support

>> +            this boot option, and hope other ARCHs to use this as

>> +            generic boot option.

>> +        passthrough

>> +            Configure DMA to bypass the IOMMU by default.

>> +        lazy

>> +            Request that DMA unmap operations use deferred

>> +            invalidation of hardware TLBs, for increased

>> +            throughput at the cost of reduced device isolation.

>> +            Will fall back to strict mode if not supported by

>> +            the relevant IOMMU driver.

>> +        strict

>> +            DMA unmap operations invalidate IOMMU hardware TLBs

>> +            synchronously.

>> +

>>      io7=        [HW] IO7 for Marvel based alpha systems

>>              See comment before marvel_specify_io7 in

>>              arch/alpha/kernel/core_marvel.c.

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

>> index 109de67d5d727c2..df1ce8e22385b48 100644

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

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

>> @@ -38,12 +38,13 @@

>>

>>  static struct kset *iommu_group_kset;

>>  static DEFINE_IDA(iommu_group_ida);

>> +

>>  #ifdef CONFIG_IOMMU_DEFAULT_PASSTHROUGH

>> -static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;

>> +#define IOMMU_DEFAULT_DMA_MODE        IOMMU_DMA_MODE_PASSTHROUGH

>>  #else

>> -static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;

>> +#define IOMMU_DEFAULT_DMA_MODE        IOMMU_DMA_MODE_STRICT

>>  #endif

>> -static bool iommu_dma_strict __read_mostly = true;

>> +static int iommu_default_dma_mode __read_mostly = 

>> IOMMU_DEFAULT_DMA_MODE;

>>

>>  struct iommu_callback_data {

>>      const struct iommu_ops *ops;

>> @@ -147,20 +148,51 @@ static int __init iommu_set_def_domain_type(char 

>> *str)

>>      int ret;

>>

>>      ret = kstrtobool(str, &pt);

>> -    if (ret)

>> -        return ret;

>> +    if (!ret && pt)

>> +        iommu_default_dma_mode = IOMMU_DMA_MODE_PASSTHROUGH;

>>

>> -    iommu_def_domain_type = pt ? IOMMU_DOMAIN_IDENTITY : 

>> IOMMU_DOMAIN_DMA;

>> -    return 0;

>> +    return ret;

>>  }

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

>>

>>  static int __init iommu_dma_setup(char *str)

>>  {

>> -    return kstrtobool(str, &iommu_dma_strict);

>> +    bool strict;

>> +    int ret;

>> +

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

>> +    if (!ret)

>> +        iommu_default_dma_mode = strict ?

>> +                IOMMU_DMA_MODE_STRICT : IOMMU_DMA_MODE_LAZY;

>> +

>> +    return ret;

>>  }

>>  early_param("iommu.strict", iommu_dma_setup);

>>

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

>> +{

>> +    if (!str)

>> +        goto fail;

>> +

>> +    if (!strncmp(str, "passthrough", 11))

>> +        iommu_default_dma_mode = IOMMU_DMA_MODE_PASSTHROUGH;

>> +    else if (!strncmp(str, "lazy", 4))

>> +        iommu_default_dma_mode = IOMMU_DMA_MODE_LAZY;

>> +    else if (!strncmp(str, "strict", 6))

>> +        iommu_default_dma_mode = IOMMU_DMA_MODE_STRICT;

>> +    else

>> +        goto fail;

>> +

>> +    pr_info("Force dma mode to be %d\n", iommu_default_dma_mode);

> 

> What happens if the cmdline option iommu.dma_mode is passed multiple 

> times? We get mutliple - possibily conflicting - prints, right?


Indeed; we ended up removing such prints for the existing options here, 
specifically because multiple messages seemed more likely to be 
confusing than useful.

> And do we need to have backwards compatibility, such that the setting 

> for iommu.strict or iommu.passthrough trumps iommu.dma_mode, regardless 

> of order?


As above I think it would be preferable to just keep using the existing 
options anyway. The current behaviour works out as:

iommu.passthrough |      Y	|	  N
iommu.strict	  |      x	|    Y         N
------------------|-------------|---------|--------
MODE		  | PASSTHROUGH | STRICT  |  LAZY

which seems intuitive enough that a specific dma_mode option doesn't add 
much value, and would more likely just overcomplicate things for users 
as well as our implementation.

Robin.

>> +

>> +    return 0;

>> +

>> +fail:

>> +    pr_debug("Boot option iommu.dma_mode is incorrect, ignored\n");

>> +    return -EINVAL;

>> +}

>> +early_param("iommu.dma_mode", iommu_dma_mode_setup);

>> +

>>  static ssize_t iommu_group_attr_show(struct kobject *kobj,

>>                       struct attribute *__attr, char *buf)

>>  {

>> @@ -1102,14 +1134,17 @@ struct iommu_group 

>> *iommu_group_get_for_dev(struct device *dev)

>>       */

>>      if (!group->default_domain) {

>>          struct iommu_domain *dom;

>> +        int def_domain_type =

>> +            (iommu_default_dma_mode == IOMMU_DMA_MODE_PASSTHROUGH)

>> +            ? IOMMU_DOMAIN_IDENTITY : IOMMU_DOMAIN_DMA;

>>

>> -        dom = __iommu_domain_alloc(dev->bus, iommu_def_domain_type);

>> -        if (!dom && iommu_def_domain_type != IOMMU_DOMAIN_DMA) {

>> +        dom = __iommu_domain_alloc(dev->bus, def_domain_type);

>> +        if (!dom && def_domain_type != IOMMU_DOMAIN_DMA) {

>>              dom = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_DMA);

>>              if (dom) {

>>                  dev_warn(dev,

>>                       "failed to allocate default IOMMU domain of type 

>> %u; falling back to IOMMU_DOMAIN_DMA",

>> -                     iommu_def_domain_type);

>> +                     def_domain_type);

>>              }

>>          }

>>

>> @@ -1117,7 +1152,7 @@ struct iommu_group 

>> *iommu_group_get_for_dev(struct device *dev)

>>          if (!group->domain)

>>              group->domain = dom;

>>

>> -        if (dom && !iommu_dma_strict) {

>> +        if (dom && (iommu_default_dma_mode == IOMMU_DMA_MODE_LAZY)) {

>>              int attr = 1;

>>              iommu_domain_set_attr(dom,

>>                            DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,

>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h

>> index ffbbc7e39ceeba3..c3f4e3416176496 100644

>> --- a/include/linux/iommu.h

>> +++ b/include/linux/iommu.h

>> @@ -42,6 +42,11 @@

>>   */

>>  #define IOMMU_PRIV    (1 << 5)

>>

>> +

>> +#define IOMMU_DMA_MODE_STRICT        0x0

>> +#define IOMMU_DMA_MODE_LAZY        0x1

>> +#define IOMMU_DMA_MODE_PASSTHROUGH    0x2

>> +

>>  struct iommu_ops;

>>  struct iommu_group;

>>  struct bus_type;

>>

> 

>
Will Deacon April 16, 2019, 3:21 p.m. UTC | #4
On Fri, Apr 12, 2019 at 02:11:31PM +0100, Robin Murphy wrote:
> On 12/04/2019 11:26, John Garry wrote:

> > On 09/04/2019 13:53, Zhen Lei wrote:

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

> > > +{

> > > +    if (!str)

> > > +        goto fail;

> > > +

> > > +    if (!strncmp(str, "passthrough", 11))

> > > +        iommu_default_dma_mode = IOMMU_DMA_MODE_PASSTHROUGH;

> > > +    else if (!strncmp(str, "lazy", 4))

> > > +        iommu_default_dma_mode = IOMMU_DMA_MODE_LAZY;

> > > +    else if (!strncmp(str, "strict", 6))

> > > +        iommu_default_dma_mode = IOMMU_DMA_MODE_STRICT;

> > > +    else

> > > +        goto fail;

> > > +

> > > +    pr_info("Force dma mode to be %d\n", iommu_default_dma_mode);

> > 

> > What happens if the cmdline option iommu.dma_mode is passed multiple

> > times? We get mutliple - possibily conflicting - prints, right?

> 

> Indeed; we ended up removing such prints for the existing options here,

> specifically because multiple messages seemed more likely to be confusing

> than useful.

> 

> > And do we need to have backwards compatibility, such that the setting

> > for iommu.strict or iommu.passthrough trumps iommu.dma_mode, regardless

> > of order?

> 

> As above I think it would be preferable to just keep using the existing

> options anyway. The current behaviour works out as:

> 

> iommu.passthrough |      Y	|	  N

> iommu.strict	  |      x	|    Y         N

> ------------------|-------------|---------|--------

> MODE		  | PASSTHROUGH | STRICT  |  LAZY

> 

> which seems intuitive enough that a specific dma_mode option doesn't add

> much value, and would more likely just overcomplicate things for users as

> well as our implementation.


Agreed. We can't remove the existing options, and they do the job perfectly
well so I don't see the need to add more options on top.

Will
Zhen Lei April 17, 2019, 2:36 a.m. UTC | #5
On 2019/4/16 23:21, Will Deacon wrote:
> On Fri, Apr 12, 2019 at 02:11:31PM +0100, Robin Murphy wrote:

>> On 12/04/2019 11:26, John Garry wrote:

>>> On 09/04/2019 13:53, Zhen Lei wrote:

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

>>>> +{

>>>> +    if (!str)

>>>> +        goto fail;

>>>> +

>>>> +    if (!strncmp(str, "passthrough", 11))

>>>> +        iommu_default_dma_mode = IOMMU_DMA_MODE_PASSTHROUGH;

>>>> +    else if (!strncmp(str, "lazy", 4))

>>>> +        iommu_default_dma_mode = IOMMU_DMA_MODE_LAZY;

>>>> +    else if (!strncmp(str, "strict", 6))

>>>> +        iommu_default_dma_mode = IOMMU_DMA_MODE_STRICT;

>>>> +    else

>>>> +        goto fail;

>>>> +

>>>> +    pr_info("Force dma mode to be %d\n", iommu_default_dma_mode);

>>>

>>> What happens if the cmdline option iommu.dma_mode is passed multiple

>>> times? We get mutliple - possibily conflicting - prints, right?

>>

>> Indeed; we ended up removing such prints for the existing options here,

>> specifically because multiple messages seemed more likely to be confusing

>> than useful.


I originally intended to be compatible with X86 printing.

 		} else if (!strncmp(str, "strict", 6)) {
 			pr_info("Disable batched IOTLB flush\n");
			intel_iommu_strict = 1;
 		}

>>

>>> And do we need to have backwards compatibility, such that the setting

>>> for iommu.strict or iommu.passthrough trumps iommu.dma_mode, regardless

>>> of order?

>>

>> As above I think it would be preferable to just keep using the existing

>> options anyway. The current behaviour works out as:

>>

>> iommu.passthrough |      Y	|	  N

>> iommu.strict	  |      x	|    Y         N

>> ------------------|-------------|---------|--------

>> MODE		  | PASSTHROUGH | STRICT  |  LAZY

>>

>> which seems intuitive enough that a specific dma_mode option doesn't add

>> much value, and would more likely just overcomplicate things for users as

>> well as our implementation.

> 

> Agreed. We can't remove the existing options, and they do the job perfectly

> well so I don't see the need to add more options on top.


OK, I will remove the iommu.dma_mode option in the next version. Thanks for you three.

I didn't want to add it at first, but later found that the boot options on
each ARCH are different, then want to normalize it.

In addition, do we need to compatible the build option name IOMMU_DEFAULT_PASSTHROUGH? or
change it to IOMMU_DEFAULT_DMA_MODE_PASSTHROUGH or IOMMU_DEFAULT_MODE_PASSTHROUGH?

> 

> Will

> 

> .

> 


-- 
Thanks!
BestRegards
Zhen Lei April 23, 2019, 2:45 a.m. UTC | #6
On 2019/4/12 19:16, Joerg Roedel wrote:
> On Tue, Apr 09, 2019 at 08:53:03PM +0800, Zhen Lei wrote:

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

>> +{

>> +	if (!str)

>> +		goto fail;

>> +

>> +	if (!strncmp(str, "passthrough", 11))

>> +		iommu_default_dma_mode = IOMMU_DMA_MODE_PASSTHROUGH;

>> +	else if (!strncmp(str, "lazy", 4))

>> +		iommu_default_dma_mode = IOMMU_DMA_MODE_LAZY;

>> +	else if (!strncmp(str, "strict", 6))

>> +		iommu_default_dma_mode = IOMMU_DMA_MODE_STRICT;

>> +	else

>> +		goto fail;

>> +

>> +	pr_info("Force dma mode to be %d\n", iommu_default_dma_mode);

> 

> Printing a number is not very desriptive or helpful to the user. Please

> print the name of the mode instead.


OK, thanks. I have given up adding iommu.dma_mode boot option according
to Robin and Will's suggestion. So these codes will be removed in v6.

> 

> 

> Regards,

> 

> 	Joerg

> 

> .

> 


-- 
Thanks!
BestRegards
diff mbox series

Patch

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 2b8ee90bb64470d..f7766f8ac8b9084 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1811,6 +1811,25 @@ 
 			1 - Bypass the IOMMU for DMA.
 			unset - Use value of CONFIG_IOMMU_DEFAULT_PASSTHROUGH.
 
+	iommu.dma_mode= Configure default dma mode. if unset, use the value
+			of CONFIG_IOMMU_DEFAULT_PASSTHROUGH to determine
+			passthrough or not.
+			Note: For historical reasons, ARM64/S390/PPC/X86 have
+			their specific options. Currently, only ARM64 support
+			this boot option, and hope other ARCHs to use this as
+			generic boot option.
+		passthrough
+			Configure DMA to bypass the IOMMU by default.
+		lazy
+			Request that DMA unmap operations use deferred
+			invalidation of hardware TLBs, for increased
+			throughput at the cost of reduced device isolation.
+			Will fall back to strict mode if not supported by
+			the relevant IOMMU driver.
+		strict
+			DMA unmap operations invalidate IOMMU hardware TLBs
+			synchronously.
+
 	io7=		[HW] IO7 for Marvel based alpha systems
 			See comment before marvel_specify_io7 in
 			arch/alpha/kernel/core_marvel.c.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 109de67d5d727c2..df1ce8e22385b48 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -38,12 +38,13 @@ 
 
 static struct kset *iommu_group_kset;
 static DEFINE_IDA(iommu_group_ida);
+
 #ifdef CONFIG_IOMMU_DEFAULT_PASSTHROUGH
-static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;
+#define IOMMU_DEFAULT_DMA_MODE		IOMMU_DMA_MODE_PASSTHROUGH
 #else
-static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;
+#define IOMMU_DEFAULT_DMA_MODE		IOMMU_DMA_MODE_STRICT
 #endif
-static bool iommu_dma_strict __read_mostly = true;
+static int iommu_default_dma_mode __read_mostly = IOMMU_DEFAULT_DMA_MODE;
 
 struct iommu_callback_data {
 	const struct iommu_ops *ops;
@@ -147,20 +148,51 @@  static int __init iommu_set_def_domain_type(char *str)
 	int ret;
 
 	ret = kstrtobool(str, &pt);
-	if (ret)
-		return ret;
+	if (!ret && pt)
+		iommu_default_dma_mode = IOMMU_DMA_MODE_PASSTHROUGH;
 
-	iommu_def_domain_type = pt ? IOMMU_DOMAIN_IDENTITY : IOMMU_DOMAIN_DMA;
-	return 0;
+	return ret;
 }
 early_param("iommu.passthrough", iommu_set_def_domain_type);
 
 static int __init iommu_dma_setup(char *str)
 {
-	return kstrtobool(str, &iommu_dma_strict);
+	bool strict;
+	int ret;
+
+	ret = kstrtobool(str, &strict);
+	if (!ret)
+		iommu_default_dma_mode = strict ?
+				IOMMU_DMA_MODE_STRICT : IOMMU_DMA_MODE_LAZY;
+
+	return ret;
 }
 early_param("iommu.strict", iommu_dma_setup);
 
+static int __init iommu_dma_mode_setup(char *str)
+{
+	if (!str)
+		goto fail;
+
+	if (!strncmp(str, "passthrough", 11))
+		iommu_default_dma_mode = IOMMU_DMA_MODE_PASSTHROUGH;
+	else if (!strncmp(str, "lazy", 4))
+		iommu_default_dma_mode = IOMMU_DMA_MODE_LAZY;
+	else if (!strncmp(str, "strict", 6))
+		iommu_default_dma_mode = IOMMU_DMA_MODE_STRICT;
+	else
+		goto fail;
+
+	pr_info("Force dma mode to be %d\n", iommu_default_dma_mode);
+
+	return 0;
+
+fail:
+	pr_debug("Boot option iommu.dma_mode is incorrect, ignored\n");
+	return -EINVAL;
+}
+early_param("iommu.dma_mode", iommu_dma_mode_setup);
+
 static ssize_t iommu_group_attr_show(struct kobject *kobj,
 				     struct attribute *__attr, char *buf)
 {
@@ -1102,14 +1134,17 @@  struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 	 */
 	if (!group->default_domain) {
 		struct iommu_domain *dom;
+		int def_domain_type =
+			(iommu_default_dma_mode == IOMMU_DMA_MODE_PASSTHROUGH)
+			? IOMMU_DOMAIN_IDENTITY : IOMMU_DOMAIN_DMA;
 
-		dom = __iommu_domain_alloc(dev->bus, iommu_def_domain_type);
-		if (!dom && iommu_def_domain_type != IOMMU_DOMAIN_DMA) {
+		dom = __iommu_domain_alloc(dev->bus, def_domain_type);
+		if (!dom && def_domain_type != IOMMU_DOMAIN_DMA) {
 			dom = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_DMA);
 			if (dom) {
 				dev_warn(dev,
 					 "failed to allocate default IOMMU domain of type %u; falling back to IOMMU_DOMAIN_DMA",
-					 iommu_def_domain_type);
+					 def_domain_type);
 			}
 		}
 
@@ -1117,7 +1152,7 @@  struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 		if (!group->domain)
 			group->domain = dom;
 
-		if (dom && !iommu_dma_strict) {
+		if (dom && (iommu_default_dma_mode == IOMMU_DMA_MODE_LAZY)) {
 			int attr = 1;
 			iommu_domain_set_attr(dom,
 					      DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ffbbc7e39ceeba3..c3f4e3416176496 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -42,6 +42,11 @@ 
  */
 #define IOMMU_PRIV	(1 << 5)
 
+
+#define IOMMU_DMA_MODE_STRICT		0x0
+#define IOMMU_DMA_MODE_LAZY		0x1
+#define IOMMU_DMA_MODE_PASSTHROUGH	0x2
+
 struct iommu_ops;
 struct iommu_group;
 struct bus_type;