Hi all,
I am working on a personal project to do with real time drivers for data acquisition cards (Comedi sort of thing) and I'd appreciate some input on this.
Sometimes (actually often) the length of a control register region mapped to a PCI BAR presented by PCI devices is not page aligned.
The UIO framework is a powerful tool to quickly implement support for these sort of devices; in fact often porting complex PCI/FGPA drivers (ie image processing silicon with hundreds of registers, internal buses and interrupt sources) from other RTOSes (VxWorks, pSOS etc) to Linux can be simplified by using this framework or something similar (I have been doing that for years)
So the question is, why does the UIO core imposes the need to map registers on page aligned quantities - ie, if the length of the PCI BAR region (a very typical use case) is not a multiple of the PAGE_SIZE you have to provide *your own mmap* call instead of simply using the one implemented in the uio core.
static int uio_mmap(struct file *filep, struct vm_area_struct *vma) { struct uio_listener *listener = filep->private_data; struct uio_device *idev = listener->dev; int mi; unsigned long requested_pages, actual_pages; int ret = 0;
if (vma->vm_end < vma->vm_start) return -EINVAL;
vma->vm_private_data = idev;
mi = uio_find_mem_index(vma); if (mi < 0) return -EINVAL;
requested_pages = vma_pages(vma); actual_pages = ((idev->info->mem[mi].addr & ~PAGE_MASK) + idev->info->mem[mi].size + PAGE_SIZE -1) >> PAGE_SHIFT; if (requested_pages > actual_pages) return -EINVAL;
if (idev->info->mmap) { ret = idev->info->mmap(idev->info, vma); return ret; }
switch (idev->info->mem[mi].memtype) { case UIO_MEM_PHYS: return uio_mmap_physical(vma); case UIO_MEM_LOGICAL: case UIO_MEM_VIRTUAL: return uio_mmap_logical(vma); default: return -EINVAL; } }
Why just not allow a user space driver (a driver after all) to map the necessary bytes beyond the length of the BAR to match the expected alignment? After all user land driver accesses to the invalid region (there should not be any!) shall only cause bus errors.
With this information in mind, does the patch below sounds as insane as it looks? Many companies are actually launching with this on their products in one way or another (for instance just reporting the BAR length to UIO as ALIGN(mem->size, PAGE_SIZE) so the test below doesn't fail.
Note: for those without the background in the patch below
* mem->size: length of the region in bytes (ie PCI BAR0) * vma->vm_end - vma->vma_start (page aligned length of the region in bytes)
[jramirez@calypso ~ (xenomai *$)]$ git diff uio.c diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c index 60fa627..d225cd9 100644 --- a/drivers/uio/uio.c +++ b/drivers/uio/uio.c @@ -643,7 +643,7 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
if (mem->addr & ~PAGE_MASK) return -ENODEV; - if (vma->vm_end - vma->vm_start > mem->size) + if (vma->vm_end - vma->vm_start > ALIGN(mem->size, PAGE_SIZE)) return -EINVAL;
vma->vm_ops = &uio_physical_vm_ops;
TIA, jorge
On 05/01/2015 03:51 PM, Jorge Ramirez-Ortiz wrote:
Hi all,
I am working on a personal project to do with real time drivers for data acquisition cards (Comedi sort of thing) and I'd appreciate some input on this.
Sometimes (actually often) the length of a control register region mapped to a PCI BAR presented by PCI devices is not page aligned.
The UIO framework is a powerful tool to quickly implement support for these sort of devices; in fact often porting complex PCI/FGPA drivers (ie image processing silicon with hundreds of registers, internal buses and interrupt sources) from other RTOSes (VxWorks, pSOS etc) to Linux can be simplified by using this framework or something similar (I have been doing that for years)
So the question is, why does the UIO core imposes the need to map registers on page aligned quantities - ie, if the length of the PCI BAR region (a very typical use case) is not a multiple of the PAGE_SIZE you have to provide *your own mmap* call instead of simply using the one implemented in the uio core.
static int uio_mmap(struct file *filep, struct vm_area_struct *vma) { struct uio_listener *listener = filep->private_data; struct uio_device *idev = listener->dev; int mi; unsigned long requested_pages, actual_pages; int ret = 0;
if (vma->vm_end < vma->vm_start) return -EINVAL; vma->vm_private_data = idev; mi = uio_find_mem_index(vma); if (mi < 0) return -EINVAL; requested_pages = vma_pages(vma); actual_pages = ((idev->info->mem[mi].addr & ~PAGE_MASK) + idev->info->mem[mi].size + PAGE_SIZE -1) >> PAGE_SHIFT; if (requested_pages > actual_pages) return -EINVAL; if (idev->info->mmap) { ret = idev->info->mmap(idev->info, vma); return ret; } switch (idev->info->mem[mi].memtype) { case UIO_MEM_PHYS: return uio_mmap_physical(vma); case UIO_MEM_LOGICAL: case UIO_MEM_VIRTUAL: return uio_mmap_logical(vma); default: return -EINVAL; }
}
Why just not allow a user space driver (a driver after all) to map the necessary bytes beyond the length of the BAR to match the expected alignment? After all user land driver accesses to the invalid region (there should not be any!) shall only cause bus errors.
With this information in mind, does the patch below sounds as insane as it looks? Many companies are actually launching with this on their products in one way or another (for instance just reporting the BAR length to UIO as ALIGN(mem->size, PAGE_SIZE) so the test below doesn't fail.
Note: for those without the background in the patch below
- mem->size: length of the region in bytes (ie PCI BAR0)
- vma->vm_end - vma->vma_start (page aligned length of the region in bytes)
to be precise the result of this subtraction is the page aligned value of the length requested by the user via the mmap call....
[jramirez@calypso ~ (xenomai *$)]$ git diff uio.c diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c index 60fa627..d225cd9 100644 --- a/drivers/uio/uio.c +++ b/drivers/uio/uio.c @@ -643,7 +643,7 @@ static int uio_mmap_physical(struct vm_area_struct *vma) if (mem->addr & ~PAGE_MASK) return -ENODEV;
if (vma->vm_end - vma->vm_start > mem->size)
if (vma->vm_end - vma->vm_start > ALIGN(mem->size, PAGE_SIZE)) return -EINVAL;
vma->vm_ops = &uio_physical_vm_ops;
TIA, jorge
linaro-kernel@lists.linaro.org