* Re: [PATCH v2] arm64: pci: add support for pci_mmap_page_range [not found] ` <20160415130953.GI22906@arm.com> @ 2017-03-16 12:17 ` David Woodhouse 2017-03-20 13:21 ` Will Deacon 0 siblings, 1 reply; 3+ messages in thread From: David Woodhouse @ 2017-03-16 12:17 UTC (permalink / raw) To: Will Deacon, Jerin Jacob Cc: lorenzo.pieralisi, benh, david.daney, catalin.marinas, Liviu.Dudau, rrichter, hanjun.guo, linux-pci, bhelgaas, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 2021 bytes --] 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? 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? 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...) [-- Attachment #1.2: smime.p7s --] [-- Type: application/x-pkcs7-signature, Size: 4938 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] arm64: pci: add support for pci_mmap_page_range 2017-03-16 12:17 ` [PATCH v2] arm64: pci: add support for pci_mmap_page_range David Woodhouse @ 2017-03-20 13:21 ` Will Deacon 0 siblings, 0 replies; 3+ messages in thread From: Will Deacon @ 2017-03-20 13:21 UTC (permalink / raw) To: David Woodhouse Cc: Jerin Jacob, lorenzo.pieralisi, david.daney, catalin.marinas, Liviu.Dudau, rrichter, hanjun.guo, bhelgaas, linux-arm-kernel, linux-pci, benh 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <29393988.imklgXkpJX@wuerfel>]
[parent not found: <20160418152126.GA3154@localhost.localdomain>]
[parent not found: <4342694.H5cy8vaG9k@wuerfel>]
[parent not found: <1489611696.4195.186.camel@infradead.org>]
[parent not found: <20170320131836.GM17263@arm.com>]
* Re: [PATCH v2] arm64: pci: add support for pci_mmap_page_range [not found] ` <20170320131836.GM17263@arm.com> @ 2017-03-20 14:07 ` David Woodhouse 0 siblings, 0 replies; 3+ messages in thread From: David Woodhouse @ 2017-03-20 14:07 UTC (permalink / raw) To: Will Deacon Cc: Jerin Jacob, lorenzo.pieralisi, Arnd Bergmann, david.daney, catalin.marinas, Liviu.Dudau, rrichter, hanjun.guo, linux-pci, bhelgaas, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 1703 bytes --] On Mon, 2017-03-20 at 13:18 +0000, Will Deacon wrote: > > The thing is, this *isn't* an architecture-specific interface where you > > get a clean slate. It's a cross-platform interface. Legacy and horrid, > > sure. But it does you no harm. > I don't agree with that: it provides (privileged) userspace with a way to > map non-prefetchable BARs using write-combining memory attributes, which > could lead to mismatched memory attributes against a kernel mapping of the > same BAR and is something that you can't achieve using the sysfs API. I think that's just a bug. I'll add it to my list. We shouldn't be allowing a WC mapping on a non-prefetchable resource, should we? For that matter, I think it allows you mmap a range of MMIO addresses that correspond to an I/O BAR, and on platforms which allow pci_mmap_io, the converse. That seems... suboptimal. > > What *else* don't you like having in /proc? Shall we have a clean slate > > and eliminate *everything* other than actual processes from /proc for > > the next architecture we add to the tree? If not, why not? > It's not about what we like and don't like in /proc, it's about not > promoting legacy that we don't need. If somebody actually needs the /proc > interface, fine, we can support it. But all the people asking for this have > been concerned solely about the sysfs interface, so I'd just like the two > divorced from each other so that we can provide what people are asking for > without pulling in a deprecated interface at the same time. > > This should be straightforward. Sure, but fairly much orthogonal. I'll roll it in. It's fairly much in the noise now I'm this far down the rabbithole... [-- Attachment #1.2: smime.p7s --] [-- Type: application/x-pkcs7-signature, Size: 4938 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-03-20 14:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1460581856-12380-1-git-send-email-jerin.jacob@caviumnetworks.com>
[not found] ` <20160415130953.GI22906@arm.com>
2017-03-16 12:17 ` [PATCH v2] arm64: pci: add support for pci_mmap_page_range David Woodhouse
2017-03-20 13:21 ` Will Deacon
[not found] ` <29393988.imklgXkpJX@wuerfel>
[not found] ` <20160418152126.GA3154@localhost.localdomain>
[not found] ` <4342694.H5cy8vaG9k@wuerfel>
[not found] ` <1489611696.4195.186.camel@infradead.org>
[not found] ` <20170320131836.GM17263@arm.com>
2017-03-20 14:07 ` David Woodhouse
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).