Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932942AbdC2Ws2 (ORCPT ); Wed, 29 Mar 2017 18:48:28 -0400 Received: from mail-yw0-f195.google.com ([209.85.161.195]:32866 "EHLO mail-yw0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753026AbdC2Ws0 (ORCPT ); Wed, 29 Mar 2017 18:48:26 -0400 MIME-Version: 1.0 In-Reply-To: <1490689242-5097-1-git-send-email-boris.brezillon@free-electrons.com> References: <1490689242-5097-1-git-send-email-boris.brezillon@free-electrons.com> From: Max Filippov Date: Wed, 29 Mar 2017 15:48:24 -0700 Message-ID: Subject: Re: [PATCH] xtensa: Fix mmap() support To: Boris Brezillon Cc: Chris Zankel , "linux-xtensa@linux-xtensa.org" , Maxime Ripard , Thomas Petazzoni , LKML Content-Type: multipart/mixed; boundary=94eb2c19e4bcca3e80054be65f43 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5725 Lines: 127 --94eb2c19e4bcca3e80054be65f43 Content-Type: text/plain; charset=UTF-8 Hi Boris, On Tue, Mar 28, 2017 at 1:20 AM, Boris Brezillon wrote: > The xtensa architecture does not implement the dma_map_ops->mmap() hook, > thus relying on the default dma_common_mmap() implementation. > This implementation is only safe on DMA-coherent architecture (hence the > !defined(CONFIG_ARCH_NO_COHERENT_DMA_MMAP) condition), but xtensa does not > fall in this case. Why not? AFAIU "DMA-coherent" may mean "having DMA memory accessible w/o caching", is that right? We have all low memory mapped in the uncached KSEG region, it may be mapped to the userspace w/o caching as well, that's what dma_common_mmap does. > This lead to bad pfn calculation when someone tries to mmap() one or > several pages that are not part of the identity mapping because the > dma_common_mmap() extract the pfn value from the virt address using > virt_to_page() which is only applicable on DMA-coherent platforms (on > other platforms, DMA coherent pages are mapped in a different region). Oh, yes. Our __pa implementation is buggy in a sense that it doesn't work correctly with addresses from the uncached KSEG. > Implement xtensa_dma_mmap() (loosely based on __arm_dma_mmap()) in which > pfn is extracted from the DMA address using PFN_DOWN(). > > While we're at it, select ARCH_NO_COHERENT_DMA_MMAP from the XTENSA > entry so that we never silently fallback to dma_common_mmap() if someone > decides to drop the xtensa_dma_mmap() implementation. I don't think this is right. > Signed-off-by: Boris Brezillon > --- > Hello, > > This bug has been detected while developping a DRM driver on an FPGA > containing an Xtensa CPU. The DRM driver is using the generic CMA GEM > implementation which is allocating DMA coherent buffers in kernel space > and then allows userspace programs to mmap() these buffers. > > Whith the existing implementation, the userspace pointer was pointing > to a completely different physical region, thus leading to bad display > output and memory corruptions. > > I'm not sure the xtensa_dma_mmap() implementation is correct, but it > seems to solve my problem. > > Don't hesitate to propose a different implementation. Could you please instead check if the dma_common_mmap works for you with the attached patch? [...] > diff --git a/arch/xtensa/kernel/pci-dma.c b/arch/xtensa/kernel/pci-dma.c > index 70e362e6038e..8f3ef49ceba6 100644 > --- a/arch/xtensa/kernel/pci-dma.c > +++ b/arch/xtensa/kernel/pci-dma.c > @@ -249,9 +249,32 @@ int xtensa_dma_mapping_error(struct device *dev, dma_addr_t dma_addr) > return 0; > } > > +static int xtensa_dma_mmap(struct device *dev, struct vm_area_struct *vma, > + void *cpu_addr, dma_addr_t dma_addr, size_t size, > + unsigned long attrs) > +{ > + int ret = -ENXIO; > +#ifdef CONFIG_MMU > + unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; > + unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; > + unsigned long pfn = PFN_DOWN(dma_addr); > + unsigned long off = vma->vm_pgoff; > + > + if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) > + return ret; > + > + if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) > + ret = remap_pfn_range(vma, vma->vm_start, pfn + off, > + vma->vm_end - vma->vm_start, > + vma->vm_page_prot); You use vma->vm_page_prot directly here, isn't it required to do vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); to make the mapping uncached? Because otherwise I guess you get a mapping with cache which on Xtensa is not going to be coherent. -- Thanks. -- Max --94eb2c19e4bcca3e80054be65f43 Content-Type: text/x-patch; charset=US-ASCII; name="0001-WIP-xtensa-make-__pa-work-with-uncached-KSEG.patch" Content-Disposition: attachment; filename="0001-WIP-xtensa-make-__pa-work-with-uncached-KSEG.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_j0vk8s8h0 RnJvbSBlOTNhOTcxZTZhNDgwMjkyN2Y3MGE3MzBmZDNiNzFlNmFlNjlmZTM5IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBNYXggRmlsaXBwb3YgPGpjbXZia2JjQGdtYWlsLmNvbT4KRGF0 ZTogV2VkLCAyOSBNYXIgMjAxNyAxNTo0NDo0NyAtMDcwMApTdWJqZWN0OiBbUEFUQ0hdIFdJUDog eHRlbnNhOiBtYWtlIF9fcGEgd29yayB3aXRoIHVuY2FjaGVkIEtTRUcKClNpZ25lZC1vZmYtYnk6 IE1heCBGaWxpcHBvdiA8amNtdmJrYmNAZ21haWwuY29tPgotLS0KIGFyY2gveHRlbnNhL2luY2x1 ZGUvYXNtL3BhZ2UuaCB8IDEzICsrKysrKysrKysrKysKIDEgZmlsZSBjaGFuZ2VkLCAxMyBpbnNl cnRpb25zKCspCgpkaWZmIC0tZ2l0IGEvYXJjaC94dGVuc2EvaW5jbHVkZS9hc20vcGFnZS5oIGIv YXJjaC94dGVuc2EvaW5jbHVkZS9hc20vcGFnZS5oCmluZGV4IDk3NmIxZDcuLmNhMDIxNjEgMTAw NjQ0Ci0tLSBhL2FyY2gveHRlbnNhL2luY2x1ZGUvYXNtL3BhZ2UuaAorKysgYi9hcmNoL3h0ZW5z YS9pbmNsdWRlL2FzbS9wYWdlLmgKQEAgLTE2NCw4ICsxNjQsMjEgQEAgdm9pZCBjb3B5X3VzZXJf aGlnaHBhZ2Uoc3RydWN0IHBhZ2UgKnRvLCBzdHJ1Y3QgcGFnZSAqZnJvbSwKIAogI2RlZmluZSBB UkNIX1BGTl9PRkZTRVQJCShQSFlTX09GRlNFVCA+PiBQQUdFX1NISUZUKQogCisjaWZkZWYgQ09O RklHX01NVQorc3RhdGljIGlubGluZSB1bnNpZ25lZCBsb25nIF9fX3BhKHVuc2lnbmVkIGxvbmcg dmEpCit7CisJdW5zaWduZWQgbG9uZyBvZmYgPSB2YSAtIFBBR0VfT0ZGU0VUOworCisJaWYgKG9m ZiA+IFhDSEFMX0tTRUdfU0laRSkKKwkJb2ZmIC09IFhDSEFMX0tTRUdfU0laRTsKKworCXJldHVy biBvZmYgKyBQSFlTX09GRlNFVDsKK30KKyNkZWZpbmUgX19wYSh4KQlfX19wYSgodW5zaWduZWQg bG9uZykoeCkpCisjZWxzZQogI2RlZmluZSBfX3BhKHgpCVwKIAkoKHVuc2lnbmVkIGxvbmcpICh4 KSAtIFBBR0VfT0ZGU0VUICsgUEhZU19PRkZTRVQpCisjZW5kaWYKICNkZWZpbmUgX192YSh4KQlc CiAJKCh2b2lkICopKCh1bnNpZ25lZCBsb25nKSAoeCkgLSBQSFlTX09GRlNFVCArIFBBR0VfT0ZG U0VUKSkKICNkZWZpbmUgcGZuX3ZhbGlkKHBmbikgXAotLSAKMi4xLjQKCg== --94eb2c19e4bcca3e80054be65f43--