Add non-strict mode support for iommu-dma
Subsystems (4)
Robin Murphy
maintainerv7 cover Add non-strict mode support for iommu-dma
Hi all,
Since we'd like to get this polished up and merged and Leizhen has other commitments, here's v7 of the previous series[1] wherein I address all my own feedback :) This is a quick tweak of the v6 I sent yesterday since I figured out slightly too late a much neater way of setting the attribute at the appropriate time.
The principal change is that I've inverted things slightly such that it's now a generic domain attribute controlled by iommu-dma given the necessary support from individual IOMMU drivers. That way we can easily enable other drivers straight away, as I've done for SMMUv2 here (which also allowed me to give it a quick test with MMU-401s on a Juno board). Otherwise it's really just cosmetic cleanup and rebasing onto Will's pending SMMU queue.
Robin.
[1] https://www.mail-archive.com/iommu at lists.linux-foundation.org/msg25150.html
Robin Murphy (1):
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/io-pgtable-arm: Add support for non-strict mode
iommu: Add bootup option "iommu.non_strict" iommu/arm-smmu-v3: Add support for non-strict mode
.../admin-guide/kernel-parameters.txt | 13 ++++++ drivers/iommu/arm-smmu-v3.c | 40 +++++++++++++++---- drivers/iommu/arm-smmu.c | 40 +++++++++++++++---- drivers/iommu/dma-iommu.c | 29 +++++++++++++- drivers/iommu/io-pgtable-arm.c | 9 +++-- drivers/iommu/io-pgtable.h | 5 +++ drivers/iommu/iommu.c | 26 ++++++++++++ include/linux/iommu.h | 1 + 8 files changed, 145 insertions(+), 18 deletions(-)
Signature · 1 line
2.19.0.dirty
Robin Murphy
maintainerPATCH v7 1/6 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 v7 2/6 iommu/dma: Add support for non-strict mode
In reply to Robin MurphyFrom: Zhen Lei <[email protected]>
1. Save the related domain pointer in struct iommu_dma_cookie, make iovad capable call domain->ops->flush_iotlb_all to flush TLB.
2. During the iommu domain initialization phase, base on domain->non_strict field to check whether non-strict mode is supported or not. If so, call init_iova_flush_queue to register iovad->flush_cb callback.
3. All unmap(contains iova-free) APIs will finally invoke __iommu_dma_unmap -->iommu_dma_free_iova. If the domain is non-strict, call queue_iova to put off iova freeing, and omit iommu_tlb_sync operation.
--- drivers/iommu/dma-iommu.c | 29 ++++++++++++++++++++++++++++- include/linux/iommu.h | 1 + 2 files changed, 29 insertions(+), 1 deletion(-)
drivers/iommu/dma-iommu.c +28 -1
@@ -55,6 +55,9 @@ struct iommu_dma_cookie { };struct list_head msi_page_list;spinlock_t msi_lock; + +/* Only be assigned in non-strict mode, otherwise it's NULL */ +struct iommu_domain *domain; };static inline size_t cookie_msi_granule (struct iommu_dma_cookie *cookie)
@@ -257,6 +260,17 @@ 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->domain; + + 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 +289,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 =1 ;if (!cookie || cookie->type !=IOMMU_DMA_IOVA_COOKIE )return -EINVAL ;
@@ -308,6 +323,13 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base, }init_iova_domain (iovad,1UL << order, base_pfn); + +if (!iommu_domain_get_attr (domain,DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE , + &attr) && attr) { + cookie->domain = domain; +init_iova_flush_queue (iovad, iommu_dma_flush_iotlb_all,NULL ); + } +if (!dev)return 0 ;
@@ -393,6 +415,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->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 +433,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->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 v7 3/6 iommu/io-pgtable-arm: Add support for non-strict mode
In reply to Robin MurphyFrom: Zhen Lei <[email protected]>
To support non-strict mode, now we only TLBI and sync for strict mode, except for non-leaf invalidations since page table updates themselves must always be synchronous.
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.
--- drivers/iommu/io-pgtable-arm.c | 9 ++++++--- drivers/iommu/io-pgtable.h | 5 +++++ 2 files changed, 11 insertions(+), 3 deletions(-)
drivers/iommu/io-pgtable-arm.c +6 -3
@@ -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,7 +610,7 @@ 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 { + }else if (!(iop->cfg.quirks &IO_PGTABLE_QUIRK_NON_STRICT )) {io_pgtable_tlb_add_flush (iop, iova, size, size, true); }
@@ -771,7 +772,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 +865,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 v7 4/6 iommu: Add bootup option "iommu.non_strict"
In reply to Robin MurphyFrom: Zhen Lei <[email protected]>
Add a bootup option to make the system manager can choose which mode to be used. The default mode is strict.
--- .../admin-guide/kernel-parameters.txt | 13 ++++++++++ drivers/iommu/iommu.c | 26 +++++++++++++++++++ 2 files changed, 39 insertions(+)
Documentation/admin-guide/kernel-parameters.txt +13 -0
@@ -1749,6 +1749,19 @@ nobypass [PPC/POWERNV] Disable IOMMU bypass, using IOMMU for PCI devices. + iommu.non_strict= [ARM64] + Format: { "0" | "1" } + 0 - strict mode, default. + Release IOVAs after the related TLBs are invalid + completely. + 1 - non-strict mode. + Put off TLBs invalidation and release memory first. + It's good for scatter-gather performance but lacks + full isolation, an untrusted device can access the + reused memory because the TLBs may still valid. + Please take full consideration before choosing this + mode. Note that, VFIO will always use strict mode. + iommu.passthrough= [ARM64] Configure DMA to bypass the IOMMU by default. Format: { "0" | "1" }
drivers/iommu/iommu.c +26 -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_non_strict__read_mostly ;struct iommu_callback_data {const struct iommu_ops *ops;
@@ -131,6 +132,24 @@ 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_non_strict); +if (ret) +return ret; + +if (iommu_dma_non_strict) { +pr_warn ("WARNING: iommu non-strict mode is chosen.\n" +"It's good for scatter-gather performance but lacks full isolation\n" ); +add_taint (TAINT_WARN ,LOCKDEP_STILL_OK ); + } + +return 0 ; +} +early_param ("iommu.non_strict" , iommu_dma_setup); +static ssize_t iommu_group_attr_show (struct kobject *kobj,struct attribute *__attr,char *buf) {
@@ -1072,6 +1091,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_non_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 v7 5/6 iommu/arm-smmu-v3: Add support for non-strict mode
In reply to Robin MurphyFrom: Zhen Lei <[email protected]>
Dynamically choose strict or non-strict mode for page table config based on the iommu domain type.
--- drivers/iommu/arm-smmu-v3.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)
drivers/iommu/arm-smmu-v3.c +24 -6
@@ -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 {
@@ -1633,6 +1634,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,13 +1938,17 @@ 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 : +if (domain->type !=IOMMU_DOMAIN_UNMANAGED ) +return -EINVAL ; *(int *)data = (smmu_domain->stage ==ARM_SMMU_DOMAIN_NESTED );return 0 ; +case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE : +if (domain->type !=IOMMU_DOMAIN_DMA ) +return -EINVAL ; + *(int *)data = smmu_domain->non_strict; +return 0 ;default :return -ENODEV ; }
@@ -1952,13 +1960,15 @@ 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 (domain->type !=IOMMU_DOMAIN_UNMANAGED ) { + ret = -EINVAL ; + goto out_unlock; + } +if (smmu_domain->smmu) { ret = -EPERM ; goto out_unlock;
@@ -1969,6 +1979,14 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,else smmu_domain->stage =ARM_SMMU_DOMAIN_S1 ; +break ; +case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE : +if (domain->type !=IOMMU_DOMAIN_DMA ) { + ret = -EINVAL ; + goto out_unlock; + } + + smmu_domain->non_strict = *(int *)data;break ;default : ret = -ENODEV ;
Signature · 1 line
2.19.0.dirty
Robin Murphy
maintainerPATCH v7 6/6 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-arm will do the rest for us. Rather than bother implementing it for v7s format for the highly unlikely chance of that being relevant, we can simply hide the non-strict flag from io-pgtable for that combination just so anyone who does actually try it will simply get over-invalidation instead of failure to initialise domains.
--- drivers/iommu/arm-smmu.c | 40 +++++++++++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 7 deletions(-)
drivers/iommu/arm-smmu.c +33 -7
@@ -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;
@@ -863,6 +864,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 && cfg->fmt !=ARM_SMMU_CTX_FMT_AARCH32_S ) + 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 +1256,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,13 +1482,17 @@ 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 : +if (domain->type !=IOMMU_DOMAIN_UNMANAGED ) +return -EINVAL ; *(int *)data = (smmu_domain->stage ==ARM_SMMU_DOMAIN_NESTED );return 0 ; +case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE : +if (domain->type !=IOMMU_DOMAIN_DMA ) +return -EINVAL ; + *(int *)data = smmu_domain->non_strict; +return 0 ;default :return -ENODEV ; }
@@ -1488,13 +1504,15 @@ 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 (domain->type !=IOMMU_DOMAIN_UNMANAGED ) { + ret = -EINVAL ; + goto out_unlock; + } +if (smmu_domain->smmu) { ret = -EPERM ; goto out_unlock;
@@ -1505,6 +1523,14 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,else smmu_domain->stage =ARM_SMMU_DOMAIN_S1 ; +break ; +case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE : +if (domain->type !=IOMMU_DOMAIN_DMA ) { + ret = -EINVAL ; + goto out_unlock; + } + + smmu_domain->non_strict = *(int *)data;break ;default : ret = -ENODEV ;
@@ -1562,7 +1588,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
Hi Robin,
Thanks for turning this around so quickly.
Quoted 6 lines · Robin Murphy
Since we'd like to get this polished up and merged and Leizhen has other commitments, here's v7 of the previous series[1] wherein I address all my own feedback :) This is a quick tweak of the v6 I sent yesterday since I figured out slightly too late a much neater way of setting the attribute at the appropriate time.
Quoted 7 lines
The principal change is that I've inverted things slightly such that it's now a generic domain attribute controlled by iommu-dma given the necessary support from individual IOMMU drivers. That way we can easily enable other drivers straight away, as I've done for SMMUv2 here (which also allowed me to give it a quick test with MMU-401s on a Juno board). Otherwise it's really just cosmetic cleanup and rebasing onto Will's pending SMMU queue.
I've been through and had a look, leaving some small comments on the patches themselves. The only part I failed to figure out is how you tie the lifetime of the flush queue to the lifetime of the domain so that the timer callback can't fire after e.g. the DMA cookie has been freed. How does that work?
Cheers,
Will
Re: [PATCH v7 2/6] iommu/dma: Add support for non-strict mode
In reply to Robin MurphyHi Robin,
Quoted 10 lines · Robin Murphy
From: Zhen Lei <[email protected]>
1. Save the related domain pointer in struct
iommu_dma_cookie, make iovad capable calldomain->ops->flush_iotlb_allto flush TLB.
2. During the iommu domain initialization phase, base ondomain->non_strictfield to check whether non-strict mode is supported or not. If so, callinit_iova_flush_queueto registeriovad->flush_cbcallback.
3. All unmap(contains iova-free) APIs will finally invoke__iommu_dma_unmap-->iommu_dma_free_iova. If the domain is non-strict, callqueue_iovato put off iova freeing, and omitiommu_tlb_syncoperation.
Hmm, this is basically just a commentary on the code. Please could you write it more in terms of the problem that's being solved?
Quoted 25 lines
Signed-off-by Zhen Lei <[email protected]>[rm: convert raw boolean to domain attribute]Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/dma-iommu.c | 29 ++++++++++++++++++++++++++++- include/linux/iommu.h | 1 + 2 files changed, 29 insertions(+), 1 deletion(-)drivers/iommu/dma-iommu.c +3
-0@@ -55,6 +55,9 @@ struct iommu_dma_cookie { };struct list_head msi_page_list;spinlock_t msi_lock; + +/* Only be assigned in non-strict mode, otherwise it's NULL */ +struct iommu_domain *domain; };static inline size_t cookie_msi_granule (struct iommu_dma_cookie *cookie)@@ -257,6 +260,17 @@ static int iova_reserve_iommu_regions(struct device *dev,return ret; }
drivers/iommu/dma-iommu.c static int iova_reserve_iommu_regions(struct device *dev, · line 271
Quoted 10 lines
drivers/iommu/dma-iommu.c +9
-0+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->domain; + + domain->ops->flush_iotlb_all (domain);
Can we rely on this function pointer being non-NULL? I think it would be better to call iommu_flush_tlb_all(cookie->domain) instead.
drivers/iommu/dma-iommu.c int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base, · line 292
Quoted 10 lines
drivers/iommu/dma-iommu.c +3
-0+} +/** * iommu_dma_init_domain - Initialise a DMA mapping domain * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie() @@ -275,6 +289,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 =1 ;
Do we actually need to initialise this?
Will
Re: [PATCH v7 4/6] iommu: Add bootup option "iommu.non_strict"
In reply to Robin MurphyQuoted 23 lines · Robin Murphy
From: Zhen Lei <[email protected]>
Add a bootup option to make the system manager can choose which mode to be used. The default mode is strict.
Signed-off-by Zhen Lei <[email protected]>[rm: move handling out of SMMUv3 driver]Signed-off-by Robin Murphy <[email protected]>--- .../admin-guide/kernel-parameters.txt | 13 ++++++++++ drivers/iommu/iommu.c | 26 +++++++++++++++++++ 2 files changed, 39 insertions(+)Documentation/admin-guide/kernel-parameters.txt +2
-0@@ -1749,6 +1749,19 @@ nobypass [PPC/POWERNV] Disable IOMMU bypass, using IOMMU for PCI devices. + iommu.non_strict= [ARM64] + Format: { "0" | "1" }
Documentation/admin-guide/kernel-parameters.txt line 1763
Quoted 10 lines
Documentation/admin-guide/kernel-parameters.txt +10
-0+ 0 - strict mode, default. + Release IOVAs after the related TLBs are invalid + completely. + 1 - non-strict mode. + Put off TLBs invalidation and release memory first. + It's good for scatter-gather performance but lacks + full isolation, an untrusted device can access the + reused memory because the TLBs may still valid. + Please take full consideration before choosing this + mode. Note that, VFIO will always use strict mode.
This text needs help. How about something like:
0 - strict mode, default.
Invalidate the TLB of the IOMMU hardware as part of every unmap() operation.
1 - lazy mode.
Defer TLB invalidation so that the TLB of the IOMMU hardware is invalidated periodically, rather than as part of every unmap() operation.
(generally, I think I'd s/non strict/lazy/ in this patch to avoid the double negatives)
Quoted 21 lines
Documentation/admin-guide/kernel-parameters.txt +1
-0+ iommu.passthrough= [ARM64] Configure DMA to bypass the IOMMU by default. Format: { "0" | "1" }drivers/iommu/iommu.c +2
-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_non_strict__read_mostly ;struct iommu_callback_data {const struct iommu_ops *ops;@@ -131,6 +132,24 @@ 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)
drivers/iommu/iommu.c static int __init iommu_set_def_domain_type(char *str) · line 145
Quoted 10 lines
drivers/iommu/iommu.c +10
-0+{ +int ret; + + ret =kstrtobool (str, &iommu_dma_non_strict); +if (ret) +return ret; + +if (iommu_dma_non_strict) { +pr_warn ("WARNING: iommu non-strict mode is chosen.\n" +"It's good for scatter-gather performance but lacks full isolation\n" );
Hmm, not sure about this message either and tainting is probably over the top. Maybe drop the taint and just pr_info something like "IOMMU DMA ops using lazy TLB invalidation: unable to protect against malicious devices"
Quoted 11 lines
drivers/iommu/iommu.c +7
-0+add_taint (TAINT_WARN ,LOCKDEP_STILL_OK ); + } + +return 0 ; +} +early_param ("iommu.non_strict" , iommu_dma_setup); +static ssize_t iommu_group_attr_show (struct kobject *kobj,struct attribute *__attr,char *buf) {
drivers/iommu/iommu.c struct iommu_group *iommu_group_get_for_dev(struct device *dev) · line 1100
Quoted 10 lines
drivers/iommu/iommu.c +7
-0@@ -1072,6 +1091,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_non_strict) { +int attr =1 ; +iommu_domain_set_attr (dom, +DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE , + &attr); + }
Hmm, I don't think we can guarantee that we're working with the DMA domain here. Does this all fall out in the wash for the identity domain?
Will
Re: [PATCH v7 5/6] iommu/arm-smmu-v3: Add support for non-strict mode
In reply to Robin MurphyQuoted 4 lines · Robin Murphy
From: Zhen Lei <[email protected]>
Dynamically choose strict or non-strict mode for page table config based on the iommu domain type.
It's the domain type in conjunction with the attribute that determines whether we use lazy or strict invalidation.
Quoted 39 lines
Signed-off-by Zhen Lei <[email protected]>[rm: convert to domain attribute]Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm-smmu-v3.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)drivers/iommu/arm-smmu-v3.c +5
-3@@ -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 {@@ -1633,6 +1634,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,13 +1938,17 @@ 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 : +if (domain->type !=IOMMU_DOMAIN_UNMANAGED )
drivers/iommu/arm-smmu-v3.c static int arm_smmu_domain_get_attr(struct iommu_domain *domain, · line 1953
Quoted 10 lines
drivers/iommu/arm-smmu-v3.c +6
-0+return -EINVAL ; *(int *)data = (smmu_domain->stage ==ARM_SMMU_DOMAIN_NESTED );return 0 ; +case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE : +if (domain->type !=IOMMU_DOMAIN_DMA ) +return -EINVAL ; + *(int *)data = smmu_domain->non_strict; +return 0 ;default :return -ENODEV ;
Hmm, there's a change in behaviour here (and also in the set function) which is that unknown attributes now return -ENODEV for managed domains instead of -EINVAL. I don't know if that's a problem, but I'd be inclined to switch on the domain type and then have a nested switch for the supported attributes.
Will
Re: [PATCH v7 6/6] iommu/arm-smmu: Support non-strict mode
In reply to Robin MurphyQuoted 26 lines · Robin Murphy
All we need is to wire up .flush_iotlb_all properly and implement the domain attribute, and iommu-dma and io-pgtable-arm will do the rest for us. Rather than bother implementing it for v7s format for the highly unlikely chance of that being relevant, we can simply hide the non-strict flag from io-pgtable for that combination just so anyone who does actually try it will simply get over-invalidation instead of failure to initialise domains.
Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm-smmu.c | 40 +++++++++++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 7 deletions(-)drivers/iommu/arm-smmu.c +1
-0@@ -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;
drivers/iommu/arm-smmu.c static int arm_smmu_init_domain_context(struct iommu_domain *domain, · line 868
Quoted 5 lines
drivers/iommu/arm-smmu.c +2
-0@@ -863,6 +864,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 && cfg->fmt !=ARM_SMMU_CTX_FMT_AARCH32_S ) + pgtbl_cfg.quirks |=IO_PGTABLE_QUIRK_NON_STRICT ;
Does this mean we end up over-invalidating when using short-descriptor? Could we not bypass the flush queue in this case instead? Ideally, we'd just reject the domain attribute but I don't know if we know about the page-table format early enough for that. Alternatively, we could force long format if the attribute is set.
What do you think?
Will
Robin Murphy
maintainerHi Will,
Quoted 3 lines · Will Deacon
Hi Robin,
Thanks for turning this around so quickly.
Cheers for a pretty rapid review too :)
Quoted 15 lines · Robin Murphy
Quoted 6 lines
Since we'd like to get this polished up and merged and Leizhen has other commitments, here's v7 of the previous series[1] wherein I address all my own feedback :) This is a quick tweak of the v6 I sent yesterday since I figured out slightly too late a much neater way of setting the attribute at the appropriate time.
Quoted 7 lines
The principal change is that I've inverted things slightly such that it's now a generic domain attribute controlled by iommu-dma given the necessary support from individual IOMMU drivers. That way we can easily enable other drivers straight away, as I've done for SMMUv2 here (which also allowed me to give it a quick test with MMU-401s on a Juno board). Otherwise it's really just cosmetic cleanup and rebasing onto Will's pending SMMU queue.
Quoted 4 lines
I've been through and had a look, leaving some small comments on the patches themselves. The only part I failed to figure out is how you tie the lifetime of the flush queue to the lifetime of the domain so that the timer callback can't fire after e.g. the DMA cookie has been freed. How does that work?
Er, to be honest I haven't looked or even considered it! Other than the parts I've massaged I kinda took the functionality of the previous series for granted. Let me cross-check the x86 code and figure it out.
Robin.
Robin Murphy
maintainerRe: [PATCH v7 2/6] iommu/dma: Add support for non-strict mode
In reply to Will DeaconQuoted 14 lines · Will Deacon
Hi Robin,
Quoted 10 lines · Robin Murphy
From: Zhen Lei <[email protected]>
1. Save the related domain pointer in struct
iommu_dma_cookie, make iovadcapable call domain->ops->flush_iotlb_all to flush TLB.2. During the iommu domain initialization phase, base on
domain->non_strictfield to check whether non-strict mode is supported or not. If so, callinit_iova_flush_queueto registeriovad->flush_cbcallback.
3. All unmap(contains iova-free) APIs will finally invoke__iommu_dma_unmap-->iommu_dma_free_iova. If the domain is non-strict, callqueue_iovato put off iova freeing, and omitiommu_tlb_syncoperation.
Quoted 2 lines
Hmm, this is basically just a commentary on the code. Please could you write it more in terms of the problem that's being solved?
Sure - I intentionally kept a light touch when it came to the documentation and commit messages in this rework (other than patch #1 where I eventually remembered the original reasoning and that it wasn't a bug). If we're more-or-less happy with the shape of the technical side I'll make sure to take a final pass through v8 to tidy up all the prose.
Quoted 26 lines
Quoted 26 lines
Signed-off-by Zhen Lei <[email protected]>[rm: convert raw boolean to domain attribute]Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/dma-iommu.c | 29 ++++++++++++++++++++++++++++- include/linux/iommu.h | 1 + 2 files changed, 29 insertions(+), 1 deletion(-)drivers/iommu/dma-iommu.c +3
-0@@ -55,6 +55,9 @@ struct iommu_dma_cookie { };struct list_head msi_page_list;spinlock_t msi_lock; + +/* Only be assigned in non-strict mode, otherwise it's NULL */ +struct iommu_domain *domain; };static inline size_t cookie_msi_granule (struct iommu_dma_cookie *cookie)@@ -257,6 +260,17 @@ static int iova_reserve_iommu_regions(struct device *dev,return ret; }
Quoted 12 lines
drivers/iommu/dma-iommu.c static int iova_reserve_iommu_regions(struct device *dev, · line 271
Quoted 9 lines
drivers/iommu/dma-iommu.c +9
-0+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->domain; + + domain->ops->flush_iotlb_all (domain);Can we rely on this function pointer being non-NULL? I think it would be better to call
iommu_flush_tlb_all(cookie->domain)instead.
Yeah, that's deliberate - in fact got as far as writing that change, then undid it as I realised that although the attribute conversion got rid of the explicit ops->flush_iotlb_all check, it still makes zero sense for an IOMMU driver to claim to support the flush queue attribute without also providing the relevant callback, so I do actually want this to blow up rather than silently do nothing if that assumption isn't met.
Quoted 4 lines
Quoted 4 lines
drivers/iommu/dma-iommu.c +2
-0+} +/** * iommu_dma_init_domain - Initialise a DMA mapping domain
Quoted 8 lines
drivers/iommu/dma-iommu.c int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base, · line 292
Quoted 6 lines
drivers/iommu/dma-iommu.c +1
-0* @domain: IOMMU domain previously prepared by iommu_get_dma_cookie() @@ -275,6 +289,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 =1 ;Do we actually need to initialise this?
Oops, no, that's a left-over from the turned-out-messier-that-I-thought v6 implementation.
Thanks,
Robin.
Robin Murphy
maintainerRe: [PATCH v7 4/6] iommu: Add bootup option "iommu.non_strict"
In reply to Will DeaconQuoted 100 lines · Will Deacon
Quoted 23 lines · Robin Murphy
From: Zhen Lei <[email protected]>
Add a bootup option to make the system manager can choose which mode to be used. The default mode is strict.
Signed-off-by Zhen Lei <[email protected]>[rm: move handling out of SMMUv3 driver]Signed-off-by Robin Murphy <[email protected]>--- .../admin-guide/kernel-parameters.txt | 13 ++++++++++ drivers/iommu/iommu.c | 26 +++++++++++++++++++ 2 files changed, 39 insertions(+)Documentation/admin-guide/kernel-parameters.txt +2
-0@@ -1749,6 +1749,19 @@ nobypass [PPC/POWERNV] Disable IOMMU bypass, using IOMMU for PCI devices. + iommu.non_strict= [ARM64] + Format: { "0" | "1" }Documentation/admin-guide/kernel-parameters.txt line 1763
Quoted 10 lines
Documentation/admin-guide/kernel-parameters.txt +10
-0+ 0 - strict mode, default. + Release IOVAs after the related TLBs are invalid + completely. + 1 - non-strict mode. + Put off TLBs invalidation and release memory first. + It's good for scatter-gather performance but lacks + full isolation, an untrusted device can access the + reused memory because the TLBs may still valid. + Please take full consideration before choosing this + mode. Note that, VFIO will always use strict mode.This text needs help. How about something like:
0 - strict mode, default.
Invalidate the TLB of the IOMMU hardware as part of everyunmap()operation.
1 - lazy mode.
Defer TLB invalidation so that the TLB of the IOMMU hardware is invalidated periodically, rather than as part of everyunmap()operation.(generally, I think I'd s/non strict/lazy/ in this patch to avoid the double negatives)
Quoted 21 lines
Documentation/admin-guide/kernel-parameters.txt +1
-0+ iommu.passthrough= [ARM64] Configure DMA to bypass the IOMMU by default. Format: { "0" | "1" }drivers/iommu/iommu.c +2
-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_non_strict__read_mostly ;struct iommu_callback_data {const struct iommu_ops *ops;@@ -131,6 +132,24 @@ 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)drivers/iommu/iommu.c static int __init iommu_set_def_domain_type(char *str) · line 145
Quoted 10 lines
drivers/iommu/iommu.c +10
-0+{ +int ret; + + ret =kstrtobool (str, &iommu_dma_non_strict); +if (ret) +return ret; + +if (iommu_dma_non_strict) { +pr_warn ("WARNING: iommu non-strict mode is chosen.\n" +"It's good for scatter-gather performance but lacks full isolation\n" );Hmm, not sure about this message either and tainting is probably over the top. Maybe drop the taint and just
pr_infosomething like "IOMMU DMA ops using lazy TLB invalidation: unable to protect against malicious devices"Quoted 16 lines
drivers/iommu/iommu.c +9
-0+add_taint (TAINT_WARN ,LOCKDEP_STILL_OK ); + } + +return 0 ; +} +early_param ("iommu.non_strict" , iommu_dma_setup); +static ssize_t iommu_group_attr_show (struct kobject *kobj,struct attribute *__attr,char *buf) {@@ -1072,6 +1091,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_non_strict) {
Quoted 8 lines
drivers/iommu/iommu.c struct iommu_group *iommu_group_get_for_dev(struct device *dev) · line 1100
Quoted 5 lines
drivers/iommu/iommu.c +5
-0+int attr =1 ; +iommu_domain_set_attr (dom, +DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE , + &attr); + }Hmm, I don't think we can guarantee that we're working with the DMA domain here. Does this all fall out in the wash for the identity domain?
Indeed so - for one, I expect drivers to reject it for anything that isn't their own default DMA ops domain type (as #5 and #6 do), and furthermore it only has any effect once iommu_dma_init_domain() reads it back if it stuck, and other domain types should never be getting passed into there anyway.
Robin.
Robin Murphy
maintainerRe: [PATCH v7 5/6] iommu/arm-smmu-v3: Add support for non-strict mode
In reply to Will DeaconQuoted 56 lines · Will Deacon
Quoted 4 lines · Robin Murphy
From: Zhen Lei <[email protected]>
Dynamically choose strict or non-strict mode for page table config based on the iommu domain type.
It's the domain type in conjunction with the attribute that determines whether we use lazy or strict invalidation.
Quoted 47 lines
Signed-off-by Zhen Lei <[email protected]>[rm: convert to domain attribute]Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm-smmu-v3.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)drivers/iommu/arm-smmu-v3.c +11
-3@@ -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 {@@ -1633,6 +1634,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,13 +1938,17 @@ 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 : +if (domain->type !=IOMMU_DOMAIN_UNMANAGED ) +return -EINVAL ; *(int *)data = (smmu_domain->stage ==ARM_SMMU_DOMAIN_NESTED );return 0 ; +case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE : +if (domain->type !=IOMMU_DOMAIN_DMA ) +return -EINVAL ; + *(int *)data = smmu_domain->non_strict; +return 0 ;
Quoted 8 lines
drivers/iommu/arm-smmu-v3.c static int arm_smmu_domain_get_attr(struct iommu_domain *domain, · line 1953
Quoted 2 lines
drivers/iommu/arm-smmu-v3.c +0
-0default :return -ENODEV ;Hmm, there's a change in behaviour here (and also in the set function) which is that unknown attributes now return
-ENODEVfor managed domains instead of-EINVAL. I don't know if that's a problem, but I'd be inclined to switch on the domain type and then have a nested switch for the supported attributes.
Sure, a nested switch did actually cross my mind, but I was worried it might be a little boilerplate-heavy since there's still only one of each case (and this quick'n'dirty copy-paste job didn't need any thought...)
If that's your preference too, though, I'll respin both driver patches that way.
Robin.
Robin Murphy
maintainerRe: [PATCH v7 6/6] iommu/arm-smmu: Support non-strict mode
In reply to Will DeaconQuoted 39 lines · Will Deacon
Quoted 26 lines · Robin Murphy
All we need is to wire up .flush_iotlb_all properly and implement the domain attribute, and iommu-dma and io-pgtable-arm will do the rest for us. Rather than bother implementing it for v7s format for the highly unlikely chance of that being relevant, we can simply hide the non-strict flag from io-pgtable for that combination just so anyone who does actually try it will simply get over-invalidation instead of failure to initialise domains.
Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm-smmu.c | 40 +++++++++++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 7 deletions(-)drivers/iommu/arm-smmu.c +1
-0@@ -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;drivers/iommu/arm-smmu.c static int arm_smmu_init_domain_context(struct iommu_domain *domain, · line 868
Quoted 5 lines
drivers/iommu/arm-smmu.c +2
-0@@ -863,6 +864,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 && cfg->fmt !=ARM_SMMU_CTX_FMT_AARCH32_S ) + pgtbl_cfg.quirks |=IO_PGTABLE_QUIRK_NON_STRICT ;Does this mean we end up over-invalidating when using short-descriptor? Could we not bypass the flush queue in this case instead? Ideally, we'd just reject the domain attribute but I don't know if we know about the page-table format early enough for that. Alternatively, we could force long format if the attribute is set.
Quoted 1 line
What do you think?
If someone manages to run an arm64 kernel on a theoretical SMMUv2 implementation which only supports short-descriptor, *and* explicitly sets the command-line option, then yes, they'll get both the synchronous TLBIs and the periodic TLBIALLs. As implied by the commit message, my natural response is "don't do that".
However, it will almost certainly take more effort to argue about it or come up with other bodges than it will to just implement the quirk in the v7s code, so if you really think it's a valid concern just shout.
Robin.