Re: [PATCH v2 1/2] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

From: Stefano Stabellini <sstabellini@kernel.org>
To: Andrii Anisov <Andrii_Anisov@epam.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
	Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>,
	"konrad.wilk@oracle.com" <konrad.wilk@oracle.com>,
	Andrii Anisov <andrii.anisov@gmail.com>,
	"julien.grall@arm.com" <julien.grall@arm.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 1/2] swiotlb-xen: implement xen_swiotlb_dma_mmap callback
Date: Wed, 18 Jan 2017 12:23:14 -0800 (PST)	[thread overview]
Message-ID: <alpine.DEB.2.10.1701181124270.10192@sstabellini-ThinkPad-X260> (raw)
In-Reply-To: <VI1PR03MB163135D5B4A49330595FC4D6E67F0@VI1PR03MB1631.eurprd03.prod.outlook.com>

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1956 bytes --]

On Wed, 18 Jan 2017, Andrii Anisov wrote:
> Dear Stefano,
> 
> 
> > Only one suggestion more. For this to work correctly, we are assuming
> > that no foreging pages are involved here, which is a very reasonable
> > assumption given that mmap should be called on memory returned by
> > dma_alloc_coherent.
> 
> I also kept in mind this problem, that's why the first version was RFC.
> 
> > Please add an in-code comment here so that we'll remember.
> 
> Do you think comment would be enough so far?

A comment is enough in the case of xen_swiotlb_dma_mmap, because we are
sure that the function can only be called with local pages. See the
comment above dma_mmap_attrs:

 * Map a coherent DMA buffer previously allocated by dma_alloc_attrs
 * into user space.  The coherent DMA buffer must not be freed by the
 * driver until the user space mapping has been released.

If the page must comes from dma_alloc_coherent, then we are safe.

I wasn't sure about dma_get_sgtable_attrs, because there is no in-tree
description, but looking at git log:

  commit d2b7428eb0caa7c66e34b6ac869a43915b294123
  Author: Marek Szyprowski <m.szyprowski@samsung.com>
  Date:   Wed Jun 13 10:05:52 2012 +0200

      common: dma-mapping: introduce dma_get_sgtable() function

      This patch adds dma_get_sgtable() function which is required to let
      drivers to share the buffers allocated by DMA-mapping subsystem. Right

It looks like dma_get_sgtable is also supposed to be called on buffers
returned by dma_alloc_coherent. We should be safe in both cases.

> Maybe fallback to common ops would be better in order to keep current (even broken) functionality for now? Or
> BUG_ON as you suggested for get_sgtable callback?

BUG_ON is good because it is an obvious failure for a case we don't know
how to handle. If it actually works as expected, we could add it to
both functions anyway, surrounded by #ifdef DEBUG_DRIVER not to slow
down the common case.

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel