linux-kernel - Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 5 May 2017 09:11:15 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Gerd Hoffmann <kraxel@...hat.com>
Cc:     "Chen, Xiaoguang" <xiaoguang.chen@...el.com>,
        "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>,
        "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>
Subject: Re: [RFC PATCH 6/6] drm/i915/gvt: support QEMU getting the dmabuf

On Fri, 05 May 2017 08:55:31 +0200
Gerd Hoffmann <kraxel@...hat.com> wrote:

>   Hi,
> 
> > > >>Hmm, that looks like a rather strange way to return a file descriptor.
> > > >>
> > > >>What is the reason to not use ioctls on the vfio file handle, like
> > > >>older version of these patches did?    
> > > >If I understood correctly that Alex prefer not to change the ioctls on the vfio file
> > > >handle like the old version.
> > > >So I used this way the smallest change to general vfio framework only adding a
> > > >subregion definition.  
> > 
> > I think I was hoping we could avoid a separate file descriptor
> > altogether and use a vfio region instead.  
> 
> What exactly did you have in mind?  Put the framebuffer information
> (struct intel_vgpu_dmabuf) into the vfio region, then access it using
> read/write/mmap?

Yeah, that was my hope.  Adding a new file descriptor means we have one
more reference floating around complicating the life cycle of the
device, group, and container.  Furthermore this one is really only
visible to the mdev vendor driver, so we can't rely on vfio-core, the
vendor driver will need to consider the reference when releasing the
device.

> > However, it was explained
> > previously why this really needs to be a separate fd and I agree that
> > using a region to expose an fd is really awkward.  
> 
> Now with this patchset we have *two* kinds of separate file handles.
> First the anon-fd created by reading from the region.  This is then used
> to run the intel ioctls on, which in turn create the other kind of file
> handle (dma-buf-fd).
> 
> The dma-buf-fd really needs to be a separate fd, because it gets passed
> around as handle and because this is the way dma-bufs work (guess this
> is the discussion you are referring to).

Yep, we're going to need to trust the vendor driver to manage it, we
have lots of places where we need to trust the vendor driver for an
mdev device, unfortunately.
 
> I can't see a compelling reason for the anon-fd though.  I suspect this
> was done due to a misunderstanding ...

Yeah, vfio-core passes device ioctls to the vendor driver, so the
vendor driver should be able to implement a
VFIO_DEVICE_GVT_GET_DMABUF_FD ioctl direclty.  Ideally maybe this
isn't even GVT specific, and we'd s/GVT_//.  Thanks,

Alex