Message-ID: <222946ee-adcc-1311-82a7-6afc9ffbc846@arm.com>
Date:   Fri, 12 Apr 2019 14:11:31 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     John Garry <john.garry@...wei.com>,
        Zhen Lei <thunder.leizhen@...wei.com>,
        Jean-Philippe Brucker <jean-philippe.brucker@....com>,
        Will Deacon <will.deacon@....com>,
        Joerg Roedel <joro@...tes.org>,
        Jonathan Corbet <corbet@....net>,
        linux-doc <linux-doc@...r.kernel.org>,
        Sebastian Ott <sebott@...ux.ibm.com>,
        Gerald Schaefer <gerald.schaefer@...ibm.com>,
        Martin Schwidefsky <schwidefsky@...ibm.com>,
        Heiko Carstens <heiko.carstens@...ibm.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Tony Luck <tony.luck@...el.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H . Peter Anvin" <hpa@...or.com>,
        David Woodhouse <dwmw2@...radead.org>,
        iommu <iommu@...ts.linux-foundation.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-s390 <linux-s390@...r.kernel.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        x86 <x86@...nel.org>, linux-ia64 <linux-ia64@...r.kernel.org>
Cc:     Hanjun Guo <guohanjun@...wei.com>
Subject: Re: [PATCH v5 1/6] iommu: add generic boot option iommu.dma_mode

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@...wei.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;
>>
> 
> 

Powered by blists - more mailing lists