[PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[David Riley]

Userspace requested command buffer allocations could be too large
to make as a contiguous allocation.  Use vmalloc if necessary to
satisfy those allocations.

Signed-off-by: David Riley <davidriley@chromium.org>
---
 drivers/gpu/drm/virtio/virtgpu_ioctl.c |  4 +-
 drivers/gpu/drm/virtio/virtgpu_vq.c    | 74 ++++++++++++++++++++++++--
 2 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index ac60be9b5c19..a8732a8af766 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -195,7 +195,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto out_free;
 
-	buf = memdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
+	buf = vmemdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
 	if (IS_ERR(buf)) {
 		ret = PTR_ERR(buf);
 		goto out_unresv;
@@ -230,7 +230,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
 	return 0;
 
 out_memdup:
-	kfree(buf);
+	kvfree(buf);
 out_unresv:
 	ttm_eu_backoff_reservation(&ticket, &validate_list);
 out_free:
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 981ee16e3ee9..bcbc48b7284f 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -154,7 +154,7 @@ static void free_vbuf(struct virtio_gpu_device *vgdev,
 {
 	if (vbuf->resp_size > MAX_INLINE_RESP_SIZE)
 		kfree(vbuf->resp_buf);
-	kfree(vbuf->data_buf);
+	kvfree(vbuf->data_buf);
 	kmem_cache_free(vgdev->vbufs, vbuf);
 }
 
@@ -251,6 +251,59 @@ void virtio_gpu_dequeue_cursor_func(struct work_struct *work)
 	wake_up(&vgdev->cursorq.ack_queue);
 }
 
+/* How many bytes left in this page. */
+static unsigned int rest_of_page(void *data)
+{
+	return PAGE_SIZE - offset_in_page(data);
+}
+
+/* Create sg_table from a vmalloc'd buffer. */
+static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size)
+{
+	int nents, ret, s, i;
+	struct sg_table *sgt;
+	struct scatterlist *sg;
+	struct page *pg;
+
+	sgt = kmalloc(sizeof(*sgt), GFP_KERNEL);
+	if (!sgt)
+		return NULL;
+
+	nents = DIV_ROUND_UP(size, PAGE_SIZE) + 1;
+	ret = sg_alloc_table(sgt, nents, GFP_KERNEL);
+	if (ret) {
+		kfree(sgt);
+		return NULL;
+	}
+
+	for_each_sg(sgt->sgl, sg, nents, i) {
+		pg = vmalloc_to_page(data);
+		if (!pg) {
+			sg_free_table(sgt);
+			kfree(sgt);
+			return NULL;
+		}
+
+		s = rest_of_page(data);
+		if (s > size)
+			s = size;
+
+		sg_set_page(sg, pg, s, offset_in_page(data));
+
+		size -= s;
+		data += s;
+
+		if (size) {
+			sg_unmark_end(sg);
+		} else {
+			sg_mark_end(sg);
+			break;
+		}
+	}
+
+	return sgt;
+}
+
 static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
 					       struct virtio_gpu_vbuffer *vbuf)
 		__releases(&vgdev->ctrlq.qlock)
@@ -260,6 +313,7 @@ static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
 	struct scatterlist *sgs[3], vcmd, vout, vresp;
 	int outcnt = 0, incnt = 0;
 	int ret;
+	struct sg_table *sgt = NULL;
 
 	if (!vgdev->vqs_ready)
 		return -ENODEV;
@@ -269,8 +323,17 @@ static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
 	outcnt++;
 
 	if (vbuf->data_size) {
-		sg_init_one(&vout, vbuf->data_buf, vbuf->data_size);
-		sgs[outcnt + incnt] = &vout;
+		if (is_vmalloc_addr(vbuf->data_buf)) {
+			spin_unlock(&vgdev->ctrlq.qlock);
+			sgt = vmalloc_to_sgt(vbuf->data_buf, vbuf->data_size);
+			spin_lock(&vgdev->ctrlq.qlock);
+			if (!sgt)
+				return -ENOMEM;
+			sgs[outcnt + incnt] = sgt->sgl;
+		} else {
+			sg_init_one(&vout, vbuf->data_buf, vbuf->data_size);
+			sgs[outcnt + incnt] = &vout;
+		}
 		outcnt++;
 	}
 
@@ -294,6 +357,11 @@ static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
 		virtqueue_kick(vq);
 	}
 
+	if (sgt) {
+		sg_free_table(sgt);
+		kfree(sgt);
+	}
+
 	if (!ret)
 		ret = vq->num_free;
 	return ret;
-- 
2.23.0.187.g17f5b7556c-goog

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[David Riley]

On Sun, Sep 1, 2019 at 10:28 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Fri, Aug 30, 2019 at 10:49:25AM -0700, David Riley wrote:
> > Hi Gerd,
> >
> > On Fri, Aug 30, 2019 at 4:16 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > >   Hi,
> > >
> > > > > > -     kfree(vbuf->data_buf);
> > > > > > +     kvfree(vbuf->data_buf);
> > > > >
> > > > > if (is_vmalloc_addr(vbuf->data_buf)) ...
> > > > >
> > > > > needed here I gues?
> > > > >
> > > >
> > > > kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check.
> > >
> > > Ok.
> > >
> > > > - videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c,
> > > > assumes contiguous array of scatterlist and that the buffer being converted
> > > > is page aligned
> > >
> > > Well, vmalloc memory _is_ page aligned.
> >
> > True, but this function gets called for all potential enqueuings (eg
> > resource_create_3d, resource_attach_backing) and I was concerned that
> > some other usage in the future might not have that guarantee.
>
> The vmalloc_to_sg call is wrapped into "if (is_vmalloc())", so this
> should not be a problem.
>
> > > sg_alloc_table_from_pages() does alot of what you need, you just need a
> > > small loop around vmalloc_to_page() create a struct page array
> > > beforehand.
> >
> > That feels like an extra allocation when under memory pressure and
> > more work, to not gain much -- there still needs to be a function that
> > iterates through all the pages.  But I don't feel super strongly about
> > it and can change it if you think that it will be less maintenance
> > overhead.
>
> Lets see how vmalloc_to_sg looks like when it assumes page-aligned
> memory.  It's probably noticeable shorter then.

It's not really.  The allocation of the table is one unit less, and
doesn't need to take into account that data might be an offset within
the page.  It still needs error handling, partial final page handling,
and marking of the end of the scatterlist.  Things could be slightly
simplified to assume that you can always get a contiguous allocation
of the table instead of using sg_alloc_table/for_each_sg, but given
that we're only going down this path when memory is fragmented and in
a fallback, doesn't seem worthwhile to make that trade-off.

I've written a different version of vmalloc_to_sgt which uses
sg_alloc_table_from_pages under the covers and it comes in slightly
shorter (39 lines vs 55 lines), but incurs another allocation as
previously so I'm personally in favour of things as written.
fpga_mgr_buf_load is another function which roughly does the same sort
of operation and it's a bit longer.

I'll post a v2 shortly, but if you think it's worth making the extra
allocation of the pages array to use, I can post that instead.

> cheers,
>   Gerd
>

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[David Riley]

On Sun, Sep 1, 2019 at 10:28 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Fri, Aug 30, 2019 at 10:49:25AM -0700, David Riley wrote:
> > Hi Gerd,
> >
> > On Fri, Aug 30, 2019 at 4:16 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > >   Hi,
> > >
> > > > > > -     kfree(vbuf->data_buf);
> > > > > > +     kvfree(vbuf->data_buf);
> > > > >
> > > > > if (is_vmalloc_addr(vbuf->data_buf)) ...
> > > > >
> > > > > needed here I gues?
> > > > >
> > > >
> > > > kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check.
> > >
> > > Ok.
> > >
> > > > - videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c,
> > > > assumes contiguous array of scatterlist and that the buffer being converted
> > > > is page aligned
> > >
> > > Well, vmalloc memory _is_ page aligned.
> >
> > True, but this function gets called for all potential enqueuings (eg
> > resource_create_3d, resource_attach_backing) and I was concerned that
> > some other usage in the future might not have that guarantee.
>
> The vmalloc_to_sg call is wrapped into "if (is_vmalloc())", so this
> should not be a problem.
>
> > > sg_alloc_table_from_pages() does alot of what you need, you just need a
> > > small loop around vmalloc_to_page() create a struct page array
> > > beforehand.
> >
> > That feels like an extra allocation when under memory pressure and
> > more work, to not gain much -- there still needs to be a function that
> > iterates through all the pages.  But I don't feel super strongly about
> > it and can change it if you think that it will be less maintenance
> > overhead.
>
> Lets see how vmalloc_to_sg looks like when it assumes page-aligned
> memory.  It's probably noticeable shorter then.

It's not really.  The allocation of the table is one unit less, and
doesn't need to take into account that data might be an offset within
the page.  It still needs error handling, partial final page handling,
and marking of the end of the scatterlist.  Things could be slightly
simplified to assume that you can always get a contiguous allocation
of the table instead of using sg_alloc_table/for_each_sg, but given
that we're only going down this path when memory is fragmented and in
a fallback, doesn't seem worthwhile to make that trade-off.

I've written a different version of vmalloc_to_sgt which uses
sg_alloc_table_from_pages under the covers and it comes in slightly
shorter (39 lines vs 55 lines), but incurs another allocation as
previously so I'm personally in favour of things as written.
fpga_mgr_buf_load is another function which roughly does the same sort
of operation and it's a bit longer.

I'll post a v2 shortly, but if you think it's worth making the extra
allocation of the pages array to use, I can post that instead.

> cheers,
>   Gerd
>

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[Gerd Hoffmann]

On Fri, Aug 30, 2019 at 10:49:25AM -0700, David Riley wrote:
> Hi Gerd,
> 
> On Fri, Aug 30, 2019 at 4:16 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >   Hi,
> >
> > > > > -     kfree(vbuf->data_buf);
> > > > > +     kvfree(vbuf->data_buf);
> > > >
> > > > if (is_vmalloc_addr(vbuf->data_buf)) ...
> > > >
> > > > needed here I gues?
> > > >
> > >
> > > kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check.
> >
> > Ok.
> >
> > > - videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c,
> > > assumes contiguous array of scatterlist and that the buffer being converted
> > > is page aligned
> >
> > Well, vmalloc memory _is_ page aligned.
> 
> True, but this function gets called for all potential enqueuings (eg
> resource_create_3d, resource_attach_backing) and I was concerned that
> some other usage in the future might not have that guarantee.

The vmalloc_to_sg call is wrapped into "if (is_vmalloc())", so this
should not be a problem.

> > sg_alloc_table_from_pages() does alot of what you need, you just need a
> > small loop around vmalloc_to_page() create a struct page array
> > beforehand.
> 
> That feels like an extra allocation when under memory pressure and
> more work, to not gain much -- there still needs to be a function that
> iterates through all the pages.  But I don't feel super strongly about
> it and can change it if you think that it will be less maintenance
> overhead.

Lets see how vmalloc_to_sg looks like when it assumes page-aligned
memory.  It's probably noticeable shorter then.

cheers,
  Gerd

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[Gerd Hoffmann]

On Fri, Aug 30, 2019 at 10:49:25AM -0700, David Riley wrote:
> Hi Gerd,
> 
> On Fri, Aug 30, 2019 at 4:16 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >   Hi,
> >
> > > > > -     kfree(vbuf->data_buf);
> > > > > +     kvfree(vbuf->data_buf);
> > > >
> > > > if (is_vmalloc_addr(vbuf->data_buf)) ...
> > > >
> > > > needed here I gues?
> > > >
> > >
> > > kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check.
> >
> > Ok.
> >
> > > - videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c,
> > > assumes contiguous array of scatterlist and that the buffer being converted
> > > is page aligned
> >
> > Well, vmalloc memory _is_ page aligned.
> 
> True, but this function gets called for all potential enqueuings (eg
> resource_create_3d, resource_attach_backing) and I was concerned that
> some other usage in the future might not have that guarantee.

The vmalloc_to_sg call is wrapped into "if (is_vmalloc())", so this
should not be a problem.

> > sg_alloc_table_from_pages() does alot of what you need, you just need a
> > small loop around vmalloc_to_page() create a struct page array
> > beforehand.
> 
> That feels like an extra allocation when under memory pressure and
> more work, to not gain much -- there still needs to be a function that
> iterates through all the pages.  But I don't feel super strongly about
> it and can change it if you think that it will be less maintenance
> overhead.

Lets see how vmalloc_to_sg looks like when it assumes page-aligned
memory.  It's probably noticeable shorter then.

cheers,
  Gerd

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[Gerd Hoffmann]

> > Completely different approach: use get_user_pages() and don't copy the
> > execbuffer at all.
> It would be really nice if execbuffer does not copy.
> 
> The user space owns the buffer and may overwrite the contents
> immediately after the ioctl.

Oh, right.  The exec ioctl doesn't block.  So this doesn't work (breaks
userspace abi).  Scratch the idea then.

> We also need a flag to indicate that the
> ownership of the buffer is transferred to the kernel.

Yes, with an additional flag for the changed behavior it could work.

cheers,
  Gerd

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[Gerd Hoffmann]

> > Completely different approach: use get_user_pages() and don't copy the
> > execbuffer at all.
> It would be really nice if execbuffer does not copy.
> 
> The user space owns the buffer and may overwrite the contents
> immediately after the ioctl.

Oh, right.  The exec ioctl doesn't block.  So this doesn't work (breaks
userspace abi).  Scratch the idea then.

> We also need a flag to indicate that the
> ownership of the buffer is transferred to the kernel.

Yes, with an additional flag for the changed behavior it could work.

cheers,
  Gerd

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[David Riley]

Hi Gerd,

On Fri, Aug 30, 2019 at 4:16 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > > -     kfree(vbuf->data_buf);
> > > > +     kvfree(vbuf->data_buf);
> > >
> > > if (is_vmalloc_addr(vbuf->data_buf)) ...
> > >
> > > needed here I gues?
> > >
> >
> > kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check.
>
> Ok.
>
> > - videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c,
> > assumes contiguous array of scatterlist and that the buffer being converted
> > is page aligned
>
> Well, vmalloc memory _is_ page aligned.

True, but this function gets called for all potential enqueuings (eg
resource_create_3d, resource_attach_backing) and I was concerned that
some other usage in the future might not have that guarantee.  But if
that's overly being paranoid, I'm willing to backtrack on that.

> sg_alloc_table_from_pages() does alot of what you need, you just need a
> small loop around vmalloc_to_page() create a struct page array
> beforehand.

That feels like an extra allocation when under memory pressure and
more work, to not gain much -- there still needs to be a function that
iterates through all the pages.  But I don't feel super strongly about
it and can change it if you think that it will be less maintenance
overhead.

> Completely different approach: use get_user_pages() and don't copy the
> execbuffer at all.

This one I'd rather not do unless we can show that the copy is
actually a problem.  As it stands I expect this to be a fallback
instead of the regular case, and if it's not then the VMs need to
revisit how long the control queue size is.  Right now most
execbuffers end up being copied into a single contiguous buffer which
results in 3 descriptors of the virtio queue.  If vmalloc ends up
being used (which is only a fallback because vmemdup_user tries
kmalloc first still), then there'll be 66 descriptors for a 256KB
buffer.   I think that's fine for exceptional cases, but not ideal if
it was commonplace.

Chia-I suggested that we could have a flag for the ioctl execbuffer
indicating that the buffer is BO to solve that, but again I'd prefer
to see that it's actually a problem.

I'll also be moving the sg table construction out of the critical
section and properly accounting for the required number of descriptors
before entering it in response to his comments.

Thanks,
Dave

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[David Riley]

Hi Gerd,

On Fri, Aug 30, 2019 at 4:16 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > > -     kfree(vbuf->data_buf);
> > > > +     kvfree(vbuf->data_buf);
> > >
> > > if (is_vmalloc_addr(vbuf->data_buf)) ...
> > >
> > > needed here I gues?
> > >
> >
> > kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check.
>
> Ok.
>
> > - videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c,
> > assumes contiguous array of scatterlist and that the buffer being converted
> > is page aligned
>
> Well, vmalloc memory _is_ page aligned.

True, but this function gets called for all potential enqueuings (eg
resource_create_3d, resource_attach_backing) and I was concerned that
some other usage in the future might not have that guarantee.  But if
that's overly being paranoid, I'm willing to backtrack on that.

> sg_alloc_table_from_pages() does alot of what you need, you just need a
> small loop around vmalloc_to_page() create a struct page array
> beforehand.

That feels like an extra allocation when under memory pressure and
more work, to not gain much -- there still needs to be a function that
iterates through all the pages.  But I don't feel super strongly about
it and can change it if you think that it will be less maintenance
overhead.

> Completely different approach: use get_user_pages() and don't copy the
> execbuffer at all.

This one I'd rather not do unless we can show that the copy is
actually a problem.  As it stands I expect this to be a fallback
instead of the regular case, and if it's not then the VMs need to
revisit how long the control queue size is.  Right now most
execbuffers end up being copied into a single contiguous buffer which
results in 3 descriptors of the virtio queue.  If vmalloc ends up
being used (which is only a fallback because vmemdup_user tries
kmalloc first still), then there'll be 66 descriptors for a 256KB
buffer.   I think that's fine for exceptional cases, but not ideal if
it was commonplace.

Chia-I suggested that we could have a flag for the ioctl execbuffer
indicating that the buffer is BO to solve that, but again I'd prefer
to see that it's actually a problem.

I'll also be moving the sg table construction out of the critical
section and properly accounting for the required number of descriptors
before entering it in response to his comments.

Thanks,
Dave

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[David Riley]

Hi Gerd,

On Fri, Aug 30, 2019 at 4:16 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > > -     kfree(vbuf->data_buf);
> > > > +     kvfree(vbuf->data_buf);
> > >
> > > if (is_vmalloc_addr(vbuf->data_buf)) ...
> > >
> > > needed here I gues?
> > >
> >
> > kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check.
>
> Ok.
>
> > - videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c,
> > assumes contiguous array of scatterlist and that the buffer being converted
> > is page aligned
>
> Well, vmalloc memory _is_ page aligned.

True, but this function gets called for all potential enqueuings (eg
resource_create_3d, resource_attach_backing) and I was concerned that
some other usage in the future might not have that guarantee.  But if
that's overly being paranoid, I'm willing to backtrack on that.

> sg_alloc_table_from_pages() does alot of what you need, you just need a
> small loop around vmalloc_to_page() create a struct page array
> beforehand.

That feels like an extra allocation when under memory pressure and
more work, to not gain much -- there still needs to be a function that
iterates through all the pages.  But I don't feel super strongly about
it and can change it if you think that it will be less maintenance
overhead.

> Completely different approach: use get_user_pages() and don't copy the
> execbuffer at all.

This one I'd rather not do unless we can show that the copy is
actually a problem.  As it stands I expect this to be a fallback
instead of the regular case, and if it's not then the VMs need to
revisit how long the control queue size is.  Right now most
execbuffers end up being copied into a single contiguous buffer which
results in 3 descriptors of the virtio queue.  If vmalloc ends up
being used (which is only a fallback because vmemdup_user tries
kmalloc first still), then there'll be 66 descriptors for a 256KB
buffer.   I think that's fine for exceptional cases, but not ideal if
it was commonplace.

Chia-I suggested that we could have a flag for the ioctl execbuffer
indicating that the buffer is BO to solve that, but again I'd prefer
to see that it's actually a problem.

I'll also be moving the sg table construction out of the critical
section and properly accounting for the required number of descriptors
before entering it in response to his comments.

Thanks,
Dave
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[Chia-I Wu]

On Fri, Aug 30, 2019 at 4:16 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > > -     kfree(vbuf->data_buf);
> > > > +     kvfree(vbuf->data_buf);
> > >
> > > if (is_vmalloc_addr(vbuf->data_buf)) ...
> > >
> > > needed here I gues?
> > >
> >
> > kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check.
>
> Ok.
>
> > - videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c,
> > assumes contiguous array of scatterlist and that the buffer being converted
> > is page aligned
>
> Well, vmalloc memory _is_ page aligned.
>
> sg_alloc_table_from_pages() does alot of what you need, you just need a
> small loop around vmalloc_to_page() create a struct page array
> beforehand.
>
> Completely different approach: use get_user_pages() and don't copy the
> execbuffer at all.
It would be really nice if execbuffer does not copy.

The user space owns the buffer and may overwrite the contents
immediately after the ioctl.  We also need a flag to indicate that the
ownership of the buffer is transferred to the kernel.




>
> cheers,
>   Gerd
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[Chia-I Wu]

On Fri, Aug 30, 2019 at 4:16 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > > -     kfree(vbuf->data_buf);
> > > > +     kvfree(vbuf->data_buf);
> > >
> > > if (is_vmalloc_addr(vbuf->data_buf)) ...
> > >
> > > needed here I gues?
> > >
> >
> > kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check.
>
> Ok.
>
> > - videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c,
> > assumes contiguous array of scatterlist and that the buffer being converted
> > is page aligned
>
> Well, vmalloc memory _is_ page aligned.
>
> sg_alloc_table_from_pages() does alot of what you need, you just need a
> small loop around vmalloc_to_page() create a struct page array
> beforehand.
>
> Completely different approach: use get_user_pages() and don't copy the
> execbuffer at all.
It would be really nice if execbuffer does not copy.

The user space owns the buffer and may overwrite the contents
immediately after the ioctl.  We also need a flag to indicate that the
ownership of the buffer is transferred to the kernel.




>
> cheers,
>   Gerd
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[Chia-I Wu]

On Thu, Aug 29, 2019 at 2:24 PM David Riley <davidriley@chromium.org> wrote:
>
> Userspace requested command buffer allocations could be too large
> to make as a contiguous allocation.  Use vmalloc if necessary to
> satisfy those allocations.
>
> Signed-off-by: David Riley <davidriley@chromium.org>
> ---
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c |  4 +-
>  drivers/gpu/drm/virtio/virtgpu_vq.c    | 74 ++++++++++++++++++++++++--
>  2 files changed, 73 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index ac60be9b5c19..a8732a8af766 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -195,7 +195,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>         if (ret)
>                 goto out_free;
>
> -       buf = memdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
> +       buf = vmemdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
>         if (IS_ERR(buf)) {
>                 ret = PTR_ERR(buf);
>                 goto out_unresv;
> @@ -230,7 +230,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>         return 0;
>
>  out_memdup:
> -       kfree(buf);
> +       kvfree(buf);
>  out_unresv:
>         ttm_eu_backoff_reservation(&ticket, &validate_list);
>  out_free:
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index 981ee16e3ee9..bcbc48b7284f 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -154,7 +154,7 @@ static void free_vbuf(struct virtio_gpu_device *vgdev,
>  {
>         if (vbuf->resp_size > MAX_INLINE_RESP_SIZE)
>                 kfree(vbuf->resp_buf);
> -       kfree(vbuf->data_buf);
> +       kvfree(vbuf->data_buf);
>         kmem_cache_free(vgdev->vbufs, vbuf);
>  }
>
> @@ -251,6 +251,59 @@ void virtio_gpu_dequeue_cursor_func(struct work_struct *work)
>         wake_up(&vgdev->cursorq.ack_queue);
>  }
>
> +/* How many bytes left in this page. */
> +static unsigned int rest_of_page(void *data)
> +{
> +       return PAGE_SIZE - offset_in_page(data);
> +}
> +
> +/* Create sg_table from a vmalloc'd buffer. */
> +static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size)
> +{
> +       int nents, ret, s, i;
> +       struct sg_table *sgt;
> +       struct scatterlist *sg;
> +       struct page *pg;
> +
> +       sgt = kmalloc(sizeof(*sgt), GFP_KERNEL);
> +       if (!sgt)
> +               return NULL;
> +
> +       nents = DIV_ROUND_UP(size, PAGE_SIZE) + 1;
> +       ret = sg_alloc_table(sgt, nents, GFP_KERNEL);
> +       if (ret) {
> +               kfree(sgt);
> +               return NULL;
> +       }
> +
> +       for_each_sg(sgt->sgl, sg, nents, i) {
> +               pg = vmalloc_to_page(data);
> +               if (!pg) {
> +                       sg_free_table(sgt);
> +                       kfree(sgt);
> +                       return NULL;
> +               }
> +
> +               s = rest_of_page(data);
> +               if (s > size)
> +                       s = size;
> +
> +               sg_set_page(sg, pg, s, offset_in_page(data));
> +
> +               size -= s;
> +               data += s;
> +
> +               if (size) {
> +                       sg_unmark_end(sg);
> +               } else {
> +                       sg_mark_end(sg);
> +                       break;
> +               }
> +       }
> +
> +       return sgt;
> +}
> +
>  static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
>                                                struct virtio_gpu_vbuffer *vbuf)
>                 __releases(&vgdev->ctrlq.qlock)
> @@ -260,6 +313,7 @@ static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
>         struct scatterlist *sgs[3], vcmd, vout, vresp;
>         int outcnt = 0, incnt = 0;
>         int ret;
> +       struct sg_table *sgt = NULL;
>
>         if (!vgdev->vqs_ready)
>                 return -ENODEV;
> @@ -269,8 +323,17 @@ static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
>         outcnt++;
>
>         if (vbuf->data_size) {
> -               sg_init_one(&vout, vbuf->data_buf, vbuf->data_size);
> -               sgs[outcnt + incnt] = &vout;
> +               if (is_vmalloc_addr(vbuf->data_buf)) {
> +                       spin_unlock(&vgdev->ctrlq.qlock);
> +                       sgt = vmalloc_to_sgt(vbuf->data_buf, vbuf->data_size);
> +                       spin_lock(&vgdev->ctrlq.qlock);
> +                       if (!sgt)
> +                               return -ENOMEM;
> +                       sgs[outcnt + incnt] = sgt->sgl;
If the construction of sgs is no longer atomic, it should be moved out
of the critical section.
> +               } else {
> +                       sg_init_one(&vout, vbuf->data_buf, vbuf->data_size);
> +                       sgs[outcnt + incnt] = &vout;
> +               }
>                 outcnt++;
>         }
>
> @@ -294,6 +357,11 @@ static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
>                 virtqueue_kick(vq);
>         }
>
> +       if (sgt) {
> +               sg_free_table(sgt);
> +               kfree(sgt);
> +       }
> +
>         if (!ret)
>                 ret = vq->num_free;
>         return ret;
> --
> 2.23.0.187.g17f5b7556c-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[Chia-I Wu]

On Thu, Aug 29, 2019 at 2:24 PM David Riley <davidriley@chromium.org> wrote:
>
> Userspace requested command buffer allocations could be too large
> to make as a contiguous allocation.  Use vmalloc if necessary to
> satisfy those allocations.
>
> Signed-off-by: David Riley <davidriley@chromium.org>
> ---
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c |  4 +-
>  drivers/gpu/drm/virtio/virtgpu_vq.c    | 74 ++++++++++++++++++++++++--
>  2 files changed, 73 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index ac60be9b5c19..a8732a8af766 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -195,7 +195,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>         if (ret)
>                 goto out_free;
>
> -       buf = memdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
> +       buf = vmemdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
>         if (IS_ERR(buf)) {
>                 ret = PTR_ERR(buf);
>                 goto out_unresv;
> @@ -230,7 +230,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>         return 0;
>
>  out_memdup:
> -       kfree(buf);
> +       kvfree(buf);
>  out_unresv:
>         ttm_eu_backoff_reservation(&ticket, &validate_list);
>  out_free:
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index 981ee16e3ee9..bcbc48b7284f 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -154,7 +154,7 @@ static void free_vbuf(struct virtio_gpu_device *vgdev,
>  {
>         if (vbuf->resp_size > MAX_INLINE_RESP_SIZE)
>                 kfree(vbuf->resp_buf);
> -       kfree(vbuf->data_buf);
> +       kvfree(vbuf->data_buf);
>         kmem_cache_free(vgdev->vbufs, vbuf);
>  }
>
> @@ -251,6 +251,59 @@ void virtio_gpu_dequeue_cursor_func(struct work_struct *work)
>         wake_up(&vgdev->cursorq.ack_queue);
>  }
>
> +/* How many bytes left in this page. */
> +static unsigned int rest_of_page(void *data)
> +{
> +       return PAGE_SIZE - offset_in_page(data);
> +}
> +
> +/* Create sg_table from a vmalloc'd buffer. */
> +static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size)
> +{
> +       int nents, ret, s, i;
> +       struct sg_table *sgt;
> +       struct scatterlist *sg;
> +       struct page *pg;
> +
> +       sgt = kmalloc(sizeof(*sgt), GFP_KERNEL);
> +       if (!sgt)
> +               return NULL;
> +
> +       nents = DIV_ROUND_UP(size, PAGE_SIZE) + 1;
> +       ret = sg_alloc_table(sgt, nents, GFP_KERNEL);
> +       if (ret) {
> +               kfree(sgt);
> +               return NULL;
> +       }
> +
> +       for_each_sg(sgt->sgl, sg, nents, i) {
> +               pg = vmalloc_to_page(data);
> +               if (!pg) {
> +                       sg_free_table(sgt);
> +                       kfree(sgt);
> +                       return NULL;
> +               }
> +
> +               s = rest_of_page(data);
> +               if (s > size)
> +                       s = size;
> +
> +               sg_set_page(sg, pg, s, offset_in_page(data));
> +
> +               size -= s;
> +               data += s;
> +
> +               if (size) {
> +                       sg_unmark_end(sg);
> +               } else {
> +                       sg_mark_end(sg);
> +                       break;
> +               }
> +       }
> +
> +       return sgt;
> +}
> +
>  static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
>                                                struct virtio_gpu_vbuffer *vbuf)
>                 __releases(&vgdev->ctrlq.qlock)
> @@ -260,6 +313,7 @@ static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
>         struct scatterlist *sgs[3], vcmd, vout, vresp;
>         int outcnt = 0, incnt = 0;
>         int ret;
> +       struct sg_table *sgt = NULL;
>
>         if (!vgdev->vqs_ready)
>                 return -ENODEV;
> @@ -269,8 +323,17 @@ static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
>         outcnt++;
>
>         if (vbuf->data_size) {
> -               sg_init_one(&vout, vbuf->data_buf, vbuf->data_size);
> -               sgs[outcnt + incnt] = &vout;
> +               if (is_vmalloc_addr(vbuf->data_buf)) {
> +                       spin_unlock(&vgdev->ctrlq.qlock);
> +                       sgt = vmalloc_to_sgt(vbuf->data_buf, vbuf->data_size);
> +                       spin_lock(&vgdev->ctrlq.qlock);
> +                       if (!sgt)
> +                               return -ENOMEM;
> +                       sgs[outcnt + incnt] = sgt->sgl;
If the construction of sgs is no longer atomic, it should be moved out
of the critical section.
> +               } else {
> +                       sg_init_one(&vout, vbuf->data_buf, vbuf->data_size);
> +                       sgs[outcnt + incnt] = &vout;
> +               }
>                 outcnt++;
>         }
>
> @@ -294,6 +357,11 @@ static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
>                 virtqueue_kick(vq);
>         }
>
> +       if (sgt) {
> +               sg_free_table(sgt);
> +               kfree(sgt);
> +       }
> +
>         if (!ret)
>                 ret = vq->num_free;
>         return ret;
> --
> 2.23.0.187.g17f5b7556c-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[Gerd Hoffmann]

  Hi,

> > > -     kfree(vbuf->data_buf);
> > > +     kvfree(vbuf->data_buf);
> >
> > if (is_vmalloc_addr(vbuf->data_buf)) ...
> >
> > needed here I gues?
> >
> 
> kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check.

Ok.

> - videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c,
> assumes contiguous array of scatterlist and that the buffer being converted
> is page aligned

Well, vmalloc memory _is_ page aligned.

sg_alloc_table_from_pages() does alot of what you need, you just need a
small loop around vmalloc_to_page() create a struct page array
beforehand.

Completely different approach: use get_user_pages() and don't copy the
execbuffer at all.

cheers,
  Gerd

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[Gerd Hoffmann]

  Hi,

> > > -     kfree(vbuf->data_buf);
> > > +     kvfree(vbuf->data_buf);
> >
> > if (is_vmalloc_addr(vbuf->data_buf)) ...
> >
> > needed here I gues?
> >
> 
> kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check.

Ok.

> - videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c,
> assumes contiguous array of scatterlist and that the buffer being converted
> is page aligned

Well, vmalloc memory _is_ page aligned.

sg_alloc_table_from_pages() does alot of what you need, you just need a
small loop around vmalloc_to_page() create a struct page array
beforehand.

Completely different approach: use get_user_pages() and don't copy the
execbuffer at all.

cheers,
  Gerd

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[David Riley]

[-- Attachment #1.1: Type: text/plain, Size: 1662 bytes --]

On Thu, Aug 29, 2019 at 11:09 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
>
> >  {
> >       if (vbuf->resp_size > MAX_INLINE_RESP_SIZE)
> >               kfree(vbuf->resp_buf);
> > -     kfree(vbuf->data_buf);
> > +     kvfree(vbuf->data_buf);
>
> if (is_vmalloc_addr(vbuf->data_buf)) ...
>
> needed here I gues?
>

kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check.


>
> > +/* Create sg_table from a vmalloc'd buffer. */
> > +static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size)
>
> Hmm, isn't there an existing function for that?
> I'd be surprised if virtio-gpu is the first driver needing this ...
>
> And it case there really isn't one this should probably added to the
> vmalloc or scatterlist code, not the virtio-gpu driver.
>

There's a few other similar ones around:
- pack_sg_list in net/9p/trans_virtio.c, assumes contiguous array of
scatterlist and non-vmalloc
- videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c,
assumes contiguous array of scatterlist and that the buffer being converted
is page aligned (the l
- vmalloc_to_sg() in drivers/media/common/saa7146/saa7146_core.c, duplicate
of videobuf_vmalloc_to_sg

None of the existing ones seemed to do what was needed and the convention
seemed to pack an sg table as needed for the usage and just keep it in the
module so that's what I followed.  I'm not very familiar with these APIs so
maybe there's something I missed, but I did look through the helpers in
lib/scatterlist.c and didn't see anything.  If you think it is better
suited to live in scatterlist I can prepare another change for that.

Dave

>
> cheers,
>   Gerd
>
>

[-- Attachment #1.2: Type: text/html, Size: 2545 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[David Riley]

[-- Attachment #1.1: Type: text/plain, Size: 1662 bytes --]

On Thu, Aug 29, 2019 at 11:09 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
>
> >  {
> >       if (vbuf->resp_size > MAX_INLINE_RESP_SIZE)
> >               kfree(vbuf->resp_buf);
> > -     kfree(vbuf->data_buf);
> > +     kvfree(vbuf->data_buf);
>
> if (is_vmalloc_addr(vbuf->data_buf)) ...
>
> needed here I gues?
>

kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check.


>
> > +/* Create sg_table from a vmalloc'd buffer. */
> > +static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size)
>
> Hmm, isn't there an existing function for that?
> I'd be surprised if virtio-gpu is the first driver needing this ...
>
> And it case there really isn't one this should probably added to the
> vmalloc or scatterlist code, not the virtio-gpu driver.
>

There's a few other similar ones around:
- pack_sg_list in net/9p/trans_virtio.c, assumes contiguous array of
scatterlist and non-vmalloc
- videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c,
assumes contiguous array of scatterlist and that the buffer being converted
is page aligned (the l
- vmalloc_to_sg() in drivers/media/common/saa7146/saa7146_core.c, duplicate
of videobuf_vmalloc_to_sg

None of the existing ones seemed to do what was needed and the convention
seemed to pack an sg table as needed for the usage and just keep it in the
module so that's what I followed.  I'm not very familiar with these APIs so
maybe there's something I missed, but I did look through the helpers in
lib/scatterlist.c and didn't see anything.  If you think it is better
suited to live in scatterlist I can prepare another change for that.

Dave

>
> cheers,
>   Gerd
>
>

[-- Attachment #1.2: Type: text/html, Size: 2545 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[Gerd Hoffmann]

  Hi,

>  {
>  	if (vbuf->resp_size > MAX_INLINE_RESP_SIZE)
>  		kfree(vbuf->resp_buf);
> -	kfree(vbuf->data_buf);
> +	kvfree(vbuf->data_buf);

if (is_vmalloc_addr(vbuf->data_buf)) ...

needed here I gues?

> +/* Create sg_table from a vmalloc'd buffer. */
> +static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size)

Hmm, isn't there an existing function for that?
I'd be surprised if virtio-gpu is the first driver needing this ...

And it case there really isn't one this should probably added to the
vmalloc or scatterlist code, not the virtio-gpu driver.

cheers,
  Gerd

Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[Gerd Hoffmann]

  Hi,

>  {
>  	if (vbuf->resp_size > MAX_INLINE_RESP_SIZE)
>  		kfree(vbuf->resp_buf);
> -	kfree(vbuf->data_buf);
> +	kvfree(vbuf->data_buf);

if (is_vmalloc_addr(vbuf->data_buf)) ...

needed here I gues?

> +/* Create sg_table from a vmalloc'd buffer. */
> +static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size)

Hmm, isn't there an existing function for that?
I'd be surprised if virtio-gpu is the first driver needing this ...

And it case there really isn't one this should probably added to the
vmalloc or scatterlist code, not the virtio-gpu driver.

cheers,
  Gerd

[PATCH] drm/virtio: Use vmalloc for command buffer allocations.

[David Riley]

Userspace requested command buffer allocations could be too large
to make as a contiguous allocation.  Use vmalloc if necessary to
satisfy those allocations.

Signed-off-by: David Riley <davidriley@chromium.org>
---
 drivers/gpu/drm/virtio/virtgpu_ioctl.c |  4 +-
 drivers/gpu/drm/virtio/virtgpu_vq.c    | 74 ++++++++++++++++++++++++--
 2 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index ac60be9b5c19..a8732a8af766 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -195,7 +195,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto out_free;
 
-	buf = memdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
+	buf = vmemdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
 	if (IS_ERR(buf)) {
 		ret = PTR_ERR(buf);
 		goto out_unresv;
@@ -230,7 +230,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
 	return 0;
 
 out_memdup:
-	kfree(buf);
+	kvfree(buf);
 out_unresv:
 	ttm_eu_backoff_reservation(&ticket, &validate_list);
 out_free:
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 981ee16e3ee9..bcbc48b7284f 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -154,7 +154,7 @@ static void free_vbuf(struct virtio_gpu_device *vgdev,
 {
 	if (vbuf->resp_size > MAX_INLINE_RESP_SIZE)
 		kfree(vbuf->resp_buf);
-	kfree(vbuf->data_buf);
+	kvfree(vbuf->data_buf);
 	kmem_cache_free(vgdev->vbufs, vbuf);
 }
 
@@ -251,6 +251,59 @@ void virtio_gpu_dequeue_cursor_func(struct work_struct *work)
 	wake_up(&vgdev->cursorq.ack_queue);
 }
 
+/* How many bytes left in this page. */
+static unsigned int rest_of_page(void *data)
+{
+	return PAGE_SIZE - offset_in_page(data);
+}
+
+/* Create sg_table from a vmalloc'd buffer. */
+static struct sg_table *vmalloc_to_sgt(char *data, uint32_t size)
+{
+	int nents, ret, s, i;
+	struct sg_table *sgt;
+	struct scatterlist *sg;
+	struct page *pg;
+
+	sgt = kmalloc(sizeof(*sgt), GFP_KERNEL);
+	if (!sgt)
+		return NULL;
+
+	nents = DIV_ROUND_UP(size, PAGE_SIZE) + 1;
+	ret = sg_alloc_table(sgt, nents, GFP_KERNEL);
+	if (ret) {
+		kfree(sgt);
+		return NULL;
+	}
+
+	for_each_sg(sgt->sgl, sg, nents, i) {
+		pg = vmalloc_to_page(data);
+		if (!pg) {
+			sg_free_table(sgt);
+			kfree(sgt);
+			return NULL;
+		}
+
+		s = rest_of_page(data);
+		if (s > size)
+			s = size;
+
+		sg_set_page(sg, pg, s, offset_in_page(data));
+
+		size -= s;
+		data += s;
+
+		if (size) {
+			sg_unmark_end(sg);
+		} else {
+			sg_mark_end(sg);
+			break;
+		}
+	}
+
+	return sgt;
+}
+
 static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
 					       struct virtio_gpu_vbuffer *vbuf)
 		__releases(&vgdev->ctrlq.qlock)
@@ -260,6 +313,7 @@ static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
 	struct scatterlist *sgs[3], vcmd, vout, vresp;
 	int outcnt = 0, incnt = 0;
 	int ret;
+	struct sg_table *sgt = NULL;
 
 	if (!vgdev->vqs_ready)
 		return -ENODEV;
@@ -269,8 +323,17 @@ static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
 	outcnt++;
 
 	if (vbuf->data_size) {
-		sg_init_one(&vout, vbuf->data_buf, vbuf->data_size);
-		sgs[outcnt + incnt] = &vout;
+		if (is_vmalloc_addr(vbuf->data_buf)) {
+			spin_unlock(&vgdev->ctrlq.qlock);
+			sgt = vmalloc_to_sgt(vbuf->data_buf, vbuf->data_size);
+			spin_lock(&vgdev->ctrlq.qlock);
+			if (!sgt)
+				return -ENOMEM;
+			sgs[outcnt + incnt] = sgt->sgl;
+		} else {
+			sg_init_one(&vout, vbuf->data_buf, vbuf->data_size);
+			sgs[outcnt + incnt] = &vout;
+		}
 		outcnt++;
 	}
 
@@ -294,6 +357,11 @@ static int virtio_gpu_queue_ctrl_buffer_locked(struct virtio_gpu_device *vgdev,
 		virtqueue_kick(vq);
 	}
 
+	if (sgt) {
+		sg_free_table(sgt);
+		kfree(sgt);
+	}
+
 	if (!ret)
 		ret = vq->num_free;
 	return ret;
-- 
2.23.0.187.g17f5b7556c-goog