| Message ID | 20230929170023.1020032-2-cleech@redhat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | UIO_MEM_DMA_COHERENT for cnic/bnx2/bnx2x | expand |
On Fri, Sep 29, 2023 at 10:00:21AM -0700, Chris Leech wrote: > Add a UIO memtype specificially for sharing dma_alloc_coherent > memory with userspace, backed by dma_mmap_coherent. Are you sure that you can share this type of memory with userspace safely? And you are saying what you are doing here, but not why you want to do it and who will use it. What are the userspace implications for accessing this type of memory? > > Signed-off-by: Chris Leech <cleech@redhat.com> > --- > drivers/uio/uio.c | 34 ++++++++++++++++++++++++++++++++++ > include/linux/uio_driver.h | 12 ++++++++++-- > 2 files changed, 44 insertions(+), 2 deletions(-) > > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c > index 62082d64ece0..f8f1f7ba6378 100644 > --- a/drivers/uio/uio.c > +++ b/drivers/uio/uio.c > @@ -24,6 +24,7 @@ > #include <linux/kobject.h> > #include <linux/cdev.h> > #include <linux/uio_driver.h> > +#include <linux/dma-mapping.h> > > #define UIO_MAX_DEVICES (1U << MINORBITS) > > @@ -759,6 +760,36 @@ static int uio_mmap_physical(struct vm_area_struct *vma) > vma->vm_page_prot); > } > > +static int uio_mmap_dma_coherent(struct vm_area_struct *vma) > +{ > + struct uio_device *idev = vma->vm_private_data; > + int mi = uio_find_mem_index(vma); > + struct uio_mem *mem; > + int rc; > + > + if (mi < 0) > + return -EINVAL; > + mem = idev->info->mem + mi; > + > + if (mem->dma_addr & ~PAGE_MASK) > + return -ENODEV; > + if (vma->vm_end - vma->vm_start > mem->size) > + return -EINVAL; > + > + /* > + * UIO uses offset to index into the maps for a device. > + * We need to clear vm_pgoff for dma_mmap_coherent. > + */ > + vma->vm_pgoff = 0; > + rc = dma_mmap_coherent(mem->dma_device, > + vma, > + mem->virtual_addr, > + mem->dma_addr, > + vma->vm_end - vma->vm_start); > + vma->vm_pgoff = mi; > + return rc; > +} > + > static int uio_mmap(struct file *filep, struct vm_area_struct *vma) > { > struct uio_listener *listener = filep->private_data; > @@ -806,6 +837,9 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma) > case UIO_MEM_VIRTUAL: > ret = uio_mmap_logical(vma); > break; > + case UIO_MEM_DMA_COHERENT: > + ret = uio_mmap_dma_coherent(vma); > + break; > default: > ret = -EINVAL; > } > diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h > index 47c5962b876b..ede58e984658 100644 > --- a/include/linux/uio_driver.h > +++ b/include/linux/uio_driver.h > @@ -36,11 +36,18 @@ struct uio_map; > */ > struct uio_mem { > const char *name; > - phys_addr_t addr; > + union { > + phys_addr_t addr; > + dma_addr_t dma_addr; > + }; > unsigned long offs; > resource_size_t size; > int memtype; > - void __iomem *internal_addr; > + union { > + void __iomem *internal_addr; > + void *virtual_addr; > + }; > + struct device *dma_device; Why are you adding a new struct device here? And why the unions? How are you going to verify that they are being used correctly? What space savings are you attempting to do here and why? thanks, greg k-h
On Sat, Sep 30, 2023 at 09:10:10AM +0200, Greg Kroah-Hartman wrote: > On Fri, Sep 29, 2023 at 10:00:21AM -0700, Chris Leech wrote: > > Add a UIO memtype specificially for sharing dma_alloc_coherent > > memory with userspace, backed by dma_mmap_coherent. > > Are you sure that you can share this type of memory with userspace > safely? And you are saying what you are doing here, but not why you > want to do it and who will use it. > > What are the userspace implications for accessing this type of memory? Thanks for taking the time to look at this Greg. I'm trying to help Marvell fix a regression with these drivers, by figuring out what the right way to handle this type of mmap is. The dma_mmap_coherent API exists for exactly this, so I thought making the uio interface aware of it made sense. There are uio drivers sharing dma_alloc_coherent memory (uio_dmem_genirq, uio_pruss) using UIO_MEM_PHYS, but that falls apart in the face of an iommu. > > struct uio_mem { > > const char *name; > > - phys_addr_t addr; > > + union { > > + phys_addr_t addr; > > + dma_addr_t dma_addr; > > + }; > > unsigned long offs; > > resource_size_t size; > > int memtype; > > - void __iomem *internal_addr; > > + union { > > + void __iomem *internal_addr; > > + void *virtual_addr; > > + }; > > + struct device *dma_device; > > Why are you adding a new struct device here? dma_mmap_coherent wants it. > And why the unions? How are you going to verify that they are being > used correctly? What space savings are you attempting to do here and > why? I should have expected that would be questioned, I was being paranoid about mixing different pointer and address types. I can remove the unions if putting a dma_addr_t in addr going to be OK. - Chris
This is the right way to map dma coherent memory to userspace. I have to say I agree with the doubts on what it is actually trying to do, though.
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c index 62082d64ece0..f8f1f7ba6378 100644 --- a/drivers/uio/uio.c +++ b/drivers/uio/uio.c @@ -24,6 +24,7 @@ #include <linux/kobject.h> #include <linux/cdev.h> #include <linux/uio_driver.h> +#include <linux/dma-mapping.h> #define UIO_MAX_DEVICES (1U << MINORBITS) @@ -759,6 +760,36 @@ static int uio_mmap_physical(struct vm_area_struct *vma) vma->vm_page_prot); } +static int uio_mmap_dma_coherent(struct vm_area_struct *vma) +{ + struct uio_device *idev = vma->vm_private_data; + int mi = uio_find_mem_index(vma); + struct uio_mem *mem; + int rc; + + if (mi < 0) + return -EINVAL; + mem = idev->info->mem + mi; + + if (mem->dma_addr & ~PAGE_MASK) + return -ENODEV; + if (vma->vm_end - vma->vm_start > mem->size) + return -EINVAL; + + /* + * UIO uses offset to index into the maps for a device. + * We need to clear vm_pgoff for dma_mmap_coherent. + */ + vma->vm_pgoff = 0; + rc = dma_mmap_coherent(mem->dma_device, + vma, + mem->virtual_addr, + mem->dma_addr, + vma->vm_end - vma->vm_start); + vma->vm_pgoff = mi; + return rc; +} + static int uio_mmap(struct file *filep, struct vm_area_struct *vma) { struct uio_listener *listener = filep->private_data; @@ -806,6 +837,9 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma) case UIO_MEM_VIRTUAL: ret = uio_mmap_logical(vma); break; + case UIO_MEM_DMA_COHERENT: + ret = uio_mmap_dma_coherent(vma); + break; default: ret = -EINVAL; } diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h index 47c5962b876b..ede58e984658 100644 --- a/include/linux/uio_driver.h +++ b/include/linux/uio_driver.h @@ -36,11 +36,18 @@ struct uio_map; */ struct uio_mem { const char *name; - phys_addr_t addr; + union { + phys_addr_t addr; + dma_addr_t dma_addr; + }; unsigned long offs; resource_size_t size; int memtype; - void __iomem *internal_addr; + union { + void __iomem *internal_addr; + void *virtual_addr; + }; + struct device *dma_device; struct uio_map *map; }; @@ -158,6 +165,7 @@ extern int __must_check #define UIO_MEM_LOGICAL 2 #define UIO_MEM_VIRTUAL 3 #define UIO_MEM_IOVA 4 +#define UIO_MEM_DMA_COHERENT 5 /* defines for uio_port->porttype */ #define UIO_PORT_NONE 0
Add a UIO memtype specificially for sharing dma_alloc_coherent memory with userspace, backed by dma_mmap_coherent. Signed-off-by: Chris Leech <cleech@redhat.com> --- drivers/uio/uio.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/uio_driver.h | 12 ++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-)