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

[Alex Williamson]

On Thu, 1 Jun 2017 03:01:28 +0000
"Chen, Xiaoguang" <xiaoguang.chen@xxxxxxxxx> wrote:

> Hi Kirti,
> 
> >-----Original Message-----
> >From: Kirti Wankhede [mailto:kwankhede@xxxxxxxxxx]
> >Sent: Thursday, June 01, 2017 1:23 AM
> >To: Chen, Xiaoguang <xiaoguang.chen@xxxxxxxxx>; Gerd Hoffmann
> ><kraxel@xxxxxxxxxx>; alex.williamson@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 v6 4/6] vfio: Define vfio based vgpu's dma-buf operations
> >
> >
> >
> >On 5/31/2017 11:48 AM, Chen, Xiaoguang wrote:  
> >> Hi,
> >>  
> >>> -----Original Message-----
> >>> From: Gerd Hoffmann [mailto:kraxel@xxxxxxxxxx]
> >>> Sent: Monday, May 29, 2017 3:20 PM
> >>> To: Chen, Xiaoguang <xiaoguang.chen@xxxxxxxxx>;
> >>> alex.williamson@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 v6 4/6] vfio: Define vfio based vgpu's dma-buf
> >>> operations
> >>>  
> >>>> +struct vfio_vgpu_dmabuf_info {
> >>>> +	__u32 argsz;
> >>>> +	__u32 flags;
> >>>> +	struct vfio_vgpu_plane_info plane_info;
> >>>> +	__s32 fd;
> >>>> +	__u32 pad;
> >>>> +};  
> >>>
> >>> Hmm, now you have argsz and flags twice in vfio_vgpu_dmabuf_info ...
> >>>
> >>> I think we should have something like this:
> >>>
> >>> 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;
> >>> };
> >>>
> >>> struct vfio_vgpu_query_plane {
> >>> 	__u32 argsz;
> >>> 	__u32 flags;
> >>> 	struct vfio_vgpu_plane_info plane_info;
> >>>        __u32 plane_id;
> >>>        __u32 padding;
> >>> };
> >>>
> >>> struct vfio_vgpu_create_dmabuf {
> >>> 	__u32 argsz;
> >>> 	__u32 flags;
> >>> 	struct vfio_vgpu_plane_info plane_info;
> >>>        __u32 plane_id;
> >>>        __s32 fd;
> >>> };  
> >> Good suggestion will apply in the next version.
> >> Thanks for review :)
> >>  
> >
> >Can you define what are the expected values of 'flags' would be?  
> Flags is not used in this case.  It is defined to follow the rules of vfio ioctls.

An important note about flags, the vendor driver must validate it.  If
they don't and the user passes an arbitrary value there, then we have a
backwards compatibility issue with ever attempting to use the flags
field.  The user passing in a flag unknown to the vendor driver should
return an -EINVAL response.  In this case, we haven't defined any
flags, so the vendor driver needs to force the user to pass zero.
Thanks,

Alex