iommu/dma: Restore scatterlist offsets correctly
Subsystems (2)
Robin Murphy
maintainerPATCH 1/2 iommu/dma: Restore scatterlist offsets correctly
With the change to stashing just the IOVA-page-aligned remainder of the CPU-page offset rather than the whole thing, the failure path in __invalidate_sg() also needs tweaking to account for that in the case of differing page sizes where the two offsets may not be equivalent. Similarly in __finalise_sg(), lest the architecture-specific wrappers later get the wrong address for cache maintenance on sync or unmap.
--- drivers/iommu/dma-iommu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/iommu/dma-iommu.c +2 -2
@@ -403,7 +403,7 @@ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents,unsigned int s_length =sg_dma_len (s);unsigned int s_dma_len = s->length; - s->offset = s_offset; + s->offset += s_offset; s->length = s_length;sg_dma_address (s) = dma_addr + s_offset; dma_addr += s_dma_len;
@@ -422,7 +422,7 @@ static void __invalidate_sg(struct scatterlist *sg, int nents)for_each_sg (sg, s, nents, i) {if (sg_dma_address (s) !=DMA_ERROR_CODE ) - s->offset =sg_dma_address (s); + s->offset +=sg_dma_address (s);if (sg_dma_len (s)) s->length =sg_dma_len (s);sg_dma_address (s) =DMA_ERROR_CODE ;
Signature · 1 line
2.7.2.333.g70bd996.dirty
Robin Murphy
maintainerPATCH 1/2 iommu/dma: Restore scatterlist offsets correctly
Quoted 29 lines · Robin Murphy
With the change to stashing just the IOVA-page-aligned remainder of the CPU-page offset rather than the whole thing, the failure path in
__invalidate_sg()also needs tweaking to account for that in the case of differing page sizes where the two offsets may not be equivalent. Similarly in__finalise_sg(), lest the architecture-specific wrappers later get the wrong address for cache maintenance on sync or unmap.Fixes 164afb1d85b8 ("iommu/dma: Use correct offset in map_sg")Reported-by Magnus Damm <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/dma-iommu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/iommu/dma-iommu.c +1
-1@@ -403,7 +403,7 @@ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents,unsigned int s_length =sg_dma_len (s);unsigned int s_dma_len = s->length; - s->offset = s_offset; + s->offset += s_offset; s->length = s_length;sg_dma_address (s) = dma_addr + s_offset; dma_addr += s_dma_len;
drivers/iommu/dma-iommu.c static void __invalidate_sg(struct scatterlist *sg, int nents) · line 428
Quoted 8 lines
drivers/iommu/dma-iommu.c +1
-1@@ -422,7 +422,7 @@ static void __invalidate_sg(struct scatterlist *sg, int nents)for_each_sg (sg, s, nents, i) {if (sg_dma_address (s) !=DMA_ERROR_CODE ) - s->offset =sg_dma_address (s); + s->offset +=sg_dma_address (s);if (sg_dma_len (s)) s->length =sg_dma_len (s);sg_dma_address (s) =DMA_ERROR_CODE ;
Any comments on these patches? Now that the dust has settled this fix really wants to get into 4.6, and folks have been wanting patch 2 for ages so it would be nice to get it queued for 4.7.
Robin.
Robin Murphy
maintainerPATCH 2/2 iommu/dma: Implement scatterlist segment merging
In reply to Robin MurphyStop wasting IOVA space by over-aligning scatterlist segments for a theoretical worst-case segment boundary mask, and instead take the real limits into account to merge consecutive segments where appropriate, so our callers can benefit from geting back nicely simplified lists.
--- drivers/iommu/dma-iommu.c | 84 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 61 insertions(+), 23 deletions(-)
drivers/iommu/dma-iommu.c +61 -23
@@ -389,26 +389,58 @@ void iommu_dma_unmap_page(struct device *dev, dma_addr_t handle, size_t size,/* * Prepare a successfully-mapped scatterlist to give back to the caller. -* Handling IOVA concatenation can come later, if needed +* +* At this point the segments are already laid out by iommu_dma_map_sg() to +* avoid individually crossing any boundaries, so we merely need to check a +* segment's start address to avoid concatenating across one. */ static int __finalise_sg (struct device *dev,struct scatterlist *sg,int nents,dma_addr_t dma_addr) { -struct scatterlist *s; -int i; +struct scatterlist *s, *cur = sg; +unsigned long seg_mask =dma_get_seg_boundary (dev); +unsigned int cur_len =0 , max_len =dma_get_max_seg_size (dev); +int i, count =0 ;for_each_sg (sg, s, nents, i) { -/* Un-swizzling the fields here, hence the naming mismatch */ -unsigned int s_offset =sg_dma_address (s); +/* Restore this segment's original unaligned fields first */ +unsigned int s_iova_off =sg_dma_address (s);unsigned int s_length =sg_dma_len (s); -unsigned int s_dma_len = s->length; +unsigned int s_iova_len = s->length; - s->offset += s_offset; + s->offset += s_iova_off; s->length = s_length; -sg_dma_address (s) = dma_addr + s_offset; - dma_addr += s_dma_len; +sg_dma_address (s) =DMA_ERROR_CODE ; +sg_dma_len (s) =0 ; + +/* +* Now fill in the real DMA data. If... +* - there is a valid output segment to append to +* - and this segment starts on an IOVA page boundary +* - but doesn't fall at a segment boundary +* - and wouldn't make the resulting output segment too long +*/ +if (cur_len && !s_iova_off && (dma_addr & seg_mask) && + (cur_len + s_length <= max_len)) { +/* ...then concatenate it with the previous one */ + cur_len += s_length; + }else { +/* Otherwise start the next output segment */ +if (i >0 ) + cur =sg_next (cur); + cur_len = s_length; + count++; + +sg_dma_address (cur) = dma_addr + s_iova_off; + } + +sg_dma_len (cur) = cur_len; + dma_addr += s_iova_len; + +if (s_length + s_iova_off < s_iova_len) + cur_len =0 ; } -return i; +return count; }/*
@@ -446,34 +478,40 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,struct scatterlist *s, *prev =NULL ;dma_addr_t dma_addr;size_t iova_len =0 ; +unsigned long mask =dma_get_seg_boundary (dev);int i;/* * Work out how much IOVA space we need, and align the segments to * IOVA granules for the IOMMU driver to handle. With some clever * trickery we can modify the list in-place, but reversibly, by -* hiding the original data in the as-yet-unused DMA fields. +* stashing the unaligned parts in the as-yet-unused DMA fields. */ for_each_sg (sg, s, nents, i) { -size_t s_offset =iova_offset (iovad, s->offset); +size_t s_iova_off =iova_offset (iovad, s->offset);size_t s_length = s->length; +size_t pad_len = (mask - iova_len +1 ) & mask; -sg_dma_address (s) = s_offset; +sg_dma_address (s) = s_iova_off;sg_dma_len (s) = s_length; - s->offset -= s_offset; - s_length =iova_align (iovad, s_length + s_offset); + s->offset -= s_iova_off; + s_length =iova_align (iovad, s_length + s_iova_off); s->length = s_length;/* -* The simple way to avoid the rare case of a segment -* crossing the boundary mask is to pad the previous one -* to end at a naturally-aligned IOVA for this one's size, -* at the cost of potentially over-allocating a little. +* Due to the alignment of our single IOVA allocation, we can +* depend on these assumptions about the segment boundary mask: +* - If mask size >= IOVA size, then the IOVA range cannot +* possibly fall across a boundary, so we don't care. +* - If mask size < IOVA size, then the IOVA range must start +* exactly on a boundary, therefore we can lay things out +* based purely on segment lengths without needing to know +* the actual addresses beforehand. +* - The mask must be a power of 2, so pad_len == 0 if +* iova_len == 0, thus we cannot dereference prev the first +* time through here (i.e. before it has a meaningful value). */ -if (prev) { -size_t pad_len =roundup_pow_of_two (s_length); - - pad_len = (pad_len - iova_len) & (pad_len -1 ); +if (pad_len && pad_len < s_length -1 ) { prev->length += pad_len; iova_len += pad_len; }
Signature · 1 line
2.7.2.333.g70bd996.dirty
Joerg Roedel
maintainerPATCH 1/2 iommu/dma: Restore scatterlist offsets correctly
In reply to Robin MurphyQuoted 10 lines · Robin Murphy
With the change to stashing just the IOVA-page-aligned remainder of the CPU-page offset rather than the whole thing, the failure path in
__invalidate_sg()also needs tweaking to account for that in the case of differing page sizes where the two offsets may not be equivalent. Similarly in__finalise_sg(), lest the architecture-specific wrappers later get the wrong address for cache maintenance on sync or unmap.Fixes 164afb1d85b8 ("iommu/dma: Use correct offset in map_sg")Reported-by Magnus Damm <[email protected]>Signed-off-by Robin Murphy <[email protected]>
Joerg
Robin Murphy
maintainerPATCH 1/2 iommu/dma: Restore scatterlist offsets correctly
Quoted 8 lines · Joerg Roedel
Quoted 7 lines · Robin Murphy
With the change to stashing just the IOVA-page-aligned remainder of the CPU-page offset rather than the whole thing, the failure path in
__invalidate_sg()also needs tweaking to account for that in the case of differing page sizes where the two offsets may not be equivalent. Similarly in__finalise_sg(), lest the architecture-specific wrappers later get the wrong address for cache maintenance on sync or unmap.
Quoted 5 lines
Quoted 3 lines
Fixes 164afb1d85b8 ("iommu/dma: Use correct offset in map_sg")Reported-by Magnus Damm <[email protected]>Signed-off-by Robin Murphy <[email protected]>Cc stable at ver.kernel.org # v4.4+ ?
Good point - the kind of people using 64k pages are also likely to be the ones sticking to stable kernels. Are you able to handle that, or would you like me to resend?
Thanks,
Robin.
Quoted 4 lines
Joerg
Joerg Roedel
maintainerPATCH 1/2 iommu/dma: Restore scatterlist offsets correctly
Quoted 13 lines · Robin Murphy
Quoted 12 lines · Joerg Roedel
Quoted 10 lines · Robin Murphy
With the change to stashing just the IOVA-page-aligned remainder of the CPU-page offset rather than the whole thing, the failure path in
__invalidate_sg()also needs tweaking to account for that in the case of differing page sizes where the two offsets may not be equivalent. Similarly in__finalise_sg(), lest the architecture-specific wrappers later get the wrong address for cache maintenance on sync or unmap.Fixes 164afb1d85b8 ("iommu/dma: Use correct offset in map_sg")Reported-by Magnus Damm <[email protected]>Signed-off-by Robin Murphy <[email protected]>
Quoted 5 lines
Quoted 1 line
Cc stable at ver.kernel.org # v4.4+ ?Good point - the kind of people using 64k pages are also likely to be the ones sticking to stable kernels. Are you able to handle that, or would you like me to resend?
I added the tag and put the commit into my iommu/fixes branch. Can you re-send me the second commit when the first is upstream (I'll send the pull-req this week)? I'd like to avoid creating an additional merge-commit just for this patch.
Joerg
Robin Murphy
maintainerPATCH 1/2 iommu/dma: Restore scatterlist offsets correctly
Quoted 16 lines · Joerg Roedel
Quoted 15 lines · Robin Murphy
Quoted 13 lines · Joerg Roedel
Quoted 10 lines · Robin Murphy
With the change to stashing just the IOVA-page-aligned remainder of the CPU-page offset rather than the whole thing, the failure path in
__invalidate_sg()also needs tweaking to account for that in the case of differing page sizes where the two offsets may not be equivalent. Similarly in__finalise_sg(), lest the architecture-specific wrappers later get the wrong address for cache maintenance on sync or unmap.Fixes 164afb1d85b8 ("iommu/dma: Use correct offset in map_sg")Reported-by Magnus Damm <[email protected]>Signed-off-by Robin Murphy <[email protected]>Cc stable at ver.kernel.org # v4.4+ ?
Quoted 8 lines
Quoted 3 lines
Good point - the kind of people using 64k pages are also likely to be the ones sticking to stable kernels. Are you able to handle that, or would you like me to resend?
I added the tag and put the commit into my iommu/fixes branch. Can you re-send me the second commit when the first is upstream (I'll send the pull-req this week)? I'd like to avoid creating an additional merge-commit just for this patch.
Sure, will do - I agree there's absolutely no need to be mucking about with context conflicts right now.
Thanks a lot,
Robin.
Quoted 9 lines
Joerg
_______________________________________________
iommu mailing list
iommu at lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu