Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754210AbdFNF3k convert rfc822-to-8bit (ORCPT ); Wed, 14 Jun 2017 01:29:40 -0400 Received: from mga07.intel.com ([134.134.136.100]:3553 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750740AbdFNF3i (ORCPT ); Wed, 14 Jun 2017 01:29:38 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,340,1493708400"; d="scan'208";a="97782540" From: "Chen, Xiaoguang" To: Alex Williamson CC: "Tian, Kevin" , "intel-gfx@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "zhenyuw@linux.intel.com" , "chris@chris-wilson.co.uk" , "Lv, Zhiyuan" , "intel-gvt-dev@lists.freedesktop.org" , "Wang, Zhi A" , "kraxel@redhat.com" Subject: RE: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations Thread-Topic: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations Thread-Index: AQHS4O0B+rtVVjzuBUyRMhjd8q2t06Iizw6AgADdRND//4IQAIAAiPlw//+CDgCAAKKrIA== Date: Wed, 14 Jun 2017 05:29:34 +0000 Message-ID: References: <1496991042-2265-1-git-send-email-xiaoguang.chen@intel.com> <1496991042-2265-5-git-send-email-xiaoguang.chen@intel.com> <20170613152459.6b120a7a@w520.home> <20170613210610.2ae532ec@t450s.home> <20170613214538.7fb2c0aa@t450s.home> In-Reply-To: <20170613214538.7fb2c0aa@t450s.home> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7524 Lines: 203 >-----Original Message----- >From: Alex Williamson [mailto:alex.williamson@redhat.com] >Sent: Wednesday, June 14, 2017 11:46 AM >To: Chen, Xiaoguang >Cc: Tian, Kevin ; intel-gfx@lists.freedesktop.org; linux- >kernel@vger.kernel.org; zhenyuw@linux.intel.com; chris@chris-wilson.co.uk; Lv, >Zhiyuan ; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi >A ; kraxel@redhat.com >Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations > >On Wed, 14 Jun 2017 03:18:31 +0000 >"Chen, Xiaoguang" wrote: > >> >-----Original Message----- >> >From: intel-gvt-dev >> >[mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of >> >Alex Williamson >> >Sent: Wednesday, June 14, 2017 11:06 AM >> >To: Chen, Xiaoguang >> >Cc: Tian, Kevin ; >> >intel-gfx@lists.freedesktop.org; linux- kernel@vger.kernel.org; >> >zhenyuw@linux.intel.com; chris@chris-wilson.co.uk; Lv, Zhiyuan >> >; intel-gvt-dev@lists.freedesktop.org; Wang, >> >Zhi A ; kraxel@redhat.com >> >Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf >> >operations >> > >> >On Wed, 14 Jun 2017 02:53:24 +0000 >> >"Chen, Xiaoguang" wrote: >> > >> >> >-----Original Message----- >> >> >From: Alex Williamson [mailto:alex.williamson@redhat.com] >> >> >Sent: Wednesday, June 14, 2017 5:25 AM >> >> >To: Chen, Xiaoguang >> >> >Cc: kraxel@redhat.com; chris@chris-wilson.co.uk; intel- >> >> >gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org; >> >> >zhenyuw@linux.intel.com; Lv, Zhiyuan ; >> >> >intel-gvt- dev@lists.freedesktop.org; Wang, Zhi A >> >> >; Tian, Kevin >> >> >Subject: Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf >> >> >operations >> >> > >> >> >On Fri, 9 Jun 2017 14:50:40 +0800 Xiaoguang Chen >> >> > wrote: >> >> > >> >> >> Here we defined a new ioctl to create a fd for a vfio device >> >> >> based on the input type. Now only one type is supported that is >> >> >> a dma-buf management fd. >> >> >> Two ioctls are defined for the dma-buf management fd: query the >> >> >> vfio vgpu's plane information and create a dma-buf for a plane. >> >> >> >> >> >> Signed-off-by: Xiaoguang Chen >> >> >> --- >> >> >> include/uapi/linux/vfio.h | 58 >> >> >> +++++++++++++++++++++++++++++++++++++++++++++++ >> >> >> 1 file changed, 58 insertions(+) >> >> >> >> >> >> diff --git a/include/uapi/linux/vfio.h >> >> >> b/include/uapi/linux/vfio.h index ae46105..24427b7 100644 >> >> >> --- a/include/uapi/linux/vfio.h >> >> >> +++ b/include/uapi/linux/vfio.h >> >> >> @@ -502,6 +502,64 @@ 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) >> >> >> + >> >> >> +struct vfio_vgpu_plane_info { >> >> >> + __u64 start; >> >> >> + __u64 drm_format_mod; >> >> >> + __u32 drm_format; >> >> >> + __u32 width; >> >> >> + __u32 height; >> >> >> + __u32 stride; >> >> >> + __u32 size; >> >> >> + __u32 x_pos; >> >> >> + __u32 y_pos; >> >> >> + __u32 padding; >> >> >> +}; >> >> >> + >> >> >> +#define VFIO_DEVICE_DMABUF_MGR_FD 0 /* Supported fd types >*/ >> >> > >> >> >Move this #define up above vfio_vgpu_plane_info to associate it >> >> >with the VFIO_DEVICE_GET_FD ioctl. >> >> OK. >> >> >> >> > >> >> >> + >> >> >> +/* >> >> >> + * VFIO_DEVICE_QUERY_PLANE - _IO(VFIO_TYPE, VFIO_BASE + 15, >> >> >> + * struct >vfio_vgpu_query_plane) >> >> >> + * Query plane information >> >> >> + */ >> >> >> +struct vfio_vgpu_query_plane { >> >> >> + __u32 argsz; >> >> >> + __u32 flags; >> >> >> + struct vfio_vgpu_plane_info plane_info; >> >> >> + __u32 plane_id; >> >> >> + __u32 padding; >> >> > >> >> >This padding doesn't make sense. >> >> This padding is still needed if we do not move the plane_id into >> >vfio_vgpu_plane_info. Right? >> > >> >I don't see why this padding is ever needed, can you explain? >> I thought we add the padding to make sure the structure layout is the same in >both 32bit and 64bit systems. >> Am I right? > >Isn't it already the same without any of the padding here? Without the padding in >vfio_vgpu_plane_info it's 4-byte aligned and we're following it with a 4-byte field, >that works the same on 32 and 64bit. Padding the outer structure here makes no >sense to me. Generally padding at the end of the structure is to allow flexibility in >expanding it within that padding without breaking the ioctl. Here we use _IO and >argsz/flags to do that. Thanks, I got your point. Yes this padding is useless. Thanks very much : ) > >Alex > >> >Does the structure >> >not being a multiple of 8 bytes affect any of the offsets within the structure? >> No. it will not affect any offsets in the structure. >> >> > >> >> >> +}; >> >> >> + >> >> >> +#define VFIO_DEVICE_QUERY_PLANE _IO(VFIO_TYPE, VFIO_BASE + 15) >> >> >> + >> >> >> +/* >> >> >> + * VFIO_DEVICE_CREATE_DMABUF - _IO(VFIO, VFIO_BASE + 16, >> >> >> + * struct >> >> >vfio_vgpu_create_dmabuf) >> >> >> + * >> >> >> + * Create a dma-buf for a plane */ struct >> >> >> +vfio_vgpu_create_dmabuf { >> >> >> + __u32 argsz; >> >> >> + __u32 flags; >> >> >> + struct vfio_vgpu_plane_info plane_info; >> >> >> + __s32 fd; >> >> >> + __u32 plane_id; >> >> >> +}; >> >> > >> >> >Both of these have a plane_id, should plane_id simply replace the >> >> >padding in plane_info? >> >> Precisely speaking plane_id does not belong to the plane info. All >> >> the other >> >information are decoded from plane except plane id. >> > >> >Ok, let's keep is separate then. Thanks, >> > >> >Alex >> > >> >> >If not, let's at least put them in the same order so that plane_id >> >> >is after plane_info for both structs. >> >> Ok. >> >> >> >> > >> >> >> + >> >> >> +#define VFIO_DEVICE_CREATE_DMABUF _IO(VFIO_TYPE, VFIO_BASE + >> >> >> +16) >> >> > >> >> >I don't think these should be named just VFIO_DEVICE_foo, that >> >> >implies they're ioctls on the vfio device fd, they're not. They >> >> >need to be associated both in name and more complete descriptions >> >> >as ioctls to the fd returned from a request for a >> >> >VFIO_DEVICE_DMABUF_MGR_FD. Perhaps >> >VFIO_DMABUF_MGR_QUERY_PLANE and >> >> >VFIO_DMABUF_MGR_CREATE_DMABUF. I'm also not sure why we're using >> >"vgpu" in the structure names here either, the ioctls aren't named after vgpus. >> >> >Aren't these rather generic to graphics dmabufs, not specifically vgpus? >> >> Make sense. I will change the names. >> >> >> >> Thanks, >> >> > >> >> >Alex >> >> > >> >> >> + >> >> >> /* -------- API for Type1 VFIO IOMMU -------- */ >> >> >> >> >> >> /** >> >> >> > >> >_______________________________________________ >> >intel-gvt-dev mailing list >> >intel-gvt-dev@lists.freedesktop.org >> >https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev