| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170613210610.2ae532ec@t450s.home>
Date: Tue, 13 Jun 2017 21:06:10 -0600
From: Alex Williamson <alex.williamson@...hat.com>
To: "Chen, Xiaoguang" <xiaoguang.chen@...el.com>
Cc: "kraxel@...hat.com" <kraxel@...hat.com>,
"chris@...is-wilson.co.uk" <chris@...is-wilson.co.uk>,
"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>,
"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>,
"Tian, Kevin" <kevin.tian@...el.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? Does the
structure not being a multiple of 8 bytes affect any of the offsets
within 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 -------- */
> >>
> >> /**
>
Powered by blists - more mailing lists