| Message ID | 1489598266.86622.12.camel@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, Mar 15, 2017 at 05:17:46PM +0000, David Woodhouse wrote: > From: Brijesh Singh <brijess@amazon.com> > > To support pci resource mapping from userspace, pci_mmap_page_range > implementation must be done for that platform. This support was > broken for arm64. > > This patch copies existing implementation from arm to > enable sysfs mmap. It's not so much "broken" as "not currently supported". [...] > +#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); > + Per the prior attempt at this [1], we only want to expose the sysfs interface, and not the legacy procfs interface, and need the two decoupled [2]. ... or has something changed in the mean time, so that this only exposes the sysfs interface? [...] > +int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma, > + enum pci_mmap_state mmap_state, int write_combine) > +{ > + 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); ... as per Will's comment in [3], the latter of these should use pgprot_device. Thanks, Mark. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/421948.html [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/423083.html [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422571.html
On Wed, 2017-03-15 at 17:54 +0000, Mark Rutland wrote: > It's not so much "broken" as "not currently supported". Yeah, I thought that when I inherited the commit, but didn't get as far as rephrasing it. Will do so. > [...] > > > > > +#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); > > + > Per the prior attempt at this [1], we only want to expose the sysfs > interface, and not the legacy procfs interface, and need the two > decoupled [2]. I do not like that idea. The procfs horridness is legacy, sure, but it's not actually an arch-specific interface. You get to mess with 'legacy' syscalls all you like on a new architecture, but there could exist arch-agnostic code which uses the procfs interface, surely? > > + if (write_combine) > > + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > > + else > > + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > ... as per Will's comment in [3], the latter of these should use > pgprot_device. Will fix that; thanks.
On Wed, Mar 15, 2017 at 07:18:16PM +0000, David Woodhouse wrote: > On Wed, 2017-03-15 at 17:54 +0000, Mark Rutland wrote: > > It's not so much "broken" as "not currently supported". > > Yeah, I thought that when I inherited the commit, but didn't get as far > as rephrasing it. Will do so. > > > [...] > > > > > > > > +#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); > > > + > > Per the prior attempt at this [1], we only want to expose the sysfs > > interface, and not the legacy procfs interface, and need the two > > decoupled [2]. > > I do not like that idea. The procfs horridness is legacy, sure, but > it's not actually an arch-specific interface. You get to mess with > 'legacy' syscalls all you like on a new architecture, but there could > exist arch-agnostic code which uses the procfs interface, surely? Just to be clear here: I'm not against exposing the proc interface if something actually needs it, but all the requests we've had for this have been concerned only with the sysfs API. So I'd rather start with just that, instead of exposing both and have new software written to the proc interface, which we certainly want to discourage. Will
On Mon, 2017-03-20 at 10:24 +0000, Will Deacon wrote: > > Just to be clear here: I'm not against exposing the proc interface if > something actually needs it, but all the requests we've had for this have > been concerned only with the sysfs API. So I'd rather start with just that, > instead of exposing both and have new software written to the proc interface, > which we certainly want to discourage. The point is that they are tied together. An architecture provides its pci_mmap_page_range() function and sets HAVE_PCI_MMAP, and then the *generic* code in drivers/pci will use it from both places. Although as noted, there is a third userspace API in drivers/vfio which does this *without* any arch-specific code (using pgprot_noncached(), and failing to set up the VMA correctly on HAVE_IOREMAP_PROT platforms. I was looking at cleaning that whole mess up, but got distracted by kdump-on-CPU-1 failures. If I'm going to blame that on firmware, I'll get back to PCI mmap this week... :)
diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h index b9a7ba9..f73734f 100644 --- a/arch/arm64/include/asm/pci.h +++ b/arch/arm64/include/asm/pci.h @@ -37,5 +37,9 @@ 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 4f0e3eb..a9b6542 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -228,3 +228,22 @@ void pcibios_remove_bus(struct pci_bus *bus) } #endif + +int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma, + enum pci_mmap_state mmap_state, int write_combine) +{ + 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); + + if (remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, + vma->vm_end - vma->vm_start, + vma->vm_page_prot)) + return -EAGAIN; + + return 0; +}