[Intel-xe] [PATCH] drm/xe: Apply upper limit to sg element size

Wed May 24 17:16:30 UTC 2023

On Tue, May 23, 2023 at 01:43:26PM -0700, Chang, Yu bruce wrote:
>Specific to this particular patch, if following the same patch below for upstreaming i915, the change looks good to me.
>
>https://patchwork.kernel.org/project/intel-gfx/patch/b0f6e50a8cdfc484f2dc18d3215e86465e2e1f1c.1627551226.git.leonro@nvidia.com/
>
>So, with that,
>
>Reviewed-by: Bruce Chang <yu.bruce.chang at intel.com>
>

Thansk Bruce. One difference with i915 is that i915 using max segment size of 0xffffffff.
But, this patch uses 0x7ffff000. This is due to below scenario.
Say, an sg table with total size of 0x200000000 (8G) has 3 segments of below length.
sg1: 0xfffff000
sg2: 0xfffff000
sg3: 0x2000

Now, iommu_dma_map_sg() loop, we get,
iter#1 (sg1): s_length = 0xfffff000, pad_len = 0
iter#2 (sg2): s_length = 0xfffff000, pad_len = 0x1000
This is due to fact that iommu code doesn't want to cross dma segment boundary (0xffffffff).
So, it pads previous segment (sg1) with pad_len so that sg2 iova can start from dma segment
boundary. So, this causes sg1 length (0xfffff000 + 0x1000) to overflow and become 0!
This ends up iommu_map_sg() returning a value less than iova_len and hence iommu_dma_map_sg()
fails. By using a max segment size of 0x7ffff000, we can ensure padding will never cause overflow.

This patch circumvents this overflow issue. I am not sure why we don't see this in i915 though
(may be something else is taking care of it or probably we never hit this scenario).

Niranjana

>Thanks,
>Bruce
>
>> -----Original Message-----
>> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of
>> Niranjana Vishwanathapura
>> Sent: Wednesday, May 17, 2023 10:33 PM
>> To: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>> Cc: Hellstrom, Thomas <thomas.hellstrom at intel.com>; intel-
>> xe at lists.freedesktop.org
>> Subject: Re: [Intel-xe] [PATCH] drm/xe: Apply upper limit to sg element size
>>
>> On Wed, May 17, 2023 at 08:01:08AM +0200, Thomas Hellström wrote:
>> >Hi, Niranjana
>> >
>> >On 5/16/23 20:44, Niranjana Vishwanathapura wrote:
>> >>Specify maximum segment size for sg elements by using
>> >>sg_alloc_table_from_pages_segment() to allocate sg_table.
>> >>
>> >>Signed-off-by: Niranjana Vishwanathapura
>> >><niranjana.vishwanathapura at intel.com>
>> >>---
>> >>  drivers/gpu/drm/xe/xe_bo.c |  8 +++++---
>> >>  drivers/gpu/drm/xe/xe_bo.h | 21 +++++++++++++++++++++
>> >>  drivers/gpu/drm/xe/xe_vm.c |  8 +++++---
>> >>  3 files changed, 31 insertions(+), 6 deletions(-)
>> >>
>> >>diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>> >>index c82e995df779..21c5aca424dd 100644
>> >>--- a/drivers/gpu/drm/xe/xe_bo.c
>> >>+++ b/drivers/gpu/drm/xe/xe_bo.c
>> >>@@ -251,9 +251,11 @@ static int xe_tt_map_sg(struct ttm_tt *tt)
>> >>  	if (xe_tt->sg)
>> >>  		return 0;
>> >>-	ret = sg_alloc_table_from_pages(&xe_tt->sgt, tt->pages,
>> num_pages,
>> >>-					0, (u64)num_pages << PAGE_SHIFT,
>> >>-					GFP_KERNEL);
>> >>+	ret = sg_alloc_table_from_pages_segment(&xe_tt->sgt, tt->pages,
>> >>+						num_pages, 0,
>> >>+						(u64)num_pages <<
>> PAGE_SHIFT,
>> >>+						xe_sg_segment_size(xe_tt-
>> >dev),
>> >>+						GFP_KERNEL);
>> >>  	if (ret)
>> >>  		return ret;
>> >>diff --git a/drivers/gpu/drm/xe/xe_bo.h b/drivers/gpu/drm/xe/xe_bo.h
>> >>index 7e111332c35a..a1c51cc0ac3c 100644
>> >>--- a/drivers/gpu/drm/xe/xe_bo.h
>> >>+++ b/drivers/gpu/drm/xe/xe_bo.h
>> >>@@ -296,6 +296,27 @@ void xe_bo_put_commit(struct llist_head
>> >>*deferred);
>> >>  struct sg_table *xe_bo_get_sg(struct xe_bo *bo);
>> >>+/*
>> >>+ * xe_sg_segment_size() - Provides upper limit for sg segment size.
>> >>+ * @dev: device pointer
>> >>+ *
>> >>+ * Returns the maximum segment size for the 'struct scatterlist'
>> >>+ * elements.
>> >>+ */
>> >>+static inline unsigned int xe_sg_segment_size(struct device *dev) {
>> >>+	size_t max = min_t(size_t, UINT_MAX,
>> dma_max_mapping_size(dev));
>> >>+
>> >>+	/*
>> >>+	 * The iommu_dma_map_sg() function ensures iova allocation
>> doesn't
>> >>+	 * cross dma segment boundary. It does so by padding some sg
>> elements.
>> >>+	 * This can cause overflow, ending up with sg->length being set to 0.
>> >>+	 * Avoid this by ensuring maximum segment size is half of 'max'
>> >>+	 * rounded down to PAGE_SIZE.
>> >>+	 */
>> >
>> >Is this a bug in the IOMMU code? In any case, shouldn't the fix on our
>> >side be using
>> >
>> >dma_set_seg_boundary() and
>> >dma_set_max_seg_size()
>> >
>> >to set reasonable values that avoid that problem?
>> >
>>
>> Thanks Thomas.
>> Hmm...I am not fully familiar with the iommu code here, but letting
>> sg->length overflow may be an oversight?
>>
>> Looks like dma_set_max_seg_size() doesn't enforce upper limit to sg
>> segment size while sg_table is created. We need to use
>> sg_alloc_table_from_pages_segment() to specify that. It seems
>> dma_set_max_seg_size() is used to specify the maximum segment size the
>> created sg_table has while doing the dma mapping. The iommu code doesn't
>> even complain when it runs into an sg element with size more that the one
>> set through dma_set_max_seg_size() during mapping.
>> So, driver must ensure that. I think we would still need this patch and update
>> it to call dma_set_max_seg_size() also.
>>
>> Yah, looks like we can call dma_set_seg_boundary() to set a very large
>> segment boundary say (0xffffffffffff) such that the maximum sized BO fits
>> into it (ie., we never run into any BO crossing the segment boundary). But I
>> am not sure if that is the right approach.
>> I am not seeing many drivers calling dma_set_seg_boundary() (pci probe
>> sets it to default 0xffffffff). Our GPU may not enforce any dma segment
>> boundary, but not sure somewhere else there is one.
>>
>> Any thoughts on which approach we should follow?
>>
>> Niranjana
>>
>> >/Thomas
>> >
>> >>+	return round_down(max / 2, PAGE_SIZE); }
>> >>+
>> >>  #if IS_ENABLED(CONFIG_DRM_XE_KUNIT_TEST)
>> >>  /**
>> >>   * xe_bo_is_mem_type - Whether the bo currently resides in the given
>> >>diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> >>index 2aa5bf9cfee1..5c2fdfc0e836 100644
>> >>--- a/drivers/gpu/drm/xe/xe_vm.c
>> >>+++ b/drivers/gpu/drm/xe/xe_vm.c
>> >>@@ -117,9 +117,11 @@ int xe_vma_userptr_pin_pages(struct xe_vma
>> *vma)
>> >>  	if (ret)
>> >>  		goto out;
>> >>-	ret = sg_alloc_table_from_pages(&vma->userptr.sgt, pages, pinned,
>> >>-					0, (u64)pinned << PAGE_SHIFT,
>> >>-					GFP_KERNEL);
>> >>+	ret = sg_alloc_table_from_pages_segment(&vma->userptr.sgt,
>> pages,
>> >>+						pinned, 0,
>> >>+						(u64)pinned << PAGE_SHIFT,
>> >>+						xe_sg_segment_size(xe-
>> >drm.dev),
>> >>+						GFP_KERNEL);
>> >>  	if (ret) {
>> >>  		vma->userptr.sg = NULL;
>> >>  		goto out;