[PATCH v1 0/3] PCI: Clean up write combining usage for user mmap

[PATCH v1 0/3] PCI: Clean up write combining usage for user mmap

[Bjorn Helgaas]

These patches:

  - Enforce a "no write combining on I/O port space mapping" policy.
    This seems like an obviously good thing but was previously
    enforced only by powerpc.

  - Stop giving powerpc users write combining mappings of prefetchable
    memory they ask for write combining with the procfs
    PCIIOC_WRITE_COMBINE ioctl or a sysfs "resourceN_wc" file.

  - Clean up arch-specific code that related to the above.

Ben, I'm particularly interested in your thoughts about the powerpc
change.  It should "only" affect performance of users who aren't
actually requesting write combining.  I don't know how many of those
users there might be.

Yinghai, I added your sign-off to the powerpc part, since you posted
that exact patch.  Let me know if you don't want that.

These are on my pci/resource branch.

---

Bjorn Helgaas (2):
      PCI: Ignore write combining when mapping I/O port space
      microblaze/PCI: Remove useless __pci_mmap_set_pgprot()

Yinghai Lu (1):
      powerpc/pci: Remove __pci_mmap_set_pgprot()


 arch/microblaze/pci/pci-common.c |   31 +------------------------------
 arch/powerpc/kernel/pci-common.c |   37 ++++---------------------------------
 drivers/pci/proc.c               |    9 ++++++---
 3 files changed, 11 insertions(+), 66 deletions(-)

[PATCH v1 1/3] PCI: Ignore write combining when mapping I/O port space

[Bjorn Helgaas]

PCI exposes files like /proc/bus/pci/00/00.0 in procfs.  These files
support operations like this:

  ioctl(fd, PCIIOC_MMAP_IS_IO);           # request I/O port space
  ioctl(fd, PCIIOC_WRITE_COMBINE, 1);     # request write-combining
  mmap(fd, ...)

Write combining is useful on PCI memory space, but I don't think it makes
sense on PCI I/O port space.

We *could* change proc_bus_pci_ioctl() to make it impossible to set
mmap_state == pci_mmap_io and write_combine at the same time, but that
would break the following sequence, which is currently legal:

  mmap(fd, ...)                           # default is I/O, non-combining
  ioctl(fd, PCIIOC_WRITE_COMBINE, 1);     # request write-combining
  ioctl(fd, PCIIOC_MMAP_IS_MEM);          # request memory space
  mmap(fd, ...)

Ignore the write-combining flag when mapping I/O port space.

This patch should have no functional effect, based on this analysis of all
implementations of pci_mmap_page_range():

  - ia64 mips parisc sh unicore32 x86 do not support mapping of I/O port
    space at all.

  - arm cris microblaze mn10300 sparc xtensa support mapping of I/O port
    space, but ignore the write_combine argument to pci_mmap_page_range().

  - powerpc supports mapping of I/O port space and uses write_combine, and
    it disables write combining for I/O port space in
    __pci_mmap_set_pgprot().

This patch makes it possible to remove __pci_mmap_set_pgprot() from
powerpc, which simplifies that path.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/proc.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
index 3f155e7..2408abe 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -231,7 +231,7 @@ static int proc_bus_pci_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct pci_dev *dev = PDE_DATA(file_inode(file));
 	struct pci_filp_private *fpriv = file->private_data;
-	int i, ret;
+	int i, ret, write_combine;
 
 	if (!capable(CAP_SYS_RAWIO))
 		return -EPERM;
@@ -245,9 +245,12 @@ static int proc_bus_pci_mmap(struct file *file, struct vm_area_struct *vma)
 	if (i >= PCI_ROM_RESOURCE)
 		return -ENODEV;
 
+	if (fpriv->mmap_state == pci_mmap_mem)
+		write_combine = fpriv->write_combine;
+	else
+		write_combine = 0;
 	ret = pci_mmap_page_range(dev, vma,
-				  fpriv->mmap_state,
-				  fpriv->write_combine);
+				  fpriv->mmap_state, write_combine);
 	if (ret < 0)
 		return ret;
 

[PATCH v1 2/3] powerpc/pci: Remove __pci_mmap_set_pgprot()

[Bjorn Helgaas]

From: Yinghai Lu <yinghai@kernel.org>

The powerpc-specific __pci_mmap_set_pgprot() does two things:

  1) Disables write combining for I/O port space mappings

     This only affects procfs mappings.  The pci_mmap_resource() sysfs path
     only requests write combining for resources with IORESOURCE_PREFETCH
     set, which doesn't include I/O resources.

     The only way to request write combining for I/O port space mappings
     was via the PCIIOC_WRITE_COMBINE ioctl and the proc_bus_pci_mmap()
     path, and we recently changed that path to ignore write combining for
     I/O, so this code in powerpc is no longer needed.

  2) Automatically enables write combining for mappings of prefetchable
     resources, even if not requested by the user

     Both procfs (via PCIIOC_MMAP_IS_MEM and PCIIOC_WRITE_COMBINE ioctls)
     and sysfs (via "resourceN_wc" files, which are created for resources
     with IORESOURCE_PREFETCH) provide ways for the user to map PCI memory
     space with write combining.

     Users that desire write combining should use one of those ways instead
     of relying on powerpc-specific behavior.

Remove the powerpc-specific __pci_mmap_set_pgprot().

The user-visible effect of this change is that users mapping prefetchable
PCI memory space via procfs without PCIIOC_WRITE_COMBINE or via sysfs
"resourceN" (not "resourceN_wc") will get regular uncacheable mappings
instead of the write combining mappings they used to get.

The new behavior matches the behavior on all other arches that support
write combining mapping.

[bhelgaas: changelog]
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/powerpc/kernel/pci-common.c |   37 ++++---------------------------------
 1 file changed, 4 insertions(+), 33 deletions(-)

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 0f7a60f..8c6beb0 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -356,36 +356,6 @@ static struct resource *__pci_mmap_make_offset(struct pci_dev *dev,
 }
 
 /*
- * Set vm_page_prot of VMA, as appropriate for this architecture, for a pci
- * device mapping.
- */
-static pgprot_t __pci_mmap_set_pgprot(struct pci_dev *dev, struct resource *rp,
-				      pgprot_t protection,
-				      enum pci_mmap_state mmap_state,
-				      int write_combine)
-{
-
-	/* Write combine is always 0 on non-memory space mappings. On
-	 * memory space, if the user didn't pass 1, we check for a
-	 * "prefetchable" resource. This is a bit hackish, but we use
-	 * this to workaround the inability of /sysfs to provide a write
-	 * combine bit
-	 */
-	if (mmap_state != pci_mmap_mem)
-		write_combine = 0;
-	else if (write_combine == 0) {
-		if (rp->flags & IORESOURCE_PREFETCH)
-			write_combine = 1;
-	}
-
-	/* XXX would be nice to have a way to ask for write-through */
-	if (write_combine)
-		return pgprot_noncached_wc(protection);
-	else
-		return pgprot_noncached(protection);
-}
-
-/*
  * This one is used by /dev/mem and fbdev who have no clue about the
  * PCI device, it tries to find the PCI device first and calls the
  * above routine
@@ -458,9 +428,10 @@ int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
 		return -EINVAL;
 
 	vma->vm_pgoff = offset >> PAGE_SHIFT;
-	vma->vm_page_prot = __pci_mmap_set_pgprot(dev, rp,
-						  vma->vm_page_prot,
-						  mmap_state, write_combine);
+	if (write_combine)
+		vma->vm_page_prot = pgprot_noncached_wc(vma->vm_page_prot);
+	else
+		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 
 	ret = remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
 			       vma->vm_end - vma->vm_start, vma->vm_page_prot);

Re: [PATCH v1 2/3] powerpc/pci: Remove __pci_mmap_set_pgprot()

[Bjorn Helgaas]

On Thu, Jun 09, 2016 at 01:20:23PM -0500, Bjorn Helgaas wrote:
> From: Yinghai Lu <yinghai@kernel.org>
> 
> The powerpc-specific __pci_mmap_set_pgprot() does two things:
> 
>   1) Disables write combining for I/O port space mappings
> 
>      This only affects procfs mappings.  The pci_mmap_resource() sysfs path
>      only requests write combining for resources with IORESOURCE_PREFETCH
>      set, which doesn't include I/O resources.
> 
>      The only way to request write combining for I/O port space mappings
>      was via the PCIIOC_WRITE_COMBINE ioctl and the proc_bus_pci_mmap()
>      path, and we recently changed that path to ignore write combining for
>      I/O, so this code in powerpc is no longer needed.
> 
>   2) Automatically enables write combining for mappings of prefetchable
>      resources, even if not requested by the user
> 
>      Both procfs (via PCIIOC_MMAP_IS_MEM and PCIIOC_WRITE_COMBINE ioctls)
>      and sysfs (via "resourceN_wc" files, which are created for resources
>      with IORESOURCE_PREFETCH) provide ways for the user to map PCI memory
>      space with write combining.
> 
>      Users that desire write combining should use one of those ways instead
>      of relying on powerpc-specific behavior.
> 
> Remove the powerpc-specific __pci_mmap_set_pgprot().
> 
> The user-visible effect of this change is that users mapping prefetchable
> PCI memory space via procfs without PCIIOC_WRITE_COMBINE or via sysfs
> "resourceN" (not "resourceN_wc") will get regular uncacheable mappings
> instead of the write combining mappings they used to get.
> 
> The new behavior matches the behavior on all other arches that support
> write combining mapping.

Powerpc folks, any thoughts on this?

It's currently on my pci/resource branch, and I plan to merge it for
v4.8 if there are no objections.

> [bhelgaas: changelog]
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  arch/powerpc/kernel/pci-common.c |   37 ++++---------------------------------
>  1 file changed, 4 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 0f7a60f..8c6beb0 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -356,36 +356,6 @@ static struct resource *__pci_mmap_make_offset(struct pci_dev *dev,
>  }
>  
>  /*
> - * Set vm_page_prot of VMA, as appropriate for this architecture, for a pci
> - * device mapping.
> - */
> -static pgprot_t __pci_mmap_set_pgprot(struct pci_dev *dev, struct resource *rp,
> -				      pgprot_t protection,
> -				      enum pci_mmap_state mmap_state,
> -				      int write_combine)
> -{
> -
> -	/* Write combine is always 0 on non-memory space mappings. On
> -	 * memory space, if the user didn't pass 1, we check for a
> -	 * "prefetchable" resource. This is a bit hackish, but we use
> -	 * this to workaround the inability of /sysfs to provide a write
> -	 * combine bit
> -	 */
> -	if (mmap_state != pci_mmap_mem)
> -		write_combine = 0;
> -	else if (write_combine == 0) {
> -		if (rp->flags & IORESOURCE_PREFETCH)
> -			write_combine = 1;
> -	}
> -
> -	/* XXX would be nice to have a way to ask for write-through */
> -	if (write_combine)
> -		return pgprot_noncached_wc(protection);
> -	else
> -		return pgprot_noncached(protection);
> -}
> -
> -/*
>   * This one is used by /dev/mem and fbdev who have no clue about the
>   * PCI device, it tries to find the PCI device first and calls the
>   * above routine
> @@ -458,9 +428,10 @@ int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
>  		return -EINVAL;
>  
>  	vma->vm_pgoff = offset >> PAGE_SHIFT;
> -	vma->vm_page_prot = __pci_mmap_set_pgprot(dev, rp,
> -						  vma->vm_page_prot,
> -						  mmap_state, write_combine);
> +	if (write_combine)
> +		vma->vm_page_prot = pgprot_noncached_wc(vma->vm_page_prot);
> +	else
> +		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>  
>  	ret = remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
>  			       vma->vm_end - vma->vm_start, vma->vm_page_prot);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 3/3] microblaze/PCI: Remove useless __pci_mmap_set_pgprot()

[Bjorn Helgaas]

The microblaze __pci_mmap_set_pgprot() was apparently copied from powerpc,
where it computes either an uncacheable pgprot_t or a write-combining one.
But on microblaze, we always use the regular uncacheable pgprot_t.

Remove the useless code in __pci_mmap_set_pgprot() and inline the
pgprot_noncached() at the only caller.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Yinghai Lu <yinghai@kernel.org>
---
 arch/microblaze/pci/pci-common.c |   31 +------------------------------
 1 file changed, 1 insertion(+), 30 deletions(-)

diff --git a/arch/microblaze/pci/pci-common.c b/arch/microblaze/pci/pci-common.c
index 14cba60..1974567 100644
--- a/arch/microblaze/pci/pci-common.c
+++ b/arch/microblaze/pci/pci-common.c
@@ -219,33 +219,6 @@ static struct resource *__pci_mmap_make_offset(struct pci_dev *dev,
 }
 
 /*
- * Set vm_page_prot of VMA, as appropriate for this architecture, for a pci
- * device mapping.
- */
-static pgprot_t __pci_mmap_set_pgprot(struct pci_dev *dev, struct resource *rp,
-				      pgprot_t protection,
-				      enum pci_mmap_state mmap_state,
-				      int write_combine)
-{
-	pgprot_t prot = protection;
-
-	/* Write combine is always 0 on non-memory space mappings. On
-	 * memory space, if the user didn't pass 1, we check for a
-	 * "prefetchable" resource. This is a bit hackish, but we use
-	 * this to workaround the inability of /sysfs to provide a write
-	 * combine bit
-	 */
-	if (mmap_state != pci_mmap_mem)
-		write_combine = 0;
-	else if (write_combine == 0) {
-		if (rp->flags & IORESOURCE_PREFETCH)
-			write_combine = 1;
-	}
-
-	return pgprot_noncached(prot);
-}
-
-/*
  * This one is used by /dev/mem and fbdev who have no clue about the
  * PCI device, it tries to find the PCI device first and calls the
  * above routine
@@ -317,9 +290,7 @@ int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
 		return -EINVAL;
 
 	vma->vm_pgoff = offset >> PAGE_SHIFT;
-	vma->vm_page_prot = __pci_mmap_set_pgprot(dev, rp,
-						  vma->vm_page_prot,
-						  mmap_state, write_combine);
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 
 	ret = remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
 			       vma->vm_end - vma->vm_start, vma->vm_page_prot);