lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
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