Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1606491 > unrolled thread

[PATCH 00/17] PCI resource mmap cleanup

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/17] PCI resource mmap cleanup David Woodhouse <dwmw2@infradead.org> - 2017-03-22 14:40 +0100
    [PATCH 04/17] pci: Add arch_can_pci_mmap_wc() macro David Woodhouse <dwmw2@infradead.org> - 2017-03-22 14:40 +0100
    [PATCH 15/17] sh: Use generic pci_mmap_resource_range() David Woodhouse <dwmw2@infradead.org> - 2017-03-22 14:40 +0100
    [PATCH 10/17] arm: Use generic pci_mmap_resource_range() David Woodhouse <dwmw2@infradead.org> - 2017-03-22 14:40 +0100
    [PATCH 17/17] arm64: Do not expose PCI mmap through procfs David Woodhouse <dwmw2@infradead.org> - 2017-03-22 14:40 +0100
      Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs Sinan Kaya <okaya@codeaurora.org> - 2017-03-22 15:00 +0100
        Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs David Woodhouse <dwmw2@infradead.org> - 2017-03-22 15:10 +0100
          Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs Will Deacon <will.deacon@arm.com> - 2017-03-22 15:20 +0100
            Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs Sinan Kaya <okaya@codeaurora.org> - 2017-03-22 16:50 +0100
          Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs Sinan Kaya <okaya@codeaurora.org> - 2017-03-22 15:20 +0100
      Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs Arnd Bergmann <arnd@arndb.de> - 2017-03-24 17:20 +0100
        Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs Arnd Bergmann <arnd@arndb.de> - 2017-03-24 17:20 +0100
          Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs David Woodhouse <dwmw2@infradead.org> - 2017-03-24 17:30 +0100
        Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs David Woodhouse <dwmw2@infradead.org> - 2017-03-24 17:30 +0100
    [PATCH 18/17] x86: Use generic pci_mmap_resource_range() David Woodhouse <dwmw2@infradead.org> - 2017-03-23 15:30 +0100
    Re: [PATCH 00/17] PCI resource mmap cleanup David Woodhouse <dwmw2@infradead.org> - 2017-03-24 12:50 +0100
      Re: [PATCH 00/17] PCI resource mmap cleanup "Luck, Tony" <tony.luck@intel.com> - 2017-03-24 18:00 +0100
    Re: [PATCH 00/17] PCI resource mmap cleanup Arnd Bergmann <arnd@arndb.de> - 2017-03-24 17:30 +0100

#1606491 — [PATCH 00/17] PCI resource mmap cleanup

This started out as a fairly trivial "add pci_mmap_page_range() for 
ARM64" patch. But pci_mmap_page_range() is a vile interface, taking 
"user visible" resource addresses converted with pci_resource_to_user() 
on those platforms unlucky enough to use that... and even in the *sane* 
sysfs-based mmap method, we convert through user addresses to call the
platform-specific method.

In most cases there's just no need for any of this crap. We can migrate
most architectures to a generic implementation without much thought,
and the few that aren't converted in this series can probably be added
fairly easily too but need a little more arch-specific attention.

Utterly untested for now; I'll do some testing while I deal with the
inevitable bikeshedding.

David Woodhouse (17):
  pci: Fix pci_mmap_fits() for HAVE_PCI_RESOURCE_TO_USER platforms
  pci: Fix another sanity check bug in /proc/pci mmap
  pci: Only allow WC mmap on prefetchable resources
  pci: Add arch_can_pci_mmap_wc() macro
  pci: Move multiple declarations of pci_mmap_page_range() to <linux/pci.h>
  pci: Add HAVE_PCI_MMAP_IO to architectures which can mmap() I/O space
  pci: Use BAR index in sysfs attr->private instead of resource pointer
  pci: Add BAR index argument to pci_mmap_page_range()
  pci: Add pci_mmap_resource_range() and use it for ARM64
  arm: Use generic pci_mmap_resource_range()
  cris: Use generic pci_mmap_resource_range()
  mips: Use generic pci_mmap_resource_range()
  mn10300: Use generic pci_mmap_resource_range()
  parisc: Use generic pci_mmap_resource_range()
  sh: Use generic pci_mmap_resource_range()
  unicore: Use generic pci_mmap_resource_range()
  arm64: Do not expose PCI mmap through procfs

 Documentation/filesystems/sysfs-pci.txt |  9 +++-
 arch/arm/include/asm/pci.h              |  3 +-
 arch/arm/kernel/bios32.c                | 19 -------
 arch/arm64/include/asm/pci.h            |  3 ++
 arch/cris/arch-v32/drivers/pci/bios.c   | 22 --------
 arch/cris/include/asm/pci.h             |  4 +-
 arch/ia64/include/asm/pci.h             |  4 +-
 arch/ia64/pci/pci.c                     |  3 +-
 arch/microblaze/include/asm/pci.h       |  6 +--
 arch/microblaze/pci/pci-common.c        |  2 +-
 arch/mips/include/asm/pci.h             |  5 +-
 arch/mips/pci/pci.c                     | 24 ---------
 arch/mn10300/include/asm/pci.h          |  4 +-
 arch/mn10300/unit-asb2305/pci-asb2305.c | 23 ---------
 arch/parisc/include/asm/pci.h           |  4 +-
 arch/parisc/kernel/pci.c                | 28 ----------
 arch/powerpc/include/asm/pci.h          |  9 ++--
 arch/powerpc/kernel/pci-common.c        |  3 +-
 arch/sh/drivers/pci/pci.c               | 21 --------
 arch/sh/include/asm/pci.h               |  4 +-
 arch/sparc/include/asm/pci_64.h         |  5 +-
 arch/sparc/kernel/pci.c                 |  6 +--
 arch/unicore32/include/asm/pci.h        |  3 +-
 arch/unicore32/kernel/pci.c             | 23 ---------
 arch/x86/include/asm/pci.h              |  6 +--
 arch/x86/pci/i386.c                     |  3 +-
 arch/xtensa/include/asm/pci.h           | 11 ++--
 arch/xtensa/kernel/pci.c                |  5 +-
 drivers/pci/Makefile                    |  2 +-
 drivers/pci/mmap.c                      | 90 +++++++++++++++++++++++++++++++++
 drivers/pci/pci-sysfs.c                 | 77 +++++++++++++---------------
 drivers/pci/proc.c                      | 55 ++++++++++++++------
 include/linux/pci.h                     | 19 +++++++
 33 files changed, 233 insertions(+), 272 deletions(-)
 create mode 100644 drivers/pci/mmap.c

-- 
2.9.3

[toc] | [next] | [standalone]

#1606497 — [PATCH 04/17] pci: Add arch_can_pci_mmap_wc() macro

From: David Woodhouse <dwmw@amazon.co.uk>

Most of the almost-identical versions of pci_mmap_page_range() silently
ignore the 'write_combine' argument and give uncached mappings.

Yet we allow the PCIIOC_WRITE_COMBINE ioctl in /proc/bus/pci, expose the
'resourceX_wc' file in sysfs, and allow an attempted mapping to apparently
succeed.

To fix this, introduce a macro arch_can_pci_mmap_wc() which indicates
whether the platform can do a write-combining mapping. On x86 this ends
up being pat_enabled(), while the few other platforms that support it
can just set it to a literal '1'.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 Documentation/filesystems/sysfs-pci.txt |  4 ++++
 arch/ia64/include/asm/pci.h             |  2 ++
 arch/powerpc/include/asm/pci.h          |  5 +++--
 arch/x86/include/asm/pci.h              |  2 ++
 arch/xtensa/include/asm/pci.h           |  6 +++++-
 arch/xtensa/kernel/pci.c                |  2 +-
 drivers/pci/pci-sysfs.c                 |  6 ++++--
 drivers/pci/proc.c                      | 17 ++++++++++-------
 8 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/Documentation/filesystems/sysfs-pci.txt b/Documentation/filesystems/sysfs-pci.txt
index 6ea1ced..25b7f1c 100644
--- a/Documentation/filesystems/sysfs-pci.txt
+++ b/Documentation/filesystems/sysfs-pci.txt
@@ -117,6 +117,10 @@ code must define HAVE_PCI_MMAP and provide a pci_mmap_page_range function.
 Platforms are free to only support subsets of the mmap functionality, but
 useful return codes should be provided.
 
+Platforms which support write-combining maps of PCI resources must define
+arch_can_pci_mmap_wc() which shall evaluate to non-zero at runtime when
+write-combining is permitted.
+
 Legacy resources are protected by the HAVE_PCI_LEGACY define.  Platforms
 wishing to support legacy functionality should define it and provide
 pci_legacy_read, pci_legacy_write and pci_mmap_legacy_page_range functions.
diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h
index c0835b0..6283758 100644
--- a/arch/ia64/include/asm/pci.h
+++ b/arch/ia64/include/asm/pci.h
@@ -51,6 +51,8 @@ extern unsigned long ia64_max_iommu_merge_mask;
 #define PCI_DMA_BUS_IS_PHYS	(ia64_max_iommu_merge_mask == ~0UL)
 
 #define HAVE_PCI_MMAP
+#define arch_can_pci_mmap_wc()	1
+
 extern int pci_mmap_page_range (struct pci_dev *dev, struct vm_area_struct *vma,
 				enum pci_mmap_state mmap_state, int write_combine);
 #define HAVE_PCI_LEGACY
diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
index 93eded8..b5b68c6 100644
--- a/arch/powerpc/include/asm/pci.h
+++ b/arch/powerpc/include/asm/pci.h
@@ -81,8 +81,9 @@ struct vm_area_struct;
 int pci_mmap_page_range(struct pci_dev *pdev, struct vm_area_struct *vma,
 			enum pci_mmap_state mmap_state, int write_combine);
 
-/* Tell drivers/pci/proc.c that we have pci_mmap_page_range() */
-#define HAVE_PCI_MMAP	1
+/* Tell drivers/pci/proc.c that we have pci_mmap_page_range() and it does WC */
+#define HAVE_PCI_MMAP		1
+#define arch_can_pci_mmap_wc()	1
 
 extern int pci_legacy_read(struct pci_bus *bus, loff_t port, u32 *val,
 			   size_t count);
diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
index 1411dbe..f6e22c2 100644
--- a/arch/x86/include/asm/pci.h
+++ b/arch/x86/include/asm/pci.h
@@ -7,6 +7,7 @@
 #include <linux/string.h>
 #include <linux/scatterlist.h>
 #include <asm/io.h>
+#include <asm/pat.h>
 #include <asm/x86_init.h>
 
 #ifdef __KERNEL__
@@ -102,6 +103,7 @@ int pcibios_set_irq_routing(struct pci_dev *dev, int pin, int irq);
 
 
 #define HAVE_PCI_MMAP
+#define arch_can_pci_mmap_wc()	pat_enabled()
 extern int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
 			       enum pci_mmap_state mmap_state,
 			       int write_combine);
diff --git a/arch/xtensa/include/asm/pci.h b/arch/xtensa/include/asm/pci.h
index 5d6bd93..f106879 100644
--- a/arch/xtensa/include/asm/pci.h
+++ b/arch/xtensa/include/asm/pci.h
@@ -51,7 +51,11 @@ int pci_mmap_page_range(struct pci_dev *pdev, struct vm_area_struct *vma,
 			enum pci_mmap_state mmap_state, int write_combine);
 
 /* Tell drivers/pci/proc.c that we have pci_mmap_page_range() */
-#define HAVE_PCI_MMAP	1
+#define HAVE_PCI_MMAP		1
+
+/* This was wrapped in #if 0 since the first merge of xtensa support...
+#define arch_can_pci_mmap_wc()	1
+*/
 
 #endif /* __KERNEL__ */
 
diff --git a/arch/xtensa/kernel/pci.c b/arch/xtensa/kernel/pci.c
index b848cc3..c5944d3 100644
--- a/arch/xtensa/kernel/pci.c
+++ b/arch/xtensa/kernel/pci.c
@@ -345,7 +345,7 @@ __pci_mmap_set_pgprot(struct pci_dev *dev, struct vm_area_struct *vma,
 
 	/* Set to write-through */
 	prot = (prot & _PAGE_CA_MASK) | _PAGE_CA_WT;
-#if 0
+#ifdef arch_can_pci_mmap_wc
 	if (!write_combine)
 		prot |= _PAGE_WRITETHRU;
 #endif
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 7ac258f..cf2c7d8 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1210,10 +1210,12 @@ static int pci_create_resource_files(struct pci_dev *pdev)
 			continue;
 
 		retval = pci_create_attr(pdev, i, 0);
+#ifdef arch_can_pci_mmap_wc
 		/* for prefetchable resources, create a WC mappable file */
-		if (!retval && pdev->resource[i].flags & IORESOURCE_PREFETCH)
+		if (!retval && arch_can_pci_mmap_wc() &&
+		    pdev->resource[i].flags & IORESOURCE_PREFETCH)
 			retval = pci_create_attr(pdev, i, 1);
-
+#endif
 		if (retval) {
 			pci_remove_resource_files(pdev);
 			return retval;
diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
index dc8912e..c49be71 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -209,15 +209,18 @@ static long proc_bus_pci_ioctl(struct file *file, unsigned int cmd,
 		fpriv->mmap_state = pci_mmap_mem;
 		break;
 
+#ifdef arch_can_pci_mmap_wc
 	case PCIIOC_WRITE_COMBINE:
-		if (arg)
-			fpriv->write_combine = 1;
-		else
-			fpriv->write_combine = 0;
-		break;
-
+		if (arch_can_pci_mmap_wc()) {
+			if (arg)
+				fpriv->write_combine = 1;
+			else
+				fpriv->write_combine = 0;
+			break;
+		}
+		/* If arch decided it can't, fall through... */
+#endif /* arch_can_pci_mmap_wc */
 #endif /* HAVE_PCI_MMAP */
-
 	default:
 		ret = -EINVAL;
 		break;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1606500 — [PATCH 15/17] sh: Use generic pci_mmap_resource_range()

From: David Woodhouse <dwmw@amazon.co.uk>

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/sh/drivers/pci/pci.c | 22 ----------------------
 arch/sh/include/asm/pci.h |  1 +
 2 files changed, 1 insertion(+), 22 deletions(-)

diff --git a/arch/sh/drivers/pci/pci.c b/arch/sh/drivers/pci/pci.c
index c8b36b7..c99ee28 100644
--- a/arch/sh/drivers/pci/pci.c
+++ b/arch/sh/drivers/pci/pci.c
@@ -269,28 +269,6 @@ void __ref pcibios_report_status(unsigned int status_mask, int warn)
 	}
 }
 
-int pci_mmap_page_range(struct pci_dev *dev, int bar,
-			struct vm_area_struct *vma,
-			enum pci_mmap_state mmap_state, int write_combine)
-{
-	/*
-	 * I/O space can be accessed via normal processor loads and stores on
-	 * this platform but for now we elect not to do this and portable
-	 * drivers should not do this anyway.
-	 */
-	if (mmap_state == pci_mmap_io)
-		return -EINVAL;
-
-	/*
-	 * Ignore write-combine; for now only return uncached mappings.
-	 */
-	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
-
-	return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
-			       vma->vm_end - vma->vm_start,
-			       vma->vm_page_prot);
-}
-
 #ifndef CONFIG_GENERIC_IOMAP
 
 void __iomem *__pci_ioport_map(struct pci_dev *dev,
diff --git a/arch/sh/include/asm/pci.h b/arch/sh/include/asm/pci.h
index 46abbc9..17fa69b 100644
--- a/arch/sh/include/asm/pci.h
+++ b/arch/sh/include/asm/pci.h
@@ -66,6 +66,7 @@ extern unsigned long PCIBIOS_MIN_IO, PCIBIOS_MIN_MEM;
 struct pci_dev;
 
 #define HAVE_PCI_MMAP
+#define ARCH_GENERIC_PCI_MMAP_RESOURCE
 
 extern void pcibios_set_master(struct pci_dev *dev);
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1606507 — [PATCH 10/17] arm: Use generic pci_mmap_resource_range()

From: David Woodhouse <dwmw@amazon.co.uk>

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/arm/include/asm/pci.h |  1 +
 arch/arm/kernel/bios32.c   | 20 --------------------
 2 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/arch/arm/include/asm/pci.h b/arch/arm/include/asm/pci.h
index 51118a0..396c92b 100644
--- a/arch/arm/include/asm/pci.h
+++ b/arch/arm/include/asm/pci.h
@@ -29,6 +29,7 @@ static inline int pci_proc_domain(struct pci_bus *bus)
 #define PCI_DMA_BUS_IS_PHYS     (1)
 
 #define HAVE_PCI_MMAP
+#define ARCH_GENERIC_PCI_MMAP_RESOURCE
 
 static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
 {
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index a4fc3f4..b259956 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -597,26 +597,6 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 	return start;
 }
 
-int pci_mmap_page_range(struct pci_dev *dev, int bar,
-			struct vm_area_struct *vma,
-			enum pci_mmap_state mmap_state, int write_combine)
-{
-	if (mmap_state == pci_mmap_io)
-		return -EINVAL;
-
-	/*
-	 * Mark this as IO
-	 */
-	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
-
-	if (remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
-			     vma->vm_end - vma->vm_start,
-			     vma->vm_page_prot))
-		return -EAGAIN;
-
-	return 0;
-}
-
 void __init pci_map_io_early(unsigned long pfn)
 {
 	struct map_desc pci_io_desc = {
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1606508 — [PATCH 17/17] arm64: Do not expose PCI mmap through procfs

From: David Woodhouse <dwmw@amazon.co.uk>

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 drivers/pci/proc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
index 2d9cfa4..a940f4b 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -17,6 +17,11 @@
 
 static int proc_initialized;	/* = 0 */
 
+#ifdef __aarch64__
+/* ARM64 wants to be special and not expose this through /proc like everyone else */
+#undef HAVE_PCI_MMAP
+#endif
+
 static loff_t proc_bus_pci_lseek(struct file *file, loff_t off, int whence)
 {
 	struct pci_dev *dev = PDE_DATA(file_inode(file));
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1606519 — Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs

On 3/22/2017 9:25 AM, David Woodhouse wrote:
>  
> +#ifdef __aarch64__
> +/* ARM64 wants to be special and not expose this through /proc like everyone else */
> +#undef HAVE_PCI_MMAP
> +#endif
> +

Where is this ARM64 special requirement coming from?

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1606527 — Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs

[Multipart message — attachments visible in raw view] — view raw

On Wed, 2017-03-22 at 09:54 -0400, Sinan Kaya wrote:
> On 3/22/2017 9:25 AM, David Woodhouse wrote:
> > 
> >  
> > +#ifdef __aarch64__
> > +/* ARM64 wants to be special and not expose this through /proc
> > like everyone else */
> > +#undef HAVE_PCI_MMAP
> > +#endif
> > +
> Where is this ARM64 special requirement coming from?

The idea is that as a new platform, ARM64 shouldn't need to implement
legacy userspace interfaces.

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422571.html

[toc] | [prev] | [next] | [standalone]

#1606556 — Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs

On Wed, Mar 22, 2017 at 10:15:04AM -0400, Sinan Kaya wrote:
> On 3/22/2017 10:04 AM, David Woodhouse wrote:
> > On Wed, 2017-03-22 at 09:54 -0400, Sinan Kaya wrote:
> >> On 3/22/2017 9:25 AM, David Woodhouse wrote:
> >>>
> >>>  
> >>> +#ifdef __aarch64__
> >>> +/* ARM64 wants to be special and not expose this through /proc
> >>> like everyone else */
> >>> +#undef HAVE_PCI_MMAP
> >>> +#endif
> >>> +
> >> Where is this ARM64 special requirement coming from?
> > 
> > The idea is that as a new platform, ARM64 shouldn't need to implement
> > legacy userspace interfaces.
> > 
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422571.html
> > 
> 
> Aren't we breaking an ABI for userspace? I know DPDK relies on this feature.

It relies on the /proc interface? That's the first I've ever heard of that
-- everybody so far has only been interested in the sysfs stuff.

Nothing's more broken than before, because we've never supported the /proc
interface, but if existing arm64 code out there is failing because of that
then I'm of course open to supporting it. I'm just surprised that nobody
else has come up with that before, since DPDK is in common use.

Can you point me at the specific code, please?

Will

[toc] | [prev] | [next] | [standalone]

#1606673 — Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs

On 3/22/2017 10:18 AM, Will Deacon wrote:
> On Wed, Mar 22, 2017 at 10:15:04AM -0400, Sinan Kaya wrote:
>> On 3/22/2017 10:04 AM, David Woodhouse wrote:
>>> On Wed, 2017-03-22 at 09:54 -0400, Sinan Kaya wrote:
>>>> On 3/22/2017 9:25 AM, David Woodhouse wrote:
>>>>>
>>>>>  
>>>>> +#ifdef __aarch64__
>>>>> +/* ARM64 wants to be special and not expose this through /proc
>>>>> like everyone else */
>>>>> +#undef HAVE_PCI_MMAP
>>>>> +#endif
>>>>> +
>>>> Where is this ARM64 special requirement coming from?
>>>
>>> The idea is that as a new platform, ARM64 shouldn't need to implement
>>> legacy userspace interfaces.
>>>
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422571.html
>>>
>>
>> Aren't we breaking an ABI for userspace? I know DPDK relies on this feature.
> 
> It relies on the /proc interface? That's the first I've ever heard of that
> -- everybody so far has only been interested in the sysfs stuff.
> 
> Nothing's more broken than before, because we've never supported the /proc
> interface, but if existing arm64 code out there is failing because of that
> then I'm of course open to supporting it. I'm just surprised that nobody
> else has come up with that before, since DPDK is in common use.
> 
> Can you point me at the specific code, please?

I'm correcting myself. I had to go back my memory from last year. 

DPDK requires HAVE_PCI_MMAP to be set. We have been carrying
some old maillist patch around for DPDK customers internally.

When HAVE_PCI_MMAP is set, resource files are created in sysfs and procfs.
DPDK is using the files in sysfs directory not procfs directory. 

Having HAVE_PCI_MMAP defined is the DPDK requirement.

> 
> Will
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1606566 — Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs

On 3/22/2017 10:04 AM, David Woodhouse wrote:
> On Wed, 2017-03-22 at 09:54 -0400, Sinan Kaya wrote:
>> On 3/22/2017 9:25 AM, David Woodhouse wrote:
>>>
>>>  
>>> +#ifdef __aarch64__
>>> +/* ARM64 wants to be special and not expose this through /proc
>>> like everyone else */
>>> +#undef HAVE_PCI_MMAP
>>> +#endif
>>> +
>> Where is this ARM64 special requirement coming from?
> 
> The idea is that as a new platform, ARM64 shouldn't need to implement
> legacy userspace interfaces.
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422571.html
> 

Aren't we breaking an ABI for userspace? I know DPDK relies on this feature.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1608613 — Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs

On Wed, Mar 22, 2017 at 2:25 PM, David Woodhouse <dwmw2@infradead.org> wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  drivers/pci/proc.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
> index 2d9cfa4..a940f4b 100644
> --- a/drivers/pci/proc.c
> +++ b/drivers/pci/proc.c
> @@ -17,6 +17,11 @@
>
>  static int proc_initialized;   /* = 0 */
>
> +#ifdef __aarch64__
> +/* ARM64 wants to be special and not expose this through /proc like everyone else */
> +#undef HAVE_PCI_MMAP
> +#endif

I'd still prefer this to be a whitelist of the existing architectures using PCI
MMAP in procfs, there is really no reason for arm64 to be special, the
one thing we want to control here is whether new architectures (including
arm64) that have never had either the sysfs or the procfs interface
should get one or both of them.

As it seems that there are important use cases for the sysfs interface
and your patch series will just make that work everywhere, I'd argue
that we should just always provide the sysfs interface now, and use
HAVE_PCI_MMAP only control the procfs interface.

That way, we turn on the sysfs interface on arc, arm64, frv and tile
as well as any future architecture with PCI support, but leave
the procfs support as opt-in.

       Arnd

[toc] | [prev] | [next] | [standalone]

#1608622 — Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs

On Fri, Mar 24, 2017 at 5:13 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Mar 22, 2017 at 2:25 PM, David Woodhouse <dwmw2@infradead.org> wrote:
>> From: David Woodhouse <dwmw@amazon.co.uk>
>>
>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>> ---
>>  drivers/pci/proc.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
>> index 2d9cfa4..a940f4b 100644
>> --- a/drivers/pci/proc.c
>> +++ b/drivers/pci/proc.c
>> @@ -17,6 +17,11 @@
>>
>>  static int proc_initialized;   /* = 0 */
>>
>> +#ifdef __aarch64__
>> +/* ARM64 wants to be special and not expose this through /proc like everyone else */
>> +#undef HAVE_PCI_MMAP
>> +#endif
>
> I'd still prefer this to be a whitelist of the existing architectures using PCI
> MMAP in procfs, there is really no reason for arm64 to be special, the
> one thing we want to control here is whether new architectures (including
> arm64) that have never had either the sysfs or the procfs interface
> should get one or both of them.
>
> As it seems that there are important use cases for the sysfs interface
> and your patch series will just make that work everywhere, I'd argue
> that we should just always provide the sysfs interface now, and use
> HAVE_PCI_MMAP only control the procfs interface.
>
> That way, we turn on the sysfs interface on arc, arm64, frv and tile
> as well as any future architecture with PCI support, but leave
> the procfs support as opt-in.

Something alone these lines, to replace your patch 17/17 and the
one that turns on HAVE_PCI_MMAP for arm64.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 25d010d449a3..c517f1b724e0 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -980,8 +980,6 @@ void pci_remove_legacy_files(struct pci_bus *b)
 }
 #endif /* HAVE_PCI_LEGACY */

-#ifdef HAVE_PCI_MMAP
-
 int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vma,
   enum pci_mmap_api mmap_api)
 {
@@ -1217,10 +1215,6 @@ static int pci_create_resource_files(struct
pci_dev *pdev)
  }
  return 0;
 }
-#else /* !HAVE_PCI_MMAP */
-int __weak pci_create_resource_files(struct pci_dev *dev) { return 0; }
-void __weak pci_remove_resource_files(struct pci_dev *dev) { return; }
-#endif /* HAVE_PCI_MMAP */

 /**
  * pci_write_rom - used to enable access to the PCI ROM display
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 8dd38e69d6f2..6c2a15d4ebf9 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -21,14 +21,12 @@ void pci_create_firmware_label_files(struct pci_dev *pdev);
 void pci_remove_firmware_label_files(struct pci_dev *pdev);
 #endif
 void pci_cleanup_rom(struct pci_dev *dev);
-#ifdef HAVE_PCI_MMAP
 enum pci_mmap_api {
  PCI_MMAP_SYSFS, /* mmap on /sys/bus/pci/devices/<BDF>/resource<N> */
  PCI_MMAP_PROCFS /* mmap on /proc/bus/pci/<BDF> */
 };
 int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai,
   enum pci_mmap_api mmap_api);
-#endif
 int pci_probe_reset_function(struct pci_dev *dev);

 /**

[toc] | [prev] | [next] | [standalone]

#1608634 — Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs

[Multipart message — attachments visible in raw view] — view raw

On Fri, 2017-03-24 at 17:16 +0100, Arnd Bergmann wrote:
> 
> Something alone these lines, to replace your patch 17/17 and the
> one that turns on HAVE_PCI_MMAP for arm64.

No, I think that won't build because you don't have a
pci_mmap_page_range() function. And you didn't define
ARCH_GENERIC_PCI_MMAP_RESOURCE_RANGE. You probably get away with it if
you just fix up the ARCH_GENERIC_PCI_MMAP_RESOURCE_RANGE checks in
drivers/pci/mmap.c to be !HAVE_PCI_MMAP?

[toc] | [prev] | [next] | [standalone]

#1608641 — Re: [PATCH 17/17] arm64: Do not expose PCI mmap through procfs

[Multipart message — attachments visible in raw view] — view raw

On Fri, 2017-03-24 at 17:13 +0100, Arnd Bergmann wrote:
> 
> I'd still prefer this to be a whitelist of the existing architectures using PCI
> MMAP in procfs, there is really no reason for arm64 to be special, the
> one thing we want to control here is whether new architectures (including
> arm64) that have never had either the sysfs or the procfs interface
> should get one or both of them.
>
> As it seems that there are important use cases for the sysfs interface
> and your patch series will just make that work everywhere, I'd argue
> that we should just always provide the sysfs interface now, and use
> HAVE_PCI_MMAP only control the procfs interface.

I still have no sympathy for the "we don't want <this> tiny part of the
legacy generic non-architecture-specific procfs ABI to exist on ARM64".
Why not just kill /proc/bus/pci *entirely* there?

But sure, I suppose we could refactor things so that the sysfs mmap
bits depend on (HAVE_PCI_MMAP || ARCH_GENERIC_MMAP_RESOURCE_RANGE)
rather than having architectures define the latter in *addition* to the
former.

> That way, we turn on the sysfs interface on arc, arm64, frv and tile
> as well as any future architecture with PCI support, but leave
> the procfs support as opt-in.

OK. My plan was for ARCH_GENERIC_MMAP_RESOURCE_RANGE to go away once
all architectures were converted, and HAVE_PCI_MMAP to be all that's
left. It does make send to do arc, fr-v and tile too though. So we can
do it that way.

[toc] | [prev] | [next] | [standalone]

#1607535 — [PATCH 18/17] x86: Use generic pci_mmap_resource_range()

From: David Woodhouse <dwmw@amazon.co.uk>

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
Actually it turns out this is fairly trivial for x86 too; it just looked
more interesting at first glance. But pgprot_writecombine() and
pgprot_device() will both do the right thing here, and allowing WC
conditionally based on pat_enabled() is already working so we'll never
get asked to do that when we don't want to.

 arch/x86/include/asm/pci.h |  1 +
 arch/x86/pci/i386.c        | 48 ----------------------------------------------
 2 files changed, 1 insertion(+), 48 deletions(-)

diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
index 734cc94..f513cc2 100644
--- a/arch/x86/include/asm/pci.h
+++ b/arch/x86/include/asm/pci.h
@@ -104,6 +104,7 @@ int pcibios_set_irq_routing(struct pci_dev *dev, int pin, int irq);
 
 #define HAVE_PCI_MMAP
 #define arch_can_pci_mmap_wc()	pat_enabled()
+#define ARCH_GENERIC_PCI_MMAP_RESOURCE
 
 #ifdef CONFIG_PCI
 extern void early_quirks(void);
diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index 8ca5e5d..68499cf 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -406,51 +406,3 @@ void __init pcibios_resource_survey(void)
 	 */
 	ioapic_insert_resources();
 }
-
-static const struct vm_operations_struct pci_mmap_ops = {
-	.access = generic_access_phys,
-};
-
-int pci_mmap_page_range(struct pci_dev *dev, int bar,
-			struct vm_area_struct *vma,
-			enum pci_mmap_state mmap_state, int write_combine)
-{
-	unsigned long prot;
-
-	/* I/O space cannot be accessed via normal processor loads and
-	 * stores on this platform.
-	 */
-	if (mmap_state == pci_mmap_io)
-		return -EINVAL;
-
-	prot = pgprot_val(vma->vm_page_prot);
-
-	/*
- 	 * Return error if pat is not enabled and write_combine is requested.
- 	 * Caller can followup with UC MINUS request and add a WC mtrr if there
- 	 * is a free mtrr slot.
- 	 */
-	if (!pat_enabled() && write_combine)
-		return -EINVAL;
-
-	if (pat_enabled() && write_combine)
-		prot |= cachemode2protval(_PAGE_CACHE_MODE_WC);
-	else if (pat_enabled() || boot_cpu_data.x86 > 3)
-		/*
-		 * ioremap() and ioremap_nocache() defaults to UC MINUS for now.
-		 * To avoid attribute conflicts, request UC MINUS here
-		 * as well.
-		 */
-		prot |= cachemode2protval(_PAGE_CACHE_MODE_UC_MINUS);
-
-	vma->vm_page_prot = __pgprot(prot);
-
-	if (io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
-			       vma->vm_end - vma->vm_start,
-			       vma->vm_page_prot))
-		return -EAGAIN;
-
-	vma->vm_ops = &pci_mmap_ops;
-
-	return 0;
-}
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1608353

[Multipart message — attachments visible in raw view] — view raw

On Wed, 2017-03-22 at 13:25 +0000, David Woodhouse wrote:
> This started out as a fairly trivial "add pci_mmap_page_range() for 
> ARM64" patch. But pci_mmap_page_range() is a vile interface, taking 
> "user visible" resource addresses converted with pci_resource_to_user() 
> on those platforms unlucky enough to use that... and even in the *sane* 
> sysfs-based mmap method, we convert through user addresses to call the
> platform-specific method.
> 
> In most cases there's just no need for any of this crap. We can migrate
> most architectures to a generic implementation without much thought,
> and the few that aren't converted in this series can probably be added
> fairly easily too but need a little more arch-specific attention.
> 
> Utterly untested for now; I'll do some testing while I deal with the
> inevitable bikeshedding.

I added PowerPC too. Rather than posting it here as patches 18/17 and
19/17 I'll just point at 
http://git.infradead.org/users/dwmw2/random-2.6.git/shortlog/refs/heads/pcimmap

To support pci_mmap_io I added a pci_iobar_pfn() function which the
arch must provide, to adjust vma->vm_pgoff to the physical address of
the I/O window of the appropriate PCI host controller. It looks
something like this:

int pci_iobar_pfn(struct pci_dev *pdev, int bar, struct vm_area_struct *vma)
{
	struct pci_controller *hose = pci_bus_to_host(pdev->bus);
	resource_size_t ioaddr = pci_resource_start(pdev, bar);

	if (!hose)
		return -EINVAL;

	/* Convert to an offset within this PCI controller */
	ioaddr -= (unsigned long)hose->io_base_virt - _IO_BASE;

	vma->vm_pgoff += (ioaddr + hose->io_base_phys) >> PAGE_SHIFT;
	return 0;
}

It looks like SPARC, xtensa and Microblaze can all do the same thing,
as they were all basically the same code in the first place.
That leaves IA64 as the last holdout, as the selection of vm_page_prot
there is rather complicated:

	prot = phys_mem_access_prot(NULL, vma->vm_pgoff, size,
				    vma->vm_page_prot);

	/*
	 * If the user requested WC, the kernel uses UC or WC for this region,
	 * and the chipset supports WC, we can use WC. Otherwise, we have to
	 * use the same attribute the kernel uses.
	 */
	if (write_combine &&
	    ((pgprot_val(prot) & _PAGE_MA_MASK) == _PAGE_MA_UC ||
	     (pgprot_val(prot) & _PAGE_MA_MASK) == _PAGE_MA_WC) &&
	    efi_range_is_wc(vma->vm_start, vma->vm_end - vma->vm_start))
		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
	else
		vma->vm_page_prot = prot;


But I suspect it's *overcomplicated*, as the kernel should only ever be
mapping PCI memory BARs as UC or WC in the first place, so the middle
two checks in the if (write_combine…) condition are redundant.

And if the efi_range_is_wc() check isn't gratuitous, perhaps that
should be in the generic code whenever CONFIG_EFI is set?

Tony?

> David Woodhouse (17):
>   pci: Fix pci_mmap_fits() for HAVE_PCI_RESOURCE_TO_USER platforms
>   pci: Fix another sanity check bug in /proc/pci mmap
>   pci: Only allow WC mmap on prefetchable resources
>   pci: Add arch_can_pci_mmap_wc() macro
>   pci: Move multiple declarations of pci_mmap_page_range() to 
>   pci: Add HAVE_PCI_MMAP_IO to architectures which can mmap() I/O space
>   pci: Use BAR index in sysfs attr->private instead of resource pointer
>   pci: Add BAR index argument to pci_mmap_page_range()
>   pci: Add pci_mmap_resource_range() and use it for ARM64
>   arm: Use generic pci_mmap_resource_range()
>   cris: Use generic pci_mmap_resource_range()
>   mips: Use generic pci_mmap_resource_range()
>   mn10300: Use generic pci_mmap_resource_range()
>   parisc: Use generic pci_mmap_resource_range()
>   sh: Use generic pci_mmap_resource_range()
>   unicore: Use generic pci_mmap_resource_range()
>   arm64: Do not expose PCI mmap through procfs
> 
>  Documentation/filesystems/sysfs-pci.txt |  9 +++-
>  arch/arm/include/asm/pci.h              |  3 +-
>  arch/arm/kernel/bios32.c                | 19 -------
>  arch/arm64/include/asm/pci.h            |  3 ++
>  arch/cris/arch-v32/drivers/pci/bios.c   | 22 --------
>  arch/cris/include/asm/pci.h             |  4 +-
>  arch/ia64/include/asm/pci.h             |  4 +-
>  arch/ia64/pci/pci.c                     |  3 +-
>  arch/microblaze/include/asm/pci.h       |  6 +--
>  arch/microblaze/pci/pci-common.c        |  2 +-
>  arch/mips/include/asm/pci.h             |  5 +-
>  arch/mips/pci/pci.c                     | 24 ---------
>  arch/mn10300/include/asm/pci.h          |  4 +-
>  arch/mn10300/unit-asb2305/pci-asb2305.c | 23 ---------
>  arch/parisc/include/asm/pci.h           |  4 +-
>  arch/parisc/kernel/pci.c                | 28 ----------
>  arch/powerpc/include/asm/pci.h          |  9 ++--
>  arch/powerpc/kernel/pci-common.c        |  3 +-
>  arch/sh/drivers/pci/pci.c               | 21 --------
>  arch/sh/include/asm/pci.h               |  4 +-
>  arch/sparc/include/asm/pci_64.h         |  5 +-
>  arch/sparc/kernel/pci.c                 |  6 +--
>  arch/unicore32/include/asm/pci.h        |  3 +-
>  arch/unicore32/kernel/pci.c             | 23 ---------
>  arch/x86/include/asm/pci.h              |  6 +--
>  arch/x86/pci/i386.c                     |  3 +-
>  arch/xtensa/include/asm/pci.h           | 11 ++--
>  arch/xtensa/kernel/pci.c                |  5 +-
>  drivers/pci/Makefile                    |  2 +-
>  drivers/pci/mmap.c                      | 90 +++++++++++++++++++++++++++++++++
>  drivers/pci/pci-sysfs.c                 | 77 +++++++++++++---------------
>  drivers/pci/proc.c                      | 55 ++++++++++++++------
>  include/linux/pci.h                     | 19 +++++++
>  33 files changed, 233 insertions(+), 272 deletions(-)
>  create mode 100644 drivers/pci/mmap.c
> 

[toc] | [prev] | [next] | [standalone]

#1608677

On Fri, Mar 24, 2017 at 11:40:33AM +0000, David Woodhouse wrote:
> That leaves IA64 as the last holdout, as the selection of vm_page_prot
> there is rather complicated:
> 
> 	prot = phys_mem_access_prot(NULL, vma->vm_pgoff, size,
> 				    vma->vm_page_prot);
> 
> 	/*
> 	 * If the user requested WC, the kernel uses UC or WC for this region,
> 	 * and the chipset supports WC, we can use WC. Otherwise, we have to
> 	 * use the same attribute the kernel uses.
> 	 */
> 	if (write_combine &&
> 	    ((pgprot_val(prot) & _PAGE_MA_MASK) == _PAGE_MA_UC ||
> 	     (pgprot_val(prot) & _PAGE_MA_MASK) == _PAGE_MA_WC) &&
> 	    efi_range_is_wc(vma->vm_start, vma->vm_end - vma->vm_start))
> 		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> 	else
> 		vma->vm_page_prot = prot;
> 
> 
> But I suspect it's *overcomplicated*, as the kernel should only ever be
> mapping PCI memory BARs as UC or WC in the first place, so the middle
> two checks in the if (write_combine…) condition are redundant.

Agreed.

> And if the efi_range_is_wc() check isn't gratuitous, perhaps that
> should be in the generic code whenever CONFIG_EFI is set?

Sounds dubious whether EFI could even get this right.  The efi
memory map table is static, but we could remap a BAR to a different
spot. Does the efi map have entries for all the places that you
could remap a BAR?  Isn't it more likely a property of the device
whether it supports WC?

-Tony

[toc] | [prev] | [next] | [standalone]

#1608636

On Wed, Mar 22, 2017 at 2:25 PM, David Woodhouse <dwmw2@infradead.org> wrote:
> This started out as a fairly trivial "add pci_mmap_page_range() for
> ARM64" patch. But pci_mmap_page_range() is a vile interface, taking
> "user visible" resource addresses converted with pci_resource_to_user()
> on those platforms unlucky enough to use that... and even in the *sane*
> sysfs-based mmap method, we convert through user addresses to call the
> platform-specific method.
>
> In most cases there's just no need for any of this crap. We can migrate
> most architectures to a generic implementation without much thought,
> and the few that aren't converted in this series can probably be added
> fairly easily too but need a little more arch-specific attention.
>
> Utterly untested for now; I'll do some testing while I deal with the
> inevitable bikeshedding.

Looks good to me overall, I have replied with one request for
clarification, and would like the bikeshed in patch 17 in a different
colour.

      Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web