Re: [Xen-devel] swiotlb-xen question

[Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>]

On Thu, Apr 3, 2014 at 3:52 PM, Stefano Stabellini
<stefano.stabellini@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel