[PATCH 1/6] dma-buf: add peer2peer flag

[Daniel Vetter]

On Wed, Apr 01, 2020 at 04:04:14PM +0000, Ruhl, Michael J wrote:
> >-----Original Message-----
> >From: dri-devel <dri-devel-bounces at lists.freedesktop.org> On Behalf Of
> >Daniel Vetter
> >Sent: Wednesday, April 1, 2020 7:35 AM
> >To: Christian König <ckoenig.leichtzumerken at gmail.com>
> >Cc: amd-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
> >Subject: Re: [PATCH 1/6] dma-buf: add peer2peer flag
> >
> >On Mon, Mar 30, 2020 at 03:55:31PM +0200, Christian König wrote:
> >> Add a peer2peer flag noting that the importer can deal with device
> >> resources which are not backed by pages.
> >>
> >> Signed-off-by: Christian König <christian.koenig at amd.com>
> >> ---
> >>  drivers/dma-buf/dma-buf.c |  2 ++
> >>  include/linux/dma-buf.h   | 10 ++++++++++
> >>  2 files changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> >> index ccc9eda1bc28..570c923023e6 100644
> >> --- a/drivers/dma-buf/dma-buf.c
> >> +++ b/drivers/dma-buf/dma-buf.c
> >> @@ -690,6 +690,8 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf,
> >struct device *dev,
> >>
> >>  	attach->dev = dev;
> >>  	attach->dmabuf = dmabuf;
> >> +	if (importer_ops)
> >> +		attach->peer2peer = importer_ops->allow_peer2peer;
> >
> >So an idea that crossed my mind to validate this, since we need quite some
> >bad amounts of bad luck if someone accidentally introduces and access to
> >struct_page in sg lists in some slowpath.
> >
> >On map_sg, if ->peer2peer is set, we could mangle the struct_page
> >pointers, e.g. swap high bits for low bits (so that NULL stays NULL). On
> >unmap_sg we obviously need to undo that, in case the exporter needs those
> >pointers for its own book-keeping for some reason. I was also pondering
> >just setting them all to NULL, but that might break some exporters. With
> >the pointer mangling trick (especially if we flip high for low bits on 64
> >where this should result in invalid addresses in almost all cases) we
> >should be able to catch buggy p2p importers quite quickly.
> 
> The scatter list usage of the struct page pointer has other information in the
> lower bits for keeping track of linking and other stuff.  Swizzling the page
> pointers will probably make the scatter list unusable.

We'd need to swizzle only the pointers that are actual struct page
pointers. Plus keep the low bits as-is, and maybe only flip the top-most
60 bits or so. Doesn't break the idea fundamentally I think.
-Daniel

> 
> Mike
> 
> >Thoughts? Maybe add as a follow-up patch for testing?
> >-Daniel
> >>  	attach->importer_ops = importer_ops;
> >>  	attach->importer_priv = importer_priv;
> >>
> >> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> >> index 1ade486fc2bb..82e0a4a64601 100644
> >> --- a/include/linux/dma-buf.h
> >> +++ b/include/linux/dma-buf.h
> >> @@ -334,6 +334,14 @@ struct dma_buf {
> >>   * Attachment operations implemented by the importer.
> >>   */
> >>  struct dma_buf_attach_ops {
> >> +	/**
> >> +	 * @allow_peer2peer:
> >> +	 *
> >> +	 * If this is set to true the importer must be able to handle peer
> >> +	 * resources without struct pages.
> >> +	 */
> >> +	bool allow_peer2peer;
> >> +
> >>  	/**
> >>  	 * @move_notify
> >>  	 *
> >> @@ -362,6 +370,7 @@ struct dma_buf_attach_ops {
> >>   * @node: list of dma_buf_attachment, protected by dma_resv lock of the
> >dmabuf.
> >>   * @sgt: cached mapping.
> >>   * @dir: direction of cached mapping.
> >> + * @peer2peer: true if the importer can handle peer resources without
> >pages.
> >>   * @priv: exporter specific attachment data.
> >>   * @importer_ops: importer operations for this attachment, if provided
> >>   * dma_buf_map/unmap_attachment() must be called with the dma_resv
> >lock held.
> >> @@ -382,6 +391,7 @@ struct dma_buf_attachment {
> >>  	struct list_head node;
> >>  	struct sg_table *sgt;
> >>  	enum dma_data_direction dir;
> >> +	bool peer2peer;
> >>  	const struct dma_buf_attach_ops *importer_ops;
> >>  	void *importer_priv;
> >>  	void *priv;
> >> --
> >> 2.17.1
> >>
> >
> >--
> >Daniel Vetter
> >Software Engineer, Intel Corporation
> >http://blog.ffwll.ch
> >_______________________________________________
> >dri-devel mailing list
> >dri-devel at lists.freedesktop.org
> >https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch