arm64: pci: add support for pci_mmap_page_range

Certain X11 servers and user space network drivers frameworks need PCI mmaped /sys/bus/pci/devices/B:D:F/resourceX file to access PCI bar address space from user space.

Signed-off-by Jerin Jacob <[email protected]>

---
Changes in v2:
- Rebased to 4.6.0-rc3.
- Tested and verified the change on Thunderx and xgene1 arm64 platforms

 arch/arm64/include/asm/pci.h |  6 ++++++
 arch/arm64/kernel/pci.c      | 20 ++++++++++++++++++++
 2 files changed, 26 insertions(+)

arch/arm64/include/asm/pci.h +0 -0

index b9a7ba9..9d7e460 100644

arch/arm64/include/asm/pci.h +5 -0

+
+extern int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
+	enum pci_mmap_state mmap_state, int write_combine);
+
+
 #endif  /* __KERNEL__ */
 #endif  /* __ASM_PCI_H */

arch/arm64/kernel/pci.c +6 -0

index c72de66..be7ddf1 100644
@@ -82,3 +82,23 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 	return NULL;
 }
 #endif
+
+int pci_mmap_page_range(struct pci_dev *dev, 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

Quoted 22 lines · Jerin Jacob

Certain X11 servers and user space network drivers frameworks need PCI mmaped /sys/bus/pci/devices/B:D:F/resourceX file to access PCI bar address space from user space.

Signed-off-by Jerin Jacob <[email protected]>

---
Changes in v2:
- Rebased to 4.6.0-rc3.
- Tested and verified the change on Thunderx and xgene1 arm64 platforms

 arch/arm64/include/asm/pci.h |  6 ++++++
 arch/arm64/kernel/pci.c      | 20 ++++++++++++++++++++
 2 files changed, 26 insertions(+)

arch/arm64/include/asm/pci.h +0 -0

index b9a7ba9..9d7e460 100644
@@ -37,5 +37,11 @@ static inline int pci_proc_domain(struct pci_bus *bus)
 }
 #endif  /* CONFIG_PCI */
 

arch/arm64/include/asm/pci.h static inline int pci_proc_domain(struct pci_bus *bus) · line 40

Quoted 1 line

arch/arm64/include/asm/pci.h +1 -0

+#define HAVE_PCI_MMAP

By defining this symbol, we also get lumbered with the legacy /proc interface, which I'd be keen to avoid exposing until we have people explicitly asking for it.

Any chance you could expose only the /sysfs interface on arm64?

Quoted 29 lines · On Friday 15 April 2016 14:09:53 Will Deacon wrote:

Quoted 19 lines · Jerin Jacob

Certain X11 servers and user space network drivers frameworks need PCI mmaped /sys/bus/pci/devices/B:D:F/resourceX file to access PCI bar address space from user space.

Signed-off-by Jerin Jacob <[email protected]>

---
Changes in v2:
- Rebased to 4.6.0-rc3.
- Tested and verified the change on Thunderx and xgene1 arm64 platforms

 arch/arm64/include/asm/pci.h |  6 ++++++
 arch/arm64/kernel/pci.c      | 20 ++++++++++++++++++++
 2 files changed, 26 insertions(+)

arch/arm64/include/asm/pci.h +0 -0

index b9a7ba9..9d7e460 100644

arch/arm64/include/asm/pci.h static inline int pci_proc_domain(struct pci_bus *bus) · line 40

Quoted 4 lines

arch/arm64/include/asm/pci.h +1 -0

@@ -37,5 +37,11 @@ static inline int pci_proc_domain(struct pci_bus *bus)
 }
 #endif  /* CONFIG_PCI */
 
+#define HAVE_PCI_MMAP

By defining this symbol, we also get lumbered with the legacy /proc interface, which I'd be keen to avoid exposing until we have people explicitly asking for it.

Quoted 2 lines

Any chance you could expose only the /sysfs interface on arm64?

Do we have an idea how much user space code is affected by this? Maybe it can instead be changed to use VFIO, which is basically the current way to do this.

Arnd

Regarding existing user space applications,
AFAIK, DPDK has the feature to support both /sysfs and vifo scheme. X11 uses only /sysfs scheme.

IMO, Nothing wrong in providing this feature in arm64 kernel. Except arm64, almost all the major architecture has this support.

Quoted 6 lines · On Monday 18 April 2016 19:31:20 Jerin Jacob wrote:

Regarding existing user space applications,
AFAIK, DPDK has the feature to support both /sysfs and vifo scheme. X11 uses only /sysfs scheme.

IMO, Nothing wrong in providing this feature in arm64 kernel. Except arm64, almost all the major architecture has this support.

My understanding was that it's considered deprecated and only supported for backwards compatibility, but now I can't find any indication of that in the source code and I don't know if that is actually the case.

I agree with Will that we should not expose the procfs interface, it's just far too ugly.

Arnd

Quoted 13 lines · Arnd Bergmann

Quoted 6 lines · On Monday 18 April 2016 19:31:20 Jerin Jacob wrote:

Regarding existing user space applications,
AFAIK, DPDK has the feature to support both /sysfs and vifo scheme. X11 uses only /sysfs scheme.

IMO, Nothing wrong in providing this feature in arm64 kernel. Except arm64, almost all the major architecture has this support.

My understanding was that it's considered deprecated and only supported for backwards compatibility, but now I can't find any indication of that in the source code and I don't know if that is actually the case.

Quoted 2 lines

I agree with Will that we should not expose the procfs interface, it's just far too ugly.

Me too agree with Will and I don't like it either.
My point was, Irrespective of this change, the /proc/bus/pci/*/* entries will be created. i.e disabling /proc/bus/pci should be a seprate patch and it does not depend on this patch.

Quoted 17 lines · On Monday 18 April 2016 20:23:49 Jerin Jacob wrote:

Quoted 15 lines · Arnd Bergmann

Quoted 6 lines · On Monday 18 April 2016 19:31:20 Jerin Jacob wrote:

Regarding existing user space applications,
AFAIK, DPDK has the feature to support both /sysfs and vifo scheme. X11 uses only /sysfs scheme.

IMO, Nothing wrong in providing this feature in arm64 kernel. Except arm64, almost all the major architecture has this support.

My understanding was that it's considered deprecated and only supported for backwards compatibility, but now I can't find any indication of that in the source code and I don't know if that is actually the case.

I agree with Will that we should not expose the procfs interface, it's just far too ugly.

Quoted 4 lines

Me too agree with Will and I don't like it either.
My point was, Irrespective of this change, the /proc/bus/pci/*/* entries will be created. i.e disabling /proc/bus/pci should be a seprate patch and it does not depend on this patch.

The problem is that once we allow mmap() on proc/bus/pci/*/*, it becomes much harder to prove that we are able to remove it again without breaking stuff that worked.

We have to decouple the sysfs interface from the procfs interface before we allow the former.

Arnd

Why only to disable mmap() serivce in proc/bus/pci/*/*. Why not other services offered though proc/bus/pci/ like config space read, /proc/bus/pci/devices etc

if a given platform not interested in proc fs then disable through CONFIG_PROC_FS in defconfig. I don't understand the logic behind disabling partial services that proc fs exposes.

Quoted 8 lines · On Monday 18 April 2016 20:51:27 Jerin Jacob wrote:

Why only to disable mmap() serivce in proc/bus/pci/*/*. Why not other services offered though proc/bus/pci/ like config space read, /proc/bus/pci/devices etc

if a given platform not interested in proc fs then disable through CONFIG_PROC_FS in defconfig. I don't understand the logic behind disabling partial services that proc fs exposes.

Disabling CONFIG_PROC_FS is not really an option for anybody.

The config space access may be something we should have disabled, or it may not be, but I think it's too late to kill that off now, as that would likely break something.

The mmap() support on those files is way uglier than the config access, so as long as nobody absolutely requires it, we should not add it to the list of things we can't get rid of again.

Quoted 16 lines · Arnd Bergmann

Quoted 8 lines · On Monday 18 April 2016 20:51:27 Jerin Jacob wrote:

Why only to disable mmap() serivce in proc/bus/pci/*/*. Why not other services offered though proc/bus/pci/ like config space read, /proc/bus/pci/devices etc

if a given platform not interested in proc fs then disable through CONFIG_PROC_FS in defconfig. I don't understand the logic behind disabling partial services that proc fs exposes.

Disabling CONFIG_PROC_FS is not really an option for anybody.

The config space access may be something we should have disabled, or it may not be, but I think it's too late to kill that off now, as that would likely break something.

Quoted 3 lines

The mmap() support on those files is way uglier than the config access, so as long as nobody absolutely requires it, we should not add it to the list of things we can't get rid of again.

Completely agreed. IIRC, there's some unspeakable ioctl() magic to configure the memory type that the BARs are mapped with via the /proc interface and I *really* don't want that on arm64.

Will

Quoted 21 lines · Will Deacon

Quoted 19 lines · Arnd Bergmann

Quoted 8 lines · On Monday 18 April 2016 20:51:27 Jerin Jacob wrote:

Why only to disable mmap() serivce in proc/bus/pci/*/*. Why not other services offered though proc/bus/pci/ like config space read, /proc/bus/pci/devices etc

if a given platform not interested in proc fs then disable through CONFIG_PROC_FS in defconfig. I don't understand the logic behind disabling partial services that proc fs exposes.

Disabling CONFIG_PROC_FS is not really an option for anybody.

The config space access may be something we should have disabled, or it may not be, but I think it's too late to kill that off now, as that would likely break something.

The mmap() support on those files is way uglier than the config access, so as long as nobody absolutely requires it, we should not add it to the list of things we can't get rid of again.

Quoted 3 lines

Completely agreed. IIRC, there's some unspeakable ioctl() magic to configure the memory type that the BARs are mapped with via the /proc interface and I *really* don't want that on arm64.

But do you think introducing a conditional compilation flag or weak function or arch specific function to disable pci proc fs mmap support in the generic code will be acceptable?

Quoted 10 lines · On Monday 18 April 2016 20:51:27 Jerin Jacob wrote:

Why only to disable mmap() serivce in proc/bus/pci/*/*. Why not other??services offered though proc/bus/pci/ like config space read,
/proc/bus/pci/devices etc

if a given platform not interested in proc fs??then disable through CONFIG_PROC_FS in defconfig. I don't understand the logic behind disabling partial services that proc fs exposes.

Disabling CONFIG_PROC_FS is not really an option for anybody.

The config space access may be something we should have disabled, or it may not be, but I think it's too late to kill that off now, as that would likely break something.

The mmap() support on those files is way uglier than the config access, so as long as nobody absolutely requires it, we should not add it to the list of things we can't get rid of again.

Quoted 13 lines · Arnd Bergmann

Quoted 10 lines · On Monday 18 April 2016 20:51:27 Jerin Jacob wrote:

Why only to disable mmap() serivce in proc/bus/pci/*/*. Why not other??services offered though proc/bus/pci/ like config space read,
/proc/bus/pci/devices etc

if a given platform not interested in proc fs??then disable through CONFIG_PROC_FS in defconfig. I don't understand the logic behind disabling partial services that proc fs exposes.

Disabling CONFIG_PROC_FS is not really an option for anybody.

Quoted 7 lines

The config space access may be something we should have disabled, or it may not be, but I think it's too late to kill that off now, as that would likely break something.

The mmap() support on those files is way uglier than the config access, so as long as nobody absolutely requires it, we should not add it to the list of things we can't get rid of again.

I don't think we should be doing this. You may have guessed that from my... less than sympathetic... implementation of it in the patch I just sent.

The thing is, this *isn't* an architecture-specific interface where you get a clean slate. It's a cross-platform interface. Legacy and horrid, sure. But it does you no harm.

What *else* don't you like having in /proc? Shall we have a clean slate and eliminate *everything* other than actual processes from /proc for the next architecture we add to the tree? If not, why not?

Quoted 3 lines

The thing is, this *isn't* an architecture-specific interface where you get a clean slate. It's a cross-platform interface. Legacy and horrid, sure. But it does you no harm.

I don't agree with that: it provides (privileged) userspace with a way to map non-prefetchable BARs using write-combining memory attributes, which could lead to mismatched memory attributes against a kernel mapping of the same BAR and is something that you can't achieve using the sysfs API.

Quoted 3 lines

What *else* don't you like having in /proc? Shall we have a clean slate and eliminate *everything* other than actual processes from /proc for the next architecture we add to the tree? If not, why not?

It's not about what we like and don't like in /proc, it's about not promoting legacy that we don't need. If somebody actually needs the /proc interface, fine, we can support it. But all the people asking for this have been concerned solely about the sysfs interface, so I'd just like the two divorced from each other so that we can provide what people are asking for without pulling in a deprecated interface at the same time.

This should be straightforward.

Quoted 22 lines · Jerin Jacob

Certain X11 servers and user space network drivers frameworks need PCI mmaped /sys/bus/pci/devices/B:D:F/resourceX file to access PCI bar address space from user space.

Signed-off-by Jerin Jacob <[email protected]>

---
Changes in v2:
- Rebased to 4.6.0-rc3.
- Tested and verified the change on Thunderx and xgene1 arm64 platforms

 arch/arm64/include/asm/pci.h |  6 ++++++
 arch/arm64/kernel/pci.c      | 20 ++++++++++++++++++++
 2 files changed, 26 insertions(+)

arch/arm64/include/asm/pci.h +0 -0

index b9a7ba9..9d7e460 100644
@@ -37,5 +37,11 @@ static inline int pci_proc_domain(struct pci_bus *bus)
 }
 #endif  /* CONFIG_PCI */
 

arch/arm64/include/asm/pci.h static inline int pci_proc_domain(struct pci_bus *bus) · line 40

Quoted 1 line

arch/arm64/include/asm/pci.h +1 -0

+#define HAVE_PCI_MMAP

By defining this symbol, we also get lumbered with the legacy /proc interface, which I'd be keen to avoid exposing until we have people explicitly asking for it.

Any chance you could expose only the /sysfs interface on arm64?

Quoted 25 lines

arch/arm64/include/asm/pci.h +5 -0

+
+extern int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
+	enum pci_mmap_state mmap_state, int write_combine);
+
+
 #endif  /* __KERNEL__ */
 #endif  /* __ASM_PCI_H */

arch/arm64/kernel/pci.c +10 -0

index c72de66..be7ddf1 100644
@@ -82,3 +82,23 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 	return NULL;
 }
 #endif
+
+int pci_mmap_page_range(struct pci_dev *dev, 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)

arch/arm64/kernel/pci.c struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) · line 100

Quoted 6 lines

arch/arm64/kernel/pci.c +6 -0

+		return -EINVAL;
+
+	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.

Will

Quoted 5 lines

+?????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.

Quoted 7 lines · Will Deacon

Quoted 5 lines

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