Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v5 1/6] iommu: add generic boot option iommu.dma_mode
To:     John Garry <john.garry@huawei.com>,
        Zhen Lei <thunder.leizhen@huawei.com>,
        Jean-Philippe Brucker <jean-philippe.brucker@arm.com>,
        Will Deacon <will.deacon@arm.com>,
        Joerg Roedel <joro@8bytes.org>,
        Jonathan Corbet <corbet@lwn.net>,
        linux-doc <linux-doc@vger.kernel.org>,
        Sebastian Ott <sebott@linux.ibm.com>,
        Gerald Schaefer <gerald.schaefer@de.ibm.com>,
        Martin Schwidefsky <schwidefsky@de.ibm.com>,
        Heiko Carstens <heiko.carstens@de.ibm.com>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Tony Luck <tony.luck@intel.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H . Peter Anvin" <hpa@zytor.com>,
        David Woodhouse <dwmw2@infradead.org>,
        iommu <iommu@lists.linux-foundation.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-s390 <linux-s390@vger.kernel.org>,
        linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
        x86 <x86@kernel.org>, linux-ia64 <linux-ia64@vger.kernel.org>
Cc:     Hanjun Guo <guohanjun@huawei.com>
References: <20190409125308.18304-1-thunder.leizhen@huawei.com>
 <20190409125308.18304-2-thunder.leizhen@huawei.com>
 <010d3cbd-ef74-ad21-c735-0af8b18955e6@huawei.com>
From:   Robin Murphy <robin.murphy@arm.com>
Message-ID: <222946ee-adcc-1311-82a7-6afc9ffbc846@arm.com>
Date:   Fri, 12 Apr 2019 14:11:31 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <010d3cbd-ef74-ad21-c735-0af8b18955e6@huawei.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-GB
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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;
>>
> 
>