| Message ID | alpine.DEB.2.02.1403261741110.2747@kaball.uk.xensource.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 26, 2014 at 06:49:57PM +0000, Stefano Stabellini wrote: > On Wed, 26 Mar 2014, Oleksandr Dmytryshyn wrote: > > On Wed, Mar 26, 2014 at 4:46 PM, Konrad Rzeszutek Wilk > > <konrad.wilk@oracle.com> wrote: > > > On Wed, Mar 26, 2014 at 12:35:12PM +0200, Oleksandr Dmytryshyn wrote: > > >> Hi to all. > > >> > > >> Currently I'm using hypervisor on ARM Cortex A15 processor (DRA7xx > > >> Jacinto 6 processor). I'm using mainline xen 4.4 with some patches on > > >> top plus dom0 and domU linux kernel 3.8. > > >> > > >> I've written a hack to make a camera working on my board in dom0 > > >> (drivers for camera and display system are in dom0). An user-space > > >> application in dom0 is used to test camera (it reads camera captured > > >> data and sends it to the framebuffer). In the kernel code I've implemented > > >> .mmap callback function in the swiotlb-xen.c file (like xen_swiotlb_mmap()) > > >> and copied to this new callback all content from the original function > > >> 'arm_dma_mmap()' from the kernel. This function creates userspace > > >> mapping for the DMA-coherent memory. With this hack camera is working > > >> (I can see captured video on display). > > > > > > Why can't you use the v4l API? Won't that work? > > > > > Currently we are using v4l api. swiotlb-xen doesnt have mmap callback > > implemented, > > so kernel use standard function dma_common_mmap() to map memory. But this > > function doesn't work correctly. We've also tried to use arm_dma_mmap() function > > instead of the dma_common_mmap() (HACK in the file > > include/asm-generic/dma-mapping-common.h only for dom0) and all is working > > in this case. > > This is not a Xen specific issue. > In fact unless some of the dma pages involved could be foreign guest > pages (for example because you have a PV framebuffer frontend in a guest > sharing pages with a framebuffer backend in Dom0, and these pages are > used directly in your camera driver), swiotlb-xen shouldn't even be > involved. > > The problem is that on ARM, when you call virt_to_phys on a virtual > address returned by dma_alloc_from_coherent, it doesn't return the right > physical address. > For example: > > virt_address = dma_alloc_from_coherent(dev, size, dma_addr, &ret); > virt_to_phys(virt_address) != dma_addr > > The issue is not that bus addresses are different from physical > addresses. The problem is that the virtual address is an ioremap address > therefore virt_to_phys and virt_to_page don't work as expected. > > To fix your problem you can simply: > > diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c > index 0ce39a3..052a5ed 100644 > --- a/drivers/base/dma-mapping.c > +++ b/drivers/base/dma-mapping.c > @@ -248,7 +248,7 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, > #ifdef CONFIG_MMU > unsigned long user_count = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; > unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT; > - unsigned long pfn = page_to_pfn(virt_to_page(cpu_addr)); > + unsigned long pfn = dma_addr >> PAGE_SHIFT; > unsigned long off = vma->vm_pgoff; > > vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > however that would cause problems on architectures for which bus > addresses are different from physical addresses. > Alternatively you could: > > + unsigned long pfn = bus_to_phys(dma_addr) >> PAGE_SHIFT; > > however bus_to_phys is not defined on all architectures. > I am not sure what is the best way to fix this in common code such us > drivers/base/dma-mapping.c. Ugh. Usually drivers have an 'dma_addr_t[]' array to go along with the virtual addresses (or the array of the 'struct page'.). That way they have both the CPU and the bus addresses for the same region. Could that be used here?
Hi, Stefano. Thank You for advice. I'll try this few days later. Oleksandr Dmytryshyn | Product Engineering and Development GlobalLogic M +38.067.382.2525 www.globallogic.com http://www.globallogic.com/email_disclaimer.txt On Wed, Mar 26, 2014 at 8:49 PM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > On Wed, 26 Mar 2014, Oleksandr Dmytryshyn wrote: >> On Wed, Mar 26, 2014 at 4:46 PM, Konrad Rzeszutek Wilk >> <konrad.wilk@oracle.com> wrote: >> > On Wed, Mar 26, 2014 at 12:35:12PM +0200, Oleksandr Dmytryshyn wrote: >> >> Hi to all. >> >> >> >> Currently I'm using hypervisor on ARM Cortex A15 processor (DRA7xx >> >> Jacinto 6 processor). I'm using mainline xen 4.4 with some patches on >> >> top plus dom0 and domU linux kernel 3.8. >> >> >> >> I've written a hack to make a camera working on my board in dom0 >> >> (drivers for camera and display system are in dom0). An user-space >> >> application in dom0 is used to test camera (it reads camera captured >> >> data and sends it to the framebuffer). In the kernel code I've implemented >> >> .mmap callback function in the swiotlb-xen.c file (like xen_swiotlb_mmap()) >> >> and copied to this new callback all content from the original function >> >> 'arm_dma_mmap()' from the kernel. This function creates userspace >> >> mapping for the DMA-coherent memory. With this hack camera is working >> >> (I can see captured video on display). >> > >> > Why can't you use the v4l API? Won't that work? >> > >> Currently we are using v4l api. swiotlb-xen doesnt have mmap callback >> implemented, >> so kernel use standard function dma_common_mmap() to map memory. But this >> function doesn't work correctly. We've also tried to use arm_dma_mmap() function >> instead of the dma_common_mmap() (HACK in the file >> include/asm-generic/dma-mapping-common.h only for dom0) and all is working >> in this case. > > This is not a Xen specific issue. > In fact unless some of the dma pages involved could be foreign guest > pages (for example because you have a PV framebuffer frontend in a guest > sharing pages with a framebuffer backend in Dom0, and these pages are > used directly in your camera driver), swiotlb-xen shouldn't even be > involved. > > The problem is that on ARM, when you call virt_to_phys on a virtual > address returned by dma_alloc_from_coherent, it doesn't return the right > physical address. > For example: > > virt_address = dma_alloc_from_coherent(dev, size, dma_addr, &ret); > virt_to_phys(virt_address) != dma_addr > > The issue is not that bus addresses are different from physical > addresses. The problem is that the virtual address is an ioremap address > therefore virt_to_phys and virt_to_page don't work as expected. > > To fix your problem you can simply: > > diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c > index 0ce39a3..052a5ed 100644 > --- a/drivers/base/dma-mapping.c > +++ b/drivers/base/dma-mapping.c > @@ -248,7 +248,7 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, > #ifdef CONFIG_MMU > unsigned long user_count = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; > unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT; > - unsigned long pfn = page_to_pfn(virt_to_page(cpu_addr)); > + unsigned long pfn = dma_addr >> PAGE_SHIFT; > unsigned long off = vma->vm_pgoff; > > vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > however that would cause problems on architectures for which bus > addresses are different from physical addresses. > Alternatively you could: > > + unsigned long pfn = bus_to_phys(dma_addr) >> PAGE_SHIFT; > > however bus_to_phys is not defined on all architectures. > I am not sure what is the best way to fix this in common code such us > drivers/base/dma-mapping.c.
Hi to all. I've investigated my issue deeper. Currently we use mmap in 2 cases: video-in (camera) and video-out (display). In case video-out all is working because mmap callback in the video-out device leads to the remap_pfn_range() function (this function doesn't use unimplemented .mmap callback in the swiotlb-xen.c file). In case video-out mmap callback in the video-in device leads to the .mmap callback from the dma ops (unimplemented callback in the swiotlb-xen.c file) I've investigated kernel code. We have patch from the Stefano Stabellini (068a6e62 - arm/xen: get_dma_ops: return xen_dma_ops if we are running as xen_initial_domain) Before this patch the function *get_dma_ops() always returned __generic_dma_ops() which returned &arm_dma_ops (if archdata.dma_ops is absent). In this case for arm architecture .mmap callback is arm_dma_mmap() function. After this patch for domU all is the same as before this patch. But in case dom0 kernel uses xen_dma_ops with unimplemented .mmap callback. If some function tries to use .mmap callback in dom0 then kernel calls generic function dma_common_mmap() which doesn't work properly. May be some mechanism for swiotlb-xen should be implemented for unimplemented callbacks: if callback is not implemented then right architectural callback should be used (.mmap - arm_dma_mmap() for ARM)? Oleksandr Dmytryshyn | Product Engineering and Development GlobalLogic M +38.067.382.2525 www.globallogic.com http://www.globallogic.com/email_disclaimer.txt On Thu, Mar 27, 2014 at 12:11 PM, Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com> wrote: > Hi, Stefano. > > Thank You for advice. > > I'll try this few days later. > > Oleksandr Dmytryshyn | Product Engineering and Development > GlobalLogic > M +38.067.382.2525 > www.globallogic.com > > http://www.globallogic.com/email_disclaimer.txt > > > On Wed, Mar 26, 2014 at 8:49 PM, Stefano Stabellini > <stefano.stabellini@eu.citrix.com> wrote: >> On Wed, 26 Mar 2014, Oleksandr Dmytryshyn wrote: >>> On Wed, Mar 26, 2014 at 4:46 PM, Konrad Rzeszutek Wilk >>> <konrad.wilk@oracle.com> wrote: >>> > On Wed, Mar 26, 2014 at 12:35:12PM +0200, Oleksandr Dmytryshyn wrote: >>> >> Hi to all. >>> >> >>> >> Currently I'm using hypervisor on ARM Cortex A15 processor (DRA7xx >>> >> Jacinto 6 processor). I'm using mainline xen 4.4 with some patches on >>> >> top plus dom0 and domU linux kernel 3.8. >>> >> >>> >> I've written a hack to make a camera working on my board in dom0 >>> >> (drivers for camera and display system are in dom0). An user-space >>> >> application in dom0 is used to test camera (it reads camera captured >>> >> data and sends it to the framebuffer). In the kernel code I've implemented >>> >> .mmap callback function in the swiotlb-xen.c file (like xen_swiotlb_mmap()) >>> >> and copied to this new callback all content from the original function >>> >> 'arm_dma_mmap()' from the kernel. This function creates userspace >>> >> mapping for the DMA-coherent memory. With this hack camera is working >>> >> (I can see captured video on display). >>> > >>> > Why can't you use the v4l API? Won't that work? >>> > >>> Currently we are using v4l api. swiotlb-xen doesnt have mmap callback >>> implemented, >>> so kernel use standard function dma_common_mmap() to map memory. But this >>> function doesn't work correctly. We've also tried to use arm_dma_mmap() function >>> instead of the dma_common_mmap() (HACK in the file >>> include/asm-generic/dma-mapping-common.h only for dom0) and all is working >>> in this case. >> >> This is not a Xen specific issue. >> In fact unless some of the dma pages involved could be foreign guest >> pages (for example because you have a PV framebuffer frontend in a guest >> sharing pages with a framebuffer backend in Dom0, and these pages are >> used directly in your camera driver), swiotlb-xen shouldn't even be >> involved. >> >> The problem is that on ARM, when you call virt_to_phys on a virtual >> address returned by dma_alloc_from_coherent, it doesn't return the right >> physical address. >> For example: >> >> virt_address = dma_alloc_from_coherent(dev, size, dma_addr, &ret); >> virt_to_phys(virt_address) != dma_addr >> >> The issue is not that bus addresses are different from physical >> addresses. The problem is that the virtual address is an ioremap address >> therefore virt_to_phys and virt_to_page don't work as expected. >> >> To fix your problem you can simply: >> >> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c >> index 0ce39a3..052a5ed 100644 >> --- a/drivers/base/dma-mapping.c >> +++ b/drivers/base/dma-mapping.c >> @@ -248,7 +248,7 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, >> #ifdef CONFIG_MMU >> unsigned long user_count = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; >> unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT; >> - unsigned long pfn = page_to_pfn(virt_to_page(cpu_addr)); >> + unsigned long pfn = dma_addr >> PAGE_SHIFT; >> unsigned long off = vma->vm_pgoff; >> >> vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); >> >> however that would cause problems on architectures for which bus >> addresses are different from physical addresses. >> Alternatively you could: >> >> + unsigned long pfn = bus_to_phys(dma_addr) >> PAGE_SHIFT; >> >> however bus_to_phys is not defined on all architectures. >> I am not sure what is the best way to fix this in common code such us >> drivers/base/dma-mapping.c.
On Tue, 1 Apr 2014, Oleksandr Dmytryshyn wrote: > Hi to all. I've investigated my issue deeper. > > Currently we use mmap in 2 cases: video-in (camera) and video-out (display). > > In case video-out all is working because mmap callback in the video-out device > leads to the remap_pfn_range() function (this function doesn't use unimplemented > .mmap callback in the swiotlb-xen.c file). > > In case video-out mmap callback in the video-in device leads to the .mmap > callback from the dma ops (unimplemented callback in the swiotlb-xen.c file) > > I've investigated kernel code. We have patch from the Stefano Stabellini > (068a6e62 - arm/xen: get_dma_ops: return xen_dma_ops if we are running as > xen_initial_domain) > > Before this patch the function *get_dma_ops() always returned > __generic_dma_ops() > which returned &arm_dma_ops (if archdata.dma_ops is absent). In this case > for arm architecture .mmap callback is arm_dma_mmap() function. > > After this patch for domU all is the same as before this patch. But in case > dom0 kernel uses xen_dma_ops with unimplemented .mmap callback. If some > function tries to use .mmap callback in dom0 then kernel calls generic > function dma_common_mmap() which doesn't work properly. > > May be some mechanism for swiotlb-xen should be implemented for unimplemented > callbacks: if callback is not implemented then right architectural callback > should be used (.mmap - arm_dma_mmap() for ARM)? I understand now. Thanks for investingating! I think that we'll have to introduce our own version of .mmap, because dma_to_pfn doesn't work correctly on Xen on ARM. The implementation would be something like (untested): int xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma, void *cpu_addr, dma_addr_t dma_addr, size_t size, struct dma_attrs *attrs) { int ret = -ENXIO; #ifdef CONFIG_MMU unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; unsigned long pfn = PFN_DOWN(xen_bus_to_phys(dev, dma_addr)); unsigned long off = vma->vm_pgoff; vma->vm_page_prot = __get_dma_pgprot(attrs, vma->vm_page_prot); if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) return ret; if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) { ret = remap_pfn_range(vma, vma->vm_start, pfn + off, vma->vm_end - vma->vm_start, vma->vm_page_prot); } #endif /* CONFIG_MMU */ return ret; } then we can add the function to xen_swiotlb_dma_ops. Would you be up for coming up with a proper patch?
On Thu, 3 Apr 2014, Oleksandr Dmytryshyn wrote: > Hi, Stefano. > > Thank You for advice! > I see that You just copied the content from the original function arm_dma_mmap() > to the swiotlb-xen driver. I did not :-) I changed the function used to translate from dma addresses to pfn from dma_to_pfn to xen_bus_to_phys. > I've tested the same solution (except that I've copied __get_dma_pgprot() > function to the swiotlb-xen.c file too because this function is static in the > original file). This solution is working (I've tested it). But there is > an easier solution: > > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c > index b0e77de..f3e820f 100644 > --- a/arch/arm/xen/mm.c > +++ b/arch/arm/xen/mm.c > @@ -48,6 +48,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = { > .sync_sg_for_device = xen_swiotlb_sync_sg_for_device, > .map_sg = xen_swiotlb_map_sg_attrs, > .unmap_sg = xen_swiotlb_unmap_sg_attrs, > + .mmap = arm_dma_mmap, > .map_page = xen_swiotlb_map_page, > .unmap_page = xen_swiotlb_unmap_page, > .dma_supported = xen_swiotlb_dma_supported, > > The original arm_dma_mmap() function is implemented in the file > arch/arm/mm/dma-mapping.c which always is compiled for arm arhitecture. > I've checked this solution too. It is working. > > I have a question. Is my solution correct? No, because if any foreign pages are involved the physical address corresponding to the dma address would not be the one returned by dma_to_pfn. We need to call xen_bus_to_phys. > Oleksandr Dmytryshyn | Product Engineering and Development > GlobalLogic > M +38.067.382.2525 > www.globallogic.com > > http://www.globallogic.com/email_disclaimer.txt > > > On Thu, Apr 3, 2014 at 1:51 PM, Stefano Stabellini > <stefano.stabellini@eu.citrix.com> wrote: > > On Tue, 1 Apr 2014, Oleksandr Dmytryshyn wrote: > >> Hi to all. I've investigated my issue deeper. > >> > >> Currently we use mmap in 2 cases: video-in (camera) and video-out (display). > >> > >> In case video-out all is working because mmap callback in the video-out device > >> leads to the remap_pfn_range() function (this function doesn't use unimplemented > >> .mmap callback in the swiotlb-xen.c file). > >> > >> In case video-out mmap callback in the video-in device leads to the .mmap > >> callback from the dma ops (unimplemented callback in the swiotlb-xen.c file) > >> > >> I've investigated kernel code. We have patch from the Stefano Stabellini > >> (068a6e62 - arm/xen: get_dma_ops: return xen_dma_ops if we are running as > >> xen_initial_domain) > >> > >> Before this patch the function *get_dma_ops() always returned > >> __generic_dma_ops() > >> which returned &arm_dma_ops (if archdata.dma_ops is absent). In this case > >> for arm architecture .mmap callback is arm_dma_mmap() function. > >> > >> After this patch for domU all is the same as before this patch. But in case > >> dom0 kernel uses xen_dma_ops with unimplemented .mmap callback. If some > >> function tries to use .mmap callback in dom0 then kernel calls generic > >> function dma_common_mmap() which doesn't work properly. > >> > >> May be some mechanism for swiotlb-xen should be implemented for unimplemented > >> callbacks: if callback is not implemented then right architectural callback > >> should be used (.mmap - arm_dma_mmap() for ARM)? > > > > I understand now. Thanks for investingating! > > I think that we'll have to introduce our own version of .mmap, > > because dma_to_pfn doesn't work correctly on Xen on ARM. > > The implementation would be something like (untested): > > > > int xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma, > > void *cpu_addr, dma_addr_t dma_addr, size_t size, > > struct dma_attrs *attrs) > > { > > int ret = -ENXIO; > > #ifdef CONFIG_MMU > > unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; > > unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; > > unsigned long pfn = PFN_DOWN(xen_bus_to_phys(dev, dma_addr)); > > unsigned long off = vma->vm_pgoff; > > > > vma->vm_page_prot = __get_dma_pgprot(attrs, vma->vm_page_prot); > > > > if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) > > return ret; > > > > if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) { > > ret = remap_pfn_range(vma, vma->vm_start, > > pfn + off, > > vma->vm_end - vma->vm_start, > > vma->vm_page_prot); > > } > > #endif /* CONFIG_MMU */ > > > > return ret; > > } > > > > then we can add the function to xen_swiotlb_dma_ops. > > Would you be up for coming up with a proper patch? >
On Thu, Apr 3, 2014 at 3:52 PM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > On Thu, 3 Apr 2014, Oleksandr Dmytryshyn wrote: >> Hi, Stefano. >> >> Thank You for advice! >> I see that You just copied the content from the original function arm_dma_mmap() >> to the swiotlb-xen driver. > > I did not :-) > I changed the function used to translate from dma addresses to pfn from > dma_to_pfn to xen_bus_to_phys. > > >> I've tested the same solution (except that I've copied __get_dma_pgprot() >> function to the swiotlb-xen.c file too because this function is static in the >> original file). This solution is working (I've tested it). But there is >> an easier solution: >> >> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c >> index b0e77de..f3e820f 100644 >> --- a/arch/arm/xen/mm.c >> +++ b/arch/arm/xen/mm.c >> @@ -48,6 +48,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = { >> .sync_sg_for_device = xen_swiotlb_sync_sg_for_device, >> .map_sg = xen_swiotlb_map_sg_attrs, >> .unmap_sg = xen_swiotlb_unmap_sg_attrs, >> + .mmap = arm_dma_mmap, >> .map_page = xen_swiotlb_map_page, >> .unmap_page = xen_swiotlb_unmap_page, >> .dma_supported = xen_swiotlb_dma_supported, >> >> The original arm_dma_mmap() function is implemented in the file >> arch/arm/mm/dma-mapping.c which always is compiled for arm arhitecture. >> I've checked this solution too. It is working. >> >> I have a question. Is my solution correct? > > No, because if any foreign pages are involved the physical address > corresponding to the dma address would not be the one returned by > dma_to_pfn. We need to call xen_bus_to_phys. > > >> Oleksandr Dmytryshyn | Product Engineering and Development >> GlobalLogic >> M +38.067.382.2525 >> www.globallogic.com >> >> http://www.globallogic.com/email_disclaimer.txt >> >> >> On Thu, Apr 3, 2014 at 1:51 PM, Stefano Stabellini >> <stefano.stabellini@eu.citrix.com> wrote: >> > On Tue, 1 Apr 2014, Oleksandr Dmytryshyn wrote: >> >> Hi to all. I've investigated my issue deeper. >> >> >> >> Currently we use mmap in 2 cases: video-in (camera) and video-out (display). >> >> >> >> In case video-out all is working because mmap callback in the video-out device >> >> leads to the remap_pfn_range() function (this function doesn't use unimplemented >> >> .mmap callback in the swiotlb-xen.c file). >> >> >> >> In case video-out mmap callback in the video-in device leads to the .mmap >> >> callback from the dma ops (unimplemented callback in the swiotlb-xen.c file) >> >> >> >> I've investigated kernel code. We have patch from the Stefano Stabellini >> >> (068a6e62 - arm/xen: get_dma_ops: return xen_dma_ops if we are running as >> >> xen_initial_domain) >> >> >> >> Before this patch the function *get_dma_ops() always returned >> >> __generic_dma_ops() >> >> which returned &arm_dma_ops (if archdata.dma_ops is absent). In this case >> >> for arm architecture .mmap callback is arm_dma_mmap() function. >> >> >> >> After this patch for domU all is the same as before this patch. But in case >> >> dom0 kernel uses xen_dma_ops with unimplemented .mmap callback. If some >> >> function tries to use .mmap callback in dom0 then kernel calls generic >> >> function dma_common_mmap() which doesn't work properly. >> >> >> >> May be some mechanism for swiotlb-xen should be implemented for unimplemented >> >> callbacks: if callback is not implemented then right architectural callback >> >> should be used (.mmap - arm_dma_mmap() for ARM)? >> > >> > I understand now. Thanks for investingating! >> > I think that we'll have to introduce our own version of .mmap, >> > because dma_to_pfn doesn't work correctly on Xen on ARM. >> > The implementation would be something like (untested): >> > >> > int xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma, >> > void *cpu_addr, dma_addr_t dma_addr, size_t size, >> > struct dma_attrs *attrs) >> > { >> > int ret = -ENXIO; >> > #ifdef CONFIG_MMU >> > unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; >> > unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; >> > unsigned long pfn = PFN_DOWN(xen_bus_to_phys(dev, dma_addr)); >> > unsigned long off = vma->vm_pgoff; >> > >> > vma->vm_page_prot = __get_dma_pgprot(attrs, vma->vm_page_prot); >> > >> > if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) >> > return ret; >> > >> > if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) { >> > ret = remap_pfn_range(vma, vma->vm_start, >> > pfn + off, >> > vma->vm_end - vma->vm_start, >> > vma->vm_page_prot); >> > } >> > #endif /* CONFIG_MMU */ >> > >> > return ret; >> > } >> > >> > then we can add the function to xen_swiotlb_dma_ops. >> > Would you be up for coming up with a proper patch? >> Hi, Stefano. Thank You for the response. I'll test Your solution and write about results. Oleksandr Dmytryshyn | Product Engineering and Development GlobalLogic M +38.067.382.2525 www.globallogic.com http://www.globallogic.com/email_disclaimer.txt
On Fri, 4 Apr 2014, Oleksandr Dmytryshyn wrote: > On Thu, Apr 3, 2014 at 1:51 PM, Stefano Stabellini > <stefano.stabellini@eu.citrix.com> wrote: > > On Tue, 1 Apr 2014, Oleksandr Dmytryshyn wrote: > >> Hi to all. I've investigated my issue deeper. > >> > >> Currently we use mmap in 2 cases: video-in (camera) and video-out (display). > >> > >> In case video-out all is working because mmap callback in the video-out device > >> leads to the remap_pfn_range() function (this function doesn't use unimplemented > >> .mmap callback in the swiotlb-xen.c file). > >> > >> In case video-out mmap callback in the video-in device leads to the .mmap > >> callback from the dma ops (unimplemented callback in the swiotlb-xen.c file) > >> > >> I've investigated kernel code. We have patch from the Stefano Stabellini > >> (068a6e62 - arm/xen: get_dma_ops: return xen_dma_ops if we are running as > >> xen_initial_domain) > >> > >> Before this patch the function *get_dma_ops() always returned > >> __generic_dma_ops() > >> which returned &arm_dma_ops (if archdata.dma_ops is absent). In this case > >> for arm architecture .mmap callback is arm_dma_mmap() function. > >> > >> After this patch for domU all is the same as before this patch. But in case > >> dom0 kernel uses xen_dma_ops with unimplemented .mmap callback. If some > >> function tries to use .mmap callback in dom0 then kernel calls generic > >> function dma_common_mmap() which doesn't work properly. > >> > >> May be some mechanism for swiotlb-xen should be implemented for unimplemented > >> callbacks: if callback is not implemented then right architectural callback > >> should be used (.mmap - arm_dma_mmap() for ARM)? > > > > I understand now. Thanks for investingating! > > I think that we'll have to introduce our own version of .mmap, > > because dma_to_pfn doesn't work correctly on Xen on ARM. > > The implementation would be something like (untested): > > > > int xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma, > > void *cpu_addr, dma_addr_t dma_addr, size_t size, > > struct dma_attrs *attrs) > > { > > int ret = -ENXIO; > > #ifdef CONFIG_MMU > > unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; > > unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; > > unsigned long pfn = PFN_DOWN(xen_bus_to_phys(dev, dma_addr)); > > unsigned long off = vma->vm_pgoff; > > > > vma->vm_page_prot = __get_dma_pgprot(attrs, vma->vm_page_prot); > > > > if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) > > return ret; > > > > if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) { > > ret = remap_pfn_range(vma, vma->vm_start, > > pfn + off, > > vma->vm_end - vma->vm_start, > > vma->vm_page_prot); > > } > > #endif /* CONFIG_MMU */ > > > > return ret; > > } > > > > then we can add the function to xen_swiotlb_dma_ops. > > Would you be up for coming up with a proper patch? > > Hi, Stefano. > > Here is a slightly modified solution from You (I've only replaced a > __get_dma_pgprot() function with its original implementation): Looks fine. Could you resend to the list as a proper patch and with a signed-off-by line? Otherwise I can do it. > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c > index b0e77de..91408b1 100644 > --- a/arch/arm/xen/mm.c > +++ b/arch/arm/xen/mm.c > @@ -48,6 +48,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = { > .sync_sg_for_device = xen_swiotlb_sync_sg_for_device, > .map_sg = xen_swiotlb_map_sg_attrs, > .unmap_sg = xen_swiotlb_unmap_sg_attrs, > + .mmap = xen_swiotlb_dma_mmap, > .map_page = xen_swiotlb_map_page, > .unmap_page = xen_swiotlb_unmap_page, > .dma_supported = xen_swiotlb_dma_supported, > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 5403855..acf0c06 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -407,6 +407,42 @@ dma_addr_t xen_swiotlb_map_page(struct device > *dev, struct page *page, > EXPORT_SYMBOL_GPL(xen_swiotlb_map_page); > > /* > + * Create userspace mapping for the DMA-coherent memory. > + */ > +int xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma, > + void *cpu_addr, dma_addr_t dma_addr, size_t size, > + struct dma_attrs *attrs) > +{ > + int ret = -ENXIO; > +#ifdef CONFIG_MMU > + unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; > + unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; > + unsigned long pfn = PFN_DOWN(xen_bus_to_phys(dma_addr)); > + unsigned long off = vma->vm_pgoff; > + pgprot_t prot = vma->vm_page_prot; > + > + prot = dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs) ? > + pgprot_writecombine(prot) : > + pgprot_dmacoherent(prot); > + > + vma->vm_page_prot = prot; > + > + if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) > + return ret; > + > + if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) { > + ret = remap_pfn_range(vma, vma->vm_start, > + pfn + off, > + vma->vm_end - vma->vm_start, > + vma->vm_page_prot); > + } > +#endif /* CONFIG_MMU */ > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(xen_swiotlb_dma_mmap); > + > +/* > * Unmap a single streaming mode DMA translation. The dma_addr and size must > * match what was provided for in a previous xen_swiotlb_map_page call. All > * other usages are undefined. > diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h > index 7b64465..930fa94 100644 > --- a/include/xen/swiotlb-xen.h > +++ b/include/xen/swiotlb-xen.h > @@ -15,6 +15,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, > void *vaddr, dma_addr_t dma_handle, > struct dma_attrs *attrs); > > +extern int > +xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma, > + void *cpu_addr, dma_addr_t dma_addr, size_t size, > + struct dma_attrs *attrs); > + > extern dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page, > unsigned long offset, size_t size, > enum dma_data_direction dir, > > > I've checked it on Xen 4.4 stable + some our architecture specific > patches on top > plus Dom0 and DomU Linux Kernel 3.8 on DRA7xxx based board. Camera application > is working (I can see a captured data from the camera on my board). Can we use > this implementation of the .mmap callback as the correct solution? > > Oleksandr Dmytryshyn | Product Engineering and Development > GlobalLogic > M +38.067.382.2525 > www.globallogic.com > > http://www.globallogic.com/email_disclaimer.txt >
On Fri, Apr 04, 2014 at 02:30:22PM +0300, Oleksandr Dmytryshyn wrote: > >From f52bafc1ba51324ca5044df22778aacf815c59bf Mon Sep 17 00:00:00 2001 > From: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Date: Fri, 4 Apr 2014 12:16:27 +0300 > Subject: [PATCH] swiotlb-xen: implement xen_swiotlb_dma_mmap callback > > This function creates userspace mapping for the DMA-coherent memory. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com> > > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c > index b0e77de..91408b1 100644 > --- a/arch/arm/xen/mm.c > +++ b/arch/arm/xen/mm.c > @@ -48,6 +48,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = { > .sync_sg_for_device = xen_swiotlb_sync_sg_for_device, > .map_sg = xen_swiotlb_map_sg_attrs, > .unmap_sg = xen_swiotlb_unmap_sg_attrs, > + .mmap = xen_swiotlb_dma_mmap, > .map_page = xen_swiotlb_map_page, > .unmap_page = xen_swiotlb_unmap_page, > .dma_supported = xen_swiotlb_dma_supported, > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 5403855..acf0c06 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -407,6 +407,42 @@ dma_addr_t xen_swiotlb_map_page(struct device > *dev, struct page *page, > EXPORT_SYMBOL_GPL(xen_swiotlb_map_page); > > /* > + * Create userspace mapping for the DMA-coherent memory. > + */ > +int xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma, > + void *cpu_addr, dma_addr_t dma_addr, size_t size, > + struct dma_attrs *attrs) Wow. Your editor ate all the nice tabs and spaces. Could you please also run scripts/checkpatch.pl on the patch and fix the issues it would complain about. Thank you! > +{ > + int ret = -ENXIO; > +#ifdef CONFIG_MMU > + unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; > + unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; > + unsigned long pfn = PFN_DOWN(xen_bus_to_phys(dma_addr)); > + unsigned long off = vma->vm_pgoff; > + pgprot_t prot = vma->vm_page_prot; > + > + prot = dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs) ? > + pgprot_writecombine(prot) : > + pgprot_dmacoherent(prot); > + > + vma->vm_page_prot = prot; > + > + if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) > + return ret; > + > + if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) { > + ret = remap_pfn_range(vma, vma->vm_start, > + pfn + off, > + vma->vm_end - vma->vm_start, > + vma->vm_page_prot); > + } > +#endif /* CONFIG_MMU */ > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(xen_swiotlb_dma_mmap); > + > +/* > * Unmap a single streaming mode DMA translation. The dma_addr and size must > * match what was provided for in a previous xen_swiotlb_map_page call. All > * other usages are undefined. > diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h > index 7b64465..930fa94 100644 > --- a/include/xen/swiotlb-xen.h > +++ b/include/xen/swiotlb-xen.h > @@ -15,6 +15,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, > void *vaddr, dma_addr_t dma_handle, > struct dma_attrs *attrs); > > +extern int > +xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma, > + void *cpu_addr, dma_addr_t dma_addr, size_t size, > + struct dma_attrs *attrs); > + > extern dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page, > unsigned long offset, size_t size, > enum dma_data_direction dir,
On Fri, Apr 4, 2014 at 7:26 PM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Fri, Apr 04, 2014 at 02:30:22PM +0300, Oleksandr Dmytryshyn wrote: >> >From f52bafc1ba51324ca5044df22778aacf815c59bf Mon Sep 17 00:00:00 2001 >> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> Date: Fri, 4 Apr 2014 12:16:27 +0300 >> Subject: [PATCH] swiotlb-xen: implement xen_swiotlb_dma_mmap callback >> >> This function creates userspace mapping for the DMA-coherent memory. >> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com> >> >> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c >> index b0e77de..91408b1 100644 >> --- a/arch/arm/xen/mm.c >> +++ b/arch/arm/xen/mm.c >> @@ -48,6 +48,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = { >> .sync_sg_for_device = xen_swiotlb_sync_sg_for_device, >> .map_sg = xen_swiotlb_map_sg_attrs, >> .unmap_sg = xen_swiotlb_unmap_sg_attrs, >> + .mmap = xen_swiotlb_dma_mmap, >> .map_page = xen_swiotlb_map_page, >> .unmap_page = xen_swiotlb_unmap_page, >> .dma_supported = xen_swiotlb_dma_supported, >> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c >> index 5403855..acf0c06 100644 >> --- a/drivers/xen/swiotlb-xen.c >> +++ b/drivers/xen/swiotlb-xen.c >> @@ -407,6 +407,42 @@ dma_addr_t xen_swiotlb_map_page(struct device >> *dev, struct page *page, >> EXPORT_SYMBOL_GPL(xen_swiotlb_map_page); >> >> /* >> + * Create userspace mapping for the DMA-coherent memory. >> + */ >> +int xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma, >> + void *cpu_addr, dma_addr_t dma_addr, size_t size, >> + struct dma_attrs *attrs) > > Wow. Your editor ate all the nice tabs and spaces. Could you > please also run > > scripts/checkpatch.pl on the patch and fix the issues it would > complain about. > > Thank you! >> +{ >> + int ret = -ENXIO; >> +#ifdef CONFIG_MMU >> + unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; >> + unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; >> + unsigned long pfn = PFN_DOWN(xen_bus_to_phys(dma_addr)); >> + unsigned long off = vma->vm_pgoff; >> + pgprot_t prot = vma->vm_page_prot; >> + >> + prot = dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs) ? >> + pgprot_writecombine(prot) : >> + pgprot_dmacoherent(prot); >> + >> + vma->vm_page_prot = prot; >> + >> + if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) >> + return ret; >> + >> + if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) { >> + ret = remap_pfn_range(vma, vma->vm_start, >> + pfn + off, >> + vma->vm_end - vma->vm_start, >> + vma->vm_page_prot); >> + } >> +#endif /* CONFIG_MMU */ >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(xen_swiotlb_dma_mmap); >> + >> +/* >> * Unmap a single streaming mode DMA translation. The dma_addr and size must >> * match what was provided for in a previous xen_swiotlb_map_page call. All >> * other usages are undefined. >> diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h >> index 7b64465..930fa94 100644 >> --- a/include/xen/swiotlb-xen.h >> +++ b/include/xen/swiotlb-xen.h >> @@ -15,6 +15,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, >> void *vaddr, dma_addr_t dma_handle, >> struct dma_attrs *attrs); >> >> +extern int >> +xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma, >> + void *cpu_addr, dma_addr_t dma_addr, size_t size, >> + struct dma_attrs *attrs); >> + >> extern dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page, >> unsigned long offset, size_t size, >> enum dma_data_direction dir, Hi to all. I've sent a patch in another thread (used git send). Oleksandr Dmytryshyn | Product Engineering and Development GlobalLogic M +38.067.382.2525 www.globallogic.com http://www.globallogic.com/email_disclaimer.txt
diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c index 0ce39a3..052a5ed 100644 --- a/drivers/base/dma-mapping.c +++ b/drivers/base/dma-mapping.c @@ -248,7 +248,7 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, #ifdef CONFIG_MMU unsigned long user_count = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT; - unsigned long pfn = page_to_pfn(virt_to_page(cpu_addr)); + unsigned long pfn = dma_addr >> PAGE_SHIFT; unsigned long off = vma->vm_pgoff; vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);