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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel