From ce6a80d1b2f923b1839655a1cda786293feaa085 Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Tue, 7 Sep 2021 14:04:25 +0200 Subject: swiotlb-xen: avoid double free Of the two paths leading to the "error" label in xen_swiotlb_init() one didn't allocate anything, while the other did already free what was allocated. Fixes: b82776005369 ("xen/swiotlb: Use the swiotlb_late_init_with_tbl to init Xen-SWIOTLB late when PV PCI is used") Signed-off-by: Jan Beulich Cc: stable@vger.kernel.org Reviewed-by: Christoph Hellwig Link: https://lore.kernel.org/r/ce9c2adb-8a52-6293-982a-0d6ece943ac6@suse.com Signed-off-by: Juergen Gross --- drivers/xen/swiotlb-xen.c | 1 - 1 file changed, 1 deletion(-) (limited to 'drivers/xen/swiotlb-xen.c') diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 24d11861ac7d..99d518526eaf 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -216,7 +216,6 @@ error: goto retry; } pr_err("%s (rc:%d)\n", xen_swiotlb_error(m_ret), rc); - free_pages((unsigned long)start, order); return rc; } -- cgit v1.2.3 From 4c092c59015f7adf0f07685f869edb96d997a756 Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Tue, 7 Sep 2021 14:04:47 +0200 Subject: swiotlb-xen: fix late init retry The commit referenced below removed the assignment of "bytes" from xen_swiotlb_init() without - like done for xen_swiotlb_init_early() - adding an assignment on the retry path, thus leading to excessively sized allocations upon retries. Fixes: 2d29960af0be ("swiotlb: dynamically allocate io_tlb_default_mem") Signed-off-by: Jan Beulich Cc: stable@vger.kernel.org Reviewed-by: Christoph Hellwig Link: https://lore.kernel.org/r/778299d6-9cfd-1c13-026e-25ee5d14ecb3@suse.com Signed-off-by: Juergen Gross --- drivers/xen/swiotlb-xen.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/xen/swiotlb-xen.c') diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 99d518526eaf..dbb18dc956f3 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -211,8 +211,8 @@ error: if (repeat--) { /* Min is 2MB */ nslabs = max(1024UL, (nslabs >> 1)); - pr_info("Lowering to %luMB\n", - (nslabs << IO_TLB_SHIFT) >> 20); + bytes = nslabs << IO_TLB_SHIFT; + pr_info("Lowering to %luMB\n", bytes >> 20); goto retry; } pr_err("%s (rc:%d)\n", xen_swiotlb_error(m_ret), rc); -- cgit v1.2.3 From d9a688add3d412c840e31060847a6a297b552316 Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Tue, 7 Sep 2021 14:05:12 +0200 Subject: swiotlb-xen: maintain slab count properly Generic swiotlb code makes sure to keep the slab count a multiple of the number of slabs per segment. Yet even without checking whether any such assumption is made elsewhere, it is easy to see that xen_swiotlb_fixup() might alter unrelated memory when calling xen_create_contiguous_region() for the last segment, when that's not a full one - the function acts on full order-N regions, not individual pages. Align the slab count suitably when halving it for a retry. Add a build time check and a runtime one. Replace the no longer useful local variable "slabs" by an "order" one calculated just once, outside of the loop. Re-use "order" for calculating "dma_bits", and change the type of the latter as well as the one of "i" while touching this anyway. Signed-off-by: Jan Beulich Reviewed-by: Christoph Hellwig Link: https://lore.kernel.org/r/dc054cb0-bec4-4db0-fc06-c9fc957b6e66@suse.com Signed-off-by: Juergen Gross --- drivers/xen/swiotlb-xen.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) (limited to 'drivers/xen/swiotlb-xen.c') diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index dbb18dc956f3..08fc694f05b7 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -106,27 +106,26 @@ static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr) static int xen_swiotlb_fixup(void *buf, unsigned long nslabs) { - int i, rc; - int dma_bits; + int rc; + unsigned int order = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT); + unsigned int i, dma_bits = order + PAGE_SHIFT; dma_addr_t dma_handle; phys_addr_t p = virt_to_phys(buf); - dma_bits = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT) + PAGE_SHIFT; + BUILD_BUG_ON(IO_TLB_SEGSIZE & (IO_TLB_SEGSIZE - 1)); + BUG_ON(nslabs % IO_TLB_SEGSIZE); i = 0; do { - int slabs = min(nslabs - i, (unsigned long)IO_TLB_SEGSIZE); - do { rc = xen_create_contiguous_region( - p + (i << IO_TLB_SHIFT), - get_order(slabs << IO_TLB_SHIFT), + p + (i << IO_TLB_SHIFT), order, dma_bits, &dma_handle); } while (rc && dma_bits++ < MAX_DMA_BITS); if (rc) return rc; - i += slabs; + i += IO_TLB_SEGSIZE; } while (i < nslabs); return 0; } @@ -210,7 +209,7 @@ retry: error: if (repeat--) { /* Min is 2MB */ - nslabs = max(1024UL, (nslabs >> 1)); + nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE)); bytes = nslabs << IO_TLB_SHIFT; pr_info("Lowering to %luMB\n", bytes >> 20); goto retry; @@ -245,7 +244,7 @@ retry: memblock_free(__pa(start), PAGE_ALIGN(bytes)); if (repeat--) { /* Min is 2MB */ - nslabs = max(1024UL, (nslabs >> 1)); + nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE)); bytes = nslabs << IO_TLB_SHIFT; pr_info("Lowering to %luMB\n", bytes >> 20); goto retry; -- cgit v1.2.3 From 79ca5f778aafbd69727d577b58d913c9ce8400be Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Tue, 7 Sep 2021 14:05:54 +0200 Subject: swiotlb-xen: suppress certain init retries Only on the 2nd of the paths leading to xen_swiotlb_init()'s "error" label it is useful to retry the allocation; the first one did already iterate through all possible order values. Signed-off-by: Jan Beulich Reviewed-by: Christoph Hellwig Link: https://lore.kernel.org/r/56477481-87da-4962-9661-5e1b277efde0@suse.com Signed-off-by: Juergen Gross --- drivers/xen/swiotlb-xen.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers/xen/swiotlb-xen.c') diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 08fc694f05b7..6520d05e62ef 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -184,7 +184,7 @@ retry: order--; } if (!start) - goto error; + 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); @@ -214,6 +214,7 @@ error: 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; } -- cgit v1.2.3 From cabb7f89b24ed40c4640dac3bca044452ab0b386 Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Tue, 7 Sep 2021 14:06:37 +0200 Subject: swiotlb-xen: limit init retries Due to the use of max(1024, ...) there's no point retrying (and issuing bogus log messages) when the number of slabs is already no larger than this minimum value. Signed-off-by: Jan Beulich Reviewed-by: Christoph Hellwig Link: https://lore.kernel.org/r/984fa426-2b7b-4b77-5ce8-766619575b7f@suse.com Signed-off-by: Juergen Gross --- drivers/xen/swiotlb-xen.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/xen/swiotlb-xen.c') diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 6520d05e62ef..d30dc5e68a22 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -207,7 +207,7 @@ retry: swiotlb_set_max_segment(PAGE_SIZE); return 0; error: - if (repeat--) { + if (nslabs > 1024 && repeat--) { /* Min is 2MB */ nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE)); bytes = nslabs << IO_TLB_SHIFT; @@ -243,7 +243,7 @@ retry: rc = xen_swiotlb_fixup(start, nslabs); if (rc) { memblock_free(__pa(start), PAGE_ALIGN(bytes)); - if (repeat--) { + if (nslabs > 1024 && repeat--) { /* Min is 2MB */ nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE)); bytes = nslabs << IO_TLB_SHIFT; -- cgit v1.2.3 From 68573c1b5c4d0ebd90c24c6055abb53c474a8fc2 Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Tue, 7 Sep 2021 14:06:55 +0200 Subject: swiotlb-xen: drop leftover __ref Commit a98f565462f0 ("xen-swiotlb: split xen_swiotlb_init") should not only have added __init to the split off function, but also should have dropped __ref from the one left. Signed-off-by: Jan Beulich Reviewed-by: Christoph Hellwig Link: https://lore.kernel.org/r/7cd163e1-fe13-270b-384c-2708e8273d34@suse.com Signed-off-by: Juergen Gross --- drivers/xen/swiotlb-xen.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/xen/swiotlb-xen.c') diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index d30dc5e68a22..f3d81b2697b7 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -154,7 +154,7 @@ static const char *xen_swiotlb_error(enum xen_swiotlb_err err) #define DEFAULT_NSLABS ALIGN(SZ_64M >> IO_TLB_SHIFT, IO_TLB_SEGSIZE) -int __ref xen_swiotlb_init(void) +int xen_swiotlb_init(void) { enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN; unsigned long bytes = swiotlb_size_or_default(); -- cgit v1.2.3 From 7fd880a38cfefb2d54a912e4ae809110adec1c94 Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Tue, 7 Sep 2021 14:07:21 +0200 Subject: swiotlb-xen: arrange to have buffer info logged I consider it unhelpful that address and size of the buffer aren't put in the log file; it makes diagnosing issues needlessly harder. The majority of callers of swiotlb_init() also passes 1 for the "verbose" parameter. Signed-off-by: Jan Beulich Reviewed-by: Christoph Hellwig Link: https://lore.kernel.org/r/2e3c8e68-36b2-4ae9-b829-bf7f75d39d47@suse.com Signed-off-by: Juergen Gross --- drivers/xen/swiotlb-xen.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/xen/swiotlb-xen.c') diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index f3d81b2697b7..afe8c7eb31b9 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -253,7 +253,7 @@ retry: panic("%s (rc:%d)", xen_swiotlb_error(XEN_SWIOTLB_EFIXUP), rc); } - if (swiotlb_init_with_tbl(start, nslabs, false)) + if (swiotlb_init_with_tbl(start, nslabs, true)) panic("Cannot allocate SWIOTLB buffer"); swiotlb_set_max_segment(PAGE_SIZE); } -- cgit v1.2.3 From d859ed25b24289c87a97889653596f8088367e16 Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Tue, 7 Sep 2021 14:07:47 +0200 Subject: swiotlb-xen: drop DEFAULT_NSLABS It was introduced by 4035b43da6da ("xen-swiotlb: remove xen_set_nslabs") and then not removed by 2d29960af0be ("swiotlb: dynamically allocate io_tlb_default_mem"). Signed-off-by: Jan Beulich Reviewed-by: Christoph Hellwig Link: https://lore.kernel.org/r/15259326-209a-1d11-338c-5018dc38abe8@suse.com Signed-off-by: Juergen Gross --- drivers/xen/swiotlb-xen.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'drivers/xen/swiotlb-xen.c') diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index afe8c7eb31b9..9c9ba500ef23 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -152,8 +152,6 @@ static const char *xen_swiotlb_error(enum xen_swiotlb_err err) return ""; } -#define DEFAULT_NSLABS ALIGN(SZ_64M >> IO_TLB_SHIFT, IO_TLB_SEGSIZE) - int xen_swiotlb_init(void) { enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN; -- cgit v1.2.3