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

[Chen, Xiaoguang]

>-----Original Message-----
>From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
>Sent: Wednesday, June 14, 2017 11:46 AM
>To: Chen, Xiaoguang <xiaoguang.chen@xxxxxxxxx>
>Cc: Tian, Kevin <kevin.tian@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; zhenyuw@xxxxxxxxxxxxxxx; chris@xxxxxxxxxxxxxxxxxx; Lv,
>Zhiyuan <zhiyuan.lv@xxxxxxxxx>; intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx; Wang, Zhi
>A <zhi.a.wang@xxxxxxxxx>; kraxel@xxxxxxxxxx
>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" <xiaoguang.chen@xxxxxxxxx> wrote:
>
>> >-----Original Message-----
>> >From: intel-gvt-dev
>> >[mailto:intel-gvt-dev-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of
>> >Alex Williamson
>> >Sent: Wednesday, June 14, 2017 11:06 AM
>> >To: Chen, Xiaoguang <xiaoguang.chen@xxxxxxxxx>
>> >Cc: Tian, Kevin <kevin.tian@xxxxxxxxx>;
>> >intel-gfx@xxxxxxxxxxxxxxxxxxxxx; linux- kernel@xxxxxxxxxxxxxxx;
>> >zhenyuw@xxxxxxxxxxxxxxx; chris@xxxxxxxxxxxxxxxxxx; Lv, Zhiyuan
>> ><zhiyuan.lv@xxxxxxxxx>; intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx; Wang,
>> >Zhi A <zhi.a.wang@xxxxxxxxx>; kraxel@xxxxxxxxxx
>> >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@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?
>> 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@xxxxxxxxxxxxxxxxxxxxx
>> >https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev