Re: [PATCH] swiotlb-xen: override common mmap and get_sgtable dma ops

[Roman Skakun <rm.skakun@xxxxxxxxx>]

Hey, Boris!
Thanks for the review.

I have an additional question about current implementation that disturbed me.
I think, that we can have cases when mapped memory can not be
physically contiguous.
In order to proceed this correctly need to apply some additional steps
to current implementation as well.

In mmap() :
1. Is the memory region physically contiguous?
2. Remap multiple ranges if it is not.

In get_sgtable() :
1. Is the memory region physically contiguous?
2. Create sgt that will be included multiple contiguous ranges if it is not.

What do you think about it?

Cheers!
Roman


пт, 11 июн. 2021 г. в 18:20, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>:
>
>
> On 6/11/21 5:55 AM, Roman Skakun wrote:
> >
> > +static int
> > +xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
> > +             void *cpu_addr, dma_addr_t dma_addr, size_t size,
> > +             unsigned long attrs)
> > +{
> > +     unsigned long user_count = vma_pages(vma);
> > +     unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> > +     unsigned long off = vma->vm_pgoff;
> > +     struct page *page;
> > +
> > +     if (is_vmalloc_addr(cpu_addr))
> > +             page = vmalloc_to_page(cpu_addr);
> > +     else
> > +             page = virt_to_page(cpu_addr);
> > +
> > +     vma->vm_page_prot = dma_pgprot(dev, vma->vm_page_prot, attrs);
> > +
> > +     if (dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
> > +             return -ENXIO;
> > +
> > +     if (off >= count || user_count > count - off)
> > +             return -ENXIO;
> > +
> > +     return remap_pfn_range(vma, vma->vm_start,
> > +                     page_to_pfn(page) + vma->vm_pgoff,
> > +                     user_count << PAGE_SHIFT, vma->vm_page_prot);
> > +}
>
>
> I suggest you create a helper for computing page value and then revert 
> 922659ea771b3fd728149262c5ea15608fab9719 and pass result of the helper 
> instead of cpu_addr. Here and in xen_swiotlb_dma_get_sgtable().
>
>
> And use this new helper in xen_swiotlb_free_coherent() too. I am curious 
> though why this was not a problem when Stefano was looking at the problem 
> that introduced this vmalloc check (i.e. 
> 8b1e868f66076490189a36d984fcce286cdd6295). Stefano?
>
>
> -boris



-- 
Best Regards, Roman.