linux-kernel - Re: [PATCH RFC 1/1] uio: Add dma-buf import ioctls

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]
Message-ID: <20190301001856.GA20971@smtp.xilinx.com>
Date:   Thu, 28 Feb 2019 16:18:57 -0800
From:   Hyun Kwon <hyun.kwon@...inx.com>
To:     Daniel Vetter <daniel@...ll.ch>
CC:     Hyun Kwon <hyunk@...inx.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Stefano Stabellini <stefanos@...inx.com>,
        Sonal Santan <sonals@...inx.com>,
        Cyril Chemparathy <cyrilc@...inx.com>,
        Jiaying Liang <jliang@...inx.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "moderated list:DMA BUFFER SHARING FRAMEWORK" 
        <linaro-mm-sig@...ts.linaro.org>,
        Michal Simek <michals@...inx.com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        "open list:DMA BUFFER SHARING FRAMEWORK" 
        <linux-media@...r.kernel.org>
Subject: Re: [PATCH RFC 1/1] uio: Add dma-buf import ioctls

Hi Daniel,

On Thu, 2019-02-28 at 02:01:46 -0800, Daniel Vetter wrote:
> On Wed, Feb 27, 2019 at 04:36:06PM -0800, Hyun Kwon wrote:
> > Hi Daniel,
> > 
> > On Wed, 2019-02-27 at 06:13:45 -0800, Daniel Vetter wrote:
> > > On Tue, Feb 26, 2019 at 11:20 PM Hyun Kwon <hyun.kwon@...inx.com> wrote:
> > > >
> > > > Hi Daniel,
> > > >
> > > > Thanks for the comment.
> > > >
> > > > On Tue, 2019-02-26 at 04:06:13 -0800, Daniel Vetter wrote:
> > > > > On Tue, Feb 26, 2019 at 12:53 PM Greg Kroah-Hartman
> > > > > <gregkh@...uxfoundation.org> wrote:
> > > > > >
> > > > > > On Sat, Feb 23, 2019 at 12:28:17PM -0800, Hyun Kwon wrote:
> > > > > > > Add the dmabuf map / unmap interfaces. This allows the user driver
> > > > > > > to be able to import the external dmabuf and use it from user space.
> > > > > > >
> > > > > > > Signed-off-by: Hyun Kwon <hyun.kwon@...inx.com>
> > > > > > > ---
> > > > > > >  drivers/uio/Makefile         |   2 +-
> > > > > > >  drivers/uio/uio.c            |  43 +++++++++
> > > > > > >  drivers/uio/uio_dmabuf.c     | 210 +++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  drivers/uio/uio_dmabuf.h     |  26 ++++++
> > > > > > >  include/uapi/linux/uio/uio.h |  33 +++++++
> > > > > > >  5 files changed, 313 insertions(+), 1 deletion(-)
> > > > > > >  create mode 100644 drivers/uio/uio_dmabuf.c
> > > > > > >  create mode 100644 drivers/uio/uio_dmabuf.h
> > > > > > >  create mode 100644 include/uapi/linux/uio/uio.h
> > > > > > >
> > > > > > > diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> > > > > > > index c285dd2..5da16c7 100644
> > > > > > > --- a/drivers/uio/Makefile
> > > > > > > +++ b/drivers/uio/Makefile
> > > > > > > @@ -1,5 +1,5 @@

[snip]

> > > > > Frankly looks like a ploy to sidestep review by graphics folks. We'd
> > > > > ask for the userspace first :-)
> > > >
> > > > Please refer to pull request [1].
> > > >
> > > > For any interest in more details, the libmetal is the abstraction layer
> > > > which provides platform independent APIs. The backend implementation
> > > > can be selected per different platforms: ex, rtos, linux,
> > > > standalone (xilinx),,,. For Linux, it supports UIO / vfio as of now.
> > > > The actual user space drivers sit on top of libmetal. Such drivers can be
> > > > found in [2]. This is why I try to avoid any device specific code in
> > > > Linux kernel.
> > > >
> > > > >
> > > > > Also, exporting dma_addr to userspace is considered a very bad idea.
> > > >
> > > > I agree, hence the RFC to pick some brains. :-) Would it make sense
> > > > if this call doesn't export the physicall address, but instead takes
> > > > only the dmabuf fd and register offsets to be programmed?
> > > >
> > > > > If you want to do this properly, you need a minimal in-kernel memory
> > > > > manager, and those tend to be based on top of drm_gem.c and merged
> > > > > through the gpu tree. The last place where we accidentally leaked a
> > > > > dma addr for gpu buffers was in the fbdev code, and we plugged that
> > > > > one with
> > > >
> > > > Could you please help me understand how having a in-kernel memory manager
> > > > helps? Isn't it just moving same dmabuf import / paddr export functionality
> > > > in different modules: kernel memory manager vs uio. In fact, Xilinx does have
> > > > such memory manager based on drm gem in downstream. But for this time we took
> > > > the approach of implementing this through generic dmabuf allocator, ION, and
> > > > enabling the import capability in the UIO infrastructure instead.
> > > 
> > > There's a group of people working on upstreaming a xilinx drm driver
> > > already. Which driver are we talking about? Can you pls provide a link
> > > to that xilinx drm driver?
> > > 
> > 
> > The one I was pushing [1] is implemented purely for display, and not
> > intended for anything other than that as of now. What I'm refering to above
> > is part of Xilinx FPGA (acceleration) runtime [2]. As far as I know,
> > it's planned to be upstreamed, but not yet started. The Xilinx runtime
> > software has its own in-kernel memory manager based on drm_cma_gem with
> > its own ioctls [3].
> > 
> > Thanks,
> > -hyun
> > 
> > [1] https://patchwork.kernel.org/patch/10513001/
> > [2] https://github.com/Xilinx/XRT
> > [3] https://github.com/Xilinx/XRT/tree/master/src/runtime_src/driver/zynq/drm
> 
> I've done a very quick look only, and yes this is kinda what I'd expect.
> Doing a small drm gem driver for an fpga/accelarator that needs lots of
> memories is the right architecture, since at the low level of kernel
> interfaces a gpu really isn't anything else than an accelarater.
> 
> And from a very cursory look the gem driver you mentioned (I only scrolled
> through the ioctl handler quickly) looks reasonable.

Thanks for taking time to look and share input. But still I'd like to
understand why it's more reasonable if the similar ioctl exists with drm
than with uio. Is it because such drm ioctl is vendor specific?

Thanks,
-hyun

> -Daniel
> > 
> > > Thanks, Daniel
> > > 
> > > > Thanks,
> > > > -hyun
> > > >
> > > > [1] https://github.com/OpenAMP/libmetal/pull/82/commits/951e2762bd487c98919ad12f2aa81773d8fe7859
> > > > [2] https://github.com/Xilinx/embeddedsw/tree/master/XilinxProcessorIPLib/drivers
> > > >
> > > > >
> > > > > commit 4be9bd10e22dfc7fc101c5cf5969ef2d3a042d8a (tag:
> > > > > drm-misc-next-fixes-2018-10-03)
> > > > > Author: Neil Armstrong <narmstrong@...libre.com>
> > > > > Date:   Fri Sep 28 14:05:55 2018 +0200
> > > > >
> > > > >     drm/fb_helper: Allow leaking fbdev smem_start
> > > > >
> > > > > Together with cuse the above patch should be enough to implement a drm
> > > > > driver entirely in userspace at least.
> > > > >
> > > > > Cheers, Daniel
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > > 
> > > 
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch