> -----Original Message-----
> From: Jason Gunthorpe <j...@ziepe.ca>
> Sent: Thursday, November 05, 2020 4:09 PM
> To: Xiong, Jianxin <jianxin.xi...@intel.com>
> Cc: linux-r...@vger.kernel.org; dri-devel@lists.freedesktop.org; Doug Ledford
> <dledf...@redhat.com>; Leon Romanovsky
> <l...@kernel.org>; Sumit Semwal <sumit.sem...@linaro.org>; Christian Koenig
> <christian.koe...@amd.com>; Vetter, Daniel
> <daniel.vet...@intel.com>
> Subject: Re: [PATCH v8 1/5] RDMA/umem: Support importing dma-buf as user
> memory region
>
> On Thu, Nov 05, 2020 at 02:48:05PM -0800, Jianxin Xiong wrote:
> > + /* modify the sgl in-place to match umem address and length */
> > +
> > + start = ALIGN_DOWN(umem_dmabuf->umem.address, PAGE_SIZE);
> > + end = ALIGN(umem_dmabuf->umem.address + umem_dmabuf->umem.length,
> > + PAGE_SIZE);
> > + cur = 0;
> > + nmap = 0;
> > + for_each_sgtable_dma_sg(sgt, sg, i) {
> > + if (cur >= end)
> > + break;
> > + if (cur + sg_dma_len(sg) <= start) {
> > + cur += sg_dma_len(sg);
> > + continue;
> > + }
>
> This seems like a strange way to compute interesections
I can rework that.
>
> if (cur <= start && start < cur + sg_dma_len(sg))
>
> > + if (cur <= start) {
> > + unsigned long offset = start - cur;
> > +
> > + umem_dmabuf->first_sg = sg;
> > + umem_dmabuf->first_sg_offset = offset;
> > + sg_dma_address(sg) += offset;
> > + sg_dma_len(sg) -= offset;
> > + if (&sg_dma_len(sg) != &sg->length)
> > + sg->length -= offset;
>
> We don't need to adjust sg->length, only dma_len, so no reason for this
> surprising if.
>
> > + cur += offset;
> > + }
> > + if (cur + sg_dma_len(sg) >= end) {
>
> Same logic here
>
> > + unsigned long trim = cur + sg_dma_len(sg) - end;
> > +
> > + umem_dmabuf->last_sg = sg;
> > + umem_dmabuf->last_sg_trim = trim;
> > + sg_dma_len(sg) -= trim;
> > + if (&sg_dma_len(sg) != &sg->length)
> > + sg->length -= trim;
>
> break, things are done here
>
> > + }
> > + cur += sg_dma_len(sg);
> > + nmap++;
> > + }
>
> > +
> > + umem_dmabuf->umem.sg_head.sgl = umem_dmabuf->first_sg;
> > + umem_dmabuf->umem.sg_head.nents = nmap;
> > + umem_dmabuf->umem.nmap = nmap;
> > + umem_dmabuf->sgt = sgt;
> > +
> > + page_size = ib_umem_find_best_pgsz(&umem_dmabuf->umem, PAGE_SIZE,
> > + umem_dmabuf->umem.iova);
> > +
> > + if (WARN_ON(cur != end || page_size != PAGE_SIZE)) {
>
> Looks like nothing prevents this warn on to tigger
>
> The user could specify a length that is beyond the dma buf, can the dma buf
> length be checked during get?
In order to check the length, the buffer needs to be mapped. That can be done.
>
> Also page_size can be 0 because iova is not OK. iova should be checked for
> alignment during get as well:
>
> iova & (PAGE_SIZE-1) == umem->addr & (PAGE_SIZE-1)
>
If ib_umem_dmabuf_map_pages is called during get this error is automatically
caught.
> But yes, this is the right idea
>
> Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
