Re: [PATCH v2 22/23] drm/virtio: implement blob resources: resource create blob ioctl

[Gurchetan Singh <gurchetansingh@chromium.org>]

On Thu, Sep 3, 2020 at 2:11 PM Chia-I Wu <olvaffe@gmail.com> wrote:

On Wed, Sep 2, 2020 at 2:09 PM Gurchetan Singh
<gurchetansingh@chromium.org> wrote:
>
> From: Gerd Hoffmann <kraxel@redhat.com>
>
> Implement resource create blob as specified.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Co-developed-by: Gurchetan Singh <gurchetansingh@chromium.org>
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> Acked-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> drivers/gpu/drm/virtio/virtgpu_drv.h  | Â4 +-
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 136 ++++++++++++++++++++++++
>Â drivers/gpu/drm/virtio/virtgpu_object.c |Â Â5 +-
> drivers/gpu/drm/virtio/virtgpu_vram.c Â| Â2 +
>Â 4 files changed, 144 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 6162865c162df..d2ea199dbdb90 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -257,8 +257,8 @@ struct virtio_gpu_fpriv {
>Â Â Â Â Âstruct mutex context_lock;
>Â };
>
> -/* virtgpu_ioctl.c */
> -#define DRM_VIRTIO_NUM_IOCTLS 10
> +/* virtio_ioctl.c */
> +#define DRM_VIRTIO_NUM_IOCTLS 11
>Â extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
>Â void virtio_gpu_create_context(struct drm_device *dev, struct drm_file *file);
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index 7dbe24248a200..442cbca59c8a5 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -34,6 +34,10 @@
>
>Â #include "virtgpu_drv.h"
>
> +#define VIRTGPU_BLOB_FLAG_USE_MASK (VIRTGPU_BLOB_FLAG_USE_MAPPABLE | \
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂVIRTGPU_BLOB_FLAG_USE_SHAREABLE | \
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂVIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE)
> +
>Â void virtio_gpu_create_context(struct drm_device *dev, struct drm_file *file)
>Â {
>Â Â Â Â Âstruct virtio_gpu_device *vgdev = dev->dev_private;
> @@ -520,6 +524,134 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device *dev,
>Â Â Â Â Âreturn 0;
>Â }
>
> +static int verify_blob(struct virtio_gpu_device *vgdev,
> +Â Â Â Â Â Â Â Â Â Â Â struct virtio_gpu_fpriv *vfpriv,
> +Â Â Â Â Â Â Â Â Â Â Â struct virtio_gpu_object_params *params,
> +Â Â Â Â Â Â Â Â Â Â Â struct drm_virtgpu_resource_create_blob *rc_blob,
> +Â Â Â Â Â Â Â Â Â Â Â bool *guest_blob, bool *host3d_blob)
> +{
> +Â Â Â Âif (!vgdev->has_resource_blob)
> +Â Â Â Â Â Â Â Âreturn -EINVAL;
> +
> +Â Â Â Âif ((rc_blob->blob_flags & ~VIRTGPU_BLOB_FLAG_USE_MASK) ||
> +Â Â Â Â Â Â!rc_blob->blob_flags)
> +Â Â Â Â Â Â Â Âreturn -EINVAL;
> +
> +Â Â Â Âif (rc_blob->blob_flags & VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE) {
> +Â Â Â Â Â Â Â Âif (!vgdev->has_resource_assign_uuid)
> +Â Â Â Â Â Â Â Â Â Â Â Âreturn -EINVAL;
> +Â Â Â Â}
> +
> +Â Â Â Âswitch (rc_blob->blob_mem) {
> +Â Â Â Âcase VIRTGPU_BLOB_MEM_GUEST:
> +Â Â Â Â Â Â Â Â*guest_blob = true;
> +Â Â Â Â Â Â Â Âbreak;
> +Â Â Â Âcase VIRTGPU_BLOB_MEM_HOST3D_GUEST:
> +Â Â Â Â Â Â Â Â*guest_blob = true;
> +Â Â Â Â Â Â Â Âfallthrough;
> +Â Â Â Âcase VIRTGPU_BLOB_MEM_HOST3D:
> +Â Â Â Â Â Â Â Â*host3d_blob = true;
> +Â Â Â Â Â Â Â Âbreak;
> +Â Â Â Âdefault:
> +Â Â Â Â Â Â Â Âreturn -EINVAL;
> +Â Â Â Â}
> +
> +Â Â Â Âif (*host3d_blob) {
> +Â Â Â Â Â Â Â Âif (!vgdev->has_virgl_3d)
> +Â Â Â Â Â Â Â Â Â Â Â Âreturn -EINVAL;
> +
> +Â Â Â Â Â Â Â Â/* Must be dword aligned. */
> +Â Â Â Â Â Â Â Âif (rc_blob->cmd_size % 4 != 0)
> +Â Â Â Â Â Â Â Â Â Â Â Âreturn -EINVAL;
> +
> +Â Â Â Â Â Â Â Âparams->ctx_id = vfpriv->ctx_id;
> +Â Â Â Â Â Â Â Âparams->blob_id = rc_blob->blob_id;
> +Â Â Â Â} else {
> +Â Â Â Â Â Â Â Âif (rc_blob->blob_id != 0)
> +Â Â Â Â Â Â Â Â Â Â Â Âreturn -EINVAL;
> +
> +Â Â Â Â Â Â Â Âif (rc_blob->cmd_size != 0)
> +Â Â Â Â Â Â Â Â Â Â Â Âreturn -EINVAL;
> +Â Â Â Â}
> +
> +Â Â Â Âparams->blob_mem = rc_blob->blob_mem;
> +Â Â Â Âparams->size = rc_blob->size;
> +Â Â Â Âparams->blob = true;
> +Â Â Â Âparams->blob_flags = rc_blob->blob_flags;
> +Â Â Â Âreturn 0;
> +}
> +
> +static int virtio_gpu_resource_create_blob(struct drm_device *dev,
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â void *data, struct drm_file *file)
> +{
> +Â Â Â Âint ret = 0;
> +Â Â Â Âuint32_t handle = 0;
> +Â Â Â Âbool guest_blob = false;
> +Â Â Â Âbool host3d_blob = false;
> +Â Â Â Âstruct drm_gem_object *obj;
> +Â Â Â Âstruct virtio_gpu_object *bo;
> +Â Â Â Âstruct virtio_gpu_object_params params = { 0 };
> +Â Â Â Âstruct virtio_gpu_device *vgdev = dev->dev_private;
> +Â Â Â Âstruct virtio_gpu_fpriv *vfpriv = file->driver_priv;
> +Â Â Â Âstruct drm_virtgpu_resource_create_blob *rc_blob = data;
> +
> +Â Â Â Âif (verify_blob(vgdev, vfpriv, &params, rc_blob,
> +Â Â Â Â Â Â Â Â Â Â Â Â&guest_blob, &host3d_blob))
> +Â Â Â Â Â Â Â Âreturn -EINVAL;
> +
> +Â Â Â Âif (vgdev->has_virgl_3d)
> +Â Â Â Â Â Â Â Âvirtio_gpu_create_context(dev, file);
> +
> +Â Â Â Âif (rc_blob->cmd_size) {
> +Â Â Â Â Â Â Â Âvoid *buf;
> +
> +Â Â Â Â Â Â Â Âbuf = memdup_user(u64_to_user_ptr(rc_blob->cmd),
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Ârc_blob->cmd_size);
> +
> +Â Â Â Â Â Â Â Âif (IS_ERR(buf))
> +Â Â Â Â Â Â Â Â Â Â Â Âreturn PTR_ERR(buf);
> +
> +Â Â Â Â Â Â Â Âvirtio_gpu_cmd_submit(vgdev, buf, rc_blob->cmd_size,
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âvfpriv->ctx_id, NULL, NULL);
> +Â Â Â Â}
> +
> +Â Â Â Âif (guest_blob)
> +Â Â Â Â Â Â Â Âret = virtio_gpu_object_create(vgdev, &params, &bo, NULL);
> +Â Â Â Âelse if (!guest_blob && host3d_blob)
> +Â Â Â Â Â Â Â Âret = virtio_gpu_vram_create(vgdev, &params, &bo);
When cmd_size != 0, a host blob has been allocated. Will it be leaked
if virtio_gpu_{object,vram}_create fails?

The actual kick happens as a result ofÂdrm_gem_handle_create (similarÂto drm_virtgpu_resource_create), so the host-side allocation doesn't happen if the virtio_gpu_{object,vram}_create sanity checks fail. We could unqueue items from the virtqueue on the failure cases, but blob is big enough already and userspace doesn't really recoverÂgracefully if an ioctl fails either.

> +Â Â Â Âelse
> +Â Â Â Â Â Â Â Âreturn -EINVAL;
> +
> +Â Â Â Âif (ret < 0)
> +Â Â Â Â Â Â Â Âreturn ret;
> +
> +Â Â Â Âbo->guest_blob = guest_blob;
> +Â Â Â Âbo->host3d_blob = host3d_blob;
> +Â Â Â Âbo->blob_mem = rc_blob->blob_mem;
> +Â Â Â Âbo->blob_flags = rc_blob->blob_flags;
> +
> +Â Â Â Âobj = &bo->base.base;
> +Â Â Â Âif (params.blob_flags & VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE) {
> +Â Â Â Â Â Â Â Âret = virtio_gpu_resource_assign_uuid(vgdev, bo);
> +Â Â Â Â Â Â Â Âif (ret) {
> +Â Â Â Â Â Â Â Â Â Â Â Âdrm_gem_object_release(obj);
> +Â Â Â Â Â Â Â Â Â Â Â Âreturn ret;
> +Â Â Â Â Â Â Â Â}
> +Â Â Â Â}
> +
> +Â Â Â Âret = drm_gem_handle_create(file, obj, &handle);
> +Â Â Â Âif (ret) {
> +Â Â Â Â Â Â Â Âdrm_gem_object_release(obj);
> +Â Â Â Â Â Â Â Âreturn ret;
> +Â Â Â Â}
> +Â Â Â Âdrm_gem_object_put(obj);
> +
> +Â Â Â Ârc_blob->res_handle = bo->hw_res_handle;
> +Â Â Â Ârc_blob->bo_handle = handle;
> +
> +Â Â Â Âreturn 0;
> +}
> +
>Â struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
>Â Â Â Â ÂDRM_IOCTL_DEF_DRV(VIRTGPU_MAP, virtio_gpu_map_ioctl,
>Â Â Â Â Â Â Â Â Â Â Â Â Â ÂDRM_RENDER_ALLOW),
> @@ -552,4 +684,8 @@ struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
>
>Â Â Â Â ÂDRM_IOCTL_DEF_DRV(VIRTGPU_GET_CAPS, virtio_gpu_get_caps_ioctl,
>Â Â Â Â Â Â Â Â Â Â Â Â Â ÂDRM_RENDER_ALLOW),
> +
> +Â Â Â ÂDRM_IOCTL_DEF_DRV(VIRTGPU_RESOURCE_CREATE_BLOB,
> +Â Â Â Â Â Â Â Â Â Â Â Â Âvirtio_gpu_resource_create_blob,
> +Â Â Â Â Â Â Â Â Â Â Â Â ÂDRM_RENDER_ALLOW),
>Â };
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
> index cef79455257df..258b4eeae7c2c 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -244,7 +244,10 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
>Â Â Â Â Â Â Â Â Âreturn ret;
>Â Â Â Â Â}
>
> -Â Â Â Âif (params->virgl) {
> +Â Â Â Âif (params->blob) {
> +Â Â Â Â Â Â Â Âvirtio_gpu_cmd_resource_create_blob(vgdev, bo, params,
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âents, nents);
> +Â Â Â Â} else if (params->virgl) {
>Â Â Â Â Â Â Â Â Âvirtio_gpu_cmd_resource_create_3d(vgdev, bo, params,
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âobjs, fence);
>Â Â Â Â Â Â Â Â Âvirtio_gpu_object_attach(vgdev, bo, ents, nents);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vram.c b/drivers/gpu/drm/virtio/virtgpu_vram.c
> index 087945fcd230f..23c21bc4d01e2 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vram.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vram.c
> @@ -149,6 +149,8 @@ int virtio_gpu_vram_create(struct virtio_gpu_device *vgdev,
>Â Â Â Â Â Â Â Â Âreturn ret;
>Â Â Â Â Â}
>
> +Â Â Â Âvirtio_gpu_cmd_resource_create_blob(vgdev, &vram->base, params, NULL,
> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â0);
>Â Â Â Â Âif (params->blob_flags & VIRTGPU_BLOB_FLAG_USE_MAPPABLE) {
>Â Â Â Â Â Â Â Â Âret = virtio_gpu_vram_map(&vram->base);
>Â Â Â Â Â Â Â Â Âif (ret) {
> --
> 2.26.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel