Hi Alex, >-----Original Message----- >From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx] >Sent: Saturday, May 20, 2017 12:34 AM >To: Chen, Xiaoguang <xiaoguang.chen@xxxxxxxxx> >Cc: kraxel@xxxxxxxxxx; 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 v2 5/5] drm/i915/gvt: Adding interface so user space can get >the dma-buf > >On Thu, 18 May 2017 17:50:05 +0800 >Xiaoguang Chen <xiaoguang.chen@xxxxxxxxx> wrote: > >> User space will try to create a management fd for the dma-buf operation. >> Using this management fd user can query the plane information and >> create a dma-buf fd if necessary. >> GVT-g will handle the life cycle of the management fd and will align >> the life cycle of the fd with the vfio device. >> User space should handle the life cycle of the created dma-buf fd >> close the dma-buf fd timely when finishing use. >> >> Signed-off-by: Xiaoguang Chen <xiaoguang.chen@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/gvt/gvt.c | 2 + >> drivers/gpu/drm/i915/gvt/gvt.h | 3 ++ >> drivers/gpu/drm/i915/gvt/kvmgt.c | 89 >++++++++++++++++++++++++++++++++++++++++ >> include/uapi/drm/i915_drm.h | 2 + >> include/uapi/linux/vfio.h | 12 ++++++ >> 5 files changed, 108 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c >> b/drivers/gpu/drm/i915/gvt/gvt.c index 2032917..48e04e6 100644 >> --- a/drivers/gpu/drm/i915/gvt/gvt.c >> +++ b/drivers/gpu/drm/i915/gvt/gvt.c >> @@ -54,6 +54,8 @@ >> .vgpu_reset = intel_gvt_reset_vgpu, >> .vgpu_activate = intel_gvt_activate_vgpu, >> .vgpu_deactivate = intel_gvt_deactivate_vgpu, >> + .vgpu_query_dmabuf = intel_vgpu_query_dmabuf, >> + .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 a553120..b7fdfd5 100644 >> --- a/drivers/gpu/drm/i915/gvt/gvt.h >> +++ b/drivers/gpu/drm/i915/gvt/gvt.h >> @@ -185,6 +185,7 @@ struct intel_vgpu { >> struct kvm *kvm; >> struct work_struct release_work; >> atomic_t released; >> + struct vfio_device *vfio_device; >> } vdev; >> #endif >> struct intel_vgpu_plane_info *plane_info; @@ -469,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_dmabuf)(struct intel_vgpu *, void *); >> + int (*vgpu_create_dmabuf)(struct intel_vgpu *, void *); >> }; >> >> >> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c >> b/drivers/gpu/drm/i915/gvt/kvmgt.c >> index 389f072..9a663df 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,66 @@ static int intel_vgpu_reg_init_opregion(struct >intel_vgpu *vgpu) >> return ret; >> } >> >> +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; >> + >> + if (vgpu->vdev.vfio_device != NULL) >> + vfio_device_put(vgpu->vdev.vfio_device); >> + else >> + gvt_vgpu_err("intel vgpu dmabuf mgr fd is in a wrong state\n"); > >You could do: > >if (WARN_ON(!vgpu->vdev.vfio_device)) > return -EINVAL; OK. > >> + >> + 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; >> + struct intel_vgpu_dmabuf dmabuf; >> + int ret; >> + struct fd f; >> + >> + minsz = offsetofend(struct intel_vgpu_dmabuf, tiled); >> + if (copy_from_user(&dmabuf, (void __user *)arg, minsz)) >> + return -EFAULT; >> + >> + f = fdget(dmabuf.fd); >> + >> + if (ioctl == INTEL_VGPU_QUERY_DMABUF) >> + ret = intel_gvt_ops->vgpu_query_dmabuf(vgpu, &dmabuf); >> + else if (ioctl == INTEL_VGPU_GENERATE_DMABUF) >> + ret = intel_gvt_ops->vgpu_create_dmabuf(vgpu, &dmabuf); > >Why are these still Intel specific? Ah, the v2 patch series were sent before our discussion in other thread. Will change it in v3. > >> + else { >> + fdput(f); >> + gvt_vgpu_err("unsupported dmabuf operation\n"); >> + return -EINVAL; >> + } >> + >> + if (ret != 0) { >> + fdput(f); >> + gvt_vgpu_err("gvt-g get dmabuf failed:%d\n", ret); >> + return -EINVAL; >> + } >> + fdput(f); >> + >> + return copy_to_user((void __user *)arg, &dmabuf, minsz) ? -EFAULT : >> +0; } >> + >> +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 +1320,34 @@ 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; >> + struct vfio_device *device; >> + >> + if (copy_from_user(&type, (void __user *)arg, sizeof(type))) >> + return -EINVAL; >> + if (type != INTEL_VGPU_DMABUF_MGR_FD) { > >Yet more unnecessary Intel specific code. Userspace doesn't want to handle a >dmabuf differently between Intel, AMD, and NVIDIA, so if we have no reason to >need vendor specific ioctl and parameters, then don't. Yes. I will change in v3. > >> + gvt_vgpu_err("not supported fd type:%d\n", type); > >Just return error, users can abuse any logging they can get to. OK. > >> + return -EINVAL; >> + } >> + >> + fd = anon_inode_getfd("intel-vgpu-dmabuf-mgr-fd", >> + &intel_vgpu_dmabuf_mgr_fd_ops, >> + vgpu, O_RDWR | O_CLOEXEC); > >And this ordering is still wrong, do this last so you don't need to worry about >getting the vfio_device reference below failing and calling release unnecessarily >(though it seems this fd is leaked in that case anyway). Will change in v3. > >> + if (fd < 0) { >> + gvt_vgpu_err("create dmabuf mgr fd failed\n"); >> + return -EINVAL; >> + } >> + >> + device = vfio_device_get_from_dev(mdev_dev(mdev)); >> + if (device == NULL) { >> + gvt_vgpu_err("kvmgt: vfio device is null\n"); >> + return -EINVAL; >> + } >> + vgpu->vdev.vfio_device = device; >> + >> + return fd; >> } >> >> return 0; >> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >> index cde4f8e..9d28433 100644 >> --- a/include/uapi/drm/i915_drm.h >> +++ b/include/uapi/drm/i915_drm.h >> @@ -1466,6 +1466,8 @@ struct intel_vgpu_dmabuf { >> __u32 tiled; >> }; >> >> +#define INTEL_VGPU_DMABUF_MGR_FD 0 >> + >> #if defined(__cplusplus) >> } >> #endif >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h >> index ae46105..c81500b 100644 >> --- a/include/uapi/linux/vfio.h >> +++ b/include/uapi/linux/vfio.h >> @@ -502,6 +502,18 @@ struct vfio_pci_hot_reset { >> >> #define VFIO_DEVICE_PCI_HOT_RESET _IO(VFIO_TYPE, VFIO_BASE + 13) >> >> +/** >> + * VFIO_DEVICE_GET_FD - _IO(VFIO_TYPE, VFIO_BASE + 14, __u32) >> + * >> + * Create a fd for a vfio device based on the input type >> + * Vendor driver should handle this ioctl to create a fd and manage >> +the >> + * life cycle of this fd. >> + * >> + * Return: a fd if vendor support that type, -errno if not supported >> +*/ >> + >> +#define VFIO_DEVICE_GET_FD _IO(VFIO_TYPE, VFIO_BASE + 14) >> + > >The known input types would need to be defined here too. Definition of the >ioctls available on that type should also be documented here. Yes. Will do it in v3. Thanks. > >> /* -------- API for Type1 VFIO IOMMU -------- */ >> >> /** _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
