LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 00/12] swiotlb-xen: fixes and adjustments
@ 2021-09-07 12:03 Jan Beulich
  2021-09-07 12:04 ` [PATCH 01/12] swiotlb-xen: avoid double free Jan Beulich
                   ` (12 more replies)
  0 siblings, 13 replies; 45+ messages in thread
From: Jan Beulich @ 2021-09-07 12:03 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, lkml, xen-devel

The primary intention really was the last patch, there you go ...

01: swiotlb-xen: avoid double free
02: swiotlb-xen: fix late init retry
03: swiotlb-xen: maintain slab count properly
04: swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
05: swiotlb-xen: suppress certain init retries
06: swiotlb-xen: limit init retries
07: swiotlb-xen: drop leftover __ref
08: swiotlb-xen: arrange to have buffer info logged
09: swiotlb-xen: drop DEFAULT_NSLABS
10: xen-pcifront: this module is PV-only
11: xen/pci-swiotlb: reduce visibility of symbols
12: swiotlb-xen: this is PV-only on x86

Jan


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 01/12] swiotlb-xen: avoid double free
  2021-09-07 12:03 [PATCH 00/12] swiotlb-xen: fixes and adjustments Jan Beulich
@ 2021-09-07 12:04 ` Jan Beulich
  2021-09-08  6:48   ` Christoph Hellwig
  2021-09-10 22:55   ` Stefano Stabellini
  2021-09-07 12:04 ` [PATCH 02/12] swiotlb-xen: fix late init retry Jan Beulich
                   ` (11 subsequent siblings)
  12 siblings, 2 replies; 45+ messages in thread
From: Jan Beulich @ 2021-09-07 12:04 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, lkml, xen-devel

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 <jbeulich@suse.com>
Cc: stable@vger.kernel.org

--- 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;
 }
 


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 02/12] swiotlb-xen: fix late init retry
  2021-09-07 12:03 [PATCH 00/12] swiotlb-xen: fixes and adjustments Jan Beulich
  2021-09-07 12:04 ` [PATCH 01/12] swiotlb-xen: avoid double free Jan Beulich
@ 2021-09-07 12:04 ` Jan Beulich
  2021-09-08  6:50   ` Christoph Hellwig
  2021-09-07 12:05 ` [PATCH 03/12] swiotlb-xen: maintain slab count properly Jan Beulich
                   ` (10 subsequent siblings)
  12 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2021-09-07 12:04 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, lkml, xen-devel

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 <jbeulich@suse.com>
Cc: stable@vger.kernel.org

--- 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);


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 03/12] swiotlb-xen: maintain slab count properly
  2021-09-07 12:03 [PATCH 00/12] swiotlb-xen: fixes and adjustments Jan Beulich
  2021-09-07 12:04 ` [PATCH 01/12] swiotlb-xen: avoid double free Jan Beulich
  2021-09-07 12:04 ` [PATCH 02/12] swiotlb-xen: fix late init retry Jan Beulich
@ 2021-09-07 12:05 ` Jan Beulich
  2021-09-08  6:53   ` Christoph Hellwig
  2021-09-10 23:10   ` Stefano Stabellini
  2021-09-07 12:05 ` [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests Jan Beulich
                   ` (9 subsequent siblings)
  12 siblings, 2 replies; 45+ messages in thread
From: Jan Beulich @ 2021-09-07 12:05 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, lkml, xen-devel

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 <jbeulich@suse.com>

--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -106,27 +106,26 @@ static int is_xen_swiotlb_buffer(struct
 
 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;


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
  2021-09-07 12:03 [PATCH 00/12] swiotlb-xen: fixes and adjustments Jan Beulich
                   ` (2 preceding siblings ...)
  2021-09-07 12:05 ` [PATCH 03/12] swiotlb-xen: maintain slab count properly Jan Beulich
@ 2021-09-07 12:05 ` Jan Beulich
  2021-09-08  6:57   ` Christoph Hellwig
  2021-09-10 23:14   ` Stefano Stabellini
  2021-09-07 12:05 ` [PATCH 05/12] swiotlb-xen: suppress certain init retries Jan Beulich
                   ` (8 subsequent siblings)
  12 siblings, 2 replies; 45+ messages in thread
From: Jan Beulich @ 2021-09-07 12:05 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, lkml, xen-devel

While the hypervisor hasn't been enforcing this, we would still better
avoid issuing requests with GFNs not aligned to the requested order.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I wonder how useful it is to include the alignment in the panic()
message. I further wonder how useful it is to wrap "bytes" in
PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
least was supposed to be, prior to "swiotlb-xen: maintain slab count
properly").

--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -231,10 +231,10 @@ retry:
 	/*
 	 * Get IO TLB memory from any location.
 	 */
-	start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE);
+	start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
 	if (!start)
-		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
-		      __func__, PAGE_ALIGN(bytes), PAGE_SIZE);
+		panic("%s: Failed to allocate %lu bytes align=%#x\n",
+		      __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
 
 	/*
 	 * And replace that memory with pages under 4GB.


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 05/12] swiotlb-xen: suppress certain init retries
  2021-09-07 12:03 [PATCH 00/12] swiotlb-xen: fixes and adjustments Jan Beulich
                   ` (3 preceding siblings ...)
  2021-09-07 12:05 ` [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests Jan Beulich
@ 2021-09-07 12:05 ` Jan Beulich
  2021-09-08  6:58   ` Christoph Hellwig
  2021-09-10 23:18   ` Stefano Stabellini
  2021-09-07 12:06 ` [PATCH 06/12] swiotlb-xen: limit " Jan Beulich
                   ` (7 subsequent siblings)
  12 siblings, 2 replies; 45+ messages in thread
From: Jan Beulich @ 2021-09-07 12:05 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, lkml, xen-devel

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 <jbeulich@suse.com>
---
I'm not convinced of the (lack of) indentation of the label, but I made
the new one matzch the existing one.

--- 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;
 }


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 06/12] swiotlb-xen: limit init retries
  2021-09-07 12:03 [PATCH 00/12] swiotlb-xen: fixes and adjustments Jan Beulich
                   ` (4 preceding siblings ...)
  2021-09-07 12:05 ` [PATCH 05/12] swiotlb-xen: suppress certain init retries Jan Beulich
@ 2021-09-07 12:06 ` Jan Beulich
  2021-09-08  7:00   ` Christoph Hellwig
  2021-09-10 23:23   ` Stefano Stabellini
  2021-09-07 12:06 ` [PATCH 07/12] swiotlb-xen: drop leftover __ref Jan Beulich
                   ` (6 subsequent siblings)
  12 siblings, 2 replies; 45+ messages in thread
From: Jan Beulich @ 2021-09-07 12:06 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, lkml, xen-devel

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 <jbeulich@suse.com>

--- 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;


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 07/12] swiotlb-xen: drop leftover __ref
  2021-09-07 12:03 [PATCH 00/12] swiotlb-xen: fixes and adjustments Jan Beulich
                   ` (5 preceding siblings ...)
  2021-09-07 12:06 ` [PATCH 06/12] swiotlb-xen: limit " Jan Beulich
@ 2021-09-07 12:06 ` Jan Beulich
  2021-09-08  7:02   ` Christoph Hellwig
  2021-09-10 23:24   ` Stefano Stabellini
  2021-09-07 12:07 ` [PATCH 08/12] swiotlb-xen: arrange to have buffer info logged Jan Beulich
                   ` (5 subsequent siblings)
  12 siblings, 2 replies; 45+ messages in thread
From: Jan Beulich @ 2021-09-07 12:06 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, lkml, xen-devel

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 <jbeulich@suse.com>

--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -154,7 +154,7 @@ static const char *xen_swiotlb_error(enu
 
 #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();


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 08/12] swiotlb-xen: arrange to have buffer info logged
  2021-09-07 12:03 [PATCH 00/12] swiotlb-xen: fixes and adjustments Jan Beulich
                   ` (6 preceding siblings ...)
  2021-09-07 12:06 ` [PATCH 07/12] swiotlb-xen: drop leftover __ref Jan Beulich
@ 2021-09-07 12:07 ` Jan Beulich
  2021-09-08  7:04   ` Christoph Hellwig
  2021-09-10 23:26   ` Stefano Stabellini
  2021-09-07 12:07 ` [PATCH 09/12] swiotlb-xen: drop DEFAULT_NSLABS Jan Beulich
                   ` (4 subsequent siblings)
  12 siblings, 2 replies; 45+ messages in thread
From: Jan Beulich @ 2021-09-07 12:07 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, lkml, xen-devel

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 <jbeulich@suse.com>

--- 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);
 }


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 09/12] swiotlb-xen: drop DEFAULT_NSLABS
  2021-09-07 12:03 [PATCH 00/12] swiotlb-xen: fixes and adjustments Jan Beulich
                   ` (7 preceding siblings ...)
  2021-09-07 12:07 ` [PATCH 08/12] swiotlb-xen: arrange to have buffer info logged Jan Beulich
@ 2021-09-07 12:07 ` Jan Beulich
  2021-09-08  7:06   ` Christoph Hellwig
  2021-09-10 23:27   ` Stefano Stabellini
  2021-09-07 12:11 ` [PATCH 11/12] xen/pci-swiotlb: reduce visibility of symbols Jan Beulich
                   ` (3 subsequent siblings)
  12 siblings, 2 replies; 45+ messages in thread
From: Jan Beulich @ 2021-09-07 12:07 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, lkml, xen-devel

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 <jbeulich@suse.com>

--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -152,8 +152,6 @@ static const char *xen_swiotlb_error(enu
 	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;


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 11/12] xen/pci-swiotlb: reduce visibility of symbols
  2021-09-07 12:03 [PATCH 00/12] swiotlb-xen: fixes and adjustments Jan Beulich
                   ` (8 preceding siblings ...)
  2021-09-07 12:07 ` [PATCH 09/12] swiotlb-xen: drop DEFAULT_NSLABS Jan Beulich
@ 2021-09-07 12:11 ` Jan Beulich
  2021-09-08  7:08   ` Christoph Hellwig
  2021-09-07 12:13 ` [PATCH 12/12] swiotlb-xen: this is PV-only on x86 Jan Beulich
                   ` (2 subsequent siblings)
  12 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2021-09-07 12:11 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky; +Cc: Stefano Stabellini, lkml, xen-devel

xen_swiotlb and pci_xen_swiotlb_init() are only used within the file
defining them, so make them static and remove the stubs. Otoh
pci_xen_swiotlb_detect() has a use (as function pointer) from the main
pci-swiotlb.c file - convert its stub to a #define to NULL.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/arch/x86/include/asm/xen/swiotlb-xen.h
+++ b/arch/x86/include/asm/xen/swiotlb-xen.h
@@ -3,14 +3,10 @@
 #define _ASM_X86_SWIOTLB_XEN_H
 
 #ifdef CONFIG_SWIOTLB_XEN
-extern int xen_swiotlb;
 extern int __init pci_xen_swiotlb_detect(void);
-extern void __init pci_xen_swiotlb_init(void);
 extern int pci_xen_swiotlb_init_late(void);
 #else
-#define xen_swiotlb (0)
-static inline int __init pci_xen_swiotlb_detect(void) { return 0; }
-static inline void __init pci_xen_swiotlb_init(void) { }
+#define pci_xen_swiotlb_detect NULL
 static inline int pci_xen_swiotlb_init_late(void) { return -ENXIO; }
 #endif
 
--- a/arch/x86/xen/pci-swiotlb-xen.c
+++ b/arch/x86/xen/pci-swiotlb-xen.c
@@ -18,7 +18,7 @@
 #endif
 #include <linux/export.h>
 
-int xen_swiotlb __read_mostly;
+static int xen_swiotlb __read_mostly;
 
 /*
  * pci_xen_swiotlb_detect - set xen_swiotlb to 1 if necessary
@@ -56,7 +56,7 @@ int __init pci_xen_swiotlb_detect(void)
 	return xen_swiotlb;
 }
 
-void __init pci_xen_swiotlb_init(void)
+static void __init pci_xen_swiotlb_init(void)
 {
 	if (xen_swiotlb) {
 		xen_swiotlb_init_early();


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 12/12] swiotlb-xen: this is PV-only on x86
  2021-09-07 12:03 [PATCH 00/12] swiotlb-xen: fixes and adjustments Jan Beulich
                   ` (9 preceding siblings ...)
  2021-09-07 12:11 ` [PATCH 11/12] xen/pci-swiotlb: reduce visibility of symbols Jan Beulich
@ 2021-09-07 12:13 ` Jan Beulich
  2021-09-08  7:14   ` Christoph Hellwig
  2021-09-08 15:18 ` [PATCH 00/12] swiotlb-xen: fixes and adjustments Juergen Gross
  2021-09-14  8:30 ` Juergen Gross
  12 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2021-09-07 12:13 UTC (permalink / raw)
  To: Juergen Gross, Boris Ostrovsky
  Cc: Stefano Stabellini, lkml, xen-devel, the arch/x86 maintainers,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov

The code is unreachable for HVM or PVH, and it also makes little sense
in auto-translated environments. On Arm, with
xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
time seeing what good the Xen specific variant does - the generic one
ought to be fine for all purposes there. Still Arm code explicitly
references symbols here, so the code will continue to be included there.

Instead of making PCI_XEN's "select" conditional, simply drop it -
SWIOTLB_XEN will be available unconditionally in the PV case anyway, and
is - as explained above - dead code in non-PV environments.

This in turn allows dropping the stubs for
xen_{create,destroy}_contiguous_region(), the former of which was broken
anyway - it failed to set the DMA handle output.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2605,7 +2605,6 @@ config PCI_OLPC
 config PCI_XEN
 	def_bool y
 	depends on PCI && XEN
-	select SWIOTLB_XEN
 
 config MMCONF_FAM10H
 	def_bool y
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -177,6 +177,7 @@ config XEN_GRANT_DMA_ALLOC
 
 config SWIOTLB_XEN
 	def_bool y
+	depends on XEN_PV || ARM || ARM64
 	select DMA_OPS
 	select SWIOTLB
 
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -46,19 +46,7 @@ extern unsigned long *xen_contiguous_bit
 int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
 				unsigned int address_bits,
 				dma_addr_t *dma_handle);
-
 void xen_destroy_contiguous_region(phys_addr_t pstart, unsigned int order);
-#else
-static inline int xen_create_contiguous_region(phys_addr_t pstart,
-					       unsigned int order,
-					       unsigned int address_bits,
-					       dma_addr_t *dma_handle)
-{
-	return 0;
-}
-
-static inline void xen_destroy_contiguous_region(phys_addr_t pstart,
-						 unsigned int order) { }
 #endif
 
 #if defined(CONFIG_XEN_PV)


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 01/12] swiotlb-xen: avoid double free
  2021-09-07 12:04 ` [PATCH 01/12] swiotlb-xen: avoid double free Jan Beulich
@ 2021-09-08  6:48   ` Christoph Hellwig
  2021-09-10 22:55   ` Stefano Stabellini
  1 sibling, 0 replies; 45+ messages in thread
From: Christoph Hellwig @ 2021-09-08  6:48 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, Sep 07, 2021 at 02:04:25PM +0200, Jan Beulich wrote:
> 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 <jbeulich@suse.com>
> Cc: stable@vger.kernel.org

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 02/12] swiotlb-xen: fix late init retry
  2021-09-07 12:04 ` [PATCH 02/12] swiotlb-xen: fix late init retry Jan Beulich
@ 2021-09-08  6:50   ` Christoph Hellwig
  0 siblings, 0 replies; 45+ messages in thread
From: Christoph Hellwig @ 2021-09-08  6:50 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, Sep 07, 2021 at 02:04:47PM +0200, Jan Beulich wrote:
> 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 <jbeulich@suse.com>
> Cc: stable@vger.kernel.org

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 03/12] swiotlb-xen: maintain slab count properly
  2021-09-07 12:05 ` [PATCH 03/12] swiotlb-xen: maintain slab count properly Jan Beulich
@ 2021-09-08  6:53   ` Christoph Hellwig
  2021-09-10 23:10   ` Stefano Stabellini
  1 sibling, 0 replies; 45+ messages in thread
From: Christoph Hellwig @ 2021-09-08  6:53 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, Sep 07, 2021 at 02:05:12PM +0200, Jan Beulich wrote:
> 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 <jbeulich@suse.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
  2021-09-07 12:05 ` [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests Jan Beulich
@ 2021-09-08  6:57   ` Christoph Hellwig
  2021-09-08  9:16     ` Jan Beulich
  2021-09-10 23:14   ` Stefano Stabellini
  1 sibling, 1 reply; 45+ messages in thread
From: Christoph Hellwig @ 2021-09-08  6:57 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, Sep 07, 2021 at 02:05:32PM +0200, Jan Beulich wrote:
> While the hypervisor hasn't been enforcing this, we would still better
> avoid issuing requests with GFNs not aligned to the requested order.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I wonder how useful it is to include the alignment in the panic()
> message. I further wonder how useful it is to wrap "bytes" in
> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> properly").
> 
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -231,10 +231,10 @@ retry:
>  	/*
>  	 * Get IO TLB memory from any location.
>  	 */
> -	start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE);
> +	start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
>  	if (!start)
> -		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> -		      __func__, PAGE_ALIGN(bytes), PAGE_SIZE);
> +		panic("%s: Failed to allocate %lu bytes align=%#x\n",
> +		      __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);

CAn you avoid the overly long lines here?  A good way to make it more
readable would be a variable to hold the byte count.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 05/12] swiotlb-xen: suppress certain init retries
  2021-09-07 12:05 ` [PATCH 05/12] swiotlb-xen: suppress certain init retries Jan Beulich
@ 2021-09-08  6:58   ` Christoph Hellwig
  2021-09-10 23:18   ` Stefano Stabellini
  1 sibling, 0 replies; 45+ messages in thread
From: Christoph Hellwig @ 2021-09-08  6:58 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, Sep 07, 2021 at 02:05:54PM +0200, Jan Beulich wrote:
> 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 <jbeulich@suse.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 06/12] swiotlb-xen: limit init retries
  2021-09-07 12:06 ` [PATCH 06/12] swiotlb-xen: limit " Jan Beulich
@ 2021-09-08  7:00   ` Christoph Hellwig
  2021-09-10 23:23   ` Stefano Stabellini
  1 sibling, 0 replies; 45+ messages in thread
From: Christoph Hellwig @ 2021-09-08  7:00 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, Sep 07, 2021 at 02:06:37PM +0200, Jan Beulich wrote:
> 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 <jbeulich@suse.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 07/12] swiotlb-xen: drop leftover __ref
  2021-09-07 12:06 ` [PATCH 07/12] swiotlb-xen: drop leftover __ref Jan Beulich
@ 2021-09-08  7:02   ` Christoph Hellwig
  2021-09-10 23:24   ` Stefano Stabellini
  1 sibling, 0 replies; 45+ messages in thread
From: Christoph Hellwig @ 2021-09-08  7:02 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, Sep 07, 2021 at 02:06:55PM +0200, Jan Beulich wrote:
> 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 <jbeulich@suse.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 08/12] swiotlb-xen: arrange to have buffer info logged
  2021-09-07 12:07 ` [PATCH 08/12] swiotlb-xen: arrange to have buffer info logged Jan Beulich
@ 2021-09-08  7:04   ` Christoph Hellwig
  2021-09-10 23:26   ` Stefano Stabellini
  1 sibling, 0 replies; 45+ messages in thread
From: Christoph Hellwig @ 2021-09-08  7:04 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, Sep 07, 2021 at 02:07:21PM +0200, Jan Beulich wrote:
> 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 <jbeulich@suse.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 09/12] swiotlb-xen: drop DEFAULT_NSLABS
  2021-09-07 12:07 ` [PATCH 09/12] swiotlb-xen: drop DEFAULT_NSLABS Jan Beulich
@ 2021-09-08  7:06   ` Christoph Hellwig
  2021-09-10 23:27   ` Stefano Stabellini
  1 sibling, 0 replies; 45+ messages in thread
From: Christoph Hellwig @ 2021-09-08  7:06 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, Sep 07, 2021 at 02:07:47PM +0200, Jan Beulich wrote:
> 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 <jbeulich@suse.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 11/12] xen/pci-swiotlb: reduce visibility of symbols
  2021-09-07 12:11 ` [PATCH 11/12] xen/pci-swiotlb: reduce visibility of symbols Jan Beulich
@ 2021-09-08  7:08   ` Christoph Hellwig
  0 siblings, 0 replies; 45+ messages in thread
From: Christoph Hellwig @ 2021-09-08  7:08 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, Sep 07, 2021 at 02:11:14PM +0200, Jan Beulich wrote:
> xen_swiotlb and pci_xen_swiotlb_init() are only used within the file
> defining them, so make them static and remove the stubs. Otoh
> pci_xen_swiotlb_detect() has a use (as function pointer) from the main
> pci-swiotlb.c file - convert its stub to a #define to NULL.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86
  2021-09-07 12:13 ` [PATCH 12/12] swiotlb-xen: this is PV-only on x86 Jan Beulich
@ 2021-09-08  7:14   ` Christoph Hellwig
  2021-09-10 23:48     ` Stefano Stabellini
  0 siblings, 1 reply; 45+ messages in thread
From: Christoph Hellwig @ 2021-09-08  7:14 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml,
	xen-devel, the arch/x86 maintainers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov

On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
> The code is unreachable for HVM or PVH, and it also makes little sense
> in auto-translated environments. On Arm, with
> xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
> time seeing what good the Xen specific variant does - the generic one
> ought to be fine for all purposes there. Still Arm code explicitly
> references symbols here, so the code will continue to be included there.

Can the Xen/arm folks look into that?  Getting ARM out of using
swiotlb-xen would be a huge step forward cleaning up some DMA APIs.

> 
> Instead of making PCI_XEN's "select" conditional, simply drop it -
> SWIOTLB_XEN will be available unconditionally in the PV case anyway, and
> is - as explained above - dead code in non-PV environments.
> 
> This in turn allows dropping the stubs for
> xen_{create,destroy}_contiguous_region(), the former of which was broken
> anyway - it failed to set the DMA handle output.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
  2021-09-08  6:57   ` Christoph Hellwig
@ 2021-09-08  9:16     ` Jan Beulich
  0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2021-09-08  9:16 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On 08.09.2021 08:57, Christoph Hellwig wrote:
> On Tue, Sep 07, 2021 at 02:05:32PM +0200, Jan Beulich wrote:
>> While the hypervisor hasn't been enforcing this, we would still better
>> avoid issuing requests with GFNs not aligned to the requested order.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I wonder how useful it is to include the alignment in the panic()
>> message. I further wonder how useful it is to wrap "bytes" in
>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
>> properly").
>>
>> --- a/drivers/xen/swiotlb-xen.c
>> +++ b/drivers/xen/swiotlb-xen.c
>> @@ -231,10 +231,10 @@ retry:
>>  	/*
>>  	 * Get IO TLB memory from any location.
>>  	 */
>> -	start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE);
>> +	start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
>>  	if (!start)
>> -		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
>> -		      __func__, PAGE_ALIGN(bytes), PAGE_SIZE);
>> +		panic("%s: Failed to allocate %lu bytes align=%#x\n",
>> +		      __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
> 
> CAn you avoid the overly long lines here?  A good way to make it more
> readable would be a variable to hold the byte count.

There already is a variable for the byte count - bytes. Did you read
the post-commit-message remark? _That's_ what I'd prefer to shorten
things. Meanwhile I can of course wrap lines; I will admit that I
failed to pay attention to line length here.

Jan


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 00/12] swiotlb-xen: fixes and adjustments
  2021-09-07 12:03 [PATCH 00/12] swiotlb-xen: fixes and adjustments Jan Beulich
                   ` (10 preceding siblings ...)
  2021-09-07 12:13 ` [PATCH 12/12] swiotlb-xen: this is PV-only on x86 Jan Beulich
@ 2021-09-08 15:18 ` Juergen Gross
  2021-09-14  8:30 ` Juergen Gross
  12 siblings, 0 replies; 45+ messages in thread
From: Juergen Gross @ 2021-09-08 15:18 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, lkml, xen-devel, Boris Ostrovsky,
	Konrad Rzeszutek Wilk, iommu


[-- Attachment #1.1.1: Type: text/plain, Size: 811 bytes --]

On 07.09.21 14:03, Jan Beulich wrote:
> The primary intention really was the last patch, there you go ...
> 
> 01: swiotlb-xen: avoid double free
> 02: swiotlb-xen: fix late init retry
> 03: swiotlb-xen: maintain slab count properly
> 04: swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
> 05: swiotlb-xen: suppress certain init retries
> 06: swiotlb-xen: limit init retries
> 07: swiotlb-xen: drop leftover __ref
> 08: swiotlb-xen: arrange to have buffer info logged
> 09: swiotlb-xen: drop DEFAULT_NSLABS
> 10: xen-pcifront: this module is PV-only
> 11: xen/pci-swiotlb: reduce visibility of symbols
> 12: swiotlb-xen: this is PV-only on x86
> 
> Jan
> 

You should have Cc-ed Konrad being the maintainer and 
iommu@lists.linux-foundation.org. Doing so now.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 01/12] swiotlb-xen: avoid double free
  2021-09-07 12:04 ` [PATCH 01/12] swiotlb-xen: avoid double free Jan Beulich
  2021-09-08  6:48   ` Christoph Hellwig
@ 2021-09-10 22:55   ` Stefano Stabellini
  1 sibling, 0 replies; 45+ messages in thread
From: Stefano Stabellini @ 2021-09-10 22:55 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, 7 Sep 2021, Jan Beulich wrote:
> 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 <jbeulich@suse.com>
> Cc: stable@vger.kernel.org

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


> --- 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;
>  }
>  
> 

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 03/12] swiotlb-xen: maintain slab count properly
  2021-09-07 12:05 ` [PATCH 03/12] swiotlb-xen: maintain slab count properly Jan Beulich
  2021-09-08  6:53   ` Christoph Hellwig
@ 2021-09-10 23:10   ` Stefano Stabellini
  1 sibling, 0 replies; 45+ messages in thread
From: Stefano Stabellini @ 2021-09-10 23:10 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, 7 Sep 2021, Jan Beulich wrote:
> 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 <jbeulich@suse.com>

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


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -106,27 +106,26 @@ static int is_xen_swiotlb_buffer(struct
>  
>  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;
> 

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
  2021-09-07 12:05 ` [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests Jan Beulich
  2021-09-08  6:57   ` Christoph Hellwig
@ 2021-09-10 23:14   ` Stefano Stabellini
  2021-09-13  7:17     ` Jan Beulich
  1 sibling, 1 reply; 45+ messages in thread
From: Stefano Stabellini @ 2021-09-10 23:14 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, 7 Sep 2021, Jan Beulich wrote:
> While the hypervisor hasn't been enforcing this, we would still better
> avoid issuing requests with GFNs not aligned to the requested order.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I wonder how useful it is to include the alignment in the panic()
> message.

Not very useful given that it is static. I don't mind either way but you
can go ahead and remove it if you prefer (and it would make the line
shorter.)


> I further wonder how useful it is to wrap "bytes" in
> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> properly").

This one I would keep, to make sure to print out the same amount passed
to memblock_alloc.


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -231,10 +231,10 @@ retry:
>  	/*
>  	 * Get IO TLB memory from any location.
>  	 */
> -	start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE);
> +	start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
>  	if (!start)
> -		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> -		      __func__, PAGE_ALIGN(bytes), PAGE_SIZE);
> +		panic("%s: Failed to allocate %lu bytes align=%#x\n",
> +		      __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
>  
>  	/*
>  	 * And replace that memory with pages under 4GB.
> 

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 05/12] swiotlb-xen: suppress certain init retries
  2021-09-07 12:05 ` [PATCH 05/12] swiotlb-xen: suppress certain init retries Jan Beulich
  2021-09-08  6:58   ` Christoph Hellwig
@ 2021-09-10 23:18   ` Stefano Stabellini
  1 sibling, 0 replies; 45+ messages in thread
From: Stefano Stabellini @ 2021-09-10 23:18 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, 7 Sep 2021, Jan Beulich wrote:
> 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 <jbeulich@suse.com>

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


> ---
> I'm not convinced of the (lack of) indentation of the label, but I made
> the new one matzch the existing one.
> 
> --- 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;
>  }
> 

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 06/12] swiotlb-xen: limit init retries
  2021-09-07 12:06 ` [PATCH 06/12] swiotlb-xen: limit " Jan Beulich
  2021-09-08  7:00   ` Christoph Hellwig
@ 2021-09-10 23:23   ` Stefano Stabellini
  1 sibling, 0 replies; 45+ messages in thread
From: Stefano Stabellini @ 2021-09-10 23:23 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, 7 Sep 2021, Jan Beulich wrote:
> 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 <jbeulich@suse.com>

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


> --- 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;
> 

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 07/12] swiotlb-xen: drop leftover __ref
  2021-09-07 12:06 ` [PATCH 07/12] swiotlb-xen: drop leftover __ref Jan Beulich
  2021-09-08  7:02   ` Christoph Hellwig
@ 2021-09-10 23:24   ` Stefano Stabellini
  1 sibling, 0 replies; 45+ messages in thread
From: Stefano Stabellini @ 2021-09-10 23:24 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, 7 Sep 2021, Jan Beulich wrote:
> 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 <jbeulich@suse.com>

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


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -154,7 +154,7 @@ static const char *xen_swiotlb_error(enu
>  
>  #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();
> 

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 08/12] swiotlb-xen: arrange to have buffer info logged
  2021-09-07 12:07 ` [PATCH 08/12] swiotlb-xen: arrange to have buffer info logged Jan Beulich
  2021-09-08  7:04   ` Christoph Hellwig
@ 2021-09-10 23:26   ` Stefano Stabellini
  1 sibling, 0 replies; 45+ messages in thread
From: Stefano Stabellini @ 2021-09-10 23:26 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, 7 Sep 2021, Jan Beulich wrote:
> 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 <jbeulich@suse.com>

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


> --- 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);
>  }
> 

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 09/12] swiotlb-xen: drop DEFAULT_NSLABS
  2021-09-07 12:07 ` [PATCH 09/12] swiotlb-xen: drop DEFAULT_NSLABS Jan Beulich
  2021-09-08  7:06   ` Christoph Hellwig
@ 2021-09-10 23:27   ` Stefano Stabellini
  1 sibling, 0 replies; 45+ messages in thread
From: Stefano Stabellini @ 2021-09-10 23:27 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, lkml, xen-devel

On Tue, 7 Sep 2021, Jan Beulich wrote:
> 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 <jbeulich@suse.com>

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


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -152,8 +152,6 @@ static const char *xen_swiotlb_error(enu
>  	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;
> 

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86
  2021-09-08  7:14   ` Christoph Hellwig
@ 2021-09-10 23:48     ` Stefano Stabellini
  2021-09-13  7:28       ` Jan Beulich
  2021-09-13  7:51       ` Jan Beulich
  0 siblings, 2 replies; 45+ messages in thread
From: Stefano Stabellini @ 2021-09-10 23:48 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jan Beulich, Juergen Gross, Boris Ostrovsky, Stefano Stabellini,
	lkml, xen-devel, the arch/x86 maintainers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov

On Wed, 8 Sep 2021, Christoph Hellwig wrote:
> On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
> > The code is unreachable for HVM or PVH, and it also makes little sense
> > in auto-translated environments. On Arm, with
> > xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
> > time seeing what good the Xen specific variant does - the generic one
> > ought to be fine for all purposes there. Still Arm code explicitly
> > references symbols here, so the code will continue to be included there.
> 
> Can the Xen/arm folks look into that?  Getting ARM out of using
> swiotlb-xen would be a huge step forward cleaning up some DMA APIs.

On ARM swiotlb-xen is used for a different purpose compared to x86.

Many ARM SoCs still don't have an IOMMU covering all DMA-mastering
devices (e.g. Raspberry Pi 4). As a consequence we map Dom0 1:1 (guest
physical == physical address).

Now if it was just for Dom0, thanks to the 1:1 mapping, we wouldn't need
swiotlb-xen. But when we start using PV drivers to share the network or
disk between Dom0 and DomU we are going to get DomU pages mapped in
Dom0, we call them "foreign pages".  They are not mapped 1:1. It can
happen that one of these foreign pages are used for DMA operations
(e.g. related to the NIC). swiotlb-xen is used to detect these
situations and translate the guest physical address to physical address
of foreign pages appropriately.

If an IOMMU is available and the DMA-mastering device is behind it, then
swiotlb-xen is not necessary. FYI there is community interest in
selectively disabling swiotlb-xen for devices that are behind an IOMMU.


> > Instead of making PCI_XEN's "select" conditional, simply drop it -
> > SWIOTLB_XEN will be available unconditionally in the PV case anyway, and
> > is - as explained above - dead code in non-PV environments.
> > 
> > This in turn allows dropping the stubs for
> > xen_{create,destroy}_contiguous_region(), the former of which was broken
> > anyway - it failed to set the DMA handle output.
> 
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

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

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
  2021-09-10 23:14   ` Stefano Stabellini
@ 2021-09-13  7:17     ` Jan Beulich
  2021-09-13 20:31       ` Stefano Stabellini
  0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2021-09-13  7:17 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: Juergen Gross, Boris Ostrovsky, lkml, xen-devel

On 11.09.2021 01:14, Stefano Stabellini wrote:
> On Tue, 7 Sep 2021, Jan Beulich wrote:
>> While the hypervisor hasn't been enforcing this, we would still better
>> avoid issuing requests with GFNs not aligned to the requested order.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I wonder how useful it is to include the alignment in the panic()
>> message.
> 
> Not very useful given that it is static. I don't mind either way but you
> can go ahead and remove it if you prefer (and it would make the line
> shorter.)
> 
> 
>> I further wonder how useful it is to wrap "bytes" in
>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
>> properly").
> 
> This one I would keep, to make sure to print out the same amount passed
> to memblock_alloc.

Oh - if I was to drop it from the printk(), I would have been meaning to
also drop it there. If it's useless, then it's useless everywhere.

Jan


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86
  2021-09-10 23:48     ` Stefano Stabellini
@ 2021-09-13  7:28       ` Jan Beulich
  2021-09-13 20:54         ` Stefano Stabellini
  2021-09-13  7:51       ` Jan Beulich
  1 sibling, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2021-09-13  7:28 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Juergen Gross, Boris Ostrovsky, lkml, xen-devel,
	the arch/x86 maintainers, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Christoph Hellwig

On 11.09.2021 01:48, Stefano Stabellini wrote:
> On Wed, 8 Sep 2021, Christoph Hellwig wrote:
>> On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
>>> The code is unreachable for HVM or PVH, and it also makes little sense
>>> in auto-translated environments. On Arm, with
>>> xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
>>> time seeing what good the Xen specific variant does - the generic one
>>> ought to be fine for all purposes there. Still Arm code explicitly
>>> references symbols here, so the code will continue to be included there.
>>
>> Can the Xen/arm folks look into that?  Getting ARM out of using
>> swiotlb-xen would be a huge step forward cleaning up some DMA APIs.
> 
> On ARM swiotlb-xen is used for a different purpose compared to x86.
> 
> Many ARM SoCs still don't have an IOMMU covering all DMA-mastering
> devices (e.g. Raspberry Pi 4). As a consequence we map Dom0 1:1 (guest
> physical == physical address).
> 
> Now if it was just for Dom0, thanks to the 1:1 mapping, we wouldn't need
> swiotlb-xen. But when we start using PV drivers to share the network or
> disk between Dom0 and DomU we are going to get DomU pages mapped in
> Dom0, we call them "foreign pages".  They are not mapped 1:1. It can
> happen that one of these foreign pages are used for DMA operations
> (e.g. related to the NIC). swiotlb-xen is used to detect these
> situations and translate the guest physical address to physical address
> of foreign pages appropriately.

Hmm, you say "translate", which isn't my understanding of swiotlb's
purpose. As per my understanding swiotlb instead double buffers data
such that is becomes accessible, or suitably arranges underlying
machine addresses. The latter part is clearly a PV-only thing, unused
by Arm as can be seen by there not being any use of XENMEM_exchange.
So it must be the former part that you're talking about, but that's
also the purpose of the non-Xen swiotlb code. If only for my own
education and understanding, could you point me at the difference
between swiotlb-xen and generic swiotlb which addresses this specific
aspect of Arm behavior?

Jan


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86
  2021-09-10 23:48     ` Stefano Stabellini
  2021-09-13  7:28       ` Jan Beulich
@ 2021-09-13  7:51       ` Jan Beulich
  2021-09-13 20:49         ` Stefano Stabellini
  1 sibling, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2021-09-13  7:51 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Juergen Gross, Boris Ostrovsky, lkml, xen-devel,
	the arch/x86 maintainers, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Christoph Hellwig

On 11.09.2021 01:48, Stefano Stabellini wrote:
> On Wed, 8 Sep 2021, Christoph Hellwig wrote:
>> On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
>>> The code is unreachable for HVM or PVH, and it also makes little sense
>>> in auto-translated environments. On Arm, with
>>> xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
>>> time seeing what good the Xen specific variant does - the generic one
>>> ought to be fine for all purposes there. Still Arm code explicitly
>>> references symbols here, so the code will continue to be included there.
>>
>> Can the Xen/arm folks look into that?  Getting ARM out of using
>> swiotlb-xen would be a huge step forward cleaning up some DMA APIs.
> 
> On ARM swiotlb-xen is used for a different purpose compared to x86.
> 
> Many ARM SoCs still don't have an IOMMU covering all DMA-mastering
> devices (e.g. Raspberry Pi 4). As a consequence we map Dom0 1:1 (guest
> physical == physical address).
> 
> Now if it was just for Dom0, thanks to the 1:1 mapping, we wouldn't need
> swiotlb-xen. But when we start using PV drivers to share the network or
> disk between Dom0 and DomU we are going to get DomU pages mapped in
> Dom0, we call them "foreign pages".  They are not mapped 1:1. It can
> happen that one of these foreign pages are used for DMA operations
> (e.g. related to the NIC). swiotlb-xen is used to detect these
> situations and translate the guest physical address to physical address
> of foreign pages appropriately.

Thinking about this some more - if Dom0 is 1:1 mapped, why don't you
map foreign pages 1:1 as well then?

>>> Instead of making PCI_XEN's "select" conditional, simply drop it -
>>> SWIOTLB_XEN will be available unconditionally in the PV case anyway, and
>>> is - as explained above - dead code in non-PV environments.
>>>
>>> This in turn allows dropping the stubs for
>>> xen_{create,destroy}_contiguous_region(), the former of which was broken
>>> anyway - it failed to set the DMA handle output.
>>
>> Looks good:
>>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Thanks for this and the other reviews.

Jan


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
  2021-09-13  7:17     ` Jan Beulich
@ 2021-09-13 20:31       ` Stefano Stabellini
  2021-09-14  9:11         ` Jan Beulich
  0 siblings, 1 reply; 45+ messages in thread
From: Stefano Stabellini @ 2021-09-13 20:31 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Juergen Gross, Boris Ostrovsky, lkml, xen-devel

On Mon, 13 Sep 2021, Jan Beulich wrote:
> On 11.09.2021 01:14, Stefano Stabellini wrote:
> > On Tue, 7 Sep 2021, Jan Beulich wrote:
> >> While the hypervisor hasn't been enforcing this, we would still better
> >> avoid issuing requests with GFNs not aligned to the requested order.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> I wonder how useful it is to include the alignment in the panic()
> >> message.
> > 
> > Not very useful given that it is static. I don't mind either way but you
> > can go ahead and remove it if you prefer (and it would make the line
> > shorter.)
> > 
> > 
> >> I further wonder how useful it is to wrap "bytes" in
> >> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> >> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> >> properly").
> > 
> > This one I would keep, to make sure to print out the same amount passed
> > to memblock_alloc.
> 
> Oh - if I was to drop it from the printk(), I would have been meaning to
> also drop it there. If it's useless, then it's useless everywhere.

That's fine too

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86
  2021-09-13  7:51       ` Jan Beulich
@ 2021-09-13 20:49         ` Stefano Stabellini
  0 siblings, 0 replies; 45+ messages in thread
From: Stefano Stabellini @ 2021-09-13 20:49 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Juergen Gross, Boris Ostrovsky, lkml,
	xen-devel, the arch/x86 maintainers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Christoph Hellwig

On Mon, 13 Sep 2021, Jan Beulich wrote:
> On 11.09.2021 01:48, Stefano Stabellini wrote:
> > On Wed, 8 Sep 2021, Christoph Hellwig wrote:
> >> On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
> >>> The code is unreachable for HVM or PVH, and it also makes little sense
> >>> in auto-translated environments. On Arm, with
> >>> xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
> >>> time seeing what good the Xen specific variant does - the generic one
> >>> ought to be fine for all purposes there. Still Arm code explicitly
> >>> references symbols here, so the code will continue to be included there.
> >>
> >> Can the Xen/arm folks look into that?  Getting ARM out of using
> >> swiotlb-xen would be a huge step forward cleaning up some DMA APIs.
> > 
> > On ARM swiotlb-xen is used for a different purpose compared to x86.
> > 
> > Many ARM SoCs still don't have an IOMMU covering all DMA-mastering
> > devices (e.g. Raspberry Pi 4). As a consequence we map Dom0 1:1 (guest
> > physical == physical address).
> > 
> > Now if it was just for Dom0, thanks to the 1:1 mapping, we wouldn't need
> > swiotlb-xen. But when we start using PV drivers to share the network or
> > disk between Dom0 and DomU we are going to get DomU pages mapped in
> > Dom0, we call them "foreign pages".  They are not mapped 1:1. It can
> > happen that one of these foreign pages are used for DMA operations
> > (e.g. related to the NIC). swiotlb-xen is used to detect these
> > situations and translate the guest physical address to physical address
> > of foreign pages appropriately.
> 
> Thinking about this some more - if Dom0 is 1:1 mapped, why don't you
> map foreign pages 1:1 as well then?

That's because the foreign page, from Linux POV, would appear out of
thin air. It would just show up in a region not considered memory just
few moments before, so there would be no memblock, no struct page,
nothing. At least in the past that caused serious issues to the kernel.

This is the reason why the kernel is using ballooned-out pages to map
foreign pages even on x86:

drivers/block/xen-blkback/blkback.c:xen_blkbk_map -> gnttab_page_cache_get
drivers/xen/grant-table.c:gnttab_page_cache_get
drivers/xen/grant-table.c:gnttab_alloc_pages
drivers/xen/unpopulated-alloc.c:xen_alloc_unpopulated_pages
drivers/xen/balloon.c:alloc_xenballooned_pages

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 12/12] swiotlb-xen: this is PV-only on x86
  2021-09-13  7:28       ` Jan Beulich
@ 2021-09-13 20:54         ` Stefano Stabellini
  0 siblings, 0 replies; 45+ messages in thread
From: Stefano Stabellini @ 2021-09-13 20:54 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Juergen Gross, Boris Ostrovsky, lkml,
	xen-devel, the arch/x86 maintainers, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Christoph Hellwig

On Mon, 13 Sep 2021, Jan Beulich wrote:
> On 11.09.2021 01:48, Stefano Stabellini wrote:
> > On Wed, 8 Sep 2021, Christoph Hellwig wrote:
> >> On Tue, Sep 07, 2021 at 02:13:21PM +0200, Jan Beulich wrote:
> >>> The code is unreachable for HVM or PVH, and it also makes little sense
> >>> in auto-translated environments. On Arm, with
> >>> xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
> >>> time seeing what good the Xen specific variant does - the generic one
> >>> ought to be fine for all purposes there. Still Arm code explicitly
> >>> references symbols here, so the code will continue to be included there.
> >>
> >> Can the Xen/arm folks look into that?  Getting ARM out of using
> >> swiotlb-xen would be a huge step forward cleaning up some DMA APIs.
> > 
> > On ARM swiotlb-xen is used for a different purpose compared to x86.
> > 
> > Many ARM SoCs still don't have an IOMMU covering all DMA-mastering
> > devices (e.g. Raspberry Pi 4). As a consequence we map Dom0 1:1 (guest
> > physical == physical address).
> > 
> > Now if it was just for Dom0, thanks to the 1:1 mapping, we wouldn't need
> > swiotlb-xen. But when we start using PV drivers to share the network or
> > disk between Dom0 and DomU we are going to get DomU pages mapped in
> > Dom0, we call them "foreign pages".  They are not mapped 1:1. It can
> > happen that one of these foreign pages are used for DMA operations
> > (e.g. related to the NIC). swiotlb-xen is used to detect these
> > situations and translate the guest physical address to physical address
> > of foreign pages appropriately.
> 
> Hmm, you say "translate", which isn't my understanding of swiotlb's
> purpose. As per my understanding swiotlb instead double buffers data
> such that is becomes accessible, or suitably arranges underlying
> machine addresses. The latter part is clearly a PV-only thing, unused
> by Arm as can be seen by there not being any use of XENMEM_exchange.
> So it must be the former part that you're talking about, but that's
> also the purpose of the non-Xen swiotlb code. If only for my own
> education and understanding, could you point me at the difference
> between swiotlb-xen and generic swiotlb which addresses this specific
> aspect of Arm behavior?

If you look at xen_swiotlb_map_page, you'll see the call to
xen_phys_to_dma which eventually calls arch/arm/xen/p2m.c:__pfn_to_mfn.
If everything goes well and we only need to do translation we'll "goto
done". Otherwise, we'll fall back on a swiotlb buffer with
swiotlb_tbl_map_single, the result of which also needs to be translated,
see the second call to xen_phys_to_dma.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 00/12] swiotlb-xen: fixes and adjustments
  2021-09-07 12:03 [PATCH 00/12] swiotlb-xen: fixes and adjustments Jan Beulich
                   ` (11 preceding siblings ...)
  2021-09-08 15:18 ` [PATCH 00/12] swiotlb-xen: fixes and adjustments Juergen Gross
@ 2021-09-14  8:30 ` Juergen Gross
  12 siblings, 0 replies; 45+ messages in thread
From: Juergen Gross @ 2021-09-14  8:30 UTC (permalink / raw)
  To: Jan Beulich, Boris Ostrovsky
  Cc: Stefano Stabellini, lkml, xen-devel, Konrad Rzeszutek Wilk


[-- Attachment #1.1.1: Type: text/plain, Size: 749 bytes --]

On 07.09.21 14:03, Jan Beulich wrote:
> The primary intention really was the last patch, there you go ...
> 
> 01: swiotlb-xen: avoid double free
> 02: swiotlb-xen: fix late init retry
> 03: swiotlb-xen: maintain slab count properly
> 04: swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
> 05: swiotlb-xen: suppress certain init retries
> 06: swiotlb-xen: limit init retries
> 07: swiotlb-xen: drop leftover __ref
> 08: swiotlb-xen: arrange to have buffer info logged
> 09: swiotlb-xen: drop DEFAULT_NSLABS
> 10: xen-pcifront: this module is PV-only
> 11: xen/pci-swiotlb: reduce visibility of symbols
> 12: swiotlb-xen: this is PV-only on x86

Pushed patches 1-3 and 5-9 to xen/tip.git for-linus-5.15


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
  2021-09-13 20:31       ` Stefano Stabellini
@ 2021-09-14  9:11         ` Jan Beulich
  2021-09-15  1:22           ` Stefano Stabellini
  0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2021-09-14  9:11 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: Juergen Gross, Boris Ostrovsky, lkml, xen-devel

On 13.09.2021 22:31, Stefano Stabellini wrote:
> On Mon, 13 Sep 2021, Jan Beulich wrote:
>> On 11.09.2021 01:14, Stefano Stabellini wrote:
>>> On Tue, 7 Sep 2021, Jan Beulich wrote:
>>>> While the hypervisor hasn't been enforcing this, we would still better
>>>> avoid issuing requests with GFNs not aligned to the requested order.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> I wonder how useful it is to include the alignment in the panic()
>>>> message.
>>>
>>> Not very useful given that it is static. I don't mind either way but you
>>> can go ahead and remove it if you prefer (and it would make the line
>>> shorter.)
>>>
>>>
>>>> I further wonder how useful it is to wrap "bytes" in
>>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
>>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
>>>> properly").
>>>
>>> This one I would keep, to make sure to print out the same amount passed
>>> to memblock_alloc.
>>
>> Oh - if I was to drop it from the printk(), I would have been meaning to
>> also drop it there. If it's useless, then it's useless everywhere.
> 
> That's fine too

Thanks, I'll see about dropping that then.

Another Arm-related question has occurred to me: Do you actually
mind the higher-than-necessary alignment there? If so, a per-arch
definition of the needed alignment would need introducing. Maybe
that could default to PAGE_SIZE, allowing Arm and alike to get away
without explicitly specifying a value ...

Jan


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
  2021-09-14  9:11         ` Jan Beulich
@ 2021-09-15  1:22           ` Stefano Stabellini
  2021-09-15  1:54             ` Stefano Stabellini
  0 siblings, 1 reply; 45+ messages in thread
From: Stefano Stabellini @ 2021-09-15  1:22 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Juergen Gross, Boris Ostrovsky, lkml, xen-devel

On Tue, 14 Sep 2021, Jan Beulich wrote:
> On 13.09.2021 22:31, Stefano Stabellini wrote:
> > On Mon, 13 Sep 2021, Jan Beulich wrote:
> >> On 11.09.2021 01:14, Stefano Stabellini wrote:
> >>> On Tue, 7 Sep 2021, Jan Beulich wrote:
> >>>> While the hypervisor hasn't been enforcing this, we would still better
> >>>> avoid issuing requests with GFNs not aligned to the requested order.
> >>>>
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>> ---
> >>>> I wonder how useful it is to include the alignment in the panic()
> >>>> message.
> >>>
> >>> Not very useful given that it is static. I don't mind either way but you
> >>> can go ahead and remove it if you prefer (and it would make the line
> >>> shorter.)
> >>>
> >>>
> >>>> I further wonder how useful it is to wrap "bytes" in
> >>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> >>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> >>>> properly").
> >>>
> >>> This one I would keep, to make sure to print out the same amount passed
> >>> to memblock_alloc.
> >>
> >> Oh - if I was to drop it from the printk(), I would have been meaning to
> >> also drop it there. If it's useless, then it's useless everywhere.
> > 
> > That's fine too
> 
> Thanks, I'll see about dropping that then.
> 
> Another Arm-related question has occurred to me: Do you actually
> mind the higher-than-necessary alignment there? If so, a per-arch
> definition of the needed alignment would need introducing. Maybe
> that could default to PAGE_SIZE, allowing Arm and alike to get away
> without explicitly specifying a value ...

Certainly a patch like that could be good. Given that it is only one
allocation I was assuming that the higher-than-necessary alignment
wouldn't be a problem worth addressing (and I cannot completely rule out
that one day we might have to use XENMEM_exchange on ARM too).

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
  2021-09-15  1:22           ` Stefano Stabellini
@ 2021-09-15  1:54             ` Stefano Stabellini
  2021-09-15  8:21               ` Jan Beulich
  0 siblings, 1 reply; 45+ messages in thread
From: Stefano Stabellini @ 2021-09-15  1:54 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Jan Beulich, Juergen Gross, Boris Ostrovsky, lkml, xen-devel

On Tue, 14 Sep 2021, Stefano Stabellini wrote:
> On Tue, 14 Sep 2021, Jan Beulich wrote:
> > On 13.09.2021 22:31, Stefano Stabellini wrote:
> > > On Mon, 13 Sep 2021, Jan Beulich wrote:
> > >> On 11.09.2021 01:14, Stefano Stabellini wrote:
> > >>> On Tue, 7 Sep 2021, Jan Beulich wrote:
> > >>>> While the hypervisor hasn't been enforcing this, we would still better
> > >>>> avoid issuing requests with GFNs not aligned to the requested order.
> > >>>>
> > >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > >>>> ---
> > >>>> I wonder how useful it is to include the alignment in the panic()
> > >>>> message.
> > >>>
> > >>> Not very useful given that it is static. I don't mind either way but you
> > >>> can go ahead and remove it if you prefer (and it would make the line
> > >>> shorter.)
> > >>>
> > >>>
> > >>>> I further wonder how useful it is to wrap "bytes" in
> > >>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> > >>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> > >>>> properly").
> > >>>
> > >>> This one I would keep, to make sure to print out the same amount passed
> > >>> to memblock_alloc.
> > >>
> > >> Oh - if I was to drop it from the printk(), I would have been meaning to
> > >> also drop it there. If it's useless, then it's useless everywhere.
> > > 
> > > That's fine too
> > 
> > Thanks, I'll see about dropping that then.
> > 
> > Another Arm-related question has occurred to me: Do you actually
> > mind the higher-than-necessary alignment there? If so, a per-arch
> > definition of the needed alignment would need introducing. Maybe
> > that could default to PAGE_SIZE, allowing Arm and alike to get away
> > without explicitly specifying a value ...
> 
> Certainly a patch like that could be good. Given that it is only one
> allocation I was assuming that the higher-than-necessary alignment
> wouldn't be a problem worth addressing (and I cannot completely rule out
> that one day we might have to use XENMEM_exchange on ARM too).

Also this code is currently #ifdef CONFIG_X86

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
  2021-09-15  1:54             ` Stefano Stabellini
@ 2021-09-15  8:21               ` Jan Beulich
  0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2021-09-15  8:21 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: Juergen Gross, Boris Ostrovsky, lkml, xen-devel

On 15.09.2021 03:54, Stefano Stabellini wrote:
> On Tue, 14 Sep 2021, Stefano Stabellini wrote:
>> On Tue, 14 Sep 2021, Jan Beulich wrote:
>>> On 13.09.2021 22:31, Stefano Stabellini wrote:
>>>> On Mon, 13 Sep 2021, Jan Beulich wrote:
>>>>> On 11.09.2021 01:14, Stefano Stabellini wrote:
>>>>>> On Tue, 7 Sep 2021, Jan Beulich wrote:
>>>>>>> While the hypervisor hasn't been enforcing this, we would still better
>>>>>>> avoid issuing requests with GFNs not aligned to the requested order.
>>>>>>>
>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>> ---
>>>>>>> I wonder how useful it is to include the alignment in the panic()
>>>>>>> message.
>>>>>>
>>>>>> Not very useful given that it is static. I don't mind either way but you
>>>>>> can go ahead and remove it if you prefer (and it would make the line
>>>>>> shorter.)
>>>>>>
>>>>>>
>>>>>>> I further wonder how useful it is to wrap "bytes" in
>>>>>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
>>>>>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
>>>>>>> properly").
>>>>>>
>>>>>> This one I would keep, to make sure to print out the same amount passed
>>>>>> to memblock_alloc.
>>>>>
>>>>> Oh - if I was to drop it from the printk(), I would have been meaning to
>>>>> also drop it there. If it's useless, then it's useless everywhere.
>>>>
>>>> That's fine too
>>>
>>> Thanks, I'll see about dropping that then.
>>>
>>> Another Arm-related question has occurred to me: Do you actually
>>> mind the higher-than-necessary alignment there? If so, a per-arch
>>> definition of the needed alignment would need introducing. Maybe
>>> that could default to PAGE_SIZE, allowing Arm and alike to get away
>>> without explicitly specifying a value ...
>>
>> Certainly a patch like that could be good. Given that it is only one
>> allocation I was assuming that the higher-than-necessary alignment
>> wouldn't be a problem worth addressing (and I cannot completely rule out
>> that one day we might have to use XENMEM_exchange on ARM too).
> 
> Also this code is currently #ifdef CONFIG_X86

Oh, good point. When writing the patch I did take this into consideration,
but I had managed to forget that aspect in the meantime. No adjustment to
this effect needed then.

Jan


^ permalink raw reply	[flat|nested] 45+ messages in thread

end of thread, other threads:[~2021-09-15  8:22 UTC | newest]

Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-07 12:03 [PATCH 00/12] swiotlb-xen: fixes and adjustments Jan Beulich
2021-09-07 12:04 ` [PATCH 01/12] swiotlb-xen: avoid double free Jan Beulich
2021-09-08  6:48   ` Christoph Hellwig
2021-09-10 22:55   ` Stefano Stabellini
2021-09-07 12:04 ` [PATCH 02/12] swiotlb-xen: fix late init retry Jan Beulich
2021-09-08  6:50   ` Christoph Hellwig
2021-09-07 12:05 ` [PATCH 03/12] swiotlb-xen: maintain slab count properly Jan Beulich
2021-09-08  6:53   ` Christoph Hellwig
2021-09-10 23:10   ` Stefano Stabellini
2021-09-07 12:05 ` [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests Jan Beulich
2021-09-08  6:57   ` Christoph Hellwig
2021-09-08  9:16     ` Jan Beulich
2021-09-10 23:14   ` Stefano Stabellini
2021-09-13  7:17     ` Jan Beulich
2021-09-13 20:31       ` Stefano Stabellini
2021-09-14  9:11         ` Jan Beulich
2021-09-15  1:22           ` Stefano Stabellini
2021-09-15  1:54             ` Stefano Stabellini
2021-09-15  8:21               ` Jan Beulich
2021-09-07 12:05 ` [PATCH 05/12] swiotlb-xen: suppress certain init retries Jan Beulich
2021-09-08  6:58   ` Christoph Hellwig
2021-09-10 23:18   ` Stefano Stabellini
2021-09-07 12:06 ` [PATCH 06/12] swiotlb-xen: limit " Jan Beulich
2021-09-08  7:00   ` Christoph Hellwig
2021-09-10 23:23   ` Stefano Stabellini
2021-09-07 12:06 ` [PATCH 07/12] swiotlb-xen: drop leftover __ref Jan Beulich
2021-09-08  7:02   ` Christoph Hellwig
2021-09-10 23:24   ` Stefano Stabellini
2021-09-07 12:07 ` [PATCH 08/12] swiotlb-xen: arrange to have buffer info logged Jan Beulich
2021-09-08  7:04   ` Christoph Hellwig
2021-09-10 23:26   ` Stefano Stabellini
2021-09-07 12:07 ` [PATCH 09/12] swiotlb-xen: drop DEFAULT_NSLABS Jan Beulich
2021-09-08  7:06   ` Christoph Hellwig
2021-09-10 23:27   ` Stefano Stabellini
2021-09-07 12:11 ` [PATCH 11/12] xen/pci-swiotlb: reduce visibility of symbols Jan Beulich
2021-09-08  7:08   ` Christoph Hellwig
2021-09-07 12:13 ` [PATCH 12/12] swiotlb-xen: this is PV-only on x86 Jan Beulich
2021-09-08  7:14   ` Christoph Hellwig
2021-09-10 23:48     ` Stefano Stabellini
2021-09-13  7:28       ` Jan Beulich
2021-09-13 20:54         ` Stefano Stabellini
2021-09-13  7:51       ` Jan Beulich
2021-09-13 20:49         ` Stefano Stabellini
2021-09-08 15:18 ` [PATCH 00/12] swiotlb-xen: fixes and adjustments Juergen Gross
2021-09-14  8:30 ` Juergen Gross

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).