Hi Alex,
>-----Original Message-----
>From: Alex Williamson [mailto:alex.william...@redhat.com]
>Sent: Saturday, May 20, 2017 12:34 AM
>To: Chen, Xiaoguang <xiaoguang.c...@intel.com>
>Cc: kra...@redhat.com; intel-gfx@lists.freedesktop.org; linux-
>ker...@vger.kernel.org; zhen...@linux.intel.com; Lv, Zhiyuan
><zhiyuan...@intel.com>; intel-gvt-...@lists.freedesktop.org; Wang, Zhi A
><zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>
>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.c...@intel.com> 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.c...@intel.com>
>> ---
>> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
