Re: xen-swiotlb issue when NVMe driver is enabled in Dom0 on ARM

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Stefano, Christoph,

> On 22 Apr 2022, at 6:04 am, Christoph Hellwig <hch@xxxxxx> wrote:
> 
> On Thu, Apr 21, 2022 at 03:01:32PM -0700, Stefano Stabellini wrote:
>> swiotlb-xen: handle DMA_ATTR_NO_KERNEL_MAPPING
>> 
>> If DMA_ATTR_NO_KERNEL_MAPPING is set then the returned vaddr is a struct
>> *page instead of the virtual mapping of the buffer.
>> 
>> In xen_swiotlb_alloc_coherent, do not call virt_to_page, instead use the
>> returned pointer directly. Also do not memset the buffer or struct page
>> to zero.
>> 
>> In xen_swiotlb_free_coherent, check DMA_ATTR_NO_KERNEL_MAPPING and set
>> the page pointer appropriately.
> 
> Something like that should work, but it makes swiotlb-xen poke even
> more into the opaque dma-direct internals.  I'd rather do something
> like the patch below that uses the dma_direct allocator directly for
> arm, and simplifies the xen-swiotlb allocator now that it just needs
> to cater to the x86 case:
> 
> diff --git a/arch/arm/include/asm/xen/page-coherent.h 
> b/arch/arm/include/asm/xen/page-coherent.h
> deleted file mode 100644
> index 27e984977402b..0000000000000
> --- a/arch/arm/include/asm/xen/page-coherent.h
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#include <xen/arm/page-coherent.h>
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index a7e54a087b802..6e603e5fdebb1 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -118,23 +118,6 @@ bool xen_arch_need_swiotlb(struct device *dev,
>               !dev_is_dma_coherent(dev));
> }
> 
> -int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
> -                              unsigned int address_bits,
> -                              dma_addr_t *dma_handle)
> -{
> -     if (!xen_initial_domain())
> -             return -EINVAL;
> -
> -     /* we assume that dom0 is mapped 1:1 for now */
> -     *dma_handle = pstart;
> -     return 0;
> -}
> -
> -void xen_destroy_contiguous_region(phys_addr_t pstart, unsigned int order)
> -{
> -     return;
> -}
> -
> static int __init xen_mm_init(void)
> {
>       struct gnttab_cache_flush cflush;
> diff --git a/arch/arm64/include/asm/xen/page-coherent.h 
> b/arch/arm64/include/asm/xen/page-coherent.h
> deleted file mode 100644
> index 27e984977402b..0000000000000
> --- a/arch/arm64/include/asm/xen/page-coherent.h
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#include <xen/arm/page-coherent.h>
> diff --git a/arch/x86/include/asm/xen/page-coherent.h 
> b/arch/x86/include/asm/xen/page-coherent.h
> deleted file mode 100644
> index 63cd41b2e17ac..0000000000000
> --- a/arch/x86/include/asm/xen/page-coherent.h
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef _ASM_X86_XEN_PAGE_COHERENT_H
> -#define _ASM_X86_XEN_PAGE_COHERENT_H
> -
> -#include <asm/page.h>
> -#include <linux/dma-mapping.h>
> -
> -static inline void *xen_alloc_coherent_pages(struct device *hwdev, size_t 
> size,
> -             dma_addr_t *dma_handle, gfp_t flags,
> -             unsigned long attrs)
> -{
> -     void *vstart = (void*)__get_free_pages(flags, get_order(size));
> -     *dma_handle = virt_to_phys(vstart);
> -     return vstart;
> -}
> -
> -static inline void xen_free_coherent_pages(struct device *hwdev, size_t size,
> -             void *cpu_addr, dma_addr_t dma_handle,
> -             unsigned long attrs)
> -{
> -     free_pages((unsigned long) cpu_addr, get_order(size));
> -}
> -
> -#endif /* _ASM_X86_XEN_PAGE_COHERENT_H */
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 47aebd98f52f5..557edb9c54879 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -36,7 +36,6 @@
> #include <xen/hvc-console.h>
> 
> #include <asm/dma-mapping.h>
> -#include <asm/xen/page-coherent.h>
> 
> #include <trace/events/swiotlb.h>
> #define MAX_DMA_BITS 32
> @@ -104,6 +103,7 @@ static int is_xen_swiotlb_buffer(struct device *dev, 
> dma_addr_t dma_addr)
>       return 0;
> }
> 
> +#ifdef CONFIG_X86
> static int xen_swiotlb_fixup(void *buf, unsigned long nslabs)
> {
>       int rc;
> @@ -129,6 +129,12 @@ static int xen_swiotlb_fixup(void *buf, unsigned long 
> nslabs)
>       } while (i < nslabs);
>       return 0;
> }
> +#else
> +static int xen_swiotlb_fixup(void *buf, unsigned long nslabs)
> +{
> +     return 0;
> +}
> +#endif
> 
> enum xen_swiotlb_err {
>       XEN_SWIOTLB_UNKNOWN = 0,
> @@ -256,97 +262,60 @@ void __init xen_swiotlb_init_early(void)
>               panic("Cannot allocate SWIOTLB buffer");
>       swiotlb_set_max_segment(PAGE_SIZE);
> }
> -#endif /* CONFIG_X86 */
> 
> static void *
> -xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size,
> -                        dma_addr_t *dma_handle, gfp_t flags,
> -                        unsigned long attrs)
> +xen_swiotlb_alloc_coherent(struct device *dev, size_t size,
> +             dma_addr_t *dma_handle, gfp_t flags, unsigned long attrs)
> {
> -     void *ret;
> +     u64 dma_mask = dev->coherent_dma_mask;
>       int order = get_order(size);
> -     u64 dma_mask = DMA_BIT_MASK(32);
>       phys_addr_t phys;
> -     dma_addr_t dev_addr;
> -
> -     /*
> -     * Ignore region specifiers - the kernel's ideas of
> -     * pseudo-phys memory layout has nothing to do with the
> -     * machine physical layout.  We can't allocate highmem
> -     * because we can't return a pointer to it.
> -     */
> -     flags &= ~(__GFP_DMA | __GFP_HIGHMEM);
> +     void *ret;
> 
> -     /* Convert the size to actually allocated. */
> +     /* Align the allocation to the Xen page size */
>       size = 1UL << (order + XEN_PAGE_SHIFT);
> 
> -     /* On ARM this function returns an ioremap'ped virtual address for
> -      * which virt_to_phys doesn't return the corresponding physical
> -      * address. In fact on ARM virt_to_phys only works for kernel direct
> -      * mapped RAM memory. Also see comment below.
> -      */
> -     ret = xen_alloc_coherent_pages(hwdev, size, dma_handle, flags, attrs);
> -
> +     ret = (void *)__get_free_pages(flags, get_order(size));
>       if (!ret)
>               return ret;
> -
> -     if (hwdev && hwdev->coherent_dma_mask)
> -             dma_mask = hwdev->coherent_dma_mask;
> -
> -     /* At this point dma_handle is the dma address, next we are
> -      * going to set it to the machine address.
> -      * Do not use virt_to_phys(ret) because on ARM it doesn't correspond
> -      * to *dma_handle. */
> -     phys = dma_to_phys(hwdev, *dma_handle);
> -     dev_addr = xen_phys_to_dma(hwdev, phys);
> -     if (((dev_addr + size - 1 <= dma_mask)) &&
> -         !range_straddles_page_boundary(phys, size))
> -             *dma_handle = dev_addr;
> -     else {
> -             if (xen_create_contiguous_region(phys, order,
> -                                              fls64(dma_mask), dma_handle) 
> != 0) {
> -                     xen_free_coherent_pages(hwdev, size, ret, 
> (dma_addr_t)phys, attrs);
> -                     return NULL;
> -             }
> -             *dma_handle = phys_to_dma(hwdev, *dma_handle);
> +     phys = virt_to_phys(ret);
> +
> +     *dma_handle = xen_phys_to_dma(dev, phys);
> +     if (*dma_handle + size - 1 > dma_mask ||
> +         range_straddles_page_boundary(phys, size)) {
> +             if (xen_create_contiguous_region(phys, order, fls64(dma_mask),
> +                             dma_handle) != 0)
> +                     goto out_free_pages;
>               SetPageXenRemapped(virt_to_page(ret));
>       }
> +
>       memset(ret, 0, size);
>       return ret;
> +
> +out_free_pages:
> +     free_pages((unsigned long)ret, get_order(size));
> +     return NULL;
> }
> 
> static void
> -xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
> -                       dma_addr_t dev_addr, unsigned long attrs)
> +xen_swiotlb_free_coherent(struct device *dev, size_t size, void *vaddr,
> +             dma_addr_t dma_handle, unsigned long attrs)
> {
> +     phys_addr_t phys = virt_to_phys(vaddr);
>       int order = get_order(size);
> -     phys_addr_t phys;
> -     u64 dma_mask = DMA_BIT_MASK(32);
> -     struct page *page;
> -
> -     if (hwdev && hwdev->coherent_dma_mask)
> -             dma_mask = hwdev->coherent_dma_mask;
> -
> -     /* do not use virt_to_phys because on ARM it doesn't return you the
> -      * physical address */
> -     phys = xen_dma_to_phys(hwdev, dev_addr);
> 
>       /* 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_ONCE(dma_handle + size - 1 > dev->coherent_dma_mask) ||
> +         WARN_ON_ONCE(range_straddles_page_boundary(phys, size)))
> +             return;
> 
> -     if (!WARN_ON((dev_addr + size - 1 > dma_mask) ||
> -                  range_straddles_page_boundary(phys, size)) &&
> -         TestClearPageXenRemapped(page))
> +     if (TestClearPageXenRemapped(virt_to_page(vaddr)))
>               xen_destroy_contiguous_region(phys, order);
> -
> -     xen_free_coherent_pages(hwdev, size, vaddr, phys_to_dma(hwdev, phys),
> -                             attrs);
> +     free_pages((unsigned long)vaddr, get_order(size));
> }
> +#endif /* CONFIG_X86 */
> 
> /*
>  * Map a single buffer of the indicated size for DMA in streaming mode.  The
> @@ -549,8 +518,13 @@ xen_swiotlb_dma_supported(struct device *hwdev, u64 mask)
> }
> 
> const struct dma_map_ops xen_swiotlb_dma_ops = {
> +#ifdef CONFIG_X86
>       .alloc = xen_swiotlb_alloc_coherent,
>       .free = xen_swiotlb_free_coherent,
> +#else
> +     .alloc = dma_direct_alloc,
> +     .free = dma_direct_free,
> +#endif
>       .sync_single_for_cpu = xen_swiotlb_sync_single_for_cpu,
>       .sync_single_for_device = xen_swiotlb_sync_single_for_device,
>       .sync_sg_for_cpu = xen_swiotlb_sync_sg_for_cpu,
> diff --git a/include/xen/arm/page-coherent.h b/include/xen/arm/page-coherent.h
> deleted file mode 100644
> index b9cc11e887ed5..0000000000000
> --- a/include/xen/arm/page-coherent.h
> +++ /dev/null
> @@ -1,20 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef _XEN_ARM_PAGE_COHERENT_H
> -#define _XEN_ARM_PAGE_COHERENT_H
> -
> -#include <linux/dma-mapping.h>
> -#include <asm/page.h>
> -
> -static inline void *xen_alloc_coherent_pages(struct device *hwdev, size_t 
> size,
> -             dma_addr_t *dma_handle, gfp_t flags, unsigned long attrs)
> -{
> -     return dma_direct_alloc(hwdev, size, dma_handle, flags, attrs);
> -}
> -
> -static inline void xen_free_coherent_pages(struct device *hwdev, size_t size,
> -             void *cpu_addr, dma_addr_t dma_handle, unsigned long attrs)
> -{
> -     dma_direct_free(hwdev, size, cpu_addr, dma_handle, attrs);
> -}
> -
> -#endif /* _XEN_ARM_PAGE_COHERENT_H */

Thanks for sharing the patch to fix the issue.
I tested both the patches and both the patches work fine.


Regards,
Rahul