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

[Alex Williamson]

On Wed, 14 Jun 2017 02:53:24 +0000
"Chen, Xiaoguang" <xiaoguang.chen@xxxxxxxxx> wrote:

> >-----Original Message-----
> >From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
> >Sent: Wednesday, June 14, 2017 5:25 AM
> >To: Chen, Xiaoguang <xiaoguang.chen@xxxxxxxxx>
> >Cc: kraxel@xxxxxxxxxx; chris@xxxxxxxxxxxxxxxxxx; 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 v8 4/6] vfio: Define vfio based vgpu's dma-buf operations
> >
> >On Fri,  9 Jun 2017 14:50:40 +0800
> >Xiaoguang Chen <xiaoguang.chen@xxxxxxxxx> 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.chen@xxxxxxxxx>
> >> ---
> >>  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 -------- */
> >>
> >>  /**  
>