Re: [PATCH 00/17] PCI resource mmap cleanup

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 6256 bytes --]

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
> 

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]