| Message ID | 20210616114205.38902-2-roman_skakun@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] Revert "swiotlb-xen: remove xen_swiotlb_dma_mmap and xen_swiotlb_dma_get_sgtable" | expand |
On 6/16/21 7:42 AM, Roman Skakun wrote: > This commit is dedicated to fix incorrect conversion from > cpu_addr to page address in cases when we get virtual > address which allocated through xen_swiotlb_alloc_coherent() > and can be mapped in the vmalloc range. > As the result, virt_to_page() cannot convert this address > properly and return incorrect page address. > > Need to detect such cases and obtains the page address using > vmalloc_to_page() instead. > > The reference code for mmap() and get_sgtable() was copied > from kernel/dma/ops_helpers.c and modified to provide > additional detections as described above. > > In order to simplify code there was added a new > dma_cpu_addr_to_page() helper. > > Signed-off-by: Roman Skakun <roman_skakun@epam.com> > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com> > --- > drivers/xen/swiotlb-xen.c | 42 +++++++++++++++++++++++++++++++-------- > 1 file changed, 34 insertions(+), 8 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 90bc5fc321bc..9331a8500547 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -118,6 +118,14 @@ static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr) > return 0; > } > > +static struct page *cpu_addr_to_page(void *cpu_addr) > +{ > + if (is_vmalloc_addr(cpu_addr)) > + return vmalloc_to_page(cpu_addr); > + else > + return virt_to_page(cpu_addr); > +} > + > static int > xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs) > { > @@ -337,7 +345,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > int order = get_order(size); > phys_addr_t phys; > u64 dma_mask = DMA_BIT_MASK(32); > - struct page *page; > + struct page *page = cpu_addr_to_page(vaddr); > > if (hwdev && hwdev->coherent_dma_mask) > dma_mask = hwdev->coherent_dma_mask; > @@ -349,11 +357,6 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if (is_vmalloc_addr(vaddr)) > - page = vmalloc_to_page(vaddr); > - else > - page = virt_to_page(vaddr); > - > if (!WARN_ON((dev_addr + size - 1 > dma_mask) || > range_straddles_page_boundary(phys, size)) && > TestClearPageXenRemapped(page)) > @@ -573,7 +576,23 @@ 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) > { > - return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size, 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 = cpu_addr_to_page(cpu_addr); > + int ret; > + > + 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 ret; > + > + 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 wonder now whether we could avoid code duplication between here and dma_common_mmap()/dma_common_get_sgtable() and use your helper there. Christoph, would that work? I.e. something like diff --git a/kernel/dma/ops_helpers.c b/kernel/dma/ops_helpers.c index 910ae69cae77..43411c2fa47b 100644 --- a/kernel/dma/ops_helpers.c +++ b/kernel/dma/ops_helpers.c @@ -12,7 +12,7 @@ int dma_common_get_sgtable(struct device *dev, struct sg_table *sgt, void *cpu_addr, dma_addr_t dma_addr, size_t size, unsigned long attrs) { - struct page *page = virt_to_page(cpu_addr); + struct page *page = cpu_addr_to_page(cpu_addr); int ret; ret = sg_alloc_table(sgt, 1, GFP_KERNEL); @@ -43,7 +43,7 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, return -ENXIO; return remap_pfn_range(vma, vma->vm_start, - page_to_pfn(virt_to_page(cpu_addr)) + vma->vm_pgoff, + page_to_pfn(cpu_addr_to_page(cpu_addr)) + vma->vm_pgoff, user_count << PAGE_SHIFT, vma->vm_page_prot); #else return -ENXIO; -boris > > /* > @@ -585,7 +604,14 @@ xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt, > void *cpu_addr, dma_addr_t handle, size_t size, > unsigned long attrs) > { > - return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size, attrs); > + struct page *page = cpu_addr_to_page(cpu_addr); > + int ret; > + > + ret = sg_alloc_table(sgt, 1, GFP_KERNEL); > + if (!ret) > + sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0); > + > + return ret; > } > > const struct dma_map_ops xen_swiotlb_dma_ops = {
On Wed, Jun 16, 2021 at 10:12:55AM -0400, Boris Ostrovsky wrote: > I wonder now whether we could avoid code duplication between here and dma_common_mmap()/dma_common_get_sgtable() and use your helper there. > > > Christoph, would that work? I.e. something like You should not duplicate the code at all, and just make the common helpers work with vmalloc addresses.
On 6/16/21 10:21 AM, Christoph Hellwig wrote: > On Wed, Jun 16, 2021 at 10:12:55AM -0400, Boris Ostrovsky wrote: >> I wonder now whether we could avoid code duplication between here and dma_common_mmap()/dma_common_get_sgtable() and use your helper there. >> >> >> Christoph, would that work? I.e. something like > You should not duplicate the code at all, and just make the common > helpers work with vmalloc addresses. Isn't the expectation of virt_to_page() that it only works on non-vmalloc'd addresses? (This is not a rhetorical question, I actually don't know). -boris
On Wed, Jun 16, 2021 at 11:33:50AM -0400, Boris Ostrovsky wrote:
> Isn't the expectation of virt_to_page() that it only works on non-vmalloc'd addresses? (This is not a rhetorical question, I actually don't know).
Yes. Thus is why I'd suggest to just do the vmalloc_to_page or
virt_to_page dance in ops_helpers.c and just continue using that.
On 6/16/21 11:35 AM, Christoph Hellwig wrote: > On Wed, Jun 16, 2021 at 11:33:50AM -0400, Boris Ostrovsky wrote: >> Isn't the expectation of virt_to_page() that it only works on non-vmalloc'd addresses? (This is not a rhetorical question, I actually don't know). > Yes. Thus is why I'd suggest to just do the vmalloc_to_page or > virt_to_page dance in ops_helpers.c and just continue using that. Ah, OK, so something along the lines of what I suggested. (I thought by "helpers" you meant virt_to_page()). -boris
On Wed, Jun 16, 2021 at 11:39:07AM -0400, Boris Ostrovsky wrote: > > On 6/16/21 11:35 AM, Christoph Hellwig wrote: > > On Wed, Jun 16, 2021 at 11:33:50AM -0400, Boris Ostrovsky wrote: > >> Isn't the expectation of virt_to_page() that it only works on non-vmalloc'd addresses? (This is not a rhetorical question, I actually don't know). > > Yes. Thus is why I'd suggest to just do the vmalloc_to_page or > > virt_to_page dance in ops_helpers.c and just continue using that. > > > Ah, OK, so something along the lines of what I suggested. (I thought by "helpers" you meant virt_to_page()). Yes. Just keeping it contained in the common code without duplicating it into a xen-specific version.
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 90bc5fc321bc..9331a8500547 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -118,6 +118,14 @@ static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr) return 0; } +static struct page *cpu_addr_to_page(void *cpu_addr) +{ + if (is_vmalloc_addr(cpu_addr)) + return vmalloc_to_page(cpu_addr); + else + return virt_to_page(cpu_addr); +} + static int xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs) { @@ -337,7 +345,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, int order = get_order(size); phys_addr_t phys; u64 dma_mask = DMA_BIT_MASK(32); - struct page *page; + struct page *page = cpu_addr_to_page(vaddr); if (hwdev && hwdev->coherent_dma_mask) dma_mask = hwdev->coherent_dma_mask; @@ -349,11 +357,6 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); - if (is_vmalloc_addr(vaddr)) - page = vmalloc_to_page(vaddr); - else - page = virt_to_page(vaddr); - if (!WARN_ON((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size)) && TestClearPageXenRemapped(page)) @@ -573,7 +576,23 @@ 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) { - return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size, 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 = cpu_addr_to_page(cpu_addr); + int ret; + + 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 ret; + + 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); } /* @@ -585,7 +604,14 @@ xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt, void *cpu_addr, dma_addr_t handle, size_t size, unsigned long attrs) { - return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size, attrs); + struct page *page = cpu_addr_to_page(cpu_addr); + int ret; + + ret = sg_alloc_table(sgt, 1, GFP_KERNEL); + if (!ret) + sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0); + + return ret; } const struct dma_map_ops xen_swiotlb_dma_ops = {