From: Will Deacon <will.deacon@arm.com> To: David Woodhouse <dwmw2@infradead.org> Cc: Jerin Jacob <jerin.jacob@caviumnetworks.com>, lorenzo.pieralisi@arm.com, david.daney@cavium.com, catalin.marinas@arm.com, Liviu.Dudau@arm.com, rrichter@cavium.com, hanjun.guo@linaro.org, bhelgaas@google.com, linux-arm-kernel@lists.infradead.org, linux-pci <linux-pci@vger.kernel.org>, benh@kernel.crashing.org Subject: Re: [PATCH v2] arm64: pci: add support for pci_mmap_page_range Date: Mon, 20 Mar 2017 13:21:32 +0000 [thread overview] Message-ID: <20170320132132.GN17263@arm.com> (raw) In-Reply-To: <1489666647.4195.238.camel@infradead.org> On Thu, Mar 16, 2017 at 12:17:27PM +0000, David Woodhouse wrote: > On Fri, 2016-04-15 at 14:09 +0100, Will Deacon wrote: > > > > > + if (write_combine) > > > + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > > > + else > > > + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > > > For consistency with ioremap, this should be pgprot_device. > > What's the difference? The different between ioremap (which used pgprot_device) and a mapping created using pgprot_noncached is that the former allows for early acknowledgement of writes (e.g. at a bridge). See this recent series from Lorenzo that is also trying to clean this up: http://lkml.kernel.org/r/20170227151436.18698-1-lorenzo.pieralisi@arm.com > I note that VFIO is using pgprot_noncached() too, in vfio_pci_mmap() — > where it open-codes an entirely arch-agnostic version of > pci_mmap_page_range() all for itself. Should that be changed to > pgprot_device() too? I think so. At least, on arm64, pgprot_noncached is only really needed for PCI config space and "I don't know that this is, but I'm going to map it anyway" regions in /dev/mem. > Let me see if I can get this straight... > > We have the legacy interface through /proc/bus/pci, where the user > passes a "user-visible" bus address not necessarily (on platforms with > HAVE_PCI_RESOURCE_TO_USER) a host physical address. > > The arch-specific pci_mmap_page_range() exists to work around that > translation, on the two platforms which need it. It *also* has (on > about three platforms) support for a write-combining mapping. > > The sysfs interface theough /sys/bus/pci/devices/*/resource* probably > doesn't need to use pci_mmap_page_range() at all, *except* for the > 'resourceX_wc' variant which has write-combining support. > > How about we do the following (probably not in this order): > • Kill pci_mmap_page_range() entirely. > • Implement a generic version which has (arch-assisted) WC support > but no knowledge of the horrid pci_resource_to_user() mapping. > • Require pci_user_to_resource() to be provided by platforms with > HAVE_ARCH_PCI_RESOURCE_TO_USER, and call that from *generic* code, > for the legacy procfs interface, before invoking the generic > replacement for pci_mmap_page_range(). > > (Yes, we still need to support mmap of I/O resources on... is it only > powerpc? And there are a few inconsistencies, like powerpc forcing WC > even on the sysfs files that *don't* have _wc in their name, that > probably want to be cleaned up as we consolidate...) Happy to review patches :) Will
WARNING: multiple messages have this Message-ID (diff)
From: will.deacon@arm.com (Will Deacon) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2] arm64: pci: add support for pci_mmap_page_range Date: Mon, 20 Mar 2017 13:21:32 +0000 [thread overview] Message-ID: <20170320132132.GN17263@arm.com> (raw) In-Reply-To: <1489666647.4195.238.camel@infradead.org> On Thu, Mar 16, 2017 at 12:17:27PM +0000, David Woodhouse wrote: > On Fri, 2016-04-15 at 14:09 +0100, Will Deacon wrote: > > > > > +?????if (write_combine) > > > +?????????????vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > > > +?????else > > > +?????????????vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > > > For consistency with ioremap, this should be pgprot_device. > > What's the difference? The different between ioremap (which used pgprot_device) and a mapping created using pgprot_noncached is that the former allows for early acknowledgement of writes (e.g. at a bridge). See this recent series from Lorenzo that is also trying to clean this up: http://lkml.kernel.org/r/20170227151436.18698-1-lorenzo.pieralisi at arm.com > I note that VFIO is using pgprot_noncached() too, in vfio_pci_mmap() ? > where it open-codes an entirely arch-agnostic version of > pci_mmap_page_range() all for itself. Should that be changed to > pgprot_device() too? I think so. At least, on arm64, pgprot_noncached is only really needed for PCI config space and "I don't know that this is, but I'm going to map it anyway" regions in /dev/mem. > Let me see if I can get this straight... > > We have the legacy interface through /proc/bus/pci, where the user > passes a "user-visible" bus address not necessarily (on platforms with > HAVE_PCI_RESOURCE_TO_USER) a host physical address. > > The arch-specific pci_mmap_page_range() exists to work around that > translation, on the two platforms which need it. It *also* has (on > about three platforms) support for a write-combining mapping. > > The sysfs interface theough /sys/bus/pci/devices/*/resource* probably > doesn't need to use pci_mmap_page_range() at all, *except* for the > 'resourceX_wc' variant which has write-combining support. > > How about we do the following (probably not in this order): > ?? Kill pci_mmap_page_range() entirely. > ?? Implement a generic version which has (arch-assisted) WC support > ? ?but no knowledge of the horrid pci_resource_to_user() mapping. > ?? Require pci_user_to_resource() to be provided by platforms with > ? ?HAVE_ARCH_PCI_RESOURCE_TO_USER, and call that from *generic* code, > ? ?for the legacy procfs interface, before invoking the generic > ? ?replacement for pci_mmap_page_range(). > > (Yes, we still need to support mmap of I/O resources on... is it only > powerpc? And there are a few inconsistencies, like powerpc forcing WC > even on the sysfs files that *don't* have _wc in their name, that > probably want to be cleaned up as we consolidate...) Happy to review patches :) Will
next prev parent reply other threads:[~2017-03-20 13:21 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-04-13 21:10 [PATCH v2] arm64: pci: add support for pci_mmap_page_range Jerin Jacob 2016-04-15 13:09 ` Will Deacon 2016-04-15 18:45 ` Arnd Bergmann 2016-04-18 14:01 ` Jerin Jacob 2016-04-18 14:15 ` Arnd Bergmann 2016-04-18 14:53 ` Jerin Jacob 2016-04-18 15:00 ` Arnd Bergmann 2016-04-18 15:21 ` Jerin Jacob 2016-04-18 15:31 ` Arnd Bergmann 2016-04-18 15:40 ` Will Deacon 2016-04-18 17:45 ` Jerin Jacob 2016-04-18 17:46 ` Arnd Bergmann 2017-03-15 21:01 ` David Woodhouse 2017-03-20 13:18 ` Will Deacon 2017-03-20 14:07 ` David Woodhouse 2017-03-20 14:07 ` David Woodhouse 2016-04-18 13:43 ` Jerin Jacob 2017-03-16 12:17 ` David Woodhouse 2017-03-16 12:17 ` David Woodhouse 2017-03-20 13:21 ` Will Deacon [this message] 2017-03-20 13:21 ` Will Deacon
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170320132132.GN17263@arm.com \ --to=will.deacon@arm.com \ --cc=Liviu.Dudau@arm.com \ --cc=benh@kernel.crashing.org \ --cc=bhelgaas@google.com \ --cc=catalin.marinas@arm.com \ --cc=david.daney@cavium.com \ --cc=dwmw2@infradead.org \ --cc=hanjun.guo@linaro.org \ --cc=jerin.jacob@caviumnetworks.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-pci@vger.kernel.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=rrichter@cavium.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.