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

Groups > linux.kernel > #1385476 > unrolled thread

Re: [PATCH v11 04/60] sparc/PCI: Use correct offset for bus address to resource

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v11 04/60] sparc/PCI: Use correct offset for bus address  to resource Bjorn Helgaas <helgaas@kernel.org> - 2016-04-22 22:50 +0200
    Re: [PATCH v11 04/60] sparc/PCI: Use correct offset for bus address  to resource Yinghai Lu <yinghai@kernel.org> - 2016-04-28 07:00 +0200

#1385476 — Re: [PATCH v11 04/60] sparc/PCI: Use correct offset for bus address to resource

[+cc Ben, Michael]

On Thu, Apr 07, 2016 at 05:15:17PM -0700, Yinghai Lu wrote:
> After we added 64bit mmio parsing, we got some "no compatible bridge window"
> warning on anther new model that support 64bit resource.
> 
> It turns out that we can not use mem_space.start as 64bit mem space
> offset, aka there is mem_space.start != offset.
> 
> Use child_phys_addr to calculate exact offset and record offset in
> pbm.
> 
> After patch we get correct offset.
> 
> /pci@305: PCI IO [io  0x2007e00000000-0x2007e0fffffff] offset 2007e00000000
> /pci@305: PCI MEM [mem 0x2000000100000-0x200007effffff] offset 2000000000000
> /pci@305: PCI MEM64 [mem 0x2000100000000-0x2000dffffffff] offset 2000000000000
> ...
> pci_sun4v f02ae7f8: PCI host bridge to bus 0000:00
> pci_bus 0000:00: root bus resource [io  0x2007e00000000-0x2007e0fffffff] (bus address [0x0000-0xfffffff])
> pci_bus 0000:00: root bus resource [mem 0x2000000100000-0x200007effffff] (bus address [0x00100000-0x7effffff])
> pci_bus 0000:00: root bus resource [mem 0x2000100000000-0x2000dffffffff] (bus address [0x100000000-0xdffffffff])
> ...

> @@ -733,30 +733,32 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
>  static int __pci_mmap_make_offset_bus(struct pci_dev *pdev, struct vm_area_struct *vma,
>  				      enum pci_mmap_state mmap_state)
>  {
> -	struct pci_pbm_info *pbm = pdev->dev.archdata.host_controller;
> -	unsigned long space_size, user_offset, user_size;
> -
> -	if (mmap_state == pci_mmap_io) {
> -		space_size = resource_size(&pbm->io_space);
> -	} else {
> -		space_size = resource_size(&pbm->mem_space);
> -	}
> +	unsigned long user_offset, user_size;
> +	struct resource res, *root_bus_res;
> +	struct pci_bus_region region;
> +	struct pci_bus *bus;
>  
>  	/* Make sure the request is in range. */
>  	user_offset = vma->vm_pgoff << PAGE_SHIFT;
>  	user_size = vma->vm_end - vma->vm_start;
>  
> -	if (user_offset >= space_size ||
> -	    (user_offset + user_size) > space_size)
> +	region.start = user_offset;
> +	region.end = user_offset + user_size - 1;
> +	memset(&res, 0, sizeof(res));
> +	if (mmap_state == pci_mmap_io)
> +		res.flags = IORESOURCE_IO;
> +	else
> +		res.flags = IORESOURCE_MEM;
> +
> +	pcibios_bus_to_resource(pdev->bus, &res, &region);
> +	bus = pdev->bus;
> +	while (bus->parent)
> +		bus = bus->parent;
> +	root_bus_res = pci_find_bus_resource(bus, &res);
> +	if (!root_bus_res)
>  		return -EINVAL;
>  
> -	if (mmap_state == pci_mmap_io) {
> -		vma->vm_pgoff = (pbm->io_space.start +
> -				 user_offset) >> PAGE_SHIFT;
> -	} else {
> -		vma->vm_pgoff = (pbm->mem_space.start +
> -				 user_offset) >> PAGE_SHIFT;
> -	}
> +	vma->vm_pgoff = res.start >> PAGE_SHIFT;
>  
>  	return 0;
>  }

I'm kind of confused here.  There are two ways to mmap PCI BARs:

  /proc/bus/pci/00/02.0 (proc_bus_pci_mmap()):
    all BARs in one file; MEM/IO determined by ioctl()
    mmap offset is a CPU physical address in the PCI resource

  /sys/devices/pci0000:00/0000:00:02.0/resource0 (pci_mmap_resource()):
    one file per BAR; MEM/IO determined by BAR type
    mmap offset is between 0 and BAR size

Both proc_bus_pci_mmap() and pci_mmap_resource() validate the
requested area with pci_mmap_fits() before calling pci_mmap_page_range().

In the proc_bus_pci_mmap() path, the offset in vma->vm_pgoff must be
within the pdev->resource[], so the user must be supplying a CPU
physical address (not an address obtained from pci_resource_to_user()).
That vma->vm_pgoff is passed unchanged to pci_mmap_page_range().

In the pci_mmap_resource() path, vma->vm_pgoff must be between 0 and
the BAR size.  Then we add in the pci_resource_to_user() information
before passing it to pci_mmap_page_range().  The comment in
pci_mmap_resource() says pci_mmap_page_range() expects a "user
visible" address, but I don't really believe that based on how
proc_bus_pci_mmap() works.

Do both proc_bus_pci_mmap() and pci_mmap_resource() work on sparc?
It looks like they call pci_mmap_page_range() with different
assumptions, so I don't see how they can both work.

In any case, pci_mmap_page_range() on sparc converts that
vma->vm_pgoff back to a CPU physical address, so there's an awful lot
of work going on in the pci_mmap_resource() path to convert the mmap
offset to a "user" address and then convert it back again.

There's also quite a bit of validation done in the arch code (in
__pci_mmap_make_offset() and __pci_mmap_make_offset_bus()) that looks
partly redundant with pci_mmap_fits() and not necessarily
arch-specific.

As far as I can see, pci_mmap_resource() doesn't need to have any
connection at all with pci_resource_to_user().  All it needs is the
pdev->resource[] (which has the CPU physical address of the BAR) and
vma->vm_pgoff (the offset into the BAR).

I don't think pci_mmap_resource() should call pci_resource_to_user(),
and I think pci_mmap_page_range() should expect a normal VMA that
contains a valid CPU PFN in vm_pgoff (or a valid CPU I/O port number,
in the case of an I/O port mmap).

The original pci_resource_to_user() was added for powerpc by
2311b1f2bbd3 ("[PATCH] PCI: fix-pci-mmap-on-ppc-and-ppc64.patch"),
but I couldn't find the linux-pci discussion it mentions.

> @@ -977,16 +979,12 @@ void pci_resource_to_user(const struct pci_dev *pdev, int bar,
>  			  const struct resource *rp, resource_size_t *start,
>  			  resource_size_t *end)
>  {
> -	struct pci_pbm_info *pbm = pdev->dev.archdata.host_controller;
> -	unsigned long offset;
> +	struct pci_bus_region region;
>  
> -	if (rp->flags & IORESOURCE_IO)
> -		offset = pbm->io_space.start;
> -	else
> -		offset = pbm->mem_space.start;
> +	pcibios_resource_to_bus(pdev->bus, &region, (struct resource *)rp);
>  
> -	*start = rp->start - offset;
> -	*end = rp->end - offset;
> +	*start = region.start;
> +	*end = region.end;
>  }

[toc] | [next] | [standalone]

#1389786

On Fri, Apr 22, 2016 at 1:49 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Ben, Michael]
> I'm kind of confused here.  There are two ways to mmap PCI BARs:
>
>   /proc/bus/pci/00/02.0 (proc_bus_pci_mmap()):
>     all BARs in one file; MEM/IO determined by ioctl()
>     mmap offset is a CPU physical address in the PCI resource
>
>   /sys/devices/pci0000:00/0000:00:02.0/resource0 (pci_mmap_resource()):
>     one file per BAR; MEM/IO determined by BAR type
>     mmap offset is between 0 and BAR size
>
> Both proc_bus_pci_mmap() and pci_mmap_resource() validate the
> requested area with pci_mmap_fits() before calling pci_mmap_page_range().
>
> In the proc_bus_pci_mmap() path, the offset in vma->vm_pgoff must be
> within the pdev->resource[], so the user must be supplying a CPU
> physical address (not an address obtained from pci_resource_to_user()).
> That vma->vm_pgoff is passed unchanged to pci_mmap_page_range().
>
> In the pci_mmap_resource() path, vma->vm_pgoff must be between 0 and
> the BAR size.  Then we add in the pci_resource_to_user() information
> before passing it to pci_mmap_page_range().  The comment in
> pci_mmap_resource() says pci_mmap_page_range() expects a "user
> visible" address, but I don't really believe that based on how
> proc_bus_pci_mmap() works.
>
> Do both proc_bus_pci_mmap() and pci_mmap_resource() work on sparc?
> It looks like they call pci_mmap_page_range() with different
> assumptions, so I don't see how they can both work.

for sysfs path: in pci_mmap_resource
        pci_resource_to_user(pdev, i, res, &start, &end);
        vma->vm_pgoff += start >> PAGE_SHIFT;
     then call pci_mmap_page_range()

the fit checking in pci_mmap_fits(),
        pci_start = (mmap_api == PCI_MMAP_PROCFS) ?
                        pci_resource_start(pdev, resno) >> PAGE_SHIFT : 0;
        if (start >= pci_start && start < pci_start + size &&
                        start + nr <= pci_start + size)

so proc fs assume resource_start for vm_pgoff ?

but current pci_mmap_page_range want to use bus address
start aka BAR value.

and we have

        /* pci_mmap_page_range() expects the same kind of entry as coming
         * from /proc/bus/pci/ which is a "user visible" value. If this is
         * different from the resource itself, arch will do necessary fixup.
         */

so we need to fix pci_mmap_fits(), please check if it is ok, will
submit it as separated one.

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index d319a9c..3768c6a 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -969,15 +969,20 @@ void pci_remove_legacy_files(struct pci_bus *b)
 int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vma,
                  enum pci_mmap_api mmap_api)
 {
-       unsigned long nr, start, size, pci_start;
+       unsigned long nr, start, size;
+       resource_size_t pci_start = 0, pci_end;

        if (pci_resource_len(pdev, resno) == 0)
                return 0;
        nr = vma_pages(vma);
        start = vma->vm_pgoff;
        size = ((pci_resource_len(pdev, resno) - 1) >> PAGE_SHIFT) + 1;
-       pci_start = (mmap_api == PCI_MMAP_PROCFS) ?
-                       pci_resource_start(pdev, resno) >> PAGE_SHIFT : 0;
+       if (mmap_api == PCI_MMAP_PROCFS) {
+               struct resource *res = &pdev->resource[resno];
+
+               pci_resource_to_user(pdev, resno, res, &pci_start, &pci_end);
+               pci_start >>= PAGE_SHIFT;
+       }
        if (start >= pci_start && start < pci_start + size &&
                        start + nr <= pci_start + size)
                return 1;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web