[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[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 <jerin.jacob@caviumnetworks.com>
---
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(+)

diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
index b9a7ba9..9d7e460 100644
--- a/arch/arm64/include/asm/pci.h
+++ b/arch/arm64/include/asm/pci.h
@@ -37,5 +37,11 @@ static inline int pci_proc_domain(struct pci_bus *bus)
 }
 #endif  /* CONFIG_PCI */
 
+#define HAVE_PCI_MMAP
+
+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 */
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index c72de66..be7ddf1 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -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)
+		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);
+
+	return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
+	       vma->vm_end - vma->vm_start, vma->vm_page_prot);
+}
-- 
2.1.0

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Will Deacon]

On Thu, Apr 14, 2016 at 02:40:56AM +0530, Jerin Jacob wrote:
> 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 <jerin.jacob@caviumnetworks.com>
> ---
> 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(+)
> 
> diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> index b9a7ba9..9d7e460 100644
> --- a/arch/arm64/include/asm/pci.h
> +++ b/arch/arm64/include/asm/pci.h
> @@ -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.

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

> +
> +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 */
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index c72de66..be7ddf1 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -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)
> +		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

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Arnd Bergmann]

On Friday 15 April 2016 14:09:53 Will Deacon wrote:
> On Thu, Apr 14, 2016 at 02:40:56AM +0530, Jerin Jacob wrote:
> > 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 <jerin.jacob@caviumnetworks.com>
> > ---
> > 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(+)
> > 
> > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> > index b9a7ba9..9d7e460 100644
> > --- a/arch/arm64/include/asm/pci.h
> > +++ b/arch/arm64/include/asm/pci.h
> > @@ -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.
> 
> 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

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Jerin Jacob]

On Fri, Apr 15, 2016 at 02:09:53PM +0100, Will Deacon wrote:
> On Thu, Apr 14, 2016 at 02:40:56AM +0530, Jerin Jacob wrote:
> > 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 <jerin.jacob@caviumnetworks.com>
> > ---
> > 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(+)
> > 
> > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> > index b9a7ba9..9d7e460 100644
> > --- a/arch/arm64/include/asm/pci.h
> > +++ b/arch/arm64/include/asm/pci.h
> > @@ -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.

AFAIK, /proc/bus/pci/* proc entry files will be created irrespective
of HAVE_PCI_MMAP, But mmap capability added only when HAVE_PCI_MMAP selected.

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

AFAIK, With existing generic code infrastructure its is not possible.
IMO, Introducing yet another kernel configuration or weak function to disable
in the /proc/ entries in the generic code may not be a good idea.


> > +
> > +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 */
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index c72de66..be7ddf1 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -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)
> > +		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 fix it in v3.

> 
> Will

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Jerin Jacob]

On Fri, Apr 15, 2016 at 08:45:20PM +0200, Arnd Bergmann wrote:
> On Friday 15 April 2016 14:09:53 Will Deacon wrote:
> > On Thu, Apr 14, 2016 at 02:40:56AM +0530, Jerin Jacob wrote:
> > > 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 <jerin.jacob@caviumnetworks.com>
> > > ---
> > > 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(+)
> > > 
> > > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> > > index b9a7ba9..9d7e460 100644
> > > --- a/arch/arm64/include/asm/pci.h
> > > +++ b/arch/arm64/include/asm/pci.h
> > > @@ -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.
> > 
> > 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.

Yes, after the introduction of vfio-noiommu mode almost all
uses case can be addressed with vfio. But still their are userspace
applications uses /sysfs scheme.

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.

Jerin

> 
> 	Arnd

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Arnd Bergmann]

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

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Jerin Jacob]

On Mon, Apr 18, 2016 at 04:15:28PM +0200, Arnd Bergmann wrote:
> 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.

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.

> 
> 	Arnd

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Arnd Bergmann]

On Monday 18 April 2016 20:23:49 Jerin Jacob wrote:
> On Mon, Apr 18, 2016 at 04:15:28PM +0200, Arnd Bergmann wrote:
> > 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.
> 
> 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

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Jerin Jacob]

On Mon, Apr 18, 2016 at 05:00:49PM +0200, Arnd Bergmann wrote:
> On Monday 18 April 2016 20:23:49 Jerin Jacob wrote:
> > On Mon, Apr 18, 2016 at 04:15:28PM +0200, Arnd Bergmann wrote:
> > > 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.
> > 
> > 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.

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.

Jerin

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

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Arnd Bergmann]

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.

	Arnd

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Will Deacon]

On Mon, Apr 18, 2016 at 05:31:14PM +0200, Arnd Bergmann wrote:
> 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.

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

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Jerin Jacob]

On Mon, Apr 18, 2016 at 04:40:14PM +0100, Will Deacon wrote:
> On Mon, Apr 18, 2016 at 05:31:14PM +0200, Arnd Bergmann wrote:
> > 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.
> 
> 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?

> 
> Will

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Arnd Bergmann]

On Monday 18 April 2016 23:15:36 Jerin Jacob wrote:
> On Mon, Apr 18, 2016 at 04:40:14PM +0100, Will Deacon wrote:
> > On Mon, Apr 18, 2016 at 05:31:14PM +0200, Arnd Bergmann wrote:
> > > 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.
> > 
> > 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?
> 

Flag yes, weak function no.

	Arnd

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[David Woodhouse]

On Mon, 2016-04-18 at 17:31 +0200, Arnd Bergmann wrote:
> 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.

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 4938 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170315/6a3ba016/attachment-0001.bin>

Re: [PATCH v2] arm64: pci: add support for pci_mmap_page_range

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 2021 bytes --]

On Fri, 2016-04-15 at 14:09 +0100, Will Deacon wrote:
> 
> > +     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...)


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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[David Woodhouse]

On Fri, 2016-04-15 at 14:09 +0100, Will Deacon wrote:
> 
> > +?????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...)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 4938 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170316/f016edd6/attachment.bin>

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Will Deacon]

On Wed, Mar 15, 2017 at 09:01:36PM +0000, David Woodhouse wrote:
> On Mon, 2016-04-18 at 17:31 +0200, Arnd Bergmann wrote:
> > 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.
> 
> 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.

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.

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

Will

Re: [PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Will Deacon]

On Thu, Mar 16, 2017 at 12:17:27PM +0000, David Woodhouse wrote:
> On Fri, 2016-04-15 at 14:09 +0100, Will Deacon wrote:
> > 
> > > +     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?

The different between ioremap (which used pgprot_device) and a mapping
created using pgprot_noncached is that the former allows for early
acknowledgement of writes (e.g. at a bridge). See this recent series from
Lorenzo that is also trying to clean this up:

http://lkml.kernel.org/r/20170227151436.18698-1-lorenzo.pieralisi@arm.com

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

I think so. At least, on arm64, pgprot_noncached is only really needed
for PCI config space and "I don't know that this is, but I'm going to map
it anyway" regions in /dev/mem.

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

Happy to review patches :)

Will

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[Will Deacon]

On Thu, Mar 16, 2017 at 12:17:27PM +0000, David Woodhouse wrote:
> On Fri, 2016-04-15 at 14:09 +0100, Will Deacon wrote:
> > 
> > > +?????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?

The different between ioremap (which used pgprot_device) and a mapping
created using pgprot_noncached is that the former allows for early
acknowledgement of writes (e.g. at a bridge). See this recent series from
Lorenzo that is also trying to clean this up:

http://lkml.kernel.org/r/20170227151436.18698-1-lorenzo.pieralisi at arm.com

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

I think so. At least, on arm64, pgprot_noncached is only really needed
for PCI config space and "I don't know that this is, but I'm going to map
it anyway" regions in /dev/mem.

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

Happy to review patches :)

Will

Re: [PATCH v2] arm64: pci: add support for pci_mmap_page_range

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1703 bytes --]

On Mon, 2017-03-20 at 13:18 +0000, Will Deacon wrote:
> > 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.

I think that's just a bug. I'll add it to my list. We shouldn't be
allowing a WC mapping on a non-prefetchable resource, should we?

For that matter, I think it allows you mmap a range of MMIO addresses
that correspond to an I/O BAR, and on platforms which allow
pci_mmap_io, the converse. That seems... suboptimal.
 
> > 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.

Sure, but fairly much orthogonal. I'll roll it in. It's fairly much in
the noise now I'm this far down the rabbithole...

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2] arm64: pci: add support for pci_mmap_page_range

[David Woodhouse]

On Mon, 2017-03-20 at 13:18 +0000, Will Deacon wrote:
> > 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.

I think that's just a bug. I'll add it to my list. We shouldn't be
allowing a WC mapping on a non-prefetchable resource, should we?

For that matter, I think it allows you mmap a range of MMIO addresses
that correspond to an I/O BAR, and on platforms which allow
pci_mmap_io, the converse. That seems... suboptimal.
?
> > 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.

Sure, but fairly much orthogonal. I'll roll it in. It's fairly much in
the noise now I'm this far down the rabbithole...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 4938 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170320/fb220219/attachment.bin>