[PATCH] iommu: Add a check to avoid invalid iotlb sync
From: Xiang Chen <chenxiang66@hisilicon.com> Currently it will send a iotlb sync at end of iommu unmap even if iotlb_gather is not valid (iotlb_gather->pgsize = 0). Actually it is not necessary, so add a check to avoid invalid iotlb sync. Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com> --- include/linux/iommu.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 9ca6e6b..6afa61b 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -529,6 +529,9 @@ static inline void iommu_flush_iotlb_all(struct iommu_domain *domain) static inline void iommu_iotlb_sync(struct iommu_domain *domain, struct iommu_iotlb_gather *iotlb_gather) { + if (!iotlb_gather->pgsize) + return; + if (domain->ops->iotlb_sync) domain->ops->iotlb_sync(domain, iotlb_gather); -- 2.8.1
On Sat, Mar 27, 2021 at 02:23:10PM +0800, chenxiang wrote:
From: Xiang Chen <chenxiang66@hisilicon.com>
Currently it will send a iotlb sync at end of iommu unmap even if iotlb_gather is not valid (iotlb_gather->pgsize = 0). Actually it is not necessary, so add a check to avoid invalid iotlb sync.
Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com> --- include/linux/iommu.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 9ca6e6b..6afa61b 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -529,6 +529,9 @@ static inline void iommu_flush_iotlb_all(struct iommu_domain *domain) static inline void iommu_iotlb_sync(struct iommu_domain *domain, struct iommu_iotlb_gather *iotlb_gather) { + if (!iotlb_gather->pgsize) + return;
In which circumstances does this occur? Will
Hi Will, 在 2021/3/29 22:45, Will Deacon 写道:
On Sat, Mar 27, 2021 at 02:23:10PM +0800, chenxiang wrote:
From: Xiang Chen <chenxiang66@hisilicon.com>
Currently it will send a iotlb sync at end of iommu unmap even if iotlb_gather is not valid (iotlb_gather->pgsize = 0). Actually it is not necessary, so add a check to avoid invalid iotlb sync.
Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com> --- include/linux/iommu.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 9ca6e6b..6afa61b 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -529,6 +529,9 @@ static inline void iommu_flush_iotlb_all(struct iommu_domain *domain) static inline void iommu_iotlb_sync(struct iommu_domain *domain, struct iommu_iotlb_gather *iotlb_gather) { + if (!iotlb_gather->pgsize) + return; In which circumstances does this occur?
Will
When iommu_unmap, it initializes iotlb_gather (then iotlb_gather->pgsize = 0). If the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && iopte_leaf(), it will fill the iotlb_gather (set iotlb_gather->pgsize = granule); but if the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && !iopte_leaf() (tlb flush walk situation), iotlb_gather is not filled (iotlb_gather->pgsize = 0), it will still send iommu_iotlb_sync(domain, &iotlb_gather) which is actually invalid iotlb sync at last.
.
On 2021-03-30 02:22, chenxiang (M) wrote:
Hi Will,
在 2021/3/29 22:45, Will Deacon 写道:
On Sat, Mar 27, 2021 at 02:23:10PM +0800, chenxiang wrote:
From: Xiang Chen <chenxiang66@hisilicon.com>
Currently it will send a iotlb sync at end of iommu unmap even if iotlb_gather is not valid (iotlb_gather->pgsize = 0). Actually it is not necessary, so add a check to avoid invalid iotlb sync.
Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com> --- include/linux/iommu.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 9ca6e6b..6afa61b 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -529,6 +529,9 @@ static inline void iommu_flush_iotlb_all(struct iommu_domain *domain) static inline void iommu_iotlb_sync(struct iommu_domain *domain, struct iommu_iotlb_gather *iotlb_gather) { + if (!iotlb_gather->pgsize) + return; In which circumstances does this occur?
Will
When iommu_unmap, it initializes iotlb_gather (then iotlb_gather->pgsize = 0). If the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && iopte_leaf(), it will fill the iotlb_gather (set iotlb_gather->pgsize = granule); but if the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && !iopte_leaf() (tlb flush walk situation), iotlb_gather is not filled (iotlb_gather->pgsize = 0), it will still send iommu_iotlb_sync(domain, &iotlb_gather) which is actually invalid iotlb sync at last.
I guess this can happen in DMA API usage if we've previously mapped a block's worth of scatterlist pages into a block-=sized IOVA region, but this is not the place to do anything about it. The main thing this patch will do is break all the other drivers that implement .iotlb_sync but do not use iotlb_gather. By all means optimise SMMUv3's sync behaviour, but the only valid place to do that is in SMMUv3's own sync callback. These particular details are beyond what the IOMMU core knows about. Robin.
On Tue, Mar 30, 2021 at 10:04:53AM +0100, Robin Murphy wrote:
On 2021-03-30 02:22, chenxiang (M) wrote:
Hi Will,
在 2021/3/29 22:45, Will Deacon 写道:
On Sat, Mar 27, 2021 at 02:23:10PM +0800, chenxiang wrote:
From: Xiang Chen <chenxiang66@hisilicon.com>
Currently it will send a iotlb sync at end of iommu unmap even if iotlb_gather is not valid (iotlb_gather->pgsize = 0). Actually it is not necessary, so add a check to avoid invalid iotlb sync.
Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com> --- include/linux/iommu.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 9ca6e6b..6afa61b 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -529,6 +529,9 @@ static inline void iommu_flush_iotlb_all(struct iommu_domain *domain) static inline void iommu_iotlb_sync(struct iommu_domain *domain, struct iommu_iotlb_gather *iotlb_gather) { + if (!iotlb_gather->pgsize) + return; In which circumstances does this occur?
Will
When iommu_unmap, it initializes iotlb_gather (then iotlb_gather->pgsize = 0). If the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && iopte_leaf(), it will fill the iotlb_gather (set iotlb_gather->pgsize = granule); but if the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && !iopte_leaf() (tlb flush walk situation), iotlb_gather is not filled (iotlb_gather->pgsize = 0), it will still send iommu_iotlb_sync(domain, &iotlb_gather) which is actually invalid iotlb sync at last.
I guess this can happen in DMA API usage if we've previously mapped a block's worth of scatterlist pages into a block-=sized IOVA region, but this is not the place to do anything about it. The main thing this patch will do is break all the other drivers that implement .iotlb_sync but do not use iotlb_gather.
By all means optimise SMMUv3's sync behaviour, but the only valid place to do that is in SMMUv3's own sync callback. These particular details are beyond what the IOMMU core knows about.
Yes, that's where I was heading. Chenxiang, please could you send a v2 with the check inside arm_smmu_iotlb_sync() instead? Thanks, Will
在 2021/3/30 17:25, Will Deacon 写道:
On Tue, Mar 30, 2021 at 10:04:53AM +0100, Robin Murphy wrote:
On 2021-03-30 02:22, chenxiang (M) wrote:
Hi Will,
在 2021/3/29 22:45, Will Deacon 写道:
On Sat, Mar 27, 2021 at 02:23:10PM +0800, chenxiang wrote:
From: Xiang Chen <chenxiang66@hisilicon.com>
Currently it will send a iotlb sync at end of iommu unmap even if iotlb_gather is not valid (iotlb_gather->pgsize = 0). Actually it is not necessary, so add a check to avoid invalid iotlb sync.
Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com> --- include/linux/iommu.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 9ca6e6b..6afa61b 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -529,6 +529,9 @@ static inline void iommu_flush_iotlb_all(struct iommu_domain *domain) static inline void iommu_iotlb_sync(struct iommu_domain *domain, struct iommu_iotlb_gather *iotlb_gather) { + if (!iotlb_gather->pgsize) + return; In which circumstances does this occur?
Will When iommu_unmap, it initializes iotlb_gather (then iotlb_gather->pgsize = 0). If the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && iopte_leaf(), it will fill the iotlb_gather (set iotlb_gather->pgsize = granule); but if the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && !iopte_leaf() (tlb flush walk situation), iotlb_gather is not filled (iotlb_gather->pgsize = 0), it will still send iommu_iotlb_sync(domain, &iotlb_gather) which is actually invalid iotlb sync at last. I guess this can happen in DMA API usage if we've previously mapped a block's worth of scatterlist pages into a block-=sized IOVA region, but this is not the place to do anything about it. The main thing this patch will do is break all the other drivers that implement .iotlb_sync but do not use iotlb_gather.
By all means optimise SMMUv3's sync behaviour, but the only valid place to do that is in SMMUv3's own sync callback. These particular details are beyond what the IOMMU core knows about. Yes, that's where I was heading. Chenxiang, please could you send a v2 with the check inside arm_smmu_iotlb_sync() instead?
Ok, thanks, i will send a v2 later.
Thanks,
Will
.
participants (4)
-
chenxiang -
chenxiang (M)
-
Robin Murphy -
Will Deacon