Date:   Tue, 13 Jun 2017 21:45:38 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     "Chen, Xiaoguang" <xiaoguang.chen@...el.com>
Cc:     "Tian, Kevin" <kevin.tian@...el.com>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "zhenyuw@...ux.intel.com" <zhenyuw@...ux.intel.com>,
        "chris@...is-wilson.co.uk" <chris@...is-wilson.co.uk>,
        "Lv, Zhiyuan" <zhiyuan.lv@...el.com>,
        "intel-gvt-dev@...ts.freedesktop.org" 
        <intel-gvt-dev@...ts.freedesktop.org>,
        "Wang, Zhi A" <zhi.a.wang@...el.com>,
        "kraxel@...hat.com" <kraxel@...hat.com>
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@...el.com> wrote:

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

Powered by blists - more mailing lists