Re: [PATCH v6 6/6] drm/i915/gvt: Adding interface so user space can get the dma-buf

[Alex Williamson]

On Fri, 2 Jun 2017 03:24:35 +0000
"Chen, Xiaoguang" <xiaoguang.chen@xxxxxxxxx> wrote:

> Hi Alex,
> 
> >-----Original Message-----
> >From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
> >Sent: Friday, June 02, 2017 2:08 AM
> >To: Chen, Xiaoguang <xiaoguang.chen@xxxxxxxxx>
> >Cc: kraxel@xxxxxxxxxx; chris@xxxxxxxxxxxxxxxxxx; intel-
> >gfx@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> >zhenyuw@xxxxxxxxxxxxxxx; Lv, Zhiyuan <zhiyuan.lv@xxxxxxxxx>; intel-gvt-
> >dev@xxxxxxxxxxxxxxxxxxxxx; Wang, Zhi A <zhi.a.wang@xxxxxxxxx>; Tian, Kevin
> ><kevin.tian@xxxxxxxxx>
> >Subject: Re: [PATCH v6 6/6] drm/i915/gvt: Adding interface so user space can get
> >the dma-buf
> >
> >On Sat, 27 May 2017 16:38:52 +0800
> >Xiaoguang Chen <xiaoguang.chen@xxxxxxxxx> wrote:
> >  
> >> User space should create the management fd for the dma-buf operation first.
> >> Then user can query the plane information and create dma-buf if
> >> necessary using the management fd.
> >>
> >> Signed-off-by: Xiaoguang Chen <xiaoguang.chen@xxxxxxxxx>
> >> ---
> >>  drivers/gpu/drm/i915/gvt/dmabuf.c |  12 ++++
> >>  drivers/gpu/drm/i915/gvt/dmabuf.h |   5 ++
> >>  drivers/gpu/drm/i915/gvt/gvt.c    |   2 +
> >>  drivers/gpu/drm/i915/gvt/gvt.h    |   5 ++
> >>  drivers/gpu/drm/i915/gvt/kvmgt.c  | 144  
> >++++++++++++++++++++++++++++++++++++++  
> >>  drivers/gpu/drm/i915/gvt/vgpu.c   |   1 +
> >>  6 files changed, 169 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c
> >> b/drivers/gpu/drm/i915/gvt/dmabuf.c
> >> index c831e91..9759e9a 100644
> >> --- a/drivers/gpu/drm/i915/gvt/dmabuf.c
> >> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
> >> @@ -226,6 +226,7 @@ int intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu,  
> >void *args)  
> >>  	struct vfio_vgpu_dmabuf_info *gvt_dmabuf = args;
> >>  	struct intel_vgpu_fb_info *fb_info;
> >>  	int ret;
> >> +	struct intel_vgpu_dmabuf_obj *dmabuf_obj;
> >>
> >>  	ret = intel_vgpu_get_plane_info(dev, vgpu, &gvt_dmabuf->plane_info);
> >>  	if (ret != 0)
> >> @@ -263,6 +264,17 @@ int intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu,  
> >void *args)  
> >>  		gvt_vgpu_err("create dma-buf fd failed ret:%d\n", ret);
> >>  		return ret;
> >>  	}
> >> +	dmabuf_obj = kmalloc(sizeof(*dmabuf_obj), GFP_KERNEL);
> >> +	if (dmabuf_obj == NULL) {
> >> +		kfree(fb_info);
> >> +		i915_gem_object_put(obj);
> >> +		gvt_vgpu_err("alloc dmabuf_obj failed\n");
> >> +		return -ENOMEM;
> >> +	}
> >> +	dmabuf_obj->obj = obj;
> >> +	INIT_LIST_HEAD(&dmabuf_obj->list);
> >> +	list_add_tail(&dmabuf_obj->list, &vgpu->dmabuf_obj_list_head);
> >> +
> >>  	gvt_dmabuf->fd = ret;
> >>
> >>  	return 0;
> >> diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.h
> >> b/drivers/gpu/drm/i915/gvt/dmabuf.h
> >> index 8be9979..cafa781 100644
> >> --- a/drivers/gpu/drm/i915/gvt/dmabuf.h
> >> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.h
> >> @@ -31,6 +31,11 @@ struct intel_vgpu_fb_info {
> >>  	uint32_t fb_size;
> >>  };
> >>
> >> +struct intel_vgpu_dmabuf_obj {
> >> +	struct drm_i915_gem_object *obj;
> >> +	struct list_head list;
> >> +};
> >> +
> >>  int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void *args);  int
> >> intel_vgpu_create_dmabuf(struct intel_vgpu *vgpu, void *args);
> >>
> >> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c
> >> b/drivers/gpu/drm/i915/gvt/gvt.c index 2032917..dbc3f86 100644
> >> --- a/drivers/gpu/drm/i915/gvt/gvt.c
> >> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> >> @@ -54,6 +54,8 @@ static const struct intel_gvt_ops intel_gvt_ops = {
> >>  	.vgpu_reset = intel_gvt_reset_vgpu,
> >>  	.vgpu_activate = intel_gvt_activate_vgpu,
> >>  	.vgpu_deactivate = intel_gvt_deactivate_vgpu,
> >> +	.vgpu_query_plane = intel_vgpu_query_plane,
> >> +	.vgpu_create_dmabuf = intel_vgpu_create_dmabuf,
> >>  };
> >>
> >>  /**
> >> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h
> >> b/drivers/gpu/drm/i915/gvt/gvt.h index 763a8c5..a855797 100644
> >> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> >> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> >> @@ -185,8 +185,11 @@ struct intel_vgpu {
> >>  		struct kvm *kvm;
> >>  		struct work_struct release_work;
> >>  		atomic_t released;
> >> +		struct vfio_device *vfio_device;
> >>  	} vdev;
> >>  #endif
> >> +	int dmabuf_mgr_fd;
> >> +	struct list_head dmabuf_obj_list_head;
> >>  };
> >>
> >>  struct intel_gvt_gm {
> >> @@ -467,6 +470,8 @@ struct intel_gvt_ops {
> >>  	void (*vgpu_reset)(struct intel_vgpu *);
> >>  	void (*vgpu_activate)(struct intel_vgpu *);
> >>  	void (*vgpu_deactivate)(struct intel_vgpu *);
> >> +	int (*vgpu_query_plane)(struct intel_vgpu *vgpu, void *);
> >> +	int (*vgpu_create_dmabuf)(struct intel_vgpu *vgpu, void *);
> >>  };
> >>
> >>
> >> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> >> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> >> index 389f072..a079080 100644
> >> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> >> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> >> @@ -41,6 +41,7 @@
> >>  #include <linux/kvm_host.h>
> >>  #include <linux/vfio.h>
> >>  #include <linux/mdev.h>
> >> +#include <linux/anon_inodes.h>
> >>
> >>  #include "i915_drv.h"
> >>  #include "gvt.h"
> >> @@ -524,6 +525,125 @@ static int intel_vgpu_reg_init_opregion(struct  
> >intel_vgpu *vgpu)  
> >>  	return ret;
> >>  }
> >>
> >> +static int kvmgt_get_vfio_device(struct intel_vgpu *vgpu) {
> >> +	struct vfio_device *device;
> >> +
> >> +	device = vfio_device_get_from_dev(mdev_dev(vgpu->vdev.mdev));
> >> +	if (device == NULL)
> >> +		return -ENODEV;
> >> +	vgpu->vdev.vfio_device = device;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void kvmgt_put_vfio_device(struct intel_vgpu *vgpu) {
> >> +	vfio_device_put(vgpu->vdev.vfio_device);
> >> +}
> >> +
> >> +static int intel_vgpu_dmabuf_mgr_fd_mmap(struct file *file,
> >> +		struct vm_area_struct *vma)
> >> +{
> >> +	return -EPERM;
> >> +}
> >> +
> >> +static int intel_vgpu_dmabuf_mgr_fd_release(struct inode *inode,
> >> +		struct file *filp)
> >> +{
> >> +	struct intel_vgpu *vgpu = filp->private_data;
> >> +	struct intel_vgpu_dmabuf_obj *obj;
> >> +	struct list_head *pos;
> >> +
> >> +	if (WARN_ON(!vgpu->vdev.vfio_device))
> >> +		return -ENODEV;
> >> +
> >> +	list_for_each(pos, &vgpu->dmabuf_obj_list_head) {
> >> +		obj = container_of(pos, struct intel_vgpu_dmabuf_obj, list);
> >> +		if (WARN_ON(!obj))
> >> +			return -ENODEV;
> >> +		kfree(obj->obj->gvt_info);
> >> +		i915_gem_object_put(obj->obj);
> >> +		kfree(obj);
> >> +		kvmgt_put_vfio_device(vgpu);  
> >
> >Can we do this?  If I understand, we're releasing all the references and allocations
> >for the dmabuf fds on release of the manager fd.  What happens if the user
> >continues trying to access those dmabuf fds after this?  
> I think we can do this here.
> The dma-buf's release function dma_buf_release() will be called by kernel which means all the created dmabufs will be invalid even we do not release all the references and allocations here.

Are you assuming that the user has closed the dmabuf fds?  They could
close the manager fd first, should the dmabuf fd continue to work?
  
> >> +	}
> >> +	kvmgt_put_vfio_device(vgpu);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static long intel_vgpu_dmabuf_mgr_fd_ioctl(struct file *filp,
> >> +		unsigned int ioctl, unsigned long arg) {
> >> +	struct intel_vgpu *vgpu = filp->private_data;
> >> +	int minsz;
> >> +	int ret;
> >> +	struct fd f;
> >> +
> >> +	f = fdget(vgpu->dmabuf_mgr_fd);
> >> +	if (!f.file)
> >> +		return -EBADF;
> >> +
> >> +	if (ioctl == VFIO_DEVICE_QUERY_PLANE) {
> >> +		struct vfio_vgpu_plane_info info;
> >> +
> >> +		minsz = offsetofend(struct vfio_vgpu_plane_info, resv);
> >> +		if (copy_from_user(&info, (void __user *)arg, minsz)) {
> >> +			fdput(f);
> >> +			return -EFAULT;
> >> +		}
> >> +		if (info.argsz < minsz) {
> >> +			fdput(f);
> >> +			return -EINVAL;
> >> +		}
> >> +		ret = intel_gvt_ops->vgpu_query_plane(vgpu, &info);
> >> +		if (ret != 0) {
> >> +			fdput(f);
> >> +			gvt_vgpu_err("query plane failed:%d\n", ret);
> >> +			return -EINVAL;
> >> +		}
> >> +		fdput(f);
> >> +		return copy_to_user((void __user *)arg, &info, minsz) ?
> >> +								-EFAULT : 0;
> >> +	} else if (ioctl == VFIO_DEVICE_CREATE_DMABUF) {
> >> +		struct vfio_vgpu_dmabuf_info dmabuf;
> >> +
> >> +		minsz = offsetofend(struct vfio_vgpu_dmabuf_info, resv);
> >> +		if (copy_from_user(&dmabuf, (void __user *)arg, minsz)) {
> >> +			fdput(f);
> >> +			return -EFAULT;
> >> +		}
> >> +		if (dmabuf.argsz < minsz) {
> >> +			fdput(f);
> >> +			return -EINVAL;
> >> +		}
> >> +		ret = kvmgt_get_vfio_device(vgpu);
> >> +		if (ret != 0)
> >> +			return ret;  
> >
> >Missed an fdput, though I'm not sure I understand the value of the original fdget
> >or the dmabuf_mgr_fd field at all.  dmabuf_mgr_fd is only used here, presumably
> >to add a reference to the fd while we're in the ioctl, but we're in the ioctl function
> >of that fd, so I think there are already references elsewhere.  
> Make sense. Fdget/fdput can be removed.
> 
> >  
> >> +
> >> +		ret = intel_gvt_ops->vgpu_create_dmabuf(vgpu, &dmabuf);
> >> +		if (ret != 0) {
> >> +			kvmgt_put_vfio_device(vgpu);
> >> +			fdput(f);
> >> +			return -EINVAL;  
> >
> >Why not return the errno that vgpu_create_dmabuf provided?  
> Will change to use the returned errno.
> 
> >  
> >> +		}
> >> +		fdput(f);
> >> +		return copy_to_user((void __user *)arg, &dmabuf, minsz) ?
> >> +								-EFAULT : 0;
> >> +	}
> >> +
> >> +	fdput(f);
> >> +	gvt_vgpu_err("unsupported dmabuf operation\n");
> >> +
> >> +	return -EINVAL;
> >> +}
> >> +
> >> +static const struct file_operations intel_vgpu_dmabuf_mgr_fd_ops = {
> >> +	.release        = intel_vgpu_dmabuf_mgr_fd_release,
> >> +	.unlocked_ioctl = intel_vgpu_dmabuf_mgr_fd_ioctl,
> >> +	.mmap           = intel_vgpu_dmabuf_mgr_fd_mmap,
> >> +	.llseek         = noop_llseek,
> >> +};
> >>  static int intel_vgpu_create(struct kobject *kobj, struct mdev_device
> >> *mdev)  {
> >>  	struct intel_vgpu *vgpu = NULL;
> >> @@ -1259,6 +1379,30 @@ static long intel_vgpu_ioctl(struct mdev_device  
> >*mdev, unsigned int cmd,  
> >>  	} else if (cmd == VFIO_DEVICE_RESET) {
> >>  		intel_gvt_ops->vgpu_reset(vgpu);
> >>  		return 0;
> >> +	} else if (cmd == VFIO_DEVICE_GET_FD) {
> >> +		int fd;
> >> +		u32 type;
> >> +		int ret;
> >> +
> >> +		if (copy_from_user(&type, (void __user *)arg, sizeof(type)))
> >> +			return -EINVAL;
> >> +		if (type != VFIO_DEVICE_DMABUF_MGR_FD)
> >> +			return -EINVAL;
> >> +
> >> +		ret = kvmgt_get_vfio_device(vgpu);
> >> +		if (ret != 0)
> >> +			return ret;
> >> +
> >> +		fd = anon_inode_getfd("intel-vgpu-dmabuf-mgr-fd",
> >> +			&intel_vgpu_dmabuf_mgr_fd_ops,
> >> +			vgpu, O_RDWR | O_CLOEXEC);
> >> +		if (fd < 0) {
> >> +			gvt_vgpu_err("create dmabuf mgr fd failed\n");
> >> +			return -EINVAL;  
> >
> >Error path leaks vfio_device reference.  
> Will correct in the next version.
> 
> >  
> >> +		}
> >> +		vgpu->dmabuf_mgr_fd = fd;  
> >
> >As above, unclear value of this field, additionally, what if the user calls
> >VFIO_DEVICE_GET_FD more than once?  
> Ah, good question.
> VFIO_DEVICE_GET_FD should only be called once.
> And we should add a check if the vgpu->dmabuf_mgr_fd is not 0 which means VFIO_DEVICE_GET_FD had been called before we should return an error.

Except we no longer need that fd and we should probably use an atomic
'opened' so it's not racy.  Thanks,

Alex

> >> +
> >> +		return fd;
> >>  	}
> >>
> >>  	return 0;
> >> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c
> >> b/drivers/gpu/drm/i915/gvt/vgpu.c index 6e3cbd8..af6fc74 100644
> >> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> >> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> >> @@ -346,6 +346,7 @@ static struct intel_vgpu  
> >*__intel_gvt_create_vgpu(struct intel_gvt *gvt,  
> >>  	vgpu->gvt = gvt;
> >>  	vgpu->sched_ctl.weight = param->weight;
> >>  	bitmap_zero(vgpu->tlb_handle_pending, I915_NUM_ENGINES);
> >> +	INIT_LIST_HEAD(&vgpu->dmabuf_obj_list_head);
> >>
> >>  	intel_vgpu_init_cfg_space(vgpu, param->primary);
> >>  
>