[V2,4/4] misc: vop: mapping kernel memory to user space as noncached
diff mbox series

Message ID 20200929084425.24052-5-sherry.sun@nxp.com
State New, archived
Headers show
Series
  • Change vring space from nomal memory to dma coherent memory
Related show

Commit Message

Sherry Sun Sept. 29, 2020, 8:44 a.m. UTC
Mapping kernel space memory to user space as noncached, since user space
need check the updates of avail_idx and device page flags timely.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
 drivers/misc/mic/vop/vop_vringh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Christoph Hellwig Sept. 29, 2020, 10:28 a.m. UTC | #1
On Tue, Sep 29, 2020 at 04:44:25PM +0800, Sherry Sun wrote:
> Mapping kernel space memory to user space as noncached, since user space
> need check the updates of avail_idx and device page flags timely.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> ---
>  drivers/misc/mic/vop/vop_vringh.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c
> index 4d5feb39aeb7..6e193bd64ef1 100644
> --- a/drivers/misc/mic/vop/vop_vringh.c
> +++ b/drivers/misc/mic/vop/vop_vringh.c
> @@ -1057,7 +1057,7 @@ static int vop_mmap(struct file *f, struct vm_area_struct *vma)
>  		}
>  		err = remap_pfn_range(vma, vma->vm_start + offset,
>  				      pa >> PAGE_SHIFT, size,
> -				      vma->vm_page_prot);
> +				      pgprot_noncached(vma->vm_page_prot));

You can't call remap_pfn_range on memory returned from
dma_alloc_coherent (which btw is not marked uncached on many platforms).

You need to use the dma_mmap_coherent helper instead.  And this also
needs to go into the first patch.
Sherry Sun Sept. 29, 2020, 1:12 p.m. UTC | #2
Hi Christoph,

> On Tue, Sep 29, 2020 at 04:44:25PM +0800, Sherry Sun wrote:
> > Mapping kernel space memory to user space as noncached, since user
> > space need check the updates of avail_idx and device page flags timely.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > ---
> >  drivers/misc/mic/vop/vop_vringh.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/mic/vop/vop_vringh.c
> > b/drivers/misc/mic/vop/vop_vringh.c
> > index 4d5feb39aeb7..6e193bd64ef1 100644
> > --- a/drivers/misc/mic/vop/vop_vringh.c
> > +++ b/drivers/misc/mic/vop/vop_vringh.c
> > @@ -1057,7 +1057,7 @@ static int vop_mmap(struct file *f, struct
> vm_area_struct *vma)
> >  		}
> >  		err = remap_pfn_range(vma, vma->vm_start + offset,
> >  				      pa >> PAGE_SHIFT, size,
> > -				      vma->vm_page_prot);
> > +				      pgprot_noncached(vma->vm_page_prot));
> 
> You can't call remap_pfn_range on memory returned from
> dma_alloc_coherent (which btw is not marked uncached on many platforms).
> 
> You need to use the dma_mmap_coherent helper instead.  And this also
> needs to go into the first patch.

Okay, will try to use dma_mmap_coherent  api in V3. Thanks.

Regards
Sherry
David Laight Sept. 29, 2020, 3:39 p.m. UTC | #3
From: Christoph Hellwig
> Sent: 29 September 2020 11:29
...
> You can't call remap_pfn_range on memory returned from
> dma_alloc_coherent (which btw is not marked uncached on many platforms).
> 
> You need to use the dma_mmap_coherent helper instead.

Hmmmm. I've a driver that does that.
Fortunately it only has to work on x86 where it doesn't matter.
However I can't easily convert it.
The 'problem' is that the mmap() request can cover multiple
dma buffers and need not start at the beginning of one.

Basically we have a PCIe card that has an inbuilt iommu
to convert internal 32bit addresses to 64bit PCIe ones.
This has 512 16kB pages.
So we do a number of dma_alloc_coherent() for 16k blocks.
The user process then does an mmap() for part of the buffer.
This request is 4k aligned so we do multiple remap_pfn_range()
calls to map the disjoint physical (and kernel virtual)
buffers into contiguous user memory.

So both ends see contiguous addresses even though the physical
addresses are non-contiguous.

I guess I could modify the vm_start address and then restore
it at the end.

I found this big discussion:
https://lore.kernel.org/patchwork/patch/351245/
about these functions.

The bit about VIPT caches is problematic.
I don't think you can change the kernel address during mmap.
What you need to do is defer allocating the user address until
you know the kernel address.
Otherwise you get into problems is you try to mmap the
same memory into two processes.
This is a general problem even for mmap() of files.
ISTR SYSV on some sparc systems having to use uncached maps.

If you might want to mmap two kernel buffers (dma or not)
into adjacent user addresses then you need some way of
allocating the second buffer to follow the first one
in the VIVT cache.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Sherry Sun Oct. 13, 2020, 2:02 a.m. UTC | #4
Hi David, thanks for your information.
Hi Christoph, please see my comments below.

> Subject: RE: [PATCH V2 4/4] misc: vop: mapping kernel memory to user space
> as noncached
> 
> From: Christoph Hellwig
> > Sent: 29 September 2020 11:29
> ...
> > You can't call remap_pfn_range on memory returned from
> > dma_alloc_coherent (which btw is not marked uncached on many
> platforms).
> >
> > You need to use the dma_mmap_coherent helper instead.
> 

I tried to use dma_mmap_coherent helper here, but I met the same problem as David said.
Since the user space calls mmap() to map all the device page and vring size at one time.

     va = mmap(NULL, MIC_DEVICE_PAGE_END + vr_size * num_vq, PROT_READ, MAP_SHARED, fd, 0);

But the physical addresses of device page and multiple vrings are not consecutive, so we called
multiple remap_pfn_range before. When changing to use dma_mmap_coherent, it will return error
because vma_pages(the size user space want to map) are bigger than the actual size we do multiple
map(one non-continuous memory size at a time).

David believes that we could modify the vm_start address before call the multiple dma_mmap_coherent to
avoid the vma_pages check error and map multiple discontinuous memory.
Do you have any suggestions?

Best regards
Sherry

> Hmmmm. I've a driver that does that.
> Fortunately it only has to work on x86 where it doesn't matter.
> However I can't easily convert it.
> The 'problem' is that the mmap() request can cover multiple dma buffers and
> need not start at the beginning of one.
> 
> Basically we have a PCIe card that has an inbuilt iommu to convert internal
> 32bit addresses to 64bit PCIe ones.
> This has 512 16kB pages.
> So we do a number of dma_alloc_coherent() for 16k blocks.
> The user process then does an mmap() for part of the buffer.
> This request is 4k aligned so we do multiple remap_pfn_range() calls to map
> the disjoint physical (and kernel virtual) buffers into contiguous user memory.
> 
> So both ends see contiguous addresses even though the physical addresses
> are non-contiguous.
> 
> I guess I could modify the vm_start address and then restore it at the end.
> 
> I found this big discussion:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k
> ernel.org%2Fpatchwork%2Fpatch%2F351245%2F&amp;data=02%7C01%7Csh
> erry.sun%40nxp.com%7C876724689688440581a708d8648dceb3%7C686ea1d
> 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637369907516376294&amp;sdat
> a=amSClQfVGhI0%2F3bZfo8HF7UmCktkPluArWW22YlQzMQ%3D&amp;reser
> ved=0
> about these functions.
> 
> The bit about VIPT caches is problematic.
> I don't think you can change the kernel address during mmap.
> What you need to do is defer allocating the user address until you know the
> kernel address.
> Otherwise you get into problems is you try to mmap the same memory into
> two processes.
> This is a general problem even for mmap() of files.
> ISTR SYSV on some sparc systems having to use uncached maps.
> 
> If you might want to mmap two kernel buffers (dma or not) into adjacent
> user addresses then you need some way of allocating the second buffer to
> follow the first one in the VIVT cache.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK Registration No: 1397386 (Wales)
Sherry Sun Oct. 20, 2020, 1:33 a.m. UTC | #5
Hi Christoph,

Gentle ping....

Can you give some suggestions on the limitations of dma_mmap_coherent api?

Best regards
Sherry

> Subject: RE: [PATCH V2 4/4] misc: vop: mapping kernel memory to user space
> as noncached
> 
> Hi David, thanks for your information.
> Hi Christoph, please see my comments below.
> 
> > Subject: RE: [PATCH V2 4/4] misc: vop: mapping kernel memory to user
> > space as noncached
> >
> > From: Christoph Hellwig
> > > Sent: 29 September 2020 11:29
> > ...
> > > You can't call remap_pfn_range on memory returned from
> > > dma_alloc_coherent (which btw is not marked uncached on many
> > platforms).
> > >
> > > You need to use the dma_mmap_coherent helper instead.
> >
> 
> I tried to use dma_mmap_coherent helper here, but I met the same problem
> as David said.
> Since the user space calls mmap() to map all the device page and vring size at
> one time.
> 
>      va = mmap(NULL, MIC_DEVICE_PAGE_END + vr_size * num_vq,
> PROT_READ, MAP_SHARED, fd, 0);
> 
> But the physical addresses of device page and multiple vrings are not
> consecutive, so we called multiple remap_pfn_range before. When changing
> to use dma_mmap_coherent, it will return error because vma_pages(the size
> user space want to map) are bigger than the actual size we do multiple
> map(one non-continuous memory size at a time).
> 
> David believes that we could modify the vm_start address before call the
> multiple dma_mmap_coherent to avoid the vma_pages check error and map
> multiple discontinuous memory.
> Do you have any suggestions?
> 
> Best regards
> Sherry
> 
> > Hmmmm. I've a driver that does that.
> > Fortunately it only has to work on x86 where it doesn't matter.
> > However I can't easily convert it.
> > The 'problem' is that the mmap() request can cover multiple dma
> > buffers and need not start at the beginning of one.
> >
> > Basically we have a PCIe card that has an inbuilt iommu to convert
> > internal 32bit addresses to 64bit PCIe ones.
> > This has 512 16kB pages.
> > So we do a number of dma_alloc_coherent() for 16k blocks.
> > The user process then does an mmap() for part of the buffer.
> > This request is 4k aligned so we do multiple remap_pfn_range() calls
> > to map the disjoint physical (and kernel virtual) buffers into contiguous
> user memory.
> >
> > So both ends see contiguous addresses even though the physical
> > addresses are non-contiguous.
> >
> > I guess I could modify the vm_start address and then restore it at the end.
> >
> > I found this big discussion:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .k
> ernel.org%2Fpatchwork%2Fpatch%2F351245%2F&amp;data=02%7C01%7Csh
> >
> erry.sun%40nxp.com%7C876724689688440581a708d8648dceb3%7C686ea1d
> >
> 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637369907516376294&amp;sdat
> >
> a=amSClQfVGhI0%2F3bZfo8HF7UmCktkPluArWW22YlQzMQ%3D&amp;reser
> > ved=0
> > about these functions.
> >
> > The bit about VIPT caches is problematic.
> > I don't think you can change the kernel address during mmap.
> > What you need to do is defer allocating the user address until you
> > know the kernel address.
> > Otherwise you get into problems is you try to mmap the same memory
> > into two processes.
> > This is a general problem even for mmap() of files.
> > ISTR SYSV on some sparc systems having to use uncached maps.
> >
> > If you might want to mmap two kernel buffers (dma or not) into
> > adjacent user addresses then you need some way of allocating the
> > second buffer to follow the first one in the VIVT cache.
> >
> > 	David
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> > MK1 1PT, UK Registration No: 1397386 (Wales)

Patch
diff mbox series

diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c
index 4d5feb39aeb7..6e193bd64ef1 100644
--- a/drivers/misc/mic/vop/vop_vringh.c
+++ b/drivers/misc/mic/vop/vop_vringh.c
@@ -1057,7 +1057,7 @@  static int vop_mmap(struct file *f, struct vm_area_struct *vma)
 		}
 		err = remap_pfn_range(vma, vma->vm_start + offset,
 				      pa >> PAGE_SHIFT, size,
-				      vma->vm_page_prot);
+				      pgprot_noncached(vma->vm_page_prot));
 		if (err)
 			goto ret;
 		size_rem -= size;