All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Woodhouse <dwmw2@infradead.org>
To: Will Deacon <will.deacon@arm.com>,
	Jerin Jacob <jerin.jacob@caviumnetworks.com>
Cc: lorenzo.pieralisi@arm.com, benh@kernel.crashing.org,
	david.daney@cavium.com, catalin.marinas@arm.com,
	Liviu.Dudau@arm.com, rrichter@cavium.com, hanjun.guo@linaro.org,
	linux-pci <linux-pci@vger.kernel.org>,
	bhelgaas@google.com, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2] arm64: pci: add support for pci_mmap_page_range
Date: Thu, 16 Mar 2017 12:17:27 +0000	[thread overview]
Message-ID: <1489666647.4195.238.camel@infradead.org> (raw)
In-Reply-To: <20160415130953.GI22906@arm.com>


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

WARNING: multiple messages have this Message-ID (diff)
From: dwmw2@infradead.org (David Woodhouse)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2] arm64: pci: add support for pci_mmap_page_range
Date: Thu, 16 Mar 2017 12:17:27 +0000	[thread overview]
Message-ID: <1489666647.4195.238.camel@infradead.org> (raw)
In-Reply-To: <20160415130953.GI22906@arm.com>

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...)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 4938 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170316/f016edd6/attachment.bin>

  parent reply	other threads:[~2017-03-16 12:17 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 [this message]
2017-03-16 12:17     ` David Woodhouse
2017-03-20 13:21     ` Will Deacon
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=1489666647.4195.238.camel@infradead.org \
    --to=dwmw2@infradead.org \
    --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=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 \
    --cc=will.deacon@arm.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: link
Be 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.