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

Wed Jun 14 03:45:38 UTC 2017

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

> >-----Original Message-----
> >From: intel-gvt-dev [mailto:intel-gvt-dev-bounces at lists.freedesktop.org] On
> >Behalf Of Alex Williamson
> >Sent: Wednesday, June 14, 2017 11:06 AM
> >To: Chen, Xiaoguang <xiaoguang.chen at intel.com>
> >Cc: Tian, Kevin <kevin.tian at intel.com>; intel-gfx at lists.freedesktop.org; linux-
> >kernel at vger.kernel.org; zhenyuw at linux.intel.com; chris at chris-wilson.co.uk; Lv,
> >Zhiyuan <zhiyuan.lv at intel.com>; intel-gvt-dev at lists.freedesktop.org; Wang, Zhi
> >A <zhi.a.wang at intel.com>; kraxel at 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" <xiaoguang.chen at intel.com> wrote:
> >  
> >> >-----Original Message-----
> >> >From: Alex Williamson [mailto:alex.williamson at redhat.com]
> >> >Sent: Wednesday, June 14, 2017 5:25 AM
> >> >To: Chen, Xiaoguang <xiaoguang.chen at intel.com>
> >> >Cc: kraxel at redhat.com; chris at chris-wilson.co.uk; intel-
> >> >gfx at lists.freedesktop.org; linux-kernel at vger.kernel.org;
> >> >zhenyuw at linux.intel.com; Lv, Zhiyuan <zhiyuan.lv at intel.com>;
> >> >intel-gvt- dev at lists.freedesktop.org; Wang, Zhi A
> >> ><zhi.a.wang at intel.com>; Tian, Kevin <kevin.tian at 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.chen at 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 at 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?    
> 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,

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 at lists.freedesktop.org
> >https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev  