Add non-strict mode support for iommu-dma
Subsystems (4)
Robin Murphy
maintainerv8 cover Add non-strict mode support for iommu-dma
Hi all,
Hopefully this is the last spin of the series - I've now dropped my light touch and fixed up all the various prose text, plus implemented the proper quirk support for short-descriptor because it's actually just a trivial cut-and-paste job.
Robin.
Robin Murphy (2):
iommu/io-pgtable-arm-v7s: Add support for non-strict mode iommu/arm-smmu: Support non-strict mode
Zhen Lei (5):
iommu/arm-smmu-v3: Implement flush_iotlb_all hook iommu/dma: Add support for non-strict mode
iommu: Add "iommu.strict" command line option iommu/io-pgtable-arm: Add support for non-strict mode iommu/arm-smmu-v3: Add support for non-strict mode
.../admin-guide/kernel-parameters.txt | 12 +++ drivers/iommu/arm-smmu-v3.c | 89 +++++++++++++----- drivers/iommu/arm-smmu.c | 93 +++++++++++++------ drivers/iommu/dma-iommu.c | 32 ++++++- drivers/iommu/io-pgtable-arm-v7s.c | 11 ++- drivers/iommu/io-pgtable-arm.c | 14 ++- drivers/iommu/io-pgtable.h | 5 + drivers/iommu/iommu.c | 23 +++++ include/linux/iommu.h | 1 + 9 files changed, 225 insertions(+), 55 deletions(-)
Signature · 1 line
2.19.0.dirty
Robin Murphy
maintainerPATCH v8 1/7 iommu/arm-smmu-v3: Implement flush_iotlb_all hook
From: Zhen Lei <[email protected]>
.flush_iotlb_all is currently stubbed to arm_smmu_iotlb_sync() since the only time it would ever need to actually do anything is for callers doing their own explicit batching, e.g.:
iommu_unmap_fast(domain, ...); iommu_unmap_fast(domain, ...); iommu_iotlb_flush_all(domain, ...);
where since io-pgtable still issues the TLBI commands implicitly in the unmap instead of implementing .iotlb_range_add, the "flush" only needs to ensure completion of those already-in-flight invalidations.
However, we're about to start using it in anger with flush queues, so let's get a proper implementation wired up.
--- drivers/iommu/arm-smmu-v3.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/iommu/arm-smmu-v3.c +9 -1
@@ -1781,6 +1781,14 @@ arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)return ops->unmap (ops, iova, size); } +static void arm_smmu_flush_iotlb_all (struct iommu_domain *domain) +{ +struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain); + +if (smmu_domain->smmu) +arm_smmu_tlb_inv_context (smmu_domain); +} +static void arm_smmu_iotlb_sync (struct iommu_domain *domain) {struct arm_smmu_device *smmu =to_smmu_domain (domain)->smmu;
@@ -2008,7 +2016,7 @@ static struct iommu_ops arm_smmu_ops = { .attach_dev = arm_smmu_attach_dev, .map = arm_smmu_map, .unmap = arm_smmu_unmap, - .flush_iotlb_all = arm_smmu_iotlb_sync, + .flush_iotlb_all = arm_smmu_flush_iotlb_all, .iotlb_sync = arm_smmu_iotlb_sync, .iova_to_phys = arm_smmu_iova_to_phys, .add_device = arm_smmu_add_device,
Signature · 1 line
2.19.0.dirty
Robin Murphy
maintainerPATCH v8 2/7 iommu/dma: Add support for non-strict mode
In reply to Robin MurphyFrom: Zhen Lei <[email protected]>
With the flush queue infrastructure already abstracted into IOVA domains, hooking it up in iommu-dma is pretty simple. Since there is a degree of dependency on the IOMMU driver knowing what to do to play along, we key the whole thing off a domain attribute which will be set on default DMA ops domains to request non-strict invalidation. That way, drivers can indicate the appropriate support by acknowledging the attribute, and we can easily fall back to strict invalidation otherwise.
The flush queue callback needs a handle on the iommu_domain which owns our cookie, so we have to add a pointer back to that, but neatly, that's also sufficient to indicate whether we're using a flush queue or not, and thus which way to release IOVAs. The only slight subtlety is switching __iommu_dma_unmap() from calling iommu_unmap() to explicit iommu_unmap_fast()/iommu_tlb_sync() so that we can elide the sync entirely in non-strict mode.
---
v8:
- Rewrite commit message/comments
- Don't initialise "attr" unnecessarily
- Rename "domain" to "fq_domain" for clarity
- Don't let init_iova_flush_queue() be called more than once
drivers/iommu/dma-iommu.c | 32 +++++++++++++++++++++++++++++++- include/linux/iommu.h | 1 + 2 files changed, 32 insertions(+), 1 deletion(-)
drivers/iommu/dma-iommu.c +31 -1
@@ -55,6 +55,9 @@ struct iommu_dma_cookie { };struct list_head msi_page_list;spinlock_t msi_lock; + +/* Domain for flush queue callback; NULL if flush queue not in use */ +struct iommu_domain *fq_domain; };static inline size_t cookie_msi_granule (struct iommu_dma_cookie *cookie)
@@ -257,6 +260,20 @@ static int iova_reserve_iommu_regions(struct device *dev,return ret; } +static void iommu_dma_flush_iotlb_all (struct iova_domain *iovad) +{ +struct iommu_dma_cookie *cookie; +struct iommu_domain *domain; + + cookie = container_of(iovad, struct iommu_dma_cookie, iovad); + domain = cookie->fq_domain; +/* +* The IOMMU driver supporting DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE +* implies that ops->flush_iotlb_all must be non-NULL. +*/ + domain->ops->flush_iotlb_all (domain); +} +/** * iommu_dma_init_domain - Initialise a DMA mapping domain * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
@@ -275,6 +292,7 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,struct iommu_dma_cookie *cookie = domain->iova_cookie;struct iova_domain *iovad = &cookie->iovad;unsigned long order, base_pfn, end_pfn; +int attr;if (!cookie || cookie->type !=IOMMU_DMA_IOVA_COOKIE )return -EINVAL ;
@@ -308,6 +326,13 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base, }init_iova_domain (iovad,1UL << order, base_pfn); + +if (!cookie->fq_domain && !iommu_domain_get_attr (domain, +DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE , &attr) && attr) { + cookie->fq_domain = domain; +init_iova_flush_queue (iovad, iommu_dma_flush_iotlb_all,NULL ); + } +if (!dev)return 0 ;
@@ -393,6 +418,9 @@ static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie,/* The MSI case is only ever cleaning up its most recent allocation */ if (cookie->type ==IOMMU_DMA_MSI_COOKIE ) cookie->msi_iova -= size; +else if (cookie->fq_domain)/* non-strict mode */ +queue_iova (iovad,iova_pfn (iovad, iova), + size >>iova_shift (iovad),0 );else free_iova_fast (iovad,iova_pfn (iovad, iova), size >>iova_shift (iovad));
@@ -408,7 +436,9 @@ static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t dma_addr, dma_addr -= iova_off; size =iova_align (iovad, size + iova_off); -WARN_ON (iommu_unmap (domain, dma_addr, size) != size); +WARN_ON (iommu_unmap_fast (domain, dma_addr, size) != size); +if (!cookie->fq_domain) +iommu_tlb_sync (domain);iommu_dma_free_iova (cookie, dma_addr, size); }
include/linux/iommu.h +1 -0
@@ -124,6 +124,7 @@ enum iommu_attr {DOMAIN_ATTR_FSL_PAMU_ENABLE ,DOMAIN_ATTR_FSL_PAMUV1 ,DOMAIN_ATTR_NESTING ,/* two stages of translation */ +DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE , DOMAIN_ATTR_MAX, };
Signature · 1 line
2.19.0.dirty
Robin Murphy
maintainerPATCH v8 3/7 iommu: Add "iommu.strict" command line option
In reply to Robin MurphyFrom: Zhen Lei <[email protected]>
Add a generic command line option to enable lazy unmapping via IOVA flush queues, which will initally be suuported by iommu-dma. This echoes the semantics of "intel_iommu=strict" (albeit with the opposite default value), but in the driver-agnostic fashion of "iommu.passthrough".
---
v8:
- Rename "non-strict" to "strict" to better match existing options
- Rewrite doc text/commit message
- Downgrade boot-time message from warn/taint to info
.../admin-guide/kernel-parameters.txt | 12 ++++++++++ drivers/iommu/iommu.c | 23 +++++++++++++++++++ 2 files changed, 35 insertions(+)
Documentation/admin-guide/kernel-parameters.txt +12 -0
@@ -1749,6 +1749,18 @@ nobypass [PPC/POWERNV] Disable IOMMU bypass, using IOMMU for PCI devices. + iommu.strict= [ARM64] Configure TLB invalidation behaviour + Format: { "0" | "1" } + 0 - Lazy mode. + 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. + 1 - Strict mode (default). + DMA unmap operations invalidate IOMMU hardware TLBs + synchronously. + iommu.passthrough= [ARM64] Configure DMA to bypass the IOMMU by default. Format: { "0" | "1" }
drivers/iommu/iommu.c +23 -0
@@ -41,6 +41,7 @@ static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;#else static unsigned int iommu_def_domain_type =IOMMU_DOMAIN_DMA ;#endif +static bool iommu_dma_strict__read_mostly = true;struct iommu_callback_data {const struct iommu_ops *ops;
@@ -131,6 +132,21 @@ static int __init iommu_set_def_domain_type(char *str) }early_param ("iommu.passthrough" , iommu_set_def_domain_type); +static int __init iommu_dma_setup (char *str) +{ +int ret; + + ret =kstrtobool (str, &iommu_dma_strict); +if (ret) +return ret; + +if (!iommu_dma_strict) +pr_info ("Enabling deferred TLB invalidation for DMA; protection against malicious/malfunctioning devices may be reduced.\n" ); + +return 0 ; +} +early_param ("iommu.strict" , iommu_dma_setup); +static ssize_t iommu_group_attr_show (struct kobject *kobj,struct attribute *__attr,char *buf) {
@@ -1072,6 +1088,13 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev) group->default_domain = dom;if (!group->domain) group->domain = dom; + +if (dom && !iommu_dma_strict) { +int attr =1 ; +iommu_domain_set_attr (dom, +DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE , + &attr); + } } ret =iommu_group_add_device (group, dev);
Signature · 1 line
2.19.0.dirty
Robin Murphy
maintainerPATCH v8 4/7 iommu/io-pgtable-arm: Add support for non-strict mode
In reply to Robin MurphyFrom: Zhen Lei <[email protected]>
Non-strict mode is simply a case of skipping 'regular' leaf TLBIs, since the sync is already factored out into ops->iotlb_sync at the core API level. Non-leaf invalidations where we change the page table structure itself still have to be issued synchronously in order to maintain walk caches correctly.
To save having to reason about it too much, make sure the invalidation in arm_lpae_split_blk_unmap() just performs its own unconditional sync to minimise the window in which we're technically violating the break- before-make requirement on a live mapping. This might work out redundant with an outer-level sync for strict unmaps, but we'll never be splitting blocks on a DMA fastpath anyway.
---
v8: Add barrier for the fiddly cross-cpu flush case
drivers/iommu/io-pgtable-arm.c | 14 ++++++++++++-- drivers/iommu/io-pgtable.h | 5 +++++ 2 files changed, 17 insertions(+), 2 deletions(-)
drivers/iommu/io-pgtable-arm.c +12 -2
@@ -576,6 +576,7 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data, tablep =iopte_deref (pte, data); }else if (unmap_idx >=0 ) {io_pgtable_tlb_add_flush (&data->iop, iova, size, size, true); +io_pgtable_tlb_sync (&data->iop);return size; }
@@ -609,6 +610,13 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,io_pgtable_tlb_sync (iop); ptep =iopte_deref (pte, data);__arm_lpae_free_pgtable (data, lvl +1 , ptep); + }else if (iop->cfg.quirks &IO_PGTABLE_QUIRK_NON_STRICT ) { +/* +* Order the PTE update against queueing the IOVA, to +* guarantee that a flush callback from a different CPU +* has observed it before the TLBIALL can be issued. +*/ +smp_wmb (); }else {io_pgtable_tlb_add_flush (iop, iova, size, size, true); }
@@ -771,7 +779,8 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)u64 reg;struct arm_lpae_io_pgtable *data; -if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS |IO_PGTABLE_QUIRK_NO_DMA )) +if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS |IO_PGTABLE_QUIRK_NO_DMA | +IO_PGTABLE_QUIRK_NON_STRICT ))return NULL ; data =arm_lpae_alloc_pgtable (cfg);
@@ -863,7 +872,8 @@ arm_64_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, void *cookie)struct arm_lpae_io_pgtable *data;/* The NS quirk doesn't apply at stage 2 */ -if (cfg->quirks & ~IO_PGTABLE_QUIRK_NO_DMA ) +if (cfg->quirks & ~(IO_PGTABLE_QUIRK_NO_DMA | +IO_PGTABLE_QUIRK_NON_STRICT ))return NULL ; data =arm_lpae_alloc_pgtable (cfg);
drivers/iommu/io-pgtable.h +5 -0
@@ -71,12 +71,17 @@ struct io_pgtable_cfg {* be accessed by a fully cache-coherent IOMMU or CPU (e.g. for a * software-emulated IOMMU), such that pagetable updates need not * be treated as explicit DMA data. +* +* IO_PGTABLE_QUIRK_NON_STRICT: Skip issuing synchronous leaf TLBIs +* on unmap, for DMA domains using the flush queue mechanism for +* delayed invalidation. */ #define IO_PGTABLE_QUIRK_ARM_NS BIT(0)#define IO_PGTABLE_QUIRK_NO_PERMS BIT(1)#define IO_PGTABLE_QUIRK_TLBI_ON_MAP BIT(2)#define IO_PGTABLE_QUIRK_ARM_MTK_4GB BIT(3)#define IO_PGTABLE_QUIRK_NO_DMA BIT(4) +#define IO_PGTABLE_QUIRK_NON_STRICT BIT(5)unsigned long quirks;unsigned long pgsize_bitmap;unsigned int ias;
Signature · 1 line
2.19.0.dirty
Robin Murphy
maintainerPATCH v8 5/7 iommu/arm-smmu-v3: Add support for non-strict mode
In reply to Robin MurphyFrom: Zhen Lei <[email protected]>
Now that io-pgtable knows how to dodge strict TLB maintenance, all that's left to do is bridge the gap between the IOMMU core requesting DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE for default domains, and showing the appropriate IO_PGTABLE_QUIRK_NON_STRICT flag to alloc_io_pgtable_ops().
---
v8:
- Use nested switches for attrs
- Document barrier semantics
drivers/iommu/arm-smmu-v3.c | 79 ++++++++++++++++++++++++++----------- 1 file changed, 56 insertions(+), 23 deletions(-)
drivers/iommu/arm-smmu-v3.c +56 -23
@@ -612,6 +612,7 @@ struct arm_smmu_domain {struct mutex init_mutex;/* Protects smmu pointer */ struct io_pgtable_ops *pgtbl_ops; +bool non_strict;enum arm_smmu_domain_stage stage;union {
@@ -1407,6 +1408,12 @@ static void arm_smmu_tlb_inv_context(void *cookie) cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; } +/* +* NOTE: when io-pgtable is in non-strict mode, we may get here with +* PTEs previously cleared by unmaps on the current CPU not yet visible +* to the SMMU. We are relying on the DSB implicit in queue_inc_prod() +* to guarantee those are observed before the TLBI. Do be careful, 007. +*/ arm_smmu_cmdq_issue_cmd (smmu, &cmd);__arm_smmu_tlb_sync (smmu); }
@@ -1633,6 +1640,9 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)if (smmu->features &ARM_SMMU_FEAT_COHERENCY ) pgtbl_cfg.quirks =IO_PGTABLE_QUIRK_NO_DMA ; +if (smmu_domain->non_strict) + pgtbl_cfg.quirks |=IO_PGTABLE_QUIRK_NON_STRICT ; + pgtbl_ops =alloc_io_pgtable_ops (fmt, &pgtbl_cfg, smmu_domain);if (!pgtbl_ops)return -ENOMEM ;
@@ -1934,15 +1944,27 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain, {struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain); -if (domain->type !=IOMMU_DOMAIN_UNMANAGED ) -return -EINVAL ; - -switch (attr) { -case DOMAIN_ATTR_NESTING : - *(int *)data = (smmu_domain->stage ==ARM_SMMU_DOMAIN_NESTED ); -return 0 ; +switch (domain->type) { +case IOMMU_DOMAIN_UNMANAGED : +switch (attr) { +case DOMAIN_ATTR_NESTING : + *(int *)data = (smmu_domain->stage ==ARM_SMMU_DOMAIN_NESTED ); +return 0 ; +default : +return -ENODEV ; + } +break ; +case IOMMU_DOMAIN_DMA : +switch (attr) { +case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE : + *(int *)data = smmu_domain->non_strict; +return 0 ; +default : +return -ENODEV ; + } +break ;default : -return -ENODEV ; +return -EINVAL ; } }
@@ -1952,26 +1974,37 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,int ret =0 ;struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain); -if (domain->type !=IOMMU_DOMAIN_UNMANAGED ) -return -EINVAL ; -mutex_lock (&smmu_domain->init_mutex); -switch (attr) { -case DOMAIN_ATTR_NESTING : -if (smmu_domain->smmu) { - ret = -EPERM ; - goto out_unlock; +switch (domain->type) { +case IOMMU_DOMAIN_UNMANAGED : +switch (attr) { +case DOMAIN_ATTR_NESTING : +if (smmu_domain->smmu) { + ret = -EPERM ; + goto out_unlock; + } + +if (*(int *)data) + smmu_domain->stage =ARM_SMMU_DOMAIN_NESTED ; +else + smmu_domain->stage =ARM_SMMU_DOMAIN_S1 ; +break ; +default : + ret = -ENODEV ; + } +break ; +case IOMMU_DOMAIN_DMA : +switch (attr) { +case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE : + smmu_domain->non_strict = *(int *)data; +break ; +default : + ret = -ENODEV ; } - -if (*(int *)data) - smmu_domain->stage =ARM_SMMU_DOMAIN_NESTED ; -else - smmu_domain->stage =ARM_SMMU_DOMAIN_S1 ; -break ;default : - ret = -ENODEV ; + ret = -EINVAL ; } out_unlock:
Signature · 1 line
2.19.0.dirty
Robin Murphy
maintainerPATCH v8 6/7 iommu/io-pgtable-arm-v7s: Add support for non-strict mode
In reply to Robin MurphyAs for LPAE, it's simply a case of skipping the leaf invalidation for a regular unmap, and ensuring that the one in split_blk_unmap() is paired with an explicit sync ASAP rather than relying on one which might only eventually happen way down the line.
---
v8: Do this properly instead of just hacking around not doing it
(look, it turns out barely any more complicated!)
drivers/iommu/io-pgtable-arm-v7s.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
drivers/iommu/io-pgtable-arm-v7s.c +10 -1
@@ -587,6 +587,7 @@ static size_t arm_v7s_split_blk_unmap(struct arm_v7s_io_pgtable *data, }io_pgtable_tlb_add_flush (&data->iop, iova, size, size, true); +io_pgtable_tlb_sync (&data->iop);return size; }
@@ -642,6 +643,13 @@ static size_t __arm_v7s_unmap(struct arm_v7s_io_pgtable *data,io_pgtable_tlb_sync (iop); ptep =iopte_deref (pte[i], lvl);__arm_v7s_free_table (ptep, lvl +1 , data); + }else if (iop->cfg.quirks &IO_PGTABLE_QUIRK_NON_STRICT ) { +/* +* Order the PTE update against queueing the IOVA, to +* guarantee that a flush callback from a different CPU +* has observed it before the TLBIALL can be issued. +*/ +smp_wmb (); }else {io_pgtable_tlb_add_flush (iop, iova, blk_size, blk_size, true);
@@ -712,7 +720,8 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,IO_PGTABLE_QUIRK_NO_PERMS |IO_PGTABLE_QUIRK_TLBI_ON_MAP |IO_PGTABLE_QUIRK_ARM_MTK_4GB | - IO_PGTABLE_QUIRK_NO_DMA)) +IO_PGTABLE_QUIRK_NO_DMA | + IO_PGTABLE_QUIRK_NON_STRICT)) return NULL;/* If ARM_MTK_4GB is enabled, the NO_PERMS is also expected. */
Signature · 1 line
2.19.0.dirty
Robin Murphy
maintainerPATCH v8 7/7 iommu/arm-smmu: Support non-strict mode
In reply to Robin MurphyAll we need is to wire up .flush_iotlb_all properly and implement the domain attribute, and iommu-dma and io-pgtable will do the rest for us. The only real subtlety is documenting the barrier semantics we're introducing between io-pgtable and the drivers for non-strict flushes.
---
v8:
- Use nested switches for attrs
- Fix barriers
drivers/iommu/arm-smmu.c | 93 ++++++++++++++++++++++++++++------------ 1 file changed, 66 insertions(+), 27 deletions(-)
drivers/iommu/arm-smmu.c +66 -27
@@ -246,6 +246,7 @@ struct arm_smmu_domain {const struct iommu_gather_ops *tlb_ops;struct arm_smmu_cfg cfg;enum arm_smmu_domain_stage stage; +bool non_strict;struct mutex init_mutex;/* Protects smmu pointer */ spinlock_t cb_lock;/* Serialises ATS1* ops and TLB syncs */ struct iommu_domain domain;
@@ -447,7 +448,11 @@ static void arm_smmu_tlb_inv_context_s1(void *cookie)struct arm_smmu_cfg *cfg = &smmu_domain->cfg;void __iomem *base =ARM_SMMU_CB (smmu_domain->smmu, cfg->cbndx); -writel_relaxed (cfg->asid, base +ARM_SMMU_CB_S1_TLBIASID ); +/* +* NOTE: this is not a relaxed write; it needs to guarantee that PTEs +* cleared by the current CPU are visible to the SMMU before the TLBI. +*/ +writel (cfg->asid, base +ARM_SMMU_CB_S1_TLBIASID );arm_smmu_tlb_sync_context (cookie); }
@@ -457,7 +462,8 @@ static void arm_smmu_tlb_inv_context_s2(void *cookie)struct arm_smmu_device *smmu = smmu_domain->smmu;void __iomem *base =ARM_SMMU_GR0 (smmu); -writel_relaxed (smmu_domain->cfg.vmid, base +ARM_SMMU_GR0_TLBIVMID ); +/* NOTE: see above */ +writel (smmu_domain->cfg.vmid, base +ARM_SMMU_GR0_TLBIVMID );arm_smmu_tlb_sync_global (smmu); }
@@ -863,6 +869,9 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,if (smmu->features &ARM_SMMU_FEAT_COHERENT_WALK ) pgtbl_cfg.quirks =IO_PGTABLE_QUIRK_NO_DMA ; +if (smmu_domain->non_strict) + pgtbl_cfg.quirks |=IO_PGTABLE_QUIRK_NON_STRICT ; + smmu_domain->smmu = smmu; pgtbl_ops =alloc_io_pgtable_ops (fmt, &pgtbl_cfg, smmu_domain);if (!pgtbl_ops) {
@@ -1252,6 +1261,14 @@ static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,return ops->unmap (ops, iova, size); } +static void arm_smmu_flush_iotlb_all (struct iommu_domain *domain) +{ +struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain); + +if (smmu_domain->tlb_ops) + smmu_domain->tlb_ops->tlb_flush_all (smmu_domain); +} +static void arm_smmu_iotlb_sync (struct iommu_domain *domain) {struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain);
@@ -1470,15 +1487,27 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain, {struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain); -if (domain->type !=IOMMU_DOMAIN_UNMANAGED ) -return -EINVAL ; - -switch (attr) { -case DOMAIN_ATTR_NESTING : - *(int *)data = (smmu_domain->stage ==ARM_SMMU_DOMAIN_NESTED ); -return 0 ; +switch (domain->type) { +case IOMMU_DOMAIN_UNMANAGED : +switch (attr) { +case DOMAIN_ATTR_NESTING : + *(int *)data = (smmu_domain->stage ==ARM_SMMU_DOMAIN_NESTED ); +return 0 ; +default : +return -ENODEV ; + } +break ; +case IOMMU_DOMAIN_DMA : +switch (attr) { +case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE : + *(int *)data = smmu_domain->non_strict; +return 0 ; +default : +return -ENODEV ; + } +break ;default : -return -ENODEV ; +return -EINVAL ; } }
@@ -1488,28 +1517,38 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,int ret =0 ;struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain); -if (domain->type !=IOMMU_DOMAIN_UNMANAGED ) -return -EINVAL ; -mutex_lock (&smmu_domain->init_mutex); -switch (attr) { -case DOMAIN_ATTR_NESTING : -if (smmu_domain->smmu) { - ret = -EPERM ; - goto out_unlock; +switch (domain->type) { +case IOMMU_DOMAIN_UNMANAGED : +switch (attr) { +case DOMAIN_ATTR_NESTING : +if (smmu_domain->smmu) { + ret = -EPERM ; + goto out_unlock; + } + +if (*(int *)data) + smmu_domain->stage =ARM_SMMU_DOMAIN_NESTED ; +else + smmu_domain->stage =ARM_SMMU_DOMAIN_S1 ; +break ; +default : + ret = -ENODEV ; + } +break ; +case IOMMU_DOMAIN_DMA : +switch (attr) { +case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE : + smmu_domain->non_strict = *(int *)data; +break ; +default : + ret = -ENODEV ; } - -if (*(int *)data) - smmu_domain->stage =ARM_SMMU_DOMAIN_NESTED ; -else - smmu_domain->stage =ARM_SMMU_DOMAIN_S1 ; -break ;default : - ret = -ENODEV ; + ret = -EINVAL ; } - out_unlock:mutex_unlock (&smmu_domain->init_mutex);return ret;
@@ -1562,7 +1601,7 @@ static struct iommu_ops arm_smmu_ops = { .attach_dev = arm_smmu_attach_dev, .map = arm_smmu_map, .unmap = arm_smmu_unmap, - .flush_iotlb_all = arm_smmu_iotlb_sync, + .flush_iotlb_all = arm_smmu_flush_iotlb_all, .iotlb_sync = arm_smmu_iotlb_sync, .iova_to_phys = arm_smmu_iova_to_phys, .add_device = arm_smmu_add_device,
Signature · 1 line
2.19.0.dirty
Quoted 9 lines · Robin Murphy
Hi all,
Hopefully this is the last spin of the series - I've now dropped my light touch and fixed up all the various prose text, plus implemented the proper quirk support for short-descriptor because it's actually just a trivial cut-and-paste job.
Robin.
Hi Robin,
JFYI, I'm trying to test this patchset to get some figures and provide a tested-by tag, but 4/8 seems to rely on "iommu/io-pgtable-arm: Fix race handling in split_blk_unmap()" - more specifically, it seems to rely on the version which Will rewrote in your patch review, and I am not sure on what branch it exists on, if any.
Thanks,
John
Quoted 23 lines
Robin Murphy (2):
iommu/io-pgtable-arm-v7s: Add support for non-strict mode iommu/arm-smmu: Support non-strict modeZhen Lei (5):
iommu/arm-smmu-v3: Implementflush_iotlb_allhook iommu/dma: Add support for non-strict mode
iommu: Add "iommu.strict" command line option iommu/io-pgtable-arm: Add support for non-strict mode iommu/arm-smmu-v3: Add support for non-strict mode.../admin-guide/kernel-parameters.txt | 12 +++ drivers/iommu/arm-smmu-v3.c | 89 +++++++++++++----- drivers/iommu/arm-smmu.c | 93 +++++++++++++------ drivers/iommu/dma-iommu.c | 32 ++++++- drivers/iommu/io-pgtable-arm-v7s.c | 11 ++- drivers/iommu/io-pgtable-arm.c | 14 ++- drivers/iommu/io-pgtable.h | 5 + drivers/iommu/iommu.c | 23 +++++ include/linux/iommu.h | 1 + 9 files changed, 225 insertions(+), 55 deletions(-)
Robin Murphy
maintainerHi John,
Quoted 12 lines · John Garry
Quoted 10 lines · Robin Murphy
Hi all,
Hopefully this is the last spin of the series - I've now dropped my light touch and fixed up all the various prose text, plus implemented the proper
quirk support for short-descriptor because it's actually just a trivial cut-and-paste job.Robin.
Quoted 7 lines
Hi Robin,
JFYI, I'm trying to test this patchset to get some figures and provide a tested-by tag, but 4/8 seems to rely on "iommu/io-pgtable-arm: Fix race handling in
split_blk_unmap()" - more specifically, it seems to rely on the version which Will rewrote in your patch review, and I am not sure on what branch it exists on, if any.
Sorry, I should have said this is based on Will's iommu/devel branch:
https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=iommu/devel
Robin.
Quoted 29 lines
Thanks,
JohnQuoted 23 lines
Robin Murphy (2):
? iommu/io-pgtable-arm-v7s: Add support for non-strict mode ? iommu/arm-smmu: Support non-strict modeZhen Lei (5):
? iommu/arm-smmu-v3: Implementflush_iotlb_allhook ? iommu/dma: Add support for non-strict mode
? iommu: Add "iommu.strict" command line option ? iommu/io-pgtable-arm: Add support for non-strict mode ? iommu/arm-smmu-v3: Add support for non-strict mode?.../admin-guide/kernel-parameters.txt???????? | 12 +++ ?drivers/iommu/arm-smmu-v3.c?????????????????? | 89 +++++++++++++----- ?drivers/iommu/arm-smmu.c????????????????????? | 93 +++++++++++++------ ?drivers/iommu/dma-iommu.c???????????????????? | 32 ++++++- ?drivers/iommu/io-pgtable-arm-v7s.c??????????? | 11 ++- ?drivers/iommu/io-pgtable-arm.c??????????????? | 14 ++- ?drivers/iommu/io-pgtable.h??????????????????? |? 5 + ?drivers/iommu/iommu.c???????????????????????? | 23 +++++ ?include/linux/iommu.h???????????????????????? |? 1 +?9 files changed, 225 insertions(+), 55 deletions(-)
Robin Murphy
maintainerQuoted 24 lines · Robin Murphy
Hi John,
Quoted 13 lines · John Garry
Quoted 11 lines · Robin Murphy
Hi all,
Hopefully this is the last spin of the series - I've now dropped my light
touch and fixed up all the various prose text, plus implemented the proper
quirk support for short-descriptor because it's actually just a trivial cut-and-paste job.Robin.
Quoted 7 lines
Hi Robin,
JFYI, I'm trying to test this patchset to get some figures and provide a tested-by tag, but 4/8 seems to rely on "iommu/io-pgtable-arm: Fix race handling in
split_blk_unmap()" - more specifically, it seems to rely on the version which Will rewrote in your patch review, and I am not sure on what branch it exists on, if any.
Quoted 3 lines
Sorry, I should have said this is based on Will's iommu/devel branch:
https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=iommu/devel
FWIW I've now pushed out a complete branch here:
git://linux-arm.org/linux-rm iommu/non-strict
Robin.
John Garry
Tested-byQuoted 26 lines · Robin Murphy
Quoted 25 lines · Robin Murphy
Hi John,
Quoted 21 lines · John Garry
Quoted 11 lines · Robin Murphy
Hi all,
Hopefully this is the last spin of the series - I've now dropped my light
touch and fixed up all the various prose text, plus implemented the proper
quirk support for short-descriptor because it's actually just a trivial cut-and-paste job.Robin.
Hi Robin,
JFYI, I'm trying to test this patchset to get some figures and provide a tested-by tag, but 4/8 seems to rely on "iommu/io-pgtable-arm: Fix race handling in
split_blk_unmap()" - more specifically, it seems to rely on the version which Will rewrote in your patch review, and I am not sure on what branch it exists on, if any.
Quoted 9 lines
Quoted 3 lines
Sorry, I should have said this is based on Will's iommu/devel branch:
https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=iommu/devel
FWIW I've now pushed out a complete branch here:
git://linux-arm.org/linux-rmiommu/non-strict
Cheers
So for my network test scenario I was getting a boost @ 250K vs 160K packet(s)/second with strict off/on
For NVMe single disk performance, I was getting a boost @ 582K vs 370K IOPS with strict off/on.
I wasn't seeing such a boost for other storage controller scenario (that's with 6 SSDs), with 776K vs 740K IOPS for strict off/on, but SMMU off was ~800K IOPS.
FWIW:
Thanks,
John
Quoted 5 lines
Robin.
.
Re: [PATCH v8 4/7] iommu/io-pgtable-arm: Add support for non-strict mode
In reply to Robin MurphyQuoted 39 lines · Robin Murphy
From: Zhen Lei <[email protected]>
Non-strict mode is simply a case of skipping 'regular' leaf TLBIs, since the sync is already factored out into
ops->iotlb_syncat the core API level. Non-leaf invalidations where we change the page table structure itself still have to be issued synchronously in order to maintain walk caches correctly.To save having to reason about it too much, make sure the invalidation in
arm_lpae_split_blk_unmap()just performs its own unconditional sync to minimise the window in which we're technically violating the break- before-make requirement on a live mapping. This might work out redundant with an outer-level sync for strict unmaps, but we'll never be splitting blocks on a DMA fastpath anyway.Signed-off-by Zhen Lei <[email protected]>[rm: tweak comment, commit message, split_blk_unmap logic and barriers]Signed-off-by Robin Murphy <[email protected]>---
v8: Add barrier for the fiddly cross-cpu flush case
drivers/iommu/io-pgtable-arm.c | 14 ++++++++++++-- drivers/iommu/io-pgtable.h | 5 +++++ 2 files changed, 17 insertions(+), 2 deletions(-)drivers/iommu/io-pgtable-arm.c +1
-0@@ -576,6 +576,7 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data, tablep =iopte_deref (pte, data); }else if (unmap_idx >=0 ) {io_pgtable_tlb_add_flush (&data->iop, iova, size, size, true); +io_pgtable_tlb_sync (&data->iop);return size; }
drivers/iommu/io-pgtable-arm.c static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data, · line 619
Quoted 10 lines
drivers/iommu/io-pgtable-arm.c +7
-0@@ -609,6 +610,13 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,io_pgtable_tlb_sync (iop); ptep =iopte_deref (pte, data);__arm_lpae_free_pgtable (data, lvl +1 , ptep); + }else if (iop->cfg.quirks &IO_PGTABLE_QUIRK_NON_STRICT ) { +/* +* Order the PTE update against queueing the IOVA, to +* guarantee that a flush callback from a different CPU +* has observed it before the TLBIALL can be issued. +*/ +smp_wmb ();
Looks good to me. In the case that everything happens on the same CPU, are we relying on the TLB invalidation code in the SMMU driver(s) to provide the DSB for pushing the new entry out to the walker?
Will
Re: [PATCH v8 5/7] iommu/arm-smmu-v3: Add support for non-strict mode
In reply to Robin MurphyQuoted 32 lines · Robin Murphy
From: Zhen Lei <[email protected]>
Now that io-pgtable knows how to dodge strict TLB maintenance, all that's left to do is bridge the gap between the IOMMU core requesting
DOMAIN_ATTR_DMA_USE_FLUSH_QUEUEfor default domains, and showing the appropriateIO_PGTABLE_QUIRK_NON_STRICTflag toalloc_io_pgtable_ops().Signed-off-by Zhen Lei <[email protected]>[rm: convert to domain attribute, tweak commit message]Signed-off-by Robin Murphy <[email protected]>---
v8:
- Use nested switches for attrs
- Document barrier semanticsdrivers/iommu/arm-smmu-v3.c | 79 ++++++++++++++++++++++++++----------- 1 file changed, 56 insertions(+), 23 deletions(-)drivers/iommu/arm-smmu-v3.c +1
-0@@ -612,6 +612,7 @@ struct arm_smmu_domain {struct mutex init_mutex;/* Protects smmu pointer */ struct io_pgtable_ops *pgtbl_ops; +bool non_strict;enum arm_smmu_domain_stage stage;union {
drivers/iommu/arm-smmu-v3.c static void arm_smmu_tlb_inv_context(void *cookie) · line 1416
Quoted 9 lines
drivers/iommu/arm-smmu-v3.c +6
-0@@ -1407,6 +1408,12 @@ static void arm_smmu_tlb_inv_context(void *cookie) cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; } +/* +* NOTE: when io-pgtable is in non-strict mode, we may get here with +* PTEs previously cleared by unmaps on the current CPU not yet visible +* to the SMMU. We are relying on the DSB implicit in queue_inc_prod() +* to guarantee those are observed before the TLBI. Do be careful, 007. +*/
Good, so you can ignore my comment on the previous patch :)
Will
Robin Murphy
maintainerRe: [PATCH v8 5/7] iommu/arm-smmu-v3: Add support for non-strict mode
Quoted 36 lines · Will Deacon
Quoted 35 lines · Robin Murphy
From: Zhen Lei <[email protected]>
Now that io-pgtable knows how to dodge strict TLB maintenance, all that's left to do is bridge the gap between the IOMMU core requesting
DOMAIN_ATTR_DMA_USE_FLUSH_QUEUEfor default domains, and showing the appropriateIO_PGTABLE_QUIRK_NON_STRICTflag toalloc_io_pgtable_ops().Signed-off-by Zhen Lei <[email protected]>[rm: convert to domain attribute, tweak commit message]Signed-off-by Robin Murphy <[email protected]>---
v8:
- Use nested switches for attrs
- Document barrier semanticsdrivers/iommu/arm-smmu-v3.c | 79 ++++++++++++++++++++++++++----------- 1 file changed, 56 insertions(+), 23 deletions(-)drivers/iommu/arm-smmu-v3.c +1
-0@@ -612,6 +612,7 @@ struct arm_smmu_domain {struct mutex init_mutex;/* Protects smmu pointer */ struct io_pgtable_ops *pgtbl_ops; +bool non_strict;enum arm_smmu_domain_stage stage;union {@@ -1407,6 +1408,12 @@ static void arm_smmu_tlb_inv_context(void *cookie) cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; }
Quoted 8 lines
drivers/iommu/arm-smmu-v3.c static void arm_smmu_tlb_inv_context(void *cookie) · line 1416
Quoted 6 lines
drivers/iommu/arm-smmu-v3.c +6
-0+/* +* NOTE: when io-pgtable is in non-strict mode, we may get here with +* PTEs previously cleared by unmaps on the current CPU not yet visible +* to the SMMU. We are relying on the DSB implicit in queue_inc_prod() +* to guarantee those are observed before the TLBI. Do be careful, 007. +*/ Good, so you can ignore my comment on the previous patch :)
Well, I suppose that comment in io-pgtable *could* have explicitly noted that same-CPU order is dealt with elsewhere - feel free to fix it up if you think it would be a helpful reminder for the future.
Cheers,
Robin.
Re: [PATCH v8 5/7] iommu/arm-smmu-v3: Add support for non-strict mode
Quoted 43 lines · Robin Murphy
Quoted 33 lines · Will Deacon
Quoted 32 lines · Robin Murphy
From: Zhen Lei <[email protected]>
Now that io-pgtable knows how to dodge strict TLB maintenance, all that's left to do is bridge the gap between the IOMMU core requesting
DOMAIN_ATTR_DMA_USE_FLUSH_QUEUEfor default domains, and showing the appropriateIO_PGTABLE_QUIRK_NON_STRICTflag toalloc_io_pgtable_ops().Signed-off-by Zhen Lei <[email protected]>[rm: convert to domain attribute, tweak commit message]Signed-off-by Robin Murphy <[email protected]>---
v8:
- Use nested switches for attrs
- Document barrier semanticsdrivers/iommu/arm-smmu-v3.c | 79 ++++++++++++++++++++++++++----------- 1 file changed, 56 insertions(+), 23 deletions(-)drivers/iommu/arm-smmu-v3.c +1
-0@@ -612,6 +612,7 @@ struct arm_smmu_domain {struct mutex init_mutex;/* Protects smmu pointer */ struct io_pgtable_ops *pgtbl_ops; +bool non_strict;enum arm_smmu_domain_stage stage;union {@@ -1407,6 +1408,12 @@ static void arm_smmu_tlb_inv_context(void *cookie) cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; }Quoted 8 lines
drivers/iommu/arm-smmu-v3.c static void arm_smmu_tlb_inv_context(void *cookie) · line 1416
Quoted 6 lines
drivers/iommu/arm-smmu-v3.c +6
-0+/* +* NOTE: when io-pgtable is in non-strict mode, we may get here with +* PTEs previously cleared by unmaps on the current CPU not yet visible +* to the SMMU. We are relying on the DSB implicit in queue_inc_prod() +* to guarantee those are observed before the TLBI. Do be careful, 007. +*/ Good, so you can ignore my comment on the previous patch :)
Quoted 3 lines
Well, I suppose that comment in io-pgtable *could* have explicitly noted that same-CPU order is dealt with elsewhere - feel free to fix it up if you think it would be a helpful reminder for the future.
I think I'll move it into the documentation for the new attribute, so that any driver authors wanting to enable lazy invalidation know that they need to provide this guarantee in their full TLB invalidation callback.
Will
Re: [PATCH v8 3/7] iommu: Add "iommu.strict" command line option
In reply to Robin MurphyQuoted 60 lines · Robin Murphy
From: Zhen Lei <[email protected]>
Add a generic command line option to enable lazy unmapping via IOVA flush queues, which will initally be suuported by iommu-dma. This echoes the semantics of "intel_iommu=strict" (albeit with the opposite default value), but in the driver-agnostic fashion of "iommu.passthrough".
Signed-off-by Zhen Lei <[email protected]>[rm: move handling out of SMMUv3 driver, clean up documentation]Signed-off-by Robin Murphy <[email protected]>---
v8:
- Rename "non-strict" to "strict" to better match existing options
- Rewrite doc text/commit message
- Downgrade boot-time message from warn/taint to info.../admin-guide/kernel-parameters.txt | 12 ++++++++++ drivers/iommu/iommu.c | 23 +++++++++++++++++++ 2 files changed, 35 insertions(+)Documentation/admin-guide/kernel-parameters.txt +12
-0@@ -1749,6 +1749,18 @@ nobypass [PPC/POWERNV] Disable IOMMU bypass, using IOMMU for PCI devices. + iommu.strict= [ARM64] Configure TLB invalidation behaviour + Format: { "0" | "1" } + 0 - Lazy mode. + 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. + 1 - Strict mode (default). + DMA unmap operations invalidate IOMMU hardware TLBs + synchronously. + iommu.passthrough= [ARM64] Configure DMA to bypass the IOMMU by default. Format: { "0" | "1" }drivers/iommu/iommu.c +1
-0@@ -41,6 +41,7 @@ static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;#else static unsigned int iommu_def_domain_type =IOMMU_DOMAIN_DMA ;#endif +static bool iommu_dma_strict__read_mostly = true;struct iommu_callback_data {const struct iommu_ops *ops;@@ -131,6 +132,21 @@ static int __init iommu_set_def_domain_type(char *str) }early_param ("iommu.passthrough" , iommu_set_def_domain_type);
drivers/iommu/iommu.c static int __init iommu_set_def_domain_type(char *str) · line 144
Quoted 10 lines
drivers/iommu/iommu.c +10
-0+static int __init iommu_dma_setup (char *str) +{ +int ret; + + ret =kstrtobool (str, &iommu_dma_strict); +if (ret) +return ret; + +if (!iommu_dma_strict) +pr_info ("Enabling deferred TLB invalidation for DMA; protection against malicious/malfunctioning devices may be reduced.\n" );
Printing here isn't quite right, because if somebody boots with something like:
"iommu.strict=1 iommu.strict=0 iommu.strict=0 iommu.strict=1"
then we'll print the wrong thing twice :)
I think we either need to drop the print, or move it to a the DMA domain initialisation.
Will
Quoted 4 lines · Robin Murphy
Hopefully this is the last spin of the series - I've now dropped my light touch and fixed up all the various prose text, plus implemented the proper quirk support for short-descriptor because it's actually just a trivial cut-and-paste job.
Did you manage to clarify how the lifetime of the domain/flush queue interacts with the periodic timer after the cookie has been freed?
Will
Robin Murphy
maintainerQuoted 8 lines · Will Deacon
Quoted 4 lines · Robin Murphy
Hopefully this is the last spin of the series - I've now dropped my light touch and fixed up all the various prose text, plus implemented the proper quirk support for short-descriptor because it's actually just a trivial cut-and-paste job.
Did you manage to clarify how the lifetime of the domain/flush queue interacts with the periodic timer after the cookie has been freed?
Oh, yes - it turned out to be wrapped up in put_iova_domain() already (see free_iova_flush_queue()), so all looks good in that regard.
Robin.
Re: [PATCH v8 5/7] iommu/arm-smmu-v3: Add support for non-strict mode
In reply to Will DeaconQuoted 25 lines · Will Deacon
Quoted 24 lines · Robin Murphy
Quoted 22 lines · Will Deacon
Quoted 11 lines · Robin Murphy
drivers/iommu/arm-smmu-v3.c +1
-0@@ -612,6 +612,7 @@ struct arm_smmu_domain {struct mutex init_mutex;/* Protects smmu pointer */ struct io_pgtable_ops *pgtbl_ops; +bool non_strict;enum arm_smmu_domain_stage stage;union {drivers/iommu/arm-smmu-v3.c static void arm_smmu_tlb_inv_context(void *cookie) · line 1415
Quoted 8 lines
drivers/iommu/arm-smmu-v3.c +6
-0@@ -1407,6 +1408,12 @@ static void arm_smmu_tlb_inv_context(void *cookie) cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; } +/* +* NOTE: when io-pgtable is in non-strict mode, we may get here with +* PTEs previously cleared by unmaps on the current CPU not yet visible +* to the SMMU. We are relying on the DSB implicit in queue_inc_prod() +* to guarantee those are observed before the TLBI. Do be careful, 007. +*/ Good, so you can ignore my comment on the previous patch :)
Quoted 7 lines
Quoted 3 lines
Well, I suppose that comment in io-pgtable *could* have explicitly noted that same-CPU order is dealt with elsewhere - feel free to fix it up if you think it would be a helpful reminder for the future.
I think I'll move it into the documentation for the new attribute, so that any driver authors wanting to enable lazy invalidation know that they need to provide this guarantee in their full TLB invalidation callback.
Hmm, so I started doing this but then realised we already required this behaviour for tlb_add_flush() afaict. That would mean that mainline currently has a bug for arm-smmu.c, because we use the _relaxed I/O accessors in there and there's no DSB after clearing the PTE on unmap().
Am I missing something?
Will
Robin Murphy
maintainerRe: [PATCH v8 5/7] iommu/arm-smmu-v3: Add support for non-strict mode
Quoted 34 lines · Will Deacon
Quoted 25 lines · Will Deacon
Quoted 24 lines · Robin Murphy
Quoted 22 lines · Will Deacon
Quoted 11 lines · Robin Murphy
drivers/iommu/arm-smmu-v3.c +1
-0@@ -612,6 +612,7 @@ struct arm_smmu_domain {struct mutex init_mutex;/* Protects smmu pointer */ struct io_pgtable_ops *pgtbl_ops; +bool non_strict;enum arm_smmu_domain_stage stage;union {drivers/iommu/arm-smmu-v3.c static void arm_smmu_tlb_inv_context(void *cookie) · line 1415
Quoted 8 lines
drivers/iommu/arm-smmu-v3.c +6
-0@@ -1407,6 +1408,12 @@ static void arm_smmu_tlb_inv_context(void *cookie) cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; } +/* +* NOTE: when io-pgtable is in non-strict mode, we may get here with +* PTEs previously cleared by unmaps on the current CPU not yet visible +* to the SMMU. We are relying on the DSB implicit in queue_inc_prod() +* to guarantee those are observed before the TLBI. Do be careful, 007. +*/ Good, so you can ignore my comment on the previous patch :)
Quoted 7 lines
Quoted 3 lines
Well, I suppose that comment in io-pgtable *could* have explicitly noted that same-CPU order is dealt with elsewhere - feel free to fix it up if you think it would be a helpful reminder for the future.
I think I'll move it into the documentation for the new attribute, so that any driver authors wanting to enable lazy invalidation know that they need to provide this guarantee in their full TLB invalidation callback.
Quoted 6 lines
Hmm, so I started doing this but then realised we already required this behaviour for
tlb_add_flush()afaict. That would mean that mainline currently has a bug forarm-smmu.c, because we use the_relaxedI/O accessors in there and there's no DSB after clearing the PTE onunmap().Am I missing something?
Argh, no - I started having the same suspicion when changing those writel()s in patch #7, resolved to either mention it to you or investigate it myself as a separate fix, then promptly forgot entirely. Thanks for the reminder...
Robin.
Robin Murphy
maintainerRe: [PATCH v8 3/7] iommu: Add "iommu.strict" command line option
In reply to Will DeaconQuoted 75 lines · Will Deacon
Quoted 60 lines · Robin Murphy
From: Zhen Lei <[email protected]>
Add a generic command line option to enable lazy unmapping via IOVA flush queues, which will initally be suuported by iommu-dma. This echoes the semantics of "intel_iommu=strict" (albeit with the opposite default value), but in the driver-agnostic fashion of "iommu.passthrough".
Signed-off-by Zhen Lei <[email protected]>[rm: move handling out of SMMUv3 driver, clean up documentation]Signed-off-by Robin Murphy <[email protected]>---
v8:
- Rename "non-strict" to "strict" to better match existing options
- Rewrite doc text/commit message
- Downgrade boot-time message from warn/taint to info.../admin-guide/kernel-parameters.txt | 12 ++++++++++ drivers/iommu/iommu.c | 23 +++++++++++++++++++ 2 files changed, 35 insertions(+)Documentation/admin-guide/kernel-parameters.txt +12
-0@@ -1749,6 +1749,18 @@ nobypass [PPC/POWERNV] Disable IOMMU bypass, using IOMMU for PCI devices. + iommu.strict= [ARM64] Configure TLB invalidation behaviour + Format: { "0" | "1" } + 0 - Lazy mode. + 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. + 1 - Strict mode (default). + DMA unmap operations invalidate IOMMU hardware TLBs + synchronously. + iommu.passthrough= [ARM64] Configure DMA to bypass the IOMMU by default. Format: { "0" | "1" }drivers/iommu/iommu.c +1
-0@@ -41,6 +41,7 @@ static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;#else static unsigned int iommu_def_domain_type =IOMMU_DOMAIN_DMA ;#endif +static bool iommu_dma_strict__read_mostly = true;struct iommu_callback_data {const struct iommu_ops *ops;@@ -131,6 +132,21 @@ static int __init iommu_set_def_domain_type(char *str) }early_param ("iommu.passthrough" , iommu_set_def_domain_type);drivers/iommu/iommu.c static int __init iommu_set_def_domain_type(char *str) · line 144
Quoted 10 lines
drivers/iommu/iommu.c +10
-0+static int __init iommu_dma_setup (char *str) +{ +int ret; + + ret =kstrtobool (str, &iommu_dma_strict); +if (ret) +return ret; + +if (!iommu_dma_strict) +pr_info ("Enabling deferred TLB invalidation for DMA; protection against malicious/malfunctioning devices may be reduced.\n" );Printing here isn't quite right, because if somebody boots with something like:
Quoted 3 lines
"iommu.strict=1 iommu.strict=0 iommu.strict=0 iommu.strict=1"
then we'll print the wrong thing twice :)
But it's not wrong! For those two brief moments it *is* enabled :P
For reasons of conciseness, the subtlety that "enabled" doesn't necessarily imply "in use" is only conveyed by the "may".
Quoted 2 lines
I think we either need to drop the print, or move it to a the DMA domain initialisation.
TBH I did toy with moving it around, but in the end it seemed neatest to have it right there next to the parameter handling, with the added advantage that it appears early in the log where system-wide things might be expected to appear, rather than mixed in with all the driver noise later.
AFAICS it's no worse than various other parameters - try booting with, say, "mem=640k mem=1G mem=cheese" and one finds that memory is in fact not limited (nor indeed cheese) regardless of what the log might say.
Robin.