| Message ID | kjsmgowrerhkk2d7qxsbccosjb55usqhfmxse6lesxfqwxtvhu@twuaxfazvq2a (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] drm/i915: ensure segment offset never exceeds allowed max | expand |
Hi Krzysztof, First of all I need to apologize for not being responsive on your patches. We had offline reviews and some discussions though, but they were not reported in the v1-v3 reviews. Next time, the reviews need to have more visibility for the community. On Mon, Oct 28, 2024 at 04:00:48PM +0000, Karas, Krzysztof wrote: > Commit 255fc1703e42 ("drm/i915/gem: Calculate object page offset for > partial memory mapping") introduced a new offset that is compared to > sg_dma_len(r.sgt.sgp) in remap_io_sg() function. However, later in > remap_sg() the offset (which at that point resides in r->sgt.curr) > is compared to r->sgt.max. Scatter-gather list's max relies on one what we compare to max is not the offset, but the current sg item, right? Or did I miss something? > of two values (see i915_scatterlist.h): > a) sg_dma_len(s.sgp) when `dma` is true, > b) s.sgp->length otherwise. > This suggests that in cases where `dma` is false, we should use > s.sgp->length to determine the max value instead of sg_dma_len(), > which is used regardless in remap_io_sg() (use_dma(iobase) might return > false there). > > This patch uses r.sgt.max to check if offset is within allowed bounds, > because that max value is already set according to the `dma` value. are you trying to fix any issues here? If so, which one? > v3: > - instead of checking if r.sgt.curr would exceed allowed max, changed > the value in the while loop to be aligned with `dma` value. > > v4: > - remove unnecessary parent relation > > Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com> > --- > drivers/gpu/drm/i915/i915_mm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_mm.c b/drivers/gpu/drm/i915/i915_mm.c > index f5c97a620962..76e2801619f0 100644 > --- a/drivers/gpu/drm/i915/i915_mm.c > +++ b/drivers/gpu/drm/i915/i915_mm.c > @@ -143,8 +143,8 @@ int remap_io_sg(struct vm_area_struct *vma, > /* We rely on prevalidation of the io-mapping to skip track_pfn(). */ > GEM_BUG_ON((vma->vm_flags & EXPECTED_FLAGS) != EXPECTED_FLAGS); > > - while (offset >= sg_dma_len(r.sgt.sgp) >> PAGE_SHIFT) { > - offset -= sg_dma_len(r.sgt.sgp) >> PAGE_SHIFT; > + while (offset >= r.sgt.max >> PAGE_SHIFT) { > + offset -= r.sgt.max >> PAGE_SHIFT; To me looks right sg_dma_len(), why max? Thanks a lot for your patch, Andi > r.sgt = __sgt_iter(__sg_next(r.sgt.sgp), use_dma(iobase)); > if (!r.sgt.sgp) > return -EINVAL; > -- > 2.34.1
Hi Andi, thanks for review! > > Commit 255fc1703e42 ("drm/i915/gem: Calculate object page offset for > > partial memory mapping") introduced a new offset that is compared to > > sg_dma_len(r.sgt.sgp) in remap_io_sg() function. However, later in > > remap_sg() the offset (which at that point resides in r->sgt.curr) > > is compared to r->sgt.max. Scatter-gather list's max relies on one > > what we compare to max is not the offset, but the current sg > item, right? Or did I miss something? What I meant is the 'offset' value that is used to alter r->sgt.curr. > > This patch uses r.sgt.max to check if offset is within allowed bounds, > > because that max value is already set according to the `dma` value. > > are you trying to fix any issues here? If so, which one? That would be issue 12031 from gitlab, which describes failure in gem_ctx_engines@invalid-engines test. > > - while (offset >= sg_dma_len(r.sgt.sgp) >> PAGE_SHIFT) { > > - offset -= sg_dma_len(r.sgt.sgp) >> PAGE_SHIFT; > > + while (offset >= r.sgt.max >> PAGE_SHIFT) { > > + offset -= r.sgt.max >> PAGE_SHIFT; > > To me looks right sg_dma_len(), why max? I am not 100% clear on what sg_dma_len() returns. Looking at i915_scatterlist.h, inside __sgt_iter(), I see that scatterlist's max is determined based on 'dma' flag, which suggests to me that sg_dma_address() and s.sgp->length may be different values. Then later, inside remap_io_sg() we compare 'offset' to the sg_dma_len(), but then 'offset' is used to set r.sgt.curr (so it corresponds to the current sg item, right?), which then is compared to r.sgt.max in remap_sg(). At the same time, also in remap_io_sg(), the while condition comparison uses sg_dma_len() regardless of 'dma' value (it is always false during the test reported in the issue), which seems wrong, if sg_dma_len() and s.sgp->length could hold different values (so we'd use s.sgp->length to set s.max value in i915_scatterlist.h, but then use sg_dma_len() as 'max' in remap_io_sg()). I also noticed that these values are different in failed test runs. Please correct me, if I'm wrong. I want to understand this issue and the code better. Thanks Krzyszotf
Hi Krzysztof, > diff --git a/drivers/gpu/drm/i915/i915_mm.c b/drivers/gpu/drm/i915/i915_mm.c > index f5c97a620962..76e2801619f0 100644 > --- a/drivers/gpu/drm/i915/i915_mm.c > +++ b/drivers/gpu/drm/i915/i915_mm.c > @@ -143,8 +143,8 @@ int remap_io_sg(struct vm_area_struct *vma, > /* We rely on prevalidation of the io-mapping to skip track_pfn(). */ > GEM_BUG_ON((vma->vm_flags & EXPECTED_FLAGS) != EXPECTED_FLAGS); > > - while (offset >= sg_dma_len(r.sgt.sgp) >> PAGE_SHIFT) { > - offset -= sg_dma_len(r.sgt.sgp) >> PAGE_SHIFT; > + while (offset >= r.sgt.max >> PAGE_SHIFT) { > + offset -= r.sgt.max >> PAGE_SHIFT; I finally defeated my laziness and looked at this a little closer, I think you are right, I missed in my first comment a few calculation. Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thanks, Andi
diff --git a/drivers/gpu/drm/i915/i915_mm.c b/drivers/gpu/drm/i915/i915_mm.c index f5c97a620962..76e2801619f0 100644 --- a/drivers/gpu/drm/i915/i915_mm.c +++ b/drivers/gpu/drm/i915/i915_mm.c @@ -143,8 +143,8 @@ int remap_io_sg(struct vm_area_struct *vma, /* We rely on prevalidation of the io-mapping to skip track_pfn(). */ GEM_BUG_ON((vma->vm_flags & EXPECTED_FLAGS) != EXPECTED_FLAGS); - while (offset >= sg_dma_len(r.sgt.sgp) >> PAGE_SHIFT) { - offset -= sg_dma_len(r.sgt.sgp) >> PAGE_SHIFT; + while (offset >= r.sgt.max >> PAGE_SHIFT) { + offset -= r.sgt.max >> PAGE_SHIFT; r.sgt = __sgt_iter(__sg_next(r.sgt.sgp), use_dma(iobase)); if (!r.sgt.sgp) return -EINVAL;
Commit 255fc1703e42 ("drm/i915/gem: Calculate object page offset for partial memory mapping") introduced a new offset that is compared to sg_dma_len(r.sgt.sgp) in remap_io_sg() function. However, later in remap_sg() the offset (which at that point resides in r->sgt.curr) is compared to r->sgt.max. Scatter-gather list's max relies on one of two values (see i915_scatterlist.h): a) sg_dma_len(s.sgp) when `dma` is true, b) s.sgp->length otherwise. This suggests that in cases where `dma` is false, we should use s.sgp->length to determine the max value instead of sg_dma_len(), which is used regardless in remap_io_sg() (use_dma(iobase) might return false there). This patch uses r.sgt.max to check if offset is within allowed bounds, because that max value is already set according to the `dma` value. v3: - instead of checking if r.sgt.curr would exceed allowed max, changed the value in the while loop to be aligned with `dma` value. v4: - remove unnecessary parent relation Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com> --- drivers/gpu/drm/i915/i915_mm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)