Re: [PATCH 11/12] swiotlb: merge swiotlb-xen initialization into swiotlb

Stefano Stabellini
2022-03-02T02:55:53Z

On Tue, 1 Mar 2022, Christoph Hellwig wrote:
> Allow to pass a remap argument to the swiotlb initialization functions
> to handle the Xen/x86 remap case. ARM/ARM64 never did any remapping
> from xen_swiotlb_fixup, so we don't even need that quirk.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> arch/arm/xen/mm.c | 23 +++---
> arch/x86/include/asm/xen/page.h | 5 --
> arch/x86/kernel/pci-dma.c | 19 +++--
> arch/x86/pci/sta2x11-fixup.c | 2 +-
> drivers/xen/swiotlb-xen.c | 128 +-------------------------------
> include/linux/swiotlb.h | 7 +-
> include/xen/arm/page.h | 1 -
> include/xen/swiotlb-xen.h | 8 +-
> kernel/dma/swiotlb.c | 120 +++++++++++++++---------------
> 9 files changed, 96 insertions(+), 217 deletions(-)
>
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index a7e54a087b802..58b40f87617d3 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -23,22 +23,20 @@
> #include <asm/xen/hypercall.h>
> #include <asm/xen/interface.h>
>
> -unsigned long xen_get_swiotlb_free_pages(unsigned int order)
> +static gfp_t xen_swiotlb_gfp(void)
> {
> phys_addr_t base;
> - gfp_t flags = __GFP_NOWARN|__GFP_KSWAPD_RECLAIM;
> u64 i;
>
> for_each_mem_range(i, &base, NULL) {
> if (base < (phys_addr_t)0xffffffff) {
> if (IS_ENABLED(CONFIG_ZONE_DMA32))
> - flags |= __GFP_DMA32;
> - else
> - flags |= __GFP_DMA;
> - break;
> + return __GFP_DMA32;
> + return __GFP_DMA;
> }
> }
> - return __get_free_pages(flags, order);
> +
> + return GFP_KERNEL;
> }

Unrelated to this specific patch series: now that I think about it, if
io_tlb_default_mem.nslabs is already allocated by the time xen_mm_init
is called, wouldn't we potentially have an issue with the GFP flags used
for the earlier allocation (e.g. GFP_DMA32 not used)? Maybe something
for another day.

> static bool hypercall_cflush = false;
> @@ -143,10 +141,15 @@ static int __init xen_mm_init(void)
> if (!xen_swiotlb_detect())
> return 0;
>
> - rc = xen_swiotlb_init();
> /* we can work with the default swiotlb */
> - if (rc < 0 && rc != -EEXIST)
> - return rc;
> + if (!io_tlb_default_mem.nslabs) {
> + if (!xen_initial_domain())
> + return -EINVAL;

I don't think we need this xen_initial_domain() check. It is all
already sorted out by the xen_swiotlb_detect() check above.

Aside from that the rest looks OK. Also, you can add my:

Tested-by: Stefano Stabellini <sstabellini@kernel.org>

> + rc = swiotlb_init_late(swiotlb_size_or_default(),
> + xen_swiotlb_gfp(), NULL);
> + if (rc < 0)
> + return rc;
> + }
>
> cflush.op = 0;
> cflush.a.dev_bus_addr = 0;
> diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
> index e989bc2269f54..1fc67df500145 100644
> --- a/arch/x86/include/asm/xen/page.h
> +++ b/arch/x86/include/asm/xen/page.h
> @@ -357,9 +357,4 @@ static inline bool xen_arch_need_swiotlb(struct device *dev,
> return false;
> }
>
> -static inline unsigned long xen_get_swiotlb_free_pages(unsigned int order)
> -{
> - return __get_free_pages(__GFP_NOWARN, order);
> -}
> -
> #endif /* _ASM_X86_XEN_PAGE_H */
> diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
> index e0def4b1c3181..2f2c468acb955 100644
> --- a/arch/x86/kernel/pci-dma.c
> +++ b/arch/x86/kernel/pci-dma.c
> @@ -71,15 +71,12 @@ static inline void __init pci_swiotlb_detect(void)
> #endif /* CONFIG_SWIOTLB */
>
> #ifdef CONFIG_SWIOTLB_XEN
> -static bool xen_swiotlb;
> -
> static void __init pci_xen_swiotlb_init(void)
> {
> if (!xen_initial_domain() && !x86_swiotlb_enable)
> return;
> x86_swiotlb_enable = false;
> - xen_swiotlb = true;
> - xen_swiotlb_init_early();
> + swiotlb_init_remap(true, x86_swiotlb_flags, xen_swiotlb_fixup);
> dma_ops = &xen_swiotlb_dma_ops;
> if (IS_ENABLED(CONFIG_PCI))
> pci_request_acs();
> @@ -87,14 +84,16 @@ static void __init pci_xen_swiotlb_init(void)
>
> int pci_xen_swiotlb_init_late(void)
> {
> - int rc;
> -
> - if (xen_swiotlb)
> + if (dma_ops == &xen_swiotlb_dma_ops)
> return 0;
>
> - rc = xen_swiotlb_init();
> - if (rc)
> - return rc;
> + /* we can work with the default swiotlb */
> + if (!io_tlb_default_mem.nslabs) {
> + int rc = swiotlb_init_late(swiotlb_size_or_default(),
> + GFP_KERNEL, xen_swiotlb_fixup);
> + if (rc < 0)
> + return rc;
> + }
>
> /* XXX: this switches the dma ops under live devices! */
> dma_ops = &xen_swiotlb_dma_ops;
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index c2da3eb4826e8..df8085b50df10 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -104,7 +104,7 @@ static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr)
> return 0;
> }
>
> -static int xen_swiotlb_fixup(void *buf, unsigned long nslabs)
> +int xen_swiotlb_fixup(void *buf, unsigned long nslabs)
> {
> int rc;
> unsigned int order = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT);
> @@ -130,132 +130,6 @@ static int xen_swiotlb_fixup(void *buf, unsigned long nslabs)
> return 0;
> }
>
> -enum xen_swiotlb_err {
> - XEN_SWIOTLB_UNKNOWN = 0,
> - XEN_SWIOTLB_ENOMEM,
> - XEN_SWIOTLB_EFIXUP
> -};
> -
> -static const char *xen_swiotlb_error(enum xen_swiotlb_err err)
> -{
> - switch (err) {
> - case XEN_SWIOTLB_ENOMEM:
> - return "Cannot allocate Xen-SWIOTLB buffer\n";
> - case XEN_SWIOTLB_EFIXUP:
> - return "Failed to get contiguous memory for DMA from Xen!\n"\
> - "You either: don't have the permissions, do not have"\
> - " enough free memory under 4GB, or the hypervisor memory"\
> - " is too fragmented!";
> - default:
> - break;
> - }
> - return "";
> -}
> -
> -int xen_swiotlb_init(void)
> -{
> - enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
> - unsigned long bytes = swiotlb_size_or_default();
> - unsigned long nslabs = bytes >> IO_TLB_SHIFT;
> - unsigned int order, repeat = 3;
> - int rc = -ENOMEM;
> - char *start;
> -
> - if (io_tlb_default_mem.nslabs) {
> - pr_warn("swiotlb buffer already initialized\n");
> - return -EEXIST;
> - }
> -
> -retry:
> - m_ret = XEN_SWIOTLB_ENOMEM;
> - order = get_order(bytes);
> -
> - /*
> - * Get IO TLB memory from any location.
> - */
> -#define SLABS_PER_PAGE (1 << (PAGE_SHIFT - IO_TLB_SHIFT))
> -#define IO_TLB_MIN_SLABS ((1<<20) >> IO_TLB_SHIFT)
> - while ((SLABS_PER_PAGE << order) > IO_TLB_MIN_SLABS) {
> - start = (void *)xen_get_swiotlb_free_pages(order);
> - if (start)
> - break;
> - order--;
> - }
> - if (!start)
> - goto exit;
> - if (order != get_order(bytes)) {
> - pr_warn("Warning: only able to allocate %ld MB for software IO TLB\n",
> - (PAGE_SIZE << order) >> 20);
> - nslabs = SLABS_PER_PAGE << order;
> - bytes = nslabs << IO_TLB_SHIFT;
> - }
> -
> - /*
> - * And replace that memory with pages under 4GB.
> - */
> - rc = xen_swiotlb_fixup(start, nslabs);
> - if (rc) {
> - free_pages((unsigned long)start, order);
> - m_ret = XEN_SWIOTLB_EFIXUP;
> - goto error;
> - }
> - rc = swiotlb_late_init_with_tbl(start, nslabs);
> - if (rc)
> - return rc;
> - return 0;
> -error:
> - if (nslabs > 1024 && repeat--) {
> - /* Min is 2MB */
> - nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
> - bytes = nslabs << IO_TLB_SHIFT;
> - pr_info("Lowering to %luMB\n", bytes >> 20);
> - goto retry;
> - }
> -exit:
> - pr_err("%s (rc:%d)\n", xen_swiotlb_error(m_ret), rc);
> - return rc;
> -}
> -
> -#ifdef CONFIG_X86
> -void __init xen_swiotlb_init_early(void)
> -{
> - unsigned long bytes = swiotlb_size_or_default();
> - unsigned long nslabs = bytes >> IO_TLB_SHIFT;
> - unsigned int repeat = 3;
> - char *start;
> - int rc;
> -
> -retry:
> - /*
> - * Get IO TLB memory from any location.
> - */
> - start = memblock_alloc(PAGE_ALIGN(bytes),
> - IO_TLB_SEGSIZE << IO_TLB_SHIFT);
> - if (!start)
> - panic("%s: Failed to allocate %lu bytes\n",
> - __func__, PAGE_ALIGN(bytes));
> -
> - /*
> - * And replace that memory with pages under 4GB.
> - */
> - rc = xen_swiotlb_fixup(start, nslabs);
> - if (rc) {
> - memblock_free(start, PAGE_ALIGN(bytes));
> - if (nslabs > 1024 && repeat--) {
> - /* Min is 2MB */
> - nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
> - bytes = nslabs << IO_TLB_SHIFT;
> - pr_info("Lowering to %luMB\n", bytes >> 20);
> - goto retry;
> - }
> - panic("%s (rc:%d)", xen_swiotlb_error(XEN_SWIOTLB_EFIXUP), rc);
> - }
> -
> - if (swiotlb_init_with_tbl(start, nslabs, SWIOTLB_VERBOSE))
> - panic("Cannot allocate SWIOTLB buffer");
> -}
> -#endif /* CONFIG_X86 */
> -
> static void *
> xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size,
> dma_addr_t *dma_handle, gfp_t flags,
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index ee655f2e4d28b..919cf82ed978e 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -34,10 +34,11 @@ struct scatterlist;
> /* default to 64MB */
> #define IO_TLB_DEFAULT_SIZE (64UL<<20)
>
> -int swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, unsigned int flags);
> unsigned long swiotlb_size_or_default(void);
> -extern int swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs);
> -int swiotlb_init_late(size_t size, gfp_t gfp_mask);
> +int swiotlb_init_late(size_t size, gfp_t gfp_mask,
> + int (*remap)(void *tlb, unsigned long nslabs));
> +void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
> + int (*remap)(void *tlb, unsigned long nslabs));
> extern void __init swiotlb_update_mem_attributes(void);
>
> phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t phys,
> diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h
> index ac1b654705631..7e199c6656b90 100644
> --- a/include/xen/arm/page.h
> +++ b/include/xen/arm/page.h
> @@ -115,6 +115,5 @@ static inline bool set_phys_to_machine(unsigned long pfn, unsigned long mfn)
> bool xen_arch_need_swiotlb(struct device *dev,
> phys_addr_t phys,
> dma_addr_t dev_addr);
> -unsigned long xen_get_swiotlb_free_pages(unsigned int order);
> #endif /* _ASM_ARM_XEN_PAGE_H */
> diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
> index b3e647f86e3e2..590ceb923f0c8 100644
> --- a/include/xen/swiotlb-xen.h
> +++ b/include/xen/swiotlb-xen.h
> @@ -10,8 +10,12 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
> void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
> size_t size, enum dma_data_direction dir);
>
> -int xen_swiotlb_init(void);
> -void __init xen_swiotlb_init_early(void);
> +#ifdef CONFIG_SWIOTLB_XEN
> +int xen_swiotlb_fixup(void *buf, unsigned long nslabs);
> +#else
> +#define xen_swiotlb_fixup NULL
> +#endif
> +
> extern const struct dma_map_ops xen_swiotlb_dma_ops;
>
> #endif /* __LINUX_SWIOTLB_XEN_H */
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 77cf73dc20a78..128363dc9b5bb 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -234,40 +234,17 @@ static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t start,
> return;
> }
>
> -int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs,
> - unsigned int flags)
> -{
> - struct io_tlb_mem *mem = &io_tlb_default_mem;
> - size_t alloc_size;
> -
> - if (swiotlb_force_disable)
> - return 0;
> -
> - /* protect against double initialization */
> - if (WARN_ON_ONCE(mem->nslabs))
> - return -ENOMEM;
> -
> - alloc_size = PAGE_ALIGN(array_size(sizeof(*mem->slots), nslabs));
> - mem->slots = memblock_alloc(alloc_size, PAGE_SIZE);
> - if (!mem->slots)
> - panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
> - __func__, alloc_size, PAGE_SIZE);
> -
> - swiotlb_init_io_tlb_mem(mem, __pa(tlb), nslabs, false);
> - mem->force_bounce = flags & SWIOTLB_FORCE;
> -
> - if (flags & SWIOTLB_VERBOSE)
> - swiotlb_print_info();
> - return 0;
> -}
> -
> /*
> * Statically reserve bounce buffer space and initialize bounce buffer data
> * structures for the software IO TLB used to implement the DMA API.
> */
> -void __init swiotlb_init(bool addressing_limit, unsigned int flags)
> +void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
> + int (*remap)(void *tlb, unsigned long nslabs))
> {
> - size_t bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
> + struct io_tlb_mem *mem = &io_tlb_default_mem;
> + unsigned long nslabs = default_nslabs;
> + size_t alloc_size = PAGE_ALIGN(array_size(sizeof(*mem->slots), nslabs));
> + size_t bytes;
> void *tlb;
>
> if (!addressing_limit && !swiotlb_force_bounce)
> @@ -275,23 +252,48 @@ void __init swiotlb_init(bool addressing_limit, unsigned int flags)
> if (swiotlb_force_disable)
> return;
>
> + /* protect against double initialization */
> + if (WARN_ON_ONCE(mem->nslabs))
> + return;
> +
> /*
> * By default allocate the bonuce buffer memory from low memory.
> */
> +retry:
> + bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
> if (flags & SWIOTLB_ANY)
> tlb = memblock_alloc(bytes, PAGE_SIZE);
> else
> tlb = memblock_alloc_low(bytes, PAGE_SIZE);
> if (!tlb)
> - goto fail;
> - if (swiotlb_init_with_tbl(tlb, default_nslabs, flags))
> - goto fail_free_mem;
> - return;
> + panic("%s: failed to allocate tlb structure\n", __func__);
> +
> + if (remap && remap(tlb, nslabs) < 0) {
> + memblock_free(tlb, PAGE_ALIGN(bytes));
> +
> + /* Min is 2MB */
> + if (nslabs <= 1024)
> + panic("%s: Failed to remap %zu bytes\n",
> + __func__, bytes);
> + nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
> + goto retry;
> + }
> +
> + mem->slots = memblock_alloc(alloc_size, PAGE_SIZE);
> + if (!mem->slots)
> + panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
> + __func__, alloc_size, PAGE_SIZE);
>
> -fail_free_mem:
> - memblock_free(tlb, bytes);
> -fail:
> - pr_warn("Cannot allocate buffer");
> + swiotlb_init_io_tlb_mem(mem, __pa(tlb), default_nslabs, false);
> + mem->force_bounce = flags & SWIOTLB_FORCE;
> +
> + if (flags & SWIOTLB_VERBOSE)
> + swiotlb_print_info();
> +}
> +
> +void __init swiotlb_init(bool addressing_limit, unsigned int flags)
> +{
> + return swiotlb_init_remap(addressing_limit, flags, NULL);
> }
>
> /*
> @@ -299,8 +301,10 @@ void __init swiotlb_init(bool addressing_limit, unsigned int flags)
> * initialize the swiotlb later using the slab allocator if needed.
> * This should be just like above, but with some error catching.
> */
> -int swiotlb_init_late(size_t size, gfp_t gfp_mask)
> +int swiotlb_init_late(size_t size, gfp_t gfp_mask,
> + int (*remap)(void *tlb, unsigned long nslabs))
> {
> + struct io_tlb_mem *mem = &io_tlb_default_mem;
> unsigned long nslabs = ALIGN(size >> IO_TLB_SHIFT, IO_TLB_SEGSIZE);
> unsigned long bytes;
> unsigned char *vstart = NULL;
> @@ -310,9 +314,14 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask)
> if (swiotlb_force_disable)
> return 0;
>
> + /* protect against double initialization */
> + if (WARN_ON_ONCE(mem->nslabs))
> + return -ENOMEM;
> +
> /*
> * Get IO TLB memory from the low pages
> */
> +retry:
> order = get_order(nslabs << IO_TLB_SHIFT);
> nslabs = SLABS_PER_PAGE << order;
> bytes = nslabs << IO_TLB_SHIFT;
> @@ -333,33 +342,28 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask)
> (PAGE_SIZE << order) >> 20);
> nslabs = SLABS_PER_PAGE << order;
> }
> - rc = swiotlb_late_init_with_tbl(vstart, nslabs);
> - if (rc)
> - free_pages((unsigned long)vstart, order);
> -
> - return rc;
> -}
> -
> -int
> -swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
> -{
> - struct io_tlb_mem *mem = &io_tlb_default_mem;
> - unsigned long bytes = nslabs << IO_TLB_SHIFT;
>
> - if (swiotlb_force_disable)
> - return 0;
> + if (remap)
> + rc = remap(vstart, nslabs);
> + if (rc) {
> + free_pages((unsigned long)vstart, order);
>
> - /* protect against double initialization */
> - if (WARN_ON_ONCE(mem->nslabs))
> - return -ENOMEM;
> + /* Min is 2MB */
> + if (nslabs <= 1024)
> + return rc;
> + nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
> + goto retry;
> + }
>
> mem->slots = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> get_order(array_size(sizeof(*mem->slots), nslabs)));
> - if (!mem->slots)
> + if (!mem->slots) {
> + free_pages((unsigned long)vstart, order);
> return -ENOMEM;
> + }
>
> - set_memory_decrypted((unsigned long)tlb, bytes >> PAGE_SHIFT);
> - swiotlb_init_io_tlb_mem(mem, virt_to_phys(tlb), nslabs, true);
> + set_memory_decrypted((unsigned long)vstart, bytes >> PAGE_SHIFT);
> + swiotlb_init_io_tlb_mem(mem, virt_to_phys(vstart), nslabs, true);
>
> swiotlb_print_info();
> return 0;
> --
> 2.30.2
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>