linux-kernel - Re: [RFC PATCH] xen/gntdev: Stop abusing DT of_dma

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: <alpine.DEB.2.21.1910011112020.20899@sstabellini-ThinkPad-T480s>
Date:   Tue, 1 Oct 2019 11:23:40 -0700 (PDT)
From:   Stefano Stabellini <sstabellini@...nel.org>
To:     Rob Herring <robh@...nel.org>
cc:     Oleksandr Andrushchenko <Oleksandr_Andrushchenko@...m.com>,
        Robin Murphy <robin.murphy@....com>,
        Julien Grall <julien.grall@....com>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Nicolas Saenz Julienne <nsaenzjulienne@...e.de>,
        Juergen Gross <jgross@...e.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
        Oleksandr Andrushchenko <andr2000@...il.com>
Subject: Re: [RFC PATCH] xen/gntdev: Stop abusing DT of_dma_configure API

On Thu, 26 Sep 2019, Rob Herring wrote:
> On Thu, Sep 26, 2019 at 6:16 AM Oleksandr Andrushchenko
> <Oleksandr_Andrushchenko@...m.com> wrote:
> >
> > On 9/26/19 1:46 PM, Robin Murphy wrote:
> > > On 2019-09-26 11:17 am, Oleksandr Andrushchenko wrote:
> > >>
> > >> On 9/26/19 12:49 PM, Julien Grall wrote:
> > >>> Hi Rob,
> > >>>
> > >>>
> > >>> On 9/25/19 10:50 PM, Rob Herring wrote:
> > >>>> As the comment says, this isn't a DT based device. of_dma_configure()
> > >>>> is going to stop allowing a NULL DT node, so this needs to be fixed.
> > >>>
> > >>> And this can't work on arch not selecting CONFIG_OF and can select
> > >>> CONFIG_XEN_GRANT_DMA_ALLOC.
> > >>>
> > >>> We are lucky enough on x86 because, AFAICT, arch_setup_dma_ops is just
> > >>> a nop.
> > >>>
> > >> No luck is needed as [1] does nothing for those platforms not using
> > >> CONFIG_OF
> > >>>>
> > >>>> Not sure exactly what setup besides arch_setup_dma_ops is needed...
> > >>>
> > >>> We probably want to update dma_mask, coherent_dma_mask and
> > >>> dma_pfn_offset.
> > >>>
> > >>> Also, while look at of_configure_dma, I noticed that we consider the
> > >>> DMA will not be coherent for the grant-table. Oleksandr, do you know
> > >>> why they can't be coherent?
> > >> The main and the only reason to use of_configure_dma is that if we don't
> > >> then we
> > >> are about to stay with dma_dummy_ops [2]. It effectively means that
> > >> operations on dma-bufs
> > >> will end up returning errors, like [3], [4], thus not making it possible
> > >> for Xen PV DRM and DMA
> > >> part of gntdev driver to do what we need (dma-bufs in our use-cases
> > >> allow zero-copying
> > >> while using graphics buffers and many more).
> > >>
> > >> I didn't find any better way of achieving that, but of_configure_dma...
> > >> If there is any better solution which will not break the existing
> > >> functionality then
> > >> I will definitely change the drivers so we do not abuse DT )
> > >> Before that, please keep in mind that merging this RFC will break Xen PV
> > >> DRM +
> > >> DMA buf support in gntdev...
> > >> Hope we can work out some acceptable solution, so everyone is happy
> > >
> > > As I mentioned elsewhere, the recent dma-direct rework means that
> > > dma_dummy_ops are now only explicitly installed for the ACPI error
> > > case, so - much as I may dislike it - you should get regular
> > > (direct/SWIOTLB) ops by default again.
> > Ah, my bad, I missed that change. So, if no dummy dma ops are to be used
> > then
> > I believe we can apply both changes, e.g. remove of_dma_configure from
> > both of the drivers.
> 
> What about the dma masks? I think there's a default setup, but it is
> considered a driver bug to not set its mask. xen_drm_front sets the
> coherent_dma_mask (why only 32-bits though?), but not the dma_mask.
> gntdev is doing neither. I could copy out what of_dma_configure does
> but better for the Xen folks to decide what is needed or not and test
> the change. I'm not setup to test any of this.

FYI I have seen the issue Oleksandr is talking about too. I confirm that
the only reason for the of_configure_dma call is to get away from the
dummy_dma_ops and use the default dma_ops instead. I think this should
be mentioned in the commit message so that if one day the behavior
regarding dummy_dma_ops changes one more time, hopefully we'll be able
to figure out the issue more easily with bisection.

In regards to the coherent_dma_mask and dma_mask, I can't see why gntdev
would have any dma addressing limitations, so we should be able to set
both to 64 bits.  I also can't see why xen_drm_front would limit it to
32 bits, after all this is just the frontend, if anything it would be
the backend that has a limitation. So, we should be able to set both
dma_mask and coherent_dma_mask in xen_drm_front to 64 bits. Oleksandr,
can you confirm?