[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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