Re: [PATCH v2] arm64: pci: add support for pci_mmap_page_range

Re: [PATCH v2] arm64: pci: add support for pci_mmap_page_range

[David Woodhouse]

[-- 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

Re: [PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Will Deacon]

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

Re: [PATCH v2] arm64: pci: add support for pci_mmap_page_range

[David Woodhouse]

[-- 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