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

[Alex Williamson <alex.williamson@redhat.com>]

On Fri,  9 Jun 2017 14:50:40 +0800
Xiaoguang Chen <xiaoguang.chen@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.chen@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.

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