From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753207AbdESQek (ORCPT ); Fri, 19 May 2017 12:34:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35606 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750783AbdESQeh (ORCPT ); Fri, 19 May 2017 12:34:37 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 1127417AC47 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=alex.williamson@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 1127417AC47 Date: Fri, 19 May 2017 10:33:52 -0600 From: Alex Williamson To: Xiaoguang Chen Cc: kraxel@redhat.com, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, zhenyuw@linux.intel.com, zhiyuan.lv@intel.com, intel-gvt-dev@lists.freedesktop.org, zhi.a.wang@intel.com, kevin.tian@intel.com Subject: Re: [PATCH v2 5/5] drm/i915/gvt: Adding interface so user space can get the dma-buf Message-ID: <20170519103352.7befec0b@w520.home> In-Reply-To: <1495101005-16045-6-git-send-email-xiaoguang.chen@intel.com> References: <1495101005-16045-1-git-send-email-xiaoguang.chen@intel.com> <1495101005-16045-6-git-send-email-xiaoguang.chen@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Fri, 19 May 2017 16:33:58 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 18 May 2017 17:50:05 +0800 Xiaoguang Chen 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 > --- > 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 > #include > #include > +#include > > #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; > + > + 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? > + 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. > + gvt_vgpu_err("not supported fd type:%d\n", type); Just return error, users can abuse any logging they can get to. > + 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). > + 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. > /* -------- API for Type1 VFIO IOMMU -------- */ > > /**