| Message ID | 1460581856-12380-1-git-send-email-jerin.jacob@caviumnetworks.com |
|---|---|
| State | New |
| Headers | show |
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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); +}
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(+) -- 2.1.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel