RE: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

[Chen, Xiaoguang]

>-Original Message-
>From: Alex Williamson [mailto:alex.william...@redhat.com]
>Sent: Wednesday, June 14, 2017 11:46 AM
>To: Chen, Xiaoguang <xiaoguang.c...@intel.com>
>Cc: Tian, Kevin <kevin.t...@intel.com>; intel-...@lists.freedesktop.org; linux-
>ker...@vger.kernel.org; zhen...@linux.intel.com; ch...@chris-wilson.co.uk; Lv,
>Zhiyuan <zhiyuan...@intel.com>; intel-gvt-...@lists.freedesktop.org; Wang, Zhi
>A <zhi.a.w...@intel.com>; kra...@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 +
>"Chen, Xiaoguang" <xiaoguang.c...@intel.com> wrote:
>
>> >-Original Message-
>> >From: intel-gvt-dev
>> >[mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On Behalf Of
>> >Alex Williamson
>> >Sent: Wednesday, June 14, 2017 11:06 AM
>> >To: Chen, Xiaoguang <xiaoguang.c...@intel.com>
>> >Cc: Tian, Kevin <kevin.t...@intel.com>;
>> >intel-...@lists.freedesktop.org; linux- ker...@vger.kernel.org;
>> >zhen...@linux.intel.com; ch...@chris-wilson.co.uk; Lv, Zhiyuan
>> ><zhiyuan...@intel.com>; intel-gvt-...@lists.freedesktop.org; Wang,
>> >Zhi A <zhi.a.w...@intel.com>; kra...@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 +
>> >"Chen, Xiaoguang" <xiaoguang.c...@intel.com> wrote:
>> >
>> >> >-Original Message-
>> >> >From: Alex Williamson [mailto:alex.william...@redhat.com]
>> >> >Sent: Wednesday, June 14, 2017 5:25 AM
>> >> >To: Chen, Xiaoguang <xiaoguang.c...@intel.com>
>> >> >Cc: kra...@redhat.com; ch...@chris-wilson.co.uk; intel-
>> >> >g...@lists.freedesktop.org; linux-kernel@vger.kernel.org;
>> >> >zhen...@linux.intel.com; Lv, Zhiyuan <zhiyuan...@intel.com>;
>> >> >intel-gvt- d...@lists.freedesktop.org; Wang, Zhi A
>> >> ><zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>
>> >> >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
>> >> ><xiaoguang.c...@intel.com> 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 <xiaoguang.c...@intel.com>
>> >> >> ---
>> >> >>  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 s

RE: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

[Chen, Xiaoguang]

>-Original Message-
>From: Alex Williamson [mailto:alex.william...@redhat.com]
>Sent: Wednesday, June 14, 2017 11:46 AM
>To: Chen, Xiaoguang 
>Cc: Tian, Kevin ; intel-...@lists.freedesktop.org; linux-
>ker...@vger.kernel.org; zhen...@linux.intel.com; ch...@chris-wilson.co.uk; Lv,
>Zhiyuan ; intel-gvt-...@lists.freedesktop.org; Wang, Zhi
>A ; kra...@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 +
>"Chen, Xiaoguang"  wrote:
>
>> >-Original Message-
>> >From: intel-gvt-dev
>> >[mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On Behalf Of
>> >Alex Williamson
>> >Sent: Wednesday, June 14, 2017 11:06 AM
>> >To: Chen, Xiaoguang 
>> >Cc: Tian, Kevin ;
>> >intel-...@lists.freedesktop.org; linux- ker...@vger.kernel.org;
>> >zhen...@linux.intel.com; ch...@chris-wilson.co.uk; Lv, Zhiyuan
>> >; intel-gvt-...@lists.freedesktop.org; Wang,
>> >Zhi A ; kra...@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 +
>> >"Chen, Xiaoguang"  wrote:
>> >
>> >> >-Original Message-
>> >> >From: Alex Williamson [mailto:alex.william...@redhat.com]
>> >> >Sent: Wednesday, June 14, 2017 5:25 AM
>> >> >To: Chen, Xiaoguang 
>> >> >Cc: kra...@redhat.com; ch...@chris-wilson.co.uk; intel-
>> >> >g...@lists.freedesktop.org; linux-kernel@vger.kernel.org;
>> >> >zhen...@linux.intel.com; Lv, Zhiyuan ;
>> >> >intel-gvt- d...@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_D

Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

[Alex Williamson]

On Wed, 14 Jun 2017 03:18:31 +
"Chen, Xiaoguang" <xiaoguang.c...@intel.com> wrote:

> >-Original Message-
> >From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> >Behalf Of Alex Williamson
> >Sent: Wednesday, June 14, 2017 11:06 AM
> >To: Chen, Xiaoguang <xiaoguang.c...@intel.com>
> >Cc: Tian, Kevin <kevin.t...@intel.com>; intel-...@lists.freedesktop.org; 
> >linux-
> >ker...@vger.kernel.org; zhen...@linux.intel.com; ch...@chris-wilson.co.uk; 
> >Lv,
> >Zhiyuan <zhiyuan...@intel.com>; intel-gvt-...@lists.freedesktop.org; Wang, 
> >Zhi
> >A <zhi.a.w...@intel.com>; kra...@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 +
> >"Chen, Xiaoguang" <xiaoguang.c...@intel.com> wrote:
> >  
> >> >-Original Message-
> >> >From: Alex Williamson [mailto:alex.william...@redhat.com]
> >> >Sent: Wednesday, June 14, 2017 5:25 AM
> >> >To: Chen, Xiaoguang <xiaoguang.c...@intel.com>
> >> >Cc: kra...@redhat.com; ch...@chris-wilson.co.uk; intel-
> >> >g...@lists.freedesktop.org; linux-kernel@vger.kernel.org;
> >> >zhen...@linux.intel.com; Lv, Zhiyuan <zhiyuan...@intel.com>;
> >> >intel-gvt- d...@lists.freedesktop.org; Wang, Zhi A
> >> ><zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>
> >> >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 <xiaoguang.c...@intel.com> 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 <xiaoguang.c...@intel.com>
> >> >> ---
> >> >>  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
> >> >> + */
> >> >&g

Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

[Alex Williamson]

On Wed, 14 Jun 2017 03:18:31 +
"Chen, Xiaoguang"  wrote:

> >-Original Message-
> >From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
> >Behalf Of Alex Williamson
> >Sent: Wednesday, June 14, 2017 11:06 AM
> >To: Chen, Xiaoguang 
> >Cc: Tian, Kevin ; intel-...@lists.freedesktop.org; 
> >linux-
> >ker...@vger.kernel.org; zhen...@linux.intel.com; ch...@chris-wilson.co.uk; 
> >Lv,
> >Zhiyuan ; intel-gvt-...@lists.freedesktop.org; Wang, 
> >Zhi
> >A ; kra...@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 +
> >"Chen, Xiaoguang"  wrote:
> >  
> >> >-Original Message-
> >> >From: Alex Williamson [mailto:alex.william...@redhat.com]
> >> >Sent: Wednesday, June 14, 2017 5:25 AM
> >> >To: Chen, Xiaoguang 
> >> >Cc: kra...@redhat.com; ch...@chris-wilson.co.uk; intel-
> >> >g...@lists.freedesktop.org; linux-kernel@vger.kernel.org;
> >> >zhen...@linux.intel.com; Lv, Zhiyuan ;
> >> >intel-gvt- d...@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.  
> &

RE: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

[Chen, Xiaoguang]

>-Original Message-
>From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
>Behalf Of Alex Williamson
>Sent: Wednesday, June 14, 2017 11:06 AM
>To: Chen, Xiaoguang <xiaoguang.c...@intel.com>
>Cc: Tian, Kevin <kevin.t...@intel.com>; intel-...@lists.freedesktop.org; linux-
>ker...@vger.kernel.org; zhen...@linux.intel.com; ch...@chris-wilson.co.uk; Lv,
>Zhiyuan <zhiyuan...@intel.com>; intel-gvt-...@lists.freedesktop.org; Wang, Zhi
>A <zhi.a.w...@intel.com>; kra...@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 +
>"Chen, Xiaoguang" <xiaoguang.c...@intel.com> wrote:
>
>> >-Original Message-
>> >From: Alex Williamson [mailto:alex.william...@redhat.com]
>> >Sent: Wednesday, June 14, 2017 5:25 AM
>> >To: Chen, Xiaoguang <xiaoguang.c...@intel.com>
>> >Cc: kra...@redhat.com; ch...@chris-wilson.co.uk; intel-
>> >g...@lists.freedesktop.org; linux-kernel@vger.kernel.org;
>> >zhen...@linux.intel.com; Lv, Zhiyuan <zhiyuan...@intel.com>;
>> >intel-gvt- d...@lists.freedesktop.org; Wang, Zhi A
>> ><zhi.a.w...@intel.com>; Tian, Kevin <kevin.t...@intel.com>
>> >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 <xiaoguang.c...@intel.com> 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 <xiaoguang.c...@intel.com>
>> >> ---
>> >>  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_FD0 /* 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?

>Does the structure
>not being a multiple

RE: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

[Chen, Xiaoguang]

>-Original Message-
>From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On
>Behalf Of Alex Williamson
>Sent: Wednesday, June 14, 2017 11:06 AM
>To: Chen, Xiaoguang 
>Cc: Tian, Kevin ; intel-...@lists.freedesktop.org; linux-
>ker...@vger.kernel.org; zhen...@linux.intel.com; ch...@chris-wilson.co.uk; Lv,
>Zhiyuan ; intel-gvt-...@lists.freedesktop.org; Wang, Zhi
>A ; kra...@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 +
>"Chen, Xiaoguang"  wrote:
>
>> >-Original Message-
>> >From: Alex Williamson [mailto:alex.william...@redhat.com]
>> >Sent: Wednesday, June 14, 2017 5:25 AM
>> >To: Chen, Xiaoguang 
>> >Cc: kra...@redhat.com; ch...@chris-wilson.co.uk; intel-
>> >g...@lists.freedesktop.org; linux-kernel@vger.kernel.org;
>> >zhen...@linux.intel.com; Lv, Zhiyuan ;
>> >intel-gvt- d...@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_FD0 /* 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?

>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 -

Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

[Alex Williamson]

On Wed, 14 Jun 2017 02:53:24 +
"Chen, Xiaoguang" <xiaoguang.c...@intel.com> wrote:

> >-Original Message-
> >From: Alex Williamson [mailto:alex.william...@redhat.com]
> >Sent: Wednesday, June 14, 2017 5:25 AM
> >To: Chen, Xiaoguang <xiaoguang.c...@intel.com>
> >Cc: kra...@redhat.com; ch...@chris-wilson.co.uk; intel-
> >g...@lists.freedesktop.org; linux-kernel@vger.kernel.org;
> >zhen...@linux.intel.com; Lv, Zhiyuan <zhiyuan...@intel.com>; intel-gvt-
> >d...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>; Tian, Kevin
> ><kevin.t...@intel.com>
> >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 <xiaoguang.c...@intel.com> 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 <xiaoguang.c...@intel.com>
> >> ---
> >>  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?  Does the
structure not being a multiple of 8 bytes affect any of the offsets
within 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  */
> >>
> >>  /**  
> 

Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

[Alex Williamson]

On Wed, 14 Jun 2017 02:53:24 +
"Chen, Xiaoguang"  wrote:

> >-Original Message-
> >From: Alex Williamson [mailto:alex.william...@redhat.com]
> >Sent: Wednesday, June 14, 2017 5:25 AM
> >To: Chen, Xiaoguang 
> >Cc: kra...@redhat.com; ch...@chris-wilson.co.uk; intel-
> >g...@lists.freedesktop.org; linux-kernel@vger.kernel.org;
> >zhen...@linux.intel.com; Lv, Zhiyuan ; intel-gvt-
> >d...@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?  Does the
structure not being a multiple of 8 bytes affect any of the offsets
within 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  */
> >>
> >>  /**  
> 

RE: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

[Chen, Xiaoguang]

>-Original Message-
>From: Alex Williamson [mailto:alex.william...@redhat.com]
>Sent: Wednesday, June 14, 2017 5:25 AM
>To: Chen, Xiaoguang <xiaoguang.c...@intel.com>
>Cc: kra...@redhat.com; ch...@chris-wilson.co.uk; intel-
>g...@lists.freedesktop.org; linux-kernel@vger.kernel.org;
>zhen...@linux.intel.com; Lv, Zhiyuan <zhiyuan...@intel.com>; intel-gvt-
>d...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@intel.com>; Tian, Kevin
><kevin.t...@intel.com>
>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 <xiaoguang.c...@intel.com> 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 <xiaoguang.c...@intel.com>
>> ---
>>  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?

>
>> +};
>> +
>> +#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.

>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  */
>>
>>  /**

RE: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

[Chen, Xiaoguang]

>-Original Message-
>From: Alex Williamson [mailto:alex.william...@redhat.com]
>Sent: Wednesday, June 14, 2017 5:25 AM
>To: Chen, Xiaoguang 
>Cc: kra...@redhat.com; ch...@chris-wilson.co.uk; intel-
>g...@lists.freedesktop.org; linux-kernel@vger.kernel.org;
>zhen...@linux.intel.com; Lv, Zhiyuan ; intel-gvt-
>d...@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?

>
>> +};
>> +
>> +#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.

>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  */
>>
>>  /**

Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

[Alex Williamson]

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_FD0 /* Supported fd types */

Move this #define up above vfio_vgpu_plane_info to associate it with the
VFIO_DEVICE_GET_FD ioctl.

> +
> +/*
> + * 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.

> +};
> +
> +#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?  If not, let's at least put them in the same
order so that plane_id is after plane_info for both structs.

> +
> +#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?  Thanks,

Alex

> +
>  /*  API for Type1 VFIO IOMMU  */
>  
>  /**

Re: [PATCH v8 4/6] vfio: Define vfio based vgpu's dma-buf operations

[Alex Williamson]

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_FD0 /* Supported fd types */

Move this #define up above vfio_vgpu_plane_info to associate it with the
VFIO_DEVICE_GET_FD ioctl.

> +
> +/*
> + * 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.

> +};
> +
> +#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?  If not, let's at least put them in the same
order so that plane_id is after plane_info for both structs.

> +
> +#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?  Thanks,

Alex

> +
>  /*  API for Type1 VFIO IOMMU  */
>  
>  /**